Bug 2251785
| Summary: | Review Request: openvino - Open Visual Inference And Optimization toolkit for AI inference | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Ali Erdinc Koroglu <aekoroglu> | ||||
| Component: | Package Review | Assignee: | Tom Rix <trix> | ||||
| Status: | ASSIGNED --- | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||
| Severity: | medium | Docs Contact: | |||||
| Priority: | unspecified | ||||||
| Version: | rawhide | CC: | package-review, trix | ||||
| Target Milestone: | --- | Keywords: | AutomationTriaged | ||||
| Target Release: | --- | Flags: | trix:
fedora-review?
|
||||
| Hardware: | Unspecified | ||||||
| OS: | Linux | ||||||
| URL: | https://github.com/openvinotoolkit/openvino | ||||||
| Whiteboard: | |||||||
| Fixed In Version: | Doc Type: | If docs needed, set a value | |||||
| Doc Text: | Story Points: | --- | |||||
| Clone Of: | Environment: | ||||||
| Last Closed: | Type: | --- | |||||
| Regression: | --- | Mount Type: | --- | ||||
| Documentation: | --- | CRM: | |||||
| Verified Versions: | Category: | --- | |||||
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
| Cloudforms Team: | --- | Target Upstream Version: | |||||
| Embargoed: | |||||||
| Bug Depends On: | 2254599 | ||||||
| Bug Blocks: | 1011110 | ||||||
| Attachments: |
|
||||||
|
Description
Ali Erdinc Koroglu
2023-11-27 15:18:48 UTC
Copr build: https://copr.fedorainfracloud.org/coprs/build/6700410 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2251785-openvino/srpm-builds/06700410/builder-live.log.gz 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. 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 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. 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})
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.spec#_178 I was really only looking at the toplevel thirdparty/ , but it looks like from above that they are also in the plugins. Hello Tom, thank you for the comments. SPEC Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-rawhide-x86_64/06766692-openvino/openvino.spec SRPM Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-rawhide-x86_64/06766692-openvino/openvino-2023.2.0-1.fc40.src.rpm Copr build: https://copr.fedorainfracloud.org/coprs/build/6766923 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2251785-openvino/fedora-rawhide-x86_64/06766923-openvino/fedora-review/review.txt 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. 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.
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-rawhide-x86_64/07399413-openvino/openvino.spec SRPM Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-rawhide-x86_64/07399413-openvino/openvino-2024.0.0-1.fc41.src.rpm 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/benchmark/sync_benchmark https://github.com/openvinotoolkit/openvino/tree/2024.0.0/samples/python/benchmark/throughput_benchmark Created attachment 2031038 [details]
The .spec file difference from Copr build 6766923 to 7400792
Copr build: https://copr.fedorainfracloud.org/coprs/build/7400792 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2251785-openvino/fedora-rawhide-x86_64/07400792-openvino/fedora-review/review.txt 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. 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.
|