Change the seq of on_train_batch_end, on_batch_end & on_train_epoch_end, on_epoch_end hooks

[kaushikb11]

What does this PR do?

Fixes #5042

The idea is to have a sequence of hooks ending from specific to generic.

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 update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

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:

• 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
• Check that target branch and milestone match!

Did you have fun?

Make sure you had fun coding 🙃

[pep8speaks]

Hello @kaushikb11! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-01-28 08:30:22 UTC

[codecov]

Codecov Report

Merging #5688 (50fd487) into release/1.2-dev (3da28fd) will decrease coverage by 1%.
The diff coverage is 55%.

@@               Coverage Diff                @@
##           release/1.2-dev   #5688    +/-   ##
================================================
- Coverage               89%     88%    -1%     
================================================
  Files                  168     181    +13     
  Lines                12423   12890   +467     
================================================
+ Hits                 11103   11339   +236     
- Misses                1320    1551   +231     

[rohitgr7]

I'd suggest we should deprecate these 2 hooks and remove them in the future. They are doing nothing different.

[Borda]

I'd suggest we should deprecate these 2 hooks and remove them in the future. They are doing nothing different.

thoughts? @PyTorchLightning/core-contributors

[kaushikb11]

I'd suggest we should deprecate these 2 hooks and remove them in the future. They are doing nothing different.

thoughts? @PyTorchLightning/core-contributors

Will had made a valid point of what if we want to do something on batch end across all three steps? We would have to duplicate the code in three methods.

[ananyahjha93]

@Borda @kaushikb11 @rohitgr7 I can think of use cases for these hooks, so its better not to deprecate them.

[rohitgr7]

I'd suggest we should deprecate these 2 hooks and remove them in the future. They are doing nothing different.

thoughts? @PyTorchLightning/core-contributors

Will had made a valid point of what if we want to do something on batch end across all three steps? We would have to duplicate the code in three methods.

one can still do:

def _on_epoch_end_common(...):
    ...

def on_train_epoch_end(...):
   self._on_epoch_end_common(...)..

def on_validation_epoch_end(...):
   self._on_epoch_end_common(...)

def on_test_epoch_end(...):
   self._on_epoch_end_common(...)

we can configure above by allowing on_epoch_end irrespective of the running_stage, but that is just one case. If some one wants to make the behavior same in case of train & val only, one has to use the above code structure.

my point here mostly users think of epoch as something that happens during training since epoch means nothing in case of validation/test. I have already seen many issues raised w.r.t to the difference between on_validation_epoch_end and on_validation_end and adding one more might create more confusion.

But if everyone thinks this is a good option, not a problem since I don't use this hook anymore ✌️

[rohitgr7]

@Borda @kaushikb11 @rohitgr7 I can think of use cases for these hooks, so its better not to deprecate them.

@ananyahjha93 mind share a use-case?

[ananyahjha93]

@rohitgr7 on_validation_epoch_end is ModelHook and on_validation_end is from callbacks so they definitely serve two different purposes.

Keeping both on_epoch_end and on_train_epoch_end if say I just have to plot GAN generated images at the end of each train, val and test loops, I can do that with on_epoch_end instead of repeating my code.

[rohitgr7]

@rohitgr7 on_validation_epoch_end is ModelHook and on_validation_end is from callbacks so they definitely serve two different purposes.

Keeping both on_epoch_end and on_train_epoch_end if say I just have to plot GAN generated images at the end of each train, val and test loops, I can do that with on_epoch_end instead of repeating my code.

@ananyahjha93 it's a callback hook too: https://github.com/PyTorchLightning/pytorch-lightning/blob/50fd4879a9d4f8b83ace27ee580b7633d4746361/pytorch_lightning/callbacks/base.py#L86

both the hooks are there in Callback as well as LightningModule.

I am just a little bit concerned about logging too since we allow logging in these hooks.

[ananyahjha93]

@rohitgr7 I have code in swav which utilizes both callback and model hook version of this function. So, deprecating one of them will probably change a lot of code not just in our repositories but other people's repositories as well.

Callbacks I would say serve a slightly different purpose at times than just calling the same function code in ModelHook's version of that method.

[rohitgr7]

yeah valid point, but one has to update the code even if we change it's behavior now.

[ananyahjha93]

@rohitgr7 the reordering ideally should not change any code. Deprecating this would probably.

[rohitgr7]

@rohitgr7 the reordering ideally should not change any code. Deprecating this would probably.

I meant changing it's behavior to run irrespective of train/val/test will require a code change. Re-ordering is not a problem.

[s-rog]

@kaushikb11 Training hooks changes LGTM, do validation hooks have a similar ordering issue?

[tchaton]

Hey @kaushikb11, great work !

[rohitgr7]

should we reorder on_epoch_start/on_batch_start too? currently it's

on_epoch_start
on_train_epoch_start
...
...
on_train_epoch_end
on_epoch_end

same for on_batch_start

[kaushikb11]

@rohitgr7 We could move the discussion for on_spoch_start hook in the Issues.