-
Notifications
You must be signed in to change notification settings - Fork 560
Re-enable test case to ensure EmbeddingBag gradient computation works #2289
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
Thanks for making the change. we also disabled couple embeddingBag related test and I think your change might fix those tests. Could you give them a try? Searching for |
e1cd942
to
651459a
Compare
59d1dab
to
76af62c
Compare
'TestNNDeviceTypeXLA': { | ||
'test_embedding_backward', # sparse | ||
'test_embedding_dense_grad', # slow | ||
'test_EmbeddingBag_per_sample_weights_and_new_offsets', # FIXME! UndefinedTensorImpl::_singleton |
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.
Btw there're 3 more embeddingbag tests below, could you confirm they're still failing (are they different issues?) 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.
Yes, they are still failing. Two of them are failing because they test sparse tensor behavior. The other fails due to a different runtime error.
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.
Would you mind updating them with the latest error message? (The singleton issue should be fixed by your PR) 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.
Done. And had a nice surprise of finding an extra test that's passing but missed before
Thanks, ended up enabling one of those tests instead of adding a new one. |
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.
Thanks @gmagogsfm ! Only had one minor comment about keeping our skipped list up to date.
'TestNNDeviceTypeXLA': { | ||
'test_embedding_backward', # sparse | ||
'test_embedding_dense_grad', # slow | ||
'test_EmbeddingBag_per_sample_weights_and_new_offsets', # FIXME! UndefinedTensorImpl::_singleton |
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.
Would you mind updating them with the latest error message? (The singleton issue should be fixed by your PR) Thanks
Pin removed. |
This PR tests pytorch/pytorch#40557 in pytorch/pytorch.
Closes #2215