-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
[dynamo] graph break on const dict KeyError #125882
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/125882
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 20b4ba9 with merge base 1966612 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
ghstack-source-id: 7104397e77939827b03c15592bba1a946db9c3ba Pull Request resolved: #125882
ghstack-source-id: 8f07c5d01b78be12fa8177a432fcd6c8995084a7 Pull Request resolved: #125882
ghstack-source-id: 0a98e95179b0b809528ecdbb6a39235beea34978 Pull Request resolved: #125882
ghstack-source-id: 9381c55dd2bada26bdb12b5b14bfe7b493af0074 Pull Request resolved: #125882
except Exception as exc: | ||
unimplemented(f"constant fold exception: {repr(exc)}") |
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.
What error is this catching? I worry this will be hiding bugs.
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.
This is catching an error on from a python call - fn
would be a builtin function. Idea is that if we get a python error when trying to constant fold, we bubble up the error to skip the frame, then we evaluate the frame as normal, which would result in the same error. In the case of the repro, we would be catching a KeyError
.
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.
Let's make catch case more narrow, perhaps by checking the message and warning if it is somthing else.
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 that any possible exception raised by a function in _constant_fold_functions
would have to be caught, e.g. KeyError, ZeroDivisionError, AttributeError, ValueError, etc. We could determine a list of allowed exceptions (by going through the builtin constant fold op list and manually checking for exceptions that could be raised), but if we miss any, then tracing any code like below would crash:
def fn(x):
try:
y = ... # builtin constant fold op with constants that raises an exception we missed
# e.g. we forgot ZeroDivisionError for y = 1 / 0
except Exception:
y = 1
return x + y
What kind of bugs do you think would be hidden? My intent here is that if running the builtin op results in an exception, we should skip the frame, then default evaluation will also raise the exception and run any exception handlers.
except Exception as exc: | ||
unimplemented(f"constant fold exception: {repr(exc)}") |
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.
Let's make catch case more narrow, perhaps by checking the message and warning if it is somthing else.
ghstack-source-id: cbd13cd86e9e1003e1ae9cde6b2c676aa77c964f Pull Request resolved: #125882
Fixes #125942 Pull Request resolved: #125943 Approved by: https://github.com/jansel ghstack dependencies: #125882
Fixes pytorch#125866 Pull Request resolved: pytorch#125882 Approved by: https://github.com/jansel
…125943) Fixes pytorch#125942 Pull Request resolved: pytorch#125943 Approved by: https://github.com/jansel ghstack dependencies: pytorch#125882
Fixes pytorch#93624 but also requires jcmgray/autoray#20 to be fixed. Pull Request resolved: pytorch#125945 Approved by: https://github.com/jansel ghstack dependencies: pytorch#125882, pytorch#125943
Stack from ghstack (oldest at bottom):
Fixes #125866
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang