Add typing annotations to models/quantization #4232

oke-aditya · 2021-07-30T11:44:52Z

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

Any feedback is welcome!

…yping3

oke-aditya

I have a few doubts. I have posted them
Let me know! I will change accordingly

This needs a review @pmeier @frgfm

~~Edit: CI is all green (:crossed_fingers: )~~

torchvision/models/quantization/googlenet.py

torchvision/models/quantization/inception.py

torchvision/models/quantization/mobilenetv2.py

torchvision/models/quantization/mobilenetv3.py

torchvision/models/quantization/shufflenetv2.py

frgfm

Thanks for the PR!
Though, I have to say: have you run mypy over this to check if this works? perhaps focus on part where you are almost positive about the typing, otherwise it takes a really long time for reviewers to go over your work 😅

My review isn't finished, but I'm already getting the feeling that this should be split into different PRs considering the volume of modifications. But @pmeier is the authority on this matter :)

torchvision/models/quantization/googlenet.py

torchvision/models/quantization/inception.py

oke-aditya

Can someone again have a look? cc @frgfm @pmeier

torchvision/models/quantization/googlenet.py

oke-aditya

It would be nice if someone can review! @fgfm or @fmassa.

I'm not sure what to do of the init error log posted over build by mypy.
At the moment I think Either we would need to rewrite the init or ignore.

Let me know your thoughts 😃

torchvision/models/quantization/googlenet.py

oke-aditya · 2021-08-02T19:06:02Z

torchvision/models/quantization/shufflenetv2.py

@@ -1,11 +1,15 @@
 import torch
 import torch.nn as nn
+from torch import Tensor
+from typing import Any
+


I had to do a slight rewrite to make mypy happy. Let me know if this is fine.

torchvision/models/quantization/shufflenetv2.py

torchvision/models/quantization/utils.py

…yping3

oke-aditya · 2021-08-05T05:30:51Z

Here are the errors and what are being suppressed.

