-
Notifications
You must be signed in to change notification settings - Fork 26.3k
Allow Tensor-likes in torch.autograd.gradcheck #45732
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
a7d151a to
9679e1a
Compare
7f05c68 to
4162da8
Compare
4162da8 to
fada1d4
Compare
albanD
left a comment
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.
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.
fada1d4 to
2ef0683
Compare
I'd prefer not to ignore a flag when it is set: |
albanD
left a comment
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.
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.
5b828ef to
fac5b0e
Compare
That's a fair point; done! |
albanD
left a comment
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.
LGTM
Just one small comment.
Out of curiosity. Does this work with gradgradcheck as well?
fac5b0e to
00de60a
Compare
00de60a to
1b463ae
Compare
It does, added a test. |
albanD
left a comment
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.
Perfect!
Thanks for the updates.
facebook-github-bot
left a comment
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.
@albanD has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
💊 CI failures summary and remediationsAs of commit c93a341 (more details on the Dr. CI page):
🕵️ 1 new failure recognized by patternsThe following CI failures do not appear to be due to upstream breakages (reran 1 job to discount flakiness):
|
…to gradcheck-override
|
The remaining test failure appears to be a persistent network issue, and unrelated to this PR. |
facebook-github-bot
left a comment
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.
@albanD has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Fixes #42942
Re-do of #43877.