Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[DataPipe] Adding a 's' to the functional names of open/list DataPipes #479

Closed
wants to merge 3 commits into from
Closed

[DataPipe] Adding a 's' to the functional names of open/list DataPipes #479

wants to merge 3 commits into from

Conversation

NivekT
Copy link
Contributor

@NivekT NivekT commented May 31, 2022 •
edited
Loading

NivekT added a commit that referenced this pull request May 31, 2022
@NivekT
[DataPipe] Adding a 's' to the functional names of open/list DataPipes
2d04683 
ghstack-source-id: 3bb1bce0576bc0ef5c298bf7bbc85b88bc8d4945
Pull Request resolved: #479
@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 May 31, 2022
ejguan
ejguan reviewed May 31, 2022
View reviewed changes
Copy link
Contributor

@ejguan ejguan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding this PR. Could you please also fix tests?

@ejguan ejguan mentioned this pull request May 31, 2022
Closed
@NivekT
Copy link
Contributor Author

NivekT commented May 31, 2022 •
edited
Loading

Thank you for adding this PR. Could you please also fix tests?

I don't think there are any existing use cases in tests but I have added some.

NivekT added a commit that referenced this pull request May 31, 2022
@NivekT
[DataPipe] Adding a 's' to the functional names of open/list DataPipes
2f885c3 
ghstack-source-id: c2d9ee08447c04e6a116943b5fb03b70edf1c722
Pull Request resolved: #479
NivekT added a commit that referenced this pull request May 31, 2022
@NivekT
[DataPipe] Adding a 's' to the functional names of open/list DataPipes
9e46695 
ghstack-source-id: 20d002fe1a679e4795f2afd7016dd0a22497c7d5
Pull Request resolved: #479
ejguan
ejguan reviewed May 31, 2022
View reviewed changes
torchdata/datapipes/iter/load/fsspec.py
Comment on lines +136 to +137
# Register for functional API for backward compatibility
IterDataPipe.register_datapipe_as_function("open_file_by_fsspec", FSSpecFileOpenerIterDataPipe)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a follow-up task, add deprecation warning for the functional API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we will need to change the registration function and IterDataPipe class in PyTorch core to do that. One possibility is described here.

Do you think we should land this PR as it is or make the changes to core first?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's land this PR first and change the core afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And, could you please update the deprecation table as well?

@ejguan ejguan mentioned this pull request May 31, 2022
Closed
@NivekT
Copy link
Contributor Author

NivekT commented May 31, 2022

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

@NivekT NivekT mentioned this pull request May 31, 2022
Closed
NivekT added a commit that referenced this pull request Jun 1, 2022
@NivekT
Adding a 's' to the functional names of open/list DataPipes (#479)
22cb967 
Summary: Pull Request resolved: #479

Test Plan: Imported from OSS

Reviewed By: ejguan

Differential Revision: D36785643

Pulled By: NivekT

fbshipit-source-id: 02c3071047ac00dd34cb83a9b392be0cfa3565b0
@NivekT NivekT mentioned this pull request Jun 1, 2022
Merged
NivekT added a commit that referenced this pull request Jun 1, 2022
@NivekT
Adding a 's' to the functional names of open/list DataPipes (#479) (#489
74c5ba3 
)

Summary: Pull Request resolved: #479

Test Plan: Imported from OSS

Reviewed By: ejguan

Differential Revision: D36785643

Pulled By: NivekT

fbshipit-source-id: 02c3071047ac00dd34cb83a9b392be0cfa3565b0
bushshrub pushed a commit to bushshrub/data that referenced this pull request Jun 2, 2022
@NivekT @bushshrub
Adding a 's' to the functional names of open/list DataPipes (pytorch#479
c36a19d 
)

Summary: Pull Request resolved: pytorch#479

Test Plan: Imported from OSS

Reviewed By: ejguan

Differential Revision: D36785643

Pulled By: NivekT

fbshipit-source-id: 02c3071047ac00dd34cb83a9b392be0cfa3565b0
@facebook-github-bot facebook-github-bot deleted the gh/NivekT/81/head branch June 4, 2022 14:17
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

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NivekT @ejguan @facebook-github-bot