search

intel / openvino-rs

Rust bindings for OpenVINO™
Apache License 2.0
77 stars 21 forks source link

[Question] Why yank openvino 0.7.0/1? #133

Open oleflb opened 2 weeks ago

oleflb commented 2 weeks ago

What is the reason 0.7.0 and 0.7.1 are yanked on crates.io? I could not find any information on that. This forces me to update the openvino dependency, which currently is hard for me since it doesnt seem to be possible anymore for tensors to be created from slices without copying the data.

abrown commented 2 weeks ago

I think you're looking for https://github.com/intel/openvino-rs/pull/125, in which @rahulchaphalkar removed the function you need after running across a crash. I tried to add some more detail to the commit message (https://github.com/intel/openvino-rs/commit/b6eacef9b5ab2cc964abbf02be3c74b0d7a948b1) but it boils down to "unsafety across the FFI." Looking at the download numbers on crates.io for those versions (very low still since they are quite new) and the potential for misuse and security vulnerabilities, I decided to just yank the crates. We published 0.7.2 without that function.

But you probably need that function, so let's discuss a way to add something similar back in, but safely. We talked about potentially using Rust lifetimes and PhantomData to ensure that users keep the underlying tensor data alive as long as Tensor, but this caused extra lifetime complexity elsewhere. It still might be a valid approach, though. Are you interested in working on this?

oleflb commented 1 week ago

Yes, this function is what we used to avoid allocations :D. I think adding a lifetime to Tensor is perfectly valid, however users probably still want owned versions of Tensor. In ndarray they do something similar by having a generic Container attribute for ArrayBase which allows both for owned arrays and array views. Do you think something like this is suitable for the Tensor type here as well?

abrown commented 1 week ago

Yeah, that's a good idea too: kind of like Cow, we could have owned and borrowed variants. I think I like that better than the PhantomData idea because it's a bit more explicit about what's going on. We may still need to propagate lifetimes in a bunch of places; specifically, Core::read_model_from_buffer accepts a Tensor and holds on to it for some indeterminate time (maybe until Core::compile_model, IIRC?).