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

mkl: init at 2019.0.117 #47182

Merged
merged 3 commits into from Oct 15, 2018
Merged

mkl: init at 2019.0.117 #47182

merged 3 commits into from Oct 15, 2018

Conversation

bhipple
Copy link
Contributor

@bhipple bhipple commented Sep 22, 2018

This packags the Intel Math Kernel library on x86-64 platforms, which is a
dependency for many data science and machine learning packages.

Upstream, Intel provides proprietary binary RPMs with a permissive
redistribution license. These have been repackaged in both Debian and Anaconda,
so we are not the first distribution to redistribute.

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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

This packags the Intel Math Kernel library on x86-64 platforms, which is a
dependency for many data science and machine learning packages.

Upstream, Intel provides proprietary binary RPMs with a permissive
redistribution license. These have been repackaged in both Debian and Anaconda,
so we are not the first distribution to redistribute.
@bhipple
Copy link
Contributor Author

bhipple commented Sep 22, 2018

@GrahamcOfBorg build mkl

@GrahamcOfBorg
Copy link

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: mkl

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnfree = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnfree = true; }
to ~/.config/nixpkgs/config.nix.


@GrahamcOfBorg
Copy link

No attempt on x86_64-linux (full log)

The following builds were skipped because they don't evaluate on x86_64-linux: mkl

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnfree = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnfree = true; }
to ~/.config/nixpkgs/config.nix.


@GrahamcOfBorg
Copy link

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: mkl

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnfree = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnfree = true; }
to ~/.config/nixpkgs/config.nix.


@GrahamcOfBorg
Copy link

No attempt on x86_64-linux (full log)

The following builds were skipped because they don't evaluate on x86_64-linux: mkl

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnfree = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnfree = true; }
to ~/.config/nixpkgs/config.nix.


@bhipple
Copy link
Contributor Author

bhipple commented Sep 22, 2018

I think borg doesn't like it because it's an unfree package? The platform matches on NixOS.

threading models.
'';
homepage = https://software.intel.com/en-us/mkl;
license = licenses.issl;
Copy link
Contributor

@orivej orivej Sep 22, 2018

Choose a reason for hiding this comment

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

We should probably use licenses.unfreeRedistributable to build this on Hydra, or extend isUnfree to support arbitrary unfree redistributable licenses.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with

We should probably use licenses.unfreeRedistributable to build this on Hydra

but not with

or extend isUnfree to support arbitrary unfree redistributable licenses.

because then they would be probably be installed with allowUnfree = false;, which is not what users expect.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant adding an attribute redistributable = true alongside unfree = false; allowUnfree = false will not install such packages.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine, sorry I misunderstood.

Copy link
Contributor Author

@bhipple bhipple Sep 23, 2018

Choose a reason for hiding this comment

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

Actually, are we sure unfreeRedistributable is working as intended? I've started digging in, and it looks like Hydra doesn't actually build any of these things. I spot-checked 5-10 packages and didn't see any of them on the hydra nixpkgs jobset. For example, consider the package aliza, which is nowhere to be found on Hydra despite having an unfreeRedistributable license. Looking at the source code on hydra and the meta evaluator, it looks like there's no extra condition for free = false but redistributable.

There's another license, unfreeRedistributableFirmware, that is being built and distributed -- but the reason for that is that it declares itself as free = true.

At any rate, I think we can cleanup some of this logic and fix these issues by updating Hydra with an unfreePredicate filter that allows through redistributable packages. There's an initial WIP RFC here

@bhipple
Copy link
Contributor Author

bhipple commented Sep 23, 2018

I like the idea of adding a redistribute boolean attribute to licenses for Hydra. Where is that checked? In the meantime I submitted #47230, which I think is a no-op cleanup of the existing logic (at least as far as I'm reading it; please correct if mistaken).

EDIT: I found it and will take a look at making the redistribute attribute for distribution: https://github.com/NixOS/nixpkgs/blob/master/pkgs/top-level/release.nix#L18-L19

@GrahamcOfBorg
Copy link

No attempt on x86_64-linux (full log)

The following builds were skipped because they don't evaluate on x86_64-linux: mkl

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnfree = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnfree = true; }
to ~/.config/nixpkgs/config.nix.


@GrahamcOfBorg
Copy link

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: mkl

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnfree = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnfree = true; }
to ~/.config/nixpkgs/config.nix.


