-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
python312Packages.bitsandbytes: 0.43.1 -> 0.43.3 #343246
Conversation
d8c2f05
to
2af0c9e
Compare
The CUDA-enabled version of this no longer builds when I do:
A fix is here: GaetanLepage/nixpkgs@2af0c9e...remexre:nixpkgs:bitsandbytes |
6d92bff
to
eee442b
Compare
Thanks, the patch seems to work indeed. |
Well, even though it does build fine with
|
|
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.
diff lgtm
This looks like a job for |
Ok, I investigated some more. ProblemWhen In this function, SolutionThe best solution I found was to:
Now everything builds and imports fine. No more error messages. Result (with
|
d7f6384
to
b1037b5
Compare
|
preBuild = | ||
if cudaSupport then | ||
'' | ||
export NVCC_APPEND_FLAGS="-I${cuda-native-redist}/include -L${cuda-native-redist}/lib" |
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.
cudaSetupHook sets NVCC_APPEND_FLAGS too. Use appendToVar
or prependVar
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 changed it to the same approach as in mistral-rs
. Is it fine now ?
if cudaSupport then | ||
'' | ||
export NVCC_APPEND_FLAGS="-I${cuda-native-redist}/include -L${cuda-native-redist}/lib" | ||
cmake -DCMAKE_CXX_FLAGS="-I${cuda-native-redist}/include" -DCOMPUTE_BACKEND=cuda -S . |
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 is normally done in confgiurePhase
which pkgs.cmake
in nativeBuildInputs
would set automatically. Istead of specifying the -D
flags in here, maybe move them to the nix attrset as cmakeFlags
so the hook passes them on as a bash variable. If for some reason you choose not to use the hook but to call cmake yourself, reimplement the flagsArray=(...) ; concatTo ...
functionality from the hook. The cmake hook respects dontUseCMake....Directory
flag and you can pass -S .
in cmakeFlags
too
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 successfully rely on the cmake hook.
I still manually run make
in the preBuild
phase.
|
||
postPatch = | ||
'' | ||
substituteInPlace Makefile --replace "/usr/bin/g++" "g++" --replace "lib64" "lib" | ||
substituteInPlace bitsandbytes/cuda_setup/main.py \ | ||
--replace "binary_path = package_dir / self.binary_name" \ | ||
"binary_path = Path('$out/${python.sitePackages}/${pname}')/self.binary_name" | ||
'' | ||
+ lib.optionalString torch.cudaSupport '' | ||
substituteInPlace bitsandbytes/cuda_setup/main.py \ | ||
--replace "/usr/local/cuda/lib64" "${cuda-native-redist}/lib" | ||
''; | ||
# By default, which library is loaded depends on the result of `torch.cuda.is_available()`. | ||
# When `cudaSupport` is enabled, bypass this check and load the cuda library unconditionnally. | ||
# Indeed, in this case, only `libbitsandbytes_cuda124.so` is built. `libbitsandbytes_cpu.so` is not. | ||
# Also, hardcode the path to the previously built library instead of relying 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.
Wow this really sounds like a bug in bitsanbytes, I'm surprised they're not reacting to the issue
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.
FWIW on our side we could actually make their presumably-inteded behaviour work, only at double the cost: we could just make the cuda-version depend on the cpu-version.
The question is, is the xxxxx_cuXXX.so
library usable in absence of GPUs?
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 think that this current approach is fine. If you set cudaSupport = true
, it means that you want to use cuda.
The question is, is the
xxxxx_cuXXX.so
library usable in absence of GPUs?
I don't think so.
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 think that this current approach is fine. If you set cudaSupport = true, it means that you want to use cuda.
Well the upstream has implemented this (broken?) dynamic dispatching logic so I guess they intended it the other way. Anyway, implementing that logic from scratch is not in-scope here
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.
Well the upstream has implemented this (broken?) dynamic dispatching logic so I guess they intended it the other way. Anyway, implementing that logic from scratch is not in-scope here
Yes, indeed.
b1037b5
to
bf85561
Compare
bf85561
to
768730d
Compare
(lib.getLib libcusparse) | ||
libcusparse.lib |
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.
drop the last? Btw getDev libcusparse
has .lib
in propagatedBuildInputs
, I forget if that matters to symlinkJoin
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.
Oh, well the last line libcusparse.lib
is clearly a mistake and is not needing because I have added lib.getLib libcusparse
above.
Now, having said that, you suggest also getting rid of that and only keeping lib.getDev libcusparse
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.
It works with lib.getDev libcusparse
alone.
768730d
to
08566de
Compare
08566de
to
4c4b30e
Compare
Ok I was able to make it build in the end. |
That matters fairly little when using |
|
Description of changes
Changelog: https://github.com/TimDettmers/bitsandbytes/releases/tag/0.43.3
cc @bcdarwin
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.