[WIP] Fix the progress bar for the sanity check #2892

manipopopo · 2020-08-09T10:17:51Z

The original progress bar will always show trainer.num_sanity_val_steps as val_progress_bar.total even if the length of the validation DataLoader is less than trainer.num_sanity_val_steps.

The pytorch_lightning.trainer.data_loading._has_len is changed to a public function has_len, which is called by pytorch_lightning.callbacks.progress.ProgressBar.

Import pytorch_lightning.trainer.data_loading from pytorch_lightning.callbacks.progress will lead to circular imports.
Maybe we could move pytorch_lightning.trainer.data_loading._has_len to other place.

Or we could save the sizes of validation (and train) DataLoaders as members of Trainer, which may be accessed by pytorch_lightning.callbacks.progress.ProgressBar.

What does this PR do?

Fixes #2891

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?

pep8speaks · 2020-08-09T10:17:55Z

Hello @manipopopo! Thanks for updating this PR.

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

Comment last updated at 2020-08-14 17:35:58 UTC

codecov · 2020-08-09T11:39:53Z

Codecov Report

Merging #2892 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #2892   +/-   ##
======================================
  Coverage      85%     85%           
======================================
  Files          82      82           
  Lines        7719    7719           
======================================
  Hits         6550    6550           
  Misses       1169    1169

pytorch_lightning/callbacks/progress.py

tests/callbacks/test_progress_bar.py

mergify · 2020-08-11T13:57:19Z

This pull request is now in conflict... :(

tests/callbacks/test_progress_bar.py

tests/utilities/test_data.py

awaelchli

good fix!

rohitgr7 · 2020-08-11T15:04:45Z

pytorch_lightning/callbacks/progress.py

+        self.val_progress_bar.total = sum(
+            min(trainer.num_sanity_val_steps, len(d) if has_len(d) else float('inf')) for d in trainer.val_dataloaders
+        )


@awaelchli num_sanity_val_steps should be independent of limit_val_batches(float)?

if num_sanity_val_steps=2, len(val_dataloader)=10 and limit_val_batches=0.1, should it run for 2 val_steps or 1?

is this relevant here? I thought this pr is just about displaying the num_sanity steps that the trainer returns.
if limit_val_batches is used, it should just truncate the sanity steps if needed, no? This should happen in the trainer I think.

Yeah it still has some issues with limit_val_batches and I think a better fix would be to set up num_sanity_val_steps as a list in Trainer itself rather than doing it here, and simple we can do a sum to get total sanity val steps.

Does that means

When num_sanity_val_steps == -1:
https://github.com/PyTorchLightning/pytorch-lightning/blob/0097630a95bddc48d6fb5d3b9a58aef2e8e89b22/tests/trainer/test_trainer.py#L802-L813

limit_val_batches != 0: run len(val_dataloader) (could be inf) steps. (independent)

limit_val_batches == 0, it shouldn't run any step. (dependent)

When num_sanity_val_steps >= 0, the number of check steps should be affected by limit_val_batches.(dependent)

I suggest in case of num_sanity_val_steps == -1 it should be affected by limit_val_batches too.

@rohitgr7 I like your suggestions. It is true, the trainer should compute these properties and the progress bars should only read them (and maybe sum them).

Should I open another PR or keep this PR going? Should we use the same num_sanity_val_steps to save these values? (#2891 (comment))

I am already working on it :)

mergify · 2020-08-11T23:29:15Z

This pull request is now in conflict... :(

pytorch_lightning/callbacks/progress.py

mergify · 2020-08-13T21:59:20Z

Great job! =)

mergify · 2020-08-13T22:45:14Z

Great job! =)

The original progress bar will always show trainer.num_sanity_val_steps even if the length of the validation DataLoader is less than trainer.num_sanity_val_steps. The pytorch_lightning.trainer.data_loading._has_len is changed to a public function has_len, which is called by pytorch_lightning/callbacks/progress.py

mergify · 2020-08-14T01:55:37Z

