[DataPipe] Convert MapDataPipe.shuffle to IterDataPipe by ejguan · Pull Request #83202 · pytorch/pytorch

ejguan · 2022-08-10T21:15:25Z

Fixes: meta-pytorch/data#718

Stack from ghstack:

[DataLoader] Add shuffling logic for MapDataPipe #82975 [DataLoader] Add shuffling logic for MapDataPipe
[DataPipe] Convert MapDataPipe.shuffle to IterDataPipe #83202 [DataPipe] Convert MapDataPipe.shuffle to IterDataPipe

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

@diff-train-skip-merge

[ghstack-poisoned]

facebook-github-bot · 2022-08-10T21:15:32Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/83202
✖️ Python docs build was skipped
✖️ C++ docs build was skipped
❓Need help or want to give feedback on the CI? Visit our office hours

✅ No Failures (0 Pending)

As of commit b8111b3 (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.

ghstack-source-id: ba52215 Pull Request resolved: #83202

[ghstack-poisoned]

ghstack-source-id: 6c7e273 Pull Request resolved: #83202

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

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

NivekT

Small changes are needed for interface generation.

Please update the MapDataPipe version's docstring so that users know it will be converted to IterDataPipe as well.

Question: Will the duplicate name ShufflerIterDataPipe cause any problem? I can't think of any since they are separated into different directories. I think it should be fine.

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

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

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 - Convert `MapDataPipe.shuffle` to an `IterDataPipe` [ghstack-poisoned]

NivekT

Overall LGTM! We have the questions as the last one; will approve as soon as we finalize the details on the last one and modify this one the similarly.

torch/utils/data/datapipes/gen_pyi.py

test/test_datapipe.py

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]

NivekT

The implementation LGTM! Just need to update the docstring. Some ideas:

Mention that IterDataPipe will now be returned (and possibly why and use converter to get MapDataPipe back again) - you can decide what to mention
In the example, call list(shuffle_dp) once more to show that the result will be different in the second call
We can potentially mention the set_seed API but if we expect users to rely on DataLoader then maybe not

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]

ejguan · 2022-08-26T21:00:35Z

@pytorchbot merge -g

pytorchmergebot · 2022-08-26T21:02:40Z

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered with the green (-g) flag. This means that your change will be merged once all checks on your PR have passed (ETA: 0-4 Hours). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

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

facebook-github-bot · 2022-08-28T17:58:54Z

@pytorchbot revert -m="Diff reverted internally" -c="ghfirst"

This Pull Request has been reverted by a revert inside Meta. To re-land this change, please open another pull request, assign the same reviewers, fix the CI failures that caused the revert and make sure that the failing CI runs on the PR by applying the proper ciflow label (e.g., ciflow/trunk).)

pytorchmergebot · 2022-08-28T18:00:13Z

@pytorchbot successfully started a revert job. Check the current status here.
Please reach out to the PyTorch DevX Team with feedback or questions!

pytorchmergebot · 2022-08-28T18:00:19Z

@ejguan your PR has been successfully reverted.

This reverts commit a423c96. Reverted #83202 on behalf of https://github.com/facebook-github-bot due to Diff reverted internally

weiwangmeta · 2022-08-29T06:09:11Z

@pytorchbot merge -l

pytorchmergebot · 2022-08-29T06:11:08Z

@pytorchbot successfully started a merge job. Check the current status here and land check progress here.
The merge job was triggered with the land checks (-l) flag. If you did not specify this flag yourself, you are likely enrolled in the land checks rollout. This means that your change will be merged once all checks on your PR and the land checks have passed (ETA 4 Hours). If you need to coordinate lands between different changes and cannot risk a land race, please add the ciflow/trunk label to your PR and wait for signal to complete, and then land your changes in proper order. Having trunk, pull, and Lint pre-run on a PR will bypass land checks and the ETA should be immediate. If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

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

