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

ONNX: Implement Equal operator #557

Closed
willstott101 opened this issue Jul 27, 2023 · 7 comments · Fixed by #661
Closed

ONNX: Implement Equal operator #557

willstott101 opened this issue Jul 27, 2023 · 7 comments · Fixed by #661
Labels

Comments

@willstott101
Copy link

willstott101 commented Jul 27, 2023

Feature description

It'd be great if the onnx import could support the Equal operator.

Feature motivation

Version 4 of the Silero VAD model uses this, FYI no rust runtime supports the latest silero model atm, so movement in that direction would be pretty cool :)

I'm sure there are plenty more operators or features missing, but making an issue for the first one.

Relates to: #322
Other silero issues in the rust ecosystem: sonos/tract#1029

@nathanielsimard
Copy link
Member

@Luni-4 implemented the equal operator, and its merged on main: 67ce5ec.

@willstott101
Copy link
Author

Oh cool! Thank you all for your hard work. However I still get the same error message - maybe nodes are different to operators?

will@will-laptop:~/repos/misc/experiments/rust-vad$ cargo build
   Compiling vad v0.1.0 (/home/will/repos/misc/experiments/rust-vad)
error: failed to run custom build command for `vad v0.1.0 (/home/will/repos/misc/experiments/rust-vad)`

Caused by:
  process didn't exit successfully: `/home/will/repos/misc/experiments/rust-vad/target/debug/build/vad-cd82745b2cee2ca8/build-script-build` (exit status: 101)
  --- stdout
  [DEBUG - /home/will/repos/others/burn/burn-import/src/onnx/from_onnx.rs:346] Found ONNX node type => Equal
  [ERROR - /home/will/repos/others/burn/burn-import/src/logger.rs:30] PANIC => panicked at 'called `Result::unwrap()` on an `Err` value: VariantNotFound', /home/will/repos/others/burn/burn-import/src/onnx/from_onnx.rs:319:73

  --- stderr
  thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: VariantNotFound', /home/will/repos/others/burn/burn-import/src/onnx/from_onnx.rs:319:73
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
will@will-laptop:~/repos/misc/experiments/rust-vad$ git -C /home/will/repos/others/burn/ log
commit 6b459fde8282a46d3c8848091676bc384b07445f (HEAD -> main, origin/main, origin/HEAD)
Author: Dilshod Tadjibaev <[email protected]>
Date:   Fri Jul 28 11:52:23 2023 -0500

    Update Conv2d source generation to support all attributes (#558)

commit 7dbd43d111224ff9881e026cf5629294122914f5
Author: Luni-4 <[email protected]>
Date:   Fri Jul 28 18:51:41 2023 +0200

    Rewrite `publish.sh` script in Rust language (#556)

commit aa4af29e3f877aea25a7676eada7aa23bb3edc6c
Author: Louis Fortier-Dubois <[email protected]>
Date:   Fri Jul 28 10:41:27 2023 -0400

    Matmul speedup (contiguous load) (#559)

commit 67ce5ec62cb0e431c0353edd015ef534bef3abe6
Author: Luni-4 <[email protected]>
Date:   Fri Jul 28 01:37:10 2023 +0200

    Implement binary operators for `burn-import` (#532)

@antimora
Copy link
Collaborator

Reopenning it to investigate closely.

@antimora antimora reopened this Jul 28, 2023
@antimora
Copy link
Collaborator

@willstott101 if it's possible, could you please share the link to the ONNX file or upload if it's possible to do so? It will help to troubleshoot.

Please, note, burn-import still being developed and there are many ops are missing. ONNX is complex, and our strategy has been to move forward by adding features required by a concrete ONNX file instance, instead of going by the ONNX spec and try to cover all cases, which isn't practical.

Thanks for using and reporting the issue.

@willstott101
Copy link
Author

willstott101 commented Jul 29, 2023

Of course! https://github.com/snakers4/silero-vad/blob/82d199ff22ae8897946ebb374d216cf6ce4f9aa3/files/silero_vad.onnx

Bear in mind that this is possibly quite an unusual model as I have struggled to find any libraries other than onnxruntime that can actually use this onnx file. I don't know if that makes it a good example for burn at this stage or a bad one? 😆

If I have time I might tinker with adding some operators it uses, but i'm really just taking a look at this for curiosity's sake. It's a model I deploy at work with onnxruntime on desktop intel cpus, and I just like the idea of being able to use vulkan and rust to speed it up.

@antimora
Copy link
Collaborator

@willstott101, thanks for sharing. I think have found an issue. The Equal implementation done earlier was with tensors but the example you shared is with scalars. I currently working on improving burn-import's binary operators to support scalars. So hopefully with that it'll fixed. However, your next problem is that your graph uses two major architectural features that missing from burn-import: branching and subgraphs, these are double with Burn but requires a bit of refactoring. Basically, burn-import will need to multi-graph to multi-model conversation. Currently, burn-import assumes a single graph and it generates a single model.

The top graph from your example:
image

One of the subgraphs (note this subgraph contains LSTM which is supported by burn but missing from burn-import):
image

The number of subgraphs:

image

Yes, your ONNX will be challenging to make it work but it's completely doable with Burn.

Tagging @nathanielsimard and @Luni-4 so they're aware of the use cases.

Note: I used Netron to view your ONNX file: https://netron.app/

@antimora
Copy link
Collaborator

@willstott101 I have filed a ticket to support subgraphs: #724

@antimora antimora added the onnx label Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants