Separate epoch validation from step validation

[rohitgr7]

What does this PR do?

Fixes #4603
Fixes #4655
Fixes #4797
Fixes #5156

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• 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? Otherwise, we ask you to create a separate PR for every change.
• Did you make sure to update the documentation with your changes?
• Did you write any new necessary tests?
• Did you verify new and existing tests pass locally with your changes?
• If you made a notable change (that affects users), did you update the CHANGELOG?

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; Bugfixes should be including in bug-fix release milestones (m.f.X) and features should be included in (m.X.b) releases.

Did you have fun?

Make sure you had fun coding 🙃

[pep8speaks]

Hello @rohitgr7! Thanks for updating this PR.

• In the file pytorch_lightning/callbacks/model_checkpoint.py:

Line 234:13: W503 line break before binary operator

• In the file pytorch_lightning/trainer/training_loop.py:

Line 592:17: W503 line break before binary operator
Line 593:17: W503 line break before binary operator
Line 895:13: W503 line break before binary operator
Line 896:13: W503 line break before binary operator
Line 899:13: W503 line break before binary operator

Comment last updated at 2021-02-08 08:00:12 UTC

[codecov]

Codecov Report

Merging #5208 (b0608b0) into master (3b7afb9) will decrease coverage by 4%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #5208    +/-   ##
=======================================
- Coverage      93%     89%    -4%     
=======================================
  Files         134     134            
  Lines       10053   10051     -2     
=======================================
- Hits         9399    8980   -419     
- Misses        654    1071   +417     

[carmocca]

Minor comments. Overall awesome!

[tchaton]

Great PR ! Small question.

[tchaton]

Slightly confused about this part. Can you explain why we check val and then decide if we should skip it.

[rohitgr7]

it's just to check whether there is any validation datasets available or not. If there isn't then we should run train_only_check else not. There are two cases for no validation, one when there is no validation_step other one when there is a validation_step but no validation_batches. Since even if we have a validation_step but no validation_batches, it used to skip the train_only_check, but ideally it should not.
Resolves: #4603 issue

[ananthsub]

@rohitgr7 I'm debugging an issue right now related to this.

pytorch_lightning/trainer/training_loop.py:753: Input params: batch_idx=1, is_last_batch=True, on_epoch=True
pytorch_lightning/trainer/training_loop.py:755: batch_idx+1=2, trainer.val_check_batch=2, is_val_check_batch=True
pytorch_lightning/trainer/training_loop.py:758: current_epoch+1=1, trainer.check_val_every_n_epoch=1, is_val_check_epoch=True
pytorch_lightning/trainer/training_loop.py:761: enable_validation=True, is_val_check_epoch=True, can_check_val=True
pytorch_lightning/trainer/training_loop.py:765: is_last_batch=True, trainer.val_check_batch=2, is_last_batch_for_infinite_dataset=False
pytorch_lightning/trainer/training_loop.py:768: batch_idx + 1=2, trainer.num_training_batches=2, epoch_end_val_check=True
pytorch_lightning/trainer/training_loop.py:774: is_val_check_batch=True, is_val_check_epoch=True, can_check_val=True, is_last_batch_for_infinite_dataset=False, epoch_end_val_check=True, should_check_val=True
pytorch_lightning/trainer/training_loop.py:775: should_check_val=True, can_check_val=True
pytorch_lightning/trainer/training_loop.py:487: should_check_val=True
pytorch_lightning/trainer/training_loop.py:489: should_skip_eval=True, trainer.num_val_batches=[]

this check for should_skip_eval is forcing the should_train_only to be True, which causes the checkpoint callback to run before validation. The checkpoint is configured for a metric that appears only in validation, which leads to a failure. I don't get why should_skip_eval affects the should_train_only - shouldn't that be decided entirely by self.trainer.disable_validation ?

this could also be pointing to a bug in how self.trainer.num_val_batches is set

[Borda]

I think that @williamFalcon had a point some time ago about training shall be till validation, and the example was with validation multiple times over long training...
cc: @tchaton @PyTorchLightning/core-contributors

[rohitgr7]

yes it still works like that only if val_check_interval < 1.0 or it an int where val_check_interval % num_training_batches != 0. But if it is set to 1.0 then validation here happens after training_epoch because we create checkpoints in on_validation_end and epoch level learning rates are updated once training is done since in case of ReduceLROnPlateau we need to have the monitor metrics and they are only available after complete training is done in case monitor is training specific.

.