Allow disabling automatic stopping after max_steps or max_epochs #8877

EricWiener · 2021-08-12T23:24:20Z

What does this PR do?

Adds the ability to disable automatic stopping by passing max_epochs < 0 and leaving max_steps = None.

Fixes #8818

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

None

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?

codecov · 2021-08-12T23:49:48Z

Codecov Report

Merging #8877 (1a17f87) into master (045c879) will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #8877   +/-   ##
======================================
  Coverage      88%     88%           
======================================
  Files         176     178    +2     
  Lines       14808   14858   +50     
======================================
+ Hits        13036   13098   +62     
+ Misses       1772    1760   -12

ananthsub

thanks for working on this @EricWiener !

pytorch_lightning/loops/fit_loop.py

ananthsub

@EricWiener more spots doing comparisons with max_epochs need to be upated:

https://github.com/PyTorchLightning/pytorch-lightning/blob/86a0cb74a4ff34e17c791b07297b4e29b90e755b/pytorch_lightning/trainer/connectors/checkpoint_connector.py#L192-L198

pytorch_lightning/loops/fit_loop.py

Co-authored-by: ananthsub <ananth.subramaniam@gmail.com>

EricWiener · 2021-08-27T17:22:35Z

@EricWiener more spots doing comparisons with max_epochs need to be upated:

https://github.com/PyTorchLightning/pytorch-lightning/blob/86a0cb74a4ff34e17c791b07297b4e29b90e755b/pytorch_lightning/trainer/connectors/checkpoint_connector.py#L192-L198

Done

pytorch_lightning/trainer/trainer.py

tests/trainer/test_trainer.py

pytorch_lightning/loops/fit_loop.py

pytorch_lightning/trainer/trainer.py

tests/callbacks/test_timer.py

tests/trainer/test_trainer.py

tchaton

Once comments are applied, LGTM ! Neat work !

docs/source/common/trainer.rst

pytorch_lightning/loops/fit_loop.py

pytorch_lightning/trainer/configuration_validator.py

pytorch_lightning/trainer/connectors/checkpoint_connector.py

tests/trainer/test_trainer.py

pytorch_lightning/trainer/connectors/checkpoint_connector.py

for more information, see https://pre-commit.ci

pytorch_lightning/loops/fit_loop.py

Co-authored-by: ananthsub <ananth.subramaniam@gmail.com>

docs/source/common/trainer.rst

pytorch_lightning/loops/fit_loop.py

pytorch_lightning/trainer/configuration_validator.py

pytorch_lightning/trainer/trainer.py

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

tests/trainer/test_trainer.py

for more information, see https://pre-commit.ci

awaelchli

nice work so far, I have only some minor comments now :)

tests/trainer/test_trainer.py

pytorch_lightning/trainer/configuration_validator.py

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

for more information, see https://pre-commit.ci

EricWiener force-pushed the feature/8818_disable_automatic_stopping branch from 7583fd8 to ab6d6e0 Compare August 13, 2021 00:49

ananthsub reviewed Aug 13, 2021

View reviewed changes

pytorch_lightning/loops/fit_loop.py Outdated Show resolved Hide resolved

EricWiener force-pushed the feature/8818_disable_automatic_stopping branch from 2087404 to 729930a Compare August 27, 2021 13:42

EricWiener marked this pull request as ready for review August 27, 2021 14:07

EricWiener requested review from awaelchli, Borda, carmocca, kaushikb11, SeanNaren, tchaton and williamFalcon as code owners August 27, 2021 14:07

EricWiener marked this pull request as draft August 27, 2021 14:09

ananthsub reviewed Aug 27, 2021

View reviewed changes

pytorch_lightning/loops/fit_loop.py Outdated Show resolved Hide resolved

Update docstring + logic to disable automatic stopping

379dda2

EricWiener force-pushed the feature/8818_disable_automatic_stopping branch from 652c597 to abd1131 Compare August 27, 2021 17:19

EricWiener and others added 5 commits August 27, 2021 13:20

Add a test to check passing negative max_epochs

d506881

Updated logic for disabling automatic stopping

8d9e707

Updated test cases for max_epochs/max_steps + max_time

3a7cc8a

Change brackets to parentheses

a0b8c61

Co-authored-by: ananthsub <ananth.subramaniam@gmail.com>

Corrected max_epoch error checking restore_loops

a239358

EricWiener force-pushed the feature/8818_disable_automatic_stopping branch from abd1131 to a239358 Compare August 27, 2021 17:21

EricWiener marked this pull request as ready for review August 27, 2021 17:21

ananthsub reviewed Aug 27, 2021

View reviewed changes

pytorch_lightning/trainer/trainer.py Show resolved Hide resolved

ananthsub reviewed Aug 27, 2021

View reviewed changes

tests/trainer/test_trainer.py Outdated Show resolved Hide resolved

ananthsub reviewed Aug 27, 2021

View reviewed changes

tests/trainer/test_trainer.py Outdated Show resolved Hide resolved

tchaton reviewed Aug 27, 2021

View reviewed changes

pytorch_lightning/loops/fit_loop.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/trainer.py Show resolved Hide resolved

tests/callbacks/test_timer.py Outdated Show resolved Hide resolved

tests/trainer/test_trainer.py Outdated Show resolved Hide resolved

EricWiener added 2 commits August 27, 2021 18:56

Validating max_epochs and max_steps

d201464

Added parameterized tests for max_epochs + max_steps

91422f5

Shortened max_* desc in trainer.rst

769c7fc

tchaton approved these changes Aug 29, 2021

View reviewed changes

awaelchli reviewed Sep 1, 2021

View reviewed changes

pytorch_lightning/trainer/connectors/checkpoint_connector.py Outdated Show resolved Hide resolved

awaelchli and others added 2 commits September 2, 2021 00:38

Update pytorch_lightning/trainer/connectors/checkpoint_connector.py

4e6bed4

[pre-commit.ci] auto fixes from pre-commit.com hooks

fe11371

for more information, see https://pre-commit.ci

ananthsub mentioned this pull request Sep 1, 2021

Allow disabling automatic stopping during fitting #8818

Closed

ananthsub reviewed Sep 1, 2021

View reviewed changes

pytorch_lightning/loops/fit_loop.py Outdated Show resolved Hide resolved

Change brackets to paranthesis

e8a3174

Co-authored-by: ananthsub <ananth.subramaniam@gmail.com>

carmocca reviewed Sep 2, 2021

View reviewed changes

docs/source/common/trainer.rst Outdated Show resolved Hide resolved

pytorch_lightning/loops/fit_loop.py Show resolved Hide resolved

pytorch_lightning/trainer/configuration_validator.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved

EricWiener and others added 3 commits September 2, 2021 09:56

Update docs/source/common/trainer.rst

1e13018

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

Update pytorch_lightning/trainer/trainer.py

db53b8c

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

No longer checking if max_epochs/steps is an int

b805ff5

awaelchli reviewed Sep 2, 2021

View reviewed changes

tests/trainer/test_trainer.py Outdated Show resolved Hide resolved

EricWiener and others added 4 commits September 3, 2021 13:28

Fixed test_trainer_max_steps_and_epochs_fit_loop_done

4c5f5d8

Fix test_timer.py::test_trainer_flag

05ed3a3

[pre-commit.ci] auto fixes from pre-commit.com hooks

d21b009

for more information, see https://pre-commit.ci

Fixed test_trainer_max_steps_and_epochs_validation

4594284

awaelchli approved these changes Sep 3, 2021

View reviewed changes

tests/trainer/test_trainer.py Outdated Show resolved Hide resolved

tests/trainer/test_trainer.py Show resolved Hide resolved

tests/trainer/test_trainer.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/configuration_validator.py Outdated Show resolved Hide resolved

mergify bot added the ready PRs ready to be merged label Sep 3, 2021

EricWiener and others added 6 commits September 3, 2021 16:11

Decrease global step in tests/trainer/test_trainer.py

318fec8

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

Change EvalModelTemplate to BoringModel

0c322ea

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

Moved max_* validation into constructors

145c27b

[pre-commit.ci] auto fixes from pre-commit.com hooks

3b3f29f

for more information, see https://pre-commit.ci

Fix pre-commit

38b4436

Keep TODO at the top

1a17f87

carmocca approved these changes Sep 4, 2021

View reviewed changes

carmocca enabled auto-merge (squash) September 4, 2021 02:08

carmocca merged commit cf1a589 into Lightning-AI:master Sep 4, 2021

justusschock mentioned this pull request Sep 7, 2021

Weekly Patch Release v1.4.6 [full merge, no squash] #9358

Merged

12 tasks

carmocca added this to the v1.5 milestone Sep 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow disabling automatic stopping after max_steps or max_epochs #8877

Allow disabling automatic stopping after max_steps or max_epochs #8877

EricWiener commented Aug 12, 2021 •

edited

Loading

codecov bot commented Aug 12, 2021 •

edited

Loading

ananthsub left a comment

ananthsub left a comment

EricWiener commented Aug 27, 2021

tchaton left a comment

awaelchli left a comment

Allow disabling automatic stopping after max_steps or max_epochs #8877

Allow disabling automatic stopping after max_steps or max_epochs #8877

Conversation

EricWiener commented Aug 12, 2021 • edited Loading

What does this PR do?

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

Before submitting

PR review

Did you have fun?

codecov bot commented Aug 12, 2021 • edited Loading

Codecov Report

ananthsub left a comment

Choose a reason for hiding this comment

ananthsub left a comment

Choose a reason for hiding this comment

EricWiener commented Aug 27, 2021

tchaton left a comment

Choose a reason for hiding this comment

awaelchli left a comment

Choose a reason for hiding this comment

EricWiener commented Aug 12, 2021 •

edited

Loading

codecov bot commented Aug 12, 2021 •

edited

Loading