-
Couldn't load subscription status.
- Fork 25.7k
[dynamo, nested graph breaks] remove _dynamo.utils.counter patch on inlined tx'es #166015
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
Conversation
…nlined tx'es [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/166015
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ✅ No FailuresAs of commit 511468b with merge base a988510 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
… patch on inlined tx'es" cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames Lucaskabela [ghstack-poisoned]
| # Hierarchically, tx can be seen as the parent of the inline tracer | ||
| # created on call_function. Any exception needs to be propagated to tx | ||
| # for Dynamo to behave correctly | ||
| with patch.dict(counters, {"unimplemented": counters["inline_call"]}): |
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 we run this with context manager tests? Seems that was the last time this patch was moved so may be worth double checking those tests.
Presuming those are green (and since nothing showing in CI), we can approve
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.
CI is green now.
… patch on inlined tx'es" This `patch.dict(counters, ...` appears to be ancient code that doesn't really seem to be doing anything? It causes issues in nested graph breaks because the patch cleanup clears out the record of the nested graph break. Removing the patch to see if it's even needed in the first place. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames Lucaskabela [ghstack-poisoned]
|
Starting merge as part of PR stack under #166016 |
…graph breaks (#165808) Previously, we would completely skip building and calling any resume function if the leaf frame's resume instruction was RETURN_VALUE/RETURN_CONST. Now, we only skip building/calling resume functions for frames that are resuming on RETURN_VALUE. Pull Request resolved: #165808 Approved by: https://github.com/Lucaskabela ghstack dependencies: #166013, #166015
…rs; enable nested_graph_breaks in test_ctx_manager.py and test_generator.py (#165809) Generators should not support nested graph breaks. Pull Request resolved: #165809 Approved by: https://github.com/Lucaskabela, https://github.com/guilhermeleobas ghstack dependencies: #166013, #166015, #165808
Stack from ghstack (oldest at bottom):
This
patch.dict(counters, ...appears to be ancient code that doesn't really seem to be doing anything? It causes issues in nested graph breaks because the patch cleanup clears out the record of the nested graph break. Removing the patch to see if it's even needed in the first place.cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames @Lucaskabela