[DataPipe] Separating DataPipes from Dataset into different files #73396

NivekT · 2022-02-24T22:31:20Z

Stack from ghstack:

Separating DataPipes from Dataset into different files. This makes the code more maintainable and simplifies some of the code generation.

I have also tried to move datapipe.py into torch.utils.data.datapipes, but that will lead to circular import and rewriting many import statements. Should I put more time and go down that path some more?

Fixes meta-pytorch/data#213

Differential Revision: D34481962

[ghstack-poisoned]

pytorch-bot · 2022-02-24T22:31:24Z

CI Flow Status

⚛️ CI Flow

Ruleset - Version: v1
Ruleset - File: https://github.com/pytorch/pytorch/blob/9220a5ac5867356aea11e1e66f6e5766d409bff2/.github/generated-ciflow-ruleset.json
PR ciflow labels: ciflow/default
Add ciflow labels to this PR to trigger more builds:

Workflows	Labels (bold enabled)	Status
Triggered Workflows
linux-binary-conda	`ciflow/binaries`, `ciflow/binaries_conda`, `ciflow/default`	✅ triggered
linux-binary-libtorch-cxx11-abi	`ciflow/binaries`, `ciflow/binaries_libtorch`, `ciflow/default`	✅ triggered
linux-binary-libtorch-pre-cxx11	`ciflow/binaries`, `ciflow/binaries_libtorch`, `ciflow/default`	✅ triggered
linux-binary-manywheel	`ciflow/binaries`, `ciflow/binaries_wheel`, `ciflow/default`	✅ triggered
linux-bionic-py3.7-clang9	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/noarch`, `ciflow/trunk`	✅ triggered
linux-bionic-rocm4.5-py3.7	`ciflow/all`, `ciflow/default`, `ciflow/linux`, `ciflow/rocm`, `ciflow/trunk`	✅ triggered
linux-docs	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/docs`, `ciflow/linux`, `ciflow/trunk`	✅ triggered
linux-vulkan-bionic-py3.7-clang9	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/trunk`, `ciflow/vulkan`	✅ triggered
linux-xenial-cuda11.3-py3.7-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/default`, `ciflow/linux`, `ciflow/trunk`	✅ triggered
linux-xenial-cuda11.3-py3.7-gcc7-bazel-test	`ciflow/all`, `ciflow/bazel`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/trunk`	✅ triggered
linux-xenial-py3-clang5-mobile-build	`ciflow/all`, `ciflow/default`, `ciflow/linux`, `ciflow/mobile`, `ciflow/trunk`	✅ triggered
linux-xenial-py3-clang5-mobile-custom-build-static	`ciflow/all`, `ciflow/default`, `ciflow/linux`, `ciflow/mobile`, `ciflow/trunk`	✅ triggered
linux-xenial-py3.7-clang7-asan	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/sanitizers`, `ciflow/trunk`	✅ triggered
linux-xenial-py3.7-clang7-onnx	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/onnx`, `ciflow/trunk`	✅ triggered
linux-xenial-py3.7-gcc5.4	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/trunk`	✅ triggered
linux-xenial-py3.7-gcc7	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/trunk`	✅ triggered
linux-xenial-py3.7-gcc7-no-ops	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/trunk`	✅ triggered
macos-arm64-binary-conda	`ciflow/binaries`, `ciflow/binaries_conda`, `ciflow/default`	✅ triggered
macos-arm64-binary-wheel	`ciflow/binaries`, `ciflow/binaries_wheel`, `ciflow/default`	✅ triggered
macos-binary-conda	`ciflow/binaries`, `ciflow/binaries_conda`, `ciflow/default`	✅ triggered
macos-binary-libtorch-cxx11-abi	`ciflow/binaries`, `ciflow/binaries_libtorch`, `ciflow/default`	✅ triggered
macos-binary-libtorch-pre-cxx11	`ciflow/binaries`, `ciflow/binaries_libtorch`, `ciflow/default`	✅ triggered
macos-binary-wheel	`ciflow/binaries`, `ciflow/binaries_wheel`, `ciflow/default`	✅ triggered
pytorch-linux-xenial-py3-clang5-android-ndk-r19c-gradle-custom-build-single	`ciflow/all`, `ciflow/android`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/trunk`	✅ triggered
pytorch-linux-xenial-py3-clang5-android-ndk-r19c-gradle-custom-build-single-full-jit	`ciflow/all`, `ciflow/android`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/trunk`	✅ triggered
win-vs2019-cpu-py3	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/trunk`, `ciflow/win`	✅ triggered
win-vs2019-cuda11.3-py3	`ciflow/all`, `ciflow/cuda`, `ciflow/default`, `ciflow/trunk`, `ciflow/win`	✅ triggered
windows-binary-libtorch-cxx11-abi	`ciflow/binaries`, `ciflow/binaries_libtorch`, `ciflow/default`	✅ triggered
windows-binary-libtorch-pre-cxx11	`ciflow/binaries`, `ciflow/binaries_libtorch`, `ciflow/default`	✅ triggered
windows-binary-wheel	`ciflow/binaries`, `ciflow/binaries_wheel`, `ciflow/default`	✅ triggered
Skipped Workflows
caffe2-linux-xenial-py3.7-gcc5.4	`ciflow/all`, `ciflow/cpu`, `ciflow/linux`, `ciflow/trunk`	🚫 skipped
docker-builds	`ciflow/all`, `ciflow/trunk`	🚫 skipped
ios-12-5-1-arm64	`ciflow/all`, `ciflow/ios`, `ciflow/macos`, `ciflow/scheduled`	🚫 skipped
ios-12-5-1-arm64-coreml	`ciflow/all`, `ciflow/ios`, `ciflow/macos`, `ciflow/scheduled`	🚫 skipped
ios-12-5-1-arm64-custom-ops	`ciflow/all`, `ciflow/ios`, `ciflow/macos`, `ciflow/scheduled`	🚫 skipped
ios-12-5-1-arm64-metal	`ciflow/all`, `ciflow/ios`, `ciflow/macos`, `ciflow/scheduled`	🚫 skipped
ios-12-5-1-x86-64	`ciflow/all`, `ciflow/ios`, `ciflow/macos`, `ciflow/trunk`	🚫 skipped
ios-12-5-1-x86-64-coreml	`ciflow/all`, `ciflow/ios`, `ciflow/macos`, `ciflow/trunk`	🚫 skipped
libtorch-linux-xenial-cuda10.2-py3.7-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/libtorch`, `ciflow/linux`, `ciflow/trunk`	🚫 skipped
libtorch-linux-xenial-cuda11.3-py3.7-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/libtorch`, `ciflow/linux`, `ciflow/trunk`	🚫 skipped
linux-bionic-cuda10.2-py3.9-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/linux`, `ciflow/slow`, `ciflow/trunk`	🚫 skipped
linux-docs-push	`ciflow/all`, `ciflow/cpu`, `ciflow/linux`, `ciflow/scheduled`	🚫 skipped
linux-xenial-cuda11.3-py3.7-gcc7-no-ops	`ciflow/all`, `ciflow/cuda`, `ciflow/linux`, `ciflow/trunk`	🚫 skipped
macos-10-15-py3-arm64	`ciflow/all`, `ciflow/macos`, `ciflow/trunk`	🚫 skipped
macos-10-15-py3-lite-interpreter-x86-64	`ciflow/all`, `ciflow/macos`, `ciflow/trunk`	🚫 skipped
macos-11-py3-x86-64	`ciflow/all`, `ciflow/macos`, `ciflow/trunk`	🚫 skipped
parallelnative-linux-xenial-py3.7-gcc5.4	`ciflow/all`, `ciflow/cpu`, `ciflow/linux`, `ciflow/trunk`	🚫 skipped
periodic-libtorch-linux-bionic-cuda11.5-py3.7-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/libtorch`, `ciflow/linux`, `ciflow/scheduled`	🚫 skipped
periodic-libtorch-linux-xenial-cuda11.1-py3.7-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/libtorch`, `ciflow/linux`, `ciflow/scheduled`	🚫 skipped
periodic-linux-bionic-cuda11.5-py3.7-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/linux`, `ciflow/scheduled`	🚫 skipped
periodic-linux-xenial-cuda10.2-py3-gcc7-slow-gradcheck	`ciflow/all`, `ciflow/cuda`, `ciflow/linux`, `ciflow/scheduled`, `ciflow/slow`, `ciflow/slow-gradcheck`	🚫 skipped
periodic-linux-xenial-cuda11.1-py3.7-gcc7-debug	`ciflow/all`, `ciflow/cuda`, `ciflow/linux`, `ciflow/scheduled`	🚫 skipped
periodic-win-vs2019-cuda11.1-py3	`ciflow/all`, `ciflow/cuda`, `ciflow/scheduled`, `ciflow/win`	🚫 skipped
periodic-win-vs2019-cuda11.5-py3	`ciflow/all`, `ciflow/cuda`, `ciflow/scheduled`, `ciflow/win`	🚫 skipped
pytorch-linux-xenial-py3-clang5-android-ndk-r19c-build	`ciflow/all`, `ciflow/android`, `ciflow/cpu`, `ciflow/linux`, `ciflow/trunk`	🚫 skipped
pytorch-xla-linux-bionic-py3.7-clang8	`ciflow/all`, `ciflow/cpu`, `ciflow/linux`, `ciflow/trunk`, `ciflow/xla`	🚫 skipped

facebook-github-bot · 2022-02-24T22:31:26Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/73396
📄 Preview docs built from this PR
📄 Preview C++ docs built from this PR
🔧 Opt-in to CIFlow to control what jobs run on your PRs

💊 CI failures summary and remediations

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

💚 💚 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: a574e48 Pull Request resolved: #73396

…t files" Separating DataPipes from Dataset into different files. This makes the code more maintainable and simplifies some of the code generation. I have also tried to move `datapipe.py` into `torch.utils.data.datapipes`, but that will lead to circular import and rewriting many import statements. Should I put more time and go down that path some more? Fixes meta-pytorch/data#213 [ghstack-poisoned]

ghstack-source-id: a23b6e8 Pull Request resolved: #73396

NivekT · 2022-02-25T18:49:57Z

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

…t files" Separating DataPipes from Dataset into different files. This makes the code more maintainable and simplifies some of the code generation. I have also tried to move `datapipe.py` into `torch.utils.data.datapipes`, but that will lead to circular import and rewriting many import statements. Should I put more time and go down that path some more? Fixes meta-pytorch/data#213 Differential Revision: [D34481962](https://our.internmc.facebook.com/intern/diff/D34481962) [ghstack-poisoned]

ejguan

Overall looks great! Thank you

Have you tested it with import torchdata. Just want to verify if this PR introduces any BC breaking changes.

And, what do you think about the name/location of this datapipe.py file because we have a folder called datapipes at the same directory. It can be confusing for users when doing import in python. Is that possible to move this file into datapipes folder but expose the API at the same place.

NivekT · 2022-03-14T14:11:16Z

Overall looks great! Thank you

Have you tested it with import torchdata. Just want to verify if this PR introduces any BC breaking changes.

I think torchdata should be fine since it uses from torch.utils.data import IterDataPipe, which is not dependent on the exact location of the classes that are being moved. I ran the unit tests within torchdata and they all worked. I can double check.

And, what do you think about the name/location of this datapipe.py file because we have a folder called datapipes at the same directory. It can be confusing for users when doing import in python. Is that possible to move this file into datapipes folder but expose the API at the same place.

I have tried moving the classes into the datapipes directory, and subsequently import them into the following __init__ to preserve backwards compatibility. But that is where I got circular reference errors. I think it will require a potentially much bigger clean up/refactor to make that happen.

pytorch/torch/utils/data/__init__.py

Lines 1 to 23 in 0b1f3bd

    
           # TODO(VitalyFedyunin): Rearranging this imports leads to crash, 
        
           # need to cleanup dependencies and fix it 
        
           from torch.utils.data.sampler import ( 
        
               BatchSampler, 
        
               RandomSampler, 
        
               Sampler, 
        
               SequentialSampler, 
        
               SubsetRandomSampler, 
        
               WeightedRandomSampler, 
        
           ) 
        
           from torch.utils.data.dataset import ( 
        
               ChainDataset, 
        
               ConcatDataset, 
        
               DFIterDataPipe, 
        
               DataChunk, 
        
               Dataset, 
        
               IterDataPipe, 
        
               IterableDataset, 
        
               MapDataPipe, 
        
               Subset, 
        
               TensorDataset, 
        
               random_split, 
        
           )

ejguan · 2022-03-14T14:31:45Z

torch/utils/data/datapipe.py

+
+from torch.utils.data._typing import _DataPipeMeta
+from torch.utils.data._utils.serialization import serialize_fn, SerializationType, deserialize_fn
+from torch.utils.data.dataset import IterableDataset, T_co, UNTRACABLE_DATAFRAME_PIPES, Dataset


T_co and UNTRACABLE_DATAFRAME_PIPES should be created from this file rather than importing them.

And, if we do import torch.utils.data.dataset.IterableDataset and import torch.utils.data.dataset.Dataset, would circular import still happen?

I just refactored it, though most of the import statements for DataPipes have to be changed.

I am explicitly leaving graph.py and graph_settings.py in torch/utils/data (instead of moving them into the datapipes directory) because they are needed by DataLoader. Let me know if you think those two files should be moved as well.

…t files" Separating DataPipes from Dataset into different files. This makes the code more maintainable and simplifies some of the code generation. I have also tried to move `datapipe.py` into `torch.utils.data.datapipes`, but that will lead to circular import and rewriting many import statements. Should I put more time and go down that path some more? Fixes meta-pytorch/data#213 Differential Revision: [D34481962](https://our.internmc.facebook.com/intern/diff/D34481962) [ghstack-poisoned]

NivekT · 2022-03-14T17:46:33Z

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

…t files" Separating DataPipes from Dataset into different files. This makes the code more maintainable and simplifies some of the code generation. I have also tried to move `datapipe.py` into `torch.utils.data.datapipes`, but that will lead to circular import and rewriting many import statements. Should I put more time and go down that path some more? Fixes meta-pytorch/data#213 Differential Revision: [D34481962](https://our.internmc.facebook.com/intern/diff/D34481962) [ghstack-poisoned]

NivekT · 2022-03-14T17:49:49Z

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

ejguan

Thank you soooo much! This is looks awesome.

torch/utils/data/dataset.py

…t files" Separating DataPipes from Dataset into different files. This makes the code more maintainable and simplifies some of the code generation. I have also tried to move `datapipe.py` into `torch.utils.data.datapipes`, but that will lead to circular import and rewriting many import statements. Should I put more time and go down that path some more? Fixes meta-pytorch/data#213 Differential Revision: [D34481962](https://our.internmc.facebook.com/intern/diff/D34481962) [ghstack-poisoned]

NivekT · 2022-03-14T19:39:47Z

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

…3396) Summary: Pull Request resolved: #73396 Separating DataPipes from Dataset into different files. This makes the code more maintainable and simplifies some of the code generation. I have also tried to move `datapipe.py` into `torch.utils.data.datapipes`, but that will lead to circular import and rewriting many import statements. Should I put more time and go down that path some more? Fixes meta-pytorch/data#213 Test Plan: Imported from OSS Reviewed By: ejguan Differential Revision: D34481962 Pulled By: NivekT fbshipit-source-id: 42fb26fe7fc334636852cfd8719fc807bdaa7912

[DataPipe] Separating DataPipes from Dataset into different files

9220a5a

[ghstack-poisoned]

pytorch-bot bot added the ciflow/default label Feb 24, 2022

facebook-github-bot added the cla signed label Feb 24, 2022

NivekT added a commit that referenced this pull request Feb 24, 2022

[DataPipe] Separating DataPipes from Dataset into different files

128a5dd

ghstack-source-id: a574e48 Pull Request resolved: #73396

NivekT requested review from VitalyFedyunin and ejguan February 24, 2022 22:31

NivekT added a commit that referenced this pull request Feb 25, 2022

[DataPipe] Separating DataPipes from Dataset into different files

488e71b

ghstack-source-id: a23b6e8 Pull Request resolved: #73396

NivekT mentioned this pull request Mar 9, 2022

[BE Hackathon][DataPipe] Automatically generate datapipe.pyi via CMake #73991

Closed

NivekT added 2 commits March 10, 2022 17:08

ejguan reviewed Mar 14, 2022

View reviewed changes

ejguan approved these changes Mar 14, 2022

View reviewed changes

torch/utils/data/dataset.py Outdated Show resolved Hide resolved

pytorchmergebot closed this in eec994f Mar 15, 2022

NivekT added module: data torch.utils.data release notes: dataloader release notes category labels Mar 15, 2022

facebook-github-bot deleted the gh/nivekt/49/head branch March 19, 2022 14:17

NivekT added the topic: not user facing topic category label Mar 30, 2022

WBobby mentioned this pull request Aug 17, 2022

Add ROCm5.2.3/AMDGPU support for PyTorch WBobby/pytorch#2

Closed

[DataPipe] Separating DataPipes from Dataset into different files #73396

[DataPipe] Separating DataPipes from Dataset into different files #73396

Uh oh!

Conversation

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

Uh oh!

Uh oh!

pytorch-bot bot commented Feb 24, 2022

⚛️ CI Flow

Uh oh!

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

Uh oh!

🔗 Helpful links

💊 CI failures summary and remediations

Uh oh!

NivekT commented Feb 25, 2022

Uh oh!

ejguan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NivekT commented Mar 14, 2022

Uh oh!

ejguan Mar 14, 2022

Choose a reason for hiding this comment

Uh oh!

NivekT Mar 14, 2022

Choose a reason for hiding this comment

Uh oh!

NivekT commented Mar 14, 2022

Uh oh!

NivekT commented Mar 14, 2022

Uh oh!

ejguan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NivekT commented Mar 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

NivekT commented Feb 24, 2022 •

edited

Loading

facebook-github-bot commented Feb 24, 2022 •

edited

Loading

ejguan left a comment •

edited

Loading