Skip to content

Ignore deprecation warning from traverse function #6641

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

Merged
merged 5 commits into from
Sep 27, 2022

Conversation

ejguan
Copy link
Contributor

@ejguan ejguan commented Sep 23, 2022

Since pytorch/pytorch#84079 is landed eventually, the CI for prototype tests will turn green tmrw when PyTorch nightly is updated. However, this PR introduces a FutureWarning that will deprecate only_datapipe and the default behavior of traverse will be changed as only_datapipe=True. And, this PR is suppressing this Warning turning to Error in the test.

Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

@ejguan there is only one test that currently uses only_datapipe=False:

@pytest.mark.parametrize("only_datapipe", [False, True])
@parametrize_dataset_mocks(DATASET_MOCKS)
def test_traversable(self, dataset_mock, config, only_datapipe):

This test together with

def test_serializable(self, dataset_mock, config):

and

def test_data_loader(self, dataset_mock, config, num_workers):

are only there to make sure our datasets work seamlessly with all torchdata utilities and don't error or throw warnings. If only_datapipe=False is going to be deprecated, we can simply remove the parametrization here instead of ignoring the warning. cc @NicolasHug

Since you are already looking at this: do these three test still cover all utilities that might interact with our datasets from your side or should we add or remove something? 😇

@NicolasHug
Copy link
Member

If only_datapipe=False is going to be deprecated, we can simply remove the parametrization here instead of ignoring the warning. cc @NicolasHug

I agree, ignoring the warning will make our tests break when the parameter is removed for good. We should remove its usage now instead.

@ejguan
Copy link
Contributor Author

ejguan commented Sep 26, 2022

If only_datapipe=False is going to be deprecated, we can simply remove the parametrization here instead of ignoring the warning. cc @NicolasHug

I agree, ignoring the warning will make our tests break when the parameter is removed for good. We should remove its usage now instead.

For sure, I will do a quick update by removing only_datapipe shortly.

Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

@ejguan I took the liberty to make the changes to fix our CI.

@pmeier
Copy link
Collaborator

pmeier commented Sep 27, 2022

mypy failures are unrelated and will be fixed in #6652.

Copy link
Member

@NicolasHug NicolasHug 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 both, LGTM

@NicolasHug NicolasHug merged commit 06d9726 into pytorch:main Sep 27, 2022
facebook-github-bot pushed a commit that referenced this pull request Sep 29, 2022
Summary:
* Ignore deprecation warning from traverse function

* remove only_datapipe

* reinstate warning ignore for DL test

* ignore warning class wide due to priority

Reviewed By: YosuaMichael

Differential Revision: D39885430

fbshipit-source-id: 2c3dbc36cbba0c24ae18b4dfa18e5f7256420b56

Co-authored-by: Philip Meier <[email protected]>
Co-authored-by: Nicolas Hug <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants