-
Notifications
You must be signed in to change notification settings - Fork 26.3k
Print raising warnings in Python rather than C++ if other error occurs #41116
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
When we return to Python from C++ in PyTorch and have warnings and and error, we have the problem of what to do when the warnings throw because we can only throw one error. Previously, if we had an error, we punted all warnings to the C++ warning handler which would write them to stderr (i.e. system fid 2) or pass them on to glog. This has drawbacks if an error happened: - Warnings are not handled through Python even if they don't raise, - warnings are always printed with no way to suppress this, - the printing bypasses sys.stderr, so Python modules wanting to modify this don't work (with the prominent example being Jupyter). This patch does the following instead: - Set the warning using standard Python extension mechanisms, - if Python decides that this warning is an error and we have a PyTorch error, we print the warning through Python and clear the error state (from the warning). This resolves the three drawbacks discussed above, in particular fixes pytorch#37240
💊 CI failures summary and remediationsAs of commit 341cf30 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 24 times. |
|
This needs some more work before it's ready... |
|
@albanD now it's actually good to look at. I'll rebase. |
|
@kostmo In the CI analysis of the first few iterations of the patch, there was an error output from a passing test highlighted instead of the output of the failing tests. I know the build log looks funny and it seems to have confused the matching. |
|
@albanD I'm going to claim that the ROCm build error is unrelated. What do you think? |
albanD
left a comment
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.
Looks good thanks !
The rocm is green on master. So I guess flakyness?
facebook-github-bot
left a comment
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.
@albanD is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
When we return to Python from C++ in PyTorch and have warnings and and error, we have the problem of what to do when the warnings throw because we can only throw one error.
Previously, if we had an error, we punted all warnings to the C++ warning handler which would write them to stderr (i.e. system fid 2) or pass them on to glog.
This has drawbacks if an error happened:
modify this don't work (with the prominent example being Jupyter).
This patch does the following instead:
PyTorch error, we print the warning through Python and clear
the error state (from the warning).
This resolves the three drawbacks discussed above, in particular it fixes #37240 .