BLD: Allow users to specify BLAS and LAPACK library link order by zerothi · Pull Request #13132 · numpy/numpy

zerothi · 2019-03-15T13:43:41Z

Prior to this enhancement compiling numpy would forcefully check BLAS/LAPACK
libraries in the following order:

BLAS:

mkl
blis
openblas
atlas
accelerate
NetLIB BLAS

LAPACK:

mkl
openblas
atlas
accelerate
NetLIB LAPACK

This is problematic if a user want to build using, say, OpenBLAS but MKL is installed.
Even populating the site.cfg correspondingly one would get a successful build, but
using MKL, if present.

The same applies to OpenBLAS vs. ATLAS etc.

Especially for developers this may be desirable to check performance with various
BLAS/LAPACK libraries.

This fixes the above issues by enabling users to forcefully set the order of loads
via environment variables:

$> export NUMPY_BLAS_ORDER=openblas,mkl,atlas
$> python setup.py config ...

would first try OpenBLAS (if existing), then MKL, and finally ATLAS.
In this case the build would fail if neither of OpenBLAS, MKL or ATLAS is present.
I.e. this can also be easierly used to test whether a linking would work. This
is because specifying a single library forces only one library check and has
no fall-back procedure (as requested by the user!).

The same applies to:

NUMPY_LAPACK_ORDER=openblas,mkl,atlas

This has meant that the blas_opt_info and lapack_opt_info classes in
system_info.py has completely changed.

Effectively there is only ONE change:

A fall-back of LAPACK was previously using get_info('blas') to get
the BLAS library to correctly link LAPACK. However, this may be undesirable
when the user has OpenBLAS/BLIS/ATLAS in a BLAS only installation but wants
to use the NetLIB LAPACK. Hence now lapack_opt_info uses get_info('blas_opt')
which does change the fall-back routine slightly. But perhaps for an easier build?

zerothi · 2019-03-15T13:44:35Z

I would be happy to create a test, but it seems extremely build specific and probably difficult in a testing environment?

Do you have a CI just for testing python setup.py config commands?

Also, sorry I created a commit with ENH, it should have been BLD, I would be happy to change this if so desired. (this PR however has the correct BLD prefix).

numpy/distutils/system_info.py

tylerjereddy · 2019-03-15T16:48:28Z

The same applies to OpenBLAS vs. ATLAS etc.

Does it? ATLAS=None should do the trick, right?

We can also do BLAS=None and LAPACK=None. I'm not saying those solve all problems, but curious as to why they aren't discussed in context above re: circumventing backends you don't want to use for building.

rgommers · 2019-03-15T16:56:19Z

I would be happy to create a test, but it seems extremely build specific and probably difficult in a testing environment?

Yeah, it's not really testable on CI. If it passes our existing CI (which uses OpenBLAS and lapack_lite IIRC) and some local testing by devs, then it's fine.

Do you have a CI just for testing python setup.py config commands?

There's a few things in numpy/distutils/tests/test_system_info.py, but nothing for BLAS/LAPACK currently. I don't think there's much you could test (easily) there, except perhaps that NPY_LAPACK_ORDER is handled correctly.

Also, sorry I created a commit with ENH, it should have been BLD, I would be happy to change this if so desired. (this PR however has the correct BLD prefix).

No worries. It is ENH anyway arguably, it's a distutils enhancement.

rgommers · 2019-03-15T16:57:32Z

The two CI failures are the same - lapack_lite build not handled.

Building with BLAS=None LAPACK=None ATLAS=None should still work.

rgommers · 2019-03-15T16:59:31Z

Actually, checking that any of BLAS, LAPACK, ATLAS work still when set explicitly would also be good.

zerothi · 2019-03-15T17:28:51Z

The same applies to OpenBLAS vs. ATLAS etc.

Does it? ATLAS=None should do the trick, right?

Only if MKL is not installed.

We can also do BLAS=None and LAPACK=None. I'm not saying those solve all problems, but curious as to why they aren't discussed in context above re: circumventing backends you don't want to use for building.

Agreed, it is probably more meant as a developer thing. And it would also be way easier to have system installed BLAS/LAPACK versions and letting users control this without complicated mixtures of ATLAS=None MKL=None BLIS=None ....

This PR tries to streamline this in a unified way. A second benefit is that the blas/lapack_opt_info's restructure makes it a bit easier to add new libraries.

zerothi · 2019-03-15T17:34:10Z

Actually, checking that any of BLAS, LAPACK, ATLAS work still when set explicitly would also be good.

I'll have another look during next week!

Thanks for both your comments!

Prior to this enhancement compiling numpy would forcefully check BLAS/LAPACK libraries in the following order: BLAS: - mkl - blis - openblas - atlas - accelerate - NetLIB BLAS - LAPACK - mkl - openblas - atlas - accelerate - NetLIB LAPACK This is problematic if a user want to build using, say, OpenBLAS but MKL is installed. Even populating the site.cfg correspondingly one would get a successfull build, but using MKL, if present. The same applies to OpenBLAS vs. ATLAS etc. Especially for developers this may be desirable to check performance with various BLAS/LAPACK libraries. This fixes the above issues by enabling users to forcefully set the order of loads via environment variables: $> export NUMPY_BLAS_ORDER=openblas,mkl,atlas $> python setup.py config ... would first try OpenBLAS (if existing), then MKL, and finally ATLAS. In this case the build would fail if neither of OpenBLAS, MKL or ATLAS is present. I.e. this can also be easierly used to test whether a linking would work. This is because specifying a single library forces only one library check and has no fall-back procedure (as requested by the user!). The same applies to: NUMPY_LAPACK_ORDER=openblas,mkl,atlas This has meant that the blas_opt_info and lapack_opt_info classes in system_info.py has *completely* changed. Effectively there is only ONE change: A fall-back of LAPACK was previously using get_info('blas') to get the BLAS library to correctly link LAPACK. However, this may be undesirable when the user has OpenBLAS/BLIS/ATLAS in a BLAS only installation but wants to use the NetLIB LAPACK. Hence now lapack_opt_info uses get_info('blas_opt') which does change the fall-back routine slightly. But perhaps for an easier build? Signed-off-by: Nick Papior <[email protected]>

Also added a test to travis (apparently ATLAS=None... is not tested on circleCI). Signed-off-by: Nick Papior <[email protected]>

When a user requests NPY_BLAS/LAPACK_ORDER they can omit Netlib BLAS/LAPACK. In that case there will not be raised anything. This commit fixes this issue so that there will always be issues raised if the user hasn't requested the basic libraries. Signed-off-by: Nick Papior <[email protected]>

zerothi · 2019-03-18T20:42:01Z

@rgommers regarding the lapack_lite fail, it seems that this is due to a missing source in f2c_[cz]_lapack.c source? Or am I missing something completely? I.e. it seems not to have anything to do with my changes? I can't see why suddenly a missing name zungqr_ should stem from my changes?

Also do you prefer NPY_LAPACK or NPY_LAPACK_ORDER? The former could also be used since it is not taken, and 2ndly because the value may be a single value or a comma-separated list?

charris · 2019-03-18T20:51:48Z

I see zungqr_ in numpy/linalg/lapack_lite/f2c_z_lapack.c.

zerothi · 2019-03-18T21:12:01Z

@charris oh yeah, there it was! 👍

I will fix the bug (in my code ;) )!

Signed-off-by: Nick Papior <[email protected]>

rgommers · 2019-03-18T21:45:31Z

Also do you prefer NPY_LAPACK or NPY_LAPACK_ORDER? The former could also be used since it is not taken, and 2ndly because the value may be a single value or a comma-separated list?

I don't have strong feelings on that one either way. However, does it make sense for it to be a single value given that we already have LAPACK for that?

zerothi · 2019-03-19T05:04:53Z

Yes and no, if people want the control its main usage will probably be single value uses. Only in special cases can I see that users want to use it multi-value.
Also this value does not reflect any linking step as it is a value equal to the name of the library. So perhaps it is fine to have it as it is now.

You'll also note I have amended the build documentation a bit which I think clarifies its use.

zerothi · 2019-03-19T11:43:53Z

I think this is ready, I think it is fine to keep the names as is. :)

rgommers

Still need to do some testing, but want to submit my comments already.

.travis.yml

numpy/distutils/system_info.py

Signed-off-by: Nick Papior <[email protected]>

