Porting to pytest #3996

tanvimoharir · 2021-06-07T17:56:09Z

group A
These ones could be bundled into a single test_random() function and be parametrized over
func, method, fn_kwargs and match_kwargs.

group F

test_random_erasing -- maybe split this one into different functions, and parametrize over test_configs
test_convert_image_dtype -- parametrize over all loop variables and convert the continue and the assertRaises into a pytest.xfail
test_autoaugment -- parametrize over policy and fill

facebook-github-bot · 2021-06-07T17:56:14Z

Hi @tanvimoharir!

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!

facebook-github-bot · 2021-06-07T18:19:54Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

…nvimoharir/vision into port-test-transform-to-pytest

NicolasHug · 2021-06-09T16:49:46Z

thanks for the PR @tanvimoharir ! I see this is still draft, let me know if you need any help, and when this is ready for a review! Thanks!

tanvimoharir · 2021-06-09T17:27:48Z

thanks for the PR @tanvimoharir ! I see this is still draft, let me know if you need any help, and when this is ready for a review! Thanks!

@NicolasHug thanks and sorry its taking me few tries. I'm actually a bit stuck with how to parametrize the T.AutoAugmentPolicy (which is an enum) from https://github.com/tanvimoharir/vision/blob/port-test-transform-to-pytest/test/test_transforms_tensor.py#L656
I tried with passing a list (list(T.AutoAugmentPolicy)) but I think mypy complains there.(same for T.AutoAugmentPolicy. __ members __ .items() or T.AutoAugmentPolicy. __ members __ .keys())
I am trying to find out a way of doing this.

test/test_transforms_tensor.py

NicolasHug

Thanks @tanvimoharir

I made a few comments but this looks good. In particular I think we should remove the use of cpu_only everywhere and instead use the cpu_and_gpu() parametrization.

Let me know if you need any help!

NicolasHug · 2021-06-10T08:34:53Z

test/test_transforms_tensor.py

-                s_transform.save(os.path.join(tmp_dir, "t_autoaugment.pt"))
+@cpu_only
+@pytest.mark.parametrize(
+    'func,method,device,fn_kwargs,match_kwargs', [


Suggested change

'func,method,device,fn_kwargs,match_kwargs', [

'func, method, device, fn_kwargs, match_kwargs', [

NicolasHug · 2021-06-10T08:39:06Z

test/test_transforms_tensor.py

-        if s_transform is not None:
-            with get_tmp_dir() as tmp_dir:
-                s_transform.save(os.path.join(tmp_dir, "t_autoaugment.pt"))
+@cpu_only


This test is actually called twice: with self.device = 'cpu' and with self.device = 'cuda' from the CUDATester class.

So instead of using the @cpu_only decorator, we should parametrize with a new

@pytest.mark.parametrize('device', cpu_and_gpu())

and replace self.device by device.

You'll need to remove device from the parametrization below as well :)

NicolasHug · 2021-06-10T08:39:46Z

test/test_transforms_tensor.py

@@ -481,7 +383,7 @@ def test_resized_crop_save(self):


 @unittest.skipIf(not torch.cuda.is_available(), reason="Skip if no CUDA device")
-class CUDATester(Tester):
+class CUDATester(unittest.TestCase):


we should be able to remove this class now

NicolasHug · 2021-06-10T08:40:15Z

test/test_transforms_tensor.py

@@ -608,6 +510,86 @@ def test_to_grayscale(device, Klass, meth_kwargs):
    )


+@cpu_only


here as well we should parametrize with cpu_and_gpu() instead.

In fact we should do it in all of the other tests here that were using self.device

So the self.device had a value of 'cpu' which is why I thought using cpu_only() instead of cpu_and_gpu()

Yes, but as I mentioned above all these tests were called twice: once as part of Tester with 'cpu', and once as part of CudaTester with 'cuda'. Which is why we need to parametrize over 'device' with cpu_and_gpu() now :)

Okay, thanks for clarifying :)

NicolasHug · 2021-06-10T08:44:21Z

test/test_transforms_tensor.py

+@pytest.mark.parametrize(
+    'in_dtype,out_dtype', [
+        (int_dtypes() + float_dtypes(), int_dtypes() + float_dtypes())
+    ]
+)


there are 2 nested for loops here so we should not parametrize over tuples, instead we should have 2 separate paraemtrization to have a cross-produce:

@pytest.mark.parametrize('in_dtype', int_dtypes() + float_dtypes()) @pytest.mark.parametrize('out_dtype', int_dtypes() + float_dtypes())

NicolasHug · 2021-06-10T08:44:45Z

test/test_transforms_tensor.py

@@ -608,6 +510,86 @@ def test_to_grayscale(device, Klass, meth_kwargs):
    )


+@cpu_only
+@pytest.mark.xfail()


why mark xfail? This should probably be removed

NicolasHug · 2021-06-10T08:45:39Z

test/test_transforms_tensor.py

+    with get_tmp_dir() as tmp_dir:
+        scripted_fn.save(os.path.join(tmp_dir, "t_convert_dtype.pt"))


this can take a bit of time, especially when the test iss heavily parametrized. Here and in the rest of the test, let's extract the saving part into separate tests. Here we could name it test_convert_image_dtype_save()

NicolasHug · 2021-06-10T08:46:22Z

test/test_transforms_tensor.py

+        _test_transform_vs_scripted(transform, s_transform, tensor)
+        _test_transform_vs_scripted_on_batch(transform, s_transform, batch_tensors)
+
+    if s_transform is not None:


let's extract this out as well in another test without parametrization

NicolasHug · 2021-06-10T08:46:38Z

test/test_transforms_tensor.py

