Skip to content
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 fixup and expand docs #85799

Merged
merged 4 commits into from Apr 23, 2020
Merged

Conversation

@matthewbauer
Copy link
Member

matthewbauer commented Apr 22, 2020

Addresses comments left in #85636.

/cc @drewrisinger @bhipple

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
To match the soname, we need to use libblas.so.3, liblapack.so.3.
To avoid segmentation fault on MKL, we need to use the CBLAS ABI.
Normally, scipy will autodetect this, but here we don’t link using the
‘libmkl_rt.so’ name. So we need to manually set this.

See https://github.com/scipy/scipy/blob/50012918db47f0ec3418dd53f0277daa589f82e3/doc/source/building/linux.rst#fortran-abi-mismatch
This adds some more information to the documentation as well as
addressing review from #85636.
@FRidh
FRidh approved these changes Apr 22, 2020
Copy link
Contributor

drewrisinger left a comment

Diff LGTM

Copy link
Contributor

bhipple left a comment

Found an issue w/ the docs; otherwise LGTM. Currently compiling with and without mkl in the stack and will report back when it's done.

doc/using/overlays.xml Outdated Show resolved Hide resolved
ln -s $out/lib/libopenblas${stdenv.hostPlatform.extensions.sharedLibrary} $out/lib/libblas${stdenv.hostPlatform.extensions.sharedLibrary}${stdenv.lib.optionalString stdenv.hostPlatform.isLinux ".3"}
ln -s $out/lib/libopenblas${stdenv.hostPlatform.extensions.sharedLibrary} $out/lib/libcblas${stdenv.hostPlatform.extensions.sharedLibrary}${stdenv.lib.optionalString stdenv.hostPlatform.isLinux ".3"}
ln -s $out/lib/libopenblas${stdenv.hostPlatform.extensions.sharedLibrary} $out/lib/liblapack${stdenv.hostPlatform.extensions.sharedLibrary}${stdenv.lib.optionalString stdenv.hostPlatform.isLinux ".3"}
ln -s $out/lib/libopenblas${stdenv.hostPlatform.extensions.sharedLibrary} $out/lib/liblapacke${stdenv.hostPlatform.extensions.sharedLibrary}${stdenv.lib.optionalString stdenv.hostPlatform.isLinux ".3"}

This comment has been minimized.

Copy link
@bhipple

bhipple Apr 22, 2020

Contributor

This is getting long enough that it might be worth factoring out into a helper var.

This comment has been minimized.

Copy link
@matthewbauer

matthewbauer Apr 23, 2020

Author Member

yes - we can actually use passthru from blas/lapack here without infinite recursion, but it's kind of confusing conceptually

@bhipple
Copy link
Contributor

bhipple commented Apr 23, 2020

Great, managed to build through most of the scientific computing stack with mkl now, and also built through pandas without the mkl overlay to ensure no regressions there! 👍

Just the one documentation fix on LD_LIBRARY_PATH, but otherwise this looks good to merge.

@bhipple bhipple merged commit 21be830 into NixOS:master Apr 23, 2020
1 check was pending
1 check was pending
grahamcofborg-eval Checking original out paths
Details
@bhipple
Copy link
Contributor

bhipple commented Apr 23, 2020

Since I just had a documentation comment and others have approved the PR, and since it's fixing builds on master and causing a lot of rebuilds, I went ahead and applied my doc update, then merged it so that Hydra can catch up.

@ofborg ofborg bot requested review from bhipple and FRidh Apr 23, 2020
@matthewbauer
Copy link
Member Author

matthewbauer commented Apr 23, 2020

Alright!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.