mxnet: use openblas, fix build #87118

jonringer · 2020-05-06T17:24:53Z

Motivation for this change

noticed it was failing when reviewing #87102

previous failure:

builder for '/nix/store/1zcd9mmm3dyyxcr588cszdclrsnjbga1-mxnet-1.6.0.drv' failed with exit code 1; last 10 log lines:
  -- Could not find OpenBLAS lib. Turning OpenBLAS_FOUND off
  CMake Error at cmake/Modules/FindOpenBLAS.cmake:82 (MESSAGE):
    Could not find OpenBLAS
  Call Stack (most recent call first):
    cmake/ChooseBlas.cmake:42 (find_package)
    CMakeLists.txt:310 (include)


  -- Configuring incomplete, errors occurred!
  See also "/build/apache-mxnet-src-1.6.0-incubating/build/CMakeFiles/CMakeOutput.log".

Things done

bhipple

We should keep using the virtual package, so that if someone overlays mkl as the blas implementation mxnet will pick it up correctly.

Sometimes, we need the specific provider package in buildInputs as well, to get things like cmake hooks or path searches working. The way to do that is to also add blas.provider to the buildInputs, which will cleanly and consistently handle the choice.

CC @matthewbauer I'm pretty sure the above is correct; I had to do something similar for pytorch. Perhaps we should add to the manual?

bhipple · 2020-05-06T18:02:12Z

pkgs/applications/science/math/mxnet/default.nix

