Continue support sharding pipes in `tud.datapipes.iter.grouping` as deprecated by wenleix · Pull Request #94527 · pytorch/pytorch

wenleix · 2023-02-09T17:58:07Z

Summary:
#94095 moves this into tud.datapipes.iter.sharding. However, since previously this is a public API, this is a BC break change.

As discussed in meta-pytorch/data#987 (comment), we will have backward compatbile support but with deprecated warning.

Differential Revision: D43161015

Deprecation:
Please import from SHARDING_PRIORITIES from torch.utils.data.datapipes.iter.sharding instead of torch.utils.data.datapipes.iter.grouping.

pytorch-bot · 2023-02-09T17:58:10Z

🔗 Helpful Links

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

📄 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 the bot commands wiki or our office hours

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

❗ 2 Active SEVs

There are 2 currently active SEVs. If your PR is affected, please view them below:

✅ No Failures

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

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

facebook-github-bot · 2023-02-09T17:58:48Z

This pull request was exported from Phabricator. Differential Revision: D43161015

facebook-github-bot · 2023-02-09T18:47:22Z

This pull request was exported from Phabricator. Differential Revision: D43161015

ejguan · 2023-02-09T18:57:57Z

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

Can we add one more comment saying it's going to be removed in PyTorch 2.1?

nit: I notice that TorchData imports from torch.utils.data.datapipes.iter for ShardingFilter and torch.utils.data.datapipes.iter.sharding for SHARDING_PRIORITIES.

Should we have separate comments for each name?

I notice that TorchData imports from torch.utils.data.datapipes.iter for ShardingFilter and torch.utils.data.datapipes.iter.sharding for SHARDING_PRIORITIES.

Good point... I think tudd.iter.ShardingFilter is preferred over tudd.iter.sharding.ShardingFilterIterDataPipe.

