[prototype] Fix BC-breakages on input params of `F` by datumbox · Pull Request #6636 · pytorch/vision

datumbox · 2022-09-23T13:59:26Z

Depends on #6635

A number of dispatchers on stable F define one typing but handle multiple. This comes from the days where JIT didn't support Union. One such an example, is the size parameter of resize which is declared as List[int] but int is also handled internally. Due to the use of these fake types, the prototype F doesn't currently have those mitigations. This PR, tries to restore BC-breakages by correcting the types on prototype F and handling all possible inputs. The issue was first reported by @vfdev-5 for resize. I took a look and here is some additional dispatchers affected:

Here is the proposed changes for each method:

Change the typing info on the prototype to reflect the true supported types. Do this for ~~_Feature methods, F dispatchers, the actual kernels and~~ the Transform constructors. Edit: Unfortunately this is not possible for the kernels due to JIT. See this [prototype] Fix BC-breakages on input params of F #6636 (review)
Add the actual code mitigations in the kernels instead on the dispatchers, so that the behaviour remains the same no matter what the user is calling (kernels are to become public in the new API).

torchvision/prototype/transforms/functional/_geometry.py

pmeier · 2022-09-23T14:05:19Z

torchvision/prototype/features/_bounding_box.py

+        if isinstance(size, int):
+            image_size = [size, size]
+        else:
+            image_size = (size[0], size[0]) if len(size) == 1 else (size[0], size[1])


I dislike the nested if here. Maybe

Suggested change

if isinstance(size, int):

image_size = [size, size]

else:

image_size = (size[0], size[0]) if len(size) == 1 else (size[0], size[1])

if isinstance(size, int):

image_size = [size, size]

elif len(size) == 1:

image_size = [size[0], size[0]]

?

This is almost equivalent with what we had before with the only difference that the previous solution silently ignored size[2:]. Meaning, it would still work if we pass sizes of arbitrary size. My suggestion will now raise an exception in the kernel.

I tried it but the bbox expected a tuple and this needs to be handled explicitly to make mypy happy. I'll revert the previous approach which just handles the integer and maintains the previous treatment on the else but I'm happy to review on the nits where and how this should be handled. We need to discuss where exactly the exception should be thrown in this case. Possibly on the new_like method itself?

pmeier · 2022-09-23T14:06:53Z

Plus, mypy seems to be complaining about the change somewhere.

datumbox · 2022-09-23T14:17:51Z

@pmeier @vfdev-5 Thanks for the comments. The PR is half-baked so I'll make sure the nits and tests are in order before finalizing. I was hoping you can provide feedback around the overall approach described on the PR description (aka where we place the mitigations, how we align the typing etc). I take it from your comments we are aligned?

Co-authored-by: Philip Meier <github.pmeier@posteo.de>

datumbox

The main changes are mostly done, though there are lots of styles / nits we might want to see.

Several JIT tests are failing but this might be due to #6635 not being merged yet. Perhaps the following error is related to my comment at https://github.com/pytorch/vision/pull/6635/files#r978934554:

test/test_prototype_transforms_functional.py:57 (TestKernels.test_scripted_vs_eager[cpu-resize_image_tensor-167])
Traceback (most recent call last):
  File "test/test_prototype_transforms_functional.py", line 66, in test_scripted_vs_eager
    actual = kernel_scripted(*args, **kwargs)
RuntimeError: resize_image_tensor() Expected a value of type 'Union[List[int], int]' for argument 'size' but instead found type 'tuple'.
Position: 1
Value: (23, 11)
Declaration: resize_image_tensor(Tensor image, Union(int[], int) size, Enum<__torch__.torchvision.transforms.functional.InterpolationMode> interpolation=Enum<InterpolationMode.BILINEAR>, int? max_size=None, bool antialias=False) -> Tensor
Cast error details: Expected a member of Union[List[int], int] but instead found type Tuple[int, int]

torchvision/prototype/transforms/_geometry.py

datumbox · 2022-09-23T17:36:10Z

torchvision/prototype/transforms/_misc.py

            if sigma <= 0:
                raise ValueError("If sigma is a single number, it must be positive.")
-            sigma = (sigma, sigma)
+            sigma = float(sigma)


Since the _setup_float_or_seq() will do the job for us, we only need to handle the integer case.

datumbox · 2022-09-23T17:36:32Z

torchvision/prototype/transforms/_utils.py

 from typing_extensions import Literal


+def _setup_float_or_seq(arg: Union[float, Sequence[float]], name: str, req_size: int = 2) -> Sequence[float]:


Moved as-is.

torchvision/prototype/transforms/functional/_geometry.py

datumbox · 2022-09-28T12:20:40Z

Unfortunately the moment we put all the real types in the signatures (for example Union[int, List[int]]) the JIT stops working when provided with a Tuple[int, int]. Currently there seems to be some mitigation in JIT that handles Tuple[int, int] even when List[int] is expected. This mitigation breaks the moment the integers are added. It's a shame we cant define the real types but maintaining JIT-scriptability is more important. I'll be changing back the typing modifications, keeping only other BC-fixes in this PR.

Reference:

_______ TestKernels.test_scripted_vs_eager[cpu-resize_image_tensor-273] ________
Traceback (most recent call last):
  File "/home/runner/work/vision/vision/test/test_prototype_transforms_functional.py", line 66, in test_scripted_vs_eager
    actual = kernel_scripted(*args, **kwargs)
RuntimeError: resize_image_tensor() Expected a value of type 'Union[List[int], int]' for argument 'size' but instead found type 'tuple'.
Position: 1
Value: (31,)
Declaration: resize_image_tensor(Tensor image, Union(int[], int) size, Enum<__torch__.torchvision.transforms.functional.InterpolationMode> interpolation=Enum<InterpolationMode.BILINEAR>, int? max_size=None, bool antialias=False) -> Tensor
Cast error details: Expected a member of Union[List[int], int] but instead found type Tuple[int]

… List[float]]`

pmeier

LGTM if CI is green. Thanks Vasilis!

Summary: * Fix `size` in resize. * Update torchvision/prototype/features/_bounding_box.py * Address some of the comments. * Fix `output_size` in center_crop. * Fix `CenterCrop` transform * Fix `size` in five_crop. * Fix `size` in ten_crop. * Fix `kernel_size` and `sigma` in gaussian_blur. * Fix `angle` and `shear` in affine. * Fixing JIT-scriptability issues. * Update TODOs. * Restore fake types for `Union[int, List[int]]` and `Union[int, float, List[float]]` * Fixing tests * Fix linter * revert unnecessary JIT mitigations. * Cherrypick Philip's 6dfc965 * Linter fix * Adding center float casting Reviewed By: datumbox Differential Revision: D40138728 fbshipit-source-id: 5cacd77921106fab286fb7840c301e62b5a0f905 Co-authored-by: Philip Meier <github.pmeier@posteo.de> Co-authored-by: Philip Meier <github.pmeier@posteo.de>

Fix size in resize.

e41e405

datumbox added enhancement module: transforms prototype labels Sep 23, 2022

datumbox requested review from pmeier and vfdev-5 September 23, 2022 13:59

facebook-github-bot added the cla signed label Sep 23, 2022

vfdev-5 reviewed Sep 23, 2022

View reviewed changes

torchvision/prototype/transforms/functional/_geometry.py Outdated Show resolved Hide resolved

pmeier reviewed Sep 23, 2022

View reviewed changes

datumbox and others added 4 commits September 23, 2022 15:25

Merge branch 'main' into prototype/fix-function-signatures

45fbf87

Update torchvision/prototype/features/_bounding_box.py

dd528d9

Co-authored-by: Philip Meier <github.pmeier@posteo.de>

Address some of the comments.

4b49089

Fix output_size in center_crop.

0128a3a

datumbox force-pushed the prototype/fix-function-signatures branch from 9a23fcb to a5ad323 Compare September 23, 2022 15:34

Fix CenterCrop transform

e3f93fb

datumbox force-pushed the prototype/fix-function-signatures branch from a5ad323 to e3f93fb Compare September 23, 2022 15:36

datumbox added 6 commits September 23, 2022 16:40

Fix size in five_crop.

5d45374

Fix size in ten_crop.

9fb9db7

Fix kernel_size and sigma in gaussian_blur.

37b415e

Fix angle and shear in affine.

0f16b84

Fixing JIT-scriptability issues.

df5a832

Update TODOs.

5dec03d

datumbox changed the title ~~[WIP] [prototype] Fix BC-breakages on input params of F~~ [prototype] Fix BC-breakages on input params of F Sep 23, 2022

datumbox commented Sep 23, 2022

View reviewed changes

Merge branch 'main' into prototype/fix-function-signatures

e848b40

pmeier mentioned this pull request Sep 27, 2022

diversify parameter types for a couple of prototype kernels #6635

Merged

Merge branch 'main' into prototype/fix-function-signatures

6a0c24b

datumbox and others added 9 commits September 28, 2022 13:52

Restore fake types for Union[int, List[int]] and `Union[int, float,…

59d5166

… List[float]]`

Merge branch 'main' into prototype/fix-function-signatures

58867f0

Fixing tests

ea0bc55

Fix linter

d62f1eb

revert unnecessary JIT mitigations.

28119be

Cherrypick Philip's 6dfc965

f0d9d85

Linter fix

350d47c

Adding center float casting

1d2ae82

Merge branch 'main' into prototype/fix-function-signatures

352b72d

pmeier approved these changes Sep 28, 2022

View reviewed changes

datumbox merged commit b8af91a into pytorch:main Sep 28, 2022

datumbox deleted the prototype/fix-function-signatures branch September 28, 2022 15:04

This was referenced Sep 29, 2022

Use correct annotations for prototype kernels? #6668

Closed

Reduce sample inputs for prototype transform kernels #6714

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[prototype] Fix BC-breakages on input params of `F`#6636

[prototype] Fix BC-breakages on input params of `F`#6636
datumbox merged 23 commits intopytorch:mainfrom
datumbox:prototype/fix-function-signatures

datumbox commented Sep 23, 2022 •

edited

Loading

Uh oh!

Uh oh!

pmeier Sep 23, 2022

Uh oh!

datumbox Sep 23, 2022

Uh oh!

pmeier commented Sep 23, 2022

Uh oh!

datumbox commented Sep 23, 2022

Uh oh!

datumbox left a comment

Uh oh!

Uh oh!

datumbox Sep 23, 2022

Uh oh!

datumbox Sep 23, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

datumbox commented Sep 28, 2022 •

edited

Loading

Uh oh!

pmeier left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		from typing_extensions import Literal


		def _setup_float_or_seq(arg: Union[float, Sequence[float]], name: str, req_size: int = 2) -> Sequence[float]:

Conversation

datumbox commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pmeier Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

datumbox Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

pmeier commented Sep 23, 2022

Uh oh!

datumbox commented Sep 23, 2022

Uh oh!

datumbox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

datumbox Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

datumbox Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

datumbox commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmeier left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

datumbox commented Sep 23, 2022 •

edited

Loading

datumbox commented Sep 28, 2022 •

edited

Loading