Skip to content

Add forward AD test for op info #57701

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

Closed
wants to merge 9 commits into from
Closed

Conversation

albanD
Copy link
Collaborator

@albanD albanD commented May 6, 2021

The new OpInfo flag has the following semantic:

  • If it says that it supports forward AD, we run gradcheck with forward AD to ensure it is correct
  • If it says that it does not support it, we check that the corresponding error is raised

All the added tests take 3s to run for CPU builds and 1min for GPU builds which should be pretty negligible compared to the test_ops runtime for each of these arch.

Stack from ghstack:

Differential Revision: D28387767

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented May 6, 2021

💊 CI failures summary and remediations

As of commit 3a9e11b (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 to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

albanD added a commit that referenced this pull request May 6, 2021
ghstack-source-id: 1a7d5d1
Pull Request resolved: #57701
@albanD albanD requested a review from mruberry May 6, 2021 13:33
The new OpInfo flag has the following semantic:
- If not specified, that means the given function does not have forward AD implemented. And we check that the corresponding error is raised
- If it says that it supports forward AD, we run gradcheck with forward AD to ensure it is correct
- If it says that it does not support it, we skip the test altogether (this happen right now because some formulas are wrong for complex and because the forward AD code make complex input invalid for some linalg functions)





[ghstack-poisoned]
The new OpInfo flag has the following semantic:
- If not specified, that means the given function does not have forward AD implemented. And we check that the corresponding error is raised
- If it says that it supports forward AD, we run gradcheck with forward AD to ensure it is correct
- If it says that it does not support it, we skip the test altogether (this happen right now because some formulas are wrong for complex and because the forward AD code make complex input invalid for some linalg functions)





[ghstack-poisoned]
albanD added a commit that referenced this pull request May 6, 2021
ghstack-source-id: 477e7eb
Pull Request resolved: #57701
albanD added 2 commits May 7, 2021 11:23
The new OpInfo flag has the following semantic:
- If not specified, that means the given function does not have forward AD implemented. And we check that the corresponding error is raised
- If it says that it supports forward AD, we run gradcheck with forward AD to ensure it is correct
- If it says that it does not support it, we skip the test altogether (this happen right now because some formulas are wrong for complex and because the forward AD code make complex input invalid for some linalg functions)





[ghstack-poisoned]
The new OpInfo flag has the following semantic:
- If not specified, that means the given function does not have forward AD implemented. And we check that the corresponding error is raised
- If it says that it supports forward AD, we run gradcheck with forward AD to ensure it is correct
- If it says that it does not support it, we skip the test altogether (this happen right now because some formulas are wrong for complex and because the forward AD code make complex input invalid for some linalg functions)





[ghstack-poisoned]
The new OpInfo flag has the following semantic:
- If not specified, that means the given function does not have forward AD implemented. And we check that the corresponding error is raised
- If it says that it supports forward AD, we run gradcheck with forward AD to ensure it is correct
- If it says that it does not support it, we skip the test altogether (this happen right now because some formulas are wrong for complex and because the forward AD code make complex input invalid for some linalg functions)





[ghstack-poisoned]
dgl-intel pushed a commit to dgl-intel/pytorch that referenced this pull request May 7, 2021
ghstack-source-id: d41fe33
Pull Request resolved: pytorch#57701
albanD added a commit that referenced this pull request May 10, 2021
ghstack-source-id: d41fe33
Pull Request resolved: #57701
if op.supports_forward_ad:
self._grad_test_helper(device, dtype, op, op.get_op(), check_forward_ad=True)
else:
err_msg = r"Trying to use forward AD with .* that does not support it\."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Really nice test

@mruberry mruberry self-requested a review May 11, 2021 04:20
Copy link
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

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

This is a brilliant extension of our automated testing

The new OpInfo flag has the following semantic:
- If it says that it supports forward AD, we run gradcheck with forward AD to ensure it is correct
- If it says that it does not support it, we check that the corresponding error is raised

All the added tests take 3s to run for CPU builds and 1min for GPU builds which should be pretty negligible compared to the test_ops runtime for each of these arch.




[ghstack-poisoned]
albanD added a commit to albanD/pytorch that referenced this pull request May 11, 2021
ghstack-source-id: 541f279
Pull Request resolved: pytorch#57701
albanD added 2 commits May 12, 2021 10:09
The new OpInfo flag has the following semantic:
- If it says that it supports forward AD, we run gradcheck with forward AD to ensure it is correct
- If it says that it does not support it, we check that the corresponding error is raised

All the added tests take 3s to run for CPU builds and 1min for GPU builds which should be pretty negligible compared to the test_ops runtime for each of these arch.




[ghstack-poisoned]
The new OpInfo flag has the following semantic:
- If it says that it supports forward AD, we run gradcheck with forward AD to ensure it is correct
- If it says that it does not support it, we check that the corresponding error is raised

All the added tests take 3s to run for CPU builds and 1min for GPU builds which should be pretty negligible compared to the test_ops runtime for each of these arch.




[ghstack-poisoned]
@albanD
Copy link
Collaborator Author

albanD commented May 12, 2021

@albanD has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@albanD merged this pull request in 26b6d04.

@facebook-github-bot
Copy link
Contributor

This pull request has been reverted by f88297c.

@facebook-github-bot facebook-github-bot deleted the gh/albanD/88/head branch May 16, 2021 14:15
krshrimali pushed a commit to krshrimali/pytorch that referenced this pull request May 19, 2021
Summary:
Pull Request resolved: pytorch#57701

The new OpInfo flag has the following semantic:
- If it says that it supports forward AD, we run gradcheck with forward AD to ensure it is correct
- If it says that it does not support it, we check that the corresponding error is raised

All the added tests take 3s to run for CPU builds and 1min for GPU builds which should be pretty negligible compared to the test_ops runtime for each of these arch.

Test Plan: Imported from OSS

Reviewed By: agolynski

Differential Revision: D28387767

Pulled By: albanD

fbshipit-source-id: 369d76921c8460aa4548f9b5159b7297994672f5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants