Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix overfit_batches when using with multiple val/test_dataloaders #2792

Closed
wants to merge 16 commits into from
Closed

Fix overfit_batches when using with multiple val/test_dataloaders #2792

wants to merge 16 commits into from

Conversation

ydcjeff
Copy link
Contributor

@ydcjeff ydcjeff commented Aug 1, 2020 •
edited
Loading

What does this PR do?

Fixes #2532
Fixes #2325 (I think)

Colab to reproduce issue: https://colab.research.google.com/drive/1BtQBCoP5fK-aZm_2uLMOUbf2c9cu-yFb?usp=sharing
Colab for this PR: https://colab.research.google.com/drive/1nzVh8xeEGLOJvSZ0Ih7WccNHgI1gDzaD?usp=sharing

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 🙃

@mergify mergify bot requested a review from a team August 1, 2020 14:49
@codecov
Copy link

codecov bot commented Aug 1, 2020

Codecov Report

Merging #2792 into master will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #2792   +/-   ##
======================================
- Coverage      91%     90%   -0%     
======================================
  Files          76      76           
  Lines        6787    6793    +6     
======================================
- Hits         6150    6135   -15     
- Misses        637     658   +21

@ydcjeff ydcjeff changed the title Fixed overfit_batches when using with multiple val_dataloaders and test_dataloaders Fixed overfit_batches when using with multiple *_dataloaders Aug 5, 2020
@Borda Borda added this to the 0.9.0 milestone Aug 6, 2020
@ydcjeff ydcjeff changed the title Fixed overfit_batches when using with multiple *_dataloaders Fixed overfit_batches when using with multiple {val,test}_dataloaders Aug 7, 2020
@ydcjeff
Copy link
Contributor Author

ydcjeff commented Aug 7, 2020

It is ready to review.

@Borda Borda added allowed_pre_1.0 bug Something isn't working labels Aug 7, 2020
@awaelchli awaelchli modified the milestones: 0.9.0, 1.0.0 Aug 8, 2020
@mergify mergify bot requested a review from a team August 12, 2020 14:02
@ydcjeff ydcjeff changed the title Fixed overfit_batches when using with multiple {val,test}_dataloaders [wip] Fixed overfit_batches when using with multiple {val,test}_dataloaders Aug 14, 2020
@mergify
Copy link
Contributor

mergify bot commented Aug 15, 2020

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

@Borda Borda removed this from the 1.0.0 milestone Aug 20, 2020
@Borda Borda added this to the 0.9.x milestone Aug 20, 2020
@pep8speaks
Copy link

pep8speaks commented Sep 22, 2020 •
edited
Loading

Hello @ydcjeff! Thanks for updating this PR.

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

Comment last updated at 2020-09-22 16:54:03 UTC

@ydcjeff ydcjeff changed the title [wip] Fixed overfit_batches when using with multiple {val,test}_dataloaders Fixed overfit_batches when using with multiple {val,test}_dataloaders Sep 22, 2020
@ydcjeff
Copy link
Contributor Author

ydcjeff commented Sep 22, 2020

Ready to review.

One thing this PR can't cover is if there is

  • overfit_batches > 0
  • only one val/test dataloaders
  • dataloader_idx argument in validation/test_step

Then, it will continue training with overfit_batches, will not show argument error.
I don't know how to cover that.

awaelchli
awaelchli suggested changes Sep 22, 2020
View reviewed changes
pytorch_lightning/trainer/evaluation_loop.py Outdated
output = self.trainer.accelerator_backend.test_step(args)
else:
output = self.trainer.accelerator_backend.validation_step(args)
except TypeError:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if the typeerror is coming from some other bug? it will make it almost impossible to debug this because an error will occur later, unrelated to this part of the code here. can we not just check if step takes multiple dataloaders?
related: #2266

Copy link
Contributor Author

@ydcjeff ydcjeff Sep 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only have access to dataloader_idx, val_dataloader and test_dataloader become train_dataloader if there is overfit_batches

@mergify mergify bot requested a review from a team September 22, 2020 12:39
@ydcjeff ydcjeff marked this pull request as draft September 22, 2020 17:03
@ydcjeff ydcjeff changed the title Fixed overfit_batches when using with multiple {val,test}_dataloaders Fixed overfit_batches when using with multiple val/test_dataloaders Sep 23, 2020
@ydcjeff ydcjeff changed the title Fixed overfit_batches when using with multiple val/test_dataloaders [WIP] Fixed overfit_batches when using with multiple val/test_dataloaders Sep 23, 2020
@ydcjeff ydcjeff marked this pull request as ready for review September 23, 2020 09:02
@ydcjeff
Copy link
Contributor Author

ydcjeff commented Sep 23, 2020

This issue can be fixed if we pass dataloader_idx=None in validation_step and test_step.
Anyway, dataloader_idx is passed with value internally.
So, I think docs update would be enough for this?

How do you think?

@Borda Borda changed the title [WIP] Fixed overfit_batches when using with multiple val/test_dataloaders Fix overfit_batches when using with multiple val/test_dataloaders Sep 25, 2020
@mergify
Copy link
Contributor

mergify bot commented Sep 29, 2020

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

@edenlightning edenlightning modified the milestones: 0.9.x, 1.0 Oct 4, 2020
williamFalcon added a commit that referenced this pull request Oct 5, 2020
@williamFalcon
Fixes #2792
6bf95f2
@williamFalcon williamFalcon mentioned this pull request Oct 5, 2020
Merged
@ydcjeff ydcjeff closed this Oct 5, 2020
@ydcjeff ydcjeff deleted the trainer/overfit-multi-val-dset branch October 5, 2020 03:04
williamFalcon added a commit that referenced this pull request Oct 5, 2020
@williamFalcon
Fixes #2792 (#3857)
d787208
@Borda Borda modified the milestones: 1.0, 0.10.0 Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@williamFalcon williamFalcon williamFalcon left review comments

@awaelchli awaelchli awaelchli requested changes

@ananyahjha93 ananyahjha93 Awaiting requested review from ananyahjha93

@nateraw nateraw Awaiting requested review from nateraw

@rohitgr7 rohitgr7 Awaiting requested review from rohitgr7

@SkafteNicki SkafteNicki Awaiting requested review from SkafteNicki

Assignees

@nateraw nateraw

Labels
bug Something isn't working
Projects
None yet
Milestone
pl:0.10
7 participants
@ydcjeff @pep8speaks @williamFalcon @awaelchli @Borda @nateraw @edenlightning