add CALL_FINALLY opcode #116159
add CALL_FINALLY opcode #116159
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/116159
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit ef915bd with merge base 4c6e842 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 aakhundov kadeng [ghstack-poisoned]
|
|
||
| result = torch.compile(fn, backend="aot_eager")() | ||
| self.assertEqual(result, 30) | ||
|
|
There was a problem hiding this comment.
What happened before this when we did try-finally?
There was a problem hiding this comment.
It would raise an exception as the CALL_FINALLY opcode wasn't implemented. As an example, here is the bytecode generated on Python 3.8 and Python 3.10 for this testcase: https://godbolt.org/z/PW6oe81Y6
There was a problem hiding this comment.
Please pass in a tensor and add a tensor operation inside the function to ensure dynamo doesn't skip this frame.
|
I'm not qualified to review this PR :P, cc'ing some folks who are |
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 aakhundov kadeng [ghstack-poisoned]
| if preserve_tos: | ||
| tos = self.pop() | ||
| assert self.pop() is None | ||
| _ = self.pop() |
There was a problem hiding this comment.
Pls don't remove the assertion, do sth like:
if sys.version_info == (3, 8):
_ = self.pop()
else:
assert self.pop() is None
There was a problem hiding this comment.
This opcode only exists in Python 3.8 (ref). Thus, I think it is safe to remove the assertion.
There was a problem hiding this comment.
Have something with a side-effect in an assert statement is bad style anyway (because those assert statements can be ommitted entirely if you pass the right args to Python).
|
|
||
| result = torch.compile(fn, backend="aot_eager")() | ||
| self.assertEqual(result, 30) | ||
|
|
There was a problem hiding this comment.
Please pass in a tensor and add a tensor operation inside the function to ensure dynamo doesn't skip this frame.
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 aakhundov kadeng [ghstack-poisoned]
|
Thanks for the review, @yanboliang |
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 aakhundov kadeng [ghstack-poisoned]
|
@pytorchbot merge |
Merge failedReason: This PR needs a If not, please add the To add a label, you can comment to pytorchbot, for example For more information, see Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
6 participants
Stack from ghstack (oldest at bottom):
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @aakhundov @kadeng