-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
[prototype] Video Classes Clean up #6751
Conversation
features.VideoTypeJIT, | ||
features.VideoTypeJIT, | ||
features.VideoTypeJIT, | ||
], |
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.
JIT doesn't like this:
____________ TestDispatchers.test_scripted_smoke[cpu-five_crop-06] _____________
Traceback (most recent call last):
File "/home/runner/work/vision/vision/test/test_prototype_transforms_functional.py", line 24, in script
return torch.jit.script(fn)
File "/opt/hostedtoolcache/Python/3.7.14/x64/lib/python3.7/site-packages/torch/jit/_script.py", line 1344, in script
qualified_name, ast, _rcb, get_default_args(obj)
RuntimeError: false INTERNAL ASSERT FAILED at "../aten/src/ATen/core/union_type.cpp":212, please report a bug to PyTorch. After type unification was performed, the Union with the original types {Tuple[Tensor, Tensor, Tensor, Tensor, Tensor] Tuple[Tensor, Tensor, Tensor, Tensor, Tensor], } has the single type Tuple[Tensor, Tensor, Tensor, Tensor, Tensor]. Use the common supertype instead of creating a Uniontype
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "/home/runner/work/vision/vision/test/test_prototype_transforms_functional.py", line 221, in test_scripted_smoke
dispatcher = script(info.dispatcher)
File "/home/runner/work/vision/vision/test/common_utils.py", line 225, in wrapper
raise exc_tb[0].with_traceback(exc_tb[1])
File "/home/runner/work/vision/vision/test/common_utils.py", line 228, in wrapper
out = fn(*args, **kwargs)
File "/home/runner/work/vision/vision/test/test_prototype_transforms_functional.py", line 26, in script
raise AssertionError(f"Trying to `torch.jit.script` '{fn.__name__}' raised the error above.") from error
AssertionError: Trying to `torch.jit.script` 'five_crop' raised the error above.
76a66fd
to
50722ee
Compare
@@ -1382,16 +1382,13 @@ def five_crop_video( | |||
return five_crop_image_tensor(video, size) | |||
|
|||
|
|||
ImageOrVideoTypeJIT = Union[features.ImageTypeJIT, features.VideoTypeJIT] |
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.
Necessary to get around the JIT issue.
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.
JIT is just so cumbersome ... It seems it recognizes that this Union
flattens to torch.Tensor
, but if we use that for the output it bails out ...
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.
Two comments out of morbid curiosity, but otherwise LGTM is CI is green. Thanks Vasilis!
@@ -1382,16 +1382,13 @@ def five_crop_video( | |||
return five_crop_image_tensor(video, size) | |||
|
|||
|
|||
ImageOrVideoTypeJIT = Union[features.ImageTypeJIT, features.VideoTypeJIT] |
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.
JIT is just so cumbersome ... It seems it recognizes that this Union
flattens to torch.Tensor
, but if we use that for the output it bails out ...
inpt: features.ImageOrVideoTypeJIT, size: List[int], vertical_flip: bool = False | ||
) -> List[features.ImageOrVideoTypeJIT]: | ||
inpt: Union[features.ImageTypeJIT, features.VideoTypeJIT], size: List[int], vertical_flip: bool = False | ||
) -> Union[List[features.ImageTypeJIT], List[features.VideoTypeJIT]]: |
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.
I haven't seen that before. So this works, but it doesn't for tuples? 🤯
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.
Yes. This is possibly because they do various mitigations for Tuples as we've seen previously.
Summary: * Removing unnecessary methods/classes. * Unions instead of ImageOrVideo types * Fixing JIT issue. Reviewed By: NicolasHug Differential Revision: D40427460 fbshipit-source-id: 4ffb95e8e13240b302b9236467c95afb82d17c17
Few of the final clean ups on the API:
ImageOrVideo
type in favour of a Union