Check version; fail if pre-2024.2 #149
Conversation
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.
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.
OpenVINO has not published packages for `ubuntu-24.04`.
|
cc: @jlb6740, @rahulchaphalkar, @mingqiusun: take a look at the effect this PR will have. |
|
@abrown In a previous comment on the issue, you suggested we can update the major version of openvino-rs (to |
|
Yes: if merged, the
I'm not sure I understand what you mean by this. |
If openvino-rs updates major version to |
|
A breaking change is typically understood as an API change of some kind — something that will cause a compile error. This is more insidious: everything looks fine on the outside but then it appears that the bindings end up doing the wrong thing. E.g., a user creates a There are other options here: we could print a warning or add the ability to turn this off with an environment variable. I would prefer to do something, though: doing nothing (except documenting the breaking change) could lead to bug reports. The approach this PR takes is "better safe than sorry." |
|
Ok, that makes sense. My concern was that if/when there are more silent breaking changes like these, we would need to keep adding checks. But I guess the answer to that would be we worry only about reasonably older versions for openvino. |
|
Looks like #150 is the first instance of this confusion. |
In #143, we discovered that upstream OpenVINO had changed the
ov_element_type_eenum 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.,U64instead ofU8). To avoid this situation, this change reads the OpenVINO library version at runtime (e.g., every time aCoreis 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 is 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 bug reports. So this version check seems like an acceptable compromise to handle this upstream breaking change.