[DataLoader] Replacing `traverse` function with `traverse_datapipes` by NivekT · Pull Request #85667 · pytorch/pytorch

NivekT · 2022-09-26T21:50:24Z

Stack from ghstack:

This PR deprecates traverse function and replaces it with traverse_datapipes instead.

While use DataLoader, I realized that it is raising FutureWarning even though I am not explicitly using traverse. What is happening is that DataLoader invokes traverse(dp, only_datapipe=True), and the usage of the keyword causes the only_datapipe warning to be raised.

/home/ubuntu/miniconda3/lib/python3.8/site-packages/torch/utils/data/graph.py:102: FutureWarning: `only_datapipe` is deprecated from `traverse` function and will be removed after 1.13.
  warnings.warn(msg, FutureWarning)

A few things we'd like to do:

Deprecate the key word arg only_datapipe
Change the default behavior from only_datapipe=False to only_datapipe=True in the future
Do not raise a warning when users are using the function correctly

This creates a paradox it is impossible for the users to change their code to match the future default behavior (i.e. call traverse(dp) without only_datapipe):

they cannot do so because the default behavior of traverse hasn't changed yet, so they must use only_datapipe=True
if they use only_datapipe=True, eventually the kwarg will go away and cause a runtime error; they also get a FutureWarning in the present

IIUC, there doesn't seem to be a way to accomplish those 3 goals without replacing the function with a new one that has a different name; hence, this PR. Let me know if there is a better alternative.

If this looks right, I will send a follow up PR in TorchData.

Differential Revision: D39832183

[ghstack-poisoned]

pytorch-bot · 2022-09-26T21:50:26Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/85667

📄 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

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures, 1 Pending

As of commit 5f5449b:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

…datapipes`" This PR deprecates `traverse` function and replaces it with `traverse_datapipes` instead. While use `DataLoader`, I realized that it is raising `FutureWarning` even though I am not explicitly using `traverse`. What is happening is that `DataLoader` invokes `traverse(dp, only_datapipe=True)`, and the usage of the keyword causes `only_datapipe` the warning to be raise. What we would like to do: 1. Deprecate the key word arg `only_datapipe` 2. Change the default behavior from `only_datapipe=False` to `only_datapipe=True` in the future 3. Not raise a warning when users are using the function correctly This creates a paradox where the users want to change their code to match the future default behavior (i.e. call `traverse(dp)` without `only_datapipe`), but: - they cannot do so because the default behavior hasn't changed yet, so they must use `only_datapipe=True` - if they use `only_datapipe=True`, eventually the kwarg will go away and it will become a runtime error and they also get a warning in the present IIUC, there doesn't seem to be a way to accomplish those 3 goals without replacing the function with a new one that has a different name; hence, this PR. Let me know if there is a better alternative. [ghstack-poisoned]

NivekT · 2022-09-26T22:15:13Z

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

ejguan · 2022-09-27T13:51:51Z

Thank you for adding this PR! I was debating with myself between your approach or doing a BC-breaking change on traverse function in #85654.
Since your approach won't cause any BC-breaking change, it might be better. And, in order to align the name with other graph functions in https://github.com/pytorch/data/blob/main/torchdata/dataloader2/graph.py, do you think traverse_dp would be a better name?

…datapipes`" This PR deprecates `traverse` function and replaces it with `traverse_datapipes` instead. While use `DataLoader`, I realized that it is raising `FutureWarning` even though I am not explicitly using `traverse`. What is happening is that `DataLoader` invokes `traverse(dp, only_datapipe=True)`, and the usage of the keyword causes the `only_datapipe` warning to be raised. ``` /home/ubuntu/miniconda3/lib/python3.8/site-packages/torch/utils/data/graph.py:102: FutureWarning: `only_datapipe` is deprecated from `traverse` function and will be removed after 1.13. warnings.warn(msg, FutureWarning) ``` A few things we'd like to do: 1. Deprecate the key word arg `only_datapipe` 2. Change the default behavior from `only_datapipe=False` to `only_datapipe=True` in the future 3. Do not raise a warning when users are using the function correctly This creates a paradox it is impossible for the users to change their code to match the future default behavior (i.e. call `traverse(dp)` without `only_datapipe`): - they cannot do so because the default behavior of `traverse` hasn't changed yet, so they must use `only_datapipe=True` - if they use `only_datapipe=True`, eventually the kwarg will go away and cause a runtime error; they also get a `FutureWarning` in the present IIUC, there doesn't seem to be a way to accomplish those 3 goals without replacing the function with a new one that has a different name; hence, this PR. Let me know if there is a better alternative. If this looks right, I will send a follow up PR in `TorchData`. Differential Revision: [D39832183](https://our.internmc.facebook.com/intern/diff/D39832183) [ghstack-poisoned]

NivekT · 2022-09-27T15:27:57Z

I updated the name to traverse_dps to match find_dps in the file that you linked. Thanks for pointing that out.

NivekT · 2022-09-27T15:28:55Z

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

ejguan

Thank you, LGTM. Do you want to open corresponding PRs for both TorchData and TorchVision?

test/test_datapipe.py

…datapipes`" This PR deprecates `traverse` function and replaces it with `traverse_datapipes` instead. While use `DataLoader`, I realized that it is raising `FutureWarning` even though I am not explicitly using `traverse`. What is happening is that `DataLoader` invokes `traverse(dp, only_datapipe=True)`, and the usage of the keyword causes the `only_datapipe` warning to be raised. ``` /home/ubuntu/miniconda3/lib/python3.8/site-packages/torch/utils/data/graph.py:102: FutureWarning: `only_datapipe` is deprecated from `traverse` function and will be removed after 1.13. warnings.warn(msg, FutureWarning) ``` A few things we'd like to do: 1. Deprecate the key word arg `only_datapipe` 2. Change the default behavior from `only_datapipe=False` to `only_datapipe=True` in the future 3. Do not raise a warning when users are using the function correctly This creates a paradox it is impossible for the users to change their code to match the future default behavior (i.e. call `traverse(dp)` without `only_datapipe`): - they cannot do so because the default behavior of `traverse` hasn't changed yet, so they must use `only_datapipe=True` - if they use `only_datapipe=True`, eventually the kwarg will go away and cause a runtime error; they also get a `FutureWarning` in the present IIUC, there doesn't seem to be a way to accomplish those 3 goals without replacing the function with a new one that has a different name; hence, this PR. Let me know if there is a better alternative. If this looks right, I will send a follow up PR in `TorchData`. Differential Revision: [D39832183](https://our.internmc.facebook.com/intern/diff/D39832183) [ghstack-poisoned]

NivekT · 2022-09-27T17:50:28Z

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

NivekT · 2022-09-27T17:55:37Z

Thank you, LGTM. Do you want to open corresponding PRs for both TorchData and TorchVision?

Will do shortly.

…ps`" CI is expected to fail for now. This should be merged only after pytorch/pytorch#85667 has been merged into nightly and internal. [ghstack-poisoned]

* Replace torch.utils.data.graph.traverse with traverse_dps [ghstack-poisoned] * Update on "Replace `torch.utils.data.graph.traverse` with `traverse_dps`" CI is expected to fail for now. This should be merged only after pytorch/pytorch#85667 has been merged into nightly and internal. [ghstack-poisoned] Co-authored-by: Philip Meier <github.pmeier@posteo.de>

…ytorch#85667) This PR deprecates `traverse` function and replaces it with `traverse_datapipes` instead. While use `DataLoader`, I realized that it is raising `FutureWarning` even though I am not explicitly using `traverse`. What is happening is that `DataLoader` invokes `traverse(dp, only_datapipe=True)`, and the usage of the keyword causes the `only_datapipe` warning to be raised. ``` /home/ubuntu/miniconda3/lib/python3.8/site-packages/torch/utils/data/graph.py:102: FutureWarning: `only_datapipe` is deprecated from `traverse` function and will be removed after 1.13. warnings.warn(msg, FutureWarning) ``` A few things we'd like to do: 1. Deprecate the key word arg `only_datapipe` 2. Change the default behavior from `only_datapipe=False` to `only_datapipe=True` in the future 3. Do not raise a warning when users are using the function correctly This creates a paradox it is impossible for the users to change their code to match the future default behavior (i.e. call `traverse(dp)` without `only_datapipe`): - they cannot do so because the default behavior of `traverse` hasn't changed yet, so they must use `only_datapipe=True` - if they use `only_datapipe=True`, eventually the kwarg will go away and cause a runtime error; they also get a `FutureWarning` in the present IIUC, there doesn't seem to be a way to accomplish those 3 goals without replacing the function with a new one that has a different name; hence, this PR. Let me know if there is a better alternative. If this looks right, I will send a follow up PR in `TorchData`. Differential Revision: [D39832183](https://our.internmc.facebook.com/intern/diff/D39832183) Pull Request resolved: pytorch#85667 Approved by: https://github.com/ejguan

This should be merged only after pytorch/pytorch#85667 has been merged into nightly and internal. Differential Revision: [D39860692](https://our.internmc.facebook.com/intern/diff/D39860692) [ghstack-poisoned]

#6657) Summary: * Replace torch.utils.data.graph.traverse with traverse_dps [ghstack-poisoned] * Update on "Replace `torch.utils.data.graph.traverse` with `traverse_dps`" CI is expected to fail for now. This should be merged only after pytorch/pytorch#85667 has been merged into nightly and internal. [ghstack-poisoned] Reviewed By: YosuaMichael Differential Revision: D39885426 fbshipit-source-id: 3079b5ad4343bb3a4b2e514461472aced8fb6998 Co-authored-by: Philip Meier <github.pmeier@posteo.de>