+    with get_tmp_dir() as tmp_dir:
+        scripted_fn.save(os.path.join(tmp_dir, "t_random_erasing.pt"))


here as well

test/test_transforms_tensor.py

…nsform-to-pytest

github-actions · 2021-06-10T15:52:30Z

Hey @NicolasHug!

You approved or merged this PR, but no labels were added.

NicolasHug · 2021-06-10T15:52:33Z

Thanks a lot @tanvimoharir ! I just took care of the merge conflicts and I removed some parametrization on the _save() tests

tanvimoharir · 2021-06-10T15:53:18Z

Thanks a lot @tanvimoharir ! I just took care of the merge conflicts and I removed some parametrization on the _save() tests

Thank you for helping me with this. 👍

Reviewed By: fmassa Differential Revision: D29097733 fbshipit-source-id: 6ab7e5bb7c1d21e3aba922bb52659aab65e5abdf

Porting to pytest

56beb8f

Merge branch 'pytorch:master' into port-test-transform-to-pytest

fe2c62e

facebook-github-bot added the cla signed label Jun 7, 2021

tanvimoharir and others added 5 commits June 7, 2021 23:59

Lint fixes

867e614

Merge branch 'port-test-transform-to-pytest' of https://github.com/ta…

2a7bd30

…nvimoharir/vision into port-test-transform-to-pytest

Using decorator for cpu_only

c292f35

Update T.AutoAugmentPolicy call

1710947

Merge branch 'master' into port-test-transform-to-pytest

1f7a6dd

NicolasHug mentioned this pull request Jun 9, 2021

Port test/test_transforms_tensor.py to pytest #3987

Closed

6 tasks

Adding missing paranthesis

bb2184f

Changes for failed tests

43b1e33

Merge branch 'master' into port-test-transform-to-pytest

fa8feba

NicolasHug reviewed Jun 9, 2021

View reviewed changes

test/test_transforms_tensor.py Outdated Show resolved Hide resolved

tanvimoharir added 2 commits June 9, 2021 23:42

Lint fixes

5c5773b

Changes

df62652

tanvimoharir marked this pull request as ready for review June 9, 2021 19:19

Merge branch 'master' into port-test-transform-to-pytest

5accfe5

NicolasHug reviewed Jun 10, 2021

View reviewed changes

Changes from review comments

ed48ad2

NicolasHug reviewed Jun 10, 2021

View reviewed changes

test/test_transforms_tensor.py Show resolved Hide resolved

tanvimoharir and others added 5 commits June 10, 2021 19:48

Adding 'return' to fix failure

1c1e5b1

Merge branch 'master' into port-test-transform-to-pytest

0c88667

remove parametrization from _save tests

28b2d9b

Merge branch 'master' of github.com:pytorch/vision into port-test-tra…

a6d5392

…nsform-to-pytest

Merge branch 'master' into port-test-transform-to-pytest

f3fdb20

NicolasHug merged commit 13ed657 into pytorch:master Jun 10, 2021

NicolasHug added code quality module: tests module: transforms labels Jun 10, 2021

tanvimoharir deleted the port-test-transform-to-pytest branch June 10, 2021 15:53

facebook-github-bot pushed a commit that referenced this pull request Jun 14, 2021

[fbsync] Finish porting test_transforms_tensor to pytest (#3996)

4bee713

Reviewed By: fmassa Differential Revision: D29097733 fbshipit-source-id: 6ab7e5bb7c1d21e3aba922bb52659aab65e5abdf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Porting to pytest #3996

Porting to pytest #3996

tanvimoharir commented Jun 7, 2021 •

edited

Loading

facebook-github-bot commented Jun 7, 2021

facebook-github-bot commented Jun 7, 2021

NicolasHug commented Jun 9, 2021

tanvimoharir commented Jun 9, 2021 •

edited

Loading

NicolasHug left a comment

NicolasHug Jun 10, 2021

NicolasHug Jun 10, 2021

NicolasHug Jun 10, 2021

NicolasHug Jun 10, 2021

NicolasHug Jun 10, 2021

tanvimoharir Jun 10, 2021 •

edited

Loading

NicolasHug Jun 10, 2021

tanvimoharir Jun 10, 2021

NicolasHug Jun 10, 2021

NicolasHug Jun 10, 2021

NicolasHug Jun 10, 2021

NicolasHug Jun 10, 2021

NicolasHug Jun 10, 2021

github-actions bot commented Jun 10, 2021

NicolasHug commented Jun 10, 2021

tanvimoharir commented Jun 10, 2021

	'func,method,device,fn_kwargs,match_kwargs', [
	'func, method, device, fn_kwargs, match_kwargs', [

		@@ -608,6 +510,86 @@ def test_to_grayscale(device, Klass, meth_kwargs):
		)


		@cpu_only

		with get_tmp_dir() as tmp_dir:
		scripted_fn.save(os.path.join(tmp_dir, "t_convert_dtype.pt"))

Porting to pytest #3996

Porting to pytest #3996

Conversation

tanvimoharir commented Jun 7, 2021 • edited Loading

facebook-github-bot commented Jun 7, 2021

Action Required

Process

facebook-github-bot commented Jun 7, 2021

NicolasHug commented Jun 9, 2021

tanvimoharir commented Jun 9, 2021 • edited Loading

NicolasHug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanvimoharir Jun 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 10, 2021

NicolasHug commented Jun 10, 2021

tanvimoharir commented Jun 10, 2021

tanvimoharir commented Jun 7, 2021 •

edited

Loading

tanvimoharir commented Jun 9, 2021 •

edited

Loading

tanvimoharir Jun 10, 2021 •

edited

Loading