Coordinating Effort around a community run organization

[boydjohnson]

I was wondering what people thought about making an /rust-onnxruntime organization with some type of maintainers structure. There are several forks with a lot of work on them, and even some crates released with forks. It would be good to consolidate this effort in the same place.

I'm pinging people with long forks that have been recently updated.

@qwerty2501 @xaynetwork @seddonm1 @EmbarkStudios

[seddonm1]

I would like to upstream my changes if possible as I have focused on implementing io_binding . My fork has changed quite a lot of log levels etc though.

I also had to expose an extra C API method to be able to copy data back from the GPU when io_binding which if we had more users would be easier to convince the onnxruntime team to include

[nbigaouette]

That's a great idea! Sorry I cannot put work on this project, transferring its ownership to an org would certainly make sense.

[boydjohnson]

I have made the rust-onnxruntime organization. https://github.com/rust-onnxruntime. I'm going to wait a little while on this to see if we get more responses.

[seddonm1]

Out of interest, have you reached out to the main https://github.com/microsoft/onnxruntime repository to see if they would consider hosting this code as well?

[regzon]

I use the ONNX Runtime bindings for Rust in my organization quite a lot (with ARM32 and ARM64 devices). And I'd be happy to help!

[qwerty2501]

I think that is good if it is hosted by microsoft. otherwise I don't feel very interested. Since Onnxruntime is not in great demand, Maintenance may not be continuous. And I need onnxruntime's thin wrapper. But this repo is a slightly high level wrapper. Therefore I guress too difficult to integrate the opinions of the wrapper's direction.

[boydjohnson]

Just giving an update here. Thanks all, for the replies. Looks like the folks behind /microsoft/onnxruntime are interested in hosting the bindings. I'm going to talk to them about logistics next week, and then hopefully make the PR shortly after.

[seddonm1]

Great. Thanks for the update.

I would like to merge a lot of the GPU binding work as I am sure it is useful for others 👍

On Fri, 8 July 2022, 11:22 am Boyd Johnson, @.***> wrote:

Just giving an update here. Thanks all, for the replies. Looks like the folks behind /microsoft/onnxruntime are interested in hosting the bindings. I'm going to talk to them about logistics next week, and then hopefully make the PR shortly after.

— Reply to this email directly, view it on GitHub https://github.com/nbigaouette/onnxruntime-rs/issues/112#issuecomment-1178432072, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA2O2UCFJWD7ID3JPNAOL33VS57F3ANCNFSM5ZSBZCLQ . You are receiving this because you were mentioned.Message ID: @.***>

[boydjohnson]

Just giving another update after I talked with the Microsoft folks. From talking to them, it might be a longer process. The initial PR will be made this month or next. A legal review may need to be made because of licensing issues. When I have my fork of onnxruntime more complete I may ask folks to take a look.

@regzon Those Arm32 and Arm64 bindings that you are using at you company, must be in another fork? If there is a company need we may be able to put them in when I make the PR to onnxruntime.

[qwerty2501]

@boydjohnson Is Microsoft going to make onnxruntime-rs from scratch? Or make it as fork this repo?

[boydjohnson]

@qwerty2501 Fork of this repo.

[regzon]

@regzon Those Arm32 and Arm64 bindings that you are using at you company, must be in another fork? If there is a company need we may be able to put them in when I make the PR to onnxruntime.

Bindings for the ARM64 arch are in the organization's fork. For the ARM32 - they're not tested well, so I didn't publish them.

Regarding the PR - I'll help however I can.

[nbigaouette]

I don't mind transferring the repo to the rust-onnxruntime org but would that help the transition to microsoft? It might help for updating crates.io's version though?

[boydjohnson]

@nbigaouette I think we have settled on microsoft/onnxruntime hosting the bindings and it just might be a slow process.

Just so everyone knows, Microsoft folks said they want to make their own crates.io listing and not use a previous one.

[seddonm1]

I have not found binding to ARM32, ARM64 to be a problem as I have been testing ARM + NVIDIA (Jetson Orin AGX) successfully with my branch. The biggest challenge is building onnxruntime itself for ARM+NVIDIA. Thanks to @nbigaouette for building the base as it would have been very difficult otherwise.

[mpetri]

@boydjohnson Any updates on the move to microsoft/onnxruntime hosting?

[fkasischke]

@boydjohnson Any updates on the move to microsoft/onnxruntime hosting?

Any updates?

[alfredodeza]

It looks like bindings just got merged into https://github.com/microsoft/onnxruntime/pull/12606

[boydjohnson]

Yeah, we are now shooting for an April or June timeframe for releasing on crates. They want to make sure there are enough eyeballs on the bindings before the crates release.

[seddonm1]

Firstly, great work @boydjohnson .

I have spent a lot of time with the previous onnxruntime-rs bindings trying to get maximum performance out of them. I have found that while @nbigaouette did a great job, the deviation from the ORT C API made it harder to use.

I have stripped back the bindings and made them much more closely align with the C API. and exposed things like the CUDA/Tensorrt plus io_binding capabilities here: https://github.com/seddonm1/onnxruntime-rs

I have found these much easier to use (and potentially change out ndarray) but would value feedback from the community given the work @boydjohnson has done getting to this point in the official repo.

I have no attachment to my code and would gladly contribute it to the official bindings based on feedback from everyone here.

[hgaiser]

I've only seen this effort since today but happy it happened! Should the README.md of this repository be updated to notify readers of the migration to the Microsoft repository? I understand it won't replace this repository (yet), but helps to put more eyes on that implementation.

[mcf06]

After looking through various evolutions of these bindings, I'd like to voice my support for @seddonm1 's approach. For the common case of repeated / frequent model inference runs, the simplified API is faster, and IMO also easier to use.

In fact, I've taken the liberty of merging @seddonm1 's code with some of the nice improvements from @boydjohnson et al. and further separated the ndarray dependency by providing an OrtNdArray container (holding the ndarray) that can generate an OrtValue for session.run() or for the io_binding.

I'd love to get others impressions, particularly with an eye toward inclusion in microsoft/onnxruntime: https://github.com/mcf06/onnxruntime (branch: public-rust)

[seddonm1]

@boydjohnson What are your thoughts here given @mcf06 comments?

For what its worth, we have run my bindings for many millions of runs (using io_binding), have applied valgrind against them and found this major issue in onnxruntime core with them https://github.com/microsoft/onnxruntime/issues/14484 (there is also going to be a 1.14.2 as a result of this regression).

[boydjohnson]

Really great, @seddonm1. I am running into a time crunch currently where I'm finding little time to do open-source contributions. So my understanding is that they are shooting for releasing the Rust bindings to crates.io with the next minor version release. It would be nice to get the the io_ring work included in microsoft/onnxruntime. I could reach out to some internal Microsoft folks about this?

[seddonm1]

Thanks @boydjohnson

I know how much work you have put in to getting the bindings to this point so I don't want to discourage you. I hope we can all work together to make them a bit more aligned with the C API as I went through the process of understanding the decisions that went into the C API whilst making my modifications.

I think it would be best to hold off on the crates.io process until this refactor is done. We can do a WIP PR against the onnxruntime repo?

@mcf06 what level of time involvement do you have?

[mcf06]

| I think it would be best to hold off on the crates.io process until this refactor is done.

+1

| what level of time involvement do you have?

I do have some time for this effort. We're also using rust bindings in some in-house code (for audio, on CPU at the moment), so I'm motivated to get a best-of-breed solution into ms/onnxruntime. Looking through the various forks and evolutions of the code, it's been really interesting to see clever little innovations all over the place. I can put in a draft PR to serve as a starting point.

[seddonm1]

@mcf06 I think your draft PR is a good idea.

I have had a quick skim of your bindings and am not convinced that ortndarray is required given that the value type has a try_from_array which implements the same code. I'm glad you also forked recently as I found a few leaks with valgrind. I would very much like to clean up the comments as I did not spend a huge amount of time on them.

If you can do the initial PR then everyone here who is interested can be involved in the review?

[alfredodeza]

I think that a PR would be excellent - it is otherwise very difficult to provide feedback on changes.

If I may add a suggestion... please include examples as part of the PR (see related open issue here https://github.com/microsoft/onnxruntime/issues/14852 )

[mcf06]

| If you can do the initial PR then everyone here who is interested can be involved in the review?

Will do. I'll also write up a brief summary of changes for people to dig into.

[mcf06]

Sorry for the delay... I wanted to give some more thought to safety concerns around OrtValue. I've made some updates, and, now that I've had a chance to use this in some in-house code, I think it's finally worth the PR: https://github.com/microsoft/onnxruntime/pull/15655

@seddonm1: The tricky part about OrtValue::try_from_array() is that the OrtValue uses the Array's data memory, but the Array may be dropped. For example, this code compiles but may crash:

let test_array = Array2::<f32>::zeros((1,2));
let ort_value = OrtValue::try_from_array(&onnx_session, &test_array).unwrap();
drop(test_array);  // ort_value's data memory is deallocated
let bad_a = ort_value.array_view::<f32>().unwrap();
dbg!(bad_a);

The current PR includes an NdArrayOrtValue, little more than the try_from_array() call, that guarantees the lifetime of the Array relative to the &OrtValue it provides.

[seddonm1]

@mcf06 sorry for the delay. I will try to review your code next week. I think we may need to re-add the String array type back that I removed.

[Chiichen]

@seddonm1 @mcf06 Friendly Ping, I'm quite curious about the current status of pushing the onnxruntime-rs into microsoft/onnxruntime. I noticed microsoft/onnxruntime #15655. It looks like it has not been updated further and remains in draft state.

[seddonm1]

I think this is the current method: https://github.com/pykeio/ort