-
-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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
Add BLAS/LAPACK switching mechanism #83888
Conversation
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.
Looks good, though I haven't taken the time to understand all the details. Thank you!
Documentation is needed though.
"lammps" "lammps-mpi" | ||
|
||
# requires openblas | ||
# "caffe" "mxnet" "flint" "sage" "sageWithDoc" |
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 don't think flint
and sage
necessarily require openblas. But since you added the comment I'm assuming you tested them. Maybe I'll have time to look into that once this is merged.
@@ -0,0 +1,91 @@ | |||
{ pkgsFun ? import ../.. |
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.
What's the intention of this file? Is it only for testing purposes?
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 suppose this was used in conjunction with Hydra to test this PR. It's still worth keeping although its not release *alternatives
but blas only.
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.
Yes - mainly to test that everything works. It certainly doesn't have to be included, but may help if we ever want to make a jobset to test this stuff.
if ! isELF "$i"; then continue; fi | ||
|
||
if $OBJDUMP -p "$i" | grep 'NEEDED' | awk '{ print $2; }' | grep -q '\(libmkl_rt.so\|libopenblas.so.0\)'; then | ||
echo "Found direct BLAS/LAPACK implementation reference in $i. Depend on blas / lapack instead." |
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 error should be a bit more actionable. It should explain why you should depend on blas
/ lapack
instead and how you can still use a specific implementation if you know what you're doing.
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 PR allows for a central configuration of blas to be applied on all packages. Before different packages were using different blas, either because no attention was paid, or because a package requires a specific one (e.g. numpy requires openblasCompat
).
Will passing in a specific blas for a specific package still function due to the renaming of -l
flags?
@@ -275,20 +275,16 @@ self: super: | |||
old: | |||
let | |||
blas = old.passthru.args.blas or pkgs.openblasCompat; |
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.
cc @adisbladis for the poetry2nix repo
@@ -0,0 +1,91 @@ | |||
{ pkgsFun ? import ../.. |
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 suppose this was used in conjunction with Hydra to test this PR. It's still worth keeping although its not release *alternatives
but blas only.
No, they need to use the generic name if you are using the generic "blas" or "lapack". But, you can still directly depend on openblas or mkl if you want and use the directly. |
This confuses me. So, we use the default And to work-around that issue, one needs to wrap it first and pass in the wrapped blas. let
my_blas = blas.override {blasProvider = mkl;};
in numpy.override {blas = my_blas;} It's a trivial operation but makes me think we should maybe have a |
Nix-review results: |
Yes, you wouldn't be able to do this because neither mkl, or openblas provide a "libblas" or "liblapack.
I think better than a wrapper would be just providing a symlink in mkl and openblas for this. I think this would actually make some other logic simpler. |
Allows overriding the BLAS lib. NixOS/nixpkgs#83888
Allows overriding the BLAS lib. NixOS/nixpkgs#83888
without master's fix in NixOS#83888, opencv3 & opencv4 end up with an 8-byte openblas, which it does work with. however this causes the python bindings to also end up with an 8-byte openblas, which numpy doesn't work with. force 4-byte openblas for opencv.
I have built R using MKL for both blas and lapack:
Now matrix multiplication for large matrices produces almost random results:
For comparison, Microsoft R Open, also with MKL, produces correct results:
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/improving-nixos-data-science-infrastructure-ci-for-mkl-cuda/5074/50 |
without master's fix in #83888, opencv3 & opencv4 end up with an 8-byte openblas, which it does work with. however this causes the python bindings to also end up with an 8-byte openblas, which numpy doesn't work with. force 4-byte openblas for opencv.
Removed non-functional lines. Openmolcas does not build with the blas/lapack switching mechanism
openmolcas: fix fallout from #83888
Most of them are: * separate packages for different openmodelica components, * qt4 -> qt5, * patches to instruct the OMEdit wrapper with stdenv executables location, * adoption of NixOS#89731 and NixOS#109595, * openblas -> blas, lapack according to NixOS#83888, * parallel building, * getting rid of spurious build phases, * correct the license, * cross-compilation, * forcing compiler to clang++ according to OM build recommendations, * drop of pangox_compat according to NixOS#75909 and NixOS#76412, * better dependencies, and more.
Co-authored-by: Jaakko Luttinen <jaakko.luttinen@iki.fi> Most of changes are: * separate packages for different openmodelica components, * qt4 -> qt5, * patches to instruct the OMEdit wrapper with stdenv executables location, * adoption of NixOS#89731 and NixOS#109595, * openblas -> blas, lapack according to NixOS#83888, * parallel building, * getting rid of spurious build phases, * correct the license, * cross-compilation, * forcing compiler to clang++ according to OM build recommendations, * drop of pangox_compat according to NixOS#75909 and NixOS#76412, * better dependencies, and more.
Motivation for this change
This is based on previous work for switching between BLAS and LAPACK
implementation in Debian[1] and Gentoo[2]. The goal is to have one way
to depend on the BLAS/LAPACK libraries that all packages must use. The
attrs “blas” and “lapack” are used to represent a wrapped BLAS/LAPACK
provider. Derivations that don’t care how BLAS and LAPACK are
implemented can just use blas and lapack directly. If you do care what
you get (perhaps for some CPP), you should verify that blas and lapack
match what you expect with an assertion.
The “blas” package collides with the old “blas” reference
implementation. This has been renamed to “blas-reference”. In
addition, “lapack-reference” is also included, corresponding to
“liblapack” from Netlib.org.
Currently, there are 3 providers of the BLAS and LAPACK interfaces:
By default, the above implementations all use the “LP64” BLAS and
LAPACK ABI. This corresponds to “openblasCompat” and is the safest way
to use BLAS/LAPACK. You may received some benefits from “ILP64” or
8-byte integer BLAS at the expense of breaking compatibility with some
packages.
This can be switched at build time with an override like:
or, switched at runtime via LD_LIBRARY_PATH like:
By default, we use OpenBLAS LP64 also known in Nixpkgs as
openblasCompat.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)