Summary: Pull Request resolved: #793 This should be merged only after pytorch/pytorch#85667 has been merged into nightly and internal. Test Plan: Imported from OSS Reviewed By: ejguan Differential Revision: D39860692 Pulled By: NivekT fbshipit-source-id: 4fd992acfa39d0877575d739c50949fe9d52056f

…85667) This PR deprecates `traverse` function and replaces it with `traverse_datapipes` instead. While use `DataLoader`, I realized that it is raising `FutureWarning` even though I am not explicitly using `traverse`. What is happening is that `DataLoader` invokes `traverse(dp, only_datapipe=True)`, and the usage of the keyword causes the `only_datapipe` warning to be raised. ``` /home/ubuntu/miniconda3/lib/python3.8/site-packages/torch/utils/data/graph.py:102: FutureWarning: `only_datapipe` is deprecated from `traverse` function and will be removed after 1.13. warnings.warn(msg, FutureWarning) ``` A few things we'd like to do: 1. Deprecate the key word arg `only_datapipe` 2. Change the default behavior from `only_datapipe=False` to `only_datapipe=True` in the future 3. Do not raise a warning when users are using the function correctly This creates a paradox it is impossible for the users to change their code to match the future default behavior (i.e. call `traverse(dp)` without `only_datapipe`): - they cannot do so because the default behavior of `traverse` hasn't changed yet, so they must use `only_datapipe=True` - if they use `only_datapipe=True`, eventually the kwarg will go away and cause a runtime error; they also get a `FutureWarning` in the present IIUC, there doesn't seem to be a way to accomplish those 3 goals without replacing the function with a new one that has a different name; hence, this PR. Let me know if there is a better alternative. If this looks right, I will send a follow up PR in `TorchData`. Differential Revision: [D39832183](https://our.internmc.facebook.com/intern/diff/D39832183) Pull Request resolved: #85667 Approved by: https://github.com/ejguan

[DataLoader] Replacing traverse function with traverse_datapipes

ce42f41

[ghstack-poisoned]

pytorch-bot bot added the release notes: dataloader release notes category label Sep 26, 2022

facebook-github-bot added the cla signed label Sep 26, 2022

NivekT mentioned this pull request Sep 26, 2022

[DataPipe] Fix MapDataPipe spawn lambda test #85668

Closed

NivekT requested a review from ejguan September 26, 2022 21:53

NivekT added the topic: deprecation topic category label Sep 26, 2022

NivekT requested a review from VitalyFedyunin September 26, 2022 21:53

NivekT changed the title ~~[DataLoader] Replacing traverse function with traverse_datapipes~~ [DataLoader] Replacing traverse function with traverse_datapipes Sep 26, 2022

ejguan approved these changes Sep 27, 2022

View reviewed changes

test/test_datapipe.py Outdated Show resolved Hide resolved

test/test_datapipe.py Outdated Show resolved Hide resolved

test/test_datapipe.py Outdated Show resolved Hide resolved

test/test_datapipe.py Outdated Show resolved Hide resolved

ejguan mentioned this pull request Sep 27, 2022

[DataLoader2] Change graph traverse default option to only_datapipe=True #85654

Closed

pytorchmergebot closed this in 64a526d Sep 27, 2022

This was referenced Sep 27, 2022

Refactor to use traverse_dps instead of traverse meta-pytorch/data#793

Closed

Replace torch.utils.data.graph.traverse with traverse_dps pytorch/vision#6657

Merged

facebook-github-bot deleted the gh/nivekt/54/head branch October 1, 2022 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataLoader] Replacing `traverse` function with `traverse_datapipes`#85667

[DataLoader] Replacing `traverse` function with `traverse_datapipes`#85667
NivekT wants to merge 4 commits intogh/nivekt/54/basefrom
gh/nivekt/54/head

NivekT commented Sep 26, 2022 •

edited

Loading

Uh oh!

pytorch-bot bot commented Sep 26, 2022 •

edited

Loading

Uh oh!

NivekT commented Sep 26, 2022

Uh oh!

ejguan commented Sep 27, 2022

Uh oh!

NivekT commented Sep 27, 2022

Uh oh!

NivekT commented Sep 27, 2022

Uh oh!

ejguan left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NivekT commented Sep 27, 2022

Uh oh!

NivekT commented Sep 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

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

Uh oh!

Uh oh!

pytorch-bot bot commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/85667

✅ No Failures, 1 Pending

Uh oh!

NivekT commented Sep 26, 2022

Uh oh!

ejguan commented Sep 27, 2022

Uh oh!

NivekT commented Sep 27, 2022

Uh oh!

NivekT commented Sep 27, 2022

Uh oh!

ejguan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NivekT commented Sep 27, 2022

Uh oh!

NivekT commented Sep 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NivekT commented Sep 26, 2022 •

edited

Loading

pytorch-bot bot commented Sep 26, 2022 •

edited

Loading