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 all 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
1 change: 1 addition & 0 deletions 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
43 changes: 39 additions & 4 deletions pkgs/development/python-modules/scipy/default.nix
View file
Open in desktop
@@ -1,9 +1,12 @@
{ lib
, stdenv
, fetchPypi
, pypaBuildHook
, python
, pythonOlder
, buildPythonPackage
, blas
, lapack
, cython
, gfortran
, meson-python
Expand All @@ -19,6 +22,8 @@
, 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";
Expand All @@ -36,10 +41,33 @@ buildPythonPackage rec {
./disable-datasets-tests.patch
];

nativeBuildInputs = [ cython gfortran meson-python pythran pkg-config wheel ];
# The pybind11 issue seems to have already been address by 2.10.3
# https://github.com/pybind/pybind11/issues/4420
#
# We should update pythran
#
# Numpy is pinned at patch versions, probably as a way to choose from a set
# of wheels published in pypi?
postPatch = ''
substituteInPlace pyproject.toml \
--replace "pybind11==2.10.1" "pybind11>=2.10.3" \
--replace '"pythran>=0.12.0,<0.13.0",' "" \
--replace "numpy==" "numpy>="
'';

nativeBuildInputs = [
pypaBuildHook
cython
gfortran
meson-python
pythran
pkg-config
wheel
];

buildInputs = [
numpy.blas
blas
lapack
pybind11
pooch
] ++ lib.optionals (pythonOlder "3.9") [
Expand Down Expand Up @@ -67,6 +95,15 @@ buildPythonPackage rec {
#
hardeningDisable = lib.optionals (stdenv.isAarch64 && stdenv.isDarwin) [ "stackprotector" ];

dontUsePipBuild = true;
pypaBuildFlags = [
# Skip sdist
"--wheel"

"-Csetup-args=-Dblas=cblas"
"-Csetup-args=-Dlapack=lapacke"
];
Comment on lines +99 to +105
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently, pip now supports this as well: mesonbuild/meson-python#415 (comment)

However, we should probably finish the build version first, and then move back to pip, because if we're to introduce pipWheelFlags we'll have to go through staging

Copy link
Member

Choose a reason for hiding this comment

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

This rebuild is already very large and needs to go through staging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FRidh OK, new plan then:

  • drop -m build (because there's no need for it),
  • introduce pipWheelFlags,
  • expose setupHook in python3Packages.meson-python, so that including meson-python in nativeBuildInputs is sufficient to build meson-python-backed packages
  • meson-python.setupHook looks at the same variables as meson, e.g. mesonFlags
  • meson-python.setupHook prepends mesonFlags with -Csetup-args= and extends pipBuildFlags with them

Does that sound OK? I fear this is too much indirection and could be fragile

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think this is the right approach.

Note we'd want this also in the future when we would replace pip with build.

Copy link
Contributor

Choose a reason for hiding this comment

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

@FRidh OK, new plan then:

  • drop -m build (because there's no need for it),
  • introduce pipWheelFlags,

I'm working on pipBuildFlags at #239969 .

  • expose setupHook in python3Packages.meson-python, so that including meson-python in nativeBuildInputs is sufficient to build meson-python-backed packages
  • meson-python.setupHook looks at the same variables as meson, e.g. mesonFlags
  • meson-python.setupHook prepends mesonFlags with -Csetup-args= and extends pipBuildFlags with them

Does that sound OK? I fear this is too much indirection and could be fragile

That sounds great to me too, but what I learned from the experience in #239969 is that passing --cross-file to meson is what's needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, does --cross-file go into mesonFlags (pipBuildFlags) in addition to -Dblas...?


checkPhase = ''
runHook preCheck
pushd "$out"
Expand All @@ -82,8 +119,6 @@ buildPythonPackage rec {
blas = numpy.blas;
};

setupPyBuildFlags = [ "--fcompiler='gnu95'" ];
SomeoneSerge marked this conversation as resolved.
Show resolved Hide resolved

SCIPY_USE_G77_ABI_WRAPPER = 1;

meta = with lib; {
Expand Down