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

Tensor::get_byte_size returns incorrect result with v2024.4.0 #143

Closed
abrown opened this issue Oct 3, 2024 · 2 comments · Fixed by #144
Closed

Tensor::get_byte_size returns incorrect result with v2024.4.0 #143

abrown opened this issue Oct 3, 2024 · 2 comments · Fixed by #144

Comments

@abrown
Copy link
Contributor

abrown commented Oct 3, 2024

TLDR: this issue describes how an breaking change in the upstream C bindings results in incorrect behavior in these Rust bindings; the solution is to use a newer version of OpenVINO (v2024.2 and up) along with a recent version of these bindings (v0.8.0 and up).


#142 adds an easier way to update the bindings to use the latest OpenVINO C headers; as I used it I discovered the Tensor::get_byte_size is returning a different result than previously. I ran:

$ cargo xtask update 2024.4.0
$ cargo xtask codegen
$ cargo test

And the memory_safety test promptly failed with:

thread 'memory_safety' panicked at crates/openvino/tests/memory-safety.rs:26:40:
source slice length (13956476) does not match destination slice length (111651808)

Interestingly enough, 111651808 / 13956476 = 8 so it could be that some internal data type was unwittingly changed upstream. It's notable that the weights buffer we pass in contains 13956476 bytes and has had that size since the mobilenet test fixture was added. And I am running the test with the OpenVINO 2024.1.0 library installed; the only thing that changes here is the OpenVINO C header.

@abrown
Copy link
Contributor Author

abrown commented Oct 4, 2024

I investigated this and the cause is the upstream commit at openvinotoolkit/openvino@0d95325972f (PR: openvinotoolkit/openvino#24608). What happened is that three new enum variants (U2, U3, U6) were inserted prior to the U8 variant, pushing its discriminant value (14) to that of U64 (17). The commit says it is part of releases v2024.2.0 and following, so if someone used the library at v2024.1.0 and below (like I did above) with bindings for v2024.2.0 and above (which is what my update did) they would see this issue.

This raises a versioning problem: a minor version bump should be backwards compatible (like adding enum variants to the end of the list!) but this change was not. But the v<year>.<version>.<patch> scheme that OpenVINO seems to be using is probably not intended for semver compatibility anyways. So, for this crate, let's bump our major version (which still lives in the minor version placeholder... 1.0 in 2025?) and document that the ElementType enum will break on older versions of OpenVINO. For the memory-safety.rs test, let's see if we can't ignore it if we check that the version is older than v2024.2.0.

@abrown
Copy link
Contributor Author

abrown commented Oct 7, 2024

@rahulchaphalkar, thinking about this more, I'm not as worried about a security issue here: there are three parts to this puzzle — the user, the bindings, and the OpenVINO library. If we're using pre-2024.2 bindings with a post-2024.2 library (or vice versa) there is a mismatch between what the user asks for (e.g., U8) and what the library sees (e.g., U64). I was worried that there would be a mismatch between the bindings and the library, but no, they will always be in sync; if they weren't, we could get the case where Tensor::get_raw_data_mut could allow access to more memory than it should. But this won't happen because Tensor always asks OpenVINO what size it should be: the call to ov_tensor_get_byte_size always returns the correct OpenVINO-side size, regardless of if the element type is confused in the bindings. Now, the user may still be very confused, but I think we can rule out a memory safety issue.

abrown added a commit to abrown/openvino-rs that referenced this issue Oct 10, 2024
In intel#143, we discovered that upstream OpenVINO had changed the
`ov_element_type_e` enum in a backwards-incompatible way. This means
that a mismatch between the Rust bindings (based on the C headers) and
the shared library will result in tensors being created with the wrong
type (e.g., `U64` instead of `U8`). To avoid this situation, this change
reads the OpenVINO library version at runtime (e.g., every time a `Core`
is created) and fails with an error if the version is older than 2024.2,
when the breaking change was introduced.

The effect of merging this change would be that newer bindings will fail
when used with older libraries, which could be problematic for users.
Users could always use an older version of the library (e.g., `v0.7.*`)
instead. And the alternative, letting users create tensors of the wrong
type, seems like it would open up the door to a slew of reported issues.
abrown added a commit that referenced this issue Oct 14, 2024
* Check version; fail if pre-2024.2

In #143, we discovered that upstream OpenVINO had changed the
`ov_element_type_e` enum in a backwards-incompatible way. This means
that a mismatch between the Rust bindings (based on the C headers) and
the shared library will result in tensors being created with the wrong
type (e.g., `U64` instead of `U8`). To avoid this situation, this change
reads the OpenVINO library version at runtime (e.g., every time a `Core`
is created) and fails with an error if the version is older than 2024.2,
when the breaking change was introduced.

The effect of merging this change would be that newer bindings will fail
when used with older libraries, which could be problematic for users.
Users could always use an older version of the library (e.g., `v0.7.*`)
instead. And the alternative, letting users create tensors of the wrong
type, seems like it would open up the door to a slew of reported issues.

* Add documentation to helper functions

* Update CI versions: library + OS

This updates CI to test only the latest three OpenVINO versions, since
older versions will no longer be accessible due to the new "breaking
change version check." It also updates the OS version of the GitHub
runner for good measure.

* ci: specify ubuntu-24.04

* ci: roll back OS version

OpenVINO has not published packages for `ubuntu-24.04`.
abrown added a commit to abrown/wasmtime that referenced this issue Oct 16, 2024
Recent changes to OpenVINO's C bindings introduced breaking changes that
prevent use of older OpenVINO libraries with newer bindings (see
[openvino-rs#143] if you're interested in the details). This updates
both the bindings and the version of OpenVINO used to test against.

Fixes bytecodealliance#9379.

[openvino-rs#143]: intel/openvino-rs#143
github-merge-queue bot pushed a commit to bytecodealliance/wasmtime that referenced this issue Oct 16, 2024
* wasi-nn: update OpenVINO bindings

Recent changes to OpenVINO's C bindings introduced breaking changes that
prevent use of older OpenVINO libraries with newer bindings (see
[openvino-rs#143] if you're interested in the details). This updates
both the bindings and the version of OpenVINO used to test against.

Fixes #9379.

[openvino-rs#143]: intel/openvino-rs#143

* vet: certify openvino version updates

All of these updates include code I've reviewed.

* ci: update `install-openvino-action`

prtest:full
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

Successfully merging a pull request may close this issue.

1 participant