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.

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

fix deprecation warning for datapipes #324

Closed
wants to merge 4 commits into from

Conversation

pmeier
Copy link
Contributor

@pmeier pmeier commented Mar 24, 2022

Sister PR to pytorch/pytorch#74685. Fixes #322. CI will fail until the change is merged upstream.

@pmeier pmeier requested a review from NivekT March 24, 2022 16:10
@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 Mar 24, 2022
Copy link
Contributor

@NivekT NivekT left a comment

Choose a reason for hiding this comment

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

Let's add the "earliest removal version/date" here too:
pytorch/pytorch#74685 (comment)

Aside from that, thanks for doing this!

@pmeier pmeier requested a review from NivekT March 24, 2022 16:31
@pmeier
Copy link
Contributor Author

pmeier commented Mar 24, 2022

Cross posting pytorch/pytorch#74685 (comment)

Note that according to BC policy of PyTorch core, we can only remove functionality if the warning stating the concrete deprecation version was emitted for two releases. Thus, if we deprecate with the next releases torch==1.12 / torchdata==0.5, we can only remove in torch==1.14 / torchdata==0.7.

Copy link
Contributor

@NivekT NivekT left a comment

Choose a reason for hiding this comment

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

Our next release is 0.4 instead of 0.5. Otherwise, it looks good. Thanks!

@pmeier pmeier requested a review from NivekT March 25, 2022 07:00
@NivekT
Copy link
Contributor

NivekT commented Apr 8, 2022

The other PR should be landed soon, so this can be merged next week

@pmeier
Copy link
Contributor Author

pmeier commented Apr 11, 2022

We get a bunch of "unclosed file" warnings for Python >= 3.8. They are valid, but unrelated to this PR. See pytorch/vision#5801.

@ejguan
Copy link
Contributor

ejguan commented Apr 11, 2022

We get a bunch of "unclosed file" warnings for Python >= 3.8. They are valid, but unrelated to this PR. See pytorch/vision#5801.

Took a quick look on the warning. It seems the warning comes from https://github.com/pytorch/vision/blob/c399c3f4957d997691561b51b0eca182ec7c3cac/torchvision/datasets/sbu.py#L51

Curious about when does this warning start to pop up? Based on git blame, the changes were made 3 years ago. To eliminate it, you may want to explicitly close the opened files in zip.

@ejguan
Copy link
Contributor

ejguan commented Apr 11, 2022

It seems we have mypy Error from deprecation_warning on TorchData CI. IIUC, the Error should be eliminated after this PR is landed.

@facebook-github-bot
Copy link
Contributor

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

@pmeier
Copy link
Contributor Author

pmeier commented Apr 12, 2022

Took a quick look on the warning. It seems the warning comes from https://github.com/pytorch/vision/blob/c399c3f4957d997691561b51b0eca182ec7c3cac/torchvision/datasets/sbu.py#L51

Nope. You linked an old dataset that is not tested in your CI. SBU with datapipes is not even landed yet (pytorch/vision#5683), so not sure where you got that from.

In any case, this is not linked to a specifc dataset as far as I can tell. See pytorch/vision#5801 for more failures that popped up when we also tested Python >= 3.8 in our CI. Let's discuss there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

Warnings for deprecated datapipes are insufficient
4 participants