search

intel / openvino-rs

Rust bindings for OpenVINO™
Apache License 2.0
80 stars 23 forks source link

Support loading non-string models from memory #107

Closed pnehrer closed 3 months ago

pnehrer commented 3 months ago

We've run into an issue with Core::read_model_from_buffer -- the wrapper assumes that the in-memory model is a valid string. However, non-native models, such as ONNX, aren't valid strings. Attempting to load an ONNX model results in an error creating a CString due to non-terminating null characters.

We propose to change the method interface to support arbitrary byte models rather than just strings.

Furthermore, we propose to make the weight tensor an optional argument, as it doesn't apply to non-native models.

abrown commented 3 months ago

Oh, and one other thing: the CI failures are not due to this change. I think you will have to rebase to see CI turn green.

pnehrer commented 3 months ago

Oh, and one other thing: the CI failures are not due to this change. I think you will have to rebase to see CI turn green.

I think this is a function that isn't in 2022.3.0, which is what seems to be failing to link. Some more recent version jobs do succeed before this one cancels the rest.

pnehrer commented 3 months ago

Oh, and one other thing: the CI failures are not due to this change. I think you will have to rebase to see CI turn green.

I think this is a function that isn't in 2022.3.0, which is what seems to be failing to link. Some more recent version jobs do succeed before this one cancels the rest.

... and as a result, I can remove the unit test if that helps.

abrown commented 3 months ago

Ah, I see: @rahulchaphalkar and I noticed a failure in the Windows builds due to the missing function and I chalked it up to some difference between the Windows and Linux releases. But actually we were never exercising the function so it would have failed if someone actually tried it. Given that, I propose we keep the test and remove the following step in CI since we just can't support those versions with this function:

https://github.com/intel/openvino-rs/blob/3a404eef023e854b592528e98e6b3b759735bff3/.github/workflows/main.yml#L28-L32

With that out of the way, I think this should be good to go 🤞

pnehrer commented 3 months ago

Would you still want to keep the "oldest supported version" with apt and just pull it up to 2023.2.0, instead of removing that combo altogether?

abrown commented 3 months ago

Would you still want to keep the "oldest supported version" with apt and just pull it up to 2023.2.0, instead of removing that combo altogether?

Well, we have 2023.2.0 up above in the non-APT build so I'm not too worried about it.