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

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

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     

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

which variables do you mean?

CHECKPOINT_JOIN_CHAR, CHECKPOINT_NAME_LAST, and CHECKPOINT_SUFFIX

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

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.

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.

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

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 _).

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

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.

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.

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.

Anything holding this back?

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

Let's process it...

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

Shall we fix this typo in this PR too?

yes, pls

@ydcjeff @Borda done!

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

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

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.

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...

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...