@@ -1,5 +1,5 @@
 { config, stdenv, lib, fetchurl, bash, cmake
-, opencv3, gtest, blas, perl
+, opencv3, gtest, openblas, perl


Suggested change

, opencv3, gtest, openblas, perl

, opencv3, gtest, blas, perl

bhipple · 2020-05-06T18:02:31Z

pkgs/applications/science/math/mxnet/default.nix

@@ -17,7 +17,7 @@ stdenv.mkDerivation rec {

  nativeBuildInputs = [ cmake perl ];

-  buildInputs = [ opencv3 gtest blas ]
+  buildInputs = [ opencv3 gtest openblas ]


Suggested change

buildInputs = [ opencv3 gtest openblas ]

buildInputs = [ opencv3 gtest blas blas.provider ]

ah, wasn't aware of the provider usage

is there any need for the actual blas package/attr set? Seems to build fine with only the provider

oh, linking maybe

Build just failed for me; looks like we need to do the same with lapack to make it visible to the linker.

is there any need for the actual blas package/attr set? Seems to build fine with only the provider

Not actually sure ... in principle it might be sufficient to just pass in blas.provider, but it seems reasonable to expose the meta-package as well, as that has the actual public API shared objects defined.

bhipple

mxnet built on my machine with this update now; kicking off a nixpkgs-review now.

jonringer · 2020-05-06T19:16:48Z

I'm not a big fan of having to do both blas blas.provider, is there a way that we can symLinkJoin (or something else) them so that people only need to specify blas

jonringer · 2020-05-06T19:18:38Z

AFAIK, the provider pass thru is pretty special to blas libraries

bhipple · 2020-05-06T19:20:21Z

Result of nixpkgs-review pr 87118 1

1 package marked as broken and skipped:

- python27Packages.myfitnesspal

5 packages built:

- mxnet
- python27Packages.mxnet
- python37Packages.mxnet
- python37Packages.optuna
- python38Packages.mxnet

jonringer · 2020-05-06T19:21:36Z

I would approve your push, but I can't approve my own PR. Lol

jonringer · 2020-05-06T19:25:50Z

also, wouldn't it be better to get the liblapack.so from the blas.provider?

[12:24:46] jon@jon-desktop /home/jon/projects/nixpkgs (fix-mxnet)
$ nix-build -A blas.provider
/nix/store/hwdbqpqknffqnc30fjxzvrpmp2d3lcg2-openblas-0.3.9
[12:24:58] jon@jon-desktop /home/jon/projects/nixpkgs (fix-mxnet)
$ ls ./result/lib/lib*
./result/lib/libblas.so.3     ./result/lib/liblapack.so.3          ./result/lib/libopenblas.so.0
./result/lib/libcblas.so.3    ./result/lib/libopenblasp-r0.3.9.so
./result/lib/liblapacke.so.3  ./result/lib/libopenblas.so

jonringer · 2020-05-06T19:27:31Z

the main thing I see, is that the blas.provider only provides the libraries with the .3 suffix, maybe we should add symlinks to their unversioned variants

bhipple · 2020-05-06T19:29:29Z

Agreed, the current setup is a little clunky, though I'm not sure symlinkJoin would cover it: IIRC we need the actual package to be in buildInputs, so that setupHooks for doing things like adding to PKG_CONFIG_PATH and the CMake search path work.

@matthewbauer anything you think we should change here?

jonringer · 2020-05-06T19:31:45Z

actually, this is specific to openblas

    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"}

jonringer · 2020-05-06T19:58:36Z

by modifying the openblas package (in the PR that's now linked to this thread), i was able to build this while only supplying blas.provider

jonringer · 2020-05-06T20:07:29Z

currently blocked by #87134

matthewbauer · 2020-05-07T21:37:33Z

CC @matthewbauer I'm pretty sure the above is correct; I had to do something similar for pytorch. Perhaps we should add to the manual?

Yes that sounds right. Something in the manual would be helpful. We may need a "maintainers" section or something since this is more about how you make sure your package is correct vs. just how you use it.

@matthewbauer anything you think we should change here?

Adding the unversioned library seems like a good idea.

jonringer · 2020-05-08T18:18:11Z

should I just target staging and get this merged?

bhipple · 2020-05-09T02:04:25Z

Sounds good, let's just merge this on staging then so it can bake with all the other openblas / numpy changes. and we don't have to remember to circle back to it.

jonringer · 2020-05-11T18:34:24Z

@GrahamcOfBorg build mxnet

doronbehar · 2020-05-27T10:19:50Z

There's a CI error.

jonringer · 2020-05-28T05:49:09Z

@GrahamcOfBorg eval

jonringer · 2020-05-28T05:49:22Z

pretty sure staging was broken at the time of PR creation

ofborg bot requested a review from abbradar May 6, 2020 17:32

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 labels May 6, 2020

bhipple requested changes May 6, 2020

View reviewed changes

jonringer force-pushed the fix-mxnet branch 2 times, most recently from 32c28a6 to 1df1507 Compare May 6, 2020 18:32

bhipple force-pushed the fix-mxnet branch from 1df1507 to 9aacb36 Compare May 6, 2020 19:13

bhipple approved these changes May 6, 2020

View reviewed changes

jonringer mentioned this pull request May 6, 2020

openblas, mkl: also export unversioned libraries for linux #87134

Merged

10 tasks

jonringer force-pushed the fix-mxnet branch from 9aacb36 to a6e9b49 Compare May 6, 2020 20:07

matthewbauer approved these changes May 7, 2020

View reviewed changes

jonringer force-pushed the fix-mxnet branch from a6e9b49 to b838242 Compare May 11, 2020 18:33

jonringer changed the base branch from master to staging May 11, 2020 18:33

mxnet: use blas.provider, fix build

1b4bbb8

jonringer force-pushed the fix-mxnet branch from b838242 to 1b4bbb8 Compare May 11, 2020 18:49

jonringer merged commit 3a4fec8 into NixOS:staging May 28, 2020

jonringer deleted the fix-mxnet branch May 28, 2020 22:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mxnet: use openblas, fix build #87118

mxnet: use openblas, fix build #87118

jonringer commented May 6, 2020 •

edited

bhipple left a comment

bhipple May 6, 2020

bhipple May 6, 2020

jonringer May 6, 2020

jonringer May 6, 2020

jonringer May 6, 2020

bhipple May 6, 2020

bhipple May 6, 2020

bhipple left a comment

jonringer commented May 6, 2020 •

edited

jonringer commented May 6, 2020

bhipple commented May 6, 2020

jonringer commented May 6, 2020

jonringer commented May 6, 2020

jonringer commented May 6, 2020

bhipple commented May 6, 2020

jonringer commented May 6, 2020

jonringer commented May 6, 2020

jonringer commented May 6, 2020

matthewbauer commented May 7, 2020

jonringer commented May 8, 2020

bhipple commented May 9, 2020

jonringer commented May 11, 2020

doronbehar commented May 27, 2020

jonringer commented May 28, 2020

jonringer commented May 28, 2020

	, opencv3, gtest, openblas, perl
	, opencv3, gtest, blas, perl

	buildInputs = [ opencv3 gtest openblas ]
	buildInputs = [ opencv3 gtest blas blas.provider ]

mxnet: use openblas, fix build #87118

mxnet: use openblas, fix build #87118

Conversation

jonringer commented May 6, 2020 • edited

Motivation for this change

Things done

bhipple left a comment

Choose a reason for hiding this comment

bhipple May 6, 2020

Choose a reason for hiding this comment

bhipple May 6, 2020

Choose a reason for hiding this comment

jonringer May 6, 2020

Choose a reason for hiding this comment

jonringer May 6, 2020

Choose a reason for hiding this comment

jonringer May 6, 2020

Choose a reason for hiding this comment

bhipple May 6, 2020

Choose a reason for hiding this comment

bhipple May 6, 2020

Choose a reason for hiding this comment

bhipple left a comment

Choose a reason for hiding this comment

jonringer commented May 6, 2020 • edited

jonringer commented May 6, 2020

bhipple commented May 6, 2020

jonringer commented May 6, 2020

jonringer commented May 6, 2020

jonringer commented May 6, 2020

bhipple commented May 6, 2020

jonringer commented May 6, 2020

jonringer commented May 6, 2020

jonringer commented May 6, 2020

matthewbauer commented May 7, 2020

jonringer commented May 8, 2020

bhipple commented May 9, 2020

jonringer commented May 11, 2020

doronbehar commented May 27, 2020

jonringer commented May 28, 2020

jonringer commented May 28, 2020

jonringer commented May 6, 2020 •

edited

jonringer commented May 6, 2020 •

edited