Skip to content
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

Changed max_steps default from None to -1 #9460

Merged

Conversation

EricWiener
Copy link
Contributor

@EricWiener EricWiener commented Sep 11, 2021

What does this PR do?

This PR addresses this comment. It changes max_steps to default to -1 and no longer allows None to be specified for max_steps.

Fixes #8818

The new combinations for max_epochs and max_steps are:
Only allow None for the max_epochs parameter and in the case (13), otherwise convert it to -1, this way we have:

max_epochs max_steps Effect
1 -1 -1 Infinite training
2 -1 0 Do nothing
3 -1 >1 No epoch limits
4 0 -1 Do nothing
5 0 0 Do nothing
6 0 >1 Do nothing*
7 >1 -1 No step limits
8 >1 0 Do nothing
9 >1 >1 Stop when one hits
10 None -1 Run for 1k epochs

Does your PR introduce any breaking changes? If yes, please list them.

Yes. You can no longer specify max_steps = None.

Not sure what needs to be done w/ regards to this.

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you list all the breaking changes introduced by this pull request?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Always

pytorch_lightning/loops/fit_loop.py Outdated Show resolved Hide resolved
pytorch_lightning/loops/fit_loop.py Outdated Show resolved Hide resolved
pytorch_lightning/loops/fit_loop.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/trainer.py Show resolved Hide resolved
pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
@awaelchli awaelchli changed the title Changed max_steps to default to -1 (vs. `None) Changed max_steps default from None to -1 Sep 12, 2021
Copy link
Contributor

@carmocca carmocca left a comment

Choose a reason for hiding this comment

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

Looking great! Thanks for your work ❤️

pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
pytorch_lightning/loops/fit_loop.py Outdated Show resolved Hide resolved
@carmocca carmocca added this to the v1.5 milestone Sep 14, 2021
@carmocca carmocca added deprecation Includes a deprecation refactor labels Sep 14, 2021
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

Great work ! Almost there.

pytorch_lightning/loops/epoch/training_epoch_loop.py Outdated Show resolved Hide resolved
pytorch_lightning/loops/utilities.py Show resolved Hide resolved
pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
@mergify mergify bot removed the has conflicts label Sep 23, 2021
@rohitgr7 rohitgr7 force-pushed the feature/clean_up_max_steps_epochs branch from 1bf50d7 to a8662f9 Compare October 21, 2021 21:17
@rohitgr7 rohitgr7 enabled auto-merge (squash) October 21, 2021 21:21
pytorch_lightning/loops/epoch/training_epoch_loop.py Outdated Show resolved Hide resolved
pytorch_lightning/loops/epoch/training_epoch_loop.py Outdated Show resolved Hide resolved
pytorch_lightning/loops/epoch/training_epoch_loop.py Outdated Show resolved Hide resolved
pytorch_lightning/loops/fit_loop.py Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@rohitgr7 rohitgr7 requested review from carmocca and tchaton October 22, 2021 07:47
@mergify mergify bot added the ready PRs ready to be merged label Oct 22, 2021
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

LGMT ! Great work !

@codecov
Copy link

codecov bot commented Oct 22, 2021

Codecov Report

Merging #9460 (abf24d3) into master (76081fb) will increase coverage by 0%.
The diff coverage is 100%.

❗ Current head abf24d3 differs from pull request most recent head 8cb57ff. Consider uploading reports for the commit 8cb57ff to get more accurate results

@@          Coverage Diff           @@
##           master   #9460   +/-   ##
======================================
  Coverage      89%     89%           
======================================
  Files         182     182           
  Lines       16157   16153    -4     
======================================
+ Hits        14322   14327    +5     
+ Misses       1835    1826    -9     

@mergify mergify bot removed the has conflicts label Oct 25, 2021
@Borda Borda requested review from ananthsub and rohitgr7 October 25, 2021 20:10
@rohitgr7 rohitgr7 merged commit 0e20119 into Lightning-AI:master Oct 25, 2021
ninginthecloud pushed a commit to ninginthecloud/pytorch-lightning that referenced this pull request Oct 27, 2021
…to `-1` (Lightning-AI#9460)

Co-authored-by: Adrian Wälchli <[email protected]>
Co-authored-by: Carlos Mocholí <[email protected]>
Co-authored-by: thomas chaton <[email protected]>
Co-authored-by: rohitgr7 <[email protected]>
@cowwoc cowwoc mentioned this pull request Nov 9, 2021
facebook-github-bot pushed a commit to facebookresearch/mmf that referenced this pull request Mar 14, 2022
Summary:
PR: Lightning-AI/pytorch-lightning#9460
Introduced by diff D31918136

Reviewed By: tangbinh

Differential Revision: D34804291

fbshipit-source-id: c125f5618e0e4b53975a7d8c1b58ef7f0750afa4
@Borda Borda changed the title Changed max_steps default from None to -1 Changed max_steps default from None to -1 Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation Includes a deprecation priority: 0 High priority task ready PRs ready to be merged refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow disabling automatic stopping during fitting
7 participants