Add on_epoch_start to run at the beginning of every loop irrespective of train/val/test

[rohitgr7]

What does this PR do?

Added on_epoch_start to run at the beginning of every loop irrespective of train/val/test. This hook is the common one that can let users do somethings required to be done in case of all train/val/test. Otherwise we already have on_train/val/test_epoch_start hooks if something needs to be done separately for train/val/test separately.

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

Did you have fun?

Make sure you had fun coding 🙃

[rohitgr7]

quick question here:
https://github.com/PyTorchLightning/pytorch-lightning/blob/680e83adab38c2d680b138bdc39d48fc35c0cb58/pytorch_lightning/trainer/evaluation_loop.py#L120-L124
https://github.com/PyTorchLightning/pytorch-lightning/blob/680e83adab38c2d680b138bdc39d48fc35c0cb58/pytorch_lightning/trainer/evaluation_loop.py#L314-L321

on_epoch_end is called after every epoch irrespective of train/val/test but this Is not the case with on_epoch_start. Is this intended behavior or missed? let me know, I'll update the PR and tests accordingly if required ✌️

cc @carmocca @tchaton

[codecov]

Codecov Report

Merging #6498 (37e0c88) into master (dcd9dd8) will decrease coverage by 45%.
The diff coverage is 80%.

@@           Coverage Diff            @@
##           master   #6498     +/-   ##
========================================
- Coverage      93%     47%    -45%     
========================================
  Files         161     161             
  Lines       11518   11413    -105     
========================================
- Hits        10661    5374   -5287     
- Misses        857    6039   +5182     

[carmocca]

Is this intended behavior or missed? let me know, I'll update the PR and tests accordingly if required ✌️

I'd say missed. I don't see any reason to have it act differently

[tchaton]

LGTM ! Is it called only once for training ?

[awaelchli]

Can we make a section "backward incompatible changes" in the release notes, like pytorch does it?

This PR is an example where the changelog entry is not enough

[rohitgr7]

Can we make a section "backward incompatible changes" in the release notes, like pytorch does it?

This PR is an example where the changelog entry is not enough

should I add a section for it in the changelog? @awaelchli

[awaelchli]

No I believe these release notes are a special section on github that gets created separately. At least it looks like it :) @Borda knows

[Borda]

No I believe these release notes are a special section on github that gets created separately. At least it looks like it :) @Borda knows

not sure what means incompatible change, we shall always be at least 0.2 version compatible with any past API
so the process is deprecation and it is written in the Deprecation section for a particular version v0.X
then when it is truly removed in v0.(X+2) we write it again in the Removed section... 🐰

[rohitgr7]

No I believe these release notes are a special section on github that gets created separately. At least it looks like it :) @Borda knows

not sure what means incompatible change, we shall always be at least 0.2 version compatible with any past API
so the process is deprecation and it is written in the Deprecation section for a particular version v0.X
then when it is truly removed in v0.(X+2) we write it again in the Removed section... rabbit

@Borda here nothing is being removed/deprecated. Only the behavior is changed since it was missed in the past when its counterpart (on_epoch_end) was updated. So guess @awaelchli is suggesting a simple changelog entry might not be enough here.

[Borda]

maybe I am missing something, can we update the PR description?
I see some rename from on_train_epoch_start but is not mentioned anywhere...
also, shall the genric on_epoch_start has a phase argument?

[ananthsub]

These generic on_epoch_start and on_epoch_end hooks cause a lot of confusion for users who don't know how an epoch is defined in lightning. nor do they account for the validation being run multiple times within the train epoch when val_check_interval is configured.

the existing hooks are explicit as to when they're applied. if anything, i think we should remove the generic on_epoch_end hook and ask users to share code across the existing train/val/test epoch end hooks

Being more explicit in the naming is also a motivation for #6420

[rohitgr7]

the existing hooks are explicit as to when they're applied. if anything, i think we should remove the generic on_epoch_end hook and ask users to share code across the existing train/val/test epoch end hooks

exactly what I suggested on slack but William suggested making it generic to run irrespective of train/val/test. Although another way around for a user to create such generic logic can be done via a separate hook they can define themselves.

def _common_epoch_start(self, ...):
    # do stuff

def on_train_epoch_start(self, ...):
    self._common_epoch_start(()

def on_val_epoch_start(self, ...):
    self._common_epoch_start(()

def on_test_epoch_start(self, ...):
    # do something else

or we can just make them on_loop_start (at least better than epoch IMO) if required.

I just updated this the way on_epoch_end behaves currently on the master.