Skip to content

[DataLoader2] Deep copy DataPipe during initialization#786

Closed
NivekT wants to merge 3 commits intogh/NivekT/93/basefrom
gh/NivekT/93/head
Closed

[DataLoader2] Deep copy DataPipe during initialization#786
NivekT wants to merge 3 commits intogh/NivekT/93/basefrom
gh/NivekT/93/head

Conversation

@NivekT
Copy link
Contributor

@NivekT NivekT commented Sep 22, 2022 •
edited
Loading

Stack from ghstack:

Deep copy DataPipe during initialization to avoid sharing states if the same DataPipe is being passed to multiple usages.

This can be merged before or after #746 (the other PR will have to rebase).

Differential Revision: D39741743

BC Breaking Note:

Previously, if a DataPipe is being passed to multiple DataLoaders, the DataPipe's state can be altered by any of those DataLoaders. In some cases, that may raise exception due to the single iterator constraint; in other cases, some behaviors can be changed due to the adapters (e.g. shuffling) of another DataLoader.

Consider the code snippet below:

dp = IterableWrapper([0, 1, 2, 3, 4])
dl1 = DataLoader2(dp)
dl2 = DataLoader2(dp)
for x, y in zip(dl1, dl2):
    print(x, y)

Prior to this change, since the DataPipe objects are identical for each DataLoader2, it would've violated the single iterator constraint and raised an exception.

RuntimeError: This iterator has been invalidated because another iterator has been created from the same IterDataPipe...

After this change, since the DataPipe objects are deep copied within the DataLoader2s, they are no longer the same object within each. You can iterate through them simultaneously without issue:

0 0
1 1
2 2
3 3
4 4

NivekT added a commit that referenced this pull request Sep 22, 2022
@NivekT
[DataLoader2] Deep copy datapipe during initialization
d066fb5 
ghstack-source-id: 43fd038
Pull Request resolved: #786
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 22, 2022
@NivekT
Copy link
Contributor Author

NivekT commented Sep 22, 2022

@NivekT has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@NivekT NivekT changed the title [DataLoader2] Deep copy datapipe during initialization [DataLoader2] Deep copy DataPipe during initialization Sep 22, 2022
@NivekT NivekT added the topic: improvements topic category label Sep 22, 2022
@NivekT
Copy link
Contributor Author

NivekT commented Sep 22, 2022

@NivekT has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@NivekT
Update on "[DataLoader2] Deep copy DataPipe during initialization"
d55fa89 
Deep copy DataPipe during initialization to avoid sharing states if the same DataPipe is being passed to multiple usages.

This can be merged before or after #746 (the other PR will have to rebase).

Differential Revision: [D39741743](https://our.internmc.facebook.com/intern/diff/D39741743)

[ghstack-poisoned]
NivekT added a commit that referenced this pull request Sep 30, 2022
@NivekT
[DataLoader2] Deep copy datapipe during initialization
c206768 
ghstack-source-id: 2b8ef58
Pull Request resolved: #786
@NivekT
Copy link
Contributor Author

NivekT commented Sep 30, 2022

@NivekT has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@NivekT
Update on "[DataLoader2] Deep copy DataPipe during initialization"
585c59d 
Deep copy DataPipe during initialization to avoid sharing states if the same DataPipe is being passed to multiple usages.

This can be merged before or after #746 (the other PR will have to rebase).

Differential Revision: [D39741743](https://our.internmc.facebook.com/intern/diff/D39741743)

[ghstack-poisoned]
NivekT added a commit that referenced this pull request Oct 5, 2022
@NivekT
[DataLoader2] Deep copy datapipe during initialization
e59b8a0 
ghstack-source-id: 62bcadc
Pull Request resolved: #786
@NivekT
Copy link
Contributor Author

NivekT commented Oct 5, 2022

@NivekT has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@ejguan ejguan mentioned this pull request Oct 12, 2022
Closed
ejguan pushed a commit to ejguan/data that referenced this pull request Oct 14, 2022
@NivekT @ejguan
Deep copy DataPipe during initialization (meta-pytorch#786)
c203fd1 
Summary:
Pull Request resolved: meta-pytorch#786

Deep copy DataPipe during initialization to avoid sharing states if the same DataPipe is being passed to multiple usages.

This can be merged before or after meta-pytorch#746 (the other PR will have to rebase).

Test Plan: Imported from OSS

Reviewed By: ejguan

Differential Revision: D39741743

Pulled By: ejguan

fbshipit-source-id: f1cdaca054e10bcb3672857b933732178b370535
@ejguan ejguan mentioned this pull request Oct 14, 2022
Merged
ejguan pushed a commit that referenced this pull request Oct 14, 2022
@NivekT @ejguan
Deep copy DataPipe during initialization (#786)
c542222 
Summary:
Pull Request resolved: #786

Deep copy DataPipe during initialization to avoid sharing states if the same DataPipe is being passed to multiple usages.

This can be merged before or after #746 (the other PR will have to rebase).

Test Plan: Imported from OSS

Reviewed By: ejguan

Differential Revision: D39741743

Pulled By: ejguan

fbshipit-source-id: f1cdaca054e10bcb3672857b933732178b370535
@NivekT NivekT mentioned this pull request Oct 14, 2022
Closed
@facebook-github-bot facebook-github-bot deleted the gh/NivekT/93/head branch October 15, 2022 14:19
ejguan pushed a commit to ejguan/data that referenced this pull request Oct 23, 2022
@NivekT @ejguan
Deep copy DataPipe during initialization (meta-pytorch#786)
785cb93 
Summary:
Pull Request resolved: meta-pytorch#786

Deep copy DataPipe during initialization to avoid sharing states if the same DataPipe is being passed to multiple usages.

This can be merged before or after meta-pytorch#746 (the other PR will have to rebase).

Test Plan: Imported from OSS

Reviewed By: ejguan

Differential Revision: D39741743

Pulled By: ejguan

fbshipit-source-id: f1cdaca054e10bcb3672857b933732178b370535
@ejguan
Copy link
Contributor

ejguan commented Oct 25, 2022

@NivekT Do you think we need to add BC breaking note to this PR?

@NivekT
Copy link
Contributor Author

NivekT commented Oct 25, 2022

@NivekT Do you think we need to add BC breaking note to this PR?

Actually yes, I think we should, even though I don't think users would intentionally rely on the state of DataPipes being shared among DataLoaders.

@ejguan
Copy link
Contributor

ejguan commented Oct 25, 2022

@NivekT Do you think we need to add BC breaking note to this PR?

Actually yes, I think we should, even though I don't think users would intentionally rely on the state of DataPipes being shared among DataLoaders.

Can you add a brief example in the bc breaking note that the same DataPipe used by multiple DLv2?

@NivekT
Copy link
Contributor Author

NivekT commented Oct 25, 2022

Will do by the afternoon.

@ejguan
Copy link
Contributor

ejguan commented Oct 25, 2022

Will do by the afternoon.

Thank you so much!

@NivekT NivekT added the topic: bc_breaking topic category label Oct 25, 2022
@NivekT
Copy link
Contributor Author

NivekT commented Oct 25, 2022

@ejguan Updated this PR's description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ejguan ejguan ejguan approved these changes

@VitalyFedyunin VitalyFedyunin Awaiting requested review from VitalyFedyunin

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. topic: bc_breaking topic category topic: improvements topic category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@NivekT @ejguan @facebook-github-bot