numpy/distutils/system_info.py

Signed-off-by: Nick Papior <[email protected]>

rgommers

I like the __doc__ or '' fix, nice and compact.

This all looks good. With distutils it's always hard to be really sure, but let's merge it and see how it does. Nice improvement overall.

rgommers · 2019-05-01T13:14:24Z

Merged, thanks @zerothi. And thanks @mattip for the review.

rgommers added 01 - Enhancement component: numpy.distutils labels Mar 15, 2019

rgommers reviewed Mar 15, 2019

View reviewed changes

numpy/distutils/system_info.py Outdated Show resolved Hide resolved

rgommers reviewed Mar 15, 2019

View reviewed changes

numpy/distutils/system_info.py Outdated Show resolved Hide resolved

charris changed the title ~~BLD: allowed external users to select BLAS and LAPACK library link order~~ BLD: Allow users to specify BLAS and LAPACK library link order Mar 16, 2019

rgommers mentioned this pull request Mar 18, 2019

Blas 2 conda-forge/numpy-feedstock#143

Merged

rgommers added this to the 1.17.0 release milestone Mar 18, 2019

zerothi added 3 commits March 18, 2019 21:27

ENH: amended documentation and changed env-vars as suggested

aa144f3

Also added a test to travis (apparently ATLAS=None... is not tested on circleCI). Signed-off-by: Nick Papior <[email protected]>

zerothi force-pushed the linalg-order branch from 219c872 to 146a174 Compare March 18, 2019 20:27

BUG: fixed lapack_lite sources

74a7808

Signed-off-by: Nick Papior <[email protected]>

zerothi mentioned this pull request Mar 19, 2019

BLD: Add libflame as a LAPACK back-end #13158

Merged

mattip requested a review from rgommers April 26, 2019 23:27

rgommers reviewed Apr 28, 2019

View reviewed changes

MAINT: fixed last issues and questions according to numpy#13132

6781e1f

Signed-off-by: Nick Papior <[email protected]>

mattip reviewed Apr 29, 2019

View reviewed changes

numpy/distutils/system_info.py Outdated Show resolved Hide resolved

zerothi added 2 commits April 29, 2019 19:11

BUG: fixed PYTHONOPTIMIZE run

178a53f

Signed-off-by: Nick Papior <[email protected]>

MAINT: fixed several PYTHONOPTIMIZE=2 failures

995665e

Signed-off-by: Nick Papior <[email protected]>

mattip requested a review from rgommers April 30, 2019 11:29

rgommers approved these changes May 1, 2019

View reviewed changes

rgommers merged commit 0f19dae into numpy:master May 1, 2019

zerothi deleted the linalg-order branch May 2, 2019 07:39

homocomputeris mentioned this pull request May 16, 2019

Tracker issue for BLIS support in NumPy #7372

Closed

seberg mentioned this pull request Jul 29, 2019

How to force build system to use a particular BLAS/LAPACK? #14146

Closed

adamjstewart mentioned this pull request Aug 1, 2019

Overhaul numpy package spack/spack#12170

Merged

26 tasks

climbfuji mentioned this pull request Sep 21, 2022

Again py-scipy with Intel issues - undefined symbols spack/spack#32732

Closed

3 tasks

Uh oh!

Conversation

zerothi commented Mar 15, 2019

Uh oh!

zerothi commented Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tylerjereddy commented Mar 15, 2019

Uh oh!

rgommers commented Mar 15, 2019

Uh oh!

rgommers commented Mar 15, 2019

Uh oh!

rgommers commented Mar 15, 2019

Uh oh!

zerothi commented Mar 15, 2019

Uh oh!

zerothi commented Mar 15, 2019

Uh oh!

zerothi commented Mar 18, 2019

Uh oh!

charris commented Mar 18, 2019

Uh oh!

zerothi commented Mar 18, 2019

Uh oh!

rgommers commented Mar 18, 2019

Uh oh!

zerothi commented Mar 19, 2019

Uh oh!

zerothi commented Mar 19, 2019

Uh oh!

rgommers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgommers left a comment

Choose a reason for hiding this comment

Uh oh!

rgommers commented May 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zerothi commented Mar 15, 2019 •

edited

Loading