Skip to content

Make not passing use_reentrant back to warning instead of erroring and clarify docs #116710

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.

Sign up for GitHub

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

Jump to bottom
Closed
soulitzer wants to merge 9 commits into from
Closed

Make not passing use_reentrant back to warning instead of erroring and clarify docs #116710

soulitzer wants to merge 9 commits into from

Conversation

@soulitzer
Copy link
Contributor

@soulitzer soulitzer commented Jan 3, 2024 •
edited
Loading

@soulitzer soulitzer mentioned this pull request Jan 3, 2024
Closed
@pytorch-bot
Copy link

pytorch-bot bot commented Jan 3, 2024 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/116710

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 529e1e5 with merge base e5f2ac1 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

soulitzer added a commit that referenced this pull request Jan 3, 2024
@soulitzer
Make clearer that you must pass use_reentrant in docs
779b44a 
ghstack-source-id: a782516
Pull Request resolved: #116710
@soulitzer soulitzer requested review from albanD and kit1980 January 3, 2024 19:48
@stas00
Copy link
Contributor

stas00 commented Jan 3, 2024

Thank you for working on providing more clarity around this transition, @soulitzer

This is being discussed at https://pytorch.slack.com/archives/C3PDTEV8E/p1704308146846339

Repasting 2 items from that thread here:

I think the current proposal of this PR is a good start but more is needed:

  1. The discussion of the defaults needs to be updated
  2. The current error message isn't very coherent either:

ValueError: torch.utils.checkpoint: please pass in use_reentrant=True or use_reentrant=False explicitly. The default value of use_reentrant will be updated to be False in the future. [...]

If I try to parse this statement to me it says " The default value will be updated" which implies that there is a default. Which implies it shouldn't be required to pass.

@soulitzer soulitzer changed the title Make clearer that you must pass use_reentrant in docs Make not passing use_reentrant back to warning instead of erroring and clarify docs Jan 5, 2024
@soulitzer
Update on "Make not passing use_reentrant back to warning instead of …
65898d9 
…erroring and clarify docs"

[ghstack-poisoned]
stas00
stas00 reviewed Jan 5, 2024
View reviewed changes
Copy link
Contributor

@stas00 stas00 left a comment

Choose a reason for hiding this comment

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

This version looks excellent, Jeffrey!

I made a few small style consistency tweaks and added a question.

@soulitzer
Update on "Make not passing use_reentrant back to warning instead of …
fc1f56f 
…erroring and clarify docs"

[ghstack-poisoned]
soulitzer added a commit that referenced this pull request Jan 5, 2024
@soulitzer
Make clearer that you must pass use_reentrant in docs
389fe63 
ghstack-source-id: 2b719e4
Pull Request resolved: #116710
@soulitzer soulitzer requested a review from stas00 January 5, 2024 02:52
@soulitzer
Copy link
Contributor Author

soulitzer commented Jan 5, 2024

Updated!

stas00
stas00 reviewed Jan 5, 2024
View reviewed changes
Copy link
Contributor

@stas00 stas00 left a comment

Choose a reason for hiding this comment

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

That's much better with the actual version! it's a call for action

added a few more small nits

@soulitzer
Update on "Make not passing use_reentrant back to warning instead of …
e87c3d1 
…erroring and clarify docs"

[ghstack-poisoned]
albanD
albanD approved these changes Jan 5, 2024
View reviewed changes
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.

Sounds good to me as well!
Thanks @stas00 for helping review this!

@soulitzer
Update on "Make not passing use_reentrant back to warning instead of …
687877d 
…erroring and clarify docs"

[ghstack-poisoned]
@soulitzer
Update on "Make not passing use_reentrant back to warning instead of …
7367ad3 
…erroring and clarify docs"

[ghstack-poisoned]
@soulitzer soulitzer added the release notes: autograd release notes category label Jan 5, 2024
@soulitzer
Copy link
Contributor Author

soulitzer commented Jan 5, 2024

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Jan 5, 2024
@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Jan 5, 2024

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Jan 5, 2024

Merge failed

Reason: 1 mandatory check(s) failed. The first few are:

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

Failing merge rule: Core Maintainers

@soulitzer
Update on "Make not passing use_reentrant back to warning instead of …
c051174 
…erroring and clarify docs"

[ghstack-poisoned]
soulitzer added a commit that referenced this pull request Jan 5, 2024
@soulitzer
Make clearer that you must pass use_reentrant in docs
515b6a4 
ghstack-source-id: 9bc4956
Pull Request resolved: #116710
@soulitzer
Update on "Make not passing use_reentrant back to warning instead of …
529e1e5 
…erroring and clarify docs"

[ghstack-poisoned]
soulitzer added a commit that referenced this pull request Jan 9, 2024
@soulitzer
Make clearer that you must pass use_reentrant in docs
cd4a262 
ghstack-source-id: 38b1790
Pull Request resolved: #116710
@soulitzer
Copy link
Contributor Author

soulitzer commented Jan 9, 2024

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Jan 9, 2024

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@soulitzer soulitzer mentioned this pull request Jan 9, 2024
Closed
xadupre pushed a commit to xadupre/pytorch that referenced this pull request Jan 10, 2024
@soulitzer @xadupre
Make not passing use_reentrant back to warning instead of erroring an…
9699584 
…d clarify docs (pytorch#116710)

Pull Request resolved: pytorch#116710
Approved by: https://github.com/albanD
ghstack dependencies: pytorch#116523
pytorchmergebot pushed a commit that referenced this pull request Jan 10, 2024
@soulitzer @pytorchmergebot
Update matmul requires_grad checks (#117067)
e10cfdd 
Fixes #116099
Pull Request resolved: #117067
Approved by: https://github.com/lezcano, https://github.com/albanD
ghstack dependencies: #116523, #116710
@facebook-github-bot facebook-github-bot deleted the gh/soulitzer/263/head branch January 13, 2024 15:23
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

@kit1980 kit1980 Awaiting requested review from kit1980

+1 more reviewer

@stas00 stas00 stas00 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged release notes: autograd release notes category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@soulitzer @stas00 @pytorchmergebot @albanD