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

Add warnings checks for v2 namespaces #7288

Merged
merged 6 commits into from
Feb 21, 2023
Merged

Add warnings checks for v2 namespaces #7288

merged 6 commits into from
Feb 21, 2023

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Feb 20, 2023 •
edited by pytorch-bot bot
Loading

This PR adds tests related to warnings we raise in the v2 namespaces (transforms, datapoints, dataset wrapper). Also added import tests related to the deprecated function_pil and functiona_tensor modules

cc @vfdev-5 @pmeier

@NicolasHug NicolasHug requested review from pmeier and vfdev-5 February 20, 2023 11:11
pmeier
pmeier reviewed Feb 20, 2023
View reviewed changes
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.

A few minor comments inline. Instead of scattering these tests in multiple files, should we maybe put them all into test_namespaces.py or the like?

test/common_utils.py Outdated Show resolved Hide resolved
test/test_transforms_v2.py Show resolved Hide resolved
test/conftest.py


torchvision.disable_beta_transforms_warning()

from common_utils import CUDA_NOT_AVAILABLE_MSG, IN_FBCODE, IN_OSS_CI, IN_RE_WORKER, OSS_CI_GPU_NO_CUDA_MSG
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you add this for #7265 or because we actually need it for these tests. I'm not against it, but want to make sure I understand correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is mostly a drive-by, I stole it from #7282.

I think #7278 added some v2 imports in common_utils and we started seeing the warnings in the test suite, so now we have to disable them before we import anything from common_utils.

@NicolasHug
Copy link
Member Author

Instead of scattering these tests in multiple files, should we maybe put them all into test_namespaces.py or the like?

I put the v2-related test in test_transforms_v2.py, the rest is related to the deprecated functional_[pil, tensor].py so I feel like they should be separate?

pmeier
pmeier approved these changes Feb 20, 2023
View reviewed changes
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.

Yeah after the duplicate removal, it looks a lot nicer. Especially since we will have all tests in a single file after 0.17. Thanks Nicolas!

NicolasHug
NicolasHug commented Feb 20, 2023
View reviewed changes
test/test_transforms.py
),
)
@pytest.mark.parametrize("from_private", (True, False))
def test_functional_deprecation_warning(import_statement, from_private):
Copy link
Member Author

@NicolasHug NicolasHug Feb 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I realize this is slightly overkill, we don't need assert_run_python_script to check the warnings of the deprecated (public) files, we could just use a normal test. But we kinda still need it to properly check the lack of warnings for the now private files, so I mixed both. The whole thing will be removed soon anyway.

@NicolasHug NicolasHug mentioned this pull request Feb 20, 2023
Closed
@NicolasHug NicolasHug merged commit 7fe5136 into pytorch:main Feb 21, 2023
NicolasHug added a commit to NicolasHug/vision that referenced this pull request Feb 21, 2023
@NicolasHug @pmeier
Add warnings checks for v2 namespaces and deprecated files (pytorch#7288
24f991d 
)

Co-authored-by: Philip Meier <[email protected]>
This was referenced Feb 21, 2023
Merged
Closed
@malfet
Copy link
Contributor

malfet commented Feb 27, 2023

This broke unittest for 3.10, as one can observe in hud:
image

@malfet malfet mentioned this pull request Feb 27, 2023
Closed
8 tasks
facebook-github-bot pushed a commit that referenced this pull request Mar 29, 2023
@NicolasHug @facebook-github-bot
[fbsync] Add warnings checks for v2 namespaces and deprecated files (#…
1202b33 
…7288)

Summary: Co-authored-by: Philip Meier <[email protected]>

Reviewed By: vmoens

Differential Revision: D44416588

fbshipit-source-id: 745e518fcca11abee87de3ced91571206c2e13fb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmeier pmeier pmeier approved these changes

@vfdev-5 vfdev-5 Awaiting requested review from vfdev-5

Assignees
No one assigned
Labels
cla signed module: tests module: transforms
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NicolasHug @malfet @pmeier @facebook-github-bot