Skip to content

Add typehints for torchvision.io #2543

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Sep 14, 2020
Merged

Add typehints for torchvision.io #2543

merged 15 commits into from
Sep 14, 2020

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Aug 2, 2020

I'll do this first before finishing torchvision.datasets since it is needed for the video datasets.

pmeier
pmeier commented Aug 2, 2020
View reviewed changes
@pmeier pmeier requested a review from fmassa August 2, 2020 09:20
fmassa
fmassa reviewed Aug 3, 2020
View reviewed changes
@pmeier pmeier changed the title Add typehints for torchvision.io [WIP] Add typehints for torchvision.io Aug 3, 2020
@codecov
Copy link

codecov bot commented Aug 3, 2020 •
edited
Loading

Codecov Report

Merging #2543 into master will increase coverage by 0.13%.
The diff coverage is 78.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2543      +/-   ##
==========================================
+ Coverage   70.68%   70.81%   +0.13%     
==========================================
  Files          94       94              
  Lines        8017     8718     +701     
  Branches     1276     1483     +207     
==========================================
+ Hits         5667     6174     +507     
- Misses       1945     2122     +177     
- Partials      405      422      +17
Impacted Files Coverage Δ
torchvision/io/_video_opt.py 46.34% <63.63%> (-1.16%) ⬇️
torchvision/io/image.py 71.11% <100.00%> (-0.63%) ⬇️
torchvision/datasets/lsun.py 21.05% <0.00%> (+0.62%) ⬆️
torchvision/transforms/functional_pil.py 63.84% <0.00%> (+1.12%) ⬆️
torchvision/datasets/semeion.py 34.92% <0.00%> (+1.58%) ⬆️
torchvision/datasets/usps.py 32.69% <0.00%> (+2.38%) ⬆️
torchvision/datasets/sbu.py 25.00% <0.00%> (+2.58%) ⬆️
torchvision/datasets/omniglot.py 34.78% <0.00%> (+2.86%) ⬆️
torchvision/datasets/stl10.py 28.26% <0.00%> (+3.00%) ⬆️
torchvision/datasets/sbd.py 37.34% <0.00%> (+4.01%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6db1569...3bc3703. Read the comment docs.

pmeier
pmeier commented Aug 3, 2020
View reviewed changes
Copy link
Collaborator Author

@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.

@fmassa I did my best, but without having worked with PyAV before and no type hints from their side to check against, this will most likely contain errors. Please be thorough. I've identified two places (see below) that will not work in the current state.

@pmeier
Copy link
Collaborator Author

pmeier commented Aug 3, 2020

Binary CI failures are related. I'll address them first and ping you if they are fixed.

@pmeier pmeier requested a review from fmassa August 4, 2020 05:28
fmassa
fmassa reviewed Aug 24, 2020
View reviewed changes
Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing @pmeier

Given that we are lacking CI testing on GitHub for _video_opt for now, can you please revert the changes to that file, disable mypy for it for now, and then maybe in the future once we have FFmpeg being compiled and tested in the CI we re-add it back?

@pmeier
Copy link
Collaborator Author

pmeier commented Aug 24, 2020

Sorry for the delay in reviewing @pmeier

Given that we are lacking CI testing on GitHub for _video_opt for now, can you please revert the changes to that file, disable mypy for it for now, and then maybe in the future once we have FFmpeg being compiled and tested in the CI we re-add it back?

Sure, I can do that. My motivation to go for torchvision.io stems from the fact that torchvision.datasets.video_utils makes heavy use of it. This includes _video_opt. Thus, if I remove those type hints again, we probably need careful manual checking for video datasets, since mypy can't help there.

@fmassa
Copy link
Member

fmassa commented Aug 25, 2020

Yeah, I would prefer if we leave _video_opt untouched for now. We can leave the video datasets last wrt typing.

@pmeier pmeier changed the title [WIP] Add typehints for torchvision.io Add typehints for torchvision.io Sep 8, 2020
fmassa
fmassa reviewed Sep 9, 2020
View reviewed changes
Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Last couple of comments and then I think we can merge this!

@pmeier pmeier requested a review from fmassa September 11, 2020 12:04
@pmeier
Copy link
Collaborator Author

pmeier commented Sep 11, 2020

@fmassa Test failures seem unrelated.

fmassa
fmassa approved these changes Sep 14, 2020
View reviewed changes
Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Thanks!

@fmassa fmassa merged commit f8bf06d into pytorch:master Sep 14, 2020
@pmeier pmeier deleted the typehints-io branch September 15, 2020 05:30
bryant1410 pushed a commit to bryant1410/vision-1 that referenced this pull request Nov 22, 2020
@pmeier @fmassa @bryant1410
Add typehints for torchvision.io (pytorch#2543)
303a8e9 
* enable typing check for torchvision.io

* fix existing errors

* Update torchvision/io/_video_opt.py

Co-authored-by: Francisco Massa <[email protected]>

* add ignores for FileFinder

* use python 3 type hints

* lint

* video_opt

* video

* try quote av type hints

* revert from .dim() to .ndim

* revert changes to _video_opt.py and ignore errors

* fix type hints

* fix type hints for read_video_timestamps

* change offset int to float

* remove unused import

Co-authored-by: Francisco Massa <[email protected]>
vfdev-5 pushed a commit to Quansight/vision that referenced this pull request Dec 4, 2020
@pmeier @fmassa @vfdev-5
Add typehints for torchvision.io (pytorch#2543)
c70b116 
* enable typing check for torchvision.io

* fix existing errors

* Update torchvision/io/_video_opt.py

Co-authored-by: Francisco Massa <[email protected]>

* add ignores for FileFinder

* use python 3 type hints

* lint

* video_opt

* video

* try quote av type hints

* revert from .dim() to .ndim

* revert changes to _video_opt.py and ignore errors

* fix type hints

* fix type hints for read_video_timestamps

* change offset int to float

* remove unused import

Co-authored-by: Francisco Massa <[email protected]>
@frgfm frgfm mentioned this pull request Oct 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmassa fmassa fmassa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pmeier @fmassa