openvino: add 2023.1.0 version #19681

ilya-lavrenov · 2023-09-08T15:13:24Z

Specify library name and version: openvino/2023.1.0

Closes #7277
Closes openvinotoolkit/openvino#7194

I've read the contributing guidelines.
I've used a recent Conan client version close to the currently deployed.
I've tried at least one configuration locally with the conan-center hook activated.

ghost · 2023-09-08T15:23:24Z

I detected other pull requests that are modifying openvino/all recipe:

openvino: add 2023.1.0 version #19164

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

recipes/openvino/all/conanfile.py

samuel-emrys · 2023-09-12T13:55:39Z

recipes/openvino/all/conandata.yml

+    "mlas":
+      url: "https://github.com/openvinotoolkit/mlas/archive/c7c8a631315000f17c650af34431009d2f22129c.tar.gz"
+      sha256: "7b790dfeef8e1dd612f920c85186c52ad3a3e2245e2a2afd6cc91ce4b1dc64a9"
+    "onednn_gpu":


I could be wrong (maintainer please chime in here!), but my understanding is that dependencies can't be vendored as source code like this for CCI (assuming I understand your intent here correctly). My understanding is that these need to be modelled as requirements, so each would need its own recipe.

Additionally, I'm not sure what the current state of oneDNN is - it looks like it might be open source now? At one point it was closed source or its license was prohibitive, and so it got a special mention in https://github.com/conan-io/conan-center-index/blob/master/docs/faqs.md#how-to-package-libraries-that-depend-on-proprietary-closed-source-libraries

If it is still closed source or proprietary, I don't think it would be packageable on CCI.

My understanding is that these need to be modelled as requirements, so each would need its own recipe.

I agree about oneDNN for GPU and ARM Computer Library, because they are official repositories. But there are not such recipes in Conan for now and it's a simplest way to enable OpenVINO recipe.

In future, once me or someone else will add such recipes, we can re-use it here. But I suppose it's not a blocker to merge current recipe. Let's solve complex things step by step (I already had to add several new recipes to add OpenVINO)

Additionally, I'm not sure what the current state of oneDNN is - it looks like it might be open source now? At one point it was closed source or its license was prohibitive

It's Apache 2.0, just check "about" section of the repo https://github.com/oneapi-src/oneDNN

Some others dependencies (like MLAS) are pure OpenVINO internal code, which is just located in a submodule for OpenVINO repo for better modularity. But we don't have a goal to distribute it as a separate product / component.

In future, once me or someone else will add such recipes, we can re-use it here. But I suppose it's not a blocker to merge current recipe.

It might be a blocker - I've had PRs blocked on this in the past. A maintainer will have to make a decision on that.

It's Apache 2.0, just check "about" section of the repo https://github.com/oneapi-src/oneDNN

Yes, I think what's confused me in the past has been that for some of the one* libraries the interfaces were open source but the implementations weren't. I think this applied to oneMKL but it wasn't clear to me how this applied to other libraries in the family.

Some others dependencies (like MLAS) are pure OpenVINO internal code, which is just located in a submodule for OpenVINO repo for better modularity. But we don't have a goal to distribute it as a separate product / component.

That's good context to have, thanks for clarifying. Does the MLAS dependency you've got share any commonality with https://github.com/microsoft/onnxruntime/tree/main/onnxruntime/core/mlas?

This statement indicates that it might need a package to avoid ODR violations if symbols haven't been deconflicted with upstream. There's no package in CCI for mlas at the moment so I'm not sure how much of a risk this poses, but there is one for onnxruntime, so maybe there's a chance for conflicts?

so maybe there's a chance for conflicts?

Yes, but OpenVINO and ONNX Runtime are both for the same tasks, so customers usually have to select which inference framework to use and go to production only with one of them. So, on practice such conflict is unlikely to happen.
Or there is always an option to build shared libraries - in this case OpenVINO hides all the internals and nobody sees internal symbols.

A maintainer will have to make a decision on that.

I'm ready to improve this recipe over the time, but we need to start with something :)
I hope current recipe is mature enough to be a start point.

I've slotted this topic for our Monday/Tuesday meeting, thanks for your patience :)

Ok, note, that we are OK to improve the recipe over the time.
Our vision is to split the work into parts. We want to add OpenVINO - let's add OpenVINO first, its dependencies later, not in opposite way :)
Currently, those dependencies are supposed to be submodules. So, for 2023.1 it's the only way. But once ARM Compute Library / oneDNN recipe is out, we can add such type of source into our cmake infra and consume packages from Conan cache as well.

I am perfectly fine with the maintainers doing a release of openvino based on 3 different sources repositories, with their own commits, etc. That is not vendoring of dependencies, they are releasing their openvino the way they intend to release it and be used by consumers of it. The fact that there are 3 different repos is an internal implementation detail, and it is not fair or necessary to force them to do separate package releases for every source repo. If they chose to do it in the future, that is good, but shouldn't be mandated from Conan side.

OneDNN coming from the same org, is a bit in the limit of the vendoring definition, but coming from the same org, I am also fine with initially allowing it, and postponing a bit the extraction as independent package, being this the recommended path.

A different story is the arm_compute package, that looks like a vendoring of a third party. Even if the result is fully embedded and isolated, at the very least it could be causing duplication of efforts for someone else that might want to use arm_compute. I'd recommend to extract this one in its own package, as we have been doing in the past, unless there are some big challenges or blockers to make arm_compute its own package, if you need help or can't do it now, we might be able to try to help and contribute it.

(Note: If we have had cases in the past where authors and library maintainers were not allowed to vendor their own organization sources and forced to release independent packages, we should revisit that, learned why it happened, and try to improve and clarify for the future. There are many things in ConanCenter that are not immutable truths, things evolve with the experience, knowledge and changes in the community and ecosystem)

I'd recommend to extract this one in its own package

I agree with such position, but am I right that it's not a blocker to merge?
It can be done later as improvement.

(BTW, I have already made 19 PRs to CCI which got merged as a preparation for OpenVINO recipe, so I want to finally merge our recipe and continue improvements after merge)

ilya-lavrenov · 2023-09-15T08:02:47Z

Hi @SpaceIm @RubenRBS @jcar87,
Please, review the recipe.

It would be great to review and merge the recipe soon, to be aligned with OpenVINO 2023.1.0 official release.

AbrilRBS

Thanks a lot! The recipe looks great as far as I can see - I only have minor comments but nothing blocking :)

I do want to bring the conandata.yml discussion up with the team, let's find the best way forward

recipes/openvino/all/conanfile.py

conan-center-bot · 2023-09-16T02:05:11Z

Conan v1 pipeline ✔️

All green in build 10 (8d19aa2ec53200d2a1db4a199af29707bc0accf0):

openvino/2023.1.0:
All packages built successfully! (All logs)

Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 10 (8d19aa2ec53200d2a1db4a199af29707bc0accf0):

openvino/2023.1.0:
All packages built successfully! (All logs)

ilya-lavrenov · 2023-09-21T17:45:53Z

Hi @RubenRBS @jcar87
I really appreciate your review. I will definitely migrate to Compute Library once its recipe will be a part of CCI.
Could you please let me know whether we need review from someone else? If so, could you please invite those people?

AbrilRBS · 2023-09-21T20:15:32Z

Only 1 team member review + 1 community review is needed for this to get merged, but CCI is currently under heavy load which might delay the automatic merge a bit (No new merges have happened in the last 20h or so) we expect things to move again early morning tomorrow

recipes/openvino/all/conanfile.py

uilianries

The ConanCenterIndex when running Conan 2.x it uses the combination */*:shared=true or */*:shaed=False, which result in invalid configuration for any build, because both Protobuf and OneTBB require a mixed configuration.

Plus, we don't have cross-building profile to check if it works with ARM. So, to prove that's working on Linux-ARM, I cross-built to armv8. It passed without errors. Full log:

openvino_build.log

About Compute Library, there is a separate PR, but will take more time to pass, and will need changes in this recipe. So, I consider this PR as good enough to be approved now and be improved as soon as we have Compute Library.

SpaceIm · 2023-09-28T11:02:51Z

recipes/openvino/all/conanfile.py

+
+    def build_requirements(self):
+        if self._target_arm:
+            self.tool_requires("scons/[>=4.2.0]")


@RubenRBS @uilianries I though that version range was only allowed for cmake, openssl, libcurl and zlib.

Correct, it was accepted by mistake!

You're totally right. We're thinking about allowing it for every tool_require in the future, but we're not there yet, thanks for noticing - This PR got sidetracked and we merged before we should have, and things like this slipped by - Pinging @ilya-lavrenov for when the update to use arm_compute is done, could you also please fix this? Thanks!

I supposed that for tools we can use ranges (and about it here #19681 (comment))

Ok, I will pin tools version as well in my nearest update (I plan to backport some patches from upstream to make recipe more future proof).

fixed in #20245

SpaceIm · 2023-09-28T11:03:34Z

recipes/openvino/all/conanfile.py

+            if self.options.enable_tf_lite_frontend:
+                self.tool_requires("flatbuffers/<host_version>")
+        if not self.options.shared:
+            self.tool_requires("cmake/[>=3.18]")


version ranges must have an upper bound according to conancenter documentation

fixed in #20245

This was referenced Sep 8, 2023

[service] Potentially conflicting recipes #3571

Closed

openvino: add 2023.1.0 version #19164

Closed

prince-chrismc mentioned this pull request Sep 8, 2023

Pull Requests Ready to be Reviewed prince-chrismc/conan-center-index-pending-review#1

Open

conan-center-bot added the Failed label Sep 8, 2023

This comment has been minimized.

Sign in to view

ilya-lavrenov force-pushed the openvino-production branch from 081dff1 to 8916e08 Compare September 8, 2023 21:44

ilya-lavrenov mentioned this pull request Sep 8, 2023

[service] c3i Support for graphs which require a mix of both shared and static dependencies #19688

Open

This comment has been minimized.

Sign in to view

ilya-lavrenov force-pushed the openvino-production branch 3 times, most recently from 501f00b to bcc7ef3 Compare September 9, 2023 09:47

This comment has been minimized.

Sign in to view

ilya-lavrenov force-pushed the openvino-production branch from bcc7ef3 to fdaa530 Compare September 11, 2023 08:42

This comment has been minimized.

Sign in to view

[openvino] new recipe

9ae7fb6

ilya-lavrenov force-pushed the openvino-production branch from fdaa530 to 9ae7fb6 Compare September 11, 2023 10:18

prince-chrismc reviewed Sep 11, 2023

View reviewed changes

recipes/openvino/all/conanfile.py Outdated Show resolved Hide resolved

prince-chrismc reviewed Sep 11, 2023

View reviewed changes

recipes/openvino/all/conanfile.py Outdated Show resolved Hide resolved

prince-chrismc reviewed Sep 11, 2023

View reviewed changes

recipes/openvino/all/conanfile.py Outdated Show resolved Hide resolved

conan-center-bot removed the Failed label Sep 12, 2023

This comment has been minimized.

Sign in to view

CJCombrink approved these changes Sep 12, 2023

View reviewed changes

samuel-emrys reviewed Sep 12, 2023

View reviewed changes

conan-center-bot added the Failed label Sep 15, 2023

This comment has been minimized.

Sign in to view

ilya-lavrenov force-pushed the openvino-production branch from 2ed4610 to 568a422 Compare September 15, 2023 08:07

ilya-lavrenov requested a review from prince-chrismc September 15, 2023 08:08

AbrilRBS reviewed Sep 15, 2023

View reviewed changes

recipes/openvino/all/conanfile.py Outdated Show resolved Hide resolved

recipes/openvino/all/conanfile.py Show resolved Hide resolved

AbrilRBS self-assigned this Sep 15, 2023

addressed review comments

8d19aa2

ilya-lavrenov force-pushed the openvino-production branch from 568a422 to 8d19aa2 Compare September 15, 2023 11:54

conan-center-bot removed the Failed label Sep 16, 2023

uilianries mentioned this pull request Sep 19, 2023

[ComputeLibrary] Add new package for ARM-software/ComputeLibrary #19958

Merged

3 tasks

ilya-lavrenov requested review from AbrilRBS and memsharded September 21, 2023 17:02

AbrilRBS approved these changes Sep 21, 2023

View reviewed changes

AbrilRBS removed request for memsharded and prince-chrismc September 21, 2023 17:25

ilya-lavrenov requested review from memsharded and prince-chrismc September 21, 2023 17:32

uilianries reviewed Sep 25, 2023

View reviewed changes

recipes/openvino/all/conanfile.py Show resolved Hide resolved

uilianries approved these changes Sep 26, 2023

View reviewed changes

AbrilRBS removed request for memsharded and prince-chrismc September 26, 2023 16:12

AbrilRBS approved these changes Sep 26, 2023

View reviewed changes

conan-center-bot merged commit 70157ec into conan-io:master Sep 26, 2023
5 checks passed

ilya-lavrenov deleted the openvino-production branch September 26, 2023 17:48

SpaceIm reviewed Sep 28, 2023

View reviewed changes

ilya-lavrenov mentioned this pull request Oct 1, 2023

[openvino] ported upstream patches to 2023.1.0 #20245

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

openvino: add 2023.1.0 version #19681

openvino: add 2023.1.0 version #19681

ilya-lavrenov commented Sep 8, 2023 •

edited

Loading

ghost commented Sep 8, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

samuel-emrys Sep 12, 2023

ilya-lavrenov Sep 12, 2023 •

edited

Loading

samuel-emrys Sep 12, 2023

ilya-lavrenov Sep 12, 2023 •

edited

Loading

AbrilRBS Sep 15, 2023

ilya-lavrenov Sep 15, 2023

memsharded Sep 15, 2023

ilya-lavrenov Sep 15, 2023 •

edited

Loading

ilya-lavrenov commented Sep 15, 2023

This comment has been minimized.

AbrilRBS left a comment

conan-center-bot commented Sep 16, 2023

ilya-lavrenov commented Sep 21, 2023 •

edited

Loading

AbrilRBS commented Sep 21, 2023

uilianries left a comment

SpaceIm Sep 28, 2023

uilianries Sep 28, 2023

AbrilRBS Sep 28, 2023

ilya-lavrenov Sep 28, 2023

ilya-lavrenov Oct 1, 2023

SpaceIm Sep 28, 2023

ilya-lavrenov Oct 1, 2023

openvino: add 2023.1.0 version #19681

openvino: add 2023.1.0 version #19681

Conversation

ilya-lavrenov commented Sep 8, 2023 • edited Loading

ghost commented Sep 8, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

ilya-lavrenov Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilya-lavrenov Sep 12, 2023 • 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

ilya-lavrenov Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

ilya-lavrenov commented Sep 15, 2023

This comment has been minimized.

AbrilRBS left a comment

Choose a reason for hiding this comment

conan-center-bot commented Sep 16, 2023

Conan v1 pipeline ✔️

Conan v2 pipeline ✔️

ilya-lavrenov commented Sep 21, 2023 • edited Loading

AbrilRBS commented Sep 21, 2023

uilianries 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

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

ilya-lavrenov commented Sep 8, 2023 •

edited

Loading

ilya-lavrenov Sep 12, 2023 •

edited

Loading

ilya-lavrenov Sep 12, 2023 •

edited

Loading

ilya-lavrenov Sep 15, 2023 •

edited

Loading

ilya-lavrenov commented Sep 21, 2023 •

edited

Loading