-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
rocmPackages.* 5.7.1→ 6.0.2 #287846
rocmPackages.* 5.7.1→ 6.0.2 #287846
Conversation
Result of 14 packages marked as broken and skipped:
13 packages failed to build:
175 packages built:
|
I added a few more fixes and the --- a/pkgs/development/rocm-modules/6/default.nix
+++ b/pkgs/development/rocm-modules/6/default.nix
@@ -16,6 +16,7 @@
let
rocmUpdateScript = callPackage ./update.nix { };
in rec {
+ gpuTargets = [ "gfx1102" ];
## ROCm ##
llvm = recurseIntoAttrs (callPackage ./llvm/default.nix { inherit rocmUpdateScript rocm-device-libs rocm-runtime rocm-thunk clr; });
@@ -73,7 +74,7 @@ in rec {
# Broken, too many errors
rdc = callPackage ./rdc {
- inherit rocmUpdateScript rocm-smi rocm-runtime;
+ inherit rocmUpdateScript rocm-smi rocm-runtime gpuTargets;
stdenv = gcc12Stdenv;
# stdenv = llvm.rocmClangStdenv;
};
+[... and so on ...] I can see that |
Hi! I just noticed that we've been working on the same thing. I hope you don't mind, but I went ahead and cherry-picked your changes into my branch: https://github.com/ScatteredRay/nixpkgs/tree/rocm6 #289187 |
That's great. Looks like you already looked at rocprofiler. |
Yeah. Rocprofiler seems to be building now with stdenv gcc.. want to get ot building with the same the rocm-llvm stdenv, but running into a few conflicts. I'm not certain, but I think it's basically running into this issue: #192459 but with the rocm-llvm |
I don't know anything about that unfortunately. |
I don't know specifically. I think some project need it to link against the hip code correctly. And then from there, there were some problem with conflicting libraries from linking with separate stdenvs with separate libraries. So I think in general it is a bit easier to link with stdenvs that have fewer differences. |
Results of building this against current master (fae7388): Result of 14 packages marked as broken and skipped:
7 packages failed to build:
88 packages built:
When building this branch by itself The |
|
The build for
the only similar file I can find is https://github.com/ROCm/composable_kernel/blob/9ce18b045d6ffa6cfa29134229b422c07984ffb7/include/ck/tensor_operation/gpu/device/device_gemm_multiple_d.hpp, but that does seems like an intentional difference since that file lives in I am not sure how to resolve this situation. |
I would like to mark ROCm 5.7 as deprecated, but using --- a/pkgs/top-level/all-packages.nix
+++ b/pkgs/top-level/all-packages.nix
@@ -7757,7 +7757,7 @@ with pkgs;
rar2fs = callPackage ../tools/filesystems/rar2fs { };
rocmPackages = rocmPackages_6;
- rocmPackages_5 = recurseIntoAttrs (callPackage ../development/rocm-modules/5 { });
+ rocmPackages_5 = lib.warn "ROCm 5.7 is deprecated in nixpkgs, please upgrade to ROCm 6.+" recurseIntoAttrs (callPackage ../development/rocm-modules/5 { });
rocmPackages_6 = recurseIntoAttrs (callPackage ../development/rocm-modules/6 { });
rune = callPackage ../development/interpreters/rune { }; EDIT: fixed eval for now |
It looks like all the packages marked as broken are still maintained upstream. I did not look into fixing them yet. I also looked at the sizes of all of the output derivations. They did increase a bit, but none of the outputs really blew up in size significantly, and none seem to be close to the 3 GB size limit. |
I am marking this as ready for review to get some feedback. EDIT: I found a PR which adds the file in question to |
That PR (ROCm/composable_kernel#1134) is not in the ROCm release tagged for 6.0.2 yet, and simply adding the correct include files to the I'll see what they say in the issue that I opened, but it looks like marking |
Running Edit, it seems like Other than that 46/54 packages are built so it looks optimistic. |
Result of 16 packages marked as broken and skipped:
90 packages built:
|
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.
From what I can tell this looks good to go
let stdenv' = stdenv; in | ||
let stdenv = | ||
if stdenv'.cc.cc.isGNU or false && lib.versionAtLeast stdenv'.cc.cc.version "13.0" | ||
then gcc12Stdenv | ||
else stdenv'; | ||
in | ||
|
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 this still be here? Doesn't rocm-llvm 6 build with gcc 13?
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.
Good catch. I will verify that it builds with gcc 13 and remove this if it does.
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 a good machine for building things so let me know if you want me to test something
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 a good machine for building things so let me know if you want me to test something
Thanks for the offer. I have 128 GB of RAM now as well, and that's enough for me to build ROCm now.
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 still does not build with GCC 13.
What fails to build without gcc12Stdenv
is rStdenv
and runtimes
inside llvm/default.nix
.
I was thinking about pushing a change to reduce the usages down to strictly those, but I have the feeling that better to not have more than one specific version of GCC involved there.
@@ -0,0 +1,58 @@ | |||
{ # stdenv FIXME: Try changing back to this with a new ROCm release https://github.com/NixOS/nixpkgs/issues/271943 | |||
gcc12Stdenv |
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.
Also this.
I think this in terms of content this PR is ready to get merged now. |
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.
Wow, this is great!
NixOS#286720 introduced these patches to address a specific compilation error mentioned in ROCm/HIP#3403, but added them to the source tree because they were originally for ROCm 6. For ROCm 6, we can now switch to using fetchpatch to get the original commits as patches.
I'm currently running |
Technically some reviews happened that left comments and not approvals. @ulrikstrid, if you say those reviewers left their comments and we don't have to go through the motions of waiting for their approval I'm fine with you merging this now. I would give them some more time, because I don't have that confidence, but I get that there is also a cost to dragging things out unnecessarily. These are the results of my run (of just the commit, not merging the PR into master). The build of rocprofiler for both 5.7 and 6.0 seems a bit flaky. With ´nixpkgs-review´ it fails quite often for me. Result of 15 packages marked as broken and skipped:
1 package failed to build:
182 packages built:
|
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.
Didn’t build all of this, but ran the OpenCL and rocm-smi tests. LGTM from me.
eval $(nix-build -A rocmPackages.clr.icd.impureTests.rocm-smi)
eval $(nix-build -A rocmPackages.clr.icd.impureTests.opencl-example)
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 had 0 failures when building and I think it looks good to go.
I'll give it a bit more time if you want to @mschwaig before I merge. I guess it's just @Tungsten842 that has a open comment right?
That's right. There are no other open review comments, besides @Tungsten842's. Using my own judgement about that issue and the discussion around it, I think we are at the point where we should merge. I think most remaining issues listed in the top comment are worse than still using GCC 12, and we are also not waiting for those to be fixed either. |
Lets ship it then! |
Great job 🎉 |
|
Also rocblas times out on hydra: https://hydra.nixos.org/build/254017164 |
And |
I don't understand why
How should we address this issue? |
Why is
That's weil below the 3 GB limit, and I though I would be surprised if symlinked contents counted I also cannot find any:
|
The NAR-size reported by
|
Turns out I should have used the |
Turns out the issue is one huge
And So I am actually tempted to compress that file in the output of the original build derivation, and then have another derivation based on Is that the kind of evil sorcery that would be allowed in nixpkgs? |
Description of changes
CC #197885
This adds ROCm 6.0.2 alongside ROCm 5.7.
This PR
rocmPackages_5
Remaining issues
migraphx
does not build ([Issue]: 'ck/host/device_gemm_multiple_d.hpp' file not found ROCm/AMDMIGraphX#2892)look at output sizes (since there seems to be a 3 GB limit if you want caching from cache.nixos.org)look through the packages marked as broken and remove them if they became irrelevantredo file copying to preserve history (see https://stackoverflow.com/a/46484848)add myself as a maintainerRelated work
I was not able to take the changes from https://github.com/Madouura/nixpkgs/tree/dev/rocm-rework,
because that branch has a large amount of changes which I felt would have taken me a long time review and understand, especially since I did not know how solid those changes are already.
Closes #280927
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.