Skip to content

Conversation

@hameerabbasi
Copy link
Collaborator

@hameerabbasi hameerabbasi commented Oct 2, 2020

Fixes #42942

Re-do of #43877.

@VitalyFedyunin VitalyFedyunin requested a review from albanD October 5, 2020 23:11
@albanD albanD added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Oct 6, 2020
@hameerabbasi hameerabbasi added the module: numpy Related to numpy support, and also numpy compatibility of our operators label Oct 8, 2020
@hameerabbasi hameerabbasi deleted the gradcheck-override branch October 8, 2020 13:26
@hameerabbasi hameerabbasi restored the gradcheck-override branch October 8, 2020 13:27
@hameerabbasi hameerabbasi reopened this Oct 8, 2020
@hameerabbasi hameerabbasi requested a review from apaszke as a code owner October 8, 2020 13:32
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.

Looks good.
Note that another option can be to leave the current default and just say:

if True, check if undefined output grads are supported and treated as zeros. This flag is ignored for non-``Tensor`` outputs.

@hameerabbasi
Copy link
Collaborator Author

Note that another option can be to leave the current default and just say:

I'd prefer not to ignore a flag when it is set:

>>> import this
...
Explicit is better than implicit.
...

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.

I don't think the doc you mention is accurate. You actually default to True if all inputs are Tensors and False otherwise.
I think there is an issue with that:

  • This will disable this for other functions that pass arguments like (t1, t2, float_eps) and I'm pretty sure we have some of these.
  • We don't have to disable this if any is non-Tensor. We just don't want to use this for non-Tensor objects.

Do you think that modifying the line below to:

outputs_to_check = [[torch._C._functions.UndefinedGrad()(o) for o in _differentiable_outputs(func(*tupled_inputs)) if isinstance(o, torch.Tensor)]]

Would work?

And you can change the doc accordingly to mention that the flag only affect Tensor objects.

@hameerabbasi hameerabbasi force-pushed the gradcheck-override branch 2 times, most recently from 5b828ef to fac5b0e Compare October 8, 2020 15:29
@hameerabbasi
Copy link
Collaborator Author

Do you think that modifying the line below to:

outputs_to_check = [[torch._C._functions.UndefinedGrad()(o) for o in _differentiable_outputs(func(*tupled_inputs)) if isinstance(o, torch.Tensor)]]

Would work?

And you can change the doc accordingly to mention that the flag only affect Tensor objects.

That's a fair point; done!

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.

LGTM
Just one small comment.

Out of curiosity. Does this work with gradgradcheck as well?

@hameerabbasi
Copy link
Collaborator Author

Out of curiosity. Does this work with gradgradcheck as well?

It does, added a test.

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.

Perfect!
Thanks for the updates.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@hameerabbasi hameerabbasi requested a review from anjali411 October 9, 2020 05:35
@hameerabbasi hameerabbasi mentioned this pull request Oct 9, 2020
7 tasks
@dr-ci
Copy link

dr-ci bot commented Oct 9, 2020

💊 CI failures summary and remediations

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


  • 1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages (reran 1 job to discount flakiness):

See CircleCI build pytorch_macos_10_13_py3_build (1/1)

Step: "Update Homebrew" (full log | diagnosis details | 🔁 rerun) <confirmed not flaky by 2 failures>

fatal: Could not read from remote repository.
remote: Total 60 (delta 51), reused 21 (delta 17), pack-reused 0         
Unpacking objects:  96% (58/60) Unpacking objects:  98% (59/60) Unpacking objects: 100% (60/60) Unpacking objects: 100% (60/60), 13.00 KiB | 302.00 KiB/s, done. 
From ssh://github.com/Homebrew/homebrew-cask-versions 
 + f127a2be3...8d1d26f57 master     -> origin/master  (forced update) 
+ git reset --hard origin/master 
HEAD is now at 8d1d26f57 Update microsoft-edge-beta from 86.0.622.36 to 86.0.622.38 (#9741) 
+ for path in '$(find /usr/local/Homebrew -type d -name .git)' 
+ cd /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/.git/.. 
+ git fetch --depth=1 origin 
Connection to github.com closed by remote host.  
fatal: Could not read from remote repository. 
 
Please make sure you have the correct access rights 
and the repository exists. 

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 on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 20 times.

@hameerabbasi hameerabbasi requested a review from albanD October 9, 2020 13:25
@hameerabbasi
Copy link
Collaborator Author

The remaining test failure appears to be a persistent network issue, and unrelated to this PR.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@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 f8b3af2.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@albanD merged this pull request in f8b3af2.

@hameerabbasi hameerabbasi deleted the gradcheck-override branch October 12, 2020 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged module: numpy Related to numpy support, and also numpy compatibility of our operators open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

torch.autograd.gradcheck support for Tensor-like types (__torch_function__).

4 participants