[DataPipe] Align shuffling behavior for IterDataPipe and MapDataPipe by ejguan · Pull Request #82974 · pytorch/pytorch

ejguan · 2022-08-08T15:30:05Z

Stack from ghstack:

[DataLoader] Add shuffling logic for MapDataPipe #82975 [DataLoader] Add shuffling logic for MapDataPipe
[DataPipe] Align shuffling behavior for IterDataPipe and MapDataPipe #82974 [DataPipe] Align shuffling behavior for IterDataPipe and MapDataPipe

This PR align the shuffling behavior of two types of DataPipe:

Shuffling will be lazily executed only after __iter__ or __getitem__ is invoked.
[Discussion] seed will be fixed unless set_seed is invoked.
- BC breaking for IterDataPipe as the original behavior would a seed will be generated whenever iter is invoked, but now each iterator would share the same seed until set_seed is invoked.
Tests exist as test_shuffler_iterdatapipe and test_shuffler_mapdatapipe

[ghstack-poisoned]

facebook-github-bot · 2022-08-08T15:30:12Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/82974
📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓Need help or want to give feedback on the CI? Visit our office hours

✅ No Failures (0 Pending)

As of commit 64eedd0 (more details on the Dr. CI page):

Expand to see more

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

…apDataPipe" [ghstack-poisoned]

…apDataPipe" This PR align the shuffling behavior of two types of `DataPipe`: - Shuffling will be lazily executed only after `__iter__` or `__getitem__` is invoked. - [Discussion] `seed` will be fixed unless `set_seed` is invoked. - BC breaking for `IterDataPipe` as the original behavior would a seed will be generated whenever `iter` is invoked, but now each `iterator` would share the same seed until `set_seed` is invoked. - Tests exist as `test_shuffler_iterdatapipe` and `test_shuffler_mapdatapipe` [ghstack-poisoned]

NivekT

Overall, I agree that we should align the shuffle behaviors between two styles of DataPipe.

Letting DataLoader control this behavior makes sense. At the expense of UX of people who don't use DataPipes with DataLoader for whatever reason.

I do wonder if we should allow users to toggle this behavior (same or different seed with new iterator) on and off.
We need to update all the docstring and documentation to make sure that the behavior is clear to users.

torch/utils/data/datapipes/iter/combinatorics.py

NivekT · 2022-08-09T20:41:56Z

test/test_datapipe.py

            len(dp1)

-    def test_shuffle_iterdatapipe(self):
+    def test_shuffler_iterdatapipe(self):


Nit: Maybe add a test in the # No seed section to ensure that that order of the output is the same in two epoch even without manually resetting the seed?

Do we have tests that use the set_seed API? Maybe the current integrated test with DataLoader is sufficient since DL uses set_seed.

Let me re-construct the whole tests for shuffle.

torch/utils/data/datapipes/map/combinatorics.py

ejguan · 2022-08-10T20:38:43Z

Thinking for a while on MapDataPipe.shuffle today, I find it becomes anti-intuitive to me that Shuffler is a still a MapDataPipe. We normally refer MapDataPipe as we provide an index to this instance, it gives a corresponding object. However, shuffling would make it so weird that a non-corresponding object is returned given an index. I personally think it makes more sense that MapDataPipe should be converted to an IterDataPipe after shuffling.

And, after making sure both shufflers are IterDataPipe, I am able to align the behavior of them to always update new shuffled order per iteration. I will open a separate PR for it #83202

cc: @NivekT @VitalyFedyunin

…apDataPipe" This PR align the shuffling behavior of two types of `DataPipe`: - Shuffling will be lazily executed only after `__iter__` or `__getitem__` is invoked. - [Discussion] `seed` will be fixed unless `set_seed` is invoked. - BC breaking for `IterDataPipe` as the original behavior would a seed will be generated whenever `iter` is invoked, but now each `iterator` would share the same seed until `set_seed` is invoked. - Tests exist as `test_shuffler_iterdatapipe` and `test_shuffler_mapdatapipe` [ghstack-poisoned]

ejguan · 2022-08-15T19:20:37Z

Closing this PR as we all agreed on moving MapDataPipe.shuffle to an IterDataPipe during an offline discussion.

…IterDataPipe" Fixes: meta-pytorch/data#718 This is an alternative PR against #82974 This PR would change the behavior for both types to the same behavior as `IterDataPipe.shuffle` - Lazily generating seed per iteration - Each iterators has a new seed However, this requires `MapDataPipe.shuffle` is converted as a `IterDataPipe`. I prefer this option simply because I find the concept of shuffling `MapDataPipe` would be shuffling indices and returning the data in the order of shuffled indices. [ghstack-poisoned]

Fixes: meta-pytorch/data#718 This is an alternative PR against #82974 This PR would change the behavior for both types to the same behavior as `IterDataPipe.shuffle` - Lazily generating seed per iteration - Each iterators has a new seed However, this requires `MapDataPipe.shuffle` is converted as a `IterDataPipe`. I prefer this option simply because I find the concept of shuffling `MapDataPipe` would be shuffling indices and returning the data in the order of shuffled indices. [ghstack-poisoned]

…IterDataPipe" Fixes: meta-pytorch/data#718 This is an alternative PR against #82974 This PR would change the behavior for both types to the same behavior as `IterDataPipe.shuffle` - Lazily generating seed per iteration - Each iterators has a new seed However, this requires `MapDataPipe.shuffle` is converted as a `IterDataPipe`. I prefer this option simply because I find the concept of shuffling `MapDataPipe` would be shuffling indices and returning the data in the order of shuffled indices. [ghstack-poisoned]

Fixes: meta-pytorch/data#718 This is an alternative PR against #82974 This PR would change the behavior for both types to the same behavior as `IterDataPipe.shuffle` - Lazily generating seed per iteration - Each iterators has a new seed However, this requires `MapDataPipe.shuffle` is converted as a `IterDataPipe`. I prefer this option simply because I find the concept of shuffling `MapDataPipe` would be shuffling indices and returning the data in the order of shuffled indices. [ghstack-poisoned]

…IterDataPipe" Fixes: meta-pytorch/data#718 This is an alternative PR against #82974 This PR would change the behavior for both types to the same behavior as `IterDataPipe.shuffle` - Lazily generating seed per iteration - Each iterators has a new seed However, this requires `MapDataPipe.shuffle` is converted as a `IterDataPipe`. I prefer this option simply because I find the concept of shuffling `MapDataPipe` would be shuffling indices and returning the data in the order of shuffled indices. [ghstack-poisoned]

Fixes: meta-pytorch/data#718 This is an alternative PR against #82974 This PR would change the behavior for both types to the same behavior as `IterDataPipe.shuffle` - Lazily generating seed per iteration - Each iterators has a new seed However, this requires `MapDataPipe.shuffle` is converted as a `IterDataPipe`. I prefer this option simply because I find the concept of shuffling `MapDataPipe` would be shuffling indices and returning the data in the order of shuffled indices. [ghstack-poisoned]

…IterDataPipe" Fixes: meta-pytorch/data#718 This is an alternative PR against #82974 This PR would change the behavior for both types to the same behavior as `IterDataPipe.shuffle` - Lazily generating seed per iteration - Each iterators has a new seed - Convert `MapDataPipe.shuffle` to an `IterDataPipe` [ghstack-poisoned]

Fixes: meta-pytorch/data#718 This is an alternative PR against #82974 This PR would change the behavior for both types to the same behavior as `IterDataPipe.shuffle` - Lazily generating seed per iteration - Each iterators has a new seed - Convert `MapDataPipe.shuffle` to an `IterDataPipe` [ghstack-poisoned]

…IterDataPipe" Fixes: meta-pytorch/data#718 This is an alternative PR against #82974 This PR would change the behavior for both types to the same behavior as `IterDataPipe.shuffle` - Lazily generating seed per iteration - Each iterators has a new seed - Convert `MapDataPipe.shuffle` to an `IterDataPipe` [ghstack-poisoned]

Fixes: meta-pytorch/data#718 This is an alternative PR against #82974 This PR would change the behavior for both types to the same behavior as `IterDataPipe.shuffle` - Lazily generating seed per iteration - Each iterators has a new seed - Convert `MapDataPipe.shuffle` to an `IterDataPipe` [ghstack-poisoned]

…IterDataPipe" Fixes: meta-pytorch/data#718 This is an alternative PR against #82974 This PR would change the behavior for both types to the same behavior as `IterDataPipe.shuffle` - Lazily generating seed per iteration - Each iterators has a new seed - Convert `MapDataPipe.shuffle` to an `IterDataPipe` [ghstack-poisoned]

Fixes: meta-pytorch/data#718 This is an alternative PR against #82974 This PR would change the behavior for both types to the same behavior as `IterDataPipe.shuffle` - Lazily generating seed per iteration - Each iterators has a new seed - Convert `MapDataPipe.shuffle` to an `IterDataPipe` [ghstack-poisoned]

…IterDataPipe" Fixes: meta-pytorch/data#718 This is an alternative PR against #82974 This PR would change the behavior for both types to the same behavior as `IterDataPipe.shuffle` - Lazily generating seed per iteration - Each iterators has a new seed - Convert `MapDataPipe.shuffle` to an `IterDataPipe` ## BC-breaking Note: This PR changes the return type of `MapDataPipe.shuffle` from a `MapDataPipe` to a `IterDataPipe`. ### 1. 12 Output as `MapDataPipe` ``` >>> from torch.utils.data import IterDataPipe, MapDataPipe >>> from torch.utils.data.datapipes.map import SequenceWrapper >>> dp = SequenceWrapper(list(range(10))).shuffle() >>> isinstance(dp, MapDataPipe) True >>> isinstance(dp, IterDataPipe) False ``` ### This PR: Output as `IterDataPipe` ``` >>> from torch.utils.data import IterDataPipe, MapDataPipe >>> from torch.utils.data.datapipes.map import SequenceWrapper >>> dp = SequenceWrapper(list(range(10))).shuffle() >>> isinstance(dp, MapDataPipe) False >>> isinstance(dp, IterDataPipe) True ``` [ghstack-poisoned]

Fixes: meta-pytorch/data#718 This is an alternative PR against #82974 This PR would change the behavior for both types to the same behavior as `IterDataPipe.shuffle` - Lazily generating seed per iteration - Each iterators has a new seed - Convert `MapDataPipe.shuffle` to an `IterDataPipe` ## BC-breaking Note: This PR changes the return type of `MapDataPipe.shuffle` from a `MapDataPipe` to a `IterDataPipe`. ### 1. 12 Output as `MapDataPipe` ``` >>> from torch.utils.data import IterDataPipe, MapDataPipe >>> from torch.utils.data.datapipes.map import SequenceWrapper >>> dp = SequenceWrapper(list(range(10))).shuffle() >>> isinstance(dp, MapDataPipe) True >>> isinstance(dp, IterDataPipe) False ``` ### This PR: Output as `IterDataPipe` ``` >>> from torch.utils.data import IterDataPipe, MapDataPipe >>> from torch.utils.data.datapipes.map import SequenceWrapper >>> dp = SequenceWrapper(list(range(10))).shuffle() >>> isinstance(dp, MapDataPipe) False >>> isinstance(dp, IterDataPipe) True ``` [ghstack-poisoned]

Fixes: meta-pytorch/data#718 This is an alternative PR against #82974 This PR would change the behavior for both types to the same behavior as `IterDataPipe.shuffle` - Lazily generating seed per iteration - Each iterators has a new seed - Convert `MapDataPipe.shuffle` to an `IterDataPipe` ## BC-breaking Note: This PR changes the return type of `MapDataPipe.shuffle` from a `MapDataPipe` to a `IterDataPipe`. ### 1. 12 Output as `MapDataPipe` ``` >>> from torch.utils.data import IterDataPipe, MapDataPipe >>> from torch.utils.data.datapipes.map import SequenceWrapper >>> dp = SequenceWrapper(list(range(10))).shuffle() >>> isinstance(dp, MapDataPipe) True >>> isinstance(dp, IterDataPipe) False ``` ### This PR: Output as `IterDataPipe` ``` >>> from torch.utils.data import IterDataPipe, MapDataPipe >>> from torch.utils.data.datapipes.map import SequenceWrapper >>> dp = SequenceWrapper(list(range(10))).shuffle() >>> isinstance(dp, MapDataPipe) False >>> isinstance(dp, IterDataPipe) True ``` Pull Request resolved: #83202 Approved by: https://github.com/NivekT

Summary: Fixes: meta-pytorch/data#718 This is an alternative PR against #82974 This PR would change the behavior for both types to the same behavior as `IterDataPipe.shuffle` - Lazily generating seed per iteration - Each iterators has a new seed - Convert `MapDataPipe.shuffle` to an `IterDataPipe` ## BC-breaking Note: This PR changes the return type of `MapDataPipe.shuffle` from a `MapDataPipe` to a `IterDataPipe`. ### 1. 12 Output as `MapDataPipe` ``` >>> from torch.utils.data import IterDataPipe, MapDataPipe >>> from torch.utils.data.datapipes.map import SequenceWrapper >>> dp = SequenceWrapper(list(range(10))).shuffle() >>> isinstance(dp, MapDataPipe) True >>> isinstance(dp, IterDataPipe) False ``` ### This PR: Output as `IterDataPipe` ``` >>> from torch.utils.data import IterDataPipe, MapDataPipe >>> from torch.utils.data.datapipes.map import SequenceWrapper >>> dp = SequenceWrapper(list(range(10))).shuffle() >>> isinstance(dp, MapDataPipe) False >>> isinstance(dp, IterDataPipe) True ``` Pull Request resolved: #83202 Approved by: https://github.com/NivekT Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/a423c966a780a1fdac6a29c6d2be2a0709de2cd5 Reviewed By: weiwangmeta Differential Revision: D39084833 Pulled By: ejguan fbshipit-source-id: e873f566f289b512e4f9f566149db0b9c7e5a89e

Fixes: meta-pytorch/data#718 This is an alternative PR against #82974 This PR would change the behavior for both types to the same behavior as `IterDataPipe.shuffle` - Lazily generating seed per iteration - Each iterators has a new seed - Convert `MapDataPipe.shuffle` to an `IterDataPipe` ## BC-breaking Note: This PR changes the return type of `MapDataPipe.shuffle` from a `MapDataPipe` to a `IterDataPipe`. ### 1. 12 Output as `MapDataPipe` ``` >>> from torch.utils.data import IterDataPipe, MapDataPipe >>> from torch.utils.data.datapipes.map import SequenceWrapper >>> dp = SequenceWrapper(list(range(10))).shuffle() >>> isinstance(dp, MapDataPipe) True >>> isinstance(dp, IterDataPipe) False ``` ### This PR: Output as `IterDataPipe` ``` >>> from torch.utils.data import IterDataPipe, MapDataPipe >>> from torch.utils.data.datapipes.map import SequenceWrapper >>> dp = SequenceWrapper(list(range(10))).shuffle() >>> isinstance(dp, MapDataPipe) False >>> isinstance(dp, IterDataPipe) True ``` Pull Request resolved: #83202 Approved by: https://github.com/NivekT

Summary: Fixes: meta-pytorch/data#718 This is an alternative PR against #82974 This PR would change the behavior for both types to the same behavior as `IterDataPipe.shuffle` - Lazily generating seed per iteration - Each iterators has a new seed - Convert `MapDataPipe.shuffle` to an `IterDataPipe` ## BC-breaking Note: This PR changes the return type of `MapDataPipe.shuffle` from a `MapDataPipe` to a `IterDataPipe`. ### 1. 12 Output as `MapDataPipe` ``` >>> from torch.utils.data import IterDataPipe, MapDataPipe >>> from torch.utils.data.datapipes.map import SequenceWrapper >>> dp = SequenceWrapper(list(range(10))).shuffle() >>> isinstance(dp, MapDataPipe) True >>> isinstance(dp, IterDataPipe) False ``` ### This PR: Output as `IterDataPipe` ``` >>> from torch.utils.data import IterDataPipe, MapDataPipe >>> from torch.utils.data.datapipes.map import SequenceWrapper >>> dp = SequenceWrapper(list(range(10))).shuffle() >>> isinstance(dp, MapDataPipe) False >>> isinstance(dp, IterDataPipe) True ``` Pull Request resolved: #83202 Approved by: https://github.com/NivekT Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/3f947264533f318355b848c070a4279032cbb5d8 Reviewed By: weiwangmeta Differential Revision: D39099339 Pulled By: ejguan fbshipit-source-id: e8a77d5ec05287e38b5cde1d81035d236cb63561

[DataPipe] Align shuffling behavior for IterDataPipe and MapDataPipe

53e9150

[ghstack-poisoned]

facebook-github-bot added the cla signed label Aug 8, 2022

ejguan mentioned this pull request Aug 8, 2022

[DataLoader] Add shuffling logic for MapDataPipe #82975

Closed

ejguan added 2 commits August 8, 2022 19:20

Update on "[DataPipe] Align shuffling behavior for IterDataPipe and M…

5cbec53

…apDataPipe" [ghstack-poisoned]

ejguan requested review from NivekT and VitalyFedyunin August 8, 2022 21:31

NivekT reviewed Aug 9, 2022

View reviewed changes

ejguan added 2 commits August 10, 2022 20:52

ejguan mentioned this pull request Aug 11, 2022

[DataPipe] Convert MapDataPipe.shuffle to IterDataPipe #83202

Closed

ejguan closed this Aug 15, 2022

facebook-github-bot deleted the gh/ejguan/41/head branch September 15, 2022 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataPipe] Align shuffling behavior for IterDataPipe and MapDataPipe#82974

[DataPipe] Align shuffling behavior for IterDataPipe and MapDataPipe#82974
ejguan wants to merge 5 commits intogh/ejguan/41/basefrom
gh/ejguan/41/head

ejguan commented Aug 8, 2022 •

edited

Loading

Uh oh!

facebook-github-bot commented Aug 8, 2022 •

edited

Loading

Uh oh!

NivekT left a comment

Uh oh!

Uh oh!

NivekT Aug 9, 2022

Uh oh!

ejguan Aug 10, 2022

Uh oh!

Uh oh!

ejguan commented Aug 10, 2022 •

edited

Loading

Uh oh!

ejguan commented Aug 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ejguan commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

✅ No Failures (0 Pending)

Uh oh!

NivekT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NivekT Aug 9, 2022

Choose a reason for hiding this comment

Uh oh!

ejguan Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ejguan commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ejguan commented Aug 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ejguan commented Aug 8, 2022 •

edited

Loading

facebook-github-bot commented Aug 8, 2022 •

edited

Loading

ejguan commented Aug 10, 2022 •

edited

Loading