Fix val_progress_bar total with num_sanity_val_steps

[rohitgr7]

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?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[Borda]

LGTM

[awaelchli]

Could you briefly explain why we need the new bool flag after refactors and not before? Thanks

[rohitgr7]

@awaelchli I don't remember what was the workflow before 😅 but since both sanity check and validation uses the same val_progress_bar, for sanity check it calls the on_validation_start hook in run_evaluation which re-registers the val_progress_bar with the number of validation batches. Partial workflow explained here.

[codecov]

Codecov Report

Merging #3751 into master will increase coverage by 4%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #3751    +/-   ##
=======================================
+ Coverage      85%     89%    +4%     
=======================================
  Files         110     110            
  Lines        8539    8711   +172     
=======================================
+ Hits         7295    7792   +497     
+ Misses       1244     919   -325     

[williamFalcon]

Please don't add another variable...

use: if trainer.running_sanity_check:

[awaelchli]

nice

[SkafteNicki]

LGTM

[rohitgr7]

@williamFalcon is it good now??

[williamFalcon]

much better... but are those tests dropped now? for checking iterable loaders?

[rohitgr7]

Never added tests for iterable datasets here. Those are already present in the tests. This is just for tqdm progress bar total which was not working correctly for sanity check after refs.

[williamFalcon]

These lines. wondering why the are deleted

[rohitgr7]

@williamFalcon these are redundant lines now since you removed some assertions here: b9f4e7c, not sure why. But these were important tests for sanity checks. Should I add them back with the updated API call in this PR if required?

[williamFalcon]

ok got it. i see, thanks

[awaelchli]

@williamFalcon these are redundant lines now since you removed some assertions here: b9f4e7c, not sure why. But these were important tests for sanity checks. Should I add them back with the updated API call in this PR if required?

yes please, if these were removed by accident, we need to add the assertions back.
These checks make sure that num sanity val steps lead to the correct number of steps with multiple dataloaders.

[rohitgr7]

yes please, if these were removed by accident, we need to add the assertions back.
These checks make sure that num sanity val steps lead to the correct number of steps with multiple dataloaders.

ok will add them back ✌️