Skip to content

Conversation

@zou3519
Copy link
Contributor

@zou3519 zou3519 commented Aug 28, 2024

Fixes #134119
From user feedback, it's difficult to understand what the tests do. We
clarify the docs more.

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Aug 28, 2024

🔗 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 Failures

As of commit 8e3864e with merge base 55236d0 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@zou3519 zou3519 requested a review from albanD August 28, 2024 16:37
Copy link
Collaborator

@albanD albanD left a 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- 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
Copy link
Collaborator

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?

Copy link
Contributor Author

@zou3519 zou3519 Aug 28, 2024

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]
@zou3519 zou3519 added the release notes: composability release notes category label Aug 28, 2024
pytorchmergebot pushed a commit that referenced this pull request Aug 28, 2024
Pavelanln pushed a commit to Pavelanln/Coppy that referenced this pull request Sep 18, 2024
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
Chao1Han pushed a commit to Chao1Han/pytorch that referenced this pull request Sep 20, 2024
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
@github-actions github-actions bot deleted the gh/zou3519/1057/head branch October 2, 2024 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants