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

Allow safe borrowing of Tensor #133

Open
oleflb opened this issue Jun 29, 2024 · 3 comments
Open

Allow safe borrowing of Tensor #133

oleflb opened this issue Jun 29, 2024 · 3 comments

Comments

@oleflb
Copy link

oleflb commented Jun 29, 2024

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
Copy link
Contributor

abrown commented Jul 3, 2024

I think you're looking for #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 (b6eacef) 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
Copy link
Author

oleflb commented Jul 3, 2024

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
Copy link
Contributor

abrown commented Jul 3, 2024

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?).

@abrown abrown changed the title [Question] Why yank openvino 0.7.0/1? Allow safe borrowing of Tensor Oct 3, 2024
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

No branches or pull requests

2 participants