-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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.
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
[Frontend] Fix shape init for ONNX frontend #7055
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dlexplorer ,
Would it be possible to add a test stressing the onnx case?
to stress test of tvmc with onnx frontend? We can add inception v1 into tests, I see some BTW, I was wrong saying in the desc that the same approach was used in the tests. It is used in another app: https://github.com/apache/tvm/blob/main/apps/wasm-standalone/wasm-graph/tools/build_graph_lib.py#L33 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dlexplorer, thanks for the PR. I got a comment, maybe to generate a quick discussion.
Also cc @comaniac.
If no explicit shapes are passed into relay.frontend.from_onnx, we can use shapes defined in the onnx model itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the overall design we've reached here, I think it's the right call. I think there's a bug with dynamic onnx inputs
Can we add a unit test that hits this import path?
if shape is None: | ||
shape = {} | ||
for i in model.graph.input: | ||
shape[i.name] = [dim.dim_value for dim in i.type.tensor_type.shape.dim] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to edit this logic just a little bit. If we have an unspecified input shape in the onnx model, it will show up as a variable, I think we need to identify that case and convert it to relay.Any() here.
For instance, this version of BERT has for one of it's input shapes [input_ids_dynamic_axes_1,384]
I believe this would fail on those inputs.
Could you specify usecases to cover in the test? |
I just want to hit the code, even creating a silly 1 op onnx model and parsing without providing shapes would be sufficient. |
There is this one (below) that compiles a network using TVMC (and hence this code), without providing shape. Are you talking about something different? tvm/tests/python/driver/tvmc/test_compiler.py Line 117 in dc51f17
|
That works for me. |
The model https://github.com/onnx/models/tree/master/vision/classification/inception_and_googlenet/inception_v1 has 0th input pointing to the weights. The change repeats onnx frontend tests approach to shapes for all inputs