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

WIP: python3Packages.scipy: allow overriding BLAS #230131

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

WIP: python3Packages.scipy: allow overriding BLAS #230131

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
14e9a3a
pythonPackages: add format = "build" for pypa/build
SomeoneSerge May 5, 2023
6af7b92
python3Packages.scipy: respect blas settings
SomeoneSerge May 5, 2023
0f3bc1c
fixup! pythonPackages: add format = "build" for pypa/build
SomeoneSerge May 5, 2023
5734bfb
fixup! python3Packages.scipy: respect blas settings
SomeoneSerge May 5, 2023
28355c2
fixup! python3Packages.scipy: respect blas settings
SomeoneSerge May 5, 2023
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
9 changes: 9 additions & 0 deletions pkgs/development/interpreters/python/hooks/default.nix
View file
Open in desktop
Expand Up @@ -71,6 +71,15 @@ in {
};
} ./pip-install-hook.sh) {};

pypaBuildHook = callPackage ({ makePythonHook, build, wheel }:
makePythonHook {
name = "pypa-build-hook.sh";
propagatedBuildInputs = [ build wheel ];
substitutions = {
inherit pythonInterpreter;
};
} ./pypa-build-hook.sh) {};

pytestCheckHook = callPackage ({ makePythonHook, pytest }:
makePythonHook {
name = "pytest-check-hook";
Expand Down
19 changes: 19 additions & 0 deletions pkgs/development/interpreters/python/hooks/pypa-build-hook.sh
View file
Open in desktop
@@ -0,0 +1,19 @@
# Setup hook to use for pypa/build projects
echo "Sourcing pypa-build-hook"

pypaBuildPhase() {
echo "Executing pypaBuildPhase"
runHook preBuild

echo "Creating a wheel..."
@pythonInterpreter@ -m build --no-isolation --outdir dist/ $pypaBuildFlags
echo "Finished creating a wheel..."

runHook postBuild
echo "Finished executing pypaBuildPhase"
}

if [ -z "${dontUsePypaBuild-}" ] && [ -z "${buildPhase-}" ]; then
echo "Using pypaBuildPhase"
buildPhase=pypaBuildPhase
fi
5 changes: 4 additions & 1 deletion pkgs/development/interpreters/python/mk-python-derivation.nix
View file
Open in desktop
Expand Up @@ -14,6 +14,7 @@
, flitBuildHook
, pipBuildHook
, pipInstallHook
, pypaBuildHook
, pythonCatchConflictsHook
, pythonImportsCheckHook
, pythonNamespacesHook
Expand Down Expand Up @@ -104,7 +105,7 @@
let
inherit (python) stdenv;

withDistOutput = lib.elem format ["pyproject" "setuptools" "flit" "wheel"];
withDistOutput = lib.elem format ["pyproject" "setuptools" "build" "flit" "wheel"];

name_ = name;

Expand Down Expand Up @@ -182,6 +183,8 @@ let
unzip
] ++ lib.optionals (format == "setuptools") [
setuptoolsBuildHook
] ++ lib.optionals (format == "build") [
SomeoneSerge marked this conversation as resolved.
Show resolved Hide resolved
pypaBuildHook
] ++ lib.optionals (format == "flit") [
flitBuildHook
] ++ lib.optionals (format == "pyproject") [
Expand Down
24 changes: 21 additions & 3 deletions pkgs/development/python-modules/scipy/default.nix
View file
Open in desktop
Expand Up @@ -4,6 +4,8 @@
, python
, pythonOlder
, buildPythonPackage
, blas
, lapack
, cython
, gfortran
, meson-python
Expand All @@ -19,10 +21,15 @@
, libxcrypt
}:

assert blas.provider == numpy.blas;
Copy link
Member

Choose a reason for hiding this comment

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

no asserts, because they can block overriding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The assert here is to ensure that when one attempts an override they override both scipy and numpy. IIRC, a similar assert is used for cudaPackages in several places, so if this change is rejected, we probably should update these as well

Previously the synchronization was achieved by putting numpy.blas in buildInputs. The reason I removed numpy.blas was because numpy.blas is blas.provider rather than blas. But as mentioned earlier, I'm not yet sure how to motivate the choice between blas and blas.provider. I'll have to inspect the history of this derivation, I guess...

Copy link
Member

Choose a reason for hiding this comment

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

The assert here is to ensure that when one attempts an override they override both scipy and numpy. IIRC, a similar assert is used for cudaPackages in several places, so if this change is rejected, we probably should update these as well

The issue is explained in this older thread #36229.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't use blas.provider when I implemented multiple blas overrides in octave, because there things are a little bit more complicated - the octave expression has to coordinate between many dependencies that depend and should use the same blas and lapack. In this case, it's much simpler to my understanding, and I don't understand what's wrong with using numpy.blas which points to blas.provider. In the current state of things you can just:

scipy-myblas = super.scipy.override {
  numpy = super.numpy.override {
    blas = self.myblas;
  };
};

Which seems very nice to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the current state of things you can just ... @doronbehar

Personally, I find this rather obscure: blas is scipy's direct dependency, but we cannot just override it, we have to override numpy instead. The actual "reason" I'm making blas an explicit argument, however, is that from numpy.blas (evaluates into mkl in case of blas = prev.blas.override { blasProvider = final.mkl; }) we cannot infer the correct pkg-config target name. With the blas-switching derivation the pkg-config target is fixed at cblas, as far as I can tell.

The reason I put the assert is because previously it wasn't possible to have un-synchronized numpy.blas and scipy.blas, but with the new interface it is and even likely to happen by accident (e.g. if one had a local override like your scipy-myblas, then it could still evaluate, but scipy its propagated numpy would be silently using different BLAS implementations)

I see the convenience argument, but global overlays are same cost in LOC (assuming there's a binary cache), are safer, and this PR doesn't break them.

I also haven't looked into why numpy exposes blas.provider instead of just blas.

Further thoughts?

@FRidh I'll switch to meta.broken, and later open a PR to migrate cuda packages as well

Copy link
Member

@FRidh FRidh Jun 28, 2023 •
edited

Choose a reason for hiding this comment

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

Exactly!

And don't forget the self = pythonWithMyBlas; so withPackages functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only I think blas and lapack come from the outer package set (nixpkgs#blas, not nixpkgs#python3Packages.blas). But the idea is the same, only that it's even coarser granularity: you overlay the entire nixpkgs to achieve mutual compatibility. We can also add meta.broken = blas != numpy.blas. I still don't see any reason we expose blas.provider instead of blas in numpy.passthru, so I think we shouldn't be doing that

RE: python3.override

  pythonPackagesExtensions = prev.pythonPackagesExtensions ++ [
    (python-final: python-prev: { ... })
  ];

Copy link
Member

Choose a reason for hiding this comment

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

Only I think blas and lapack come from the outer package set (nixpkgs#blas, not nixpkgs#python3Packages.blas).

CallPackage should pick it up if I am correct

I still don't see any reason we expose blas.provider instead of blas in numpy.passthru, so I think we shouldn't be doing that

I don't know anymore why that was. Maybe that should be changed throughout?

Only I think blas and lapack come from the outer package set (nixpkgs#blas, not nixpkgs#python3Packages.blas). But the idea is the same, only that it's even coarser granularity: you overlay the entire nixpkgs to achieve mutual compatibility. We can also add meta.broken = blas != numpy.blas. I still don't see any reason we expose blas.provider instead of blas in numpy.passthru, so I think we shouldn't be doing that

RE: python3.override

  pythonPackagesExtensions = prev.pythonPackagesExtensions ++ [
    (python-final: python-prev: { ... })
  ];

That would have an effect on the entire nixpkgs then (all interpreters + every package using any of these).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, so we could just add the blas attribute to the python package set via an overlay, is that what you're saying? And in either case it would be picked up by callPackage.

Copy link
Member

Choose a reason for hiding this comment

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

Exactly!


buildPythonPackage rec {
pname = "scipy";
version = "1.10.1";
format = "pyproject";

# Required to pass -Dblas options
# https://github.com/scipy/scipy/issues/17244
format = "build";
SomeoneSerge marked this conversation as resolved.
Show resolved Hide resolved

src = fetchPypi {
inherit pname version;
Expand All @@ -39,7 +46,8 @@ buildPythonPackage rec {
nativeBuildInputs = [ cython gfortran meson-python pythran pkg-config wheel ];

buildInputs = [
numpy.blas
blas
lapack
pybind11
pooch
] ++ lib.optionals (pythonOlder "3.9") [
Expand Down Expand Up @@ -82,7 +90,17 @@ buildPythonPackage rec {
blas = numpy.blas;
};

setupPyBuildFlags = [ "--fcompiler='gnu95'" ];
SomeoneSerge marked this conversation as resolved.
Show resolved Hide resolved
pypaBuildFlags = [
# Current release pins pybind11's patch version and ours is newer.
# Consider restoring build-time dependency check later
"--skip-dependency-check"
SomeoneSerge marked this conversation as resolved.
Show resolved Hide resolved

# Skip sdist
"--wheel"

"-Csetup-args=-Dblas=cblas"
"-Csetup-args=-Dlapack=lapacke"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This uses the .pc file generated by build-support/alternatives/blas/. I am not sure if that's what we want to use, and I'm entirely not sure when build-support/alternatives/blas should be used.

For comparison:

❯ pkg-config mkl-dynamic-ilp64-gomp --libs
-L/nix/store/ryqzxrbdabpjxbjw97w8x6nj4fg0qhcx-mkl-2023.0.0.25398/lib -Wl,--no-as-needed -lmkl_intel_ilp64 -lmkl_gnu_thread -lmkl_core -lgomp -lpthread -lm -ldl
❯ pkg-config cblas --libs
-L/nix/store/byi7kk3jd4nblhwbzavf82pc5p4s34b1-blas-3/lib -lcblas

Note that MKL does not seem to distribute a mkl.pc file, instead it ships:

❯ pkg-config --list-all
mkl-dynamic-ilp64-gomp mkl - Intel(R) oneAPI Math Kernel Library
mkl-dynamic-ilp64-tbb  mkl - Intel(R) oneAPI Math Kernel Library
mkl-dynamic-lp64-iomp  mkl - Intel(R) oneAPI Math Kernel Library
mkl-dynamic-lp64-seq   mkl - Intel(R) oneAPI Math Kernel Library
mkl-dynamic-ilp64-iomp mkl - Intel(R) oneAPI Math Kernel Library
mkl-dynamic-ilp64-seq  mkl - Intel(R) oneAPI Math Kernel Library
mkl-dynamic-lp64-gomp  mkl - Intel(R) oneAPI Math Kernel Library
mkl-dynamic-lp64-tbb   mkl - Intel(R) oneAPI Math Kernel Library

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm concerned whether using these cblas.pc and lapacke.pc allows for static linkage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIU, blas.override { blasProvider = mkl; } is somehow a way to expose mkl_rt the "single dynamic library", and libcblas.so in that derivation is a copy of libblas.so from mkl? What I do not understand if scipy is expected to link any of MKL statically, and whether the blas switching mechanism supports static linkage.

@matthewbauer I see you worked both on blas switching, and on blas/lapack in numpy. Any hints?

];

SCIPY_USE_G77_ABI_WRAPPER = 1;

Expand Down