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

Conversation

matthewbauer
Copy link
Member

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 NixOS#85636.
Copy link
Contributor

@drewrisinger drewrisinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff LGTM

Copy link
Contributor

@bhipple bhipple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@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 April 23, 2020 02:02
@matthewbauer
Copy link
Member Author

Alright!

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

Successfully merging this pull request may close these issues.

None yet

4 participants