remove private imports from torch.testing #7525

pmeier · 2023-04-17T22:10:50Z

No description provided.

pytorch-bot · 2023-04-17T22:10:52Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/7525

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 3 Failures

As of commit f9e77a6:

NEW FAILURES - The following jobs have failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

pmeier · 2023-04-17T22:16:09Z

test/common_utils.py


-    if error_metas:
-        raise error_metas[0].to_error(msg)
+    for actual_item, expected_item in zip(actual_flat, expected_flat):


The only "regression" compared to the patched assert_close is that we don't get any information where inside a sample an error happened. Consider the input sample to a detection model, i.e. a two-tuple with an image and a target dictionary. If the comparison of the bounding boxes target fails, assert_close would include [1]["boxes"] in the error message, whereas now we will only see the regular message and thus have to figure out ourselves what part of the input caused this.

We can re-implement this behavior if needed, but not sure its worth it. I guesstimate that 99% of our comparisons are single elements and thus no extra traceback is needed.

Apart from this, assert_close_with_image_support should be a faithful reproduction of the patched assert_close we had before.

pmeier · 2023-04-17T22:16:54Z

test/common_utils.py

+            # This is a dirty hack that let's us "hook" into the comparison logic of `torch.testing.assert_close`. It
+            # will only be triggered in case of failed value comparison. This let's us reuse all of the attribute
+            # checking that `torch.testing.assert_close` does while giving us the ability to ignore the regular value
+            # comparison.


Per comment. Note while this is a hack, it doesn't depend on any non-public functionality or assumptions.

NIT

Suggested change

# This is a dirty hack that let's us "hook" into the comparison logic of `torch.testing.assert_close`. It

# will only be triggered in case of failed value comparison. This let's us reuse all of the attribute

# checking that `torch.testing.assert_close` does while giving us the ability to ignore the regular value

# comparison.

# This is a dirty hack that lets us "hook" into the comparison logic of `torch.testing.assert_close`. It

# will only be triggered in case of failed value comparison. This lets us reuse all of the attribute

# checking that `torch.testing.assert_close` does while giving us the ability to ignore the regular value

# comparison.

NicolasHug

Thanks Philip, some quick pass

NicolasHug · 2023-04-20T14:23:04Z

test/common_utils.py

+            nonlocal value_comparison_failure
+            value_comparison_failure = True


IIUC this is the "hack" part and the rest below is just the same as the implementation for the default msg_callback?

Yes. Basically this is our way of detecting that there was value comparison failure since this callback is only called in this specific case.

NicolasHug · 2023-04-20T14:25:10Z

test/common_utils.py

+            if not (value_comparison_failure and mae):
+                raise


I might be misunderstanding, but why do we even bother trying torch.testing.assert_close when mae is True?

assert_close does a lot more checks than just the value comparison. For us the most important ones are shape, dtype, and device checks. If we don't invoke assert_close, we'll have to do this ourselves or live with the fact that check_dtype=True is ignored when mae=True is set.

NicolasHug · 2023-04-20T14:27:04Z

test/common_utils.py

+        if actual.dtype is torch.uint8:
+            actual, expected = actual.to(torch.int), expected.to(torch.int)
+        mae_value = float(torch.abs(actual - expected).float().mean())


(Hopefully correct?) NIT

Suggested change

if actual.dtype is torch.uint8:

actual, expected = actual.to(torch.int), expected.to(torch.int)

mae_value = float(torch.abs(actual - expected).float().mean())

mae_value = torch.abs(actual.float() - expected.float()).mean()

NicolasHug · 2023-04-20T14:27:45Z

test/common_utils.py

+            # This is a dirty hack that let's us "hook" into the comparison logic of `torch.testing.assert_close`. It
+            # will only be triggered in case of failed value comparison. This let's us reuse all of the attribute
+            # checking that `torch.testing.assert_close` does while giving us the ability to ignore the regular value
+            # comparison.


NIT

Suggested change

