Update Openvino API to 2.0 #91

rahulchaphalkar · 2024-03-29T03:42:46Z

Updated to the new API 2.0

Some notes about the tests -
classify-mobilenet, classify-alexnet, and classify-inception required image tensor layout to be NHWC.
detect-inception required slight modifications in the expected results noted in the comments, like the actual results starting from 1st element instead of 0th.

@abrown

openvino-sys crate builds and tests pass with api 2.0 with some caveats update openvino api to 2.0, classify-mobilenet working test remove comments and outdated files cleanup, change methods in impl based on parameters alexnet test fix samples/tests, start tests for tensor add tests tests continue fix version test, add tests to lib update and add docs Add Result<>, fix tests cargo fmt fix warnings in request.rs of unused imports delete vscode file change to accept variadic args with '...' link macro format format link macro

abrown

I think this is almost there — thanks for all this work! Most of my comments are minor nits and easily fixable except for two bigger problems:

our handling of the instance: *mut ... fields needs to be very safe: there's a dangling null pointer in there somewhere and I would prefer if we just made all of those fields private to avoid any munging with the pointer. For the structs that are built somewhere else, we can always add a private fn new(instance: *mut ...) -> Self { Self { instance } }.
the pre- and post-processing is just... confusing: maybe just do the best you can with doc tests and comments and we can refactor this later. I see that the C++ has what is called a PrePostProcessor which operates on a Model and then eventually one build()s the new thing. The C API is a bit more confusing so that might be what is going on here.

abrown · 2024-03-29T11:36:18Z

crates/openvino-sys/src/lib.rs

-//! openvino_sys::library::load().expect("to have an OpenVINO library available");
-//! let version = unsafe { CStr::from_ptr(openvino_sys::ie_c_api_version().api_version) };
-//! assert!(version.to_string_lossy().starts_with("2"));
-//! ```


Let's keep any doc examples.

I still don't see this as added back in in the final PR state.

crates/openvino-sys/upstream

crates/openvino/src/core.rs

crates/xtask/src/codegen.rs

crates/openvino/tests/detect-inception.rs

BTOdell · 2024-04-03T17:45:27Z

Really nice to see this work getting done. Thank you!

I'm in the process of porting my company's Python app to Rust, and these API 2.0 bindings are required. I was about to start this conversion process myself since I knew from a couple months ago that these Rust bindings were outdated for the 2024.0 release of OpenVINO.

I've been reviewing this PR and I have some feedback:

"Getter" functions should not be prefixed with "get_". This is a standard Rust naming convention: https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter
There are a couple instances where the struct name is included in the struct's function names and results in overly verbose naming.
Functions that take file paths as inputs should use &Path instead of &str.
There are some places where enums (instead of plain strings) could be used to make the Rust API more discoverable and less error prone. For example, pre-defined Layout variants.

I will continue reviewing this PR and submit any more feedback I have.

Thanks again! 🎉

abrown · 2024-04-03T18:20:25Z

@BTOdell, I think I'll take a pass or two after this merges to move this towards "ergonomic Rust." I'll definitely keep your suggestions in mind as I do that (and you're welcome to contribute PRs!). With this PR, I'm more concerned that the basic functionality and correctness remains in place; we can always smooth things out in follow-on PRs before the next release.

BTOdell · 2024-04-03T20:14:56Z

I'll be sure to open a PR with the suggestions. I based my fork from Rahul's fork and going to work from there.

Currently trying to get this crate building on my Windows machine, but having linking errors:
LINK : fatal error LNK1181: cannot open input file 'openvino.lib'
openvino-finder successfully finds the openvino.dll and openvino_c.dll; not sure why it's trying to link against a .lib.

rahulchaphalkar · 2024-04-18T06:44:53Z

@abrown Thanks for the detailed review and suggestions, I have addressed your feedback (kept a few conversations unresolved in case you wanted to go through my comments).
I have also opened 2 new issues - #93 to separately keep track on adding a new test similar to detect-inception, since detect-inception is outdated and based on a demo which no longer exists.
#94 for adding examples and docs specifically for PreProcessing functionality

@BTOdell thanks for reviewing the PR as well as providing good feedback! I have created a separate issue #95 to keep track of the changes that you suggested in your comment above.

abrown

I think the only big thing to resolve is the recent addition of anyhow; let's talk offline about that. For efficiency's sake, I'll just start committing some of the small suggestions from this review.

crates/openvino/src/model.rs

abrown · 2024-04-18T08:18:03Z

crates/openvino/Cargo.toml

@@ -16,6 +16,7 @@ exclude = ["/tests"]
 openvino-sys = { path = "../openvino-sys", version = "0.6.0" }
 openvino-finder = { path = "../openvino-finder", version = "0.6.0" }
 thiserror = "1.0"
+anyhow = "1.0"


I'm not sure I understand why we would need to import this: just using the regular Result with the appropriate error type from this crate should be good enough.

Oh, I see, you're just using anyhow for easier testing... in that case I'll just move it to [dev-dependencies] so that it is available during testing but not required by users of the crate.

crates/openvino/src/layout.rs

crates/openvino/src/lib.rs

crates/openvino/src/model.rs

crates/openvino/src/port.rs

abrown · 2024-04-18T08:26:09Z

crates/openvino/src/port.rs

+    }
+
+    ///Create a new [`Port`] from [`ov_output_const_port_t`].
+    pub(crate) fn new(instance: *mut ov_output_const_port_t) -> Result<Self> {


No need to return a Result when there is no chance for an error.

Suggested change

pub(crate) fn new(instance: *mut ov_output_const_port_t) -> Result<Self> {

pub(crate) fn new(instance: *mut ov_output_const_port_t) -> Self {

crates/openvino/src/shape.rs

abrown · 2024-04-18T08:41:28Z

crates/openvino-sys/src/lib.rs

-//! openvino_sys::library::load().expect("to have an OpenVINO library available");
-//! let version = unsafe { CStr::from_ptr(openvino_sys::ie_c_api_version().api_version) };
-//! assert!(version.to_string_lossy().starts_with("2"));
-//! ```


I still don't see this as added back in in the final PR state.

crates/openvino/Cargo.toml

abrown · 2024-04-18T08:55:55Z

crates/openvino/src/shape.rs

+    /// Create a new shape object from ov_shape_t.
+    pub(crate) fn new_from_instance(instance: ov_shape_t) -> Result<Self> {
+        Ok(Self { instance })
+    }


Suggested change

/// Create a new shape object from ov_shape_t.

pub(crate) fn new_from_instance(instance: ov_shape_t) -> Result<Self> {

Ok(Self { instance })

}

/// Create a new shape object from `ov_shape_t`.

pub(crate) fn new_from_instance(instance: ov_shape_t) -> Self {

Self { instance }

}

abrown · 2024-04-18T08:56:38Z

crates/openvino/src/shape.rs

+    /// Returns the rank of the shape.
+    pub fn get_rank(&self) -> Result<i64> {
+        Ok(self.instance.rank)
+    }


Here and many other places we return a Result unnecessarily.

Suggested change

/// Returns the rank of the shape.

pub fn get_rank(&self) -> Result<i64> {

Ok(self.instance.rank)

}

/// Returns the rank of the shape.

pub fn get_rank(&self) -> i64 {

self.instance.rank

}

abrown

I fixed up some of the small stuff but can't push to this branch so I can't get rid of Model::new and unnecessary returns of Result. Otherwise looks great!

abrown

Thanks for all the work on this!

rahulchaphalkar added 2 commits March 28, 2024 16:00

formatting dynamic.rs

7362b75

abrown reviewed Mar 29, 2024

View reviewed changes

rahulchaphalkar added 5 commits March 29, 2024 08:23

fix runtime linking

f3615d3

clippy warnings fix

1ad9f22

change xtask/codegen to relative path

6dd9f4f

cargo fmt

8cb7fe2

fix bindgen wchar_t issue

08fdbda

BTOdell mentioned this pull request Apr 4, 2024

API 2.0 and 2024.0 compatibility #92

Closed

6 tasks

rahulchaphalkar added 7 commits April 8, 2024 15:29

make struct members private

5c0ac40

add doc tests for lib.rs

26bc1b0

cleaned up new core with config

0d33135

Rename symbol(s) from Preprocess* to PreProcess*

c86d0b8

clean up tests

723be06

remove unused preprocessor method

267e509

remove comment from codegen.rs

4e2ba11

rahulchaphalkar mentioned this pull request Apr 17, 2024

detect-inception test uses outdated model, needs update #93

Open

rahulchaphalkar added 2 commits April 17, 2024 16:44

remove outdated test detect-inception

07fd835

clean up docs

31101bf

This was referenced Apr 18, 2024

Add examples and documentation for PreProcessing / PostProcessing #94

Open

Make changes to adhere to Rust conventions #95

Closed

abrown reviewed Apr 18, 2024

View reviewed changes

abrown added 4 commits April 18, 2024 10:43

Avoid exposing openvino-sys types

2716c76

Remove dead code

b629660

Avoid exposing openvino-sys types

4b4d0f8

Add missing space

d42bf13

abrown added 5 commits April 18, 2024 10:44

Add missing space

e562b18

Avoid exposing openvino-sys types

f70a45c

Add missing space

29e1a37

Avoid exposing openvino-sys types

584e7fc

Add missing space

daec6b9

abrown reviewed Apr 18, 2024

View reviewed changes

crates/openvino/Cargo.toml Outdated Show resolved Hide resolved

Make anyhow a dev dependency

c3d14f9

abrown reviewed Apr 18, 2024

View reviewed changes

abrown approved these changes Apr 18, 2024

View reviewed changes

rahulchaphalkar added 3 commits April 23, 2024 12:58

add doc test in openvino-sys libs.rs

17c2aea

remove Result where not needed

cdd7f46

change comments and formatting

75b7150

abrown approved these changes Apr 24, 2024

View reviewed changes

abrown merged commit 7efb30d into intel:main Apr 24, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Openvino API to 2.0 #91

Update Openvino API to 2.0 #91

rahulchaphalkar commented Mar 29, 2024

abrown left a comment

abrown Mar 29, 2024

rahulchaphalkar Apr 9, 2024

abrown Apr 18, 2024

BTOdell commented Apr 3, 2024 •

edited

Loading

abrown commented Apr 3, 2024

BTOdell commented Apr 3, 2024 •

edited

Loading

rahulchaphalkar commented Apr 18, 2024

abrown left a comment •

edited

Loading

abrown Apr 18, 2024

abrown Apr 18, 2024

abrown Apr 18, 2024

abrown Apr 18, 2024

abrown Apr 18, 2024

abrown Apr 18, 2024

abrown left a comment

abrown left a comment

	pub(crate) fn new(instance: *mut ov_output_const_port_t) -> Result<Self> {
	pub(crate) fn new(instance: *mut ov_output_const_port_t) -> Self {

Update Openvino API to 2.0 #91

Update Openvino API to 2.0 #91

Conversation

rahulchaphalkar commented Mar 29, 2024

abrown left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BTOdell commented Apr 3, 2024 • edited Loading

abrown commented Apr 3, 2024

BTOdell commented Apr 3, 2024 • edited Loading

rahulchaphalkar commented Apr 18, 2024

abrown left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abrown left a comment

Choose a reason for hiding this comment

abrown left a comment

Choose a reason for hiding this comment

BTOdell commented Apr 3, 2024 •

edited

Loading

BTOdell commented Apr 3, 2024 •

edited

Loading

abrown left a comment •

edited

Loading