Replace get_tmp_dir() with tmpdir fixture in pytest #4280

ccongge · 2021-08-16T18:30:27Z

Per title.

Replaced get_tmp_dir() in tests with tmpdir fixture
Moved get_tmp_dir() last remaining caller to test/dataset_util.py (as it is not ported to pytest yet)

facebook-github-bot · 2021-08-16T18:30:32Z

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

NicolasHug · 2021-08-17T08:02:13Z

Thanks a lot for the PR @ccongge ! This looks great so far, LMK when it's ready for a proper review.
We can safely ignore the fate-related CI issues: #4278

NicolasHug · 2021-08-17T08:04:58Z

There are still references for get_tmp_dir(), but they are not used in pytest tests but as util functions.

We can safely ignore the get_tmp_dir() that's in torchvision/datasets/imagenet.py. However it looks like there are still some occurrences in the test folder in this PR branch:

(pt) ➜  vision git:(replace_get_tmp_dir) ✗ git grep get_tmp_dir test
test/common_utils.py:def get_tmp_dir(src=None, **kwargs):
test/datasets_utils.py:from common_utils import get_tmp_dir, disable_console_output
test/datasets_utils.py:        with get_tmp_dir() as tmpdir:
test/test_datasets_samplers.py:from common_utils import get_tmp_dir, assert_equal
test/test_datasets_samplers.py:    with get_tmp_dir() as tmp_dir:
test/test_datasets_video_utils.py:from common_utils import get_tmp_dir, assert_equal
test/test_datasets_video_utils.py:    with get_tmp_dir() as tmp_dir:
test/test_transforms_tensor.py:    get_tmp_dir,
test/test_transforms_tensor.py:    with get_tmp_dir() as tmp_dir:

NicolasHug

Thanks a lot @ccongge , this look great! I made a few comments below but this is in good shape already

NicolasHug · 2021-08-17T13:11:24Z

test/test_datasets_samplers.py

-    def test_random_clip_sampler(self):
-        with get_list_of_videos(num_videos=3, sizes=[25, 25, 25]) as video_list:
+    def test_random_clip_sampler(self, tmpdir):
+        with get_list_of_videos(tmpdir, num_videos=3, sizes=[25, 25, 25]) as video_list:


I think that get_list_of_videos was written as a context manager because it was relying on get_tmp_dir() which was itself a context manager.

But now that we're getting rid of get_tmp_dir(), I believe we should be able to convert get_list_of_videos() into a simple function: we can remove the @contextlib.contextmanager decorator and change yield into a return. And here, we could just do

video_list = get_list_of_videos(tmpdir, num_videos=3, sizes=[25, 25, 25])

and indent everything to the left

Thanks, edited. :)

NicolasHug · 2021-08-17T13:13:31Z

test/test_datasets_video_utils.py

-            io.write_video(name, data, fps=f)
-
-        yield names
+def get_list_of_videos(tmpdir, num_videos=5, sizes=None, fps=None):


Oh, I'm discovering now that this function is duplicated. Let's get rid of both context managers (in this file and the other one above) and instead put the function version that I suggested in common_utils.py.

Thanks, edited. :)

NicolasHug · 2021-08-17T13:22:25Z

test/test_transforms_tensor.py

 @pytest.mark.parametrize('device', cpu_and_gpu())
 class TestColorJitter:

    @pytest.mark.parametrize('brightness', [0.1, 0.5, 1.0, 1.34, (0.3, 0.7), [0.4, 0.5]])
-    def test_color_jitter_brightness(self, brightness, device):
+    @pytest.fixture()


We should remove all the newly added @pytest.fixture() decorators, as this transforms the test into a fixture, and the test doesn't get run

Thanks, edited. :)

NicolasHug · 2021-08-17T13:31:31Z

test/test_transforms_tensor.py

