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

dnnl: init at 1.0.2 #68014

Closed
wants to merge 2 commits into from
Closed

dnnl: init at 1.0.2 #68014

wants to merge 2 commits into from

Conversation

@alexarice
Copy link
Contributor

alexarice commented Sep 3, 2019

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.

fixes #67982

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

@alexarice alexarice force-pushed the alexarice:mkldnn-init branch 2 times, most recently from 23536b2 to ca052a6 Sep 3, 2019
@alexarice alexarice force-pushed the alexarice:mkldnn-init branch from ca052a6 to d6535c7 Sep 3, 2019
@alexarice

This comment has been minimized.

Copy link
Contributor Author

alexarice commented Sep 3, 2019

@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

This comment has been minimized.

Copy link
Contributor

jonringer commented Sep 3, 2019

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

@alexarice

This comment has been minimized.

Copy link
Contributor Author

alexarice commented Sep 3, 2019

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

@jonringer

This comment has been minimized.

Copy link
Contributor

jonringer commented Sep 3, 2019

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

This comment has been minimized.

Copy link
Contributor Author

alexarice commented Sep 3, 2019

Do you know how to do that?

@jonringer

This comment has been minimized.

Copy link
Contributor

jonringer commented Sep 3, 2019

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

This comment has been minimized.

Copy link
Contributor

jonringer commented Sep 3, 2019

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

@alexarice

This comment has been minimized.

Copy link
Contributor Author

alexarice commented Sep 3, 2019

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

@alexarice alexarice force-pushed the alexarice:mkldnn-init branch from d6535c7 to ee4c957 Sep 3, 2019
@alexarice

This comment has been minimized.

Copy link
Contributor Author

alexarice commented Sep 3, 2019

More tests pass but not all of them

@alexarice alexarice force-pushed the alexarice:mkldnn-init branch from ee4c957 to d129194 Sep 3, 2019
@alexarice

This comment has been minimized.

Copy link
Contributor Author

alexarice commented Sep 3, 2019

All test pass now, thanks for all the help

@alexarice alexarice changed the title [WIP] mkldnn: init at 1.0.2 mkldnn: init at 1.0.2 Sep 3, 2019
Copy link
Contributor

jonringer left a comment

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

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

jonringer left a comment

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

This comment has been minimized.

Copy link
Contributor Author

alexarice commented Sep 4, 2019

Shall I add myself as a maintainer?

@jonringer

This comment has been minimized.

Copy link
Contributor

jonringer commented Sep 4, 2019

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 alexarice force-pushed the alexarice:mkldnn-init branch from f0369dc to abfef23 Sep 4, 2019
@alexarice

This comment has been minimized.

Copy link
Contributor Author

alexarice commented Sep 4, 2019

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

@jonringer

This comment has been minimized.

Copy link
Contributor

jonringer commented Sep 4, 2019

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

@jonringer

This comment has been minimized.

Copy link
Contributor

jonringer commented Sep 4, 2019

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

This comment has been minimized.

Copy link
Contributor Author

alexarice commented Sep 4, 2019

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;

@alexarice alexarice force-pushed the alexarice:mkldnn-init branch from abfef23 to c8d257b Sep 4, 2019
pkgs/development/libraries/mkldnn/default.nix Outdated Show resolved Hide resolved
inherit bash;
}) ];

nativeBuildInputs = [ cmake ];

This comment has been minimized.

Copy link
@bhipple

bhipple Sep 6, 2019

Contributor

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

This comment has been minimized.

Copy link
@alexarice

alexarice Sep 10, 2019

Author Contributor

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

@stites stites mentioned this pull request Sep 7, 2019
5 of 10 tasks complete
@alexarice alexarice force-pushed the alexarice:mkldnn-init branch from c8d257b to 60c2ebe Sep 10, 2019
@alexarice alexarice changed the title mkldnn: init at 1.0.2 dnnl: init at 1.0.2 Sep 10, 2019
@lheckemann lheckemann added this to the 20.03 milestone Sep 10, 2019
Copy link

jmillerpdt left a comment

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

addAutoPatchelfSearchPath $PWD/src
autoPatchelf tests examples
'';

This comment has been minimized.

Copy link
@jmillerpdt

jmillerpdt Sep 10, 2019

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

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

This comment has been minimized.

Copy link
@alexarice

alexarice Sep 10, 2019

Author Contributor

This doesn't seem to work

This comment has been minimized.

Copy link
@jonringer

jonringer Sep 10, 2019

Contributor

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

This comment has been minimized.

Copy link
@jonringer

jonringer Sep 10, 2019

Contributor

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

This comment has been minimized.

Copy link
@jonringer

jonringer Sep 10, 2019

Contributor

perhaps:

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

would work best

This comment has been minimized.

Copy link
@alexarice

alexarice Sep 10, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@jonringer

jonringer Sep 10, 2019

Contributor

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

This comment has been minimized.

Copy link
@alexarice

alexarice Sep 10, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@jonringer

jonringer Sep 11, 2019

Contributor

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

This comment has been minimized.

Copy link
@jmillerpdt

jmillerpdt Sep 25, 2019

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.

alexarice added 2 commits Sep 3, 2019
@alexarice alexarice force-pushed the alexarice:mkldnn-init branch from 60c2ebe to 6cac33c Sep 10, 2019

nativeBuildInputs = [ cmake ];

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

This comment has been minimized.

Copy link
@timokau

timokau Oct 19, 2019

Member

Would something like this be appropriate here?

This comment has been minimized.

Copy link
@jmillerpdt

jmillerpdt Oct 20, 2019

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.

This comment has been minimized.

Copy link
@timokau

timokau Oct 20, 2019

Member

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.

This comment has been minimized.

Copy link
@bhipple

bhipple Dec 3, 2019

Contributor

@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 added a commit to bhipple/nixpkgs that referenced this pull request Dec 3, 2019
This commit continues the work proposed in NixOS#68014, but fixes the issue with the
proprietary `mkl` defaults. Since `mkl` is a heavyweight proprietary package, we
only want to enable it when the user explicitly requests it.

By default, `dnnl` will be built with all FOSS packages on Hydra. The
proprietary `mkl` variant can be enabled easily via an overlay for users who
wish to use it.

Fixes NixOS#67982, closes NixOS#68014

Co-authored-by: Alex Rice <alexrice999@hotmail.co.uk>
bhipple added a commit to bhipple/nixpkgs that referenced this pull request Dec 3, 2019
This commit continues the work proposed in NixOS#68014, but fixes the issue with the
proprietary `mkl` defaults. Since `mkl` is a heavyweight proprietary package, we
only want to enable it when the user explicitly requests it.

By default, `dnnl` will be built with all FOSS packages on Hydra. The
proprietary `mkl` variant can be enabled easily via an overlay for users who
wish to use it.

Fixes NixOS#67982, closes NixOS#68014

Co-authored-by: Alex Rice <alexrice999@hotmail.co.uk>
@bhipple bhipple mentioned this pull request Dec 3, 2019
3 of 10 tasks complete
Copy link
Contributor

bhipple left a comment

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

bhipple added a commit to bhipple/nixpkgs that referenced this pull request Jan 9, 2020
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>
bhipple added a commit to bhipple/nixpkgs that referenced this pull request Jan 9, 2020
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>
bhipple added a commit to bhipple/nixpkgs that referenced this pull request Jan 11, 2020
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>
bhipple added a commit to bhipple/nixpkgs that referenced this pull request Jan 12, 2020
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>
bhipple added a commit to bhipple/nixpkgs that referenced this pull request Jan 12, 2020
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>
jonringer added a commit that referenced this pull request Jan 24, 2020
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>
dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Jan 24, 2020
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)
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.

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