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

[3/3] Update lightning callbacks to Stateful, deprecations for old on_save/load_checkpoint signatures #11887

Merged
merged 59 commits into from
Mar 25, 2022

Conversation

jjenniferdai
Copy link
Contributor

@jjenniferdai jjenniferdai commented Feb 11, 2022

What does this PR do?

Note this currently includes the changes in part [2/n] (PR #12232) until that can be merged - review here for parts 3 and 4 only.

part of / Fixes #11429: introduce Stateful Callbacks (see Appendix code changes outlined in issue)
This part:

    1. updates lightning callbacks to Stateful
    1. add deprecation warnings for old on_save/load_checkpoint signatures

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

see CHANGELOG "changed" section: removed lightning Callback.on_save/load_checkpoint methods in favor of state_dict/load_state_dict for the callbacks listed under Labels: early stopping, finetuning, model checkpoint, pruning, timer.

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 list all the breaking changes introduced by this pull request?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • 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

Did you have fun?

Make sure you had fun coding 🙃

Copy link
Contributor

@ananthsub ananthsub left a comment

Choose a reason for hiding this comment

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

Once you have a proof of concept working, I'd strongly recommend splitting this into a few different PRs:

  1. Add state_dict/load_state_dict to the base Callback
  2. Add support for this inside of the checkpoint connector/trainer
  3. Change relevant callback implementations to use state_dict/load_state_dict instead

This will be much easier to review for merging

pytorch_lightning/callbacks/lambda_function.py Outdated Show resolved Hide resolved
@ananthsub ananthsub added callback checkpointing Related to checkpointing labels Feb 12, 2022
@ananthsub ananthsub added this to the 1.6 milestone Feb 12, 2022
@jjenniferdai jjenniferdai changed the title [wip] Introduce Stateful Callbacks Introduce Stateful Callbacks Feb 17, 2022
@jjenniferdai
Copy link
Contributor Author

jjenniferdai commented Feb 17, 2022

edit: was missing \( for parentheses

re: test failure, not sure what I'm missing here, they exactly match?

Failed: DID NOT WARN. No warnings of type (<class 'DeprecationWarning'>, <class 'PendingDeprecationWarning'>) matching ('Method Callback.on_load_checkpoint(callback_state) is deprecated in v1.6 and will be removed in v1.8. Please use Callback.load_state_dict instead, or new method signature Callback.on_load_checkpoint_new(checkpoint).') were emitted. The list of emitted warnings is: [LightningDeprecationWarning('Method Callback.on_load_checkpoint(callback_state) is deprecated in v1.6 and will be removed in v1.8. Please use Callback.load_state_dict instead, or new method signature Callback.on_load_checkpoint_new(checkpoint).')]

@mergify mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Mar 22, 2022
@mergify mergify bot added has conflicts and removed ready PRs ready to be merged labels Mar 23, 2022
@mergify mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Mar 24, 2022
@jjenniferdai jjenniferdai modified the milestones: 1.7, 1.6 Mar 24, 2022
@ananthsub ananthsub enabled auto-merge (squash) March 24, 2022 18:52
auto-merge was automatically disabled March 24, 2022 23:22

Head branch was pushed to by a user without write access

@mergify mergify bot added has conflicts and removed ready PRs ready to be merged labels Mar 24, 2022
@mergify mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Mar 24, 2022
@ananthsub ananthsub enabled auto-merge (squash) March 24, 2022 23:35
@ananthsub ananthsub merged commit d4a4b77 into Lightning-AI:master Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] Standardize all stateful components on state_dict/load_state_dict
4 participants