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
WIP: python3Packages.scipy: allow overriding BLAS #230131
base: master
Are you sure you want to change the base?
Changes from all commits
14e9a3a
6af7b92
0f3bc1c
5734bfb
28355c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,12 @@ | ||
{ lib | ||
, stdenv | ||
, fetchPypi | ||
, pypaBuildHook | ||
, python | ||
, pythonOlder | ||
, buildPythonPackage | ||
, blas | ||
, lapack | ||
, cython | ||
, gfortran | ||
, meson-python | ||
|
@@ -19,6 +22,8 @@ | |
, libxcrypt | ||
}: | ||
|
||
assert blas.provider == numpy.blas; | ||
|
||
buildPythonPackage rec { | ||
pname = "scipy"; | ||
version = "1.10.1"; | ||
|
@@ -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") [ | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apparently, However, we should probably finish the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This rebuild is already very large and needs to go through staging. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @FRidh OK, new plan then:
Does that sound OK? I fear this is too much indirection and could be fragile There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm working on
That sounds great to me too, but what I learned from the experience in #239969 is that passing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, does |
||
|
||
checkPhase = '' | ||
runHook preCheck | ||
pushd "$out" | ||
|
@@ -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; { | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
andnumpy
. IIRC, a similar assert is used forcudaPackages
in several places, so if this change is rejected, we probably should update these as wellPreviously the synchronization was achieved by putting
numpy.blas
inbuildInputs
. The reason I removednumpy.blas
was becausenumpy.blas
isblas.provider
rather thanblas
. But as mentioned earlier, I'm not yet sure how to motivate the choice betweenblas
andblas.provider
. I'll have to inspect the history of this derivation, I guess...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is explained in this older thread #36229.
There was a problem hiding this comment.
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 multipleblas
overrides inoctave
, because there things are a little bit more complicated - theoctave
expression has to coordinate between many dependencies that depend and should use the sameblas
andlapack
. In this case, it's much simpler to my understanding, and I don't understand what's wrong with usingnumpy.blas
which points toblas.provider
. In the current state of things you can just:Which seems very nice to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 fromnumpy.blas
(evaluates intomkl
in case ofblas = 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 atcblas
, as far as I can tell.The reason I put the
assert
is because previously it wasn't possible to have un-synchronizednumpy.blas
andscipy.blas
, but with the new interface it is and even likely to happen by accident (e.g. if one had a local override like yourscipy-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
exposesblas.provider
instead of justblas
.Further thoughts?
@FRidh I'll switch to
meta.broken
, and later open a PR to migrate cuda packages as wellThere was a problem hiding this comment.
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;
sowithPackages
functions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only I think
blas
andlapack
come from the outer package set (nixpkgs#blas
, notnixpkgs#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 addmeta.broken = blas != numpy.blas
. I still don't see any reason we exposeblas.provider
instead ofblas
innumpy.passthru
, so I think we shouldn't be doing thatRE:
python3.override
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CallPackage should pick it up if I am correct
I don't know anymore why that was. Maybe that should be changed throughout?
That would have an effect on the entire nixpkgs then (all interpreters + every package using any of these).
There was a problem hiding this comment.
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 bycallPackage
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly!