Skip to content

Added missing typing annotations in datasets/video_utils #4172

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 14 commits into from
Nov 22, 2021

Conversation

frgfm
Copy link
Contributor

@frgfm frgfm commented Jul 13, 2021

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

Any feedback is welcome!

cc @pmeier

@pmeier
Copy link
Collaborator

pmeier commented Jul 15, 2021

I remember there was some issue preventing me from adding these annotations earlier. I think it was missing annotations in torchvision.io since we depend on that here. @fmassa is it fine to add these now?

Edit: I just saw #4173 lets do that one first.

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.

@frgfm the mypy failures are related:

torchvision/datasets/kinetics.py:137: error: Argument 4 to "VideoClips" has
incompatible type "Optional[float]"; expected "Optional[int]"  [arg-type]
                frame_rate,
                ^
Found 1 error in 1 file (checked 110 source files)

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

frgfm commented Jul 29, 2021

@frgfm the mypy failures are related:

torchvision/datasets/kinetics.py:137: error: Argument 4 to "VideoClips" has
incompatible type "Optional[float]"; expected "Optional[int]"  [arg-type]
                frame_rate,
                ^
Found 1 error in 1 file (checked 110 source files)

Hi @pmeier,
I think this was due to another PR not being merged yet! I just merged master after my other PRs got merged and everything seems fine locally (it includes the PR about kinetics)!

@frgfm frgfm requested a review from pmeier July 29, 2021 15:11
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 @frgfm for the update! I have some comments inline.

@datumbox datumbox requested a review from pmeier August 16, 2021 11:05
@pmeier
Copy link
Collaborator

pmeier commented Aug 16, 2021

@datumbox Since the annotations heavily depend on torchvision.io._video_opts that will be annotated in #4173, let's finish that one first. Afterwards we can rebase and see if mypy still happy with the current implementation.

@frgfm
Copy link
Contributor Author

frgfm commented Nov 18, 2021

@datumbox Since the annotations heavily depend on torchvision.io._video_opts that will be annotated in #4173, let's finish that one first. Afterwards we can rebase and see if mypy still happy with the current implementation.

Just updated the PR @pmeier 👍

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.

Three minor things left.

@@ -320,22 +326,22 @@ def get_clip(self, idx):
end_pts = clip_pts[-1].item()
video, audio, info = read_video(video_path, start_pts, end_pts)
else:
info = _probe_video_from_file(video_path)
video_fps = info.video_fps
_info = _probe_video_from_file(video_path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we now have

allow_redefinition = True

this renaming is probably not needed anymore. Could you check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did and unfortunately it's still an issue

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems even with allow_redefinition, mypy is stricter than I thought:

Allows variables to be redefined with an arbitrary type, as long as the redefinition is in the same block and nesting level as the original definition. [emphasis mine]

Since that is not the case here, we need to keep your fix.

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.

LGTM when CI is happy. Thanks a lot @frgfm!

@pmeier pmeier requested a review from datumbox November 18, 2021 16:10
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. I have a few comments.

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.

@prabhat00155 All the question can be answered with "Because we are using a static analyzer on dynamic code". Usually when we encounter weirdness, it is either because mypy is unnecessarily strict or we actually have spaghetti / unsafe code that should be refactored. Given that the latter is usually not obvious, we opted to simply silence mypy where it complains, since the code works as is.

If you want to see all the errors, you can checkout this PR locally, revert the changes you had questions about and run mypy.

@prabhat00155 prabhat00155 merged commit 999ef25 into pytorch:main Nov 22, 2021
@frgfm frgfm deleted the video-utils-typing branch November 22, 2021 21:22
facebook-github-bot pushed a commit that referenced this pull request Nov 30, 2021
)

Summary:
* style: Fixed last missing typing annotation

* style: Fixed typing

* style: Fixed remaining typing annotations

* style: Fixed typing

* style: Fixed typing

* refactor: Removed unused import

* Update torchvision/datasets/video_utils.py

Reviewed By: NicolasHug

Differential Revision: D32694309

fbshipit-source-id: c9aadbdc09d15de9a2a6a2b9b6a99edd829d89f2

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.

4 participants