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
Add a decomposition for _weight_norm_interface. #112193
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/112193
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit fbf7080 with merge base 9bfebf7 (): This comment was automatically generated by Dr. CI and updates every 15 minutes. |
@unittest.skipIf(TEST_WITH_ASAN, "Skipped under ASAN") | ||
@onlyNativeDeviceTypes | ||
@skipIfCrossRef | ||
def test_weight_norm_interface(self, device): | ||
g = torch.randn((3, 10, 10), device=device) | ||
v = torch.randn((1, 1, 10), device=device) | ||
|
||
ref = torch.ops.aten._weight_norm_interface(g, v, 2) | ||
res = torch._decomp.decompositions._weight_norm_interface(g, v, 2) | ||
self.assertTrue(torch.allclose(ref[0], res[0])) | ||
self.assertTrue(torch.allclose(ref[1], res[1])) | ||
|
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.
Rather than a one-off test, please add relevant samples that exercise the _wegith_norm_interface
path.
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.
Hi,
What do you mean by "relevant samples" exactly? Just more random test cases of different shapes or you have specific test cases in mind?
Thanks!
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.
Add samples to WeightNorm's OpInfo so that they exercise this path. No need to add any new test
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.
@qihqi will you do this in a follow up PR?
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.
sure!
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'm going to go ahead and land this for now, but the opinfo would be appreciated in follow up
test failure is legit |
Thanks, taking a look |
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Follow up for pytorch#112193
Follow up for pytorch#112193
Fixes pytorch#112086 Pull Request resolved: pytorch#112193 Approved by: https://github.com/ezyang
Follow up for pytorch#112193
Fixes pytorch#112086 Pull Request resolved: pytorch#112193 Approved by: https://github.com/ezyang
Fixes pytorch#112086 Pull Request resolved: pytorch#112193 Approved by: https://github.com/ezyang
Fixes #112086