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

[WASI-NN] Add support for a ONNXruntime backend using ort #7691

Merged
merged 8 commits into from
Mar 13, 2024

Conversation

devigned
Copy link
Contributor

This change adds an ONNXruntime backend for WASI-NN. Since there is only one backend implemented, this will help to move the standardization of this proposal forward.

Also, the example usage of the ONNXruntime backend shows how to build a component using WASI-NN and ONNXruntime.

@devigned devigned requested review from a team as code owners December 14, 2023 23:27
@devigned devigned requested review from alexcrichton and removed request for a team December 14, 2023 23:27
@devigned
Copy link
Contributor Author

@abrown please take a look when you have a minute. Thank you!!

@alexcrichton alexcrichton requested review from abrown and removed request for a team and alexcrichton December 15, 2023 16:19
@abrown
Copy link
Contributor

abrown commented Dec 15, 2023

@devigned, I'll take a closer look next week. I will say I've been been changing the ground under your feet here (sorry 😁) with #7679; that PR will mean we have to move some of your tests over to test-programs and write Rust tests instead of bash scripts. An overall improvement, just inconvenient timing.

As for the CI failures here: I think we can add a line to skip-tree in deny.toml to ignore the version mismatch of the half dependency: { name = "half", depth = 1 }. As for the cargo vet` failures, I think we'll have to audit those, maybe even as a separate PR... anything you can do to reduce the number of dependencies coming in will make that easier (drop features?).

Copy link
Contributor

@abrown abrown left a comment

Choose a reason for hiding this comment

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

Thanks for all the work to make this happen! This is going to be a great addition to Wasmtime. Let me know on Zulip if you need help working through the test refactoring, CI issues, etc.

ci/run-wasi-nn-example.sh Outdated Show resolved Hide resolved
ci/run-wasi-nn-example.sh Outdated Show resolved Hide resolved
crates/wasi-nn/src/backend/onnxruntime.rs Outdated Show resolved Hide resolved
crates/wasi-nn/src/backend/onnxruntime.rs Show resolved Hide resolved
}
}

pub fn f32_vec_to_bytes(data: Vec<f32>) -> Vec<u8> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should explain somewhere why we are forced to copy the tensor twice — at input and output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to find a better way of doing this rather than chunking from u8 to the type and size of the model input.

@mtobin-tdab
Copy link

This is great, I have been testing different ONNX models and encountered a data type mismatch error for certain models, specifically at the wasi-nn compute function in bindings.rs. The error indicates a failure to parse ctx: GraphExecutionContext as i32:

1: error while executing at wasm backtrace:
       0: 0x245b42a3 - wit-component:shim!indirect-wasi:nn/inference-compute
       1: 0x698ad - octaioxide::bindings::wasi::nn::inference::compute::h1bf34b4c8ec1bf77
                       at ....................bindings.rs:11137:15
2: Failed while accessing backend
3: Data type mismatch: was Int64, tried to convert to Float32

As far as I can tell the ctx value is 0 when I pass it to compute, I've tried to cast it to f32, but the compiler expects a u32.

Very possible I'm missing something obvious as I am fairly new to both WASM and Rust. But thought I would share.

For some extra context, both models which have produced this error have been classification models (specifcially an xgboost and sklearn's SGDClassifier model) - Are there model types in ONNX format which we would never expect this to be compatible with?

@devigned devigned force-pushed the wasi-nn-ort branch 5 times, most recently from 3324720 to 6c5022f Compare February 8, 2024 14:41
@devigned
Copy link
Contributor Author

devigned commented Feb 8, 2024

This is great, I have been testing different ONNX models and encountered a data type mismatch error for certain models, specifically at the wasi-nn compute function in bindings.rs. The error indicates a failure to parse ctx: GraphExecutionContext as i32:

1: error while executing at wasm backtrace:
       0: 0x245b42a3 - wit-component:shim!indirect-wasi:nn/inference-compute
       1: 0x698ad - octaioxide::bindings::wasi::nn::inference::compute::h1bf34b4c8ec1bf77
                       at ....................bindings.rs:11137:15
2: Failed while accessing backend
3: Data type mismatch: was Int64, tried to convert to Float32

As far as I can tell the ctx value is 0 when I pass it to compute, I've tried to cast it to f32, but the compiler expects a u32.

Very possible I'm missing something obvious as I am fairly new to both WASM and Rust. But thought I would share.

For some extra context, both models which have produced this error have been classification models (specifcially an xgboost and sklearn's SGDClassifier model) - Are there model types in ONNX format which we would never expect this to be compatible with?

Is there any chance you could share code that reproduces this behavior, or create a branch with a failing test reproducing this behavior?

@devigned
Copy link
Contributor Author

devigned commented Feb 8, 2024

@sunfishcode, I believe I need someone that is a trusted contributor to Wasmtime to approve the cargo vet dependencies added in this PR (per https://docs.wasmtime.dev/contributing-coding-guidelines.html#dependencies-of-wasmtime). Would you please consider allow listing the failing vet dependencies?

I'm not sure if the cargo deny result for ittapi-sys is spurious or if the license is going to be a problem. What would you advise?

Also, if this is not how this stuff should work, please let me know :).

@devigned devigned force-pushed the wasi-nn-ort branch 11 times, most recently from 4eb2d8a to c438db1 Compare February 8, 2024 21:28
@squillace
Copy link

let's go baby

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 12, 2024
@squillace
Copy link

GACK. MingGW!!! crazy amazing

@squillace
Copy link

you having fun yet, @devigned ?

@alexcrichton
Copy link
Member

Since this isn't intended to work everywhere just yet, it might be best to add a new builder on CI specifically dedicated to testing ONNX rather than trying to get it to work across all the platforms.

If that works for you I'd recommend copying this to start, updating all the bits there (e.g. run on ubuntu, not on windows), and then be sure to add an entry down here to ensure we gate on it in CI.

Also, what I might also recommend, if you add prtest:full as a string in a commit message it'll run full CI on this PR before going to the merge queue, and that may help avoid the bouncing back and forth between the merge queue and back here.

@squillace
Copy link

drinking booze all night now over here, best soap opera I've watched in years

@devigned
Copy link
Contributor Author

devigned commented Mar 12, 2024

@alexcrichton, in 2b7a104 I'm reducing the set of triplets in which ONNX will run. I do not plan on pursuing riscv or s390 precompiled onnxruntime bins at this point. Are you good with the matrix-test approach or would you still prefer I break it out into another builder?

@devigned devigned force-pushed the wasi-nn-ort branch 2 times, most recently from 7f7fca5 to 2b7a104 Compare March 12, 2024 21:07
@alexcrichton
Copy link
Member

Nah if that works for you seems fine, just trying to save some CI headache.

@abrown abrown added this pull request to the merge queue Mar 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 12, 2024
devigned and others added 7 commits March 12, 2024 20:30
Signed-off-by: David Justice <david@devigned.com>
This change is the result of a long slog through the dependencies of the
`ort` library. The only missing dependency is `compact_str`, which needs
further discussion.
Signed-off-by: David Justice <david@devigned.com>
Signed-off-by: David Justice <david@devigned.com>
Signed-off-by: David Justice <david@devigned.com>
Signed-off-by: David Justice <david@devigned.com>
@devigned devigned force-pushed the wasi-nn-ort branch 4 times, most recently from 08c41f2 to 2416343 Compare March 13, 2024 10:58
Signed-off-by: David Justice <david@devigned.com>
@abrown abrown added this pull request to the merge queue Mar 13, 2024
Merged via the queue into bytecodealliance:main with commit d6945bc Mar 13, 2024
43 checks passed
@devigned devigned deleted the wasi-nn-ort branch March 13, 2024 16:37
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.

6 participants