dnnl: init at 1.0.2 #68014

alexarice · 2019-09-03T15:48:18Z

Motivation for this change

Things done

I can't get the tests to build atm here is the log
https://gist.github.com/alexarice/5fde03409d660638036d3a0b6fedf704

pkgs/development/libraries/mkldnn/default.nix

alexarice · 2019-09-03T18:55:50Z

@jonringer Do you know if my patch to change /bin/bash to /bin/sh is the correct thing to do? would it be better to hard link to bash via storepath

jonringer · 2019-09-03T19:04:06Z

I personally would do a substitueAll, see pyproj/default.nix and the related patch as an example

alexarice · 2019-09-03T19:08:28Z

Removing the checkPhase did not fix the tests, should I just set doCheck = false?

jonringer · 2019-09-03T19:13:45Z

the tests are failing because of:

51/51 Test #51: test_api ......................................***Failed    0.00 sec
/build/source/build/tests/gtests/api/test_api: error while loading shared libraries: libmkldnn.so.1: cannot open shared object file: No such file or directory

We just need the tests to link against the shared library that gets produced.

alexarice · 2019-09-03T19:14:39Z

Do you know how to do that?

jonringer · 2019-09-03T19:33:17Z

The shared libraries seem to be written to $PWD/src, modifying LD_LIBRARY_PATH seems to do nothing. This should help though:

checkInputs = [ autoPatchelfHook ];
  preCheck = ''
    addAutoPatchelfSearchPath $PWD/src
    autoPatchelf tests examples
  '';

jonringer · 2019-09-03T19:33:59Z

also, take note that this library may be highly coupled to intel processors, it might not work for AMD.

alexarice · 2019-09-03T19:38:24Z

I'll try that in a bit, my laptop is Intel I think and the tests worked in a nix shell

alexarice · 2019-09-03T21:34:45Z

More tests pass but not all of them

pkgs/development/libraries/mkldnn/default.nix

alexarice · 2019-09-03T22:08:25Z

All test pass now, thanks for all the help

jonringer

Don't forget to resolve conversations if you made the suggested changes :)

pkgs/development/libraries/mkldnn/default.nix

jonringer

nix-review passes on NixOS
diff LGTM (Although should squash all changes to a single commit)
leaf package

[1 built, 0.0 MiB DL]
https://github.com/NixOS/nixpkgs/pull/68014
1 package were build:
mkldnn

[nix-shell:~/.cache/nix-review/pr-68014-1]$ nix path-info -Sh ./results/mkldnn
/nix/store/r22kq0jvvxgndcyywvddgcm7sfc2cq4n-mkldnn-1.0.2          48.8M

alexarice · 2019-09-04T01:40:06Z

Shall I add myself as a maintainer?

jonringer · 2019-09-04T01:41:23Z

It's up to you, but since you're familiar with how to build it to some degree, i would say that's some familiarity. Ideally a maintainer is someone with a vested interest in using the package as well.

alexarice · 2019-09-04T02:25:19Z

The issue also mentioned adding this to the nativeBuildInputs of pytorch so I'll add this to this pr

jonringer · 2019-09-04T07:13:06Z

it's a shared library, it would need to be added to build inputs

jonringer · 2019-09-04T07:15:21Z

undefined variable 'alexarice' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/eval-2-shlevy.ewr1.nix.ci/pkgs/development/libraries/mkldnn/default.nix:35:21

you need to add yourself as a maintainer see c3c8f17#diff-498fa90aaf17420b9719f0b04e1c3d08 for an example

your commit history should look like:

maintainers: add alexarice
mkldnn: init at 1.0.2

alexarice · 2019-09-04T08:06:42Z

undefined variable 'alexarice' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/eval-2-shlevy.ewr1.nix.ci/pkgs/development/libraries/mkldnn/default.nix:35:21
you need to add yourself as a maintainer see c3c8f17#diff-498fa90aaf17420b9719f0b04e1c3d08 for an example

your commit history should look like:
maintainers: add alexarice
mkldnn: init at 1.0.2

The issue isn't that I'm not a maintainer, it's that I was tired and didn't write with maintainers;

pkgs/development/libraries/mkldnn/default.nix

bhipple · 2019-09-06T23:26:36Z

pkgs/development/libraries/mkldnn/default.nix

+    inherit bash;
+  }) ];
+
+  nativeBuildInputs = [ cmake ];


mkl-dnn can optionally be built against the mkl package:
https://github.com/intel/mkl-dnn/blob/master/cmake/MKL.cmake

This has a proprietary license so it's not built/distributed by default in NixPkg and Hydra, but it is packaged. For users who want to use it it might be nice to give them an optional switch, like we do with numpy:
https://nixos.org/nixpkgs/manual/#how-to-use-intels-mkl-with-numpy-and-scipy

I'm slightly confused, should this have blas as an input?

jmillerpdt

I've attempted to provide details on integrating Intel MKL below.

pkgs/development/libraries/dnnl/default.nix

jmillerpdt · 2019-09-10T16:32:56Z

pkgs/development/libraries/dnnl/default.nix

+    addAutoPatchelfSearchPath $PWD/src
+    autoPatchelf tests examples
+  '';
+


cmake should be able to find them if you override the install directory:

cmakeFlags = [ "-DMAKE_INSTALL_PREFIX=$out" ];

This doesn't seem to work

The checkPhase runs after build, but before install, so $out should still be empty when this gets called

Another options would be

preCheck = ''export LD_LIBRARY_PATH=$PWD/src:$LD_LIBRARY_PATH'';

When i tried originally, i left off the export, and that seems to make a difference in some cases.

Problem with this, is that during the fixup, it might then discover the built shared libraries and try to link against them in the build directory, which is not what we want, and the derivation will fail

perhaps:

checkPhase = '' LD_LIBRARY_PATH=$PWD/src:$LD_LIBRARY_PATH make check '';

would work best

strangely I didn't need to do anything when I built it in a nix-shell

if you did nix-shell --pure then that might not be the case.

even then it works, I can't work it out. I even messed around to get it to run tree in the checkphase and all the files look identical

well, if it works, then it works. I just remember getting the "libdnn.so" cannot be found error

In a nutshell, I believe the behavior is broken because of CMAKE_SKIP_BUILD_RPATH=ON
(Nix open issue and workarounds here: #22060).

I think you can resolve this with your solution or adopt one of the others from that thread.

timokau · 2019-10-19T08:35:04Z

pkgs/development/libraries/dnnl/default.nix

+
+  nativeBuildInputs = [ cmake ];
+
+  buildInputs = lib.optional (mkl != null) mkl;


Would something like this be appropriate here?

DNNL can be used outside of a Python context, so I'm afraid that won't work in all circumstances (this doesn't depend on numpy).

One can either rely on MKL being allowed in the unfree list (which allows this to work in both mkl and non-mkl situations), as is done here, or it could be enabled by a dedicated flag and require downstream libraries to opt-in.

It would be nice if Nix had a base environment configuration for these numerics, since generally you want consistent libraries across the whole of the stack rather approaching these on a per-library basis.

One can either rely on MKL being allowed in the unfree list

I'M not sure what you mean by this. As far as I know in the current implementation nix would just always try to fulfill the MKL dependency, throwing an error when unfree packages are disabled. One could get around that by explicitly passing null, but a proper switch would be nicer.

@timokau is right: a quick test demonstrates that this will fail to build if you don't have the unfree packages enabled. Since callPackage will fill in mkl from all-packages.nix, the default value inside dnnl/default.nix has no effect.

An additional issue is that dnnl's CMake build requires a non-default flag to use MKL: https://github.com/intel/mkl-dnn/blob/master/cmake/options.cmake#L157

This PR follows up on what's implemented here and fixes the above two issues:
#74893

bhipple

This has two issues (see above comment and follow-up PR which fixes them).

This commit continues the work proposed in NixOS#68014, and provides an entirely FOSS variant of the `dnnl` package. Updates to add an `mkl` flavor will be added later, pending discussion about the cleanest way to overlay this consistently. Fixes NixOS#67982, closes NixOS#68014 Co-authored-by: Alex Rice <alexrice999@hotmail.co.uk>

This commit continues the work proposed in #68014, and provides an entirely FOSS variant of the `dnnl` package. Updates to add an `mkl` flavor will be added later, pending discussion about the cleanest way to overlay this consistently. Fixes #67982, closes #68014 Co-authored-by: Alex Rice <alexrice999@hotmail.co.uk>

This commit continues the work proposed in NixOS#68014, and provides an entirely FOSS variant of the `dnnl` package. Updates to add an `mkl` flavor will be added later, pending discussion about the cleanest way to overlay this consistently. Fixes NixOS#67982, closes NixOS#68014 Co-authored-by: Alex Rice <alexrice999@hotmail.co.uk> (cherry picked from commit d411938)

alexarice force-pushed the mkldnn-init branch 2 times, most recently from 23536b2 to ca052a6 Compare September 3, 2019 16:20

ofborg bot added 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Sep 3, 2019

alexarice force-pushed the mkldnn-init branch from ca052a6 to d6535c7 Compare September 3, 2019 17:10

jonringer reviewed Sep 3, 2019

View reviewed changes

pkgs/development/libraries/mkldnn/default.nix Outdated Show resolved Hide resolved

pkgs/development/libraries/mkldnn/default.nix Outdated Show resolved Hide resolved

alexarice force-pushed the mkldnn-init branch from d6535c7 to ee4c957 Compare September 3, 2019 21:34

jonringer reviewed Sep 3, 2019

View reviewed changes

pkgs/development/libraries/mkldnn/default.nix Outdated Show resolved Hide resolved

alexarice force-pushed the mkldnn-init branch from ee4c957 to d129194 Compare September 3, 2019 22:07

alexarice changed the title ~~[WIP] mkldnn: init at 1.0.2~~ mkldnn: init at 1.0.2 Sep 3, 2019

jonringer reviewed Sep 4, 2019

View reviewed changes

pkgs/development/libraries/mkldnn/default.nix Outdated Show resolved Hide resolved

pkgs/development/libraries/mkldnn/default.nix Outdated Show resolved Hide resolved

jonringer approved these changes Sep 4, 2019

View reviewed changes

alexarice force-pushed the mkldnn-init branch from f0369dc to abfef23 Compare September 4, 2019 01:47

alexarice force-pushed the mkldnn-init branch from abfef23 to c8d257b Compare September 4, 2019 08:13

ofborg bot added the 11.by: package-maintainer label Sep 4, 2019

bhipple requested changes Sep 6, 2019

View reviewed changes

stites mentioned this pull request Sep 7, 2019

pytorch: 1.0.0 -> 1.2.0 #65041

Merged

10 tasks

alexarice force-pushed the mkldnn-init branch from c8d257b to 60c2ebe Compare September 10, 2019 09:36

alexarice changed the title ~~mkldnn: init at 1.0.2~~ dnnl: init at 1.0.2 Sep 10, 2019

lheckemann added this to the 20.03 milestone Sep 10, 2019

jmillerpdt suggested changes Sep 10, 2019

View reviewed changes

alexarice added 2 commits September 10, 2019 19:11

dnnl: init at 1.0.2

533af73

dnnl: Add alias from mkldnn

6cac33c

alexarice force-pushed the mkldnn-init branch from 60c2ebe to 6cac33c Compare September 10, 2019 18:11

jmillerpdt mentioned this pull request Sep 26, 2019

pythonPackages.tensorflow: fix for bazel settings for intel mkl, dnnl #69454

Merged

10 tasks

bhipple approved these changes Sep 29, 2019

View reviewed changes

timokau reviewed Oct 19, 2019

View reviewed changes

bhipple mentioned this pull request Dec 3, 2019

dnnl: init at 1.1.2 #74893

Merged

10 tasks

bhipple requested changes Dec 3, 2019

View reviewed changes

jonringer closed this in #74893 Jan 24, 2020


		nativeBuildInputs = [ cmake ];

		buildInputs = lib.optional (mkl != null) mkl;

dnnl: init at 1.0.2 #68014

dnnl: init at 1.0.2 #68014

Conversation

alexarice commented Sep 3, 2019

Motivation for this change

Things done

alexarice commented Sep 3, 2019

jonringer commented Sep 3, 2019 • edited

alexarice commented Sep 3, 2019

jonringer commented Sep 3, 2019

alexarice commented Sep 3, 2019

jonringer commented Sep 3, 2019 • edited

jonringer commented Sep 3, 2019

alexarice commented Sep 3, 2019

alexarice commented Sep 3, 2019

alexarice commented Sep 3, 2019

jonringer left a comment

Choose a reason for hiding this comment

jonringer left a comment

Choose a reason for hiding this comment

alexarice commented Sep 4, 2019

jonringer commented Sep 4, 2019

alexarice commented Sep 4, 2019

jonringer commented Sep 4, 2019

jonringer commented Sep 4, 2019

alexarice commented Sep 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmillerpdt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonringer Sep 10, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexarice Sep 10, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhipple left a comment

Choose a reason for hiding this comment

jonringer commented Sep 3, 2019 •

edited

jonringer commented Sep 3, 2019 •

edited

alexarice commented Sep 4, 2019 •

edited

jonringer Sep 10, 2019 •

edited

alexarice Sep 10, 2019 •

edited