[RFC] Deprecate on_pretrain_routine_start and on_pretrain_routine_end LM/callback hooks

[daniellepintz]

Proposed refactor

Deprecate these hooks:
https://github.com/PyTorchLightning/pytorch-lightning/blob/6369e3b77fa3f38613b661517f6361f842f611c9/pytorch_lightning/trainer/trainer.py#L1261-L1269

Motivation

Users can use on_train_start, on_fit_start, or setup for the same purpose.

Additional Context

These hooks were originally added in #2850

I see about 30 usages of each hook using grep.app:
https://www.grep.app/search?q=on_pretrain_routine_start
https://www.grep.app/search?q=on_pretrain_routine_end

Part of #7740
Carries forward discussion #9043 on these hooks

cc @Borda @tchaton @rohitgr7 @carmocca @awaelchli @ninginthecloud @daniellepintz

[rohitgr7]

I have suggested some users over slack #questions to use these hooks for their use-case because on_train_start wasn't working for them. I can't remember what their use-case was.

Users can use on_train_start and on_train_end for the same purpose.

not sure if on_train_end is an alternative here for these hooks?

[ananthsub]

not sure if on_train_end is an alternative here for these hooks?

good catch. it should just be on_train_start

another weird thing with these hooks now is that there is nothing that happens between on_pretrain_routine_start and on_pretrain_routine_end, meaning at the very least, these hooks could be collapsed into one, if not fully deprecated. before, the model summary used to be printed here, but that was since refactored out to a callback. that callback could also run in on_train_start

similarly, this could be run in setup: https://github.com/PyTorchLightning/pytorch-lightning/blob/aeb0b5595fd73d086f4ae0f99d3f1f112f6a4c29/pytorch_lightning/callbacks/model_checkpoint.py#L257

[daniellepintz]

not sure if on_train_end is an alternative here for these hooks?

yeah good call, updated the description.

meaning at the very least, these hooks could be collapsed into one, if not fully deprecated.

yeah agreed. I think between on_train_start and setup we should be able to give users the same functionality. I searched in the slack #questions channel but was unable to find the users @rohitgr7 was referring to. Rohit, do you think setup would also have worked for the users who were having trouble with on_train_start?

[rohitgr7]

yeah agreed. I think between on_train_start and setup we should be able to give users the same functionality. I searched in the slack #questions channel but was unable to find the users @rohitgr7 was referring to. Rohit, do you think setup would also have worked for the users who were having trouble with on_train_start?

tbh I don't recall the actual use-case, I can't find them either since we have 10K limit for messages on slack. I am ok if these could be collapsed into one. I don't use this hook often. If everyone else agrees, I am ok with deprecation too.

[daniellepintz]

Ok, let's see what other people think about deprecation

[carmocca]

Looking at their position here

https://github.com/PyTorchLightning/pytorch-lightning/blob/6369e3b77fa3f38613b661517f6361f842f611c9/tests/models/test_hooks.py#L525-L528

I'd say on_fit_start is a more natural replacement.

I'm not sure at the moment whether they should be removed (after a deprecation ofc), but they sure could be collapsed into one hook.

This proposal is also missing links for

(1): The issue/PR that added them. You can use git blame to find it.
(2): Examples of public usage (or the lack of public usage). You can use www.grep.app for this

[daniellepintz]

This proposal is also missing links for
(1): The issue/PR that added them. You can use git blame to find it.
(2): Examples of public usage (or the lack of public usage). You can use www.grep.app for this

Added both to the issue. From the grep.app search, it does seem to me that these usages could be replaced by on_fit_start or setup.

[daniellepintz]

@tchaton what do you think about deprecating these hooks?

[tchaton]

Hey @daniellepintz. Great question. I can see Bolt and other repos use it, but we should incentivize for cleaner code. So I am in favor of the depreciation.

[carmocca]

Me too

[daniellepintz]

For anyone who wants to pick up this issue, here is an example PR to follow: #10940

[krishnakalyan3]

I would like to work on this

[daniellepintz]

@krishnakalyan3 Assigned to you, thanks!!

[daniellepintz]

Reopening since we still need to do the LM hooks