-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
onnxruntime: 1.13.1 -> 1.15.1 and build on Darwin #226734
Conversation
142c528
to
be8f9af
Compare
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.
Result of nixpkgs-review pr 226734
run on aarch64-darwin 1
3 packages failed to build:
- onnxruntime
- onnxruntime.dev
- onnxruntime.dist
$ git -c fetch.prune=false fetch --no-tags --force https://github.com/NixOS/nixpkgs master:refs/nixpkgs-review/0 pull/226734/head:refs/nixpkgs-review/1
remote: Enumerating objects: 21, done.
remote: Counting objects: 100% (21/21), done.
remote: Compressing objects: 100% (17/17), done.
remote: Total 21 (delta 7), reused 9 (delta 2), pack-reused 0
Unpacking objects: 100% (21/21), 98.32 KiB | 1.31 MiB/s, done.
From https://github.com/NixOS/nixpkgs
0fd3aa7..9eb1b6a master -> refs/nixpkgs-review/0
- 7cddde92187...be8f9af855e refs/pull/226734/head -> refs/nixpkgs-review/1 (forced update)
$ git worktree add /Users/yamashita/.cache/nixpkgs-review/pr-226734/nixpkgs 9eb1b6a
Preparing worktree (detached HEAD 9eb1b6a)
Updating files: 100% (34561/34561), done.
HEAD is now at 9eb1b6a Merge pull request exportarr: 1.2.6 -> 1.3.1 #226786 from r-ryantm/auto-update/exportarr
$ nix-env --extra-experimental-features no-url-literals --option system aarch64-darwin -f /Users/yamashita/.cache/nixpkgs-review/pr-226734/nixpkgs -qaP --xml --out-path --show-trace --no-allow-import-from-derivation
$ git merge --no-commit --no-ff be8f9af855ef20e93b4b22c1d73519626623162c
Auto-merging pkgs/top-level/all-packages.nix
Automatic merge went well; stopped before committing as requested
$ nix-env --extra-experimental-features no-url-literals --option system aarch64-darwin -f /Users/yamashita/.cache/nixpkgs-review/pr-226734/nixpkgs -qaP --xml --out-path --show-trace --no-allow-import-from-derivation --meta
1 package updated:
onnxruntime (1.13.1 → 1.14.1)$ nix build --extra-experimental-features nix-command no-url-literals --no-link --keep-going --no-allow-import-from-derivation -f /Users/yamashita/.cache/nixpkgs-review/pr-226734/build.nix
error: hash mismatch in fixed-output derivation '/nix/store/jll6r5lvsy3bc9wmxbz7k643fykian8m-source.drv':
specified: sha256-6LG2Q9QSQBG6oynBkgdtXBsUra6LXPOZWR6i0dPMdeY=
got: sha256-LVLEn+e7c8013pwiLzJiiIObyrlbBHYaioO/SWbItPQ=
error: 1 dependencies of derivation '/nix/store/aw64zwxwqski1gij4c52h43n0xxzbyxw-onnxruntime-1.14.1.drv' failed to build
error: 1 dependencies of derivation '/nix/store/l6p33rm26164bfyjrpw4r4cwabgiqyg2-review-shell.drv' failed to buildLink to currently reviewing PR:
#2267343 packages failed to build:
onnxruntime onnxruntime.dev onnxruntime.disterror: build log of '/nix/store/aw64zwxwqski1gij4c52h43n0xxzbyxw-onnxruntime-1.14.1.drv!' is not available
error: build log of '/nix/store/9f2dk9h7rjcf82fxz76fnb3xamiw5jgx-onnxruntime-1.14.1' is not available
error: build log of '/nix/store/aw64zwxwqski1gij4c52h43n0xxzbyxw-onnxruntime-1.14.1.drv!' is not available
error: build log of '/nix/store/d7sl1sasbmk0f5sbid14vf0gk6h7hvv3-onnxruntime-1.14.1-dev' is not available
error: build log of '/nix/store/aw64zwxwqski1gij4c52h43n0xxzbyxw-onnxruntime-1.14.1.drv!*' is not available
error: build log of '/nix/store/8j6prfm74vylwn06xm7k13pzh05lc62c-onnxruntime-1.14.1-dist' is not available
I fixed the hash, thanks! |
"-DFETCHCONTENT_SOURCE_DIR_DATE=${howard-hinnant-date}" | ||
"-DFETCHCONTENT_SOURCE_DIR_EIGEN=${eigen}" | ||
"-DFETCHCONTENT_SOURCE_DIR_FLATBUFFERS=${flatbuffers}" | ||
"-DFETCHCONTENT_SOURCE_DIR_GOOGLETEST=${gtest}" |
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 was able to build it with the already packaged gtest.src
even though that is a little bit older.
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.
IIRC it builds but some tests segfault.
"-DFETCHCONTENT_QUIET=OFF" | ||
"-DFETCHCONTENT_SOURCE_DIR_ABSEIL_CPP=${abseil-cpp_202206.src}" | ||
"-DFETCHCONTENT_SOURCE_DIR_DATE=${howard-hinnant-date}" | ||
"-DFETCHCONTENT_SOURCE_DIR_EIGEN=${eigen}" |
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.
It also builds fine for me with the nixpkgs version with eigen.src
.
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.
Fails for me on MacOS with the following error. The eigen version is 3.4.0
.
In file included from /tmp/nix-build-onnxruntime-1.14.1.drv-0/source/onnxruntime/contrib_ops/cpu/inverse.cc:8:
In file included from /tmp/nix-build-onnxruntime-1.14.1.drv-0/source/onnxruntime/core/util/math_cpuonly.h:68:
In file included from /nix/store/aq0wbcxh58drkasgfr0rkidmmi0liwk0-source/Eigen/Core:358:
/nix/store/aq0wbcxh58drkasgfr0rkidmmi0liwk0-source/Eigen/src/Core/PartialReduxEvaluator.h:57:64: error: no matching function for call to 'pset1'
PacketType packetwise_redux_empty_value(const Func& ) { return pset1<PacketType>(0); }
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 have seen this error before. ONNX Runtime 1.15.0 will be released in next week. It will use eigen 3.4.0. But I don't see this build error anymore. I am also curious what gets changed.
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.
It seems that that this change did not make it into 1.15.0 after all, did it, @snnn?
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.
You are right.
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.
Is there one or two patches we can fetch to fix this? We don't want to vendor.
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.
No, updating the dependencies is a major undertaking by the looks of it.
If we decide to use this current approach, the commits would need to be squashed before we want to merge. I am still thinking about an alternative way. There is a csv-like file at https://github.com/microsoft/onnxruntime/blob/main/cmake/deps.txt where they track the dependencies in a somewhat readable format. If it would be helpful, I could try to come up with an updatescript that generates the expressions that we currently hand-code. With the hope, that this file will be around for a while. |
I'm adding ROCm support to this derivation, and finally got the thing to build fully (pretty hacky solution for some problems, the ROCm backend has all kinds of hardcoded things that rely on monolithic ROCm installs, but that's another story). Ended up getting runtime error during some tests (permissions issue probably for nixbld user?), so I changed doCheck from true to !useROCm. This exposes a problem with the build (which is apparently being hidden by doCheck being enabled somehow?):
you shouldn't have removed protobuf from buildInputs, I believe. Restoring that makes it work again with doCheck false |
I originally removed protobuf in 34d1d33. The expression has changed quite a bit since and I haven't being keeping up. |
shouldn't the oneDNN buildInput for python-onnxruntime somehow be put behind an (inherited?) useOneDNN parameter? not sure how to accomplish that, pretty new to nix still |
I agree. I didn't want to break the existing behavior, though. Not sure what the usual way to deal with a situation like this is. |
c7d4829
to
93e9852
Compare
Is there something missing at this point to push this over the finish line? |
Result of 16 packages failed to build:
|
Result of 16 packages failed to build:
Result of 4 packages marked as broken and skipped:
2 packages failed to build:
9 packages built:
|
93e9852
to
db96f3f
Compare
It appears that there is a major update coming down the pike which will lift some of the currently outdated dependencies to a current version: https://github.com/microsoft/onnxruntime/pull/15470/files That PR is unfortunately a little out of sync with the |
df29972
to
fad7b12
Compare
Result of 22 packages built:
|
@cbourjau Are you still planning to finish this up? |
fad7b12
to
4ff7745
Compare
, re2 | ||
, zlib | ||
, microsoft-gsl | ||
, microsoft-wil |
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.
To my embarrassment, it appears that microsoft-wil
is not needed after all to build onnxruntime
4ff7745
to
4827365
Compare
Any reason this is still marked as a draft? |
When I ran |
Result of 4 packages marked as broken and skipped:
2 packages failed to build:
33 packages built:
|
I see the following test failure on aarch64-linux:
|
Update to latest version and build on Darwin. Release notes: https://github.com/microsoft/onnxruntime/releases/tag/v1.15.1
4827365
to
6ae62cd
Compare
Removed the test file on aarch64-linux. Compiles on all 4 supported targets now. |
Thanks for pushing this over the finish line, @mweinelt ! I had some issues with tensorflow related packages on Monday, but am glad to finally see this PR land! I hope things will get a little easier with the next |
Description of changes
Update onnxruntime to the latest version and build it on Darwin. The build system saw several changes with the latest release. Most notably upstream has ceased to use git submodules for its dependencies in favor of cmake's
FetchContent
functionality.Release notes:
https://github.com/microsoft/onnxruntime/releases/tag/v1.15.0
Other changes:
test suite. This is not a regression since it was previously blocked
on this package not being available on Darwin.
onnxruntime API
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)