[DataLoader] Deep copy ReadingService during DL2 initialization by NivekT · Pull Request #746 · meta-pytorch/data

NivekT · 2022-08-18T22:10:38Z

Stack from ghstack:

By creating a deep copy of the input ReadingService during the initialization of DataLoader2, we allow the instance of ReadingService to store state without worrying about the users passing the same ReadingService to initialize additional DataLoaders.

For instance, PrototypeMultiProcessingReadingService stores processes and datapipes as instance variables. If a single object of that ReadingService is used to initialize multiple DataLoaders, it can led to conflicts and incorrect states. This should no longer be an issue with this change.

Relevant test are added in subsequent PR related to lengths and eventually PrototypeMultiprocessingReadingService.

Differential Revision: D38868731

BC Breaking Note:

At the time when this PR landed, MultiProcessingReadingService was known as PrototypeMultiProcessingReadingService.

Previously, if a ReadingService is being passed to multiple DataLoaders, the ReadingService's state can be altered by any of those DataLoaders. This use case is not intended and can lead to silent incorrectness.

For instance, MultiProcessingReadingService stores processes and datapipes as instance variables. If a single object of that ReadingService is used to initialize multiple DataLoaders, it can led to conflicts and incorrect states.

Consider the code snippet below:

dp = IterableWrapper([0, 1, 2, 3, 4])
rs = MultiProcessingReadingService(num_workers=2)
dl1 = DataLoader2(dp, reading_service=rs)
dl2 = DataLoader2(dp, reading_service=rs)
next(iter(dl1))
print(f"Number of processes that exist in `dl1`'s RS after initializing `dl1`: {len(dl1.reading_service._worker_processes)}")
print(next(iter(dl2)))
# Note that we are still examining `dl1.read_service` below
print(f"Number of processes that exist in `dl1`'s RS after initializing `dl2`: {len(dl1.reading_service._worker_processes)}")

Prior to this change, since the reading service objects are identical within each DataLoader2, initializing dl2 will change the state of the reading service that exists in dl1.

Number of processes that exist in `dl1`'s RS after initializing `dl1`: 2
Number of processes that exist in `dl1`'s RS after initializing `dl2`: 4

After this change, the reading service that exist in each DataLoader2 is independent and will not impact each other.

Number of processes that exist in `dl1`'s RS after initializing `dl1`: 2
Number of processes that exist in `dl1`'s RS after initializing `dl2`: 2

[ghstack-poisoned]

ejguan · 2022-08-19T13:30:26Z

torchdata/dataloader2/dataloader2.py

        else:
            self.datapipe_adapter_fns = [datapipe_adapter_fn]
-        self.reading_service = reading_service
+        self.reading_service = deepcopy(reading_service)


Let's try to import your PR to internal to validate if this is working properly with internal RS.

+1 on checking existing implementations.

NivekT · 2022-08-19T17:14:54Z

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

VitalyFedyunin · 2022-08-19T17:46:17Z

We should put this information into the ReadingService interface file too. To make sure that RS developers know about potential deep copy.

VitalyFedyunin · 2022-08-19T17:47:35Z

One more though, we deepcopy RS, but we pickle-unpickle DP. I think we need to pick consistent approach.

ejguan · 2022-08-19T17:53:12Z

One more though, we deepcopy RS, but we pickle-unpickle DP. I think we need to pick consistent approach.

Totally agree. And, I support deepcopy because it's more readable and even working with unpickable objects.

NivekT · 2022-08-22T22:16:53Z

One more though, we deepcopy RS, but we pickle-unpickle DP. I think we need to pick consistent approach.

Can you elaborate on this? What make a consistent approach matter here?

NivekT · 2022-08-22T22:22:47Z

Also, it seems like some internal ReadingService cannot be deep-copied. One alternative is for users to pass in a constructor instead of the object itself, but that will change the interface quite a bit.

ejguan · 2022-08-23T13:41:06Z

Also, it seems like some internal ReadingService cannot be deep-copied. One alternative is for users to pass in a constructor instead of the object itself, but that will change the interface quite a bit.

It seems the Error comes from construction of process group in __init__ function. You might be able to move them into initialize function to make it working.

…ation" By creating a deep copy of the input `ReadingService` during the initialization of `DataLoader2`, we can allow the instance of ReadingService to store state, without worrying about the users passing the same `ReadingService` to initialize additional DataLoaders. For instance, `PrototypeMultiProcessingReadingService` stores `processes` and `datapipes` as instance variables. In a single object of that ReadingService is used to initialize multiple DataLoaders, it could've led to conflicts and incorrect states. This should no longer be an issue with this change. Relevant test are added in subsequent PR related to lengths and eventually `PrototypeMultiprocessingReadingService` Differential Revision: [D38868731](https://our.internmc.facebook.com/intern/diff/D38868731) [ghstack-poisoned]

