Skip to content

Conversation

@carmocca
Copy link
Contributor

@carmocca carmocca commented Jul 13, 2021

What does this PR do?

Adds ModelCheckpoint(save_on_train_epoch_end) which works just as EarlyStopping(check_on_train_epoch_end) and ModelPruning(prune_on_train_epoch_end)

The flag defaults to True if the Trainer(val_check_interval) flag is unchanged. This is because if you are running validation multiple times per training epoch, you probably want to save right after. Otherwise, saving on_train_epoch_end is the safer option as it is the last hook called so it's more likely the monitor will have been logged.

Part of #7724
Closes #6671

Does your PR introduce any breaking changes ? If yes, please list them.

If your workflow required running the ModelCheckpoint save exactly after validation ends, this will break it unless save_on_train_epoch_end=False is specified.

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)
  • Did you list all the breaking changes introduced by this pull request?

PR review

  • 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

@carmocca carmocca added feature Is an improvement or enhancement priority: 0 High priority task labels Jul 13, 2021
@carmocca carmocca added this to the v1.4 milestone Jul 13, 2021
@carmocca carmocca self-assigned this Jul 13, 2021
@pep8speaks
Copy link

pep8speaks commented Jul 13, 2021

Hello @carmocca! Thanks for updating this PR.

Line 306:13: W503 line break before binary operator

Comment last updated at 2021-07-13 14:16:08 UTC

@codecov
Copy link

codecov bot commented Jul 13, 2021

Codecov Report

Merging #8389 (101368d) into master (000fbe6) will decrease coverage by 1%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #8389   +/-   ##
======================================
- Coverage      93%     92%   -1%     
======================================
  Files         216     216           
  Lines       14115   14123    +8     
======================================
- Hits        13087   13023   -64     
- Misses       1028    1100   +72     

Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT !

@carmocca carmocca force-pushed the refactor/mc-save-on-train-epoch-end branch from 4aa95a6 to 277525a Compare July 13, 2021 14:15
@carmocca carmocca added the ready PRs ready to be merged label Jul 13, 2021
@carmocca carmocca enabled auto-merge (squash) July 13, 2021 14:16
@carmocca carmocca merged commit 321689f into master Jul 13, 2021
@carmocca carmocca deleted the refactor/mc-save-on-train-epoch-end branch July 13, 2021 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Is an improvement or enhancement priority: 0 High priority task ready PRs ready to be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants