Skip to content

Added annotation typing to shufflenet #2864

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 6 commits into from
Nov 6, 2020
Merged

Conversation

frgfm
Copy link
Contributor

@frgfm frgfm commented Oct 21, 2020

Hi there!

As per #2025, annotation typing are welcome in torchvision. So, this PR focuses on torchvision.models.shufflenet this PR!

Any feedback is welcome!

@pmeier pmeier self-assigned this Oct 21, 2020
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.

Minor nitpick plus incremental changes from the other PRs. Otherwise LGTM.

@frgfm
Copy link
Contributor Author

frgfm commented Oct 22, 2020

Still getting a troublesome error with mypy though:

torchvision/models/shufflenetv2.py:148: error: "Tensor" not callable  [operator]
            x = self.stage2(x)
                ^
torchvision/models/shufflenetv2.py:149: error: "Tensor" not callable  [operator]
            x = self.stage3(x)
                ^
torchvision/models/shufflenetv2.py:150: error: "Tensor" not callable  [operator]
            x = self.stage4(x)

Any ideas? I tried a few things in vain unfortunately

@fmassa
Copy link
Member

fmassa commented Oct 23, 2020

I'll let @pmeier have a look, I don't know why mypy is thinking that stage2 is a Tensor. One thing to note is that stage2 is created programmatically with setattr, which might confuse mypy. Maybe there is a way to annotate stage{n} to tell mypy that it's a nn.Module ?

@pmeier
Copy link
Collaborator

pmeier commented Oct 23, 2020

I don't know why mypy is thinking that stage2 is a Tensor

Me neither. I think that might be caused by the way nn.Module is typed, but I'm certainly not sure.

One thing to note is that stage2 is created programmatically with setattr, which might confuse mypy. Maybe there is a way to annotate stage{n} to tell mypy that it's a nn.Module ?

We could simply include self.stage2: nn.Module in the constructor. Since mypy only inspects but does not the code we can't use a loop for that. Thus I propose to wrap the loop in a function an call it several times from the constructor.

@fmassa
Copy link
Member

fmassa commented Oct 23, 2020

Thus I propose to wrap the loop in a function an call it several times from the constructor

I'm not exactly sure how to do it, given that we use setattr. So you propose to make the change that we instead do by hand

self.stage2 = create(...)

?

That is a big big change for just typing, can't we disable mypy somehow?

@pmeier
Copy link
Collaborator

pmeier commented Oct 23, 2020

can't we disable mypy somehow?

Sure we can. We could also casting in forward

x = cast(nn.Sequential, self.stage2)(x)

or put a block of static annotations above the creation loop

self.stage1: nn.Sequential
self.stage2: nn.Sequential

@facebook-github-bot
Copy link

Hi @frgfm!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

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 052edce into pytorch:master Nov 6, 2020
Comment on lines +127 to +129
self.stage2: nn.Sequential
self.stage3: nn.Sequential
self.stage4: nn.Sequential
Copy link
Member

Choose a reason for hiding this comment

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

note: it would have been preferable to let stage{} be a nn.Module, as they could potentially be any type of nn.Module, not just Sequential

@frgfm frgfm deleted the shufflenet-typing branch November 6, 2020 13:52
bryant1410 pushed a commit to bryant1410/vision-1 that referenced this pull request Nov 22, 2020
* style: Added annotation typing for shufflenet

* fix: Removed duplicate type hint

* refactor: Removed un-necessary import

* fix: Fixed constructor typing

* style: Added black formatting on depthwise_conv

* style: Fixed stage typing in shufflenet
vfdev-5 pushed a commit to Quansight/vision that referenced this pull request Dec 4, 2020
* style: Added annotation typing for shufflenet

* fix: Removed duplicate type hint

* refactor: Removed un-necessary import

* fix: Fixed constructor typing

* style: Added black formatting on depthwise_conv

* style: Fixed stage typing in shufflenet
@frgfm frgfm mentioned this pull request Oct 27, 2022
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