torchvision/models/quantization/utils.py:31: error: Incompatible types in assignment (expression has type "QConfig", variable has type "Union[Tensor, Module]")  [assignment]
            model.qconfig = torch.quantization.QConfig(
                            ^

torchvision/models/quantization/utils.py:35: error: "Tensor" not callable  [operator]
model.fuse_model()
        ^

The cat cat errors

torchvision/models/quantization/shufflenetv2.py:33: error: Argument 1 to "cat" of "FloatFunctional" has incompatible type "Tuple[Tensor, Any]"; expected "List[Tensor]"  [arg-type]
                out = self.cat.cat((x1, self.branch2(x2)), dim=1)
                                    ^
torchvision/models/quantization/shufflenetv2.py:35: error: Argument 1 to "cat" of "FloatFunctional" has incompatible type "Tuple[Any, Any]"; expected "List[Tensor]"  [arg-type]
                out = self.cat.cat((self.branch1(x), self.branch2(x)), dim=1)
                                    ^

The init errors, this probably has a nice workaround

torchvision/models/quantization/inception.py:111: error: "__init__" of "InceptionA" gets multiple values for keyword argument "conv_block"  [misc]
            super(QuantizableInceptionA, self).__init__(conv_block=QuantizableBasicConv2d, *args, **kwargs)
            ^

torchvision/models/quantization/inception.py:184: error: "__init__" of "InceptionAux" gets multiple values for keyword argument "conv_block"  [misc]
            super(QuantizableInceptionAux, self).__init__(conv_block=QuantizableBasicConv2d, *args, **kwargs)
            ^

Assigning a layer of model to None error

torchvision/models/quantization/googlenet.py:81: error: Incompatible types in assignment (expression has type "None", variable has type Module)  [assignment]
                model.aux1 = None
                             ^

For now I have skipped the errors (except init error) by specifying return type

@frgfm can you have a look ?
Once we solve the above issues (either telling mypy to ignore or fix it) The PR would be good to go

pmeier

Hey @oke-aditya and sorry for the delay. Apart from your questions I've added two more comments inline.

torchvision/models/quantization/utils.py:31: error: Incompatible types in assignment (expression has type "QConfig", variable has type "Union[Tensor, Module]")  [assignment]
            model.qconfig = torch.quantization.QConfig(
                            ^

The ignore is justified, because we are adding an attribute to a live object and mypy can't deal with that.

torchvision/models/quantization/utils.py:35: error: "Tensor" not callable  [operator]
model.fuse_model()
        ^

mypy complains, because it correctly can't find a callable fuse_model parameter on an nn.Module. Thus, we need make the input type annotation more precise. I'm guessing we only accepting the quantized models that we have built-in? If yes, we could create a custom class mixin (for example QuantizedModuleMixin) that also inherits from nn.Module and adds this as abstract method. All our models could inherit from making fuse_model also available for checking.

The cat cat errors

torchvision/models/quantization/shufflenetv2.py:33: error: Argument 1 to "cat" of "FloatFunctional" has incompatible type "Tuple[Tensor, Any]"; expected "List[Tensor]"  [arg-type]
                out = self.cat.cat((x1, self.branch2(x2)), dim=1)
                                    ^
torchvision/models/quantization/shufflenetv2.py:35: error: Argument 1 to "cat" of "FloatFunctional" has incompatible type "Tuple[Any, Any]"; expected "List[Tensor]"  [arg-type]
                out = self.cat.cat((self.branch1(x), self.branch2(x)), dim=1)
                                    ^

There are actually two errors here:

We are passing a tuple when a list is expected. We should be able to replace the parentheses with brackets: (x1, self.branch2(x2)) to [x1, self.branch2(x2)].
self.branch*() has no proper return type, so we need to cast ourselves: self.branch2(x2) to cast(torch.Tensor, self.branch2(x2))

The init errors, this probably has a nice workaround

torchvision/models/quantization/inception.py:111: error: "__init__" of "InceptionA" gets multiple values for keyword argument "conv_block"  [misc]
            super(QuantizableInceptionA, self).__init__(conv_block=QuantizableBasicConv2d, *args, **kwargs)
            ^

torchvision/models/quantization/inception.py:184: error: "__init__" of "InceptionAux" gets multiple values for keyword argument "conv_block"  [misc]
            super(QuantizableInceptionAux, self).__init__(conv_block=QuantizableBasicConv2d, *args, **kwargs)
            ^

I don't think so, because we currently make the hard assumption that no one passes conv_block as keyword argument. If someone would do, we would get an error at runtime, because we also pass it.

If we insist on using **kwargs here, I'm afraid there is no clean way to do it. One workaround would be to add a conv_block: None = None before the **kwargs and bail out if we encounter anything else than None. cc @fmassa @datumbox

Note that there are a lot more of these in the CI failure.

Assigning a layer of model to None error

torchvision/models/quantization/googlenet.py:81: error: Incompatible types in assignment (expression has type "None", variable has type Module)  [assignment]
                model.aux1 = None
                             ^

Porbably fine, because we do the same thing in the regular GoogLeNet.

vision/torchvision/models/googlenet.py

Lines 56 to 57 in db530d8

    
           model.aux1 = None  # type: ignore[assignment] 
        
           model.aux2 = None  # type: ignore[assignment]

Not sure if this is a jit issue, but maybe we can type them as Optional[nn.Module] in the first place?

torchvision/models/quantization/googlenet.py

torchvision/models/quantization/inception.py

…yping3

oke-aditya · 2021-08-16T19:57:56Z

Hi Phillip!

Thanks a lot for detailed analysis! Such work teaches a lot to a newbie developer like me. 🙏
For now I'm fixing up the annotations by adding quotes.

As you said there isn't a very neat workaround for the init errors, so probably we can take that in a new PR after some discussion.

Hence, I think that we can suppress the bunch of init errors we are getting in CI

Let me know further steps 😄

pmeier · 2021-08-17T06:03:04Z

As you said there isn't a very neat workaround for the init errors, so probably we can take that in a new PR after some discussion.

Hence, I think that we can suppress the bunch of init errors we are getting in CI

That is fair in the scope of this PR. Can you open an issue afterwards so we can track this? Maybe also add a quick # TODO comment in the code linking to my comment above.

The same (ignore + issue) holds for the fuse_model() part detailed above.

Let me know further steps 😄

The cat error seems to be fixable without larger refactorings. Could you try and see if my suggestion passes CI? After that the PR is good to go!

…yping3

oke-aditya · 2021-08-17T19:18:34Z

Hi @pmeier and @datumbox
The cat.cat issue is fixed by using a List instead of Tuple. And the remaining init errors are silenced with a #TODO.

So I think this is good to go! I will open the issue as per Phillip's comments once merged.

pmeier

LGTM, thanks a lot @oke-aditya! I've cleaned up the #TODO comments to always be directly over the def __init__ line. Plus, I've added one the .fuse_model() call.

torchvision/models/quantization/inception.py

torchvision/models/quantization/mobilenetv3.py

torchvision/models/quantization/shufflenetv2.py

torchvision/models/quantization/utils.py

oke-aditya · 2021-08-20T12:42:15Z

Just pinging for visibility @datumbox (Probably he was busy in other PRs 😃)

pmeier · 2021-08-20T12:53:54Z

Hey @oke-aditya, Vasilis' review will be delayed until next week. Unfortunately, besides me no one has time to review now, so we'll have to wait. Sorry for that.

oke-aditya · 2021-08-20T12:55:41Z

No problem Philip ! 🙂

oke-aditya · 2021-08-28T06:31:13Z

Hmm unsure if it is appropriate. But just re-tagging @datumbox

Edit: Sorry, I pinged on Weekend. :(

datumbox

@oke-aditya Apologies for the late response. The addition of EfficientNets took most of my time last week. I think we are good to merge this one. I'll wait for the green tests.

Concerning the valid remarks that @pmeier raised for the conv_block, I believe we can consider this internal only for now. There some new developments on quantization so we might revisit how we do it.

Summary: * fix * add typings * fixup some more types * Type more * remove mypy ignore * add missing typings * fix a few mypy errors * fix mypy errors * fix mypy * ignore types * fixup annotation * fix remaining types * cleanup #TODO comments Reviewed By: fmassa Differential Revision: D30793343 fbshipit-source-id: 0448f6f24f406827abc9e1825489c786b6f0eb11 Co-authored-by: Philip Meier <[email protected]> Co-authored-by: Vasilis Vryniotis <[email protected]>

oke-aditya added 11 commits May 20, 2021 12:47

fix

b2f6615

Merge branch 'master' of https://github.com/pytorch/vision

4fb038d

Merge branch 'master' of https://github.com/pytorch/vision

deda5d7

Merge branch 'master' of https://github.com/pytorch/vision

5490821

Merge branch 'master' of https://github.com/pytorch/vision

4cfc220

Merge branch 'master' of https://github.com/pytorch/vision

6306746

Merge branch 'master' of https://github.com/pytorch/vision

e8c93cf

Merge branch 'master' of https://github.com/pytorch/vision

6871ccc

Merge branch 'master' of https://github.com/pytorch/vision

80060bf

Merge branch 'master' of https://github.com/pytorch/vision

3b5f0ca

add typings

304fa9e

facebook-github-bot added the cla signed label Jul 30, 2021

oke-aditya added 2 commits July 30, 2021 18:58

Merge branch 'master' of https://github.com/pytorch/vision into add_t…

b4b35b3

…yping3

fixup some more types

a20ee80

oke-aditya commented Jul 30, 2021

View reviewed changes

frgfm reviewed Jul 30, 2021

View reviewed changes

Type more

78ddc54

oke-aditya commented Jul 30, 2021

View reviewed changes

torchvision/models/quantization/googlenet.py Outdated Show resolved Hide resolved

torchvision/models/quantization/googlenet.py Outdated Show resolved Hide resolved

frgfm mentioned this pull request Jul 31, 2021

Type annotations #2025

Closed

oke-aditya added 5 commits August 2, 2021 11:50

remove mypy ignore

1e0de0c

add missing typings

4bc3a80

fix a few mypy errors

3c1bd67

fix mypy errors

479c64c

fix mypy

ee1b93e

oke-aditya commented Aug 2, 2021

View reviewed changes

oke-aditya added 2 commits August 5, 2021 10:22

Merge branch 'master' of https://github.com/pytorch/vision into add_t…

03b3ba4

…yping3

ignore types

73f28df

pmeier reviewed Aug 16, 2021

View reviewed changes

torchvision/models/quantization/googlenet.py Outdated Show resolved Hide resolved

torchvision/models/quantization/inception.py Outdated Show resolved Hide resolved

Merge branch 'master' of https://github.com/pytorch/vision into add_t…

d059d04

…yping3

fixup annotation

2528d13

oke-aditya added 2 commits August 18, 2021 00:01

Merge branch 'master' of https://github.com/pytorch/vision into add_t…

10d4d67

…yping3

fix remaining types

2bba207

oke-aditya requested a review from pmeier August 17, 2021 19:17

pmeier approved these changes Aug 18, 2021

View reviewed changes

cleanup #TODO comments

a6b8528

pmeier requested a review from datumbox August 18, 2021 05:24

Merge branch 'main' into add_typing3

cf5f6de

datumbox approved these changes Aug 31, 2021

View reviewed changes

Merge branch 'main' into add_typing3

57d8970

datumbox merged commit 0725ccc into pytorch:main Aug 31, 2021

oke-aditya deleted the add_typing3 branch August 31, 2021 10:00

NicolasHug added code quality module: models.quantization Issues related to the quantizable/quantized models labels Sep 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add typing annotations to models/quantization #4232

Add typing annotations to models/quantization #4232

oke-aditya commented Jul 30, 2021

oke-aditya left a comment •

edited

Loading

frgfm left a comment •

edited

Loading

oke-aditya left a comment •

edited

Loading

oke-aditya left a comment

oke-aditya Aug 2, 2021

oke-aditya commented Aug 5, 2021 •

edited

Loading

pmeier left a comment

oke-aditya commented Aug 16, 2021

pmeier commented Aug 17, 2021

oke-aditya commented Aug 17, 2021 •

edited

Loading

pmeier left a comment

oke-aditya commented Aug 20, 2021

pmeier commented Aug 20, 2021

oke-aditya commented Aug 20, 2021

oke-aditya commented Aug 28, 2021 •

edited

Loading

datumbox left a comment

	model.aux1 = None # type: ignore[assignment]
	model.aux2 = None # type: ignore[assignment]

Add typing annotations to models/quantization #4232

Add typing annotations to models/quantization #4232

Conversation

oke-aditya commented Jul 30, 2021

oke-aditya left a comment • edited Loading

Choose a reason for hiding this comment

frgfm left a comment • edited Loading

Choose a reason for hiding this comment

oke-aditya left a comment • edited Loading

Choose a reason for hiding this comment

oke-aditya left a comment

Choose a reason for hiding this comment

oke-aditya Aug 2, 2021

Choose a reason for hiding this comment

oke-aditya commented Aug 5, 2021 • edited Loading

pmeier left a comment

Choose a reason for hiding this comment

oke-aditya commented Aug 16, 2021

pmeier commented Aug 17, 2021

oke-aditya commented Aug 17, 2021 • edited Loading

pmeier left a comment

Choose a reason for hiding this comment

oke-aditya commented Aug 20, 2021

pmeier commented Aug 20, 2021

oke-aditya commented Aug 20, 2021

oke-aditya commented Aug 28, 2021 • edited Loading

datumbox left a comment

Choose a reason for hiding this comment

oke-aditya left a comment •

edited

Loading

frgfm left a comment •

edited

Loading

oke-aditya left a comment •

edited

Loading

oke-aditya commented Aug 5, 2021 •

edited

Loading

oke-aditya commented Aug 17, 2021 •

edited

Loading

oke-aditya commented Aug 28, 2021 •

edited

Loading