Repository: Lightning-AI/pytorch-lightning
Pull request: Fix validation progress counter with check_val_every_n_epoch > 1
## What does this PR do?

<!--
IMPORTANT:
 We separated bug-fix PRs and feature PRs and they shall land in master and release/1.X-dev accordingly.
 By default all PR are targeted to master which is correct for bug-fixes, but need to be change for features.
 If you miss it we can still fix it for you, just ping us... :]

Please include a summary of the change and which issue is fixed.
 Please also include relevant motivation and context.
 List any dependencies that are required for this change.

If we didn't discuss your PR in Github issues there's a high chance it will not be merged.
-->

Fixes #5039

## Before submitting
- [ ] Was this discussed/approved via a GitHub issue? (not for typos and docs)
- [ ] Did you read the [contributor guideline](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/.github/CONTRIBUTING.md), **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](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/CHANGELOG.md)? (not for typos, docs, test updates, or internal minor changes/refactorings)

<!-- For CHANGELOG separate each item in the unreleased section by a blank line to reduce collisions -->

## 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](https://github.com/PyTorchLightning/pytorch-lightning/wiki/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 🙃


Problem statement:
validating bar keep showing 0 when check_val_every_n_epoch is bigger than 1
## 🐛 Bug

Normally, the validating bar should update according to the procedure of validation_step, but when I set check_val_every_n_epoch bigger than 1, 2 for example, it keeps showing 0it.

I encountered this issue under a terminal environment.



 - PyTorch 1.6.0:
 - Linux

Hints from issue discussion:
Hi! thanks for your contribution!, great first issue!
@ZhichaoDuan can you reproduce this using the BoringModel?
Same issue here. Setting check_val_every_n_epoch to 2 gives me a validation progress bar stuck at `Validating: 0it [00:00, ?it/s]`.

Perhaps related; the first epoch (either correctly or not depending on whether the Trainer should validate on epoch 0) shows the sum of train and validation batches, but never loops through the validation batches. E.g. I have 100/50 train/val batches, first epoch will show n/150 but will stop at 100. Second epoch will show the correct number (100).

This smells like an off-by-one error somewhere because after the validation loop finishes; the following epoch again shows the wrong batch number.

- PyTorch 1.7.0
- PyTorch Lightning 1.0.8
- Linux

EDIT:

Think I've found it:

https://github.com/PyTorchLightning/pytorch-lightning/blob/6d2aeff26ae20a3e76e6354f44c4d9c61ea4317a/pytorch_lightning/trainer/training_loop.py#L884

https://github.com/PyTorchLightning/pytorch-lightning/blob/89e8796e2a14429541f923b2ecf8fd7079c32a65/pytorch_lightning/callbacks/progress.py#L116

Progress bar is missing the +1 offset for the current epoch.
> @ZhichaoDuan can you reproduce this using the BoringModel?

Istefanou below have located the issue, could you take a look at it?
Anyone wants to try and submit a fix?
This issue has been automatically marked as stale because it hasn't had any recent activity. This issue will be closed in 7 days if no further activity occurs. Thank you for your contributions, Pytorch Lightning Team!

Why was this closed?

Commit message: add test

Patch to review:
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 21c52539a890d..ef5fdc8101045 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -17,7 +17,7 @@ default_language_version:
 
 repos:
   - repo: https://github.com/pre-commit/pre-commit-hooks
-    rev: v2.3.0
+    rev: v3.4.0
     hooks:
       - id: trailing-whitespace
       - id: end-of-file-fixer
@@ -35,6 +35,6 @@ repos:
         args: [--parallel, --in-place]
 
   - repo: https://github.com/pre-commit/mirrors-mypy
-    rev: v0.790
+    rev: v0.812
     hooks:
       - id: mypy
diff --git a/pytorch_lightning/callbacks/progress.py b/pytorch_lightning/callbacks/progress.py
index 74e57e2b5642e..489ef081bcc7c 100644
--- a/pytorch_lightning/callbacks/progress.py
+++ b/pytorch_lightning/callbacks/progress.py
@@ -149,9 +149,10 @@ def total_val_batches(self) -> int:
         validation dataloader is of infinite size.
         """
         total_val_batches = 0
-        if not self.trainer.disable_validation:
-            is_val_epoch = (self.trainer.current_epoch) % self.trainer.check_val_every_n_epoch == 0
+        if self.trainer.enable_validation:
+            is_val_epoch = (self.trainer.current_epoch + 1) % self.trainer.check_val_every_n_epoch == 0
             total_val_batches = sum(self.trainer.num_val_batches) if is_val_epoch else 0
+
         return total_val_batches
 
     @property
diff --git a/tests/trainer/flags/test_check_val_every_n_epoch.py b/tests/trainer/flags/test_check_val_every_n_epoch.py
new file mode 100644
index 0000000000000..e5d18e31c8837
--- /dev/null
+++ b/tests/trainer/flags/test_check_val_every_n_epoch.py
@@ -0,0 +1,56 @@
+# Copyright The PyTorch Lightning team.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+import pytest
+
+from pytorch_lightning.trainer import Trainer
+from pytorch_lightning.trainer.states import TrainerState
+from tests.helpers import BoringModel
+
+
+@pytest.mark.parametrize(
+    'max_epochs,expected_val_loop_calls,expected_val_batches', [
+        (1, 0, [0]),
+        (4, 2, [0, 2, 0, 2]),
+        (5, 2, [0, 2, 0, 2, 0]),
+    ]
+)
+def test_check_val_every_n_epoch(tmpdir, max_epochs, expected_val_loop_calls, expected_val_batches):
+
+    class TestModel(BoringModel):
+
+        def __init__(self):
+            super().__init__()
+            self.val_epoch_calls = 0
+            self.val_batches = []
+
+        def on_train_epoch_end(self, *args, **kwargs):
+            self.val_batches.append(self.trainer.progress_bar_callback.total_val_batches)
+
+        def on_validation_epoch_start(self) -> None:
+            self.val_epoch_calls += 1
+
+    model = TestModel()
+    trainer = Trainer(
+        default_root_dir=tmpdir,
+        max_epochs=max_epochs,
+        num_sanity_val_steps=0,
+        limit_val_batches=2,
+        check_val_every_n_epoch=2,
+        logger=False,
+    )
+    trainer.fit(model)
+    assert trainer.state == TrainerState.FINISHED, f"Training failed with {trainer.state}"
+
+    assert model.val_epoch_calls == expected_val_loop_calls
+    assert model.val_batches == expected_val_batches


Code review comments:
diff_hunk: @@ -17,7 +17,7 @@ default_language_version:
 
 repos:
   - repo: https://github.com/pre-commit/pre-commit-hooks
-    rev: v2.3.0
+    rev: v3.4.0
path: .pre-commit-config.yaml
text: @user1:
```suggestion
    rev: v2.3.0
```

diff_hunk: @@ -35,6 +35,6 @@ repos:
         args: [--parallel, --in-place]
 
   - repo: https://github.com/pre-commit/mirrors-mypy
-    rev: v0.790
+    rev: v0.812
path: .pre-commit-config.yaml
text: @user1:
```suggestion
    rev: v0.790
```