Add __context/cause/suppress_context/traceback__ to Exception
#146499
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/146499
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 5861ae7 with merge base 50c9f6d ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
|
||
| def reconstruct(self, codegen): | ||
| if type(self.__context__) is not ConstantVariable: | ||
| unimplemented("ExceptionVariable with __context__") | ||
| codegen.add_push_null( | ||
| lambda: codegen.load_import_from("builtins", self.exc_type.__name__) | ||
| ) | ||
| codegen.foreach(self.args) |
There was a problem hiding this comment.
What about __cause__? If you set __cause__, you need to reconstruct that as well.
There was a problem hiding this comment.
From the test, it seems that setting __cause__ is reflected. But, I dont understand how.
There was a problem hiding this comment.
Changed the code to reconstruct these attributes.
There was a problem hiding this comment.
So there was a test that was passing without reconstruction cause. Do you know why it was happening? Just confused
There was a problem hiding this comment.
Yes. __cause__ by default is set to None. The only way cause is set to a value different than None is with when there's a raise ... from exc but dynamo only supports the raise ... from None form.
|
Looks good to me. Just have a question on |
| # See test_exceptions::test_raise_does_not_create_context_chain_cycle | ||
| # Based on https://github.com/python/cpython/blob/e635bf2e49797ecb976ce45a67fce2201a25ca68/Python/errors.c#L207-L228 | ||
| # As noted on CPython, this is O(chain length) but the context chains | ||
| # are usually very small |
There was a problem hiding this comment.
Er what is going on here? Why is this needed?
There was a problem hiding this comment.
When setting the context attribute of an exception, there's a chance this creates a reference cycle. This piece of code is to detect and remove such cases.
There was a problem hiding this comment.
how exactly are you removing a reference cycle?
There was a problem hiding this comment.
We don't set the context if it creates a cycle. There are a couple of tests that checks for this behavior. The algorithm I implemented is a straight translation of what CPython do in this case.
There was a problem hiding this comment.
I didn't quite answer how we detect cycles. But it works as follows:
(i) Set the __context__ attribute
(ii) Detect if there's a cycle by using floyd's algorithm (ref) - One pointer moves twice as fast as the second pointer. If there's a cycle, they eventually will reach the same element.
(iii) If yes, remove the context.
|
Starting merge as part of PR stack under #146502 |
|
Starting merge as part of PR stack under #146502 |
Pull Request resolved: #146502 Approved by: https://github.com/anijain2305, https://github.com/williamwen42, https://github.com/zou3519 ghstack dependencies: #146504, #146499
Stack from ghstack (oldest at bottom):
contextlib.suppress#147990raise ... from ...#148766StopIteration#148765__context/cause/suppress_context/traceback__to Exception #146499UserDefinedExceptionClassVariable#146504cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames