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

Deprecate callback hooks on_pretrain_routine_start and on_pretrain_routine_end #11794

Merged
merged 43 commits into from
Feb 24, 2022
Merged

Deprecate callback hooks on_pretrain_routine_start and on_pretrain_routine_end #11794

merged 43 commits into from
Feb 24, 2022

Conversation

krishnakalyan3
Copy link
Contributor

@krishnakalyan3 krishnakalyan3 commented Feb 7, 2022 •
edited
Loading

What does this PR do?

Fixes #10984

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

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 🙃

@krishnakalyan3 krishnakalyan3 marked this pull request as draft February 7, 2022 15:50
carmocca
carmocca reviewed Feb 7, 2022
View reviewed changes
pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
pytorch_lightning/core/hooks.py Outdated Show resolved Hide resolved
ananthsub
ananthsub reviewed Feb 7, 2022
View reviewed changes
pytorch_lightning/callbacks/lambda_function.py Outdated Show resolved Hide resolved
pytorch_lightning/callbacks/model_summary.py Outdated Show resolved Hide resolved
pytorch_lightning/core/hooks.py Outdated Show resolved Hide resolved
pytorch_lightning/core/hooks.py Outdated Show resolved Hide resolved
@ananthsub ananthsub added callback deprecation Includes a deprecation labels Feb 7, 2022
@krishnakalyan3
Copy link
Contributor Author

krishnakalyan3 commented Feb 7, 2022 •
edited
Loading

Thanks for the feedback gents. I am not sure what is expected in pytorch_lightning/callbacks/model_summary.py.

Should the signature be changed to setup or on_train_start?

cc: @ananthsub @carmocca

@ananthsub ananthsub changed the title Deprecate callback hooks on_init_start and on_init_end Deprecate callback hooks on_pretrain_routine_start and on_pretrain_routine_end Feb 7, 2022
@ananthsub
Copy link
Contributor

Thanks for the feedback gents. I am not sure what is expected in pytorch_lightning/callbacks/model_summary.py.

Should the signature be changed to setup or on_train_start?

cc: @ananthsub @carmocca

This logic needs to happen after the model is setup and after the checkpoint state is loaded. Either on_fit_start or on_train_start will work. on_train_start is less ambiguous to me than on_fit_start, which is why I recommended using it in my comment above.

@daniellepintz
Copy link
Contributor

Will you make a separate PR for deprecating the LM hooks?

@krishnakalyan3
Copy link
Contributor Author

krishnakalyan3 commented Feb 9, 2022 •
edited
Loading

Yes. @daniellepintz I will make a separate PR for LM hooks.

@mergify mergify bot removed the has conflicts label Feb 19, 2022
@mergify mergify bot removed the has conflicts label Feb 24, 2022
@mergify mergify bot added the ready PRs ready to be merged label Feb 24, 2022
@daniellepintz daniellepintz added this to the 1.6 milestone Feb 24, 2022
@carmocca carmocca enabled auto-merge (squash) February 24, 2022 15:24
@daniellepintz
Copy link
Contributor

daniellepintz commented Feb 24, 2022 •
edited
Loading

@krishnakalyan3 please address the last comments and then this will auto-merge. Are you still planning on making the PR for the LM hooks as well?

@Borda Borda requested a review from daniellepintz February 24, 2022 16:48
Borda
Borda approved these changes Feb 24, 2022
View reviewed changes
pytorch_lightning/callbacks/base.py Outdated Show resolved Hide resolved
pytorch_lightning/callbacks/base.py Outdated Show resolved Hide resolved
rohitgr7 and others added 2 commits February 24, 2022 22:40
@rohitgr7 @Borda @daniellepintz
Apply suggestions from code review
75da9fa 
Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: Danielle Pintz <[email protected]>
@rohitgr7 @Borda
Update pytorch_lightning/callbacks/base.py
41f5489 
Co-authored-by: Jirka Borovec <[email protected]>
@daniellepintz
Copy link
Contributor

Could someone please rerun TPU tests?

@carmocca carmocca merged commit 29d5afb into Lightning-AI:master Feb 24, 2022
@krishnakalyan3
Copy link
Contributor Author

Thank you @rohitgr7

@ananthsub ananthsub added the breaking change Includes a breaking change label Mar 4, 2022
@ananthsub
Copy link
Contributor

ananthsub commented Mar 4, 2022 •
edited
Loading

This is a breaking change because we changed callback implementations. The ModelCheckpoint callback in particular can be extended. By moving the logic from on_pretrain_routine_start to setup, child classes that extend this now call setup which may not be expected.

@daniellepintz
Copy link
Contributor

@ananthsub I'm curious if you see a better way we could have gone about this in this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rohitgr7 rohitgr7 rohitgr7 approved these changes

@daniellepintz daniellepintz daniellepintz approved these changes

@carmocca carmocca carmocca approved these changes

@Borda Borda Borda approved these changes

@ananthsub ananthsub ananthsub approved these changes

@awaelchli awaelchli Awaiting requested review from awaelchli

@kaushikb11 kaushikb11 Awaiting requested review from kaushikb11

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

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

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

@edenlightning edenlightning Awaiting requested review from edenlightning

Assignees
No one assigned
Labels
breaking change Includes a breaking change callback deprecation Includes a deprecation ready PRs ready to be merged
Projects
None yet
Milestone
pl:1.6
Development

Successfully merging this pull request may close these issues.

[RFC] Deprecate on_pretrain_routine_start and on_pretrain_routine_end LM/callback hooks
7 participants
@krishnakalyan3 @ananthsub @daniellepintz @Borda @carmocca @tchaton @rohitgr7