Skip to content

Conversation

albanD
Copy link
Collaborator

@albanD albanD commented May 7, 2021

Slow gradcheck also passes for this PR and can be found here: #57976

Stack from ghstack:

Differential Revision: D28387763

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented May 7, 2021

💊 CI failures summary and remediations

As of commit 697868e (more details on the Dr. CI page):


  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-scanned failure(s)

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 7, 2021
ghstack-source-id: 099f688
Pull Request resolved: #57863
albanD added a commit that referenced this pull request May 7, 2021
ghstack-source-id: 4cd3eae
Pull Request resolved: #57863
albanD added a commit that referenced this pull request May 10, 2021
ghstack-source-id: 4cd3eae
Pull Request resolved: #57863
albanD added a commit to albanD/pytorch that referenced this pull request May 11, 2021
ghstack-source-id: 5b32b08
Pull Request resolved: pytorch#57863
@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.

Slow gradcheck also passes for this PR and can be found here: #57976


Differential Revision: [D28387763](https://our.internmc.facebook.com/intern/diff/D28387763)

[ghstack-poisoned]
Slow gradcheck also passes for this PR and can be found here: #57976


Differential Revision: [D28387763](https://our.internmc.facebook.com/intern/diff/D28387763)

[ghstack-poisoned]
Slow gradcheck also passes for this PR and can be found here: #57976


Differential Revision: [D28387763](https://our.internmc.facebook.com/intern/diff/D28387763)

[ghstack-poisoned]
Copy link
Contributor

@soulitzer soulitzer left a comment

Choose a reason for hiding this comment

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

Haven't looked at the actual formulas, but here are some comments on the rest

@@ -5369,9 +5369,10 @@ def gradgradcheck_method_precision_override(test_name):
return override

def run_grad_and_gradgrad_checks(test_case, name, test_name, apply_method, output_variable,
input_variables, run_gradgradcheck=True, check_batched_grad=True):
input_variables, run_gradgradcheck=True, check_batched_grad=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes to test_autograd are for method tests? Maybe you could add a similar check that checks_forward_ad=True would raise an error unless its explicitly set to True?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will induce larger changes because some other tests will need to be updated as well. I'll do that in a follow up PR.

Slow gradcheck also passes for this PR and can be found here: #57976


Differential Revision: [D28387763](https://our.internmc.facebook.com/intern/diff/D28387763)

[ghstack-poisoned]
@albanD
Copy link
Collaborator Author

albanD commented May 24, 2021

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

@@ -137,7 +137,7 @@ def postprocess_forward_derivatives(
def find_required_inputs(formula: str, postfix: str) -> Tuple[str, ...]:
required_inputs = set()
for arg in args_with_derivatives:
if arg.type == 'TensorList':
if arg.type == 'at::TensorList':
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a bugfix? (Should this be arg.type == 'at::TensorList' or arg.type == 'TensorList'?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a bugfix indeed. This code wasn't actually used before and it is now that we have the "cat" formula.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we assert 'TensorList' not in arg.type after this if statement? Would make it easier to catch bugs like this in the future

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so. It was just me not knowing that the namespace is included in this name.
I think there should be methods on the type to check that directly but I couldn't find it.

Copy link
Contributor

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

Formula for cross looks fishy, other than that the formulas lgtm

Slow gradcheck also passes for this PR and can be found here: #57976


Differential Revision: [D28387763](https://our.internmc.facebook.com/intern/diff/D28387763)

[ghstack-poisoned]
Copy link
Contributor

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

Don't forget to revert the changes to the submodules, otherwise this lgtm

Copy link
Contributor

@soulitzer soulitzer left a comment

Choose a reason for hiding this comment

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

Still seems weird to me that the Tensor variant of clamp does not raise an error since we added formulas for both scalar/tensor variants but only added support_forward_ad=True for the scalar variant in OpInfos. Any idea whats up with that?

LGTM otherwise though.

@albanD
Copy link
Collaborator Author

albanD commented Jun 2, 2021

Still seems weird to me that the Tensor variant of clamp does not raise an error since we added formulas for both scalar/tensor variants but only added support_forward_ad=True for the scalar variant in OpInfos. Any idea whats up with that?

We did not add the formula for the Tensor variant actually, only the multiple scalar versions. So this is expected.

Slow gradcheck also passes for this PR and can be found here: #57976


Differential Revision: [D28387763](https://our.internmc.facebook.com/intern/diff/D28387763)

[ghstack-poisoned]
@albanD
Copy link
Collaborator Author

albanD commented Jun 2, 2021

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

albanD added a commit to albanD/pytorch that referenced this pull request Jun 2, 2021
ghstack-source-id: 46ae249
Pull Request resolved: pytorch#57863
@albanD
Copy link
Collaborator Author

albanD commented Jun 2, 2021

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

@soulitzer
Copy link
Contributor

We did not add the formula for the Tensor variant actually, only the multiple scalar versions. So this is expected.

Aren't clamp_max.Tensor and clamp_min.Tensor tensor variants? Or maybe I'm missing something?

@albanD
Copy link
Collaborator Author

albanD commented Jun 3, 2021

They are, but for the clamp_min/max functions. The Tensor variant for clamp is clamp.Tensor which is not getting a forward formula.

@facebook-github-bot
Copy link
Contributor

@albanD merged this pull request in d095ec7.

@facebook-github-bot facebook-github-bot deleted the gh/albanD/91/head branch June 7, 2021 14:17
deniskokarev pushed a commit to deniskokarev/pytorch that referenced this pull request Jun 9, 2021
Summary: Pull Request resolved: pytorch#57863

Test Plan: Imported from OSS

Reviewed By: zou3519

Differential Revision: D28387763

Pulled By: albanD

fbshipit-source-id: e1b60ab728bb05b9e3323ee0dc7e401aaf5b8817
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.

4 participants