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.

Sign up for GitHub

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

Jump to bottom

python3Packages.pytorch: remove oneDNN dependency #96350

Merged
merged 1 commit into from
Aug 29, 2020
Merged

python3Packages.pytorch: remove oneDNN dependency #96350

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions pkgs/development/python-modules/pytorch/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
openMPISupport ? false, openmpi ? null,
buildDocs ? false,
cudaArchList ? null,
numpy, pyyaml, cffi, click, typing, cmake, oneDNN, hypothesis, numactl, psutil,
numpy, pyyaml, cffi, click, typing, cmake, hypothesis, numactl, psutil,
linkFarm, symlinkJoin,

# virtual pkg that consistently instantiates blas across nixpkgs
Expand Down Expand Up @@ -159,9 +159,9 @@ in buildPythonPackage rec {

USE_MKL = blas.implementation == "mkl";

# Unlike MKL, MKLDNN is FOSS, so we enable support for it by default. Note
# that this was renamed to dnnl and then renamed again to oneDNN upstream, but
# pytorch still calls it by the old name mkldnn.
# Unlike MKL, oneDNN (née MKLDNN) is FOSS, so we enable support for
# it by default. PyTorch currently uses its own vendored version
# of oneDNN through Intel iDeep.
Copy link
Contributor

Choose a reason for hiding this comment

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

Sigh vendored dependencies . . . I guess it is what it is though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's a bit annoying that they do not provide some option to provide your own oneDNN. Doesn't help that CMake is hidden behind their own script that gates build flags :/.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be really nice if you could contribute a patch upstream to support non-vendored distribution deps, but in the meantime this PR seems OK as it at least faithfully represents the status quo.

Copy link
Contributor Author

@danieldk danieldk Aug 29, 2020 •
edited

Choose a reason for hiding this comment

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

I will do that if I find some time. First some more BLAS implementation fights, erm, I mean benchmarks. Intel has removed MKL_DEBUG_CPU_TYPE since MKL 2020 release 1, so Ryzen performance is abysmal. There is a workaround, but not too happy with it ;).

USE_MKLDNN = mklDnnSupport;
USE_MKLDNN_CBLAS = mklDnnSupport;

Expand Down Expand Up @@ -210,7 +210,7 @@ in buildPythonPackage rec {
ninja
] ++ lib.optionals cudaSupport [ cudatoolkit_joined ];

buildInputs = [ blas blas.provider oneDNN ]
buildInputs = [ blas blas.provider ]
++ lib.optionals cudaSupport [ cudnn magma nccl ]
++ lib.optionals stdenv.isLinux [ numactl ];

Expand Down