-
Notifications
You must be signed in to change notification settings - Fork 24.9k
Handle is
/is not
#146496
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
Handle is
/is not
#146496
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/146496
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 4b55b2a with merge base fd8ae1a ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
torch/_dynamo/variables/builtin.py
Outdated
# is => left is not right -> False | ||
# is not => left is right -> False | ||
# is not => left is not right -> True | ||
return ConstantVariable(op(left, right)) |
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 am not sure if this is safe today.
If the types don't match, we can safely return a constant.
But if the types match, this change basically relies on comparing two variable trackers. For is_
op, this is equivalent to saying return left is right
. I am concerned if we can create two different variable trackers for the same object, and then this op fails. This can happen for sourceless builder.
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.
For the use case that I have, I only care about exceptions right now. Would you think the code now is safe?
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 a comment.
torch/_dynamo/variables/builtin.py
Outdated
( | ||
(variables.ExceptionVariable, variables.ExceptionVariable), | ||
lambda tx, l, r: ConstantVariable(op(l, r)), | ||
), |
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 think this is safe to do on ExceptionVariable because we do not use SourcelessBuilder on it. Maybe we should just add an assertion into SourcelessBuilder that we don't see one? Thoughts @anijain2305
Starting merge as part of PR stack under #146497 |
Starting merge as part of PR stack under #146497 |
Pull Request resolved: #146497 Approved by: https://github.com/anijain2305, https://github.com/zou3519 ghstack dependencies: #146496
ghstack-source-id: 514ff83 Pull Request resolved: pytorch/pytorch#146496
Pull Request resolved: #146497 Approved by: https://github.com/anijain2305, https://github.com/zou3519 ghstack dependencies: #146496
Pull Request resolved: pytorch#146496 Approved by: https://github.com/anijain2305, https://github.com/zou3519
…46497) Pull Request resolved: pytorch#146497 Approved by: https://github.com/anijain2305, https://github.com/zou3519 ghstack dependencies: pytorch#146496
Stack from ghstack (oldest at bottom):
__context/cause/suppress_context/traceback__
to Exception #146499UserDefinedExceptionClassVariable
#146504AttributeError
to user code in user_defined.py #146497is
/is not
#146496cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames