None check for filepath in ModelCheckpoint

[yukw777]

Check if the optional filepath is None before checking if it exists

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?
• N/A Did you make sure to update the docs?
• Did you write any new necessary tests?
• If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Fixes #1535. #1535 (comment)

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[pep8speaks]

Hello @yukw777! Thanks for updating this PR.

• In the file pytorch_lightning/callbacks/model_checkpoint.py:

Line 89:111: E501 line too long (113 > 110 characters)

Comment last updated at 2020-04-29 16:08:12 UTC

[yukw777]

Line 89:111: E501 line too long (113 > 110 characters)

I'm assuming this is OK? I thought the line length limit was 120.

[codecov]

Codecov Report

Merging #1654 into master will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1654   +/-   ##
======================================
  Coverage      88%     88%           
======================================
  Files          71      71           
  Lines        4175    4175           
======================================
+ Hits         3691    3692    +1     
+ Misses        484     483    -1     

[Borda]

Line 89:111: E501 line too long (113 > 110 characters)

I'm assuming this is OK? I thought the line length limit was 120.

we set kind or warning limit 110 which is applied via pep8speaks and 120 as a hard limit in tests (flake8)

[Borda]

LGTM 🦝

[tullie]

Do you think it's worth having a log for this case? E.g. "Model checkpoint isn't saved because file path not provided".

I think it's almost worth making this a warning log, it's pretty frustrating if you train a model and then realize after a few days of training that the models weren't checkpointing.

[yukw777]

@tullie checkpoints are still saved if filepath is None. It just means that the user wants PL to dynamically figure out the path, which is inside the log folder under checkpoints.

[tullie]

@yukw777 makes sense! Thanks

[mergify]

Great job! =)

[mergify]

Great job! =)

[olineumann]

Good work!

Wouldn't it be better to check save_top_k != 0 instead of save_top_k > 0 because also when save_top_k == -1 checkpoints could be overwritten.

See:
olineumann@1e0844a

@Borda @awaelchli
Because it is already merged, I can create a PR if needed.