Skip to content
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

New Squeeze (ONNX opset >= 13) #1154

Closed
alexander-camuto opened this issue Aug 14, 2023 · 3 comments · Fixed by #1155
Closed

New Squeeze (ONNX opset >= 13) #1154

alexander-camuto opened this issue Aug 14, 2023 · 3 comments · Fixed by #1155

Comments

@alexander-camuto
Copy link
Contributor

A user of ours ran into a set of models which tract fails to analyze.

Here is the output for one such model using the tract cli built from the latest commit (0ee4781 at time of writing):

┏ 0 Source input
┃   ━━━ batch_size,128,F32
┣┻ 8 Mul /Mul
┃   ━━━ batch_size,128,F32
┣┻┻ 9 Gemm /mlp_1/layers/layers.0/fc/Gemm
┃   ━━━ batch_size,64,F32
┣ 10 Clip /mlp_1/layers/layers.0/activation/Relu
┣┻┻ 11 Gemm /mlp_1/layers/layers.1/fc/Gemm
┃   ━━━ batch_size,32,F32
┣ 12 Clip /mlp_1/layers/layers.1/activation/Relu
┣┻┻ 13 Gemm /lin_layer_1/fc/Gemm
┃   ━━━ batch_size,1,F32
┣ 14 Squeeze13 /Squeeze
    ━━━ ..,?
[2023-08-14T19:17:14.078176000Z ERROR tract] Error at stage analyse
    
    Caused by:
        0: Failed analyse for node #14 "/Squeeze" Squeeze13
        1: Failed analyse for node #14 "/Squeeze" Squeeze13
        2: Infering facts
        3: Wrong input number. Rules expect 2, node has 1.

You can find a zip with all the models in question at zkonduit/ezkl#411 .

To reproduce the issue simply run tract <MODEL IN QUESTION> with one of the files in the zip.

@kali kali changed the title Squeeze fails to analyze for some models New Squeeze (ONNX opset >= 13) Aug 15, 2023
@kali
Copy link
Collaborator

kali commented Aug 15, 2023

Squeeze now takes the axes to remove as a second input instead of a parameter. Will fix.

In the meantime, serializing the model with ONNX operator set <13 may be a workaround.

@kali
Copy link
Collaborator

kali commented Aug 15, 2023

OK, more subtle than I thought. Second input is already implemented, but it is optional (tract code assume it is required). When it is optional, squeeze remove all 1-dim indexed.

I will implement it. But I strongly recommend not using this feature, it is an absolutely terrible idea. This network has a variable batch size, as long as it will be using a symbol, tract will not remove the axis. But if something sets the batch_size to 1, then the axis will be removed. This is bad.

@alexander-camuto
Copy link
Contributor Author

ty so much

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 a pull request may close this issue.

2 participants