Add typing Annotations to detection/utils #4583

oke-aditya · 2021-10-10T14:31:16Z

Let's begin #4582

@khushi-411 do you want to take this up? I think you can push to this branch on my fork.
You can clone my forked repo and push changes. They should reflect here.

cc @datumbox

oke-aditya

The PR isn't complete. A small headstart for @khushi-411 😃

mypy.ini

torchvision/models/detection/_utils.py

khushi-411 · 2021-10-10T15:04:59Z

Definitely @oke-aditya, I'd love to start up with this PR! The explanations you provided are really helpful.
I'll ping you once it is complete or if I have some doubts.

oke-aditya · 2021-10-11T10:06:49Z

It worked. Your commit appeared here.

khushi-411 · 2021-10-11T10:11:18Z

Yep, Cheers!

oke-aditya · 2021-10-11T15:44:17Z

@khushi-411 you would need to use pre-commit hooks to format your code.

See the contributing guide

https://github.com/pytorch/vision/blob/main/CONTRIBUTING.md#pre-commit-hooks

It will automatically format your code to pass python lint CI.

I will have a look at PR before end of day, don't worry it's very natural to get a Red from CI, little bit of debugging can fix it.

torchvision/models/detection/_utils.py

oke-aditya · 2021-10-11T18:55:44Z

torchvision/models/detection/_utils.py

@@ -290,7 +285,7 @@ def __call__(self, match_quality_matrix):
        if self.allow_low_quality_matches:
            all_matches = matches.clone()
        else:
-            all_matches = None
+            all_matches = None  # type: ignore[assignment]


I think we should ignore this error, as we are directly assigning None to a Tensor object.

