Fix num batches in case of multiple dataloaders and percent_check #1920

rohitgr7 · 2020-05-21T19:49:54Z

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 to update the docs?
Did you write any new necessary tests?
If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

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 🙃

rohitgr7 · 2020-05-21T19:51:10Z

Still needs to figure out why tests are failing. Need some help.

pytorch_lightning/trainer/evaluation_loop.py

awaelchli

in _reset_eval_dataloader you need to update the type hint for the retun to
Tuple[List[int], List[DataLoader]]:

pytorch_lightning/trainer/data_loading.py

pytorch_lightning/trainer/evaluation_loop.py

pytorch_lightning/trainer/data_loading.py

awaelchli

one more thing to make a test pass:
in tests/deprecated.py change max_batches=1 to max_batches=[1]

pytorch_lightning/trainer/trainer.py

pep8speaks · 2020-05-21T21:43:22Z

Hello @rohitgr7! Thanks for updating this PR.

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

Comment last updated at 2020-06-18 07:40:19 UTC

awaelchli · 2020-05-21T21:44:26Z

I will help fix the rest of the tests tomorrow

rohitgr7 · 2020-05-21T22:40:37Z

I will write new test cases tomorrow.

rohitgr7 · 2020-05-22T18:42:29Z

@awaelchli I was thinking to add just 1 test in which we will test both multiple val and test dataloaders with different sizes by just comparing num_batches for both. This is sufficient or should I add more tests and assertions??

awaelchli · 2020-05-22T18:54:01Z

Testing the num_batches for multiple with different sizes is good. However, a more important test imo is checking also that the dataloaders get consumed by that amount.
We could easily do this with a custom dataloader that just returns the index of the sample. Then we check how many samples we have drawn from each dataloader. What do you think?

rohitgr7 · 2020-05-22T20:13:41Z

@awaelchli Yeah, good idea. But can't figure out how to do that within tests.

awaelchli · 2020-05-25T20:35:30Z

I think the test is good and covers the base case, thanks for adding it! I will try to generalize it a bit more if you don't mind and post here.

awaelchli · 2020-05-26T19:53:54Z

hey @rohitgr7 I was able to parameterize the test and make it consistent for train, val, test.
Would you mind to merge this branch into yours and push it?
https://github.com/awaelchli/pytorch-lightning/tree/fix_num_batches
Then I think the PR is ready for review/merge :)

awaelchli · 2020-05-26T19:56:09Z

There is also a PR #1959 that adds support for multiple train dataloaders, so if that one gets merged first we need to make a few adjustments here :)

mergify · 2020-05-26T20:27:48Z

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

mergify · 2020-05-26T20:42:54Z

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

rohitgr7 · 2020-05-26T20:43:51Z

I made a small mistake with git there! Apologies.

awaelchli · 2020-05-26T20:47:06Z

yes I saw, no big deal, now we just need to fix this small merge conflict :)

rohitgr7 · 2020-05-26T21:59:52Z

Just a suggestion! Can we have *_percent_check to be a list too where len(*_percent_check) == len(*_dataloaders)? In case if it is int then it will be same for all the dataloaders passed. Don't know how this can be useful in any case, just a thought.

awaelchli · 2020-05-26T22:01:53Z

Not sure how useful that is, but how about you open an issue for that?

codecov · 2020-05-26T22:35:41Z

Codecov Report

❗ No coverage uploaded for pull request base (num_batches_missing_test@db84ca9). Click here to learn what that means.
The diff coverage is n/a.

@@                    Coverage Diff                     @@
##             num_batches_missing_test   #1920   +/-   ##
==========================================================
  Coverage                            ?     88%           
==========================================================
  Files                               ?      69           
  Lines                               ?    5249           
  Branches                            ?       0           
==========================================================
  Hits                                ?    4606           
  Misses                              ?     643           
  Partials                            ?       0

awaelchli · 2020-06-17T21:44:14Z

@williamFalcon @Borda Thanks, it's very kind you were able to add us as authors there. I already submitted a new PR #2226 with the missing test, because was not able to push here.

EDIT: sorry I don't understand all this co-author stuff, I hope I did not create a mixup.

Borda · 2020-06-17T21:54:18Z

we can still merge this one, @rohitgr7 🐰

rohitgr7 · 2020-06-17T23:00:35Z

@Borda Getting some problems with the tests here. Not sure why it's not showing any conflict to me locally when I rebase on the master. @awaelchli has already created a new PR that is working perfectly. I think we can close this one and merge that one.

pytorch_lightning/trainer/evaluation_loop.py

mergify · 2020-06-18T07:38:15Z

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

Borda · 2020-06-18T07:43:28Z

@rohitgr7 @awaelchli the simples solution from this situation is that I merger this PR to #2226 and then the other PR will be merged in master so this PR will have status merged for sats purposes and also this PR will be listed in changelog to be in release contributors list... hole all fine 🐰

rohitgr7 · 2020-06-18T07:55:38Z

Thanks @Borda for the fix.

@rohitgr7

