-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
CI/BLD: fail by default if no BLAS/LAPACK, add 32-bit Python on Windows CI job #24279
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
Conversation
Well that is smoking out a bunch of issues ... I bet there's a whole bunch of projects building NumPy in CI without BLAS support, either by accident or because it doesn't matter to them. I think we need to have this default though, because it affects the |
91f335a
to
bb3d188
Compare
4571d11
to
401dca5
Compare
Disable using BLAS in the PyPy job on Azure because it was broken. Before this PR, it uses to silently not find OpenBLAS and continue, now we have to be explicit about it.
@@ -28,26 +28,33 @@ steps: | |||
mkdir C:/opt/openblas/openblas_dll | |||
mkdir C:/opt/32/lib/pkgconfig | |||
mkdir C:/opt/64/lib/pkgconfig | |||
# TBD: support 32 bit testing | |||
$target=$(python -c "import tools.openblas_support as obs; plat=obs.get_plat(); ilp64=obs.get_ilp64(); target=f'openblas_{plat}.zip'; obs.download_openblas(target, plat, ilp64);print(target)") |
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.
Oh, this is ugly :)
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.
Yes, this stuff is awful. I'm really looking forward to the day we have OpenBLAS in a wheel and get rid of this.
LGTM. Just for confirmation
|
Indeed, that's how it has always been - you get 32-bit by default, and have to opt in to ILP64.
It's working fine and is used in a bunch of places. Only the PyPy build is changed here, it was silently failing to find OpenBLAS ILP64 before (so passing with lapack-lite), and when I fixed the detection logic it started failing with a "can't import |
I am not sure what you meant by "it" in the context of PyPy failing. It seems all the CI is passing here? |
@mattip I had to add |
Let's get this in. Thanks Ralf. |
@charris I think we can backport all this to 1.26.x, but set the default for |
This seems already done in 1.26 except main isn't building pp39. |
Except the default is still |
I think we should allow building without OpenBLAS by default on 1.26. That has been the policy up to now. The time to change it is in the move to 2.0, not in a 1.26 interim release which is mainly meant to enable building with meson for python 3.12. |
@mattip if it were only about the policy and there were no side effects, it would be a simple decision indeed and I'd agree we should only consider it for 2.0. Why I am hesitant (either way) is that the BLAS/LAPACK detection mechanisms changed. E.g. someone who had a So the trade-off we deal with here is:
Given the above, would you still prefer |
Hmm. Crazy thought: could we port just enough of the
|
My first thought was "even more work, don't really wanna". Second thought: "yeah, I think this is actually feasible and may be appreciated by our users". There's two ways of doing that:
Neither is going to be 100% complete without a lot of effort, but it's doable to catch most of the common cases with maybe half a day of work. The trickiest situation is also the most common one: when the user did nothing special like setting an env var or creating a |
I'd be happy just to be able to continue using something like All this makes me appreciate Pearu's work way back in the beginning. |
@charris you can use a custom file to specify the location, compile flags, etc. of BLAS/LAPACK. It has changed from a numpy-specific and ad-hoc format to a pkg-config file, as documented at http://scipy.github.io/devdocs/building/blas_lapack.html#using-pkg-config-to-detect-libraries-in-a-nonstandard-location.
Yep, that part is still in the works (also for SciPy). I should have that done by the end of September; recreating all of
definitely! |
I ended up downloading a tarball from the OpenBLAS nightlies and installing it in We could upgrade |
That's not how that works. You can put a
We will get rid of that file as soon as possible, so please don't spend more than the minimum needed effort on it. The plan is to change our custom OpenBLAS build from a |
This xfail was added in numpygh-24279 for 32-bit Python + MSVC when switching to Meson and having temporarily no SIMD support. That is back now, so this test passes again. [skip circle] [skip cirrus] [skip travis]
The change of default in build system behavior follows up on the discussion in gh-24200. It avoids large accidental regression in performance. If users really want to build with fallback routines, it's a matter of passing a single
-Dallow-noblas
to opt into that.The 32-bit Python on Windows CI job was removed from Azure when switching to Meson. This re-introduces it, on GitHub Actions because it's much easier to debug there. This should be low-maintenance, and exercise the
-Dallow-noblas
flag at the same time. It addressed one of the TODO's in gh-23981.There was one failing test on this config, not surprising because it hasn't been tested before. Those were for floating point exceptions, which already had a note about issues with MSVC + 32-bit Python. Given that the behavior is going to change when SIMD support comes back, I simply disabled the test for now.