-
Notifications
You must be signed in to change notification settings - Fork 603
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
re-add support for tf.function with abstract shape #5089
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5089 +/- ##
==========================================
- Coverage 99.68% 99.68% -0.01%
==========================================
Files 392 392
Lines 35844 35578 -266
==========================================
- Hits 35732 35465 -267
- Misses 112 113 +1 ☔ View full report in Codecov by Sentry. |
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.
I recall we made batch checking lazy for tapes. Curious to see how much of a difference this PR actually makes with that change in mind. Do you have any light benchmarks?
not personally, I do believe @albi3ro mentioned the performance improvement so she might have some numbers handy. Either way, this change is now concerned with correctness because I broke something that used to work, so I'd like to restore functionality |
[sc-55335] |
Context:
While working on adding poetry support, this qcut test failed in a familiar way - it tried to check batching with an abstract TensorShape (of None)! Sub-context: it hasn't been run for a very long time because it needs networkx<3 and poetry is making me use that, unlike today's CI runs. I thought I had fixed this in #4911 mostly because tests stopped failing. However, they (eg. the test that I am changing here) stopped failing because we were no longer running the batch-size evaluation. It's a niche corner case (see that PR description for more details), but it used to pass and it now fails on master.
Description of the Change:
Operator._check_batching()
where it skips the check if the operator params are abstract TF tensors with a shape of None in its signature within atf.function
MyRX
from the test because it only made sense when I added it (in other words, before I re-definedOperator.ndim_params
in the linked PR), but am now callingop.batch_size
to ensure the loading of the propertyBenefits:
Tests that used to pass are passing again, specifically if you use
tf.function
withinput_signature=(tf.TensorSpec(shape=None))
Possible Drawbacks:
Additional try-except logic (re-)added to
Operator._check_batching()
which is called frequently