* Init fix num_batches * Fix num_batches in case of multiple dataloaders * Apply suggestions from code review * Changes based on suggestions * Flake8 * Add test to check num_batches * generalize dataloader percent check test * fix formatting * remove hparams * tests * CHANGELOG * Update CHANGELOG.md * max_batches can be int * conflict and rebase * add back the test fix fix message 0.0 works Revert "fix message" This reverts commit 839cacf8b8610f4e697e654ef6f3d2501bf23984. * update changelog * Update CHANGELOG.md * Fix num batches in case of multiple dataloaders and percent_check (#1920) * git conflict Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com> * missing union * doc update suggestion by @rohitgr7 * extend test * changelog * docs add note about multiple loaders * update changelog * remove unused variable Co-authored-by: rohitgr7 <rohitgr1998@gmail.com> Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

mergify bot requested a review from a team May 21, 2020 19:50

rohitgr7 changed the title ~~Fix num batches~~ Fix num batches in case of multiple dataloaders and percent_check May 21, 2020

rohitgr7 commented May 21, 2020

View reviewed changes

pytorch_lightning/trainer/evaluation_loop.py Outdated Show resolved Hide resolved

Borda added the bug Something isn't working label May 21, 2020

awaelchli suggested changes May 21, 2020

View reviewed changes

mergify bot requested a review from a team May 21, 2020 20:58

awaelchli suggested changes May 21, 2020

View reviewed changes

pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team May 21, 2020 21:55

awaelchli changed the title ~~Fix num batches in case of multiple dataloaders and percent_check~~ [WIP] Fix num batches in case of multiple dataloaders and percent_check May 22, 2020

rohitgr7 requested review from awaelchli and removed request for a team May 25, 2020 18:53

mergify bot requested a review from a team May 25, 2020 18:53

awaelchli mentioned this pull request May 27, 2020

Separate *_percent_check for each *_dataloader #1964

Closed

rohitgr7 and others added 13 commits June 18, 2020 03:55

Fix num_batches in case of multiple dataloaders

8a1bb94

Apply suggestions from code review

14f434d

Changes based on suggestions

c616772

Flake8

8d58b4b

Add test to check num_batches

18e2746

generalize dataloader percent check test

4ca768c

fix formatting

eb3c164

remove hparams

1d2155a

tests

721556e

CHANGELOG

aa86e72

Update CHANGELOG.md

511c35a

max_batches can be int

cddc2d8

conflict and rebase

5e684ea

Borda reviewed Jun 18, 2020

View reviewed changes

pytorch_lightning/trainer/evaluation_loop.py Outdated Show resolved Hide resolved

git conflict

5f18d9a

mergify bot requested a review from a team June 18, 2020 07:34

Borda changed the base branch from master to num_batches_missing_test June 18, 2020 07:37

Merge branch 'num_batches_missing_test' into fix_num_batches

18c9581

Borda merged commit 4cc7223 into Lightning-AI:num_batches_missing_test Jun 18, 2020

This was referenced Jun 18, 2020

Increment port when taken #2140

Closed

Pid port + duplicate rank_zero logging #2231

Merged

Borda modified the milestones: 0.8.x, 0.8.0 Jun 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix num batches in case of multiple dataloaders and percent_check #1920

Fix num batches in case of multiple dataloaders and percent_check #1920

rohitgr7 commented May 21, 2020 •

edited

Loading

rohitgr7 commented May 21, 2020

awaelchli left a comment •

edited

Loading

awaelchli left a comment

pep8speaks commented May 21, 2020 •

edited

Loading

awaelchli commented May 21, 2020

rohitgr7 commented May 21, 2020

rohitgr7 commented May 22, 2020 •

edited

Loading

awaelchli commented May 22, 2020

rohitgr7 commented May 22, 2020 •

edited

Loading

awaelchli commented May 25, 2020

awaelchli commented May 26, 2020

awaelchli commented May 26, 2020

mergify bot commented May 26, 2020

mergify bot commented May 26, 2020

rohitgr7 commented May 26, 2020

awaelchli commented May 26, 2020

rohitgr7 commented May 26, 2020

awaelchli commented May 26, 2020

codecov bot commented May 26, 2020 •

edited

Loading

awaelchli commented Jun 17, 2020 •

edited

Loading

Borda commented Jun 17, 2020

rohitgr7 commented Jun 17, 2020

mergify bot commented Jun 18, 2020

Borda commented Jun 18, 2020

rohitgr7 commented Jun 18, 2020

Fix num batches in case of multiple dataloaders and percent_check #1920

Fix num batches in case of multiple dataloaders and percent_check #1920

Conversation

rohitgr7 commented May 21, 2020 • edited Loading

Before submitting

What does this PR do?

PR review

Did you have fun?

rohitgr7 commented May 21, 2020

awaelchli left a comment • edited Loading

Choose a reason for hiding this comment

awaelchli left a comment

Choose a reason for hiding this comment

pep8speaks commented May 21, 2020 • edited Loading

Comment last updated at 2020-06-18 07:40:19 UTC

awaelchli commented May 21, 2020

rohitgr7 commented May 21, 2020

rohitgr7 commented May 22, 2020 • edited Loading

awaelchli commented May 22, 2020

rohitgr7 commented May 22, 2020 • edited Loading

awaelchli commented May 25, 2020

awaelchli commented May 26, 2020

awaelchli commented May 26, 2020

mergify bot commented May 26, 2020

mergify bot commented May 26, 2020

rohitgr7 commented May 26, 2020

awaelchli commented May 26, 2020

rohitgr7 commented May 26, 2020

awaelchli commented May 26, 2020

codecov bot commented May 26, 2020 • edited Loading

Codecov Report

awaelchli commented Jun 17, 2020 • edited Loading

Borda commented Jun 17, 2020

rohitgr7 commented Jun 17, 2020

mergify bot commented Jun 18, 2020

Borda commented Jun 18, 2020

rohitgr7 commented Jun 18, 2020

rohitgr7 commented May 21, 2020 •

edited

Loading

awaelchli left a comment •

edited

Loading

pep8speaks commented May 21, 2020 •

edited

Loading

rohitgr7 commented May 22, 2020 •

edited

Loading

rohitgr7 commented May 22, 2020 •

edited

Loading

codecov bot commented May 26, 2020 •

edited

Loading

awaelchli commented Jun 17, 2020 •

edited

Loading