…ipe (#83202) Test Plan: revert-hammer Differential Revision: D39084833 Original commit changeset: e873f566f289 Original Phabricator Diff: D39084833 fbshipit-source-id: 12a9f85310d140e87ca23472ec6c08c33625f408

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

This PR requires PR is landed: #83202 ## changes - For `apply_shuffle_setting` and `apply_shuffle_seed`, it makes sure it will apply shuffle setting to each of DataPipe that contains a method called `set_shuffle` or `set_seed`. - Change the API from `apply_shuffle_seed` to `apply_random_seed`. - Fix a bug that `apply_shuffle_seed` only accepts DataPipe that is hashable. After the PR, this function uses `id` to prevent seeding the same DataPipe multiple times per epoch. - Fix another bug from `shuffler` that `reset` with `_enable=False` would also reset `_seed`. Pull Request resolved: #83741 Approved by: https://github.com/NivekT

…ization ghstack-source-id: 7b310d0 Pull Request resolved: pytorch/pytorch#83202

[DataPipe] Convert MapDataPipe.shuffle to IterDataPipe

559c572

[ghstack-poisoned]

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

ejguan added a commit that referenced this pull request Aug 10, 2022

[DataPipe] Convert MapDataPipe.shuffle to IterDataPipe

2cb5cc6

ghstack-source-id: ba52215 Pull Request resolved: #83202

ejguan mentioned this pull request Aug 10, 2022

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

Closed

Update on "[DataPipe] Convert MapDataPipe.shuffle to IterDataPipe"

afc7a43

[ghstack-poisoned]

Update on "[DataPipe] Convert MapDataPipe.shuffle to IterDataPipe"

3ca4fe1

[ghstack-poisoned]

ejguan added a commit that referenced this pull request Aug 10, 2022

[DataPipe] Convert MapDataPipe.shuffle to IterDataPipe

3453250

ghstack-source-id: 6c7e273 Pull Request resolved: #83202

ejguan requested review from NivekT and VitalyFedyunin August 11, 2022 15:39

NivekT reviewed Aug 12, 2022

View reviewed changes

torch/utils/data/datapipes/iter/combinatorics.py Outdated Show resolved Hide resolved

torch/utils/data/datapipes/map/combinatorics.py Show resolved Hide resolved

VitalyFedyunin reviewed Aug 15, 2022

View reviewed changes

torch/utils/data/datapipes/map/combinatorics.py Show resolved Hide resolved

NivekT reviewed Aug 15, 2022

View reviewed changes

torch/utils/data/datapipes/map/combinatorics.py Show resolved Hide resolved

ejguan commented Aug 16, 2022

View reviewed changes

torch/utils/data/datapipes/iter/combinatorics.py Outdated Show resolved Hide resolved

This was referenced Aug 16, 2022

[DataPipe] Reset Shuffler's iterator when NotStarted #83535

Closed

[DataLoader] Add shuffling logic for MapDataPipe #82975

Closed

ejguan added 3 commits August 16, 2022 21:04

ejguan added release notes: dataloader release notes category topic: bc breaking topic category labels Aug 19, 2022

ejguan mentioned this pull request Aug 22, 2022

Set shuffle to DataPipes with set_shuffle API #83741

Closed

NivekT reviewed Aug 22, 2022

View reviewed changes

torch/utils/data/datapipes/gen_pyi.py Show resolved Hide resolved

test/test_datapipe.py Show resolved Hide resolved

ejguan added 2 commits August 23, 2022 20:29

ejguan requested review from NivekT and VitalyFedyunin August 25, 2022 15:17

NivekT approved these changes Aug 25, 2022

View reviewed changes

NivekT approved these changes Aug 26, 2022

View reviewed changes

pytorchmergebot added the Merged label Aug 26, 2022

pytorchmergebot closed this in a423c96 Aug 26, 2022

pytorchmergebot added the Reverted label Aug 28, 2022

pytorchmergebot added a commit that referenced this pull request Aug 28, 2022

Revert "[DataPipe] Convert MapDataPipe.shuffle to IterDataPipe (#83202)"

7244a37

This reverts commit a423c96. Reverted #83202 on behalf of https://github.com/facebook-github-bot due to Diff reverted internally

weiwangmeta reopened this Aug 29, 2022

pytorchmergebot closed this in 3f94726 Aug 29, 2022

facebook-github-bot deleted the gh/ejguan/43/head branch September 1, 2022 14:20

ejguan mentioned this pull request Sep 8, 2022

Shuffler inside a Zipper only shuffle some elements meta-pytorch/data#766

Open

Rick0317 pushed a commit to Rick0317/pytorch that referenced this pull request Oct 19, 2022

[DataPipe] Convert MapDataPipe.shuffle to IterDataPipe and fix serial…

d069ea0

…ization ghstack-source-id: 7b310d0 Pull Request resolved: pytorch/pytorch#83202

NivekT mentioned this pull request Apr 17, 2023

torchdata.datapipes.map.Shuffler should return a MapDataPipe meta-pytorch/data#1132

Closed

Conversation

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

Uh oh!

BC-breaking Note:

1. 12

This PR:

Uh oh!

facebook-github-bot commented Aug 10, 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!

Uh oh!

Uh oh!

Uh oh!

NivekT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

NivekT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

NivekT left a comment

Choose a reason for hiding this comment

Uh oh!

ejguan commented Aug 26, 2022

Uh oh!

pytorchmergebot commented Aug 26, 2022

Uh oh!

facebook-github-bot commented Aug 28, 2022

Uh oh!

pytorchmergebot commented Aug 28, 2022

Uh oh!

pytorchmergebot commented Aug 28, 2022

Uh oh!

weiwangmeta commented Aug 29, 2022

Uh oh!

pytorchmergebot commented Aug 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ejguan commented Aug 10, 2022 •

edited by weiwangmeta

Loading

facebook-github-bot commented Aug 10, 2022 •

edited

Loading