[Bug 2251785] New: Review Request: openvino - Open Visual Inference And Optimization toolkit for AI inference
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
Bug ID: 2251785 Summary: Review Request: openvino - Open Visual Inference And Optimization toolkit for AI inference Product: Fedora Version: rawhide OS: Linux Status: NEW Component: Package Review Severity: medium Assignee: nobody@fedoraproject.org Reporter: aekoroglu@linux.intel.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
SPEC Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-r... SRPM Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-r...
Description: OpenVINO is an open-source toolkit for optimizing and deploying AI inference. It can be used to develop applications and solutions based on deep learning tasks, such as: emulation of human vision, automatic speech recognition, natural language processing, recommendation systems, etc.
Reproducible: Always
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
--- Comment #1 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/6700410 (failed)
Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
Please make sure the package builds successfully at least for Fedora Rawhide.
- If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field
--- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service
If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
Tom Rix trix@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |1011110 (ML-SIG) Status|NEW |ASSIGNED Doc Type|--- |If docs needed, set a value Flags| |fedora-review? CC| |trix@redhat.com Assignee|nobody@fedoraproject.org |trix@redhat.com
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1011110 [Bug 1011110] Machine Learning SIG - review tracker
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
--- Comment #2 from Tom Rix trix@redhat.com --- On first look, the big problem is the third_party/ dir in the tarball. These should should be their own packages. If they are not needed, then the third_party dir should be rm-ed in %prep
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
Tom Rix trix@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends On| |2254599
--- Comment #3 from Tom Rix trix@redhat.com --- What I mean is these third party packages need to be their own packages. Please see https://bugzilla.redhat.com/show_bug.cgi?id=2254599 this removes the openvino/thirdparty/json dependency. And if you have the time, please review.
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=2254599 [Bug 2254599] Review Request: nlohmann_json - JSON for Modern C++
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
--- Comment #4 from Ali Erdinc Koroglu aekoroglu@linux.intel.com --- Hello Tom, I disabled them in the patch file, but do you want me to delete them instead?
--- a/CMakeLists.txt 2023-11-22 08:22:33.730665512 +0200 +++ b/CMakeLists.txt 2023-11-24 00:34:16.675363131 +0200 @@ -38,7 +38,7 @@ include(cmake/features.cmake)
# These options are shared with 3rdparty plugins by means of developer package -include(cmake/dependencies.cmake) +# include(cmake/dependencies.cmake)
if(ENABLE_COVERAGE) include(cmake/coverage.cmake) @@ -133,7 +133,7 @@ include(cmake/test_model_zoo.cmake) endif()
-include(thirdparty/dependencies.cmake) +# include(thirdparty/dependencies.cmake) add_subdirectory(src)
if(ENABLE_SAMPLES OR ENABLE_TESTS) @@ -146,10 +146,10 @@ endif()
include(cmake/extra_modules.cmake) -add_subdirectory(docs) -add_subdirectory(tools) -add_subdirectory(scripts) -add_subdirectory(licensing) +# add_subdirectory(docs) +# add_subdirectory(tools) +# add_subdirectory(scripts) +# add_subdirectory(licensing)
if(ENABLE_TESTS) # layers and other more high-level / e2e tests --- a/src/CMakeLists.txt 2023-11-22 08:23:48.722438766 +0200 +++ b/src/CMakeLists.txt 2023-11-22 08:25:27.304120882 +0200 @@ -8,7 +8,9 @@ ov_add_compiler_flags(-Wmissing-declarations) endif()
-include(cmake/install_tbb.cmake) +# dont install_tbb +# include(cmake/install_tbb.cmake) +include(cmake/ov_parallel.cmake)
# CC library should be registered before other cc targets add_subdirectory(common) --- a/src/plugins/intel_cpu/thirdparty/CMakeLists.txt 2023-11-22 08:35:23.572344257 +0200 +++ b/src/plugins/intel_cpu/thirdparty/CMakeLists.txt 2023-11-22 08:35:40.634293194 +0200 @@ -115,7 +115,7 @@ list(APPEND CMAKE_MODULE_PATH "${intel_cpu_thirdparty_SOURCE_DIR}") endif()
- add_subdirectory(onednn EXCLUDE_FROM_ALL) + # add_subdirectory(onednn EXCLUDE_FROM_ALL)
# install static libraries ov_install_static_lib(dnnl ${OV_CPACK_COMP_CORE})
https://bugzilla.redhat.com/show_bug.cgi?id=2251785 Bug 2251785 depends on bug 2254599, which changed state.
Bug 2254599 Summary: Review Request: nlohmann_json - JSON for Modern C++ https://bugzilla.redhat.com/show_bug.cgi?id=2254599
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |CLOSED Resolution|--- |NOTABUG
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
--- Comment #5 from Tom Rix trix@redhat.com --- Unwinding third_party can be complicated. To prove you do not needed it, remove it in prep Like https://src.fedoraproject.org/rpms/python-torch/blob/rawhide/f/python-torch....
I was really only looking at the toplevel thirdparty/ , but it looks like from above that they are also in the plugins.
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
--- Comment #6 from Ali Erdinc Koroglu aekoroglu@linux.intel.com --- Hello Tom, thank you for the comments.
SPEC Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-r... SRPM Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-r...
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
Fedora Review Service fedora-review-bot@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- URL| |https://github.com/openvino | |toolkit/openvino
--- Comment #7 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/6766923 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
Please take a look if any issues were found.
--- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service
If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
--- Comment #8 from Tom Rix trix@redhat.com --- At least the licensecheck seems to be missing from the above auto review. Spec looks generally fine.
This line should not be needed. -DCMAKE_CXX_FLAGS="%{optflags} -Wformat -Wformat-security" \
There is also at least third party for src/bindings/python/thirdparty src/plugins/intel_gpu/thirdparty
That needs to be removed.
I think the two small plugin subpackages should be rolled into the the main package.
There should be a way, even a manual way to test.
The samples/ would be good to include into the devel package.
Can python be enabled ? python is widely used in AI so this would be good.
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
--- Comment #9 from Ali Erdinc Koroglu aekoroglu@linux.intel.com --- Hello Tom, it's been a while sorry about it. Python API, PyTorch + IR frontend supports are enabled and tested
SPEC Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-r... SRPM Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-r...
Known issues - onednn (opevino forked version) and intel-mlas bundled to compile intel_cpu_plugin - onnx disabled (requested version of Google Protobuf is 3.20.3) - aarch64 disabled (https://github.com/ARM-software/ComputeLibrary not packed yet) - 2024.1.0 version has some more thirdparty deps.
PS: I added CXX_FLAGS="%{optflags} -Wformat -Wformat-security" to fix src/bindings/c/src/ov_dimension.cpp compilation error cc1plus: error: ‘-Wformat-security’ ignored without ‘-Wformat’ [-Werror=format-security]
Manual test results: [aekoroglu@test samples]$ python sync_benchmark.py /home/aekoroglu/models/public/googlenet-v1/FP32/googlenet-v1.xml [ INFO ] OpenVINO: [ INFO ] Build ................................. 2024.0.0-000-- [ INFO ] Count: 1856 iterations [ INFO ] Duration: 10000.79 ms [ INFO ] Latency: [ INFO ] Median: 4.91 ms [ INFO ] Average: 5.39 ms [ INFO ] Min: 4.46 ms [ INFO ] Max: 15.30 ms [ INFO ] Throughput: 185.59 FPS [aekoroglu@test samples]$ python throughput_benchmark.py /home/aekoroglu/models/public/googlenet-v1/FP32/googlenet-v1.xml [ INFO ] OpenVINO: [ INFO ] Build ................................. 2024.0.0-000-- [ INFO ] Count: 2893 iterations [ INFO ] Duration: 10017.42 ms [ INFO ] Latency: [ INFO ] Median: 13.50 ms [ INFO ] Average: 13.71 ms [ INFO ] Min: 11.57 ms [ INFO ] Max: 28.85 ms [ INFO ] Throughput: 288.80 FPS
https://github.com/openvinotoolkit/openvino/tree/2024.0.0/samples/python/ben... https://github.com/openvinotoolkit/openvino/tree/2024.0.0/samples/python/ben...
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
--- Comment #10 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Created attachment 2031038 --> https://bugzilla.redhat.com/attachment.cgi?id=2031038&action=edit The .spec file difference from Copr build 6766923 to 7400792
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
Fedora Review Service fedora-review-bot@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords| |AutomationTriaged
--- Comment #11 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/7400792 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
Please take a look if any issues were found.
--- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service
If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
--- Comment #12 from Tom Rix trix@redhat.com --- License: Apache-2.0
[ ]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "Apache License 2.0", "*No copyright* Apache License 2.0", "BSD 3-Clause License", "MIT License", "Apache License 2.0 and/or Intel Open Source License", "MIT License and/or zlib License", "Apache License", "*No copyright* MIT License", "*No copyright* Apache License 2.0 and/or MIT License", "GNU General Public License, Version 2", "BSD 3-Clause License and/or GNU General Public License, Version 2", "Apache License 2.0 and/or MIT License", "Apache License 2.0 and/or Boost Software License 1.0", "Apache License 2.0 and/or BSD 3-Clause License", "Apache License 2.0 [generated file]", "Apache License 2.0 and/or Historical Permission Notice and Disclaimer". 5539 files have unknown license. Detailed output of licensecheck in /home/trix/reviews/review-openvino/licensecheck.txt
Your license: file is not complete. Since you have bundled components, go into detail which of these licenses are for the main package and which are for the others.
[ ]: Package requires other packages for directories it uses. Note: No known owner of /usr/lib64/python3.12/site-packages, /usr/lib64/python3.12, /usr/lib64/pkgconfig, /usr/lib64/openvino-2024.0.0, /usr/lib64/cmake
Need to add %dir's here ex/ %dir %_libdir/openvino-2024.0.0
ExclusiveArch: x86_64 %ifarch x86_64 BuildRequires: xbyak-devel %endif
This ifarch check is not needed.
Provides: bundled(onednn)
should have a bundled(onednn) == version similar for mlas
Requires: lib%{name}-ir-frontend = %{version} Requires: lib%{name}-pytorch-frontend = %{version} Recommends: %{name}-auto-plugin = %{version} Recommends: %{name}-auto-batch-plugin = %{version} Recommends: %{name}-hetero-plugin = %{version} Recommends: %{name}-intel-cpu-plugin = %{version}
This is an overly complicated use of subpackages, most (all?) have a single *.so Reduce the subpackages to just the main, python and devel or make a strong case for each.
-DCMAKE_CXX_FLAGS="%{optflags} -Wformat -Wformat-security" \
Should be redundant, why is this needed ?
-python package seems to missing its distinfo/egg dir.
The should be a %check or some option -test subpackage.
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
Ali Erdinc Koroglu aekoroglu@linux.intel.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(trix@redhat.com)
--- Comment #13 from Ali Erdinc Koroglu aekoroglu@linux.intel.com --- @trix@redhat.com : I fixed the licensing issues, converted the sub-plugin packages into one, resolved the Python packaging issue, and added tests.
Ps: CXX_FLAGS="%{optflags} -Wformat -Wformat-security" added to fix src/bindings/c/src/ov_dimension.cpp compilation error
SPEC Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-r... SRPM Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-r...
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
--- Comment #14 from Ali Erdinc Koroglu aekoroglu@linux.intel.com --- Need to add %dir's here ex/ %dir %_libdir/openvino-2024.0.0
Sorry forget this fix
SPEC Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-r... SRPM Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-r...
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
--- Comment #15 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Created attachment 2039354 --> https://bugzilla.redhat.com/attachment.cgi?id=2039354&action=edit The .spec file difference from Copr build 7400792 to 7720382
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
--- Comment #16 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/7720382 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
Please take a look if any issues were found.
--- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service
If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
--- Comment #17 from Adam Jackson ajax@redhat.com --- Why is onednn bundled here? We have a onednn package in Fedora already, does it need to be updated to a newer version to make openvino happy?
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
Adam Jackson ajax@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@fedoraproject.org |ajax@redhat.com
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
--- Comment #19 from Adam Jackson ajax@redhat.com --- The onednn bundling is fine here for now. It would be nice to see a way to try to build this with Fedora's onednn-devel instead, even if it doesn't work to do so yet.
I hadn't caught the ExclusiveArch on the first pass. We should be able to build _some_ CPU support on non-x86, right?
https://bugzilla.redhat.com/show_bug.cgi?id=2251785
--- Comment #20 from Ali Erdinc Koroglu aekoroglu@linux.intel.com --- We'll need to patch several components to demonstrate that Fedora's onednn-devel package will fail during the build. Are you sure you want to proceed with that? Yes, we should be able to build for arm64. But since the https://github.com/ARM-software/ComputeLibrary package is not available in our repository, I have added ExclusiveArch for x86_64.
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com