Ah, I was stuck at this point. Is there any other method of adding types here?
I read one issue (#40867). That's quite relatable.

I'm not sure if we have a better way as we are assigning all_matches on the fly to be either Tensor or None.
Where could we declare all_matches to be Optional[Tensor] ?

Previously too we had to use such workaround see https://github.com/pytorch/vision/blob/main/torchvision/models/quantization/googlenet.py#L80

cc @pmeier

If Optional[Tensor] is possible, than we should definitively go for that. Otherwise probably Tensor and ignore the error here.

Let me have a quick try at that. I will get back in an hour.

Didn't work well, see the below JIT error.

oke-aditya · 2021-10-11T19:19:28Z

Apart from above comment everything looks good.

cc @pmeier @NicolasHug

datumbox · 2021-10-11T19:28:10Z

Had a look and the first impression is that all types are correct. I can have a look again if you want tomorrow.

khushi-411 · 2021-10-11T19:30:03Z

Yes definitely, thanks a lot @datumbox!

pmeier

LGTM after #4583 (comment) is resolved and CI is green. Thanks a lot @oke-aditya and @khushi-411!

khushi-411 · 2021-10-11T19:34:45Z

Thanks a lot @pmeier!

oke-aditya · 2021-10-11T19:35:48Z

Mypy was happy locally Hopefully CI will be all green.

Edit: I'm amazed that people in 3 different time zones synced so fast. Hats off to @pmeier @datumbox @khushi-411 !

oke-aditya · 2021-10-11T19:39:24Z

Ok so mpy was happy and JIT wasn't.

https://app.circleci.com/pipelines/github/pytorch/vision/11222/workflows/bf1a9029-353d-4d55-ad61-2c8f98a2b943/jobs/866525

torch.jit.frontend.UnsupportedNodeError: annotated assignments without assigned value aren't supported:
  File "/root/project/torchvision/models/detection/_utils.py", line 285
        # Max over gt elements (dim 0) to find best gt candidate for each prediction
        matched_vals, matches = match_quality_matrix.max(dim=0)
        all_matches: Optional[Tensor]
        ~ <--- HERE
        if self.allow_low_quality_matches:
            all_matches = matches.clone()

So we have no choice but to ignore ☹️

oke-aditya · 2021-10-11T19:43:00Z

This should be good to go now @datumbox @pmeier 😃

pmeier · 2021-10-11T19:59:48Z

I'm amazed that people in 3 different time zones synced so fast.

Well for Vasilis and me the time is at least somewhat reasonable whereas for you folks is the middle of the night 😛

datumbox

LGTM, thanks!

Summary: * Start annotating utils * checking * Add annotations at _utils.py * Remove unnecessary comments. * re-checked typings * Update typing * Ignore small error * Use optional tensor * Ignore for JIT Reviewed By: fmassa Differential Revision: D31649971 fbshipit-source-id: 687cd2f5511b0d8cad015b74b60f3af0c108c2b0 Co-authored-by: Khushi Agrawal <[email protected]> Co-authored-by: Vasilis Vryniotis <[email protected]>

* Start annotating utils * checking * Add annotations at _utils.py * Remove unnecessary comments. * re-checked typings * Update typing * Ignore small error * Use optional tensor * Ignore for JIT Co-authored-by: Khushi Agrawal <[email protected]> Co-authored-by: Vasilis Vryniotis <[email protected]>

Start annotating utils

50ab495

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

oke-aditya mentioned this pull request Oct 10, 2021

Add typing annotations to models/detection #4582

Open

14 tasks

oke-aditya commented Oct 10, 2021

View reviewed changes

mypy.ini Show resolved Hide resolved

torchvision/models/detection/_utils.py Show resolved Hide resolved

torchvision/models/detection/_utils.py Show resolved Hide resolved

checking

dab9402

khushi-411 added 3 commits October 11, 2021 16:10

Add annotations at _utils.py

5f7d0d9

Remove unnecessary comments.

d017161

Merge branch 'main' into type_utils

8e83805

khushi-411 and others added 2 commits October 11, 2021 23:42

re-checked typings

f6bb442

Update typing

c4c2884

oke-aditya commented Oct 11, 2021

View reviewed changes

torchvision/models/detection/_utils.py Show resolved Hide resolved

oke-aditya commented Oct 11, 2021

View reviewed changes

torchvision/models/detection/_utils.py Show resolved Hide resolved

oke-aditya commented Oct 11, 2021

View reviewed changes

torchvision/models/detection/_utils.py Show resolved Hide resolved

Ignore small error

f157190

oke-aditya commented Oct 11, 2021

View reviewed changes

oke-aditya marked this pull request as ready for review October 11, 2021 19:17

pmeier approved these changes Oct 11, 2021

View reviewed changes

oke-aditya and others added 2 commits October 12, 2021 01:04

Use optional tensor

f1df465

Merge branch 'main' into type_utils

c8fa413

Ignore for JIT

f78dd49

datumbox approved these changes Oct 12, 2021

View reviewed changes

Merge branch 'main' into type_utils

4df6ccf

datumbox added code quality module: models topic: object detection labels Oct 12, 2021

datumbox merged commit c790216 into pytorch:main Oct 12, 2021

oke-aditya deleted the type_utils branch October 12, 2021 10:39

frgfm mentioned this pull request Oct 27, 2022

Type annotations #2025

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add typing Annotations to detection/utils #4583

Add typing Annotations to detection/utils #4583

oke-aditya commented Oct 10, 2021 •

edited by pytorch-probot bot

Loading

oke-aditya left a comment

khushi-411 commented Oct 10, 2021

oke-aditya commented Oct 11, 2021

khushi-411 commented Oct 11, 2021

oke-aditya commented Oct 11, 2021

oke-aditya Oct 11, 2021

khushi-411 Oct 11, 2021 •

edited

Loading

oke-aditya Oct 11, 2021

pmeier Oct 11, 2021

oke-aditya Oct 11, 2021

oke-aditya Oct 11, 2021

oke-aditya commented Oct 11, 2021

datumbox commented Oct 11, 2021

khushi-411 commented Oct 11, 2021

pmeier left a comment

khushi-411 commented Oct 11, 2021

oke-aditya commented Oct 11, 2021 •

edited

Loading

oke-aditya commented Oct 11, 2021 •

edited

Loading

oke-aditya commented Oct 11, 2021

pmeier commented Oct 11, 2021

datumbox left a comment

Add typing Annotations to detection/utils #4583

Add typing Annotations to detection/utils #4583

Conversation

oke-aditya commented Oct 10, 2021 • edited by pytorch-probot bot Loading

oke-aditya left a comment

Choose a reason for hiding this comment

khushi-411 commented Oct 10, 2021

oke-aditya commented Oct 11, 2021

khushi-411 commented Oct 11, 2021

oke-aditya commented Oct 11, 2021

oke-aditya Oct 11, 2021

Choose a reason for hiding this comment

khushi-411 Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

oke-aditya Oct 11, 2021

Choose a reason for hiding this comment

pmeier Oct 11, 2021

Choose a reason for hiding this comment

oke-aditya Oct 11, 2021

Choose a reason for hiding this comment

oke-aditya Oct 11, 2021

Choose a reason for hiding this comment

oke-aditya commented Oct 11, 2021

datumbox commented Oct 11, 2021

khushi-411 commented Oct 11, 2021

pmeier left a comment

Choose a reason for hiding this comment

khushi-411 commented Oct 11, 2021

oke-aditya commented Oct 11, 2021 • edited Loading

oke-aditya commented Oct 11, 2021 • edited Loading

oke-aditya commented Oct 11, 2021

pmeier commented Oct 11, 2021

datumbox left a comment

Choose a reason for hiding this comment

oke-aditya commented Oct 10, 2021 •

edited by pytorch-probot bot

Loading

khushi-411 Oct 11, 2021 •

edited

Loading

oke-aditya commented Oct 11, 2021 •

edited

Loading

oke-aditya commented Oct 11, 2021 •

edited

Loading