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

API 2.0 and 2024.0 compatibility #92

Closed
wants to merge 43 commits into from
Closed

Conversation

BTOdell
Copy link
Contributor

@BTOdell BTOdell commented Apr 4, 2024

This PR is based on #91 but with additional changes, bug fixes, and feature improvements.

Changes

  • Code style and API design changes according to comments in Update Openvino API to 2.0 #91.
  • Update upstream to version 2024.0.0.
  • Fix dynamic linking on Windows.
  • Fix memory leak with C strings.
  • Create enums for several stringly-typed values (Device, ResizeAlgorithm, Layout?)
  • Implement infer_async function. May need help making sure the unsafe callback code is sound and with the custom Future implementation needed connect the OpenVINO infer request callback to Rust's async syntax.

rahulchaphalkar and others added 16 commits March 28, 2024 16:00
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
@BTOdell BTOdell marked this pull request as ready for review April 4, 2024 06:31
@BTOdell
Copy link
Contributor Author

BTOdell commented Apr 4, 2024

I reverted the result assertion in the detect_inception test to see what the actual values were:

assertion `left == right` failed
  left: [Result(22, 67.0), Result(15, 59.0), Result(1, 1.0), Result(8, 1.0), Result(12, 1.0)]
 right: [Result(15, 59.0), Result(1, 1.0), Result(8, 1.0), Result(12, 1.0), Result(16, 0.9939936)]

The second value in the Result is supposed to be a probability, so I'm not sure why values greater than 1.0 are showing up at all. Maybe the output needs a manual post-processing step to throw away outliers?

@@ -1,4 +1,4 @@
[submodule "crates/upstream"]
path = crates/openvino-sys/upstream
url = https://github.com/openvinotoolkit/openvino
depth = 1
shallow = true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://git-scm.com/docs/gitmodules#Documentation/gitmodules.txt-submoduleltnamegtshallow

It's possible that depth = 1 was working as expected, but it's not a documented property in .gitmodules.

@BTOdell
Copy link
Contributor Author

BTOdell commented Apr 4, 2024

Also, on my AMD laptop, the classify_mobilenet test is also failing with:

Expected probability 0.7134405 but found 0.6203276 (outside of default margin of error)

@abrown
Copy link
Contributor

abrown commented Apr 26, 2024

@BTOdell, #91 has merged; do you want to rebase this on that? Feel free to contact me on Zulip if you want to discuss a strategy for merging this. I would lean toward smaller PRs (except the one-time "jump to v2.0" insanity); how do you feel about breaking this up into a few smaller PRs?

@BTOdell
Copy link
Contributor Author

BTOdell commented Apr 26, 2024

Yes, I plan on rebasing this PR at some point. My fork is working for my use case and I'm very busy at the moment, so it will be a while before I can work on this again.

@rahulchaphalkar
Copy link
Contributor

Hi @BTOdell, we're looking forward to the next release of the openvino crate soon, and we were hoping some of the improvements you've made here would be incorporated in that release. Do you want me to cherry-pick some of the changes if possible from this PR, so that we would have a release candidate ready?

@BTOdell
Copy link
Contributor Author

BTOdell commented May 7, 2024

Yeah, feel free to cherry-pick.

In addition to the items mentioned in the description, I added quite a few functions that were not originally ported in your PR:

  1. Async infer with callback
  2. Property get/set for core and devices
  3. Versions/available_devices query functions
  4. More tensor-accessing functions for InferRequest

I was also having an issue calling variadic C functions due to a bug in the link! macro. I don't know if my "fix" is the proper way to do it, as I'm a noob when it comes to Rust macros. I think I only fixed the runtime linking, but not the dynamic linking (as I don't need it for my use case, but needs to be working for the official crate obviously).

@rahulchaphalkar
Copy link
Contributor

Hi @BTOdell , I've started cherry picking some of the commits, and for few others I need to manually change some code so as to not cause merge conflicts. If you can let me know your email associated with github, I can add you as co-author (cherry-pick did this automatically). This can even be your no-reply* email github provides.

@abrown
Copy link
Contributor

abrown commented Sep 19, 2024

Looking at this now, it seems like @rahulchaphalkar cherry-picked most of this into separate PRs that are now merged and published as the 0.7.* versions. I'm going to close this but feel free to re-open if anything was missed.

@abrown abrown closed this Sep 19, 2024
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 this pull request may close these issues.

3 participants