Add typing annotations to detection/generalized_rcnn #4631
Conversation
This is the only mypy error. I think it should be safe to ignore. |
|
Hi @datumbox @NicolasHug can you guys have a review? I think this should be good to go. |
|
A Friendly ping to @NicolasHug and @datumbox for a review :) |
| @@ -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] | |||
There was a problem hiding this comment.
Out of curiosity, what's up with that warning?
There was a problem hiding this comment.
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 ?
|
Hey @datumbox! You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py |
| 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]]]: |
There was a problem hiding this comment.
@datumbox This change causes failure in internal tests, check D32216673.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
----------------------------------------------------------------------
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Thanks, I'll amend the diff(removing the type annotation).
Summary: * Update typing * Fix bug * Unblock mypy * Ignore small error Reviewed By: kazhang Differential Revision: D32216673 fbshipit-source-id: cc73b84b830c1e0c31502a2cb6cd7408a899d6ba Co-authored-by: Vasilis Vryniotis <[email protected]>
* Update typing * Fix bug * Unblock mypy * Ignore small error Co-authored-by: Vasilis Vryniotis <[email protected]>
Helps #4582
This is easier to tackle. I think this should be good to go with less friction.
cc @datumbox