This pull request is now in conflict... :(

justusschock · 2020-08-14T06:47:38Z

pytorch_lightning/callbacks/progress.py

@@ -293,7 +294,9 @@ def init_test_tqdm(self) -> tqdm:
    def on_sanity_check_start(self, trainer, pl_module):
        super().on_sanity_check_start(trainer, pl_module)
        self.val_progress_bar = self.init_sanity_tqdm()
-        self.val_progress_bar.total = convert_inf(trainer.num_sanity_val_steps * len(trainer.val_dataloaders))
+        self.val_progress_bar.total = sum(
+            min(trainer.num_sanity_val_steps, len(d) if has_len(d) else float('inf')) for d in trainer.val_dataloaders


this is a quite common case, can't we add a function for this like

def len_or_default(to_be_checked: Any, default_length: int = int('inf')): if has_len(to_be_checked): return len(to_be_checked) return default_length

This may be an overhead now, but we really need similar things quite often

I believe this is a repeated code here. This is already done in reset_val_dataloader. All we need is just to sum num_sanity_val_steps here once #2917 is fixed.

agree with both of you. should we block this PR with 2917 or the other way around? Does it matter which one goes first?

I suggest block this one. Once I get some answers there I asked, I'll fix that one tonight and then we can complete this one :)

mergify · 2020-08-14T17:50:28Z

Great job! =)

mergify · 2020-08-16T01:46:22Z

This pull request is now in conflict... :(

mergify · 2020-08-21T18:13:06Z

This pull request is now in conflict... :(

rohitgr7 · 2020-08-21T18:19:54Z

@manipopopo I think now we can finish this :)

manipopopo · 2020-08-22T04:06:32Z

Hi @rohitgr7 , it seems that #2917 has fixed the issue. Should we close this PR?

ananyahjha93 · 2020-08-26T17:26:13Z

@manipopopo closing this then
@rohitgr7 just make sure if #2891 is really fixed.

* Follow up of #2892 * typo * iterabledataset

mergify bot requested a review from a team August 9, 2020 10:18

ananyahjha93 self-requested a review August 9, 2020 11:08

awaelchli self-requested a review August 10, 2020 16:28

Borda added the bug Something isn't working label Aug 11, 2020

Borda requested changes Aug 11, 2020

View reviewed changes

pytorch_lightning/callbacks/progress.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team August 11, 2020 09:22

awaelchli suggested changes Aug 11, 2020

View reviewed changes

tests/callbacks/test_progress_bar.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team August 11, 2020 13:34

awaelchli reviewed Aug 11, 2020

View reviewed changes

tests/callbacks/test_progress_bar.py Outdated Show resolved Hide resolved

tests/utilities/test_data.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team August 11, 2020 14:12

awaelchli approved these changes Aug 11, 2020

View reviewed changes

mergify bot requested a review from a team August 11, 2020 14:29

rohitgr7 reviewed Aug 11, 2020

View reviewed changes

mergify bot requested a review from a team August 11, 2020 15:05

rohitgr7 mentioned this pull request Aug 11, 2020

Int num_sanity_val_steps is always replaced by float limit_val_batches #2882

Closed

ananyahjha93 force-pushed the fix_sanity_check_progress_bar_total branch from 21a1489 to 8920d69 Compare August 11, 2020 15:09

Borda self-requested a review August 11, 2020 15:35

Borda approved these changes Aug 11, 2020

View reviewed changes

mergify bot requested a review from a team August 11, 2020 15:38

Borda added this to the 0.9.0 milestone Aug 11, 2020

SkafteNicki approved these changes Aug 12, 2020

View reviewed changes

ananyahjha93 force-pushed the fix_sanity_check_progress_bar_total branch from 7f8751a to ed25a6b Compare August 13, 2020 21:16

ananyahjha93 approved these changes Aug 13, 2020

View reviewed changes

pytorch_lightning/callbacks/progress.py Outdated Show resolved Hide resolved

manipopopo and others added 6 commits August 13, 2020 19:56

Fix W504 line break after binary operator

0a9342a

Move functions to pytorch_lightning.utilities.data

f87074f

Simplify test cases

8b1fde4

Update CHANGELOG

a6be809

rename

8041aa9

removed pep8 issue

117c1fe

ananyahjha93 force-pushed the fix_sanity_check_progress_bar_total branch from e691bcc to 117c1fe Compare August 13, 2020 23:56

doc fix

5987308

justusschock self-requested a review August 14, 2020 06:44

justusschock approved these changes Aug 14, 2020

View reviewed changes

ananyahjha93 and others added 2 commits August 14, 2020 13:35

doc

e978a58

Merge branch 'master' into fix_sanity_check_progress_bar_total

03191e7

rohitgr7 changed the title ~~Fix the progress bar for the sanity check~~ [WIP] [Blocked by 2917] Fix the progress bar for the sanity check Aug 14, 2020

awaelchli mentioned this pull request Aug 19, 2020

Runtime Error if validation_step is defined, but valid_loader isn't provided to Trainer #3052

Closed

edenlightning modified the milestones: 0.9.0, 0.9.x Aug 20, 2020

rohitgr7 changed the title ~~[WIP] [Blocked by 2917] Fix the progress bar for the sanity check~~ [WIP] Fix the progress bar for the sanity check Aug 21, 2020

rohitgr7 added a commit that referenced this pull request Aug 26, 2020

Follow up of #2892

a645542

rohitgr7 mentioned this pull request Aug 26, 2020

Follow up of #2892 #3202

Merged

7 tasks

ananyahjha93 closed this Aug 26, 2020

ananyahjha93 pushed a commit that referenced this pull request Aug 27, 2020

Follow up of #2892 (#3202)

85cd558

* Follow up of #2892 * typo * iterabledataset

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Fix the progress bar for the sanity check #2892

[WIP] Fix the progress bar for the sanity check #2892

manipopopo commented Aug 9, 2020 •

edited

Loading

pep8speaks commented Aug 9, 2020 •

edited

Loading

codecov bot commented Aug 9, 2020 •

edited

Loading

mergify bot commented Aug 11, 2020

awaelchli left a comment

rohitgr7 Aug 11, 2020

rohitgr7 Aug 11, 2020

awaelchli Aug 11, 2020 •

edited

Loading

rohitgr7 Aug 11, 2020 •

edited

Loading

manipopopo Aug 11, 2020

rohitgr7 Aug 11, 2020

awaelchli Aug 11, 2020

manipopopo Aug 11, 2020

rohitgr7 Aug 11, 2020

manipopopo Aug 11, 2020

mergify bot commented Aug 11, 2020

mergify bot commented Aug 13, 2020

mergify bot commented Aug 13, 2020

mergify bot commented Aug 14, 2020

justusschock Aug 14, 2020

rohitgr7 Aug 14, 2020

awaelchli Aug 14, 2020

rohitgr7 Aug 14, 2020

mergify bot commented Aug 14, 2020

mergify bot commented Aug 16, 2020

mergify bot commented Aug 21, 2020

rohitgr7 commented Aug 21, 2020

manipopopo commented Aug 22, 2020

ananyahjha93 commented Aug 26, 2020

[WIP] Fix the progress bar for the sanity check #2892

[WIP] Fix the progress bar for the sanity check #2892

Conversation

manipopopo commented Aug 9, 2020 • edited Loading

What does this PR do?

Before submitting

pep8speaks commented Aug 9, 2020 • edited Loading

Comment last updated at 2020-08-14 17:35:58 UTC

codecov bot commented Aug 9, 2020 • edited Loading

Codecov Report

mergify bot commented Aug 11, 2020

awaelchli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awaelchli Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

rohitgr7 Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Aug 11, 2020

mergify bot commented Aug 13, 2020

mergify bot commented Aug 13, 2020

mergify bot commented Aug 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Aug 14, 2020

mergify bot commented Aug 16, 2020

mergify bot commented Aug 21, 2020

rohitgr7 commented Aug 21, 2020

manipopopo commented Aug 22, 2020

ananyahjha93 commented Aug 26, 2020

manipopopo commented Aug 9, 2020 •

edited

Loading

pep8speaks commented Aug 9, 2020 •

edited

Loading

codecov bot commented Aug 9, 2020 •

edited

Loading

awaelchli Aug 11, 2020 •

edited

Loading

rohitgr7 Aug 11, 2020 •

edited

Loading