Skip to content

Fix unreachable validation for gradcheck#37915

Closed
takp wants to merge 3 commits intopytorch:masterfrom
takp:feature/Fix-unreachable-code
Closed

Fix unreachable validation for gradcheck#37915
takp wants to merge 3 commits intopytorch:masterfrom
takp:feature/Fix-unreachable-code

Conversation

@takp
Copy link
Copy Markdown
Contributor

@takp takp commented May 6, 2020

Hi, I found the validation that is unreachable in gradcheck function :)

@takp takp requested review from albanD and apaszke as code owners May 6, 2020 06:16
@dr-ci
Copy link
Copy Markdown

dr-ci Bot commented May 6, 2020 •
edited
Loading

💊 CI failures summary and remediations

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

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

ci.pytorch.org: 1 failed

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.

See how this bot performed.

This comment has been revised 23 times.

@albanD
Copy link
Copy Markdown
Collaborator

albanD commented May 6, 2020

Hi,

Thanks for catching this.
Your change seems to be braking many tests though.

@takp
Copy link
Copy Markdown
Contributor Author

takp commented May 7, 2020

That's true... I'll try to update the tests.

@takp
Copy link
Copy Markdown
Contributor Author

takp commented May 9, 2020

@albanD Would you please give me an advise?

This test_where_functional executes gradcheck, but the gradcheck requires the all input should have requires_grad=True. On the other hand, cond (one of the input) can't have requires_grad=True because cond is boolean matrix, not a floating point dtype.

I think we should choose one of the following:

  • Stop testing gradcheck for the tests that are not incompatible with its validation of requires_grad=True
  • Stop raising exception for this validation? (if some_input_not_requiring_grad)

@albanD
Copy link
Copy Markdown
Collaborator

albanD commented May 11, 2020

The current gradcheck code seems to handle the case where some inputs do not require gradients.
So just removing the check sounds ok to me.

@ailzhang ailzhang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 12, 2020
albanD
albanD approved these changes May 12, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Thanks !

The complex errors are most likely because you rebased on master when it was broken this morning :/
could you rebase again please to make sure all is good?

@takp takp force-pushed the feature/Fix-unreachable-code branch from 486748c to b01475b Compare May 13, 2020 10:29
@takp
Copy link
Copy Markdown
Contributor Author

takp commented May 13, 2020

I merged the current master branch, but it fails with 1 test... 🤔

@albanD
Copy link
Copy Markdown
Collaborator

albanD commented May 13, 2020 •
edited
Loading

No worries, this one is a known issue ;)

Thanks for the PR !

facebook-github-bot
facebook-github-bot reviewed May 13, 2020
View reviewed changes
Copy link
Copy Markdown
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 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented May 14, 2020

@albanD merged this pull request in 628e3b6.

laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
@takp @facebook-github-bot
Fix unreachable validation for gradcheck (pytorch#37915)
eeb7a41 
Summary:
Hi, I found the validation that is unreachable in `gradcheck` function :)
Pull Request resolved: pytorch#37915

Differential Revision: D21551661

Pulled By: albanD

fbshipit-source-id: 8acadcc09cd2afb539061eda0ca5e98860e321eb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@albanD albanD albanD approved these changes

@apaszke apaszke Awaiting requested review from apaszke

+1 more reviewer

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Merged open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@takp @albanD @facebook-github-bot @ailzhang @pytorchbot @mruberry