rocm-related: 5.4.X -> 5.7.0#258328
Merged
lovesegfault merged 30 commits intoNixOS:masterfrom Oct 10, 2023
Merged
Conversation
github-actions
bot
added
the
6.topic: python
Madouura
force-pushed
the
pr/rocm-update
branch
from
575763f to
6d24a6e
Compare
ofborg
bot
requested review from
Flakebi,
acowley,
dtzWill,
kira-bruneau and
lovesegfault
ofborg
bot
added
10.rebuild-darwin: 11-100
Madouura
force-pushed
the
pr/rocm-update
branch
from
8f7127e to
7d53d41
Compare
Contributor
Author
|
At some point after |
Madouura
force-pushed
the
pr/rocm-update
branch
from
7d53d41 to
7e67e21
Compare
Contributor
🎉 Hooray! Yeah ran into the same thing when trying to build this locally last night - makes maintenance for rocfft a lot simpler 😊 I think we still might want to keep diff --git a/pkgs/development/rocm-modules/5/rocfft/default.nix b/pkgs/development/rocm-modules/5/rocfft/default.nix
index 5325744540b5..ac50318622ce 100644
--- a/pkgs/development/rocm-modules/5/rocfft/default.nix
+++ b/pkgs/development/rocm-modules/5/rocfft/default.nix
@@ -14,6 +14,7 @@
, gtest
, openmp
, rocrand
+, gpuTargets ? [ ]
}:
stdenv.mkDerivation (finalAttrs: {
@@ -45,6 +46,8 @@ stdenv.mkDerivation (finalAttrs: {
"-DCMAKE_INSTALL_BINDIR=bin"
"-DCMAKE_INSTALL_LIBDIR=lib"
"-DCMAKE_INSTALL_INCLUDEDIR=include"
+ ] ++ lib.optionals (gpuTargets != [ ]) [
+ "-DAMDGPU_TARGETS=${lib.concatStringsSep ";" gpuTargets}"
];
passthru = { |
Tungsten842
reviewed
Oct 4, 2023
kira-bruneau
reviewed
Oct 4, 2023
Madouura
force-pushed
the
pr/rocm-update
branch
from
7e67e21 to
f22d15e
Compare
Contributor
Author
Done. |
Madouura
force-pushed
the
pr/rocm-update
branch
5 times, most recently
from
8e17d3f to
4072599
Compare
github-actions
bot
added
6.topic: nixos
Contributor
Author
|
Did the nixpkgs docs change? |
Madouura
force-pushed
the
pr/rocm-update
branch
3 times, most recently
from
9cb4560 to
156915a
Compare
Contributor
Author
|
I am still unsure as to why python3.11-torch> Successfully preprocessed all matching files.
python3.11-torch> no configure script, doing nothing
python3.11-torch> building
python3.11-torch> Executing setuptoolsBuildPhase
python3.11-torch> Traceback (most recent call last):
python3.11-torch> File "/build/source/setup.py", line 224, in <module>
python3.11-torch> from setuptools import setup, Extension, find_packages
python3.11-torch> ModuleNotFoundError: No module named 'setuptools'
python3.11-torch> /nix/store/3pfjacvdg9f491lfqc9qb2d0nknx73fb-stdenv-linux/setup: line 144: pop_var_context: head of shell_variables not a function contextRelated: #246250 |
Contributor
Author
echo $PYTHONPATH/nix/store/pzf6dnxg8gf04xazzjdwarm7s03cbrgz-python3-3.10.12/lib/python3.10/site-packagesLooks like there's a |
Contributor
Author
|
Result of 8 packages marked as broken and skipped:
1 package blacklisted:
6 packages failed to build:
240 packages built:
|
Flakebi
approved these changes
Oct 10, 2023
delroth
added
the
12.approvals: 1
Member
|
Kicked-off another build ~30 min ago, just waiting on that. |
Member
|
I'm noticing that |
Member
|
I'm trying out this patch that explicitly makes the build |
Member
|
I rebuilt Maybe the build for |
Contributor
Author
|
Member
|
Yeah, the |
Member
|
Result of 15 packages marked as broken and skipped:
2 packages blacklisted:
233 packages built:
|
Member
|
I'm going to merge this, once again, huge props to @Madouura for pushing this fantastic work to completion! |
Contributor
Author
|
Thanks for the testing and the merge! ❤️ |
2 tasks
This was referenced Oct 10, 2023
| propagatedBuildInputs = [ filelock ]; | ||
|
|
||
| postPatch = let | ||
| # Bash was getting weird without linting, |
Contributor
There was a problem hiding this comment.
RE: cosmetic changes
Sorry I post this after the fact, but it's conventional to keep postPatch next to src
Contributor
Author
There was a problem hiding this comment.
I always did pname -> version -> outputs -> src -> patches -> native/build/propagated/inputs -> cmakeFlags -> post/pre/phase.
Where is this convention?
Contributor
There was a problem hiding this comment.
What I think I remember is a comment from SuperSandro, but I'm failing to find it. The logic was, roughly speaking, to list attributes in the "chronological" order of when they are relevant: the first phase is unpacking the sources, so src goes first, and the very next thing is patching, and the very next after that is postPatch, and all of that happens before the configurePhase and before the build
Contributor
Author
There was a problem hiding this comment.
I see. Are native/build/propagated/inputs and cmakeFlags after patchPhase?
Contributor
There was a problem hiding this comment.
Something like https://discourse.nixos.org/t/document-attribute-ordering-in-package-expressions/4887?u=sergek:
# Phases attrs are ordered exactly as they're executed in setup.sh
That being said, it's just a convention
Contributor
There was a problem hiding this comment.
But also generally big formatting changes are undesirable because they're inconvenient to review, especially when formatting changes are entangled with functional changes
|
|
||
| # Upstream's setup.py tries to write cache somewhere in ~/ | ||
| export HOME=$TMPDIR | ||
| export HOME=$(mktemp -d) |
Contributor
There was a problem hiding this comment.
What's wrong with TMPDIR?
Contributor
Author
There was a problem hiding this comment.
Is TMPDIR always guaranteed? I swear I remember someone telling me to use mktemp -d instead a while back on nixpkgs.
Member
There was a problem hiding this comment.
TMPDIR is documented at https://nixos.org/manual/nix/stable/language/derivations
(but otherwise I'm just passing by)
Member
|
@Madouura We need to remove the warning from the aliases, unfortunately, they're causing all invocations of I apologize for misguiding you, I should've known this would happen. Let's just leave them as normal aliases, and convert to a throw after the release. |
12 tasks
Contributor
Author
|
Done. #260910 |
Contributor
|
Hey, I think this might be causing build failures not sure but I am getting this now when building Ollama And the full error I assume PR #3853 fixes it though correct? |
Member
|
Missing some digit(s) in the PR number? Anyway, I've noticed this and suspected that it was triggered by cmake upgrade from 3.28 to 3.29. |
Contributor
Oops yeah, must have done something weird with the mouse. My mouse has a really bad double clicking issue to be honest |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of changes
Tracking: #197885
Time for a ROCm update.
Not ready for review, unless you just want me to fix some things you happen to find.We're ready for review!
Any huge changes we want to make, we should wait for ROCm 6.0 where there's a bit of a refactor.I'm bad at following my own advice.
TODO
switch to cudaPackages-like package set✅base ROCm stack✅openmp (depends on rocm-device-libs and rocm-runtime)✅mlir (upstream problem?)✅openai-triton (needs rebase on LLVM 16 or 17. Switch to triton-unstable?)✅rocm-llvmfor this!separate rocBLAS for hydra (we're at 3.7GB!)✅rest of the ROCm stack✅make sure torchWithRocm works✅Shelved
resolves #257477, resolves #255814, resolves #255345, resolves #252668, resolves #252262, resolves #243383, resolves #242885, resolves #239621, resolves #238161, resolves #238112, resolves #238109, resolves #233556, resolves #231000, resolves #228088, resolves #259796, resolves #259910, fixes #246250
Things done
sandbox = trueset 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/)