search

intel / openvino-rs

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

Update Openvino API to 2.0 #91

Closed rahulchaphalkar closed 4 months ago

rahulchaphalkar commented 5 months ago

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

BTOdell commented 4 months ago

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:

  1. "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
  2. There are a couple instances where the struct name is included in the struct's function names and results in overly verbose naming.
  3. Functions that take file paths as inputs should use &Path instead of &str.
  4. 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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

@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 - https://github.com/intel/openvino-rs/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. https://github.com/intel/openvino-rs/issues/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 https://github.com/intel/openvino-rs/issues/95 to keep track of the changes that you suggested in your comment above.