Skip to content

Type annotations for torchvision.ops #2331

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jul 6, 2020
Merged

Type annotations for torchvision.ops #2331

merged 6 commits into from
Jul 6, 2020

Conversation

iamvukasin
Copy link
Contributor

Addresses #2025

@fmassa
Copy link
Member

fmassa commented Jun 19, 2020

Thanks for the PR!

Unfortunately, torchscript currently doesn't support Union, so for the type annotations we need to use only one of the overload that are supported (and in many cases we only support one for torchscript)

Also, if a function argument is not annotated, torchscript assumes that the argument is a Tensor, so for the functions that are missing type annotations you can safely annotate them with Tensor.

Can you look into fixing this?

fmassa
fmassa reviewed Jun 19, 2020
View reviewed changes
torchvision/ops/boxes.py Outdated
@@ -5,7 +5,7 @@


@torch.jit.script
def nms(boxes, scores, iou_threshold):
def nms(boxes: Tensor, scores: Tensor, iou_threshold: float) -> Tensor:
# type: (Tensor, Tensor, float) -> Tensor
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the comment-style type annotations if you add the type annotations inline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I have it in the local repo, will push it soon.

@iamvukasin
Copy link
Contributor Author

Thanks for the fast review! I have locally updated the code, but still have minor issues before pushing it.

Unfortunately, torchscript currently doesn't support Union, so for the type annotations we need to use only one of the overload that are supported (and in many cases we only support one for torchscript)

What to do with check_roi_boxes_shape(boxes) in _utils.py as boxes can be List[Tensor], Tuple[Tensor] or Tensor? Which one is preferred?

Also, if a function argument is not annotated, torchscript assumes that the argument is a Tensor, so for the functions that are missing type annotations you can safely annotate them with Tensor.

Is this for functions nested in _register_custom_op() in _register_onnx_ops.py, since they are only without annotations?

@fmassa
Copy link
Member

fmassa commented Jun 22, 2020

Is this for functions nested in _register_custom_op() in _register_onnx_ops.py, since they are only without annotations?

No, this is for all functions in torchvision which support torchscript.

What to do with check_roi_boxes_shape(boxes) in _utils.py as boxes can be List[Tensor], Tuple[Tensor] or Tensor? Which one is preferred?

As I mentioned in my previous comment, if there is no type annotation for an argument, torchscript assumes the argument is a tensor, so for check_roi_boxes_shape, we should use Tensor

@iamvukasin iamvukasin requested a review from fmassa June 30, 2020 11:00
@fmassa
Copy link
Member

fmassa commented Jun 30, 2020

Hi,

Test failures look weird, and might be due to the fact that CircleCI is running on your account. Let me have a look to understand what is going on.

cc @seemethere for thoughts

@seemethere
Copy link
Member

@iamvukasin I think the reason this is happening is because you have enabled builds for your fork on CircleCI.

Could you go and disable builds on your fork

  1. Click this link -> https://app.circleci.com/settings/project/github/iamvukasin/vision
  2. Click this button

Screen Shot 2020-06-30 at 9 25 25 AM

@iamvukasin
Copy link
Contributor Author

@seemethere Definitely the problem was that the builds had been running with my CirceCI plan. I have stopped any further builds by following your guide.

@fmassa
Copy link
Member

fmassa commented Jul 1, 2020

@iamvukasin Can you try rebasing this PR on top of master to try running new CI jobs?

@codecov
Copy link

codecov bot commented Jul 4, 2020 •
edited
Loading

Codecov Report

Merging #2331 into master will increase coverage by 0.00%.
The diff coverage is 98.33%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2331   +/-   ##
=======================================
  Coverage   70.69%   70.70%           
=======================================
  Files          94       94           
  Lines        7839     7841    +2     
  Branches     1221     1221           
=======================================
+ Hits         5542     5544    +2     
  Misses       1925     1925           
  Partials      372      372
Impacted Files Coverage Δ
torchvision/ops/feature_pyramid_network.py 75.82% <90.90%> (ø)
torchvision/ops/_utils.py 82.60% <100.00%> (ø)
torchvision/ops/boxes.py 84.44% <100.00%> (ø)
torchvision/ops/deform_conv.py 66.66% <100.00%> (ø)
torchvision/ops/misc.py 86.04% <100.00%> (+0.68%) ⬆️
torchvision/ops/new_empty_tensor.py 80.00% <100.00%> (ø)
torchvision/ops/poolers.py 97.02% <100.00%> (ø)
torchvision/ops/ps_roi_align.py 71.42% <100.00%> (ø)
torchvision/ops/ps_roi_pool.py 73.07% <100.00%> (ø)
torchvision/ops/roi_align.py 68.96% <100.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75f5b57...c408ac6. Read the comment docs.

fmassa
fmassa approved these changes Jul 6, 2020
View reviewed changes
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 a lot!

I have one minor comment that I think could be fixed in a follow-up PR, but let's merge this as is as it's already a nice improvement.

torchvision/ops/misc.py
def __init__(self, num_features, eps=0., n=None):
def __init__(
self,
num_features: Tuple[int, ...],
Copy link
Member

Choose a reason for hiding this comment

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

nit: I would have expected num_features to be an int, not a tuple with a varying number of elements.

@fmassa fmassa merged commit 971c3e4 into pytorch:master Jul 6, 2020
@vfdev-5 vfdev-5 mentioned this pull request Jul 6, 2020
Closed
16 tasks
@iamvukasin iamvukasin mentioned this pull request Jul 11, 2020
Merged
de-vri-es pushed a commit to fizyr-forks/torchvision that referenced this pull request Aug 4, 2020
@iamvukasin @de-vri-es
Type annotations for torchvision.ops (pytorch#2331)
483cf89 
* Add type annotations for torchvision.ops

* Fix type annotations for torchvision.ops

* Fix typo in import

* Fix undefined name in FeaturePyramidNetwork
@frgfm frgfm mentioned this pull request Mar 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmassa fmassa fmassa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@iamvukasin @fmassa @seemethere