-
Notifications
You must be signed in to change notification settings - Fork 180
Bump ONNXRuntime #5793
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
Bump ONNXRuntime #5793
Conversation
| ${LIBJALIENO2_ROOT:+-DlibjalienO2_ROOT=$LIBJALIENO2_ROOT} \ | ||
| ${XROOTD_REVISION:+-DXROOTD_DIR=$XROOTD_ROOT} \ | ||
| ${JALIEN_ROOT_REVISION:+-DJALIEN_ROOT_ROOT=$JALIEN_ROOT_ROOT} \ | ||
| ${ALIBUILD_O2_FORCE_GPU:+-DENABLE_CUDA=ON -DENABLE_HIP=ON -DENABLE_OPENCL=ON} \ |
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 seems to revert some of my recent changes, I think you rebased some collision incorrectly.
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 will have a look again. When did you merge them?
1a30bba to
0e38c22
Compare
|
I meant this: |
|
Ok, this misses David's patch, however now the dependencies for ONNXRuntime seem to be fully under control thanks to: notice there is the need for an additional "out of band" flatc invocation to have it work with our own version of flatbuffers. |
|
@ktf : Should we take one of the build container locally, and build it and compile locally to get this through? |
|
I thought we had the whole HIP coming from the system, while apparently there is some extra bits on top. Working on it. |
|
All CUDA / ROCm should come from the system. Where do you see some extra bits on top? Clearly ONNX builds some GPU stuff on top, but that should be ONNX only. |
|
As discussed privately, I am checking the eigen issue. In principle it should have worked. There is also: which is indeed not part of HIP, just has HIP in the name. |
7af2e90 to
524a3b7
Compare
|
The problem with Eigen is merely v1.21.0 needing an extra commit to support the Eigen which actually compiles correctly. unless they release v1.21.1 soon, we should probably fork and patch. So to my understanding there is only two things remaining:
Working around the above I get to the end of it on my box. |
4f4a1f5 to
3ed119c
Compare
|
I just fixed a couple of extra bits which make it compile on the EPN. |
2bf0c3e to
4b1deb6
Compare
Since the new abseil in alisw/alidist#5793 exposes c++20 in headers, we have to bump accordingly.
8786249 to
184b0a5
Compare
|
There is now a bunch of QC tests that fail. will have a look tomorrow. |
|
I think the issue with QC comes from compiling gRPC as archive library. Changing that back. |
|
So indeed the issue with QC seems to be corrected by going back to a shared library for gRPC. That's because if we don't some singleton gets initialised twice. I also had to add one more library for mesos due to that change. @singiamtel is checking the container. Apart from that I am back to believing this is ready to go. |
|
@ChSonnabend : Could you have a look? It has the hipblaslt headers available now, but not migraphx fails compilation with We could also disable migraphx at first, and solve that later. |
|
I would disable migraphx for now then and merge as it is. I haven't used the migraphx API calls myself yet, I can make a separate PR once I know what the issue is |
|
Merging since this was already tested. Will start a build now so that we gain a few hours for tomorrow. |
| # Our environment | ||
| set ${PKGNAME}_ROOT \$::env(BASEDIR)/$PKGNAME/\$version | ||
| prepend-path ROOT_INCLUDE_PATH \$${PKGNAME}_ROOT/include/onnxruntime |
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.
why was this taken out? I believe this change leads to crashes of all MC jobs since 4 days. See here: #5793

No description provided.