Skip to content

Add typing annotations to detection/generalized_rcnn #4631

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

Merged
merged 7 commits into from
Nov 2, 2021
Merged

Add typing annotations to detection/generalized_rcnn #4631

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6d737e2
Update typing
oke-aditya Oct 15, 2021
3ba9494
Fix bug
oke-aditya Oct 15, 2021
193834f
Merge branch 'main' of https://github.com/pytorch/vision into type_ge…
oke-aditya Oct 18, 2021
886af22
Unblock mypy
oke-aditya Oct 18, 2021
afe34a8
Merge branch 'main' of https://github.com/pytorch/vision into type_ge…
oke-aditya Oct 28, 2021
10cc414
Ignore small error
oke-aditya Oct 28, 2021
5bc863a
Merge branch 'main' into type_generalized
datumbox Nov 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions mypy.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,6 @@ ignore_errors = True

ignore_errors = True

[mypy-torchvision.models.detection.generalized_rcnn]

ignore_errors = True

[mypy-torchvision.models.detection.faster_rcnn]

ignore_errors = True
Expand Down
21 changes: 14 additions & 7 deletions torchvision/models/detection/generalized_rcnn.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class GeneralizedRCNN(nn.Module):
the model
"""

def __init__(self, backbone, rpn, roi_heads, transform):
def __init__(self, backbone: nn.Module, rpn: nn.Module, roi_heads: nn.Module, transform: nn.Module) -> None:
super().__init__()
_log_api_usage_once(self)
self.transform = transform
Expand All @@ -36,19 +36,26 @@ def __init__(self, backbone, rpn, roi_heads, transform):
self._has_warned = False

@torch.jit.unused
def eager_outputs(self, losses, detections):
# type: (Dict[str, Tensor], List[Dict[str, Tensor]]) -> Union[Dict[str, Tensor], List[Dict[str, Tensor]]]
def eager_outputs(
self,
losses: Dict[str, Tensor],
detections: List[Dict[str, Tensor]],
) -> Union[Dict[str, Tensor], List[Dict[str, Tensor]]]:

if self.training:
return losses

return detections

def forward(self, images, targets=None):
# type: (List[Tensor], Optional[List[Dict[str, Tensor]]]) -> Tuple[Dict[str, Tensor], List[Dict[str, Tensor]]]
def forward(
self,
images: List[Tensor],
targets: Optional[List[Dict[str, Tensor]]] = None,
) -> Union[Tuple[Dict[str, Tensor], List[Dict[str, Tensor]]], Dict[str, Tensor], List[Dict[str, Tensor]]]:
Copy link
Contributor

Choose a reason for hiding this comment

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

@datumbox This change causes failure in internal tests, check D32216673.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI
This change was done as Union is supported by Torchscript. There isn't a CI failure on main branch. (Probably you are referring to fbcode)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah.

RuntimeError: 
Union[Tuple[Dict[str, Tensor], List[Dict[str, Tensor]]], List[Dict[str, Tensor]], Dict[str, Tensor]] cannot be used as a tuple:
  File "/data/sandcastle/boxes/eden-trunk-hg-fbcode-fbsource/fbcode/buck-out/dev/gen/smart/smart_catalog_filter/inference/handler_test#binary,link-tree/smart_catalog_filters/embedding.py", line 144
    def forward(self, images: List[Tensor]):
        _, results = self.rcnn(images)
                     ~~~~~~~~~~~~~~~~ <--- HERE
        return [
            (self.get_emb(result["features"]), result["boxes"]) for result in results


----------------------------------------------------------------------

Copy link
Contributor

Choose a reason for hiding this comment

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

@prabhat00155 Thanks for flagging, I see it. This is something we should keep in mind for future Annotations and PRs.

Union of a tuple with a non-tuple is likely to break code because of expressions like:
a, b = result

My proposal is to amend the diff on FBcode to either remove the annotation or remove the union (whichever passes all tests) and then sync the change that you will do on FBcode to GH on a cherrypick OR on a new PR (Which ever easier).

Let me know your thoughts on the above and if I can do something to help.

Copy link
Contributor

@prabhat00155 prabhat00155 Nov 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, I'll amend the diff(removing the type annotation).

"""
Args:
images (list[Tensor]): images to be processed
targets (list[Dict[Tensor]]): ground-truth boxes present in the image (optional)
targets (list[Dict[str, Tensor]]): ground-truth boxes present in the image (optional)

Returns:
result (list[BoxList] or dict[Tensor]): the output from the model.
Expand Down Expand Up @@ -97,7 +104,7 @@ def forward(self, images, targets=None):
features = OrderedDict([("0", features)])
proposals, proposal_losses = self.rpn(images, features, targets)
detections, detector_losses = self.roi_heads(features, proposals, images.image_sizes, targets)
detections = self.transform.postprocess(detections, images.image_sizes, original_image_sizes)
detections = self.transform.postprocess(detections, images.image_sizes, original_image_sizes) # type: ignore[operator]
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, what's up with that warning?

Copy link
Contributor Author

@oke-aditya oke-aditya Nov 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Here is the error.

torchvision/models/detection/generalized_rcnn.py:106: error: "Tensor" not
callable  [operator]
            detections = self.transform.postprocess(detections, images.ima...
                         ^
Found 1 error in 1 file (checked 143 source files)

Exited with code exit status 1

We pass transform as nn.Module to generalized_rcnn. While calling this function we use the GeneralizedRCNNTransform.

TheGeneralizedRCNNTransform class in other file has a method called postprocess but generalized_rcnn does not know that. So it thinks it's a Tensor and not a function, hence the error.

I don't know easy way to fix this, maybe we should type transform as GeneralizedRCNNTransform ?


losses = {}
losses.update(detector_losses)
Expand Down