[onnx,onnxruntime] new port for v1.18.0 with onnx 1.16.0 #36850
Conversation
jimwang118
added
the
category:new-port
|
Hmm.. for some reason CMake tries old version of Xcode ... |
|
IIRC the xcode generator needs a GUI session, i.e. doesn't work in CI. |
|
Added notes for the current features. |
|
Can't fix the errors of the CUDA code. I will remove |
|
So the build error comes from arm_neon build.
Let me check the details... |
For NDK 25(Clang 14.0.7), I tried |
|
ports/onnxruntime/portfile.cmake
Outdated
| if(("openvino" IN_LIST FEATURES) AND VCPKG_TARGET_IS_WINDOWS) | ||
| file(RENAME "${CURRENT_PACKAGES_DIR}/debug/lib/onnxruntime_providers_openvino.dll" "${CURRENT_PACKAGES_DIR}/debug/bin/onnxruntime_providers_openvino.dll") | ||
| file(RENAME "${CURRENT_PACKAGES_DIR}/lib/onnxruntime_providers_openvino.dll" "${CURRENT_PACKAGES_DIR}/bin/onnxruntime_providers_openvino.dll") | ||
| endif() |
There was a problem hiding this comment.
Renaming after install: Expected to break exported CMake config.
There was a problem hiding this comment.
The installation was fixed in microsoft/onnxruntime#19966
Use the latest release to apply the patch
ports/onnxruntime/portfile.cmake
Outdated
| if(VCPKG_LIBRARY_LINKAGE STREQUAL "static") | ||
| file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/bin" "${CURRENT_PACKAGES_DIR}/bin") | ||
| endif() |
There was a problem hiding this comment.
Are these dirs empty? Or are there programs which need to be handled regardless of linkage?
There was a problem hiding this comment.
I guess there might some tool build based on features, but they are not defined in this port version.
luncliff
changed the title
|
Hi, requesting a review and a tag for it |
luncliff
force-pushed
the
port/onnxruntime
branch
2 times, most recently
from
d4aa6f4
to
9e67876
Compare
* merge fix-dependency-protobuf.patch into fix-cmakelists.patch
luncliff
force-pushed
the
port/onnxruntime
branch
from
9e67876
to
4e24728
Compare
|
Hi team. I was waiting the upstream's pull request, but seems like it's taking more time than I expected. Here is the current status of this pull request. (Just in my memory, feel free to comment more!) Notes
Issues
As a vcpkg user and registry maintainer, I think #38879 can be closed.
Building CUDA in vcpkg CI was simple. However, installation path of TensorRT is unknown. |
### Changes
1. CMake option `onnxruntime_USE_VCPKG`. It will be used in the vcpkg
port
* Unit test may fail because this option leads to a mixture of
unexpected external library versions.
Especially ONNX, Protobuf, and Flatbuffers version can be different
2. Overhaul of `onnxruntime_external_deps.cmake`
* Make `FetchContent_Declare` to try `find_package`.
See
https://cmake.org/cmake/help/latest/guide/using-dependencies/index.html
* Relocated `FetchContent_Declare` and `FetchContent_MakeAvailable`(or
`onnxruntime_fetchcontent_makeavailable`) to closer lines.
It was too hard to navigate the entire file to search related
sections...
* Alias `IMPORTED` targets like build targets (e.g. `ONNX::onnx` -->
`onnx`)
```cmake
# The script uses `find_package` with the changes.
# In this case, use vcpkg to search dependencies
# See https://cmake.org/cmake/help/latest/guide/using-dependencies/index.html
include(external/onnxruntime_external_deps.cmake)
```
3. Create CMakePresets.json and presets to [run vcpkg in manifest
mode](https://learn.microsoft.com/en-us/vcpkg/concepts/manifest-mode)
* Currently, it's NOT for training build
* Main triplets are `x64-windows` and `x64-osx`
```pwsh
Push-Location "cmake"
cmake --preset "x64-windows-vcpkg"
cmake --build --preset "x64-windows-vcpkg-debug"
Pop-Location
```
```bash
pushd "cmake"
cmake --preset "x64-osx-vcpkg"
cmake --build --preset "x64-osx-vcpkg-debug"
popd
```
4. Updated tools/ci_build/build.py
* `--use_vcpkg` option: it needs `CMAKE_TOOLCHAIN_FILE` with
[vcpkg.cmake toolchain
script](https://github.com/microsoft/vcpkg/blob/master/scripts/buildsystems/vcpkg.cmake)
* `--compile_no_warning_as_error` is recommended because library version
differences will cause unexpected compiler warnings
```bash
python ./tools/ci_build/build.py \
--compile_no_warning_as_error \
--use_vcpkg \
--cmake_extra_defines "CMAKE_TOOLCHAIN_FILE:FILEPATH=${VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake" \
--cmake_extra_defines "VCPKG_TARGET_TRIPLET=..."
```
5. Created Job `Vcpkg` for Windows and macOS
* Show how to setup and use vcpkg.
Similar to the CMakePresets.json usage
### Motivation and Context
* Help #7150
* Help microsoft/vcpkg#36850
* luncliff/vcpkg-registry#212
* microsoft/vcpkg#39881
* luncliff/vcpkg-registry#215
* luncliff/vcpkg-registry#216
* luncliff/vcpkg-registry#227
*
https://cmake.org/cmake/help/latest/guide/using-dependencies/index.html
*
https://github.com/microsoft/vcpkg/blob/master/scripts/buildsystems/vcpkg.cmake
### Future Works?
More feature coverage with the vcpkg supported libraries
* CUDA feature support
* Training feature support
* remove 'eigen3' from dependency * suppress C++23 deprecation warnings
ports/onnxruntime/portfile.cmake
Outdated
| # https://github.com/microsoft/onnxruntime/blob/26250ae74d2c9a3c6860625ba4a147ddfb936907/cmake/deps.txt#L20-L25 | ||
| vcpkg_from_gitlab( | ||
| GITLAB_URL https://gitlab.com | ||
| OUT_SOURCE_PATH EIGEN_SOURCE_PATH | ||
| REPO libeigen/eigen | ||
| REF e7248b26a1ed53fa030c5c459f7ea095dfd276ac | ||
| SHA512 2ede6fa56b8374cd5618a9cca9f3666909255277d0fe23eb54266972a9ab4e6c8c00abcb8fab918ea45b4aec37a3d316ca01107ff082dc9f19851df58d7ac80d | ||
| ) |
There was a problem hiding this comment.
Here, we are using a different version of eigen3.
There was a problem hiding this comment.
This will introduce potential dependency conflicts and https://learn.microsoft.com/en-us/vcpkg/contributing/maintainer-guide#do-not-use-vendored-dependencies
Could the required fixes be backported?
There was a problem hiding this comment.
Or temporarily change the port eigen3 to the commit first, and then wait for the upstream release.
There was a problem hiding this comment.
I don't think I can backport the related part correctly in ONNXRuntime. Either bumping eigen3 for the other ports.
In my vcpkg-registry, I can move to any commit I want, but it's different in this repository.
So I need some direction for the next move.
- To bump
eigen3, it should be done in another PR considering the work size - To backport
eigen3, I can try, but some API might block coupled to the version and I can't make a correct change in the case
There was a problem hiding this comment.
I would like the simple upgrade but upstream hasn’t released new version. You could keep this usage temporarily while fixing onnx.
If fixed version of eigen3 is still not released before merging current PR, I will need further review by the team.
There was a problem hiding this comment.
I don't think vendoring a different eigen than the one selected by vcpkg is acceptable. It leads to ODR violations.
There was a problem hiding this comment.
Based on the comment "# This Eigen commit id matches the eigen archive being consumed from https://gitlab.com/libeigen/eigen/-/archive/3.4/eigen-3.4.zip" given that vcpkg is consuming that same thing the one in our port should have the fixes that onnxruntime needs?
Do we know of a specific change onnxruntime needs that we could backport to our port?
I prefer to recommend upstreams to not use vcpkg-specific hacks. There is nothing special about wanting de-vendored dependencies. (vcpkg may have unofficial CMake configs, but they are unofficial to limit their proliferation.) |
Most of I think it's enough for now. |
| # # Handle copyright | ||
| vcpkg_install_copyright(FILE_LIST "${SOURCE_PATH}/onnxruntime-win-x64-gpu-${VERSION}/LICENSE") | ||
| set(VCPKG_POLICY_EMPTY_PACKAGE enabled) | ||
| message(WARNING "${PORT} is deprecated. Please use port onnxruntime instead.") |
There was a problem hiding this comment.
Should we just outright deindex it?
There was a problem hiding this comment.
Any rules for deindexing? I simply followed the case of qt
There was a problem hiding this comment.
As written is OK; it's more a user experience thing. If you think there are lots of existing users of onnxruntime-gpu who would be confused by it suddenly not existing, then keeping this makes sense. That was the case with qt, in particular because vcpkg install qt is something one is likely to do.
If you think there aren't a huge number of users who would be confused by it not existing, making it not exist might be clearer for most users because they won't be shown that a port exists that they shouldn't use.
To make it not exist:
- Revert all changes to
versions/o-/onnxruntime-gpu.json - Delete the
onnxruntime-gpuentry fromversions/baseline.json - Delete
ports/onnxruntime-gpu
Otherwise it's fine as written
|
I'm fine with #40962 and other updates of related ports. I'm checking them regularly. I will try supporting the current |
Ok, I merged it then. Thanks! |
Notes
With my experience in the registry, Some features may not work well with toolchain.
Suggestions from the comments.
*targets.cmakein static builcfind_dependencyfor the vcpkg portsHope more feature contributions after this work.
References
ONNXRuntime
microsoft/vcpkg
onnxonnxruntime-gpuluncliff/vcpkg-registry
Checklist
find_packagecalls are REQUIRED, are satisfied byvcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.vcpkg.jsonmatches what upstream says.vcpkg.jsonmatches what upstream says../vcpkg x-add-version --alland committing the result.