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
BLD: introduce use of BLAS_LIBS and LAPACK_LIBS in distutils/system_info #18737
Conversation
Could the linter tell me what's wrong? This is a bit opaque. I hope this doesn't prevent people from looking at this request. |
Hit the details button. Long lines are the problem, they need to be < 80 characters. |
Ah, thanks. I needed to allow more external scripts in the browser to see the actual messages. Too bad just about anything looks like an XSS attack nowadays. I fixed the long lines now. |
Well, I guess the checks are as good as it gets now. I don't know what to make of the codecov/patch thing. And that one failed build seems unrelated. |
LGTM, but a guick review by @rgommers would be useful. |
Could use a release note under |
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.
Thanks @drhpc. This is valuable I think, my comments on env var name and doc change are minor. It's cleaner than dynamically writing a site.cfg
like done here for example: https://github.com/conda-forge/numpy-feedstock/blob/master/recipe/build.sh#L11
This works kind of differently from the other options though, for example include_dirs
is not filled in. It also won't work on Windows out of the box (see the msvc_gfortran
stuff that's bypassed), and without the CBLAS interface I think this stub method won't work with MKL (see scipy/scipy@1334bfe). That's the main reason for asking to separate it out from the other libraries in the docs.
Right, because there are no header files;-) Except when you use CBLAS … and there I might need some clarification. In pkgsrc, we opted to keep CBLAS out of the library choices that offer it. We have CBLAS or LAPACKE wrapper packages built from Netlib code. If you want to use the C interfaces, it is the task of the C interface to choose the implementation it pulls in. Of course, waters are muddy as often, but not always, CBLAS with cblas.h is shipped with BLAS. Sometimes LAPACKE may be present, too. But they are redundant to the Netlib wrappers, AFAIK. I am unsure about how numpy would benefit from CBLAS. What about the LAPACK part; is it possibly using the C interface there, too?
I think I did not quite understand what you ask for in the docs. Regarding ABI compatibility, we have the policy to build things with the same toolchain, exactly because Fortran 95 and C++ ABI is generally incompatible between differing compilers. If a platform has specific requirements like combining MSVC with gfortran, the other non-generic options still apply. Personally, I think it is madness if each package like scipy tries to bother with the user mixing incompatible builds. This adds complexity all over the software stack where each layer wants to be smart about things. Since you pointed to scipy: I am not clear about its BLAS linkage … is it doing this independently of numpy? I noticed that the pkgsrc scipy build seems to pick up what numpy was built with, but it does contain redundant code in its system_info.py for detecting implementations. I hoped that would all be channeled through numpy. I guess scipy's system_info should get a similar patch to make sure it behaves the same? Edit: scipy re-uses the system_info code installed by numpy, so it picks up BLAS_LIBS because of my numpy patch in pkgsrc. I don't quite grok what scipy does there with its own lapack_opt_info() and blas_opt_info(). |
Reading #3439 , I must admit that I was not aware of Fortran 77 ABI being broken between compilers, too, for functions. I also did not encounter trouble linking from gfortran to Intel MKL in the past. Is this a Windows-specific problem? Has CBLAS usage in numpy any other purpose than glossing over this ABI issue? Edit: Or is the ABI thing rather a historic issue? Seems to me that the default target for Intel MKL nowadays is the ABI of gfortran. |
NumPy uses CBLAS for matrix multiplication, see the included headers/code in |
To clarify: When HAVE_CBLAS is defined, some CBLAS API is used (exclusively?), but when it is not defined, NumPy resorts to plain BLAS to do the same? Or is it not using BLAS at all in some cases and so some CBLAS should be mandatory for a proper build? This was unclear to me from skimming the sources. I seemed that CBLAS is optional for no actual benefit if you're just building and running on the same setup. If not, I'd have to roll it in as a dependency. Do I parse numpy/core/src/umath/matmul.c.src correct in that indeed it seems to resort to hand-rolled matul (with _noblas suffix) when HAVE_CBLAS is not there? That would mean NumPy absolutely needs CBLAS … but in addition to direct BLAS and LAPACK API usage? |
Yes, it will not be using BLAS at all, and it will be using the reference lapack-lite implementation for linear algebra routines (not quite sure what that does if there is no blas, I guess it also ships its own slow blas). So you are right and you should consider it a dependency. Without a CBLAS available, you will get very slow matrix multiplication (and linear algebra routines) and without lapack you will get the lapack-lite reference implementation for linear algebra. Both is a lot slower if your program does linear algebra. (If you don't, you may not see a difference.) |
So what is the purpose of a build without HAVE_CBLAS at all? So I contributed something useless when making math/py-numpy depend on optimized BLAS in pkgsrc. I did not find any mention of CBLAS in the documentation. It should be noted very prominently then, that NumPy needs CBLAS. I need to re-think my appoach here, then. CBLAS needs to be included. I noticed that NumPy ships its own cblas.h anyway (right?), so it is just about providing the right library symbols. But it does not use the C interface for LAPACK? Just this partial mix of BLAS+CBLAS+LAPACK? |
Correct, historically it wasn't always present. IIRC, a missing cblas.h initiated a fall back search for a library containing the symbols. I suppose that with the Fortran to C interface things might could be simplified, distutils is fragile and we don't like to touch it. Someday it may be replaced by meson depending on how that works out for SciPy. NumPy does contain f2c translations of the reference BLAS and selected parts of LAPACK as a fallback when neither can be found. |
OK, then. so I need to rework this pull request. Since you ship cblas.h by yourself and the interface is standardized (you'll only miss some specifics from OpenBLAS' one about its configuration, for example), the easy way out would be simply adding CBLAS_LIBS. I'll modify my patch to also look for CBLAS_LIBS (even if empty, just being defined) and in that case add those to the linker The only open question then would be how to handle the 64 bit offset versions. I did not encounter those yet. I guess they're still a speciality. I am also not sure how selection is supposed to work from reading system_info.py. |
It's not accommodating weirdness, there literally is no other choice on Windows than MSVC + gfortran. Well maybe today with oneAPI,
I'm not sure that's needed. I think this PR was fine aside from doc changes. For NumPy itself we will simply not use
That was an unfortunate necessity because SciPy had to disable Accelerate detection, and had to build with an old NumPy version. This is the problem of shipping |
OK, I got a new patch now that adds NPY_CBLAS_LIBS along with the others having the NPY prefix now. I tested a build and got exactly one object linking to libcblas.so:
Does that look sensible? PS: The banter about weird build choices … for me, Windows with MSVC is already weirdness; especially since you now got WSL to get a sensible work environment on that platform, in addition to Cygwin or just installing a proper OS for scientific work;-) |
With this change, our setting of the env vars will ensure that packages dependent on NumPy and also using its distutils for BLAS will make the same choices, right? So I don't have to hack SciPy, too? Until meson changes things? I hope the fight for telling it which lib to use won't have to start anew with a change in build systems. Anyhow, please consider the current state of the pull request. |
This enables the behaviour of the build honouring the environment variables NPY_BLAS_LIBS, NPY_CBLAS_LIBS, and NPY_LAPACK_LIBS to select any standard-conforming implementation of the f77 interfaces. This is needed to sanely have automated builds provide BLAS libs in a way common to multiple packages (using some environment variable, BLAS_LIBS being canonical, but variants exist). Hacking a site.cfg is fine for a single user, but does not scale well. As example, pkgsrc, the NetBSD packages collection, offers differing builds of Netlib or OpenBLAS, the latter in variables openblas, openblas_openmp, and openblas_pthread. With this patch, differing builds can just use different variable values like NPY_BLAS_LIBS=-lblas NPY_CBLAS_LIBS=-lcblas \ NPY_LAPACK_LIBS='-lapack -lblas' \ python setup.py build NPY_BLAS_LIBS=-lopenblas_pthread NPY_CBLAS_LIBS= \ NPY_LAPACK_LIBS='-lopenblas_pthread' \ python setup.py build (NPY_LAPACK_LIBS contains reference to BLAS, too, for packages that only use LAPACK and would miss underlying BLAS otherwise for static linking.) To ensure the generic BLAS being used, pkgsrc also sets NPY_BLAS_ORDER=generic NPY_LAPACK_ORDER=generic but I do think it makes sense to keep it first in line to enable the above simple usage that matches other scientific software with less sophisticated build systems.
Now the state is current. Forgot to add some. |
I pushed a few changes to make clearer what actually happens, and make this more orthogonal to the autodetection logic. Also changed
Correct (2x). |
I am still in doubt about whether it's a good idea to put this in |
The orthogonal change is fine by me … so I do not have to mess with NPY_BLAS_ORDER. I think it should be definitely be documented. The CBLAS requirement, definitely! Also, I do think that it is a valid scenario also for end users to possibly want to avoid the autodetection when the explicit task may be to make another build of NumPy with a specific BLAS. Edit: Also, I would hope that some direct NumPy users also know what they are doing … Where else would it be documented if not in user/building.rst? Is there a secret documentation for package managers only elsewhere? ;-) |
The shippable error can be ignored, I've disabled the webhook so that it won't happen anymore. |
@rgommers Good to go? The new environment variables need to be documented somewhere and the build documentation is probably as good a place as any. |
There isn't, but there really should be:) |
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.
Okay, let's give this a try. Thanks @drhpc & all reviewers.
This enables the behaviour of the build honouring the environment
variables BLAS_LIBS and LAPACK_LIBS to select any standard-conforming
implementation of the f77 interfaces.
This is needed to sanely have automated builds provide BLAS libs
in a way common to multiple packages (using some environment variable,
BLAS_LIBS being canonical, but variants exist). Hacking a site.cfg
is fine for a single user, but does not scale well. As example,
pkgsrc, the NetBSD packages collection, offers differing builds
of Netlib or OpenBLAS, the latter in variables openblas,
openblas_openmp, and openblas_pthread. With this patch, differing
builds can just use different variable values like
BLAS_LIBS=-lblas LAPACK_LIBS='-lapack -lblas'
python setup.py build
BLAS_LIBS=-lopenblas_pthread LAPACK_LIBS='-lopenblas_pthread'
python setup.py build
(LAPACK_LIBS contains reference to BLAS, too, for packages that only
use LAPACK and would miss underlying BLAS otherwise for static linking.)
EDIT: this part was removed from the PR:
To ensure the generic BLAS being used, pkgsrc also setsNPY_BLAS_ORDER=generic NPY_LAPACK_ORDER=genericbut I do think it makes sense to keep it first in line to enable the above simple usage that matches other scientific software with less sophisticated build systems.