Fix ModelCheckpoint period #3630

carmocca · 2020-09-23T21:46:46Z

What does this PR do?

Before submitting

Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
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? Otherwise, we ask you to create a separate PR for every change.
Did you make sure to update the documentation with your changes?
Did you write any new necessary tests?
Did you verify new and existing tests pass locally with your changes?
If you made a notable change (that affects users), did you update the CHANGELOG?

pep8speaks · 2020-09-23T21:46:51Z

Hello @carmocca! Thanks for updating this PR.

In the file pytorch_lightning/callbacks/model_checkpoint.py:

Line 188:13: W503 line break before binary operator
Line 189:13: W503 line break before binary operator
Line 190:13: W503 line break before binary operator
Line 191:13: W503 line break before binary operator
Line 192:13: W503 line break before binary operator

Comment last updated at 2020-09-29 13:04:12 UTC

carmocca · 2020-09-23T21:52:16Z

Line 327:13: W503 line break before binary operator
Line 328:13: W503 line break before binary operator
Line 329:13: W503 line break before binary operator
Line 330:13: W503 line break before binary operator
Line 331:13: W503 line break before binary operatoro.gl/hqbW4r)

Ignoring this because https://www.flake8rules.com/rules/W503.html says: "line breaks should occur before the binary operator because it keeps all operators aligned."

pytorch_lightning/callbacks/model_checkpoint.py

codecov · 2020-09-23T22:14:42Z

Codecov Report

Merging #3630 into master will decrease coverage by 2%.
The diff coverage is 75%.

@@           Coverage Diff            @@
##           master   #3630     +/-   ##
========================================
- Coverage      86%     84%     -2%     
========================================
  Files         110     110             
  Lines        8129    9352   +1223     
========================================
+ Hits         6963    7812    +849     
- Misses       1166    1540    +374

awaelchli · 2020-09-23T22:30:39Z

please, if you don't mind, could you split the PR in two? as I understand, these are 2 independent features/fixes?

carmocca · 2020-09-23T22:35:10Z

Yes they are independent. I joined them because they are easier to test together.

I'll split them

awaelchli · 2020-09-23T22:43:57Z

If you think it is best to have the test combined, you may prioritize one PR, get it reviewed and merged, then focus on the other integrating the test. if that's possible

carmocca · 2020-09-23T23:12:53Z

Okay, this is ready again. Blocked on 3633 because i'm using monitor=None in the test

mergify · 2020-09-25T13:55:37Z

This pull request is now in conflict... :(

pytorch_lightning/callbacks/model_checkpoint.py

awaelchli

solid.
I verified that the test fails on master.

pytorch_lightning/callbacks/model_checkpoint.py

mergify · 2020-09-27T03:15:22Z

This pull request is now in conflict... :(

pytorch_lightning/callbacks/model_checkpoint.py

carmocca · 2020-09-27T16:17:47Z

d79bce1 destroyed this PR so easiest thing was to rebase, undo previous commits and redo them.

Should be ready to go again

pytorch_lightning/callbacks/model_checkpoint.py

tests/callbacks/test_model_checkpoint.py

pytorch_lightning/callbacks/model_checkpoint.py

rohitgr7

LGTM

awaelchli · 2020-09-27T16:50:43Z

anything substantial changed since we last reviewed?
we should probably check that test still fails on master. I will do that.

tests/callbacks/test_model_checkpoint.py

carmocca · 2020-09-27T16:54:55Z

anything substantial changed since we last reviewed?
we should probably check that test still fails on master. I will do that.

The only additional change is:

https://github.com/PyTorchLightning/pytorch-lightning/pull/3630/files#diff-63eb50f09b915db4167aed7cb38ac5c0R218

which I think is a bug

awaelchli · 2020-09-27T17:05:47Z

anything substantial changed since we last reviewed?
we should probably check that test still fails on master. I will do that.

The only additional change is:

https://github.com/PyTorchLightning/pytorch-lightning/pull/3630/files#diff-63eb50f09b915db4167aed7cb38ac5c0R218

which I think is a bug

why not >= 1? In order to track the top 1, we need to have a monitor, no?

carmocca · 2020-09-27T17:16:27Z

why not >= 1? In order to track the top 1, we need to have a monitor, no?

Yes but if we change that, the default ModelCheckpoint parameters are incorrect because monitor is None by default but top_k is 1 (and would fail in that check)

williamFalcon · 2020-09-27T17:25:05Z

monitor is by default None.
top_k should probably be None in this case as well.

To save ALL checkpoints, then top_k=-1.

by default we will only save the last checkpoint (otherwise you WILL blow up your disk)

carmocca · 2020-09-27T17:30:33Z

Im okay with that. Just one thing:

If default behaviour does the same as save_last, why not have save_last=True by default to make it obvious?

williamFalcon · 2020-09-27T17:41:45Z

because save_last is a separate operation...

We have these modes:
a. default
b. save top k
c. save last

When not in default, user CAN save last AND top k or only one of them. Thus each one is its own behavior that must also be turned on.

Once the user leaves default mode, they'd be very surprised to continue finding the last ckpts saved

carmocca · 2020-09-27T17:54:44Z

Then maybe we should throw a warning:

if save_last and monitor is None and save_top_k is not None:
    rank_zero_warn(
        "ModelCheckpoint save_last is set to True but the last checkpoint is already saved"
        f" by having monitor=None and save_top_k={save_top_k}"
    )

(or a similar warning)

williamFalcon · 2020-09-27T17:57:48Z

this is unnecessarily complicated...

save_top_k is NOT the same as save_last...

the last ckpt may not be the best one (ie: overfitting)

We're going to keep the current behavior with the only change which is top_k defaults to None.

Again... to be clear:
Once a user stops using the default behavior, they can save top k or the last.
If the user just wants to save the last, then they would keep it as the default behavior.

If the user sets to save the top k then they can ALSO opt in to saving the last by enabling that.

I understand what you're saying, but for a user to get the last when they set top k is very confusing... unless they also ask for the last.

Maybe the answer is to ALSO set save_last=None by default.

carmocca · 2020-09-27T18:05:22Z

save_top_k is NOT the same as save_last..

Of course it is not always, but the default values of ModelCheckpoint monitor=None and top_k=1 (None in the future) means that only the last run epoch checkpoint is kept.

That means that ModelCheckpoint() does the same as ModelCheckpoint(save_last=True). But in the second case you get two files for the same thing.

Thinking as a new user, I would be a bit confused about this.

The warning I propose is for the case where monitor is None and save_last=True. Im not proposing that and having save_last=True by default.

carmocca · 2020-09-27T18:08:01Z

Maybe the answer is to ALSO set save_last=None by default.

I think so, considering that save_last only makes sense when we have a monitor and we don't by default

awaelchli · 2020-09-27T18:08:20Z

working on the default savetopk here #3680
want to get it done asap so we can continue on this period fix.

williamFalcon · 2020-09-27T18:11:46Z

@awaelchli let's make save_last = None by default as well?

mergify · 2020-09-28T00:06:27Z

This pull request is now in conflict... :(

mergify · 2020-09-28T00:40:51Z

This pull request is now in conflict... :(

carmocca · 2020-09-29T12:21:29Z

Anything holding this back?

mergify · 2020-09-29T12:29:05Z

This pull request is now in conflict... :(

Already pushed to master This reverts commit 00d9e77.

Borda · 2020-09-29T13:16:22Z

Anything holding this back?

it shall be fine now...

mergify bot requested a review from a team September 23, 2020 21:47

carmocca commented Sep 23, 2020

View reviewed changes

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

carmocca changed the title ~~Fix ModelCheckpoint period and allow None monitor~~ Fix ModelCheckpoint period Sep 23, 2020

Borda added the bug Something isn't working label Sep 23, 2020

carmocca changed the title ~~Fix ModelCheckpoint period~~ [blocked by #3644] Fix ModelCheckpoint period Sep 23, 2020

carmocca changed the title ~~[blocked by #3644] Fix ModelCheckpoint period~~ [blocked by #3633] Fix ModelCheckpoint period Sep 23, 2020

justusschock approved these changes Sep 25, 2020

View reviewed changes

mergify bot requested a review from a team September 25, 2020 13:43

awaelchli reviewed Sep 25, 2020

View reviewed changes

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

mergify bot requested a review from a team September 25, 2020 14:05

carmocca changed the title ~~[blocked by #3633] Fix ModelCheckpoint period~~ Fix ModelCheckpoint period Sep 25, 2020

awaelchli approved these changes Sep 25, 2020

View reviewed changes

mergify bot requested a review from a team September 25, 2020 14:42

Borda approved these changes Sep 25, 2020

View reviewed changes

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

Borda added the ready PRs ready to be merged label Sep 25, 2020

rohitgr7 reviewed Sep 27, 2020

View reviewed changes

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

rohitgr7 suggested changes Sep 27, 2020

View reviewed changes

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

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

rohitgr7 reviewed Sep 27, 2020

View reviewed changes

pytorch_lightning/callbacks/model_checkpoint.py Show resolved Hide resolved

rohitgr7 approved these changes Sep 27, 2020

View reviewed changes

awaelchli reviewed Sep 27, 2020

View reviewed changes

tests/callbacks/test_model_checkpoint.py Show resolved Hide resolved

awaelchli changed the title ~~Fix ModelCheckpoint period~~ [blocked by #3680] Fix ModelCheckpoint period Sep 27, 2020

awaelchli changed the title ~~[blocked by #3680] Fix ModelCheckpoint period~~ Fix ModelCheckpoint period Sep 28, 2020

carmocca and others added 4 commits September 28, 2020 02:55

Fix ModelCheckpoint period

323f9c6

Remove comma

a13b7c5

Minor changes

90d1323

skip check

00d9e77

Revert "skip check"

34086ba

Already pushed to master This reverts commit 00d9e77.

awaelchli merged commit 3b2efe5 into Lightning-AI:master Sep 29, 2020

carmocca deleted the bug/3619_fix-period branch September 29, 2020 13:41

Borda added this to the 0.10.0 milestone Oct 7, 2020

carmocca self-assigned this Nov 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ModelCheckpoint period #3630

Fix ModelCheckpoint period #3630

carmocca commented Sep 23, 2020 •

edited

Loading

pep8speaks commented Sep 23, 2020 •

edited

Loading

carmocca commented Sep 23, 2020

codecov bot commented Sep 23, 2020 •

edited

Loading

awaelchli commented Sep 23, 2020

carmocca commented Sep 23, 2020

awaelchli commented Sep 23, 2020 •

edited

Loading

carmocca commented Sep 23, 2020

mergify bot commented Sep 25, 2020

awaelchli left a comment

mergify bot commented Sep 27, 2020

carmocca commented Sep 27, 2020

rohitgr7 left a comment

awaelchli commented Sep 27, 2020

carmocca commented Sep 27, 2020 •

edited

Loading

awaelchli commented Sep 27, 2020

carmocca commented Sep 27, 2020

williamFalcon commented Sep 27, 2020

carmocca commented Sep 27, 2020

williamFalcon commented Sep 27, 2020 •

edited

Loading

carmocca commented Sep 27, 2020 •

edited

Loading

williamFalcon commented Sep 27, 2020 •

edited

Loading

carmocca commented Sep 27, 2020 •

edited

Loading

carmocca commented Sep 27, 2020

awaelchli commented Sep 27, 2020

williamFalcon commented Sep 27, 2020 •

edited

Loading

mergify bot commented Sep 28, 2020

mergify bot commented Sep 28, 2020

carmocca commented Sep 29, 2020

mergify bot commented Sep 29, 2020

Borda commented Sep 29, 2020

Fix ModelCheckpoint period #3630

Fix ModelCheckpoint period #3630

Conversation

carmocca commented Sep 23, 2020 • edited Loading

What does this PR do?

Before submitting

pep8speaks commented Sep 23, 2020 • edited Loading

Comment last updated at 2020-09-29 13:04:12 UTC

carmocca commented Sep 23, 2020

codecov bot commented Sep 23, 2020 • edited Loading

Codecov Report

awaelchli commented Sep 23, 2020

carmocca commented Sep 23, 2020

awaelchli commented Sep 23, 2020 • edited Loading

carmocca commented Sep 23, 2020

mergify bot commented Sep 25, 2020

awaelchli left a comment

Choose a reason for hiding this comment

mergify bot commented Sep 27, 2020

carmocca commented Sep 27, 2020

rohitgr7 left a comment

Choose a reason for hiding this comment

awaelchli commented Sep 27, 2020

carmocca commented Sep 27, 2020 • edited Loading

awaelchli commented Sep 27, 2020

carmocca commented Sep 27, 2020

williamFalcon commented Sep 27, 2020

carmocca commented Sep 27, 2020

williamFalcon commented Sep 27, 2020 • edited Loading

carmocca commented Sep 27, 2020 • edited Loading

williamFalcon commented Sep 27, 2020 • edited Loading

carmocca commented Sep 27, 2020 • edited Loading

carmocca commented Sep 27, 2020

awaelchli commented Sep 27, 2020

williamFalcon commented Sep 27, 2020 • edited Loading

mergify bot commented Sep 28, 2020

mergify bot commented Sep 28, 2020

carmocca commented Sep 29, 2020

mergify bot commented Sep 29, 2020

Borda commented Sep 29, 2020

carmocca commented Sep 23, 2020 •

edited

Loading

pep8speaks commented Sep 23, 2020 •

edited

Loading

codecov bot commented Sep 23, 2020 •

edited

Loading

awaelchli commented Sep 23, 2020 •

edited

Loading

carmocca commented Sep 27, 2020 •

edited

Loading

williamFalcon commented Sep 27, 2020 •

edited

Loading

carmocca commented Sep 27, 2020 •

edited

Loading

williamFalcon commented Sep 27, 2020 •

edited

Loading

carmocca commented Sep 27, 2020 •

edited

Loading

williamFalcon commented Sep 27, 2020 •

edited

Loading