Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[skip ci] Allow ModelCheckpoint in Trainer callbacks list #3990

Closed
wants to merge 10 commits into from

Conversation

awaelchli
Copy link
Contributor

@awaelchli awaelchli commented Oct 8, 2020

Allows passing ModelCheckpoint directly to the callbacks list:

Trainer(callbacks=[ModelCheckpoint(...)])

This was not possible before because save_function was not set. We now set it in the callback directly.

The following cases need to be considered:

  • Trainer(checkpoint_callback=True, callbacks=[ModelCheckpoint()])
  • Trainer(checkpoint_callback=False, callbacks=[ModelCheckpoint()])
  • Trainer(checkpoint_callback=ModelCheckpoint(), callbacks=[ModelCheckpoint()])

This PR ignores the checkpoint_callback setting if one is passed to callbacks. Does this make sense @PyTorchLightning/core-contributors ?

TODO:

  • needs a test
  • need to define trainer.checkpoint_callback, we probably need to limit it to 1 checkpoint callback for now?

@awaelchli awaelchli added checkpointing Related to checkpointing feature Is an improvement or enhancement labels Oct 8, 2020
@codecov
Copy link

codecov bot commented Oct 8, 2020

Codecov Report

Merging #3990 into master will decrease coverage by 47%.
The diff coverage is 82%.

@@           Coverage Diff            @@
##           master   #3990     +/-   ##
========================================
- Coverage      89%     42%    -47%     
========================================
  Files         212     212             
  Lines       15562   15702    +140     
========================================
- Hits        13867    6623   -7244     
- Misses       1695    9079   +7384     

@awaelchli
Copy link
Contributor Author

The tests currently fail as they reveal another bug, which I will fix here: #4027

@stale
Copy link

stale bot commented Oct 23, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need further help see our docs: https://pytorch-lightning.readthedocs.io/en/latest/CONTRIBUTING.html#pull-request or ask the assistance of a core contributor here or on Slack. Thank you for your contributions.

@stale stale bot added the won't fix This will not be worked on label Oct 23, 2020
@ydcjeff ydcjeff removed the won't fix This will not be worked on label Oct 23, 2020
@awaelchli awaelchli deleted the feature/chkpt-callback-arg branch October 30, 2020 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
checkpointing Related to checkpointing feature Is an improvement or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants