-
-
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
BLAS/LAPACK fix fallout from #83888 #85636
BLAS/LAPACK fix fallout from #83888 #85636
Conversation
We have some unused RPATHs we don’t want shrunk. These are used in MKL to dlopen based on architecture.
This is a better name since we have multiple 64-bit things that could be referred to. LP64 : integer=32, long=64, pointer=64 ILP64 : integer=64, long=64, pointer=64
This now relies on the "blas" and "lapack" packages.
dd9bcdb
to
a6a502f
Compare
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.
Currently compiling, going well so far ...
liblapack.so.3. | ||
</para> | ||
</section> | ||
</section> |
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 great, thank you! Can you also update (or delete / forward reference to) the docs next to numpy
?
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.
Can we add a paragraph and an example for howto select LP64/ILP64 with the new version. Consider the following situation: someone is writing a new derivation which needs a specific API (eg. ILP64, thanks btw for chaning the name) and uses the new attributes (blas, lapack) as inputs. What needs to be done to ensure this? I guess that assert (blas.is64bit) ...
is only a safeguard, not the selector?
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.
Updated the documentation based on this. Let me know if anything more is needed.
What needs to be done to ensure this? I guess that assert (blas.is64bit) ... is only a safeguard, not the selector?
Yes - right now assert is the only good way to do this. By default, LP64 should be used for compatibility, with ILP64 carrying the assertion. Perhaps there is a better way to handle this?
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.
Updated the documentation based on this. Let me know if anything more is needed.
What needs to be done to ensure this? I guess that assert (blas.is64bit) ... is only a safeguard, not the selector?
Yes - right now assert is the only good way to do this. By default, LP64 should be used for compatibility, with ILP64 carrying the assertion. Perhaps there is a better way to handle this?
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.
@matthewbauer So you are saying that putting in an assert (blas.isILP64)
or assert (!blas.isILP64)
will automatically build a package with the desired version (instead of just throwing an error)?
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.
Currently compiling a bunch of pkgs, will report back in a bit. In the meantime we can touchup the numpy documentation in the python section, possibly by just deleting it and referencing the general overlay alternatives.
Closes #85637 |
This is needed for numpy to detect mkl correctly.
Some of these are necessary for things like python.pkgs.numexpr
This fixes the build of scipy. |
This expands the documentation and explains how to assert LP64.
set it with <literal>LD_PRELOAD</literal>. Note that | ||
<literal>mkl</literal> is only available on | ||
<literal>x86_64-linux</literal> and | ||
<literal>x86_64-darwin</literal>. Moreover, Hydra is not build |
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.
<literal>x86_64-darwin</literal>. Moreover, Hydra is not build | |
<literal>x86_64-darwin</literal>. Moreover, Hydra is not building |
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.
Some minor doc feedback
<section xml:id="sec-overlays-alternatives"> | ||
<title>Using overlays to configure alternatives</title> | ||
<para> | ||
Certain software has different implementations of the same |
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.
Certain software has different implementations of the same | |
Certain software packages have different implementations of the same |
</para> | ||
<para> | ||
The Nixpkgs attribute is <literal>openblas</literal> for | ||
ILP64 and <literal>openblasCompat</literal> for LP64. This |
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.
define ILP64/LP64 here? It's defined lower.
packages to use different implementations, through the | ||
‘blasProvider’ and ‘lapackProvider’ argument. This can be used | ||
to select a different provider. For example, an overlay can be | ||
created that looks like: |
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.
created that looks like: | |
created to use Intel MKL that looks like: |
This overlay uses Intel’s MKL library for both BLAS and LAPACK | ||
interfaces. Note that the same can be accomplished at runtime | ||
using <literal>LD_PRELOAD</literal> of libblas.so.3 and | ||
liblapack.so.3. |
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 does this mean re LD_PRELOAD? Should it include an 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.
numexpr
now builds, but scipy
still segfaults in the mkl
variant. To reproduce:
$ nix-build -A python37Packages.scipy
Merged to unbreak packages on master. Further fixes can be made in a follow-up PR. |
FYI, I think the |
This adds some more information to the documentation as well as addressing review from NixOS#85636.
@matthewbauer looks like Python docs is missing a section. link to overlays is broken on MKL section |
It's a reference to another section in manual. It does render properly in the full html, but is a little awkward on its own. |
@matthewbauer am I missing something or is there no way to have an application rely on MKL, while rest of system is on whatever a user wants? Previously, I could write an application that does: let
pythonOverrides = python-self: python-super: {
# openblas does not work https://github.com/AlexEMG/DeepLabCut/issues/637#issuecomment-606893058
numpy = python-super.numpy.override { blas = mkl; };
};
python = python3.override {
packageOverrides = pythonOverrides;
};
py = python.pkgs;
in
py.toPythonApplication [...] There are scientific applications that only work with MKL, or only get correct results with OpenBLAS for example. How should I specify this as a package maintainer? |
@matthewbauer I have the same question as @tbenst. Due to #104026 I need to force R use OpenBlas while keeping the rest of the system default to MKL. After few unsuccessful attempts I ended up with this which appears to be working:
Is that the right approach? |
LGTM you can do |
Yes, looks good. I really wish #104026 is fixed somehow. R from nixpkgs is the only R I know that works incorrectly with MKL. I am thinking to create my own derivation for R which uses MKL directly. |
might also be nice to just change the R packaging so that it can work with the blas/lapack providers more nicely. Looking at https://stackoverflow.com/questions/14996697/compile-r-with-mkl-with-mulithreads-support , may expose some passthru linking options, and just have R consume those additional configure flags conditionally |
from R/default.nix
should be something like
in mkl/default.nix
in alternative/blas/default.nix
If we did go this route, probably would want to do the same for lapack. And I would probably make the flags an array, as string are kind of "messy". |
@jonringer I copied R/default.nix and tried applying the changes directly. It didn't work at first but then I decided to just replace the two lines
and the resulting R finally showed correct results for my test (#104026). Note that there is no |
Commits addressing review from #83888.
Motivation for this change
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)