fix: clip-onnx spec 9 & 11 #70584
fix: clip-onnx spec 9 & 11 #70584
Conversation
CI Flow Status⚛️ CI FlowRuleset - Version:
You can add a comment to the PR and tag @pytorchbot with the following commands: # ciflow rerun, "ciflow/default" will always be added automatically
@pytorchbot ciflow rerun
# ciflow rerun with additional labels "-l <ciflow/label_name>", which is equivalent to adding these labels manually and trigger the rerun
@pytorchbot ciflow rerun -l ciflow/scheduled -l ciflow/slowFor more information, please take a look at the CI Flow Wiki. |
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit f7610b8 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
|
@thiagocrepaldi @BowenBao Could you kindly review this PR? :-) |
mruberry
added
the
triaged
thiagocrepaldi
added
the
onnx-needs-info
There was a problem hiding this comment.
Why doesn't Operators.md list opset9 for clip? I expected to either have opset 9 listed or not having a symbolic for clip on symbolic_opset9.py.
@ganler Please clarify
|
@thiagocrepaldi Thanks for the reply! and I am sorry for the lack of clarification. You are right that Clip does not have a version called version 9. But I just found that the clip operator was originally implemented in pytorch/torch/onnx/symbolic_opset9.py Line 1594 in 4b47047 According to the doc, Clip has version [13, 12, 11, 6, 1] where [11, 6, 1] do not support integer-typed clip but PyTorch seems to export that unsupported operator still, leading to an error in model importation for ONNXRuntime. Therefore, I was thinking maybe we can still support that by apply the Regarding the versions, you are right that maybe we should not put the clip implementation in |
Given the spec, does the behavior implemented on symbolic_opset9.py belongs to 1, 6, 11, 12 or 13? Whatever is exported by pytorch has to fit the onnx spec. If that is not the case, then we have a bug on pytorch as opposed to the onnx symbolic |
IMHO, Clip in pytorch/torch/onnx/symbolic_opset9.py Line 1611 in 4b47047
Agreed. So I would suggest:
I am happy to implement that in this PR if it looks to you. :-) |
Looks sensible to me. let me check with a colleague @BowenBao how do you feel about moving clip-9 code to clip-6? Looking the onnx spec, there is no such clip-9 and clip-6 does support float for min/max |
|
@thiagocrepaldi Thanks for the review. Comments should be resolved! Could you also take a look at #70571 ? |
… Models There are a few ONNX operators do not support non-float (e.g., integer) inputs at early versions. For example, Clip supports non-float types until [opset 12](https://github.com/onnx/onnx/blob/main/docs/Changelog.md#type-constraints-280), that said older versions like [opset 6](https://github.com/onnx/onnx/blob/main/docs/Changelog.md#type-constraints-107) cannot deal with integer types. I initially find such a bug in Clip (#70584), but later found more: 1. Clip < 12; 2. Min/Max < 12; 3. ReLU < 14; 4. Pad < 11; In PyTorch, if we export Max-11 with integer inputs, actually the exportation will succeed; however, fail when imported by other frameworks like ONNXRuntime. ```python import torch class Net(torch.nn.Module): def __init__(self) -> None: super().__init__() def forward(self, x: torch.Tensor): return torch.max(x, x + 1) net = Net() onnx_model = 'test.onnx' torch.onnx.export(net, (torch.zeros((3, 3), dtype=torch.int32),), onnx_model, verbose=True, opset_version=11) ``` This is an unexpected behavior as we want to ensure that every model exported by PyTorch is valid (#70584 (comment)). Theoretically, we can simply forbid such cases (e.g., `Clip<int>` < 12, `ReLU<int>` < 14). But actually we can enhance the compatibility and flexibility of PyTorch by simply casting inputs of those operators into float tensors, which allows the float operator functions, and then casting it back to original types. This PR implements the second approach to achieve better compatibility in PyTorch. @garymm @thiagocrepaldi Pull Request resolved: #72401 Approved by: https://github.com/garymm, https://github.com/thiagocrepaldi
… Models (#72401) Summary: There are a few ONNX operators do not support non-float (e.g., integer) inputs at early versions. For example, Clip supports non-float types until [opset 12](https://github.com/onnx/onnx/blob/main/docs/Changelog.md#type-constraints-280), that said older versions like [opset 6](https://github.com/onnx/onnx/blob/main/docs/Changelog.md#type-constraints-107) cannot deal with integer types. I initially find such a bug in Clip (#70584), but later found more: 1. Clip < 12; 2. Min/Max < 12; 3. ReLU < 14; 4. Pad < 11; In PyTorch, if we export Max-11 with integer inputs, actually the exportation will succeed; however, fail when imported by other frameworks like ONNXRuntime. ```python import torch class Net(torch.nn.Module): def __init__(self) -> None: super().__init__() def forward(self, x: torch.Tensor): return torch.max(x, x + 1) net = Net() onnx_model = 'test.onnx' torch.onnx.export(net, (torch.zeros((3, 3), dtype=torch.int32),), onnx_model, verbose=True, opset_version=11) ``` This is an unexpected behavior as we want to ensure that every model exported by PyTorch is valid (#70584 (comment)). Theoretically, we can simply forbid such cases (e.g., `Clip<int>` < 12, `ReLU<int>` < 14). But actually we can enhance the compatibility and flexibility of PyTorch by simply casting inputs of those operators into float tensors, which allows the float operator functions, and then casting it back to original types. This PR implements the second approach to achieve better compatibility in PyTorch. garymm thiagocrepaldi Pull Request resolved: #72401 Approved by: https://github.com/garymm, https://github.com/thiagocrepaldi Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/c1f0e6e763b6720789e54cb83477bb353139a12f Reviewed By: mehtanirav Differential Revision: D35582765 fbshipit-source-id: 10bf99c429f0ea7eaaa37c667211a5db2e0b3c09
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
titaiwangms
removed
the
onnx-needs-info
|
@ganler do you mind rebasing this? |
|
/easycla As part of the transition to the PyTorch Foundation, this project now requires contributions be covered under the new CLA. See #85559 for additional details. This comment will trigger a new check of this PR. If you are already covered, you will simply see a new "EasyCLA" check that passes. If you are not covered, a bot will leave a new comment with a link to sign. |
|
|
/easycla |
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
9 participants
clip-v6 and clip-v9 only support float(16/32/64) as inputs such that prior implementation will make exported
clamp(int_min, int_max)fail in ONNXRuntime.To fix it, use the "cast-clip-cast" pattern as a workaround.
cc: @peterbell10 @BowenBao