NivekT · 2022-08-24T22:45:10Z

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

…ation" By creating a deep copy of the input `ReadingService` during the initialization of `DataLoader2`, we allow the instance of `ReadingService` to store state without worrying about the users passing the same `ReadingService` to initialize additional DataLoaders. For instance, `PrototypeMultiProcessingReadingService` stores `processes` and `datapipes` as instance variables. If a single object of that ReadingService is used to initialize multiple DataLoaders, it can led to conflicts and incorrect states. This should no longer be an issue with this change. Relevant test are added in subsequent PR related to lengths and eventually `PrototypeMultiprocessingReadingService`. Differential Revision: [D38868731](https://our.internmc.facebook.com/intern/diff/D38868731) [ghstack-poisoned]

NivekT · 2022-08-25T17:24:28Z

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

ejguan

Approving from OSS side. We might want @Miiira double check on the Diff.

And, could you please add a comment to ReadingServiceInterface that it's required to be deepcopied / picklable before initialize function.

…ation" By creating a deep copy of the input `ReadingService` during the initialization of `DataLoader2`, we allow the instance of `ReadingService` to store state without worrying about the users passing the same `ReadingService` to initialize additional DataLoaders. For instance, `PrototypeMultiProcessingReadingService` stores `processes` and `datapipes` as instance variables. If a single object of that ReadingService is used to initialize multiple DataLoaders, it can led to conflicts and incorrect states. This should no longer be an issue with this change. Relevant test are added in subsequent PR related to lengths and eventually `PrototypeMultiprocessingReadingService`. Differential Revision: [D38868731](https://our.internmc.facebook.com/intern/diff/D38868731) [ghstack-poisoned]

NivekT · 2022-08-25T20:50:28Z

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

…itialization" 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]

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]

…itialization" 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]

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]

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

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

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

…ation" By creating a deep copy of the input `ReadingService` during the initialization of `DataLoader2`, we allow the instance of `ReadingService` to store state without worrying about the users passing the same `ReadingService` to initialize additional DataLoaders. For instance, `PrototypeMultiProcessingReadingService` stores `processes` and `datapipes` as instance variables. If a single object of that ReadingService is used to initialize multiple DataLoaders, it can led to conflicts and incorrect states. This should no longer be an issue with this change. Relevant test are added in subsequent PR related to lengths and eventually `PrototypeMultiprocessingReadingService`. Differential Revision: [D38868731](https://our.internmc.facebook.com/intern/diff/D38868731) [ghstack-poisoned]

NivekT · 2022-10-18T22:57:28Z

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

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

[DataLoader] Deep copy ReadingService during initialization

34b0f86

[ghstack-poisoned]

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 Aug 18, 2022

This was referenced Aug 18, 2022

[DataPipe] Adding LengthSetterIterDataPipe #747

Closed

[DataLoader] Add __len__ to DataLoader2 #728

Closed

NivekT requested review from VitalyFedyunin and ejguan August 18, 2022 22:10

NivekT added the topic: improvements topic category label Aug 18, 2022

NivekT changed the title ~~[DataLoader] Deep copy ReadingService during initialization~~ [DataLoader] Deep copy ReadingService during DL2 initialization Aug 18, 2022

ejguan reviewed Aug 19, 2022

View reviewed changes

NivekT added 2 commits August 24, 2022 14:46

ejguan approved these changes Aug 25, 2022

View reviewed changes

NivekT mentioned this pull request Sep 22, 2022

[DataLoader2] Deep copy DataPipe during initialization #786

Closed

NivekT added the topic: bc_breaking topic category label Oct 25, 2022

facebook-github-bot closed this in d55f9ee Nov 17, 2022

facebook-github-bot deleted the gh/NivekT/89/head branch November 21, 2022 15:19

Conversation

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

Uh oh!

Uh oh!

ejguan Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

VitalyFedyunin Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

NivekT commented Aug 19, 2022

Uh oh!

VitalyFedyunin commented Aug 19, 2022

Uh oh!

VitalyFedyunin commented Aug 19, 2022

Uh oh!

ejguan commented Aug 19, 2022

Uh oh!

NivekT commented Aug 22, 2022

Uh oh!

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

Uh oh!

Uh oh!

ejguan commented Aug 23, 2022

Uh oh!

NivekT commented Aug 24, 2022

Uh oh!

NivekT commented Aug 25, 2022

Uh oh!

ejguan left a comment

Choose a reason for hiding this comment

Uh oh!

NivekT commented Aug 25, 2022

Uh oh!

NivekT commented Oct 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

NivekT commented Aug 18, 2022 •

edited

Loading

NivekT commented Aug 22, 2022 •

edited

Loading