Skip to content

[frontend][onnx]fix pad constant value is none#12555

Closed
chengven027 wants to merge 1 commit intoapache:mainfrom
chengven027:pad.fix
Closed

[frontend][onnx]fix pad constant value is none#12555
chengven027 wants to merge 1 commit intoapache:mainfrom
chengven027:pad.fix

Conversation

@chengven027
Copy link
Contributor

hi, cc @AndrewZhaoLuo
When I import my onnx model. I found my model get a error with pad. because the pad constant value is none. shown as follow:
1661258976676
It`s just a very little bug. So I fix it and no unnit test.

@github-actions github-actions bot requested a review from AndrewZhaoLuo August 23, 2022 12:51
@chengven027 chengven027 changed the title fix pad constant value is none [frontend][onnx]fix pad constant value is none Aug 23, 2022
def _impl_v11(cls, inputs, attr, params):
pads = inputs[1]
if len(inputs) == 3:
if len(inputs) == 3 and input[2] is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading the docs https://github.com/onnx/onnx/blob/main/docs/Operators.md#Pad we should also be checking if input[2] is also an empty string or False.

Pythonically we can just do and input[2] (though I would add a comment since this handles multiple types)

Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

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

Hi @chengven027-intellif, thanks for the PR. Any chance we can have a test case for this change?

@chengven027 chengven027 requested review from AndrewZhaoLuo and leandron and removed request for AndrewZhaoLuo and leandron August 24, 2022 13:42
@chengven027
Copy link
Contributor Author

Dose this PR need to be reviewed again? @AndrewZhaoLuo @leandron

verify_pad_v11(np.random.randn(2, 2).astype(np.float32), [0, 1, 0, 0], "constant", 0.0)
verify_pad_v11(np.random.randn(2, 3).astype(np.float32), [1, 0, 0, 1], "constant", 0.0)
verify_pad_v11(np.random.randn(3, 2).astype(np.float32), [0, 0, 1, 0], "constant", 5.0)
verify_pad_v11(np.random.randn(3, 2).astype(np.float32), [0, 0, 1, 0], "constant", False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is setting False here (as opposed to None) reproducing the same issue this patch is fixing?

Copy link
Contributor Author

@chengven027 chengven027 Aug 26, 2022

Choose a reason for hiding this comment

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

No, But I can't add a test case with None.
First, In the doc test_forward.py test_pad, The value is a default number 0.0,can't set None.
def verify_pad_v11(indata, pads, mode="constant", value=0.0)
Then, the input node constant_value is a TensorProto.FLOAT. not None too.
Can you give me some suggestions to add a test case with 'None'? Thanks very much.

@AndrewZhaoLuo
Copy link
Contributor

Dose this PR need to be reviewed again? @AndrewZhaoLuo @leandron

Yeah I'll leave it to Leandron B)

@chengven027
Copy link
Contributor Author

I am sorry, I really can't create an unit test with empty string of constant_value.
@leandron @AndrewZhaoLuo

@AndrewZhaoLuo
Copy link
Contributor

I am sorry, I really can't create an unit test with empty string of constant_value. @leandron @AndrewZhaoLuo

In verify_pad_v11 I can seem to make the following:

          graph = helper.make_graph(
                [node],
                "pad_test",
                inputs=[
                    helper.make_tensor_value_info("input", TensorProto.FLOAT, list(indata.shape)),
                    helper.make_tensor_value_info("pads", TensorProto.INT64, (len(pads),)),
                    helper.make_tensor_value_info("constant_value", TensorProto.STRING, (1,)),
                ],
                initializer=[
                    helper.make_tensor("pads", TensorProto.INT64, (len(pads),), pads),
                    helper.make_tensor("constant_value", TensorProto.STRING, (1,), [b""]),
                ],
                outputs=[
                    helper.make_tensor_value_info("output", TensorProto.FLOAT, list(outdata.shape))
                ],
            )

However, my version of ONNXRT can't seem to accept the string type. What have you tried?

@chengven027
Copy link
Contributor Author

chengven027 commented Sep 1, 2022

@AndrewZhaoLuo Yes, I tried many times but I got the same error as yours.
onnxruntime.capi.onnxruntime_pybind11_state.InvalidGraph: [ONNXRuntimeError] : 10 : INVALID_GRAPH : This is an invalid model. Type Error: Type 'tensor(string)' of input parameter (constant_value) of operator (Pad) in node () is invalid.

@AndrewZhaoLuo
Copy link
Contributor

Can you share your model by any chance? Can you also run your model via ONNX-RT?

@chengven027
Copy link
Contributor Author

Can you share your model by any chance? Can you also run your model via ONNX-RT?

coat_model.zip

  1. model have been uploaded.
  2. The model can pass by ONNX-RT. And the floating point comparison between tvm and onnx-RT result can also pass.

@AndrewZhaoLuo
Copy link
Contributor

AndrewZhaoLuo commented Sep 6, 2022

Sorry, I'll take a look tomorrow.

Edit: On Friday

@chengven027
Copy link
Contributor Author

Hey, @AndrewZhaoLuo Have you seen this PR, and any update with me? thanks.

@AndrewZhaoLuo
Copy link
Contributor

Oops, I am so sorry, I have been very busy lately. I will try to get to this this weekend

@areusch areusch added needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it and removed needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it labels Oct 19, 2022
@chengven027 chengven027 deleted the pad.fix branch January 12, 2024 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants