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.

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

Merged
merged 5 commits into from Apr 11, 2021

Conversation

drhpc
Copy link
Contributor

@drhpc drhpc commented Apr 7, 2021

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 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.

@drhpc
Copy link
Contributor Author

drhpc commented Apr 7, 2021

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.

@charris
Copy link
Member

charris commented Apr 7, 2021

Could the linter tell me what's wrong?

Hit the details button. Long lines are the problem, they need to be < 80 characters.

@drhpc
Copy link
Contributor Author

drhpc commented Apr 7, 2021

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.

@drhpc
Copy link
Contributor Author

drhpc commented Apr 7, 2021

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.

@charris
Copy link
Member

charris commented Apr 9, 2021

LGTM, but a guick review by @rgommers would be useful.

@charris
Copy link
Member

charris commented Apr 9, 2021

Could use a release note under new_feature. Look in doc/release/upcoming_changes for examples.

Copy link
Member

@rgommers rgommers left a 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.

doc/release/upcoming_changes/18737.new_feature.rst Outdated Show resolved Hide resolved
doc/source/user/building.rst Outdated Show resolved Hide resolved
@drhpc
Copy link
Contributor Author

drhpc commented Apr 9, 2021

This works kind of differently from the other options though, for example include_dirs is not filled in.

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?

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.

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().

@drhpc
Copy link
Contributor Author

drhpc commented Apr 9, 2021

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.

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.

@charris
Copy link
Member

charris commented Apr 9, 2021

NumPy uses CBLAS for matrix multiplication, see the included headers/code in numpy/core/src/common.

@drhpc
Copy link
Contributor Author

drhpc commented Apr 9, 2021

NumPy uses CBLAS for matrix multiplication, see the included headers/code in numpy/core/src/common.

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?

@seberg
Copy link
Member

seberg commented Apr 9, 2021

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.)

@drhpc
Copy link
Contributor Author

drhpc commented Apr 9, 2021

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)

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?

@charris
Copy link
Member

charris commented Apr 9, 2021

NumPy ships its own cblas.h

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.

@drhpc
Copy link
Contributor Author

drhpc commented Apr 9, 2021

NumPy ships its own cblas.h

Correct, historically it wasn't always present. IIRC, a missing cblas.h initiated a fall back search for a library containing the symbols.

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
args and define the HAVE_CBLAS. This should cover the cases where the builder (script) really does know what is provided in
a single or even up to separate three libraries. Any funky compiler/preprocessor flags can be added to CFLAGS in general.

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.

@rgommers
Copy link
Member

rgommers commented Apr 9, 2021

Personally, I think it is madness if each package like scipy tries to bother with the user mixing incompatible builds

It's not accommodating weirdness, there literally is no other choice on Windows than MSVC + gfortran. Well maybe today with oneAPI, ifort is finally accessible (not sure). But the short version of this story is: you have to build Python extensions with the same MSVC version as Python is built with, and there is no other freely available Fortran compiler than gfortran that can build SciPy. So the official wheels and conda-forge packages are built that way.

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'm not sure that's needed. I think this PR was fine aside from doc changes. For NumPy itself we will simply not use BLAS_LIBS since things work fine today. So I'd say we can merge this as is if my comments were addressed.

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().

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 numpy.distutils together with NumPy rather than separate. It's always surprised me that this didn't cause more problems.

@drhpc
Copy link
Contributor Author

drhpc commented Apr 9, 2021

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:

build/lib.linux-x86_64-3.8/numpy/core/_multiarray_umath.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;-)

@drhpc
Copy link
Contributor Author

drhpc commented Apr 9, 2021

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 numpy.distutils together with NumPy rather than separate. It's always surprised me that this didn't cause more problems.

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.
@drhpc
Copy link
Contributor Author

drhpc commented Apr 9, 2021

Now the state is current. Forgot to add some.

@drhpc drhpc requested a review from rgommers April 9, 2021 18:30
@rgommers
Copy link
Member

I pushed a few changes to make clearer what actually happens, and make this more orthogonal to the autodetection logic. Also changed generic (which arguably the Netlib option already is - this is exactly how conda-forge does runtime switching) to from_envvar. Please let me know if that looks good to you @drhpc.

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?

Correct (2x).

@rgommers
Copy link
Member

I am still in doubt about whether it's a good idea to put this in user/building.rst, because this option really is only healthy for the likes of pkgsrc who know exactly what they're doing, not for end users. Autodetection is in general more robust (there are compile-time checks for both presence of BLAS, the flags it needs, and in some cases that a trivial C program works), and it's what all decent build systems do AFAIK.

@drhpc
Copy link
Contributor Author

drhpc commented Apr 10, 2021

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? ;-)

@charris
Copy link
Member

charris commented Apr 11, 2021

The shippable error can be ignored, I've disabled the webhook so that it won't happen anymore.

@charris
Copy link
Member

charris commented Apr 11, 2021

@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.

@rgommers
Copy link
Member

Where else would it be documented if not in user/building.rst? Is there a secret documentation for package managers only elsewhere? ;-)

There isn't, but there really should be:)

Copy link
Member

@rgommers rgommers left a 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.

@rgommers rgommers merged commit 98ec22a into numpy:main Apr 11, 2021
@rgommers rgommers added this to the 1.21.0 release milestone Apr 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants