Added typing annotations to models/detection [1/n] #4220

oke-aditya · 2021-07-29T12:48:40Z

Linked with #2025

I'm unsure of few types. I will tag them.

EDIT:
~~I will go 3 files at a time, otherwise the diff would be too big to review.~~

I will finish major files in this PR and follow-up another for utils.

Also note that I have re-written the code a bit so that it looks similar to black / vertical code formatting which would reduce the diffs when any such tool is used.

cc @pmeier

oke-aditya

Hey @pmeier @frgfm 😄 would be great if you have a look

torchvision/models/detection/faster_rcnn.py

torchvision/models/detection/retinanet.py

…yping1

datumbox

Thanks @oke-aditya.

I had a first pass and added a few comments. Let me know what you think. Please note that my current setup does not allow me to do thorough reviews, so I will leave it for someone else from the team to approve (or wait until I'm properly back. :) )

datumbox · 2021-07-29T14:30:58Z

torchvision/models/detection/faster_rcnn.py

+        min_size: int = 800,
+        max_size: int = 1333,
+        image_mean: Optional[Tuple[float]] = None,
+        image_std: Optional[Tuple[float]] = None,


I think it should be List[float] instead of Tuple[float]?

Similar to:

vision/torchvision/models/detection/ssd.py

Line 169 in 3e7653c

image_mean: Optional[List[float]] = None, image_std: Optional[List[float]] = None,

I would argue that the typing in ssd is too specific: it can be a Optional[List[float]] indeed, but Optional[Tuple[float, float, float]] is accepted since it will be passed down to https://github.com/pytorch/vision/blob/master/torchvision/models/detection/transform.py#L136-L137 for the transformations

Should we use a Union or enforce a List?

actually, I just saw your comment later on, since the backbone can take any number of channels in the input tensor, the second option is actually Optional[Tuple[float, ...]]

I have changed to Optional[Tuple[float, ...]] Since in the docstring we mention Tuple[float, float, float].

torchvision/models/detection/faster_rcnn.py

torchvision/models/detection/mask_rcnn.py

torchvision/models/detection/retinanet.py

…yping1

frgfm

Thanks for the help!
You forgot typing annotations on the forward methods of most classes of faster_rcnn.py module, I added comments about things that weren't already pointed out by @datumbox!

frgfm · 2021-07-29T15:58:28Z

torchvision/models/detection/faster_rcnn.py

+        min_size: int = 800,
+        max_size: int = 1333,
+        image_mean: Optional[Tuple[float]] = None,
+        image_std: Optional[Tuple[float]] = None,


I would argue that the typing in ssd is too specific: it can be a Optional[List[float]] indeed, but Optional[Tuple[float, float, float]] is accepted since it will be passed down to https://github.com/pytorch/vision/blob/master/torchvision/models/detection/transform.py#L136-L137 for the transformations

Should we use a Union or enforce a List?

frgfm · 2021-07-29T15:59:49Z

torchvision/models/detection/faster_rcnn.py

+        min_size: int = 800,
+        max_size: int = 1333,
+        image_mean: Optional[Tuple[float]] = None,
+        image_std: Optional[Tuple[float]] = None,


actually, I just saw your comment later on, since the backbone can take any number of channels in the input tensor, the second option is actually Optional[Tuple[float, ...]]

frgfm · 2021-07-29T16:14:15Z

torchvision/models/detection/faster_rcnn.py

+        rpn_score_thresh: float = 0.0,
+        # Box parameters
+        box_roi_pool: Optional[MultiScaleRoIAlign] = None,
+        box_head: Optional[nn.Module] = None,


box_head: Optional[RPNHead] = None I believe

I was bit unsure on these. I think that user can write their own classes and just pass them to FasterRCNN.
Hence I thought it shouldn't be so particular class like RPNHead.

Let me know, I will change all of these otherwise.

If we allow passing custom classes, we need structural duck typing here. Meaning we need to define a custom typing.Protocol that defines how these objects need to look like. @oke-aditya can you make a list of all the attributes we access and methods we call on these objects?

I'm unsure of what you mean @pmeier . I'm new to type hints.
I think trying to be very strict in typing by custom classes might be too much. If nn.Module satisfies the purpose I think it should be good enough?

If nn.Module satisfies the purpose I think it should be good enough?

Yes. As a rule of thumb, the input types should be as loose as possible and the output types as strict as possible.

After some digging, I think

vision/torchvision/models/detection/roi_heads.py

Line 753 in fe3118c

box_features = self.box_head(box_features)

is the only time we do anything with the object passed as box_head. Since it is possible to call an nn.Module, using it as annotation is fine.

In other cases @frgfm's comment might have some merit in other cases: for example, if we at some point access box_head.foo, nn.Module is not sufficient as annotation, since it doesn't define a foo attribute. In such a case we have two options:

Annotate with a custom class such as RPNHead that has this attribute.

Create a typing.Protocol that defines the interface we are looking for. In the example above the object passed to box_head needs to be callable and define an attribute foo.

Especially if users are allowed to pass custom objects, I would always prefer 2. over 1. because it doesn't require any changes on the user side. I don't know if this works with torchscript though.

Let's discuss this if the need for something like this arises.

torchvision/models/detection/mask_rcnn.py

torchvision/models/detection/faster_rcnn.py

torchvision/models/detection/mask_rcnn.py

torchvision/models/detection/retinanet.py

oke-aditya · 2021-07-29T16:25:20Z

I have combined the PRs. I will address all the above comments 😃

oke-aditya

Let me know the thoughts.

I can't type some internal forward functions. 😕

cc @frgfm

oke-aditya · 2021-07-29T16:43:06Z

torchvision/models/detection/faster_rcnn.py

+        min_size: int = 800,
+        max_size: int = 1333,
+        image_mean: Optional[Tuple[float]] = None,
+        image_std: Optional[Tuple[float]] = None,


I have changed to Optional[Tuple[float, ...]] Since in the docstring we mention Tuple[float, float, float].

oke-aditya · 2021-07-29T16:45:06Z

torchvision/models/detection/faster_rcnn.py

+        rpn_score_thresh: float = 0.0,
+        # Box parameters
+        box_roi_pool: Optional[MultiScaleRoIAlign] = None,
+        box_head: Optional[nn.Module] = None,


I was bit unsure on these. I think that user can write their own classes and just pass them to FasterRCNN.
Hence I thought it shouldn't be so particular class like RPNHead.

Let me know, I will change all of these otherwise.

torchvision/models/detection/retinanet.py

oke-aditya

A small changelog, we aren't done yet but would say halfway there.
Mypy now complains slightly less.

torchvision/models/detection/generalized_rcnn.py

torchvision/models/detection/keypoint_rcnn.py

torchvision/models/detection/mask_rcnn.py

torchvision/models/detection/retinanet.py

torchvision/models/detection/rpn.py

torchvision/models/detection/ssd.py

torchvision/models/detection/transform.py

pmeier

Hey @oke-aditya, I did another pass. I have some comments inline. This gets harder and harder to review due to the sheer amount of changes. I suggest we split this into multiple PRs for each file for the "final" review, if we are mostly happy here.

torchvision/models/detection/generalized_rcnn.py

torchvision/models/detection/ssd.py

mypy.ini

torchvision/models/detection/keypoint_rcnn.py

pmeier · 2021-09-23T05:47:38Z

torchvision/models/detection/roi_heads.py

@@ -461,11 +461,10 @@ def _onnx_paste_masks_in_image_loop(masks, boxes, im_h, im_w):
    return res_append


-def paste_masks_in_image(masks, boxes, img_shape, padding=1):
-    # type: (Tensor, Tensor, Tuple[int, int], int) -> Tensor
+def paste_masks_in_image(masks: Tensor, boxes: Tensor, img_shape: List[int], padding: int = 1) -> Tensor:


Since this file is currently ignored by mypy, let's not introduce annotations here.

torchvision/models/detection/rpn.py

pmeier · 2021-09-23T05:52:41Z

torchvision/models/detection/transform.py

@@ -222,8 +237,8 @@ def batch_images(self, images: List[Tensor], size_divisible: int = 32) -> Tensor

    def postprocess(self,
                    result: List[Dict[str, Tensor]],
-                    image_shapes: List[Tuple[int, int]],


Thought the convention seems to be using List[List[int]] everywhere

If that is the convention, let's also roll with it here. Although I would prefer Tuple[int, int], because an image size is always of length 2 and not arbitrary.

oke-aditya

HI! Feel free to add your thoughts. I get that this PR modfiies lot of files which makes reviewing a harder as we progress.
Once we finalize, I will send PR for each file individually!

torchvision/models/detection/keypoint_rcnn.py

oke-aditya · 2021-09-23T15:18:43Z

torchvision/models/detection/roi_heads.py

@@ -461,11 +461,10 @@ def _onnx_paste_masks_in_image_loop(masks, boxes, im_h, im_w):
    return res_append


-def paste_masks_in_image(masks, boxes, img_shape, padding=1):
-    # type: (Tensor, Tensor, Tuple[int, int], int) -> Tensor
+def paste_masks_in_image(masks: Tensor, boxes: Tensor, img_shape: List[int], padding: int = 1) -> Tensor:


Well I have to annotate it due to the img_shape which should be Tuple[int, int], Otherwise mypy will compain in other places.

torchvision/models/detection/rpn.py

torchvision/models/detection/ssd.py

oke-aditya · 2021-09-23T15:29:08Z

torchvision/models/detection/transform.py

@@ -222,8 +237,8 @@ def batch_images(self, images: List[Tensor], size_divisible: int = 32) -> Tensor

    def postprocess(self,
                    result: List[Dict[str, Tensor]],
-                    image_shapes: List[Tuple[int, int]],


Ok so I will refactor to Tuple[int, int]. This might be bc breaking as it changes the old annotation of List[int] and users would get error if they were not passing List of length 2.

pmeier

I get that this PR modfiies lot of files which makes reviewing a harder as we progress. Once we finalize, I will send PR for each file individually!

I would prefer to split sooner than later. It is really hard to judge what comments need addressing since there are 10+ open conversations.

oke-aditya · 2021-09-24T05:23:55Z

I can split it, bit since the files are highly coupled, it makes hard to annotate in multiple PRs.

pmeier · 2021-09-24T05:27:51Z

I can split it, bit since the files are highly coupled, it makes hard to annotate in multiple PRs.

Start with the one that has no dependencies and work up the chain:

image_list.py
_utils.py
anchor_utils.py
roi_heads.py
transform.py
rpn.py
...

…ing1

oke-aditya

A small review, I will split this PR soon. I have substantially reduced mypy errors.

I think we can start with _utils and utility files, these would help making this PR easier.

torchvision/models/detection/transform.py

oke-aditya · 2021-09-25T15:53:29Z

torchvision/models/detection/transform.py

+            size_divisible: int = 32,
+            fixed_size: Optional[Tuple[int, int]] = None
+    ) -> None:
+
        super(GeneralizedRCNNTransform, self).__init__()
        if not isinstance(min_size, (list, tuple)):


So basically it should Union[int, List[int]]. Since if we are passing int values in some cases, we will be converting it to tuple / list.

oke-aditya · 2021-09-25T18:25:33Z

One of the problems in typing utils and leading errors to mypy are because of #4389, as per discussion there, we can't use nn.Module to type the detection utils, so it some errors for mypy remain.

oke-aditya · 2021-10-05T18:27:54Z

Using μfmt and setting mypy setting to allow redefinition has made this PR lot easier !!!!

Current error count has decreased to.

Found 32 errors in 8 files (checked 128 source files)

If I recall well the error count was close to 100 in the beginning. We are getting there.

I'm sorry that It's bit ugly work from my side by not splitting the PR and tackling easier files first. I have been very slow on working this PR :( .

pmeier · 2021-10-06T08:35:42Z

I'm sorry that It's bit ugly work from my side by not splitting the PR and tackling easier files first. I have been very slow on working this PR :( .

No worries, this PR is not blocking anything. We are grateful that you are picking up the work! Just note, that I'm not going to review this PR any further before the split. It just takes to much time to read all the discussions over and over again to get up to speed.

…ing1

oke-aditya · 2021-10-10T12:26:41Z

Hi all. I'm closing this PR and today I will split this up into multiple ones. Each file will unblock only the particular file which mypy needs to address. That is mypy will ignore all the other files that aren't going to be modified by each independent PR.

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

start adding typing

53fe949

finish typing frcnn

ecc58a7

Type up models

6a30c92

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

oke-aditya commented Jul 29, 2021

View reviewed changes

oke-aditya changed the title ~~Add typings to FasterRCNN, MaskRCNN and RetinaNet~~ Added annotations to models/detection FasterRCNN, MaskRCNN and RetinaNet Jul 29, 2021

oke-aditya changed the title ~~Added annotations to models/detection FasterRCNN, MaskRCNN and RetinaNet~~ Added annotations to models/detection FasterRCNN, MaskRCNN and RetinaNet Jul 29, 2021

oke-aditya added 3 commits July 29, 2021 18:35

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

fb3ea88

…yping1

fix bugs

1f5f715

fix bugs

d13311d

oke-aditya changed the title ~~Added annotations to models/detection FasterRCNN, MaskRCNN and RetinaNet~~ Added typing annotations to models/detection FasterRCNN, MaskRCNN and RetinaNet Jul 29, 2021

datumbox reviewed Jul 29, 2021

View reviewed changes

datumbox mentioned this pull request Jul 29, 2021

Added typing annotations to models/detection generalized_rcnn, keypoint_rcnn, rpn #4223

Closed

frgfm mentioned this pull request Jul 29, 2021

Type annotations #2025

Closed

oke-aditya added 2 commits July 29, 2021 21:47

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

5adda72

…yping1

combine PRs

d16be19

oke-aditya changed the title ~~Added typing annotations to models/detection FasterRCNN, MaskRCNN and RetinaNet~~ Added typing annotations to models/detection FasterRCNN, MaskRCNN and RetinaNet, generalized_rcnn, keypoint_rcnn, rpn Jul 29, 2021

frgfm suggested changes Jul 29, 2021

View reviewed changes

fix typing

4580541

oke-aditya commented Jul 29, 2021

View reviewed changes

oke-aditya changed the title ~~Added typing annotations to models/detection FasterRCNN, MaskRCNN and RetinaNet, generalized_rcnn, keypoint_rcnn, rpn~~ Added typing annotations to models/detection Jul 29, 2021

oke-aditya added 2 commits September 22, 2021 22:45

Use Union for TS

8976768

Fix

6fc8812

oke-aditya commented Sep 22, 2021

View reviewed changes

oke-aditya added 2 commits September 22, 2021 23:31

Fix small type

7422d3b

Fix type

a10588c

pmeier reviewed Sep 23, 2021

View reviewed changes

oke-aditya commented Sep 23, 2021

View reviewed changes

oke-aditya requested a review from pmeier September 23, 2021 16:43

pmeier reviewed Sep 24, 2021

View reviewed changes

oke-aditya added 3 commits September 25, 2021 18:01

Merge branch 'main' of https://github.com/pytorch/vision into add_typ…

5f2dcc4

…ing1

Handle tuple[tuple[int, int]] case, List[float]

70e5b6a

Use nn.ModuleList and nn.ModuleDict

7d4e919

oke-aditya commented Sep 25, 2021

View reviewed changes

oke-aditya mentioned this pull request Oct 4, 2021

Set allow_redefinition = True for mypy #4531

Merged

Resolve conflicts

01fdb48

pytorch-probot bot added the ciflow/default label Oct 5, 2021

oke-aditya requested a review from pmeier October 5, 2021 18:28

oke-aditya added 4 commits October 6, 2021 16:47

Merge branch 'main' of https://github.com/pytorch/vision into add_typ…

cace5ff

…ing1

Fix JIT issue, final update for CI Check

e941e48

Merge branch 'main' of https://github.com/pytorch/vision into add_typ…

623017b

…ing1

Fix JIT issue

becbbd6

This was referenced Oct 10, 2021

Add typing to detection model (rpn). #4581

Closed

Add typing annotations to models/detection #4582

Open

oke-aditya closed this Oct 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added typing annotations to models/detection [1/n] #4220

Added typing annotations to models/detection [1/n] #4220

oke-aditya commented Jul 29, 2021 •

edited

Loading

oke-aditya left a comment

datumbox left a comment

datumbox Jul 29, 2021

frgfm Jul 29, 2021

frgfm Jul 29, 2021

oke-aditya Jul 29, 2021

frgfm left a comment

frgfm Jul 29, 2021

frgfm Jul 29, 2021

frgfm Jul 29, 2021

oke-aditya Jul 29, 2021

pmeier Jul 30, 2021

oke-aditya Jul 30, 2021

pmeier Sep 14, 2021

oke-aditya commented Jul 29, 2021

oke-aditya left a comment •

edited

Loading

oke-aditya Jul 29, 2021

oke-aditya Jul 29, 2021

oke-aditya left a comment

pmeier left a comment

pmeier Sep 23, 2021

pmeier Sep 23, 2021

oke-aditya left a comment

oke-aditya Sep 23, 2021

oke-aditya Sep 23, 2021

pmeier left a comment •

edited

Loading

oke-aditya commented Sep 24, 2021

pmeier commented Sep 24, 2021

oke-aditya left a comment

oke-aditya Sep 25, 2021

oke-aditya commented Sep 25, 2021

oke-aditya commented Oct 5, 2021

pmeier commented Oct 6, 2021

oke-aditya commented Oct 10, 2021

Added typing annotations to models/detection [1/n] #4220

Added typing annotations to models/detection [1/n] #4220

Conversation

oke-aditya commented Jul 29, 2021 • edited Loading

oke-aditya left a comment

Choose a reason for hiding this comment

datumbox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frgfm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oke-aditya commented Jul 29, 2021

oke-aditya left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oke-aditya left a comment

Choose a reason for hiding this comment

pmeier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oke-aditya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmeier left a comment • edited Loading

Choose a reason for hiding this comment

oke-aditya commented Sep 24, 2021

pmeier commented Sep 24, 2021

oke-aditya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oke-aditya commented Sep 25, 2021

oke-aditya commented Oct 5, 2021

pmeier commented Oct 6, 2021

oke-aditya commented Oct 10, 2021

oke-aditya commented Jul 29, 2021 •

edited

Loading

oke-aditya left a comment •

edited

Loading

pmeier left a comment •

edited

Loading