Added annotation typing to densenet #2860

frgfm · 2020-10-21T16:01:49Z

Hi there!

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

Any feedback is welcome!

pmeier · 2020-10-22T17:37:41Z

@frgfm Before I review could you enable mypy be removing

vision/mypy.ini

Lines 19 to 21 in 65591f1

    
           [mypy-torchvision.models.densenet.*] 
        
           ignore_errors = True

I remember I deactivated it in the first step because they were already errors present.

frgfm · 2020-10-22T19:14:02Z

@frgfm Before I review could you enable mypy be removing

vision/mypy.ini

Lines 19 to 21 in 65591f1

[mypy-torchvision.models.densenet.*]

ignore_errors = True

I remember I deactivated it in the first step because they were already errors present.

Sure, just did it!

fmassa

Thanks for the PR!

mypy is complaining because of Tuple[int], could you have a look.

Otherwise looks good to me!

torchvision/models/densenet.py

frgfm · 2020-10-23T10:01:19Z

I'm still getting some errors, I'm not sure how you prefer to handle:

torchvision/models/densenet.py:32: error: "add_module" of "Module" does not return a value 
[func-returns-value]
            self.add_module('norm1', nn.BatchNorm2d(num_input_features)),
            ^
torchvision/models/densenet.py:33: error: "add_module" of "Module" does not return a value 
[func-returns-value]
            self.add_module('relu1', nn.ReLU(inplace=True)),
            ^
torchvision/models/densenet.py:34: error: "add_module" of "Module" does not return a value 
[func-returns-value]
            self.add_module('conv1', nn.Conv2d(num_input_features, bn_size *
            ^
torchvision/models/densenet.py:37: error: "add_module" of "Module" does not return a value 
[func-returns-value]
            self.add_module('norm2', nn.BatchNorm2d(bn_size * growth_rate)),
            ^
torchvision/models/densenet.py:38: error: "add_module" of "Module" does not return a value 
[func-returns-value]
            self.add_module('relu2', nn.ReLU(inplace=True)),
            ^
torchvision/models/densenet.py:39: error: "add_module" of "Module" does not return a value 
[func-returns-value]
            self.add_module('conv2', nn.Conv2d(bn_size * growth_rate, growth_rate,
            ^
torchvision/models/densenet.py:47: error: "Tensor" not callable  [operator]
            bottleneck_output = self.conv1(self.relu1(self.norm1(concated_features)))  # noqa: ...
                                ^

We could directly set the attribute modules, but that would be extra changes. I'd need some minimal change suggestion on this one 😅

fmassa · 2020-10-23T10:30:35Z

Maybe @pmeier knows of a trick, but I would prefer to disable mypy in this block instead of rewriting the code.

pmeier · 2020-10-23T10:40:36Z

We will always face issues if we dynamically assign attributes, since mypy is a static type checker. The only thing we can do here is to add lines that annotate this statically

self.norm1: nn.BatchNorm2d
self.add_module('norm1', nn.BatchNorm2d(num_input_features))

frgfm · 2020-10-23T10:46:59Z

Alright, I'll change that then!
Quick question: any specific reasons why there are commas at the end of each self.add_module line?
Should I remove then?

pmeier · 2020-10-23T12:04:56Z

Quick question: any specific reasons why there are commas at the end of each self.add_module line?

Probably an artifact from a previous implementation.

Should I remove then?

Since we don't need to touch the lines for the typing, you could do so in a follow-up PR.

frgfm · 2020-10-25T20:24:02Z

We will always face issues if we dynamically assign attributes, since mypy is a static type checker. The only thing we can do here is to add lines that annotate this statically
self.norm1: nn.BatchNorm2d
self.add_module('norm1', nn.BatchNorm2d(num_input_features))

FYI @pmeier, I tried and unfortunately, on my hand, it's still throwing the same error :/

facebook-github-bot · 2020-11-02T19:11:47Z

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!

frgfm · 2020-11-04T16:37:19Z

Any suggestion about this error @pmeier ? The same thing goes for #2864

pmeier · 2020-11-04T19:32:00Z

@frgfm

This checks fine:

from torch import nn


class DenseLayer(nn.Module):
    def __init__(self, num_features: int):
        super().__init__()
        self.norm1: nn.BatchNorm2d
        self.add_module("norm1", nn.BatchNorm2d(num_features))

Could you compile a minimal example for the error you are seeing?

frgfm · 2020-11-04T20:36:48Z

OK I think I found why I'm still getting errors @pmeier : mypy seems to be taking the commas at the end of line quite badly. The error is gone if I remove them, but stays if I leave them. Should I remove the commas?

pmeier · 2020-11-04T20:48:13Z

Should I remove the commas?

Sure. I only suggested otherwise before, since I thought they are unrelated to this PR. If that is not the case, we can act on this.

pmeier

Two nitpicks. Otherwise LGTM!

pmeier · 2020-11-05T06:23:20Z

torchvision/models/densenet.py

@@ -114,7 +130,7 @@ def forward(self, init_features):


 class _Transition(nn.Sequential):
-    def __init__(self, num_input_features, num_output_features):
+    def __init__(self, num_input_features: int, num_output_features: int):


Suggested change

def __init__(self, num_input_features: int, num_output_features: int):

def __init__(self, num_input_features: int, num_output_features: int) -> None:

pmeier · 2020-11-05T06:24:22Z

torchvision/models/densenet.py

+def _densenet(arch: str, growth_rate: int, block_config: Tuple[int, int, int, int], num_init_features: int,
+              pretrained: bool, progress: bool, **kwargs: Any) -> DenseNet:


Could you format this with black?

pmeier · 2020-11-05T09:12:13Z

@frgfm As of lately, you are required to sign CLA as detailed in #2860 (comment). Without that the work that you have done cannot be committed into our repository.

frgfm · 2020-11-06T09:33:45Z

@pmeier done ✔️

fmassa

Thanks a lot!

fmassa · 2020-11-06T09:52:25Z

torchvision/models/densenet.py

+        self.norm1: nn.BatchNorm2d
+        self.add_module('norm1', nn.BatchNorm2d(num_input_features))
+        self.relu1: nn.ReLU
+        self.add_module('relu1', nn.ReLU(inplace=True))
+        self.conv1: nn.Conv2d


note for the future: we should just use

self.norm1 = nn.BatchNorm2(...)

instead of the add_module alternative, as the names are not dynamic here.

fmassa · 2020-11-06T09:54:59Z

Just waiting on the CLA to get propagated and will merge the PR

* style: Added annotation typing for densenet * fix: Fixed import * refactor: Removed un-necessary import * fix: Fixed constructor typing * chore: Updated mypy.ini * fix: Fixed tuple typing * style: Ignored some mypy errors * style: Fixed typing * fix: Added missing constructor typing

style: Added annotation typing for densenet

a6adcf9

pmeier self-assigned this Oct 21, 2020

frgfm added 2 commits October 21, 2020 23:09

fix: Fixed import

a4794cc

refactor: Removed un-necessary import

d7f3231

frgfm added 2 commits October 22, 2020 21:12

fix: Fixed constructor typing

a5dd590

chore: Updated mypy.ini

ea8bcbe

fmassa reviewed Oct 23, 2020

View reviewed changes

torchvision/models/densenet.py Outdated Show resolved Hide resolved

frgfm added 2 commits October 23, 2020 11:52

fix: Fixed tuple typing

98c5cd2

style: Ignored some mypy errors

6065ce0

style: Fixed typing

6c01e71

pmeier approved these changes Nov 5, 2020

View reviewed changes

fix: Added missing constructor typing

27ca7ee

pmeier requested a review from fmassa November 5, 2020 09:09

fmassa approved these changes Nov 6, 2020

View reviewed changes

facebook-github-bot added the cla signed label Nov 6, 2020

fmassa merged commit a8d8496 into pytorch:master Nov 6, 2020

lutzroeder mentioned this pull request Nov 19, 2020

#2860 regressed torch.jit.script(densenet161).save #3027

Closed

frgfm deleted the densenet-typing branch November 19, 2020 08:09

frgfm mentioned this pull request Nov 19, 2020

Fixed typing exception throwing issues with JIT #3029

Merged

frgfm mentioned this pull request Oct 26, 2022

Type annotations #2025

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added annotation typing to densenet #2860

Added annotation typing to densenet #2860

frgfm commented Oct 21, 2020

pmeier commented Oct 22, 2020

frgfm commented Oct 22, 2020

fmassa left a comment

frgfm commented Oct 23, 2020

fmassa commented Oct 23, 2020

pmeier commented Oct 23, 2020

frgfm commented Oct 23, 2020

pmeier commented Oct 23, 2020

frgfm commented Oct 25, 2020

facebook-github-bot commented Nov 2, 2020

frgfm commented Nov 4, 2020

pmeier commented Nov 4, 2020 •

edited

Loading

frgfm commented Nov 4, 2020

pmeier commented Nov 4, 2020

pmeier left a comment

pmeier Nov 5, 2020

pmeier Nov 5, 2020

pmeier commented Nov 5, 2020

frgfm commented Nov 6, 2020

fmassa left a comment

fmassa Nov 6, 2020

fmassa commented Nov 6, 2020

	def __init__(self, num_input_features: int, num_output_features: int):
	def __init__(self, num_input_features: int, num_output_features: int) -> None:

		def _densenet(arch: str, growth_rate: int, block_config: Tuple[int, int, int, int], num_init_features: int,
		pretrained: bool, progress: bool, **kwargs: Any) -> DenseNet:

Added annotation typing to densenet #2860

Added annotation typing to densenet #2860

Conversation

frgfm commented Oct 21, 2020

pmeier commented Oct 22, 2020

frgfm commented Oct 22, 2020

fmassa left a comment

Choose a reason for hiding this comment

frgfm commented Oct 23, 2020

fmassa commented Oct 23, 2020

pmeier commented Oct 23, 2020

frgfm commented Oct 23, 2020

pmeier commented Oct 23, 2020

frgfm commented Oct 25, 2020

facebook-github-bot commented Nov 2, 2020

frgfm commented Nov 4, 2020

pmeier commented Nov 4, 2020 • edited Loading

frgfm commented Nov 4, 2020

pmeier commented Nov 4, 2020

pmeier left a comment

Choose a reason for hiding this comment

pmeier Nov 5, 2020

Choose a reason for hiding this comment

pmeier Nov 5, 2020

Choose a reason for hiding this comment

pmeier commented Nov 5, 2020

frgfm commented Nov 6, 2020

fmassa left a comment

Choose a reason for hiding this comment

fmassa Nov 6, 2020

Choose a reason for hiding this comment

fmassa commented Nov 6, 2020

pmeier commented Nov 4, 2020 •

edited

Loading