But if user already used tudd.iter.grouping.ShardingFilterIterDataPipe, perhaps the immediate rerouting would be tudd.iter.sharding.ShardingFilterIterDataPipe ~ (and we should have seperate guide to route user from tudd.iter.sharding.ShardingFilterIterDataPipe to tudd.iter.ShardingFilter

I think we should do two warnings here because users probably won't be happy to see another warning after changing the code to the proposed fix.

Do we have a guideline on this? Since tudd.iter.sharding.ShardingFilterIterDataPipe is also a public API. While we encourage/promote to use tudd.iter.ShardingFilter, both should be supported.

If we don't want to support tudd.iter.sharding.ShardingFilterIterDataPipe (and should always use the tud.iter version, shall make it explicitly make it like tudd.iter.sharding._ShardingFilterIterDataPipe? (Similar question applies to other DataPipes under tudd.iter.XXX?

ejguan · 2023-02-09T18:58:22Z

test/test_datapipe.py

Should we add a assertWarning at import?
And, other tests can be removed as they just references to the right classes.

And, other tests can be removed as they just references to the right classes.

discussed offline, just use them as some smoke integration test~

facebook-github-bot · 2023-02-09T18:59:39Z

This pull request was exported from Phabricator. Differential Revision: D43161015

NivekT

Didn't realize __getattr__ worked for modules. LGTM after resolving the comments!

NivekT · 2023-02-09T22:19:37Z

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

nit: I notice that TorchData imports from torch.utils.data.datapipes.iter for ShardingFilter and torch.utils.data.datapipes.iter.sharding for SHARDING_PRIORITIES.

Should we have separate comments for each name?

wenleix · 2023-02-09T23:11:09Z

Didn't realize getattr worked for modules. LGTM after resolving the comments!

PEP-562: https://peps.python.org/pep-0562/ ;) . torch.ops used to have to create runtime module before it's officially supported in Python, see #72448

facebook-github-bot · 2023-02-09T23:21:06Z

This pull request was exported from Phabricator. Differential Revision: D43161015

facebook-github-bot · 2023-02-10T00:59:28Z

This pull request was exported from Phabricator. Differential Revision: D43161015

facebook-github-bot · 2023-02-10T01:08:39Z

This pull request was exported from Phabricator. Differential Revision: D43161015

facebook-github-bot · 2023-02-10T01:17:38Z

This pull request was exported from Phabricator. Differential Revision: D43161015

wenleix · 2023-02-10T04:37:14Z

@pytorchbot merge

pytorchmergebot · 2023-02-10T04:39:10Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

pytorchmergebot · 2023-02-10T04:39:13Z

Merge failed

Reason: 1 mandatory check(s) failed (Rule superuser). The first few are:

pull / linux-focal-rocm5.4.2-py3.8 / build

Dig deeper by viewing the failures on hud

Details for Dev Infra team

Raised by workflow job

facebook-github-bot · 2023-02-10T04:52:57Z

This pull request was exported from Phabricator. Differential Revision: D43161015

facebook-github-bot · 2023-02-10T05:01:16Z

This pull request was exported from Phabricator. Differential Revision: D43161015

…eprecated (pytorch#94527) Summary: Pull Request resolved: pytorch#94527 pytorch#94095 moves this into `tud.datapipes.iter.sharding`. However, since previously this is a public API, this is a BC break change. As discussed in meta-pytorch/data#987 (comment), we will have backward compatbile support but with deprecated warning. Test Plan: ``` buck test mode/opt //caffe2/test:datapipe ``` Reviewed By: ejguan Differential Revision: D43161015 fbshipit-source-id: 82049979981b83aefd247843fbc2c004a7e7a90a

facebook-github-bot · 2023-02-10T05:08:42Z

This pull request was exported from Phabricator. Differential Revision: D43161015

facebook-github-bot · 2023-02-10T18:38:52Z

@pytorchbot merge -f 'Landed internally'

(Initiating merge automatically since Phabricator Diff has merged, using force because this PR might not pass merge_rules.json but landed internally)

facebook-github-bot · 2023-02-10T18:40:17Z

@pytorchbot merge -f 'Landed internally'

(Initiating merge automatically since Phabricator Diff has merged, using force because this PR might not pass merge_rules.json but landed internally)

pytorchmergebot · 2023-02-10T18:42:05Z

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

pytorch-bot bot added the release notes: dataloader release notes category label Feb 9, 2023

facebook-github-bot added the fb-exported label Feb 9, 2023

wenleix force-pushed the export-D43161015 branch from 2888d96 to 975ca2e Compare February 9, 2023 18:47

wenleix requested review from NivekT, ejguan and pmeier and removed request for ejguan February 9, 2023 18:54

ejguan reviewed Feb 9, 2023

View reviewed changes

wenleix force-pushed the export-D43161015 branch from 975ca2e to e1e8115 Compare February 9, 2023 18:59

NivekT reviewed Feb 9, 2023

View reviewed changes

wenleix force-pushed the export-D43161015 branch from e1e8115 to f59207a Compare February 9, 2023 23:21

wenleix force-pushed the export-D43161015 branch from f59207a to 1320492 Compare February 10, 2023 00:59

wenleix force-pushed the export-D43161015 branch from 1320492 to b7083bd Compare February 10, 2023 01:08

wenleix force-pushed the export-D43161015 branch from b7083bd to fc9c477 Compare February 10, 2023 01:17

ejguan approved these changes Feb 10, 2023

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Feb 10, 2023

wenleix force-pushed the export-D43161015 branch from fc9c477 to e8cd848 Compare February 10, 2023 04:52

wenleix force-pushed the export-D43161015 branch from e8cd848 to 1d5264c Compare February 10, 2023 05:01

wenleix force-pushed the export-D43161015 branch from 1d5264c to 3a62fd5 Compare February 10, 2023 05:08

NivekT approved these changes Feb 10, 2023

View reviewed changes

pytorchmergebot added the Merged label Feb 10, 2023

pytorchmergebot closed this in d8f4026 Feb 10, 2023

Conversation

wenleix commented Feb 9, 2023 • edited by NivekT Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Feb 9, 2023 • 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/94527

❗ 2 Active SEVs

✅ No Failures

Uh oh!

facebook-github-bot commented Feb 9, 2023

Uh oh!

facebook-github-bot commented Feb 9, 2023

Uh oh!

ejguan Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

NivekT Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

wenleix Feb 9, 2023 • 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 Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

wenleix Feb 10, 2023

Choose a reason for hiding this comment

Uh oh!

ejguan Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

wenleix Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Feb 9, 2023

Uh oh!

NivekT left a comment

Choose a reason for hiding this comment

Uh oh!

NivekT Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

wenleix commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Feb 9, 2023

Uh oh!

facebook-github-bot commented Feb 10, 2023

Uh oh!

facebook-github-bot commented Feb 10, 2023

Uh oh!

facebook-github-bot commented Feb 10, 2023

Uh oh!

wenleix commented Feb 10, 2023

Uh oh!

pytorchmergebot commented Feb 10, 2023

Merge started

Uh oh!

pytorchmergebot commented Feb 10, 2023

Merge failed

Uh oh!

facebook-github-bot commented Feb 10, 2023

Uh oh!

facebook-github-bot commented Feb 10, 2023

Uh oh!

facebook-github-bot commented Feb 10, 2023

Uh oh!

facebook-github-bot commented Feb 10, 2023

Uh oh!

facebook-github-bot commented Feb 10, 2023

Uh oh!

pytorchmergebot commented Feb 10, 2023

Merge started

Uh oh!

Reviewers

Assignees

Labels

wenleix commented Feb 9, 2023 •

edited by NivekT

Loading

pytorch-bot bot commented Feb 9, 2023 •

edited

Loading

wenleix Feb 9, 2023 •

edited

Loading

wenleix commented Feb 9, 2023 •

edited

Loading