Skip to content

Added typing annotations to io/_video_opts #4173

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

Merged
merged 29 commits into from
Nov 17, 2021

Conversation

frgfm
Copy link
Contributor

@frgfm frgfm commented Jul 13, 2021

Following up on #2025, this PR adds missing typing annotations in io/_video_opts.py.

Any feedback is welcome!

Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @frgfm. Unfortunately this breaks torch.jit. Relevant test failure from test_probe_video_from_memory_script - test.test_video_reader.TestVideoReader

E           RuntimeError: 
E           Unknown type name 'np.ndarray':
E             File "/root/project/torchvision/io/_video_opt.py", line 432
E           def _probe_video_from_memory(
E               video_data: Union[torch.Tensor, np.ndarray],
E                                               ~~~~~~~~~~ <--- HERE
E           ) -> VideoMetaData:
E               """

@frgfm
Copy link
Contributor Author

frgfm commented Jul 19, 2021

Thanks for the PR @frgfm. Unfortunately this breaks torch.jit. Relevant test failure from test_probe_video_from_memory_script - test.test_video_reader.TestVideoReader

E           RuntimeError: 
E           Unknown type name 'np.ndarray':
E             File "/root/project/torchvision/io/_video_opt.py", line 432
E           def _probe_video_from_memory(
E               video_data: Union[torch.Tensor, np.ndarray],
E                                               ~~~~~~~~~~ <--- HERE
E           ) -> VideoMetaData:
E               """

Oh, should I remove the typing for this function only then @pmeier?

@pmeier
Copy link
Collaborator

pmeier commented Jul 20, 2021

The problem here is that torch.jit currently does not support the Union type. There is a PR pytorch/pytorch#53180 that adds support, but it has not been landed yet.

I think the way forward is to try to eliminate the Union and try to get mypy to live with it. If that is not possible than yes, we need to remove the annotations again since in general torch.jit > mypy.

@frgfm frgfm mentioned this pull request Jul 28, 2021
@frgfm
Copy link
Contributor Author

frgfm commented Jul 31, 2021

Apart from the Union issue with torch.jit, I checked locally and mypy is working perfectly.
I had to ignore assignment I couldn't get to work at the end of the file, and if you have an idea how to get around the jit issue, I'm all ears @pmeier :)

@frgfm frgfm requested a review from pmeier July 31, 2021 13:21
Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @frgfm and sorry for the delay.

Apart from the Union issue with torch.jit

Until this is fixed upstream, there is nothing we can do about it.

I had to ignore assignment I couldn't get to work at the end of the file

I've answered inline with some more comments.

@frgfm frgfm requested a review from pmeier September 18, 2021 14:05
Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @frgfm!

@pmeier pmeier requested a review from fmassa September 20, 2021 12:28
@@ -438,15 +447,20 @@ def _probe_video_from_memory(video_data):
return info


def _convert_to_sec(start_pts, end_pts, pts_unit, time_base):
def _convert_to_sec(start_pts: float, end_pts: float, pts_unit: str, time_base: Fraction) -> Tuple[float, float, str]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that start_pts, end_pts, *pts* switch between ints and floats in different methods. It's worth checking that in all signatures where we declare them as ints are indeed always ints. That might be easier to do with a debugger.

cc @prabhat00155

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the other comment, there is no clear dtype in

torch::List<torch::Tensor> read_video_from_file(
and considering it's not casted afterwards, I'm not sure how to answer this 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed the other comment. I understand it's not straightforward to identify the types. That's why I think finding out what they are and annotating the code-base is useful. Nevertheless if we fail to annotate them correctly (say they are floats and we mark them as ints) it's going to be really confusing.

Can we confirm the types of the various pts vars by running the unit-tests and putting a debugger to observe their materialized types?

@prabhat00155 prabhat00155 self-requested a review September 20, 2021 21:23
@datumbox
Copy link
Contributor

Unfortunately you got a few linter and typing failures.

@frgfm frgfm requested a review from prabhat00155 November 17, 2021 15:03
Copy link
Contributor

@prabhat00155 prabhat00155 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @frgfm!

Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor things left from my side. LGTM, when the other open comments are resolved and CI is happy. Thanks a lot @frgfm!

@prabhat00155 prabhat00155 merged commit 29e0f66 into pytorch:main Nov 17, 2021
@github-actions
Copy link

Hey @prabhat00155!

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

@frgfm frgfm deleted the video-opt-typing branch November 18, 2021 09:01
facebook-github-bot pushed a commit that referenced this pull request Nov 30, 2021
Summary:
* style: Added typing annotations

* style: Fixed lint

* style: Fixed typing

* chore: Updated mypy.ini

* style: Fixed typing

* chore: Updated mypy.ini

* style: Fixed typing compatibility with jit

* style: Fixed typing

* style: Fixed typing

* style: Fixed missing import

* style: Fixed typing of __iter__

* style: Fixed typing

* style: Fixed lint

* style: Finished typing

* style: ufmt the file

* style: Removed unnecessary typing

* style: Fixed typing of iterator

Reviewed By: NicolasHug

Differential Revision: D32694320

fbshipit-source-id: afdd8c0a70cfdba91a5c349a5961051b993185a1

Co-authored-by: Philip Meier <[email protected]>
Co-authored-by: Prabhat Roy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants