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.

Sign up for GitHub

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

Jump to bottom

Add "monitor" to saved ModelCheckpoints #4383

Merged
merged 6 commits into from
Oct 28, 2020
Merged

Add "monitor" to saved ModelCheckpoints #4383

merged 6 commits into from
Oct 28, 2020

Conversation

carmocca
Copy link
Contributor

@carmocca carmocca commented Oct 27, 2020 •
edited
Loading

What does this PR do?

Adds "monitor" to saved ModelCheckpoints
Removes CHECKPOINT_STATE_BEST_{SCORE,PATH}. I think these were forgotten after a checkpoint connector refactor. They are unused now.

I don't think we have to load the monitor in on_load_checkpoint.

Fixes #4368

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?

PR review

  • Is this pull request ready for review? (if not, please submit in draft mode)

@mergify mergify bot requested a review from a team October 27, 2020 02:55
@ydcjeff ydcjeff added the feature Is an improvement or enhancement label Oct 27, 2020
@ydcjeff ydcjeff added this to the 1.1 milestone Oct 27, 2020
@mergify mergify bot requested a review from a team October 27, 2020 08:57
@codecov
Copy link

codecov bot commented Oct 27, 2020 •
edited
Loading

Codecov Report

Merging #4383 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #4383   +/-   ##
======================================
  Coverage      92%     92%           
======================================
  Files         111     111           
  Lines        8260    8260           
======================================
  Hits         7560    7560           
  Misses        700     700

@carmocca carmocca changed the title Add "best_model_monitor" to saved ModelCheckpoints Add "monitor" to saved ModelCheckpoints Oct 27, 2020
@carmocca
Copy link
Contributor Author

Changed the key to monitor. This is ready to go 😄

@rohitgr7 rohitgr7 added the checkpointing Related to checkpointing label Oct 27, 2020
@mergify mergify bot requested a review from a team October 27, 2020 14:38
@rohitgr7 rohitgr7 merged commit 00cc69a into Lightning-AI:master Oct 28, 2020
@carmocca carmocca deleted the feature/4368_best-model-monitor branch October 28, 2020 13:55
@Borda Borda modified the milestones: 1.1, 1.0.x Nov 3, 2020
@edenlightning edenlightning modified the milestones: 1.0.x, 1.1 Nov 4, 2020
SeanNaren pushed a commit that referenced this pull request Nov 10, 2020
@carmocca
Add "monitor" to saved ModelCheckpoints (#4383)
6fbd9f4 
* Add key

* Remove unused variables

* Update CHANGELOG [skip ci]

* best_model_monitor -> monitor

Co-authored-by: Sean Naren <[email protected]>
Co-authored-by: Rohit Gupta <[email protected]>

(cherry picked from commit 00cc69a)
@daandres daandres mentioned this pull request Dec 4, 2020
Closed
@carmocca carmocca self-assigned this Nov 1, 2023
@mergify mergify bot added the ready PRs ready to be merged label Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awaelchli awaelchli awaelchli left review comments

@ananthsub ananthsub ananthsub approved these changes

@SeanNaren SeanNaren SeanNaren approved these changes

@justusschock justusschock justusschock approved these changes

@SkafteNicki SkafteNicki SkafteNicki approved these changes

@rohitgr7 rohitgr7 rohitgr7 approved these changes

@s-rog s-rog s-rog approved these changes

@ananyahjha93 ananyahjha93 Awaiting requested review from ananyahjha93

@Borda Borda Awaiting requested review from Borda Borda is a code owner

@nateraw nateraw Awaiting requested review from nateraw

@tchaton tchaton Awaiting requested review from tchaton tchaton is a code owner

@teddykoker teddykoker Awaiting requested review from teddykoker

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

Assignees

@carmocca carmocca

Labels
checkpointing Related to checkpointing feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Milestone
pl:1.1
Development

Successfully merging this pull request may close these issues.

Add best_model_monitor to ModelCheckpoint