-
Notifications
You must be signed in to change notification settings - Fork 26.3k
Improve opcheck docs. #134692
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
Improve opcheck docs. #134692
Conversation
Fixes #134119 From user feedback, it's difficult to understand what the tests do. We clarify the docs more. [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/134692
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 8e3864e with merge base 55236d0 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
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.
Small nit but SGTM
torch/library.py
Outdated
| - test_schema: if the operator's schema is correct. | ||
| - test_autograd_registration: if autograd was registered correctly. | ||
| - test_schema: We test that the schema matches the implementation of |
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.
| - test_schema: We test that the schema matches the implementation of | |
| - test_schema: If the schema matches the implementation of |
For consistency with the other entries below :)
| specifies that we return a new Tensor, then we check that the | ||
| implementation returns a new Tensor (instead of an existing one or | ||
| a view of an existing one). | ||
| - test_autograd_registration: If the operator supports training |
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 was almost expecting one of these entries to use gradcheck to test gradients. But I only see the aot_dispatch below checking matching eager vs compile.
Is that expected?
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.
Yes, we don't run gradcheck. opcheck is to check that the "registrations were correct", we recommend the user use gradcheck for their gradients.
Fixes #134119 From user feedback, it's difficult to understand what the tests do. We clarify the docs more. [ghstack-poisoned]
Fixes pytorch/pytorch#134119 From user feedback, it's difficult to understand what the tests do. We clarify the docs more. ghstack-source-id: aad726b Pull Request resolved: pytorch/pytorch#134692
Fixes pytorch#134119 From user feedback, it's difficult to understand what the tests do. We clarify the docs more. Pull Request resolved: pytorch#134692 Approved by: https://github.com/albanD ghstack dependencies: pytorch#134466, pytorch#134490, pytorch#134491, pytorch#134690
Fixes pytorch#134278 Test Plan: - tested locally Pull Request resolved: pytorch#134688 Approved by: https://github.com/yushangdi ghstack dependencies: pytorch#134466, pytorch#134490, pytorch#134491, pytorch#134690, pytorch#134692
Stack from ghstack (oldest at bottom):
Fixes #134119
From user feedback, it's difficult to understand what the tests do. We
clarify the docs more.