Deprecate the default `EarlyStopping` callback monitor value #7907

bamblebam · 2021-06-09T18:05:04Z

What does this PR do?

Removes the default value for the monitor argument from the early stopping callback. Also adds the deprecation message.
Fixes #7894

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 update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
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?

Make sure you had fun coding 🙃

pep8speaks · 2021-06-09T18:05:07Z

Hello @bamblebam! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-06-10 19:12:24 UTC

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

codecov · 2021-06-09T18:07:25Z

Codecov Report

Merging #7907 (8cfe43e) into master (ec4f885) will decrease coverage by 1%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #7907    +/-   ##
=======================================
- Coverage      88%     88%    -1%     
=======================================
  Files         204     200     -4     
  Lines       13667   12839   -828     
=======================================
- Hits        12047   11237   -810     
+ Misses       1620    1602    -18

…github.com/bamblebam/pytorch-lightning into bugfix/7894-early_stopping_monitor_default

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

ananthsub

thanks for working on this @bamblebam !

please add a test here: https://github.com/PyTorchLightning/pytorch-lightning/blob/master/tests/deprecated_api/test_remove_1-6.py

which instantiates an EarlyStopping callback without setting the monitor to confirm the warning appears as expected

pytorch_lightning/callbacks/early_stopping.py

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

bamblebam · 2021-06-10T05:37:05Z

@ananthsub can you tell me why my test is failing?

pytorch_lightning/callbacks/early_stopping.py

ananthsub

Be sure to add a CHANGELOG entry to the deprecated section of 1.4.0 with this PR:

See here for examples
https://github.com/PyTorchLightning/pytorch-lightning/blob/master/CHANGELOG.md#deprecated

bamblebam · 2021-06-10T08:01:11Z

Hey I updated the changelog.
@ananthsub should I mark this as ready for review?

ananthsub · 2021-06-10T17:54:23Z

CHANGELOG.md