@@ -85,13 +84,12 @@ def _test_class_op(method, device, meth_kwargs=None, test_exact_match=True, **ma
    batch_tensors = _create_data_batch(height=23, width=34, channels=3, num_samples=4, device=device)
    _test_transform_vs_scripted_on_batch(f, scripted_fn, batch_tensors)

-    with get_tmp_dir() as tmp_dir:
-        scripted_fn.save(os.path.join(tmp_dir, f"t_{method.__name__}.pt"))
+    scripted_fn.save(os.path.join(tmp_dir, f"t_{method.__name__}.pt"))


hm, the structure of the tests in this file is slightly awkward (for various historical reasons), and it makes it a bit cumbersome to pass the tmpdir fixture from the actual test_...() function all the way down to here.

So I would instead suggest to actually just rely on our custom get_tmp_dir() function for this and revert those bits. I'll try to find a cleaner solution in the future, potentially refactoring further those tests. This means that we should put back get_tmp_dir() in the common_utils.py file (sorry for the extra work!)

Yes. That is also what I was wondering. I moved get_tmp_dir() back to common_utils.py but still replaced other get_tmp_dir() with tmpdir fixsure except the one in _test_class_op().

test/test_transforms_tensor.py

NicolasHug

Thanks a lot @ccongge ! LGTM

Summary: * Replace in test_datasets* * Replace in test_image.py * Replace in test_transforms_tensor.py * Replace in test_internet.py and test_io.py * get_list_of_videos is util function still use get_tmp_dir * Fix get_list_of_videos siginiture * Add get_tmp_dir import * Modify test_datasets_video_utils.py for test to pass * Fix indentation * Replace get_tmp_dir in util functions in test_dataset_sampler.py * Replace get_tmp_dir in util functions in test_dataset_video_utils.py * Move get_tmp_dir() to datasets_utils.py and refactor * Fix pylint, indentation and imports * import shutil to common_util.py * Fix function signiture * Remove get_list_of_videos under context manager * Move get_list_of_videos to common_utils.py * Move get_tmp_dir() back to common_utils.py * Fix pylint and imports Reviewed By: NicolasHug Differential Revision: D30417192 fbshipit-source-id: fd5ae2ad7f21509dbe09f7df85f8d9006b9ed1ea Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

ccongge added 4 commits August 16, 2021 19:20

Replace in test_datasets*

06c8b48

Replace in test_image.py

4c79e6f

Replace in test_transforms_tensor.py

d5d3ea4

Replace in test_internet.py and test_io.py

d2b2da4

get_list_of_videos is util function still use get_tmp_dir

29c1de1

facebook-github-bot added the cla signed label Aug 16, 2021

ccongge added 2 commits August 16, 2021 21:52

Fix get_list_of_videos siginiture

9176eda

Add get_tmp_dir import

991714c

pytorch deleted a comment from facebook-github-bot Aug 16, 2021

ccongge added 2 commits August 17, 2021 08:22

Modify test_datasets_video_utils.py for test to pass

3b7a065

Fix indentation

4f6226d

ccongge added 6 commits August 17, 2021 09:24

Replace get_tmp_dir in util functions in test_dataset_sampler.py

6adeaf2

Replace get_tmp_dir in util functions in test_dataset_video_utils.py

6fbf9dc

Move get_tmp_dir() to datasets_utils.py and refactor

12fda56

Fix pylint, indentation and imports

686174c

import shutil to common_util.py

49f8e4a

Fix function signiture

03e8238

ccongge requested a review from NicolasHug August 17, 2021 12:07

ccongge marked this pull request as ready for review August 17, 2021 12:11

NicolasHug reviewed Aug 17, 2021

View reviewed changes

ccongge added 4 commits August 17, 2021 14:48

Remove get_list_of_videos under context manager

8796cbe

Move get_list_of_videos to common_utils.py

ab2435f

Move get_tmp_dir() back to common_utils.py

860b5fb

Fix pylint and imports

3304723

ccongge requested a review from NicolasHug August 17, 2021 16:00

Merge branch 'master' into replace_get_tmp_dir

78aa6ed

Merge branch 'master' into replace_get_tmp_dir

a2a8107

NicolasHug approved these changes Aug 18, 2021

View reviewed changes

NicolasHug merged commit 98cb4ea into pytorch:master Aug 18, 2021

NicolasHug added code quality module: tests labels Aug 18, 2021

pmeier mentioned this pull request Mar 8, 2023

torchhub tests are not run in CI #7398

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace get_tmp_dir() with tmpdir fixture in pytest #4280

Replace get_tmp_dir() with tmpdir fixture in pytest #4280

ccongge commented Aug 16, 2021 •

edited

Loading

facebook-github-bot commented Aug 16, 2021

NicolasHug commented Aug 17, 2021

NicolasHug commented Aug 17, 2021

NicolasHug left a comment

NicolasHug Aug 17, 2021

ccongge Aug 17, 2021

NicolasHug Aug 17, 2021 •

edited

Loading

ccongge Aug 17, 2021

NicolasHug Aug 17, 2021

ccongge Aug 17, 2021

NicolasHug Aug 17, 2021

ccongge Aug 17, 2021

NicolasHug left a comment

Replace get_tmp_dir() with tmpdir fixture in pytest #4280

Replace get_tmp_dir() with tmpdir fixture in pytest #4280

Conversation

ccongge commented Aug 16, 2021 • edited Loading

facebook-github-bot commented Aug 16, 2021

Action Required

Process

NicolasHug commented Aug 17, 2021

NicolasHug commented Aug 17, 2021

NicolasHug left a comment

Choose a reason for hiding this comment

NicolasHug Aug 17, 2021

Choose a reason for hiding this comment

ccongge Aug 17, 2021

Choose a reason for hiding this comment

NicolasHug Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

ccongge Aug 17, 2021

Choose a reason for hiding this comment

NicolasHug Aug 17, 2021

Choose a reason for hiding this comment

ccongge Aug 17, 2021

Choose a reason for hiding this comment

NicolasHug Aug 17, 2021

Choose a reason for hiding this comment

ccongge Aug 17, 2021

Choose a reason for hiding this comment

NicolasHug left a comment

Choose a reason for hiding this comment

ccongge commented Aug 16, 2021 •

edited

Loading

NicolasHug Aug 17, 2021 •

edited

Loading