Skip to content

Added typing annotations to models/video #4229

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 15 commits into from
Aug 23, 2021

Conversation

frgfm
Copy link
Contributor

@frgfm frgfm commented Jul 29, 2021

Following up on #2025, this PR adds missing typing annotations in models/video.

Any feedback is welcome!

cc @datumbox

@frgfm
Copy link
Contributor Author

frgfm commented Aug 2, 2021

Same here @fmassa, the failed test looks unrelated, ready for review 👌

Copy link
Contributor

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

I spotted few cases where the intended type might be different from what you added. Unfortunately our code was not too clear either in this case. I flagged some of them, let me know your thoughts.

zero_init_residual=False):
def __init__(
self,
block: Type[Union[BasicBlock, Bottleneck]],
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe block should be an nn.Module. The intention is to allow users pass their own custom blocks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, but apart from nn.Module default class, it needs an expansion attribute

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, leave as is. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
block: Type[Union[BasicBlock, Bottleneck]],
block: Callable[..., Union[BasicBlock, Bottleneck]],

@frgfm frgfm requested a review from datumbox August 23, 2021 10:52
datumbox
datumbox previously approved these changes Aug 23, 2021
Copy link
Contributor

@datumbox datumbox 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. Looks good to me. I left few more non-blocking nits.

Could you also please merge the main branch to your branch (master got remained to main) to see if the failing tests are fixed?

@datumbox datumbox dismissed their stale review August 23, 2021 11:06

Failures at build_docs seem related.

@frgfm
Copy link
Contributor Author

frgfm commented Aug 23, 2021

@datumbox Any clue about the reason of this failure?
Apart from if it was because of the out-of-date branch, I haven't modified anything related to the sphinx toctree so the error looks confusing to me 🤔

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

Apparently

./torchvision/models/video/resnet.py:223 in private method `__init__`:
        D417: Missing argument descriptions in the docstring (argument(s) conv_makers are missing descriptions in '__init__' docstring)

Try these:

zero_init_residual=False):
def __init__(
self,
block: Type[Union[BasicBlock, Bottleneck]],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
block: Type[Union[BasicBlock, Bottleneck]],
block: Callable[..., Union[BasicBlock, Bottleneck]],

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

LGTM @frgfm. Thanks for the PR.

I'll merge once the CI is green.

@datumbox datumbox merged commit 11d3629 into pytorch:main Aug 23, 2021
@frgfm frgfm deleted the models-video-typing branch August 23, 2021 17:49
facebook-github-bot pushed a commit that referenced this pull request Aug 26, 2021
Summary:
* style: Added typing to models/video

* style: Fixed typing

* style: Fixed typing

* style: Fixed typing

* refactor: Removed default value for stem

* docs: Fixed docstring of VideoResNet

* style: Refactored typing

* docs: Fixed docstring

* style: Fixed typing

* docs: Specified docstring

* typing: Fixed tying

* docs: Fixed docstring

* Undoing change.

Reviewed By: fmassa

Differential Revision: D30525888

fbshipit-source-id: 0015be22677c06f07d5d8f3c2e46fe05c2a455e4
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