@bhipple
Copy link
Contributor Author

bhipple commented Sep 26, 2018

I opened a separate PR to start taking a look at unfreeRedistributable and some potential license refactorings, but in the meantime I don't think it's much of a problem if Hydra does not build and distribute this. After all, the only thing the "build" is doing is transmuting one binary package into another.

@GrahamcOfBorg
Copy link

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: mkl

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnfree = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnfree = true; }
to ~/.config/nixpkgs/config.nix.


@GrahamcOfBorg
Copy link

No attempt on x86_64-linux (full log)

The following builds were skipped because they don't evaluate on x86_64-linux: mkl

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnfree = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnfree = true; }
to ~/.config/nixpkgs/config.nix.


@xeji
Copy link
Contributor

xeji commented Sep 26, 2018

After all, the only thing the "build" is doing is transmuting one binary package into another.

I suggest to set preferLocalBuild = true; to disable Hydra builds even if Hydra would build packages with an unfree-redistributable license. It's >800M download after all, and the "build" operations are really trivial. It makes no sense to put it in the binary cache.

@orivej
Copy link
Contributor

orivej commented Sep 26, 2018

It's >800M download after all, and the "build" operations are really trivial. It makes no sense to put it in the binary cache.

I don't know which is better, but there is a benefit of having it in the binary cache in saving user's disk space: if they build it themselves, they'll store both the source archives and the unpacked content.

Cache efficiency can be improved by moving the extracted content into a fixed output derivation (e.g. by replacing fetchurl with fetchzip with a custom postFetch script, as is common in fonts) and symlinking the content into the final openmp-dependent derivation.

@xeji
Copy link
Contributor

xeji commented Sep 26, 2018

Cache efficiency can be improved by moving the extracted content into a fixed output derivation (e.g. by replacing fetchurl with fetchzip with a custom postFetch script, as is common in fonts) and symlinking the content into the final openmp-dependent derivation.

Good idea! Couldn't we do that locally to avoid duplication in the local store without involving the binary cache?

@orivej
Copy link
Contributor

orivej commented Sep 26, 2018

Good idea! Couldn't we do that locally to avoid duplication in the local store without involving the binary cache?

My point was that Hydra would cache the fixed-output derivation once per version (rather than once per stdenv/openmp rebuild), but yes, this will also be space-efficient with preferLocalBuild = true;.

@bhipple
Copy link
Contributor Author

bhipple commented Sep 27, 2018

It's >800M download after all, and the "build" operations are really trivial. It makes no sense to put it in the binary cache.

Aside from the benefit @orivej mentioned with the double-space use, there's a second benefit for this particular use case: Intel distributes the src package as a massive tar of all the architecture binaries, and we're filtering it to a subset of what we need -- so the final binary package is actually smaller than the starting "source" package (though still huge):

λ brh nixpkgs →  du -sh $(nix-build . -A mkl.src)
836M    /nix/store/xlwgac8h9s1rpqjp3ycwzza2bnzm61yv-l_mkl_2019.0.117.tgz

λ brh nixpkgs →  du -sh $(nix-build . -A mkl)
626M    /nix/store/p2qhmh8fn94p8fwy24d46y7fl5lcyh8k-mkl-2019.0.117

I'd prefer to omit preferLocalBuild for that reason, plus I'd like my own machines to share the binary with each other, plus it's irrelevant (for now) for Hydra. The idea with the fixed output fetch for the initial src tarball seems nice; I've never looked at those and have been meaning to investigate how they work for things like TexLive. Is there some documentation I can read up on them? It looks to me like that fetchzip package still depends on stdenv and would be rebuilt on a change?

@smaret
Copy link
Member

smaret commented Sep 27, 2018

I was able to build this expression on Darwin with the following changes:

  src = fetchurl {
    url = "http://registrationcenter-download.intel.com/akdlm/irc_nas/tec/13565/m_mkl_${version}.dmg";
    sha256 = "1f1jppac7vqwn00hkws0p4njx38ajh0n25bsjyb5d7jcacwfvm02";
  };
  buildPhase = ''for f in Contents/Resources/pkg/*.tgz; do tar xzvf $f; done'' ;
 installPhase = ''
     mkdir -p $out/lib
     cp -r compilers_and_libraries_${version}/mac/mkl/include $out/
     cp -r compilers_and_libraries_${version}/mac/mkl/lib/* $out/lib/
     cp -r compilers_and_libraries_${version}/licensing/mkl/en/license.txt $out/lib/
  '';

undmg also needs to be added to the buildInputs.

Perhaps the package could be modified to support both Linux and Darwin? I'm not familiar enough with nix expressions to do it myself, unfortunately.

@bhipple
Copy link
Contributor Author

bhipple commented Oct 3, 2018

I tossed in Darwin support as well, using your commands. I can confirm this still works on Linux, but I don't have access to a Darwin machine to test; would you mind giving it a try to make sure I didn't miss something in the conditionals?

I also swapped this to use rpmextract (which is a tiny script that does the command I was piping to cpio) and added both licenses for the redistributable (even though it's currently a no-op).

Regarding openmp, we could omit it entirely, which would simplify the expression and reduce the dependencies. Not everything that needs mkl needs a section that needs openmp, so we could have libraries/applications themselves add to LD_LIBRARY_PATH with wrapProgram. Is that preferable? That said, since the need of openmp is actually in mkl itself, it seems sensible to export it and have it "just work" without the caller having to know that.

@GrahamcOfBorg
Copy link

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: mkl

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnfree = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnfree = true; }
to ~/.config/nixpkgs/config.nix.


@GrahamcOfBorg
Copy link

No attempt on x86_64-linux (full log)

The following builds were skipped because they don't evaluate on x86_64-linux: mkl

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnfree = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnfree = true; }
to ~/.config/nixpkgs/config.nix.


@smaret
Copy link
Member

smaret commented Oct 3, 2018

I can confirm this still works on Linux, but I don't have access to a Darwin machine to test; would you mind giving it a try to make sure I didn't miss something in the conditionals?

Done, it works fine on Darwin. Thanks!

@orivej
Copy link
Contributor

orivej commented Oct 3, 2018

Is there some documentation I can read up on them?

There is https://nixos.org/nix/manual/#fixed-output-drvs

It looks to me like that fetchzip package still depends on stdenv and would be rebuilt on a change?

Derivations that specify outputHash are not rebuilt when their dependencies change.

outputHashAlgo = "sha256";
outputHashMode = "recursive";
outputHash = if stdenvNoCC.isDarwin
then "0000000000000000000000000000000000000000000000000000"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smaret can you try building this again and let me know what the fixed-output hash is for Mac?

Copy link
Member

Choose a reason for hiding this comment

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

Here it is: "1224dln7n8px1rk8biiggf77wjhxh8mzw0hd8zlyjm8i6j8w7i12"

@bhipple
Copy link
Contributor Author

bhipple commented Oct 4, 2018

That was easy! Since the majority of mkl doesn't actually need libomp and applications that use those portions of mkl are already expected to manipulate LD_LIBRARY_PATH [1], I think we can just make this entire thing a fixed-output derivation and save a ton of build time + space. This seems a lot cleaner to me; let me know what you guys think.

[1] For example, see: https://github.com/oxfordcontrol/osqp/blob/master/docs/get_started/linear_system_solvers.rst#install-with-anaconda

@GrahamcOfBorg
Copy link

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: mkl

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnfree = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnfree = true; }
to ~/.config/nixpkgs/config.nix.


@GrahamcOfBorg
Copy link

No attempt on x86_64-linux (full log)

The following builds were skipped because they don't evaluate on x86_64-linux: mkl

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnfree = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnfree = true; }
to ~/.config/nixpkgs/config.nix.


@GrahamcOfBorg
Copy link

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: mkl

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnfree = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnfree = true; }
to ~/.config/nixpkgs/config.nix.


@GrahamcOfBorg
Copy link

No attempt on x86_64-linux (full log)

The following builds were skipped because they don't evaluate on x86_64-linux: mkl

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnfree = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnfree = true; }
to ~/.config/nixpkgs/config.nix.


@bhipple
Copy link
Contributor Author

bhipple commented Oct 15, 2018

CC @xeji @orivej @matthewbauer anyone have any further review feedback? I think this is much polished from the initial cut and should be good to go now, but as always am interested if there are ways to improve it.

@matthewbauer
Copy link
Member

Looks alright to me!

@matthewbauer matthewbauer merged commit 0b298d5 into NixOS:master Oct 15, 2018
@bhipple bhipple deleted the add/mkl branch February 27, 2019 23:04
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

7 participants