-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[warning] Add a warning with missing callback with resume_from_checkpoint #7254
Conversation
Hello @tchaton! Thanks for updating this PR. There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-04-29 12:13:09 UTC |
Codecov Report
@@ Coverage Diff @@
## master #7254 +/- ##
=======================================
+ Coverage 87% 91% +4%
=======================================
Files 199 199
Lines 12799 12802 +3
=======================================
+ Hits 11170 11683 +513
+ Misses 1629 1119 -510 |
"Be aware that when using ``resume_from_checkpoint``, " | ||
"callbacks used to create the checkpoint need to be provided. " | ||
f"Please, add the following callbacks: {list(difference)}. ", UserWarning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Be aware that when using ``resume_from_checkpoint``, " | |
"callbacks used to create the checkpoint need to be provided. " | |
f"Please, add the following callbacks: {list(difference)}. ", UserWarning | |
"Be aware that when using ``resume_from_checkpoint``," | |
" callbacks used to create the checkpoint need to be provided." | |
f" Please add the following callbacks: {list(difference)}. ", UserWarning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this warning always needed? For example, i may have saved a checkpoint dict which contained callback states from the model checkpoint. But when I resume from checkpoint, I might not set a new model checkpoint callback on the trainer
is there an issue/question which prompted the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR checks the AU~B
between save_callback_state
and current_callback_state
.
Basically, if you trained with a CustomCallback
and you don't provide it back when resuming
,
we won't be able to reload the state as this callback isn't provided.
Therefore we should provide a warning that resume
might not have the expected behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resume_from_checkpoint
is bringing lot of confusion as we expect exactly the same arguments to the Trainer as provided when training before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 😃 maybe just do set(callback_states.keys())
for readability
Co-authored-by: Ethan Harris <[email protected]>
What does this PR do?
Add a warning with missing callback with resume_from_checkpoint
Before submitting
PR review
Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