Fix ModelCheckpoints name formatting #3163

carmocca · 2020-08-25T15:41:11Z

What does this PR do?

Fixes several typos.
Adds CHECKPOINT_JOIN_CHAR~~, CHECKPOINT_SUFFIX~~ variables for customization.
Allows using templates in the last checkpoint saved.
Adds a test.

Fixes #3125
Fixes #2583
Closes #2584

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-08-25T15:41:15Z

Hello @carmocca! Thanks for updating this PR.

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

Comment last updated at 2020-09-18 13:13:23 UTC

pytorch_lightning/callbacks/model_checkpoint.py

carmocca · 2020-08-25T16:14:33Z

Should we add the possibility of modifying these variables to the doc-string?

codecov · 2020-08-25T16:41:37Z

Codecov Report

Merging #3163 into master will decrease coverage by 0%.
The diff coverage is 97%.

@@          Coverage Diff           @@
##           master   #3163   +/-   ##
======================================
- Coverage      91%     91%   -0%     
======================================
  Files         109     109           
  Lines        8031    8066   +35     
======================================
+ Hits         7291    7311   +20     
- Misses        740     755   +15

Borda · 2020-08-25T17:18:13Z

Should we add the possibility of modifying these variables to the doc-string?

which variables do you mean?

carmocca · 2020-08-25T17:34:12Z

CHECKPOINT_JOIN_CHAR, CHECKPOINT_NAME_LAST~~, and CHECKPOINT_SUFFIX~~

carmocca · 2020-08-25T17:50:54Z

I believe this would also solve #2583 #2584 if merged

pytorch_lightning/callbacks/model_checkpoint.py

rohitgr7 · 2020-08-25T18:14:28Z

I believe this would also solve #2583 #2584 if merged

First, we need to clear up whether we want a separate filename parameter or not, because #2583 is a different issue. I don't think this PR solves that problem.

carmocca · 2020-08-25T20:25:58Z

First, we need to clear up whether we want a separate filename parameter or not, because #2583 is a different issue. I don't think this PR solves that problem.

With the changes in this PR, one can do:

checkpoint_callback = pl.callbacks.ModelCheckpoint(filepath="my/file/path/name")
# Generates checkpoints called "name.ckpt" in my/file/path

But if filepath is an existing directory:

checkpoint_callback = pl.callbacks.ModelCheckpoint(filepath="my/file/path")
# Generates checkpoints called "epoch={epoch}.ckpt" in my/file/path
# note: this is the previous default behaviour

So there is no need of a filename parameter.

ydcjeff · 2020-08-26T16:14:02Z

can we have a test for this r = ModelCheckpoint._format_checkpoint_name('path/to/ckpt', 3, {}, prefix='test') as well?

carmocca · 2020-08-27T01:13:10Z

can we have a test for this r = ModelCheckpoint._format_checkpoint_name('path/to/ckpt', 3, {}, prefix='test') as well?

_format_checkpoint_name's first parameter is assumed to be a filename. Your example passes a filepath so the result would be r == "test-path/to/ckpt". I added tests using filepaths with format_checkpoint_name(no leading _).

awaelchli · 2020-08-29T12:18:47Z

pytorch_lightning/callbacks/model_checkpoint.py

+            self._del_model(self.last_model_path)
            self._save_model(filepath, trainer, pl_module)
+            self.last_model_path = filepath


can we (in a new PR pls) first rename the model to "oldname.ckpt.backup" before saving the model, and then delete the backup file after save has completed? Otherwise, if a crash happens during save_model, we will have lost the old checkpoint and are only left with a corrupted file for the current.

A new PR based on master or this branch?

yes, just a small suggestion after this PR is merged. but it will touch other locations as well, for example topk saving.

awaelchli · 2020-08-29T16:42:33Z

I prefer this PR over the one that adds an extra filename parameter.

rohitgr7 · 2020-08-29T18:33:14Z

So there is no need of a filename parameter.

The reason I suggested separate dirpath and filename parameter is because it gives the functionality to get the default versioning of checkpoints out-of-the-box as if dirpath is set to None and filename is defined then ModelCheckpoint will use the versioning functionality of checkpoints in lightning_logs/version_num. Also, resolve some issues that are handled by his PR and is a bit easy to use and manipulate checkpoints for users.

ananthsub · 2020-08-31T04:15:56Z

Regarding this:

# Generates checkpoints called "name.ckpt" in my/file/path
checkpoint_callback = pl.callbacks.ModelCheckpoint(filepath="my/file/path/name")

This may not be the only intention for the filepath. For instance, the user could also expect the callback to save checkpoints to directory my/file/path/name with name {epoch}.ckpt. This is hard to distinguish with filepath being overloaded. It's also confusing because you could pass a yet non-existent directory path as filepath to the checkpoint callback and expect Lightning to make the directory to save checkpoints to. Parsing whether the last component should be part of the directory or the checkpoint name is impossible to do right, and suggests better support from the API arguments. IMO splitting filepath into 2 args, dirpath and file_template_name (with default value = "{epoch}"), would resolve this nicely.

carmocca · 2020-08-31T04:47:11Z

I am not against the idea of splitting filepath into dirpath and filename, however, that discussion is out of scope of this PR.

Considering that this PR does not change the API, we could merge this and then (possibly?) work on the filepath improvement.

carmocca · 2020-09-03T10:10:36Z

Anything holding this back?

There will be some conflicts with #3320. Should be easy to fix

Borda · 2020-09-03T10:48:56Z

Let's process it...

ydcjeff · 2020-09-03T11:33:07Z

Just found out this
Shall we fix this typo in this PR too?
https://github.com/PyTorchLightning/pytorch-lightning/blob/83e51def23ecf98c7c4d98a212b52af4c0a40515/pytorch_lightning/callbacks/early_stopping.py#L112

Should be validation_epoch_end

Borda · 2020-09-03T11:57:55Z

Shall we fix this typo in this PR too?

yes, pls

carmocca · 2020-09-03T12:16:30Z

@ydcjeff @Borda done!

mergify · 2020-09-03T12:19:25Z

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

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

tests/callbacks/test_model_checkpoint.py

tests/trainer/test_trainer.py

awaelchli

I think this is a great improvement. Thanks for your effort.
Please note for the future, this PR has some independent changes that, although they are good to have, they could have been split over multiple PR's.

pytorch_lightning/callbacks/model_checkpoint.py

awaelchli · 2020-09-21T00:08:48Z

Parsing whether the last component should be part of the directory or the checkpoint name is impossible to do right, and suggests better support from the API arguments. IMO splitting filepath into 2 args, dirpath and file_template_name (with default value = "{epoch}"), would resolve this nicely.

as mentioned by @ananthsub it is not possible to distinguish between filename and folder, this bug: #3001
My suggestion was to force user to choose a file extension so that we know when it is a file. But perhaps a separate arg is nicer.
Also there is a 2nd bug which complicates things, self._fs.isdir checks for a real existing dir, but we want to accept a folder name that does not exist yet.

Either way, to fix this I don't see a good way that keeps back compatibility. If back compatibility is unavoidable, I feel like now with the v1 release would be a good time to introduce this breaking change. If not, I think we should at least update docs.
@PyTorchLightning/core-contributors

carmocca · 2020-09-21T01:50:38Z

Either way, to fix this I don't see a good way that keeps back compatibility.

Cant we keep back compat by having filepath (old parameter) and dirpath, filename (new parameters)?

if filepath is set: old logic, show deprecated warning.
if dirpath and filename are set: new logic.
if filepath, dirpath, and filename are set: raise MisconfigurationException or use new logic

BTW this discussion should continue elsewhere. Maybe in the issue you linked.

Borda · 2020-09-21T07:41:43Z

Also there is a 2nd bug which complicates things, self._fs.isdir checks for a real existing dir, but we want to accept a folder name that does not exist yet.

then we can do some bubble up testing to find the existing folder in upstream and create the remaining?

Either way, to fix this I don't see a good way that keeps back compatibility. If back compatibility is unavoidable, I feel like now with the v1 release would be a good time to introduce this breaking change. If not, I think we should at least update docs.

yes, 1.0 is a good place to do it...

Borda · 2020-09-21T07:42:55Z

BTW this discussion should continue elsewhere. Maybe in the issue you linked.

@carmocca mind creating a new issue for this discussion and link it here...

mergify bot requested a review from a team August 25, 2020 15:41

Borda added bug Something isn't working feature Is an improvement or enhancement labels Aug 25, 2020

Borda added this to the 0.9.x milestone Aug 25, 2020

Borda reviewed Aug 25, 2020

View reviewed changes

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

mergify bot requested a review from a team August 25, 2020 15:53

rohitgr7 reviewed Aug 25, 2020

View reviewed changes

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

mergify bot requested a review from a team August 25, 2020 18:12

awaelchli reviewed Aug 29, 2020

View reviewed changes

mergify bot requested a review from a team August 29, 2020 12:19

carmocca and others added 9 commits September 17, 2020 05:21

Remove CHECKPOINT_SUFFIX

06498a6

Fix typos. Use appropriate f-string format

3996287

Apply suggestions from code review

9797847

Fix broken tests after #3320

b922f94

Finish changes suggested by Borda

946ffb6

Use explicit test var names

0996b9e

Apply suggestions

a62f29b

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

Apply suggestions

470225d

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

Update CHANGELOG

fca38cb

Borda reviewed Sep 17, 2020

View reviewed changes

Apply suggestions from code review

e931aad

mergify bot requested a review from a team September 17, 2020 19:51

Borda reviewed Sep 17, 2020

View reviewed changes

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

mergify bot requested a review from a team September 17, 2020 20:07

for

cbc84be

Borda approved these changes Sep 17, 2020

View reviewed changes

awaelchli approved these changes Sep 18, 2020

View reviewed changes

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

prepend whitespace in warn msg

02ef29a

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

Borda merged commit 580b04b into Lightning-AI:master Sep 18, 2020

Borda mentioned this pull request Sep 18, 2020

Add filename parameter to ModelCheckpoint #2584

Closed

This was referenced Sep 21, 2020

Split ModelCheckpoint filepath into dirpath and filename #3583

Closed

testing bug from #3163 #3594

Merged

carmocca deleted the modelcheckpoint-naming branch September 22, 2020 18:33

carmocca mentioned this pull request Dec 3, 2020

Custom Checkpoint file extension #4963

Closed

carmocca self-assigned this Nov 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ModelCheckpoints name formatting #3163

Fix ModelCheckpoints name formatting #3163

carmocca commented Aug 25, 2020 •

edited

Loading

pep8speaks commented Aug 25, 2020 •

edited

Loading

carmocca commented Aug 25, 2020

codecov bot commented Aug 25, 2020 •

edited

Loading

Borda commented Aug 25, 2020

carmocca commented Aug 25, 2020 •

edited

Loading

carmocca commented Aug 25, 2020

rohitgr7 commented Aug 25, 2020

carmocca commented Aug 25, 2020 •

edited

Loading

ydcjeff commented Aug 26, 2020

carmocca commented Aug 27, 2020 •

edited

Loading

awaelchli Aug 29, 2020 •

edited

Loading

carmocca Aug 29, 2020

awaelchli Aug 29, 2020

awaelchli commented Aug 29, 2020

rohitgr7 commented Aug 29, 2020

ananthsub commented Aug 31, 2020 •

edited

Loading

carmocca commented Aug 31, 2020

carmocca commented Sep 3, 2020

Borda commented Sep 3, 2020

ydcjeff commented Sep 3, 2020 •

edited

Loading

Borda commented Sep 3, 2020

carmocca commented Sep 3, 2020

mergify bot commented Sep 3, 2020

awaelchli left a comment

awaelchli commented Sep 21, 2020 •

edited

Loading

carmocca commented Sep 21, 2020 •

edited

Loading

Borda commented Sep 21, 2020

Borda commented Sep 21, 2020

Fix ModelCheckpoints name formatting #3163

Fix ModelCheckpoints name formatting #3163

Conversation

carmocca commented Aug 25, 2020 • edited Loading

What does this PR do?

Before submitting

pep8speaks commented Aug 25, 2020 • edited Loading

Comment last updated at 2020-09-18 13:13:23 UTC

carmocca commented Aug 25, 2020

codecov bot commented Aug 25, 2020 • edited Loading

Codecov Report

Borda commented Aug 25, 2020

carmocca commented Aug 25, 2020 • edited Loading

carmocca commented Aug 25, 2020

rohitgr7 commented Aug 25, 2020

carmocca commented Aug 25, 2020 • edited Loading

ydcjeff commented Aug 26, 2020

carmocca commented Aug 27, 2020 • edited Loading

awaelchli Aug 29, 2020 • edited Loading

Choose a reason for hiding this comment

carmocca Aug 29, 2020

Choose a reason for hiding this comment

awaelchli Aug 29, 2020

Choose a reason for hiding this comment

awaelchli commented Aug 29, 2020

rohitgr7 commented Aug 29, 2020

ananthsub commented Aug 31, 2020 • edited Loading

carmocca commented Aug 31, 2020

carmocca commented Sep 3, 2020

Borda commented Sep 3, 2020

ydcjeff commented Sep 3, 2020 • edited Loading

Borda commented Sep 3, 2020

carmocca commented Sep 3, 2020

mergify bot commented Sep 3, 2020

awaelchli left a comment

Choose a reason for hiding this comment

awaelchli commented Sep 21, 2020 • edited Loading

carmocca commented Sep 21, 2020 • edited Loading

Borda commented Sep 21, 2020

Borda commented Sep 21, 2020

carmocca commented Aug 25, 2020 •

edited

Loading

pep8speaks commented Aug 25, 2020 •

edited

Loading

codecov bot commented Aug 25, 2020 •

edited

Loading

carmocca commented Aug 25, 2020 •

edited

Loading

carmocca commented Aug 25, 2020 •

edited

Loading

carmocca commented Aug 27, 2020 •

edited

Loading

awaelchli Aug 29, 2020 •

edited

Loading

ananthsub commented Aug 31, 2020 •

edited

Loading

ydcjeff commented Sep 3, 2020 •

edited

Loading

awaelchli commented Sep 21, 2020 •

edited

Loading

carmocca commented Sep 21, 2020 •

edited

Loading