# This is a dirty hack that let's us "hook" into the comparison logic of `torch.testing.assert_close`. It

# will only be triggered in case of failed value comparison. This let's us reuse all of the attribute

# checking that `torch.testing.assert_close` does while giving us the ability to ignore the regular value

# comparison.

# This is a dirty hack that lets us "hook" into the comparison logic of `torch.testing.assert_close`. It

# will only be triggered in case of failed value comparison. This lets us reuse all of the attribute

# checking that `torch.testing.assert_close` does while giving us the ability to ignore the regular value

# comparison.

NicolasHug · 2023-04-20T14:29:58Z

test/common_utils.py


-        self._compare_attributes(actual, expected)
-        actual, expected = self._equalize_attributes(actual, expected)
+        enable_mae_comparison = all(isinstance(input, torch.Tensor) for input in [actual, expected])


Should this be

Suggested change

enable_mae_comparison = all(isinstance(input, torch.Tensor) for input in [actual, expected])

enable_mae_comparison = all(isinstance(input, torch.Tensor) for input in [actual, expected]) and mae

? Regardless I don't really understand the reason for enable_mae_comparison. Why can't we just convert all inputs to tensors and apply mae comparison iff mae = True?

I wrote this as a general function that one can use like assert_close, but with_image_support. Meaning, you can still do assert_close_with_image_support(None, 5) and get a proper error message. If we force everything into a tensor, that would not be possible.

But are we ever going to use it when the inputs aren't images (either PIL or Tensors)?

Yes, we have such usages:

In the functional v2 tests, we are using assert_close (or now assert_close_with_image_support) for all kernels and dispatchers and this of course also means bounding boxes, masks and videos.

In the consistency tests for the detection and segmentation tests, we test full samples against each other and thus the inputs are more than just images. This is somewhat also the case in our (fairly limited) transforms tests, but will probably increase there when we ramp them up.

NicolasHug · 2023-04-20T14:30:36Z

test/common_utils.py

-        actual, expected = self.actual, self.expected
+def assert_close_with_image_support(actual, expected, *, mae=False, atol=None, msg=None, **kwargs):
+    def compare(actual, expected):
+        if all(isinstance(input, PIL.Image.Image) for input in [actual, expected]):


Why not convert all inputs to tensors, regardless of whether they're all PIL Images? Does that mean we can't do assert_close_with_image_support(some_tensor, some_pil_img)?

See #7525 (comment) for one reason. Plus, we originally had the option to pass in one tensor and one PIL image and let the function handle the conversion to tensor. However, we often saw extra flakiness in the tests just to this conversion and thus we scraped that feature. Since this irregular anyway, the few tests that need this, perform the conversion themselves now. E.g.

vision/test/transforms_v2_kernel_infos.py

Line 117 in 5579995

def pil_reference_wrapper(pil_kernel):

remove private imports from torch.testing

6e9e607

pmeier added the module: tests label Apr 17, 2023

pmeier requested a review from NicolasHug April 17, 2023 22:10

facebook-github-bot added the cla signed label Apr 17, 2023

pmeier commented Apr 17, 2023

View reviewed changes

pmeier added 2 commits April 18, 2023 00:19

fix prototype tests

726adde

Merge branch 'main' into assert-close-public

f9e77a6

NicolasHug reviewed Apr 20, 2023

View reviewed changes

		nonlocal value_comparison_failure
		value_comparison_failure = True

	enable_mae_comparison = all(isinstance(input, torch.Tensor) for input in [actual, expected])
	enable_mae_comparison = all(isinstance(input, torch.Tensor) for input in [actual, expected]) and mae

remove private imports from torch.testing #7525

Are you sure you want to change the base?

remove private imports from torch.testing #7525

Uh oh!

Conversation

pmeier commented Apr 17, 2023

Uh oh!

pytorch-bot bot commented Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/7525

❌ 3 Failures

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pytorch-bot bot commented Apr 17, 2023 •

edited

Loading