+- Deprecated `monitor` argument in EarlyStopping will be required. ([#7907](https://github.com/PyTorchLightning/pytorch-lightning/pull/7907))
+


Suggested change

- Deprecated `monitor` argument in EarlyStopping will be required. ([#7907](https://github.com/PyTorchLightning/pytorch-lightning/pull/7907))

- Deprecated default value of `monitor` argument in EarlyStopping callback to enforce `monitor` as a required argument ([#7907](https://github.com/PyTorchLightning/pytorch-lightning/pull/7907))

ananthsub · 2021-06-10T17:55:06Z

yes this is ready for review

carmocca · 2021-06-10T18:05:15Z

pytorch_lightning/callbacks/early_stopping.py

@@ -120,6 +119,13 @@ def __init__(
        torch_inf = torch.tensor(np.Inf)
        self.best_score = torch_inf if self.monitor_op == torch.lt else -torch_inf

+        if monitor is None:
+            rank_zero_deprecation(
+                "The `monitor` argument will be required to be set starting in v1.6."


Suggested change

"The `monitor` argument will be required to be set starting in v1.6."

"The `EarlyStoppinf(monitor)` argument will be required starting in v1.6."

awaelchli

we may want to consider making the change to our tests wherever the argument is missing.

awaelchli · 2021-06-10T18:01:17Z

CHANGELOG.md

+- Deprecated `monitor` argument in EarlyStopping will be required. ([#7907](https://github.com/PyTorchLightning/pytorch-lightning/pull/7907))
+


awaelchli · 2021-06-10T18:15:06Z

pytorch_lightning/callbacks/early_stopping.py

+                "The `monitor` argument will be required to be set starting in v1.6."
+                " For backward compatibility, setting this to `early_stop_on`."
+            )
+        self.monitor = monitor or 'early_stop_on'


Suggested change

self.monitor = monitor or 'early_stop_on'

self.monitor = monitor or "early_stop_on"

double quotes everywhere else it seems

alright I will do the change

you can just click the button "commit suggestion" here on github below my message. so you don't have to manually commit it. what you prefer!

note quoting will get standardized with #7783

bamblebam · 2021-06-10T19:06:02Z

@awaelchli can you tell me why the tests might be failing?

tests/deprecated_api/test_remove_1-6.py

ananthsub

great job @bamblebam !

bamblebam · 2021-06-11T01:45:38Z

Thanks for guiding me through this :)

…ng-AI#7907) * removed monitor default value and added depreceation message * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * format change * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * requested changes * added test * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * format changes * typehint change * Update CHANGELOG.md * requested changes * regex Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Adrian Wälchli <[email protected]>

removed monitor default value and added depreceation message

56613a6

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

a4c415c

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

bamblebam and others added 3 commits June 9, 2021 23:45

format change

c02d4d6

Merge branch 'bugfix/7894-early_stopping_monitor_default' of https://…

00152e1

…github.com/bamblebam/pytorch-lightning into bugfix/7894-early_stopping_monitor_default

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

3c42c13

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

ananthsub suggested changes Jun 9, 2021

View reviewed changes

pytorch_lightning/callbacks/early_stopping.py Outdated Show resolved Hide resolved

pytorch_lightning/callbacks/early_stopping.py Show resolved Hide resolved

bamblebam and others added 4 commits June 10, 2021 10:18

requested changes

7a44c11

added test

c6e37dc

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

e159b15

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

format changes

da08ffd

ananthsub reviewed Jun 10, 2021

View reviewed changes

pytorch_lightning/callbacks/early_stopping.py Outdated Show resolved Hide resolved

typehint change

e080181

ananthsub reviewed Jun 10, 2021

View reviewed changes

Update CHANGELOG.md

ff15f18

ananthsub reviewed Jun 10, 2021

View reviewed changes

ananthsub marked this pull request as ready for review June 10, 2021 17:55

ananthsub requested review from awaelchli, Borda, carmocca, justusschock, kaushikb11, SeanNaren, tchaton and williamFalcon as code owners June 10, 2021 17:55

carmocca reviewed Jun 10, 2021

View reviewed changes

carmocca added this to the v1.4 milestone Jun 10, 2021

carmocca added the feature Is an improvement or enhancement label Jun 10, 2021

carmocca changed the title ~~Fixes #7894 Removing early stopping callback monitor default value~~ Deprecate the default EarlyStopping callback monitor value Jun 10, 2021

awaelchli approved these changes Jun 10, 2021

View reviewed changes

awaelchli reviewed Jun 10, 2021

View reviewed changes

awaelchli added the callback label Jun 10, 2021

requested changes

60e5d82

carmocca approved these changes Jun 10, 2021

View reviewed changes

awaelchli reviewed Jun 10, 2021

View reviewed changes

tests/deprecated_api/test_remove_1-6.py Outdated Show resolved Hide resolved

regex

8cfe43e

awaelchli requested a review from ananthsub June 10, 2021 22:44

awaelchli added the ready PRs ready to be merged label Jun 10, 2021

ananthsub approved these changes Jun 11, 2021

View reviewed changes

ananthsub merged commit 8b73869 into Lightning-AI:master Jun 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate the default `EarlyStopping` callback monitor value #7907

Deprecate the default `EarlyStopping` callback monitor value #7907

bamblebam commented Jun 9, 2021 •

edited by ananthsub

Loading

pep8speaks commented Jun 9, 2021 •

edited

Loading

codecov bot commented Jun 9, 2021 •

edited

Loading

ananthsub left a comment

bamblebam commented Jun 10, 2021

ananthsub left a comment

bamblebam commented Jun 10, 2021 •

edited

Loading

ananthsub Jun 10, 2021

awaelchli Jun 10, 2021

ananthsub commented Jun 10, 2021

carmocca Jun 10, 2021

awaelchli left a comment

awaelchli Jun 10, 2021

awaelchli Jun 10, 2021

bamblebam Jun 10, 2021

awaelchli Jun 10, 2021

carmocca Jun 10, 2021

bamblebam commented Jun 10, 2021

ananthsub left a comment

bamblebam commented Jun 11, 2021

		- Deprecated `monitor` argument in EarlyStopping will be required. ([#7907](https://github.com/PyTorchLightning/pytorch-lightning/pull/7907))

	- Deprecated `monitor` argument in EarlyStopping will be required. ([#7907](https://github.com/PyTorchLightning/pytorch-lightning/pull/7907))
	- Deprecated default value of `monitor` argument in EarlyStopping callback to enforce `monitor` as a required argument ([#7907](https://github.com/PyTorchLightning/pytorch-lightning/pull/7907))

	"The `monitor` argument will be required to be set starting in v1.6."
	"The `EarlyStoppinf(monitor)` argument will be required starting in v1.6."

	self.monitor = monitor or 'early_stop_on'
	self.monitor = monitor or "early_stop_on"

Deprecate the default EarlyStopping callback monitor value #7907

Deprecate the default EarlyStopping callback monitor value #7907

Conversation

bamblebam commented Jun 9, 2021 • edited by ananthsub Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented Jun 9, 2021 • edited Loading

Comment last updated at 2021-06-10 19:12:24 UTC

codecov bot commented Jun 9, 2021 • edited Loading

Codecov Report

ananthsub left a comment

Choose a reason for hiding this comment

bamblebam commented Jun 10, 2021

ananthsub left a comment

Choose a reason for hiding this comment

bamblebam commented Jun 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ananthsub commented Jun 10, 2021

Choose a reason for hiding this comment

awaelchli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bamblebam commented Jun 10, 2021

ananthsub left a comment

Choose a reason for hiding this comment

bamblebam commented Jun 11, 2021

Deprecate the default `EarlyStopping` callback monitor value #7907

Deprecate the default `EarlyStopping` callback monitor value #7907

bamblebam commented Jun 9, 2021 •

edited by ananthsub

Loading

pep8speaks commented Jun 9, 2021 •

edited

Loading

codecov bot commented Jun 9, 2021 •

edited

Loading

bamblebam commented Jun 10, 2021 •

edited

Loading