-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Fix some annotations in transforms v2 for JIT v1 compatibility #7252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -96,25 +96,6 @@ def xfail_jit_python_scalar_arg(name, *, reason=None): | |||
) | |||
|
|||
|
|||
def xfail_jit_tuple_instead_of_list(name, *, reason=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually observed that our functionals didn't work for tuples, but missed to check if v1 enforces this. Since we have aligned the behavior now, we can also remove this helper as it is no longer in use.
xfail_jit_python_scalar_arg("shear"), | ||
xfail_jit_tuple_instead_of_list("fill"), | ||
# TODO: check if this is a regression since it seems that should be supported if `int` is ok |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were on the right track 🤦
@@ -450,21 +430,21 @@ def _full_affine_params(**partial_params): | |||
] | |||
|
|||
|
|||
def get_fills(*, num_channels, dtype, vector=True): | |||
def get_fills(*, num_channels, dtype): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now make sure that we get all possible fill types.
@@ -12,7 +12,7 @@ | |||
|
|||
D = TypeVar("D", bound="Datapoint") | |||
FillType = Union[int, float, Sequence[int], Sequence[float], None] | |||
FillTypeJIT = Union[int, float, List[float], None] | |||
FillTypeJIT = Optional[List[float]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert this to what we had in v1 ...
@@ -118,7 +118,7 @@ def resized_crop( | |||
def pad( | |||
self, | |||
padding: Union[int, Sequence[int]], | |||
fill: FillTypeJIT = None, | |||
fill: Optional[Union[int, float, List[float]]] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... but keep it for F.pad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot Philip.
IIUC, you didn't really add new tests to make sure everything is OK, and instead you removed some of the xfail
marks so that the pre-existing tests are actually ran? Are we sure they cover all the cases we want to support?
Regardless, I'll approve to unblock so we can merge ASAP and test these new changes against #7159
Yes and no. Yes, I've removed some xfails, but I also expanded the tested parameters. See #7252 (comment). Previously we didn't test for single value lists or tuples in general for |
…ty (#7252) Reviewed By: vmoens Differential Revision: D44416629 fbshipit-source-id: ab4950cc6c3d313355f29c069838fb96fe9a2dbf
TL;DR this reverts changes to the annotation of the
fill
andpadding
parameter that we made for v2, but turned out to not be compatible with the JIT behavior of v1.We have three groups of transforms that take the
fill
parameter in v1:vision/torchvision/transforms/autoaugment.py
Lines 117 to 118 in f6b5b82
vision/torchvision/transforms/transforms.py
Lines 1412 to 1413 in f6b5b82
transforms.Pad
vision/torchvision/transforms/transforms.py
Lines 408 to 412 in f6b5b82
In eager mode v1 and v2 behave the same and we enforce that in our consistency tests. However, for JIT they behave differently. The transforms aren't annotating the
fill
parameter, so we have to look at the functionals:vision/torchvision/transforms/autoaugment.py
Line 14 in f6b5b82
vision/torchvision/transforms/functional.py
Line 1148 in f6b5b82
F.pad
:vision/torchvision/transforms/functional.py
Line 495 in f6b5b82
Let's start with the AA and affine family, since they use the same annotation:
On
main
this prints:So how did that happen? In v2 we changed the annotation to
vision/torchvision/prototype/datapoints/_datapoint.py
Line 15 in f6b5b82
Meaning, failures for the list of integers are "expected" (we'll get to why later), but what happened to the tuples? v1 didn't annotate them either?
This is caused by some (undocumented) automagic of JIT. Annotating something with
List[int]
will automatically handle tuple inputs as well:However, if correct the annotation to
Union[int, List[int]]
, the automagic is no longer applied:Well, we could just add
Tuple[int]
to the new annotation, right? Nope.Tuple[int]
is not the equivalent toList[int]
. That would beTuple[int, ...]
, but that is not supported by JIT. And sincefill
corresponds to the number of channels that will only be known at runtime, we cannot useTuple[int, int, int]
or any other fixed number. Meaning, we need to rely on the automagic for BC and need to revert our annotation changes.So this is the end of the story? Nope again. As we saw above,
F.pad
uses different annotations. Let's run our script from above withNone
does not work while scripting)Meaning, we can keep our new annotation for
F.pad
since the v2 variant supports a superset of the values of v1 in eager as in scripted mode.What's left is the
padding
argument onF.pad
:vision/torchvision/transforms/functional.py
Line 495 in f6b5b82
vision/torchvision/prototype/transforms/functional/_geometry.py
Line 1134 in f6b5b82
You can probably see where this is going. Changing the script from above to
and iterating over different
padding
values gives us:Meaning, we need to revert the new annotation and rely on the automagic handling.
If this whole story wasn't so sad, this should probably should have been a blog post rather than a PR description.
cc @vfdev-5 @bjuncek