move passthrough for unknown types from dispatchers to transforms #7804
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/7804
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ❌ 1 New FailureAs of commit 8932f74: NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
| if datapoint_cls in _BUILTIN_DATAPOINT_TYPES: | ||
| raise ValueError(f"Kernels cannot be registered for the builtin datapoint classes, but got {datapoint_cls}") |
There was a problem hiding this comment.
I don't think this is really needed. The previous error message was fine, but anyway.
Note that this means users can't override e.g. erase for bboxes, but this could be a desirable feature.
There was a problem hiding this comment.
Isn't that exactly what we wanted? We don't want users to register custom kernels for our builtin datapoints. Otherwise
vision/torchvision/transforms/v2/functional/_utils.py
Lines 130 to 134 in 8faa1b1
would be valid again.
We could special case F.erase and the other outliers, but I feel users could come to us if they see the warning. We could need the input, since we ourselves are not 100% sure what we should actually do there.
There was a problem hiding this comment.
It's just that erase is a bit special in that we probably could have a kernel for it, we just haven't implemented it yet. So it's reasonable for users to want to have their own. But that's not super important.
This reverts commit 9f51230.
| @@ -187,66 +184,6 @@ def test__get_params(self, mocker): | |||
| assert params["needs_pad"] | |||
| assert any(pad > 0 for pad in params["padding"]) | |||
|
|
|||
| @pytest.mark.parametrize("needs", list(itertools.product((False, True), repeat=2))) | |||
| def test__transform(self, mocker, needs): | |||
There was a problem hiding this comment.
We remove this test, because it never checked correctness, but rather if the specific dispatchers were being called. Due to our new architecture, they are never called and thus this test fails.
The same argument applies to the removed tests in test/test_transforms_v2.py.
| ("dispatcher", "registered_input_types"), | ||
| [(dispatcher, set(registry.keys())) for dispatcher, registry in _KERNEL_REGISTRY.items()], | ||
| ) | ||
| def test_exhaustive_kernel_registration(dispatcher, registered_input_types): |
There was a problem hiding this comment.
We no longer have an exhaustive registration and thus this test needs to be removed.
|
Hey @pmeier! You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py |
…forms (#7804) Reviewed By: matteobettini Differential Revision: D48642257 fbshipit-source-id: c562182ae2f6bcf344bf7adc7f5d07cd1da627d3
Addresses 2. / 3. of #7747 (comment). This is done by making the noop behavior of
_get_kerneloptional (turned off by default) and add a new method on the transformsdef _call_or_noop(dispatcher, inpt, *args, **kwargs). Instead of calling any dispatcher directly inside_transform, we now go through the new method to retain the noop behavior for the transforms. For examplechanges to
cc @vfdev-5