-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
[onnx,onnxruntime] new port for v1.19.2 with onnx 1.16.0 #36850
base: master
Are you sure you want to change the base?
Conversation
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 |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of work! 🚀
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renaming after install: Expected to break exported CMake config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these dirs empty? Or are there programs which need to be handled regardless of linkage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess there might some tool build based on features, but they are not defined in this port version.
Hi, requesting a review and a tag for it |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, we are using a different version of eigen3
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or temporarily change the port eigen3
to the commit first, and then wait for the upstream release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just outright deindex it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any rules for deindexing? I simply followed the case of qt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-gpu
entry 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! |
Status updateAnother blocker appeared in be8880a
Changing |
/azp run |
Rerunning due to network mistake last night. |
Azure Pipelines successfully started running 1 pipeline(s). |
I've updated vcpkg in the last couple of days, and as a result the onnxruntime overlay, which had been working before, is no longer compiling. After looking for the cause so far, I found two:
|
Ah, my bad... I forgot to change this into draft mode. I will check the port updates and rebase again... |
I found a new compile error,
|
Notes
With my experience in the registry, Some features may not work well with toolchain.
Suggestions from the comments.
*targets.cmake
in static builcfind_dependency
for the vcpkg portsHope more feature contributions after this work.
References
ONNXRuntime
eigen3
3.4.0 in vcpkg)microsoft/vcpkg
onnx
onnxruntime-gpu
nvidia-cutlass
luncliff/vcpkg-registry
Checklist
find_package
calls are REQUIRED, are satisfied byvcpkg.json
's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.vcpkg.json
matches what upstream says.vcpkg.json
matches what upstream says../vcpkg x-add-version --all
and committing the result.