register tensor and PIL kernel the same way as datapoints #7797

pmeier · 2023-08-02T22:50:28Z

Follow-up to #7747 and prerequisite for 2./3. in #7747 (comment).

cc @vfdev-5

pytorch-bot · 2023-08-02T22:50:30Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR

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

❌ 32 New Failures

As of commit 4068251:

NEW FAILURES - The following jobs have failed:

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

pmeier · 2023-08-02T22:51:40Z

test/test_transforms_v2_functional.py

@@ -449,6 +394,9 @@ def test_datapoint_output_type(self, info, args_kwargs):

        assert isinstance(output, type(datapoint))

+        if isinstance(datapoint, datapoints.BoundingBoxes) and info.dispatcher is not F.convert_format_bounding_boxes:


Since this test is part of the legacy framework, I was to lazy to handle this more elegantly.

test/test_transforms_v2_refactored.py

pmeier · 2023-08-02T22:59:45Z

test/test_transforms_v2_refactored.py

 def check_dispatcher(
    dispatcher,
+    # TODO: remove this parameter


We no longer need this parameter. However, we previously parametrized over it together with the function to create the input. Thus, removing it is chore that I'll deal with after release. It doesn't have any effect on the runtime, because the number of tests stays exactly the same after this parameter is removed.

torchvision/transforms/v2/functional/_utils.py

pmeier · 2023-08-02T23:17:21Z

vision/torchvision/transforms/v2/functional/_utils.py

Lines 52 to 54 in f3c89cc

    
           for registered_cls, kernel in registry.items(): 
        
               if issubclass(datapoint_cls, registered_cls): 
        
                   return kernel

as part of _get_kernel creates an issue when we register a kernel for torch.Tensor. Imagine I have class MyDatapoint(Datapoint). With the current state, I would expect it being passed through if I don't register anything for it. However, since issubclass(MyDatapoint, torch.Tensor), we select the plain tensor kernel instead and try to transform it. This is what causing the failures on prototype, since we still have class Label(Datapoint) there.

The reason why I think this loop is valuable in general rather than just having the exact type match

vision/torchvision/transforms/v2/functional/_utils.py

Lines 49 to 50 in f3c89cc

    
           if datapoint_cls in registry: 
        
               return registry[datapoint_cls]

is explained in #7747 (comment).

pmeier · 2023-08-02T23:20:17Z

torchvision/transforms/v2/functional/_color.py

 @_register_kernel_internal(rgb_to_grayscale, datapoints.Image)
 def rgb_to_grayscale_image_tensor(image: torch.Tensor, num_output_channels: int = 1) -> torch.Tensor:
+    if num_output_channels not in (1, 3):


For some reason, this previously was done on the dispatcher rather than in the kernel. I moved it to the kernel here and the same for the PIL kernel below.

torchvision/transforms/v2/functional/_color.py

pmeier · 2023-08-02T23:23:20Z

torchvision/transforms/v2/functional/_geometry.py

+@_register_kernel_internal(resize, PIL.Image.Image)
+def _resize_image_pil_dispatch(


I needed to extract this out due to the warning.

torchvision/transforms/v2/functional/_geometry.py

pmeier · 2023-08-03T21:08:16Z

#7797 (comment) is resolved now with the following rules:

Users are not allowed to register anything but datapoint subclasses with @register_kernel. Our decorator @_register_kernel_internal does not have this limitation allowing us to register kernels for PIL.Image.Image and torch.Tensor.
When retrieving a kernel with _get_kernel
1. We first check for an exact type match. For datapoints this is still just a performance improvement, while for PIL.Image.Image and torch.Tensor this is the only way to retrieve kernels.
2. If there is no exact match and the input is a datapoint, we move up its MRO looking for a kernel registered for a superclass. This is for the class MyImage(datapoints.Image) use case, but in theory also supports retrieving the kernel for datapoints.Image if it weren't for the exact type match above. In case we don't find anything, we return a no-op kernel. This is only temporary until we can go for 2. / 3. in refactor Datapoint dispatch mechanism #7747 (comment) and only happens for datapoints.
3. If there is no exact match and the input is not a datapoint, we error out.

test/test_transforms_v2_refactored.py

NicolasHug

Took a look at the utils and a few kernels, LGTM.

I'll approve so we don't have to go through another review round. I hope we can make the tests (comments above) a bit more trivial, but this is non-blocking and can be addressed later

Thanks a lot for all this Philip, I'm sure it wasn't fun shuffling so much code, but it's great to see that a lot of the clutter was removed from the dispatchers logic

torchvision/transforms/v2/functional/_utils.py

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

…egister-tensor-pil

…7797) Summary: Co-authored-by: Nicolas Hug <contact@nicolas-hug.com> Reviewed By: matteobettini Differential Revision: D48642326 fbshipit-source-id: ee9cd9d5d32b1c4433554763812e44fb6c189fc9

register tensor and PIL kernel the same way as datapoints

94cf81e

pmeier added module: transforms code quality labels Aug 2, 2023

facebook-github-bot added the cla signed label Aug 2, 2023

pmeier commented Aug 2, 2023

View reviewed changes

pmeier requested review from NicolasHug and vfdev-5 August 2, 2023 23:05

pmeier commented Aug 2, 2023

View reviewed changes

pmeier added 5 commits August 3, 2023 22:23

refactor _get_kernel and add test

1eaf82e

add test for register_kernel

390021f

Merge branch 'main' into register-tensor-pil

fe73f50

fix antialias warning for resized_crop for PIL

8c0a8ea

improve kernel registration for PIL alias kernels

5599729

cleanup

f61f595

pmeier commented Aug 3, 2023

View reviewed changes

test/test_transforms_v2_refactored.py Outdated Show resolved Hide resolved

NicolasHug reviewed Aug 4, 2023

View reviewed changes

NicolasHug approved these changes Aug 4, 2023

View reviewed changes

torchvision/transforms/v2/functional/_utils.py Show resolved Hide resolved

pmeier and others added 7 commits August 4, 2023 15:51

(hopefully) simplify tests

7f67e52

Update torchvision/transforms/v2/functional/_utils.py

dca577e

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

Merge branch 'register-tensor-pil' of github.com:pmeier/vision into r…

fe64e4c

…egister-tensor-pil

cleanup

8557cdf

more

88904db

Merge branch 'main' into register-tensor-pil

29d0cd4

Merge branch 'main' into register-tensor-pil

4068251

pmeier merged commit 2030d20 into pytorch:main Aug 7, 2023

pmeier deleted the register-tensor-pil branch August 7, 2023 07:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

register tensor and PIL kernel the same way as datapoints #7797

register tensor and PIL kernel the same way as datapoints #7797

pmeier commented Aug 2, 2023 •

edited by pytorch-bot bot

Loading

pytorch-bot bot commented Aug 2, 2023 •

edited

Loading

pmeier Aug 2, 2023

pmeier Aug 2, 2023

pmeier commented Aug 2, 2023 •

edited

Loading

pmeier Aug 2, 2023 •

edited

Loading

pmeier Aug 2, 2023

pmeier commented Aug 3, 2023

NicolasHug left a comment

		@@ -449,6 +394,9 @@ def test_datapoint_output_type(self, info, args_kwargs):

		assert isinstance(output, type(datapoint))

		if isinstance(datapoint, datapoints.BoundingBoxes) and info.dispatcher is not F.convert_format_bounding_boxes:

		@_register_kernel_internal(resize, PIL.Image.Image)
		def _resize_image_pil_dispatch(

register tensor and PIL kernel the same way as datapoints #7797

register tensor and PIL kernel the same way as datapoints #7797

Conversation

pmeier commented Aug 2, 2023 • edited by pytorch-bot bot Loading

pytorch-bot bot commented Aug 2, 2023 • edited Loading

🔗 Helpful Links

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

❌ 32 New Failures

pmeier Aug 2, 2023

Choose a reason for hiding this comment

pmeier Aug 2, 2023

Choose a reason for hiding this comment

pmeier commented Aug 2, 2023 • edited Loading

pmeier Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

pmeier Aug 2, 2023

Choose a reason for hiding this comment

pmeier commented Aug 3, 2023

NicolasHug left a comment

Choose a reason for hiding this comment

pmeier commented Aug 2, 2023 •

edited by pytorch-bot bot

Loading

pytorch-bot bot commented Aug 2, 2023 •

edited

Loading

pmeier commented Aug 2, 2023 •

edited

Loading

pmeier Aug 2, 2023 •

edited

Loading