microsoft / onnxruntime

ONNX Runtime: cross-platform, high performance ML inferencing and training accelerator
https://onnxruntime.ai
MIT License
14.63k stars 2.92k forks source link

Any interest in hosting the Rust bindings #11992

Open boydjohnson opened 2 years ago

boydjohnson commented 2 years ago

This repo has csharp, java, js, objectivec bindings. There are rust bindings at https://github.com/nbigaouette/onnxruntime-rs/. The maintainer isn't able to put in the time now and the community is trying to decide how best to organize themselves. This is the coordinating issue (https://github.com/nbigaouette/onnxruntime-rs/issues/112). One idea that was floated was to host the rust bindings here. It would give them greater visibility and attention.

Would you host the Rust bindings along with the other bindings in this repo? I could provide most of the work needed.

edgchen1 commented 2 years ago

@faxu @pranavsharma What do you think?

faxu commented 2 years ago

@boydjohnson That sounds great, and we'd love to have that contribution. Please let us know the best way to reach you and we can hop on a call to discuss further. CC @RyanUnderhill

boydjohnson commented 2 years ago

Hi @faxu, my email address is johnson [dot] boyd [at] gmail [dot] com I am in the central timezone in US, just to let you know.

Qinka commented 2 years ago

I think it is a good idea to host rust binding, and I also hope there is an official binding for rust.

But I think rust-ndarray might be an obstacle. rust-ndarray lack a lot of function, e.g. argmax/argmin, and there should be some API for reading image.

qwerty2501 commented 2 years ago

But I think rust-ndarray might be an obstacle. rust-ndarray lack a lot of function, e.g. argmax/argmin, and there should be some API for reading image.

Certainly, that's true. I guess better only merge onnxruntime-sys (without onnxruntime crate) to this repo in #12606 as first step of rust binding. And then after merged #12606, need discussion how to make wrapper.

@boydjohnson I would like to hear your opinion.

boydjohnson commented 2 years ago

Hi @qwerty2501 I think the updates that may be needed with the high-level onnxruntime Rust bindings should be made after it gets merged into this repo, but before it is released to crates.io. The things that I thought weren't right about the high-level bindings had to do with Send and Sync not being implemented for Session. In the work that I did in #12606 Session is now Send.

What are your thoughts on what is not quite right with the high-level bindings?

qwerty2501 commented 2 years ago

@boydjohnson Thank you for your opinion. In my opinion, I think it does not necessarily have to be released high-level onnxruntime. However, I think it would be preferable to release thin wrapper.

What are your thoughts on what is not quite right with the high-level bindings?

Here is things that is not right about the high-level bindings.

function name is not correctly corresponds 1 on 1 to sys API.

I guress function name is too abstract. e.g. with_number_threads actually calls SetIntraOpNumThreads function in with_number_threads .
And onnxruntime has SetInterOpNumThreads function. So I changed onnxruntime with_number_threads function to with_intra_op_num_threads and with_inter_op_num_threads in VOICEVOX/onnxruntime-rs https://github.com/VOICEVOX/onnxruntime-rs/blob/master/onnxruntime/src/session.rs#L112-L126

depends on rust-ndarray

It is similar opinion by @Qinka. Rust already has multiple type n-dimensional array libraries in crates.io. And I think user should be able to choose among those n-dimensional array libraries.
Of course onnxruntime-rs can provide as cargo feature base selection, If you want high-level onnxruntime API. However, considering the transition of the libraries, I think it is better that provide more low API. And onnxruntime-rs can not passing any dimension arrays like Session::run . So I modified it to be able to pass any dimension array in VOICEVOX/onnxruntime-rs

And here is a summary of the wrapper bindings I want.

Thin wrapper but safety guaranteed

To avoid depends on rust-ndarray. e.g. Session::run pub fn run(&self,input_arrays:Vec<Array<TIn, D>>) to pub fn run(&self,input_data: &[D]) . It is able to because it pass pointer to raw run function in the end.

function name should be corresponds 1 on 1 sys API function

It is necessary to rename the function names like with_number_threads. I believe there are still similar names.

boydjohnson commented 2 years ago

Thanks, @qwerty2501 , I will look at the whether the function names match the sys API, and change the one you mentioned.

With the Session::run function, I think we can have a lower-level one and then a higher-level one that works with ndarray.

qwerty2501 commented 2 years ago

Thank you!

davidatsurge commented 1 year ago

Hello all. Are there any updates on this? Thank you for all the work you've put into this so far!

boydjohnson commented 1 year ago

Hi @davidatsurge, We ran into a slight issues between Windows and Linux that we are sorting out. Overall, the problem is between having a specific location to put the onnxruntime dll, or letting the OS find it itself. The compromise we came to is using the libloading facility within bindgen to be able to pass a path to the Rust code, specifying where to find onnxruntime dll. I'll have an update to the code shortly.

davidatsurge commented 1 year ago

Amazing! Thank you!

wasd96040501 commented 1 year ago

Hi @boydjohnson, has this rust binding published to crates.io?

boydjohnson commented 1 year ago

Hi @wasd96040501 Not yet. I hope soon, though.

ikrivosheev commented 1 year ago

@boydjohnson hello! Any updates?)

devigned commented 1 year ago

Hi @wasd96040501 Not yet. I hope soon, though.

Is there anything holding back crate publication? Is there something someone can help with to get crates published?