Fix Blob::tensor_desc() #56
Conversation
|
Let's merge #54 first; I'll take another look after that. |
|
Ok, I will rebase this once #54 is merged. |
chemicstry
force-pushed
the
fix_blob_tensor_desc
branch
from
6cd109b to
eb863b4
Compare
chemicstry
force-pushed
the
fix_blob_tensor_desc
branch
from
eb863b4 to
c32ad11
Compare
|
@chemicstry, I think the failing test is due to not having the OpenVINO library available for whatever call that test is making. For the runtime version of these bindings, something like the following needs to happen to try to dynamically link up with the OpenVINO shared library (see other tests that do this): openvino_sys::library::load().expect("unable to find an OpenVINO shared library"); |
| instance: unsafe { | ||
| tensor_desc_t { | ||
| layout: layout.assume_init(), | ||
| dims: dimensions.assume_init(), |
There was a problem hiding this comment.
I think I'm fine with the MaybeUninit changes but can we add a double-check assertion that dimension.dims is an array of size 8? crates/openvino-sys/src/generated/types.rs has a definition like the following:
pub struct dimensions {
pub ranks: usize,
pub dims: [usize; 8usize],
}And if for some reason some version of OpenVINO changed this, I would want to know with a panic, not some undefined behavior. See TensorDesc::new() for how this looks there.
There was a problem hiding this comment.
I'm not sure I understand what you mean. MaybeUninit type is automatically inferred from dimensions_t (aka dimensions). So in this case it is MaybeUninit<dimensions_t> and if dimensions were changed, the type of MaybeUninit would change accordingly.
Or do you want to ensure that when bindings are regenerated dimensions_t dims remain a size of 8? In that case I believe a compile time assertion in bindings would be better than a runtime check.
There was a problem hiding this comment.
Unfortunately it is probably too complicated to do so in the bindings since they are auto-generated and that still doesn't solve the undefined-ness of potentially storing a wrongly-sized array in that field. It's a bit paranoid of me, I guess (why would OpenVINO itself return the wrong size anyways!?), but I would feel better if there is some check there. I'll merge this as-is and add something in a follow-on commit.
As discussed [here], this change adds a (perhaps overkill) safety check of the size of the dimensions array returned by the OpenVINO library. The added assertion is trying to avoid the improbable case where some future version of the OpenVINO library returns a dimensions array with size different than the one auto-generated in the bindings; see `struct dimensions` in `openvino-sys/src/generated/types.rs`. [here]: intel#56 (comment)
As discussed [here], this change adds a (perhaps overkill) safety check of the size of the dimensions array returned by the OpenVINO library. The added assertion is trying to avoid the improbable case where some future version of the OpenVINO library returns a dimensions array with size different than the one auto-generated in the bindings; see `struct dimensions` in `openvino-sys/src/generated/types.rs`. [here]: #56 (comment)
This depends on #54.
Current implementation of
Blob::tensor_desc()always panics, because it doesn't trim dimensions array and passes the full[u8; 8]intoTensorDesc::new(), which panics withassertion failed: dimensions.len() < 8.This change fixes that by constructing
tensor_desc_tdirectly, since we have all the members of it. I also changed accessor functions to useMaybeUninit, which is faster. There is no need to initialize all of them with default values.