Support limit_mode_batches (int) for infinite dataloader #2787

rohitgr7 · 2020-08-01T11:11:52Z

What does this PR do?

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 🙃

rohitgr7 · 2020-08-01T11:23:54Z

pytorch_lightning/trainer/data_loading.py

+                        raise MisconfigurationException(
+                            'When using an infinite DataLoader (e.g. with an IterableDataset'
+                            f' or when DataLoader does not implement `__len__`) for `limit_{mode}_batches`,'
+                            f' `Trainer(limit_{mode}_batches)` must be `0.0`, `1.0` or `int`')


How can limit_mode_batches = 0.0 or 1.0 for infinite dataloader?

As far as I remember, a value of 0.0 should just run nothing and a value of 1.0 should run until the dataloader stops. If it is an actually infinite dataloader then it would just never end, so perhaps this should be written differently?

yeah, as of now I just moved some if-else statements and with 0.0 it was neither working before nor it is working now. I can change it as per requirement. But just like you said I agree it should work with both 0.0 and int and otherwise raise this exception with an updated statement here.

and not just 1.0 but with every 0.0 < limit_mode_batches <= 1.0 it should raise this exception.

@awaelchli what do you suggest here? I'll update and add tests accordingly.

We have the same for val_check_interval=1.0 (but we don't print a warning), where we distinguish between 1.0 (float, 100% of epoch) and 1 (int, batch count). I think it is reasonable to differentiate using the type and the docs make it very clear with the examples. Regarding this exception message, I think it should be changed to "When using an IterableDataset (e.g. of infinite size ...". Or just not refer to inf dataset as Ethan suggested.

and for non-iterable datasets should 0.0 be supported with inf dataloader?

and separate exceptions for Iterable and inf dataset? For Iterable 1.0 is supported and for inf dataloader 0.0 and int is supported?

can you clarify. what would that look like? I don't know of any inf datasets that are not of type IterableDataset.

Neither do I, never seen one. To he honest, I didn't even know about IterableDataset before this point 😅. Well I guess, I'll revert recent commits and change the exception message as suggested by Ethan.

codecov · 2020-08-01T13:07:30Z

Codecov Report

Merging #2787 into master will increase coverage by 4%.
The diff coverage is 85%.

@@           Coverage Diff           @@
##           master   #2787    +/-   ##
=======================================
+ Coverage      86%     90%    +4%     
=======================================
  Files          78      78            
  Lines        7001    7001            
=======================================
+ Hits         6033    6317   +284     
+ Misses        968     684   -284

pep8speaks · 2020-08-02T20:48:45Z

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-08-05 16:40:43 UTC

mergify · 2020-08-04T17:46:21Z

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

rohitgr7 · 2020-08-04T19:08:44Z

Should I add supporting docs here? https://pytorch-lightning.readthedocs.io/en/stable/sequences.html?highlight=IterableDataset#iterable-datasets

awaelchli

nice, also the tests 🚀

pytorch_lightning/trainer/data_loading.py

tests/trainer/test_dataloaders.py

CHANGELOG.md

mergify · 2020-08-05T09:34:34Z

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

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

ethanwharris

Awesome, looks good to me :) can clean up the last elif slightly

pytorch_lightning/trainer/data_loading.py

mergify · 2020-08-05T12:48:30Z

Great job! =)

Co-authored-by: Ethan Harris <ewah1g13@soton.ac.uk>

docs/source/sequences.rst

pytorch_lightning/core/lightning.py

pytorch_lightning/trainer/data_loading.py

Borda · 2020-08-05T13:23:52Z

pytorch_lightning/trainer/data_loading.py

+        if isinstance(self.limit_train_batches, int) or self.limit_train_batches == 0.0:
+            self.num_training_batches = min(self.num_training_batches, int(self.limit_train_batches))
+        elif self.num_training_batches != float('inf'):
+            self.num_training_batches = int(self.num_training_batches * self.limit_train_batches)


can it be also 5.1 meaning 510% ?
cc: @PyTorchLightning/core-contributors

@Borda I don't think so. We iterate like this:

# run epoch for batch_idx, (batch, is_last_batch) in self.profiler.profile_iterable( enumerate(_with_is_last(train_dataloader)), "get_train_batch" ): # stop epoch if we limited the number of training batches if batch_idx >= self.num_training_batches: break

so if the loader is exhausted before, this would trigger an stop iteration meaning that the condition will never be True.

still, it would be cleaner to have there val = min(val, 1.0)

we can do _check_batch_limits there to avoid floating values > 1.0

mergify · 2020-08-05T17:04:53Z

Great job! =)

rohitgr7 · 2020-08-05T17:07:32Z

@Borda can we unmerge it there is some irrelevant code I just deleted locally and need to push it? or should I create a new pr with updated changes?
Duplicate code:
https://github.com/PyTorchLightning/pytorch-lightning/blob/bef27c58eda4c4425c8aa750d38e16522bfcbe39/pytorch_lightning/trainer/data_loading.py#L106-L119
https://github.com/PyTorchLightning/pytorch-lightning/blob/bef27c58eda4c4425c8aa750d38e16522bfcbe39/pytorch_lightning/trainer/trainer.py#L1433-L1441

Borda · 2020-08-05T19:49:37Z

@Borda can we unmerge it there is some irrelevant code I just deleted locally and need to push it? or should I create a new pr with updated changes?

yes, create a new PR and refer that it it fix to this one, lest get it done asap

…)" This reverts commit de9c9f0.

) * Revert "Support limit_mode_batches (int) for infinite dataloader (#2787)" This reverts commit de9c9f0. * Update training_tricks.py

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

rohitgr7 commented Aug 1, 2020

View reviewed changes

rohitgr7 force-pushed the fix_limit_batches_infdl branch from d0f0f32 to acbe573 Compare August 2, 2020 20:48

rohitgr7 requested review from ethanwharris and awaelchli August 2, 2020 21:08

rohitgr7 changed the title ~~[WIP] Support limit_mode_batches (int) for infinite dataloader~~ Support limit_mode_batches (int) for infinite dataloader Aug 3, 2020

Borda added allowed_pre_1.0 feature Is an improvement or enhancement labels Aug 4, 2020

rohitgr7 force-pushed the fix_limit_batches_infdl branch from 139139f to 40253e1 Compare August 4, 2020 17:52

awaelchli approved these changes Aug 5, 2020

View reviewed changes

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

tests/trainer/test_dataloaders.py Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

mergify bot requested a review from a team August 5, 2020 06:01

justusschock approved these changes Aug 5, 2020

View reviewed changes

mergify bot requested a review from a team August 5, 2020 08:13

rohitgr7 and others added 9 commits August 5, 2020 16:38

Support limit_mode_batches(int) for infinite dataloader

1d88a45

flake8

34c2fe2

revert and update

7cebf71

add and update tests

9afb9a3

pep8

a89b2bb

chlog

2a78fc3

Update CHANGELOG.md

c649932

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

Add suggestions by @awaelchli

71d75ea

docs

8322e1a

rohitgr7 force-pushed the fix_limit_batches_infdl branch from df71791 to 8322e1a Compare August 5, 2020 11:10

ethanwharris approved these changes Aug 5, 2020

View reviewed changes

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

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

Merge branch 'master' into fix_limit_batches_infdl

87717fd

Apply suggestions from code review

c032f99

Co-authored-by: Ethan Harris <ewah1g13@soton.ac.uk>

Borda requested changes Aug 5, 2020

View reviewed changes

Apply suggestions from code review

baa88fb

Borda approved these changes Aug 5, 2020

View reviewed changes

Borda and others added 3 commits August 5, 2020 17:04

fix

4145298

max

dea47b1

check

0246e3c

mergify bot merged commit de9c9f0 into master Aug 5, 2020

Borda deleted the fix_limit_batches_infdl branch August 5, 2020 19:47

williamFalcon added a commit that referenced this pull request Aug 5, 2020

Revert "Support limit_mode_batches (int) for infinite dataloader (#2787…

c7a0f60

…)" This reverts commit de9c9f0.

williamFalcon mentioned this pull request Aug 5, 2020

Revert "Support limit_mode_batches (int) for infinite dataloader" #2839

Merged

williamFalcon added a commit that referenced this pull request Aug 5, 2020

Revert "Support limit_mode_batches (int) for infinite dataloader" (#2839

5d0f032

) * Revert "Support limit_mode_batches (int) for infinite dataloader (#2787)" This reverts commit de9c9f0. * Update training_tricks.py

rohitgr7 mentioned this pull request Aug 5, 2020

Support limit_mode_batches (int) for infinite dataloader #2840

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support limit_mode_batches (int) for infinite dataloader #2787

Support limit_mode_batches (int) for infinite dataloader #2787

rohitgr7 commented Aug 1, 2020 •

edited

Loading

rohitgr7 Aug 1, 2020

ethanwharris Aug 1, 2020

rohitgr7 Aug 1, 2020 •

edited

Loading

rohitgr7 Aug 1, 2020 •

edited

Loading

rohitgr7 Aug 2, 2020

awaelchli Aug 2, 2020 •

edited

Loading

rohitgr7 Aug 2, 2020

rohitgr7 Aug 2, 2020

awaelchli Aug 2, 2020

rohitgr7 Aug 2, 2020

codecov bot commented Aug 1, 2020 •

edited

Loading

pep8speaks commented Aug 2, 2020 •

edited

Loading

mergify bot commented Aug 4, 2020

rohitgr7 commented Aug 4, 2020

awaelchli left a comment

mergify bot commented Aug 5, 2020

ethanwharris left a comment

mergify bot commented Aug 5, 2020

Borda Aug 5, 2020

justusschock Aug 5, 2020

Borda Aug 5, 2020

rohitgr7 Aug 5, 2020

mergify bot commented Aug 5, 2020

rohitgr7 commented Aug 5, 2020 •

edited

Loading

Borda commented Aug 5, 2020

Support limit_mode_batches (int) for infinite dataloader #2787

Support limit_mode_batches (int) for infinite dataloader #2787

Conversation

rohitgr7 commented Aug 1, 2020 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohitgr7 Aug 1, 2020 • edited Loading

Choose a reason for hiding this comment

rohitgr7 Aug 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awaelchli Aug 2, 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

codecov bot commented Aug 1, 2020 • edited Loading

Codecov Report

pep8speaks commented Aug 2, 2020 • edited Loading

Comment last updated at 2020-08-05 16:40:43 UTC

mergify bot commented Aug 4, 2020

rohitgr7 commented Aug 4, 2020

awaelchli left a comment

Choose a reason for hiding this comment

mergify bot commented Aug 5, 2020

ethanwharris left a comment

Choose a reason for hiding this comment

mergify bot commented Aug 5, 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 5, 2020

rohitgr7 commented Aug 5, 2020 • edited Loading

Borda commented Aug 5, 2020

rohitgr7 commented Aug 1, 2020 •

edited

Loading

rohitgr7 Aug 1, 2020 •

edited

Loading

rohitgr7 Aug 1, 2020 •

edited

Loading

awaelchli Aug 2, 2020 •

edited

Loading

codecov bot commented Aug 1, 2020 •

edited

Loading

pep8speaks commented Aug 2, 2020 •

edited

Loading

rohitgr7 commented Aug 5, 2020 •

edited

Loading