-
Notifications
You must be signed in to change notification settings - Fork 330
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduces MaybeApply layer. #435
Introduces MaybeApply layer. #435
Conversation
You could try to trace/profile the code to collect more insight: |
@bhack However, running without XLA: I'm getting a different message for I am not reading any data from the disk, so I'd assume it could be Python's overhead? Not sure. |
Do you have a gist with tf.profile to reproduce this? |
@bhack this is what I'm using: |
Do you have, can you post, Tensorboard screenshots for the ops section for plain vs maybe wrapper? The ops tables are like these: More in general with XLA is also interesting to check how well the graph ops that we have used in the implementaton are optimized-fused or not with the HLO dump (not all the possible ops permutation sequences have the same XLA coverage and optimizzation quality). |
You cannot do like you have done when you don't use a Keras model. If you don't want to use a model check: But as tensorflow/tensorboard#1961 is still open it is better that you still use a model. |
@bhack Instead of running the layer, I should wrap it in a I'm still struggling to create a HLO graph. Will post when I gather more information. |
Yes as the graph is on model build:
|
I feel like we need a page in our docs for performance related artifacts like these! These are great to have. |
@bhack Following your suggestion on using the When using |
@sebastian-sz Can you post the same graph without XLA? As we still don't expose an API to control the XLA compilation x layer in Keras/Keras-cv (see keras-team/keras-io#1541) using just Extra: Without extending Keras-cv layers test for XLA compilation we will never know the exact list of layers with one or more usupported XLA ops. |
P.s. Just to clarify I meant in graph mode as Edit: |
Thanks, yes, my bad. Added option for
I'm not sure I follow - Is the above graph what you requested? |
No It was the previous one labeled as eager but It was in graph mode without jit compile |
with self.assertRaises(ValueError): | ||
MaybeApply(rate=invalid_rate, layer=ZeroOut()) | ||
|
||
def test_works_with_batched_input(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you pass a seed so that this test is not potentially flaky? Given, it is 1/2^32 flakiness, but still may as well seed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added seed to rng
on line 37.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! does this seed the layer too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Added seed
param to layer as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 minor comments then good to go.
Looks good to me @sebastian-sz Thanks for the contribution! |
* Added MaybeApply layer. * Changed MaybeApply to override _augment method. * Added seed to maybe_apply_test random generator. * Added seed to layer in batched input test. * Fixed MaybeApply docs.
* Added MaybeApply layer. * Changed MaybeApply to override _augment method. * Added seed to maybe_apply_test random generator. * Added seed to layer in batched input test. * Fixed MaybeApply docs.
Silly bug, we were literally just adding the trainable field twice
Closes keras-team/keras#422 .
Maybe Apply layer is a wrapper around native Keras layers or
BaseImageAugmentationLayer
that applies, specified operation to random samples in a batch.Example usage
Performance / overhead
The layer introduces an overhead for the layer it wraps. It looks like the layer wrapped in
MaybeApply
takes ~2x the time to execute. I'm not sure if any improvements here can be made - I tried to implement the same behaviour usingtf.gather_nd
+tf.scatter_nd_update
but got similar or even worse results for larger batches.Below are latency (ms) measurements for Solarization and Posterization. Both were benchmarked with XLA to avoid
tf.function
retracing.Known Issues
The layer throws error in XLA with
auto_vectorize=True
, regardless of what layer it wraps. I'm not really sure whybut it works with
auto_vectorize=False
.