python3Packages.bitsandbytes: do not require cuda for non cuda build#424174
python3Packages.bitsandbytes: do not require cuda for non cuda build#424174ferrine wants to merge 1 commit intoNixOS:masterfrom
Conversation
|
|
bcdarwin
left a comment
There was a problem hiding this comment.
Overall looks good although unfortunately needs a rebase - sorry about the delay.
(I don't have a strong opinion about the separation into two separate attrsets - seems Nixpkgs style is mostly to push the conditions down instead as with the previous code. This does result in a proliferation of essentially the same conditional but doesn't seem to be a big problem in practice, probably because individual packages are leaf nodes in Nixpkgs?)
|
|
||
| cmakeFlags = [ (lib.cmakeFeature "COMPUTE_BACKEND" "cuda") ]; | ||
|
|
||
| CUDA_HOME = "${cuda-native-redist}"; |
There was a problem hiding this comment.
Should be env.CUDA_HOME if possible (though I realize this is just preserving what was there before).
| ]; | ||
|
|
||
| doCheck = false; # tests require CUDA and also GPU access | ||
| doCheck = false; |
There was a problem hiding this comment.
There should probably be a comment explaining why tests are disabled, so better to update than remove the previous one
|
|
||
| CUDA_HOME = "${cuda-native-redist}"; | ||
|
|
||
| NVCC_PREPEND_FLAGS = [ |
There was a problem hiding this comment.
Similarly consider updating to env.NVCC_PREPEND_FLAGS.
|
Yes, I also had a concern over the chosen approach and wanted to get into the feedback loop once #424179 is finished |
|
bitsandbytes has ROCm support upstream so there'll need to be a third set of attrs to merge in and it may get a bit repetitive using this structure. |
| buildInputs = [ ]; | ||
|
|
There was a problem hiding this comment.
| buildInputs = [ ]; |
if it is empty, we can just omit it, right?
|
Due to #441275, will now need to be rebased .. |
|
The pr might solve the problem as well, will check later after xgrammar is pr merged |
|
I don't think this convoluted approach achieves anything more than has been already fixed by #441275 |
|
Agree |
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-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.