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: Allow users to specify BLAS and LAPACK library link order #13132

Merged
merged 7 commits into from May 1, 2019

Conversation

@zerothi
Copy link
Contributor

commented Mar 15, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

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

@tylerjereddy

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

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

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

@rgommers

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

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

@zerothi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

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!

@charris 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 rgommers referenced this pull request Mar 18, 2019

Merged

Blas 2 #143

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

zerothi added some commits Mar 15, 2019

ENH: allowed external users to select BLAS and LAPACK library link order
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 <nickpapior@gmail.com>
ENH: amended documentation and changed env-vars as suggested
Also added a test to travis (apparently ATLAS=None... is not tested
on circleCI).

Signed-off-by: Nick Papior <nickpapior@gmail.com>
BUG: ensured that warnings/errors are raised when nothing is requested
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 <nickpapior@gmail.com>

@zerothi zerothi force-pushed the zerothi:linalg-order branch from 219c872 to 146a174 Mar 18, 2019

@zerothi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

@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

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

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

@zerothi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

@charris oh yeah, there it was! 👍

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

BUG: fixed lapack_lite sources
Signed-off-by: Nick Papior <nickpapior@gmail.com>
@rgommers

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

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

@mattip mattip requested a review from rgommers Apr 26, 2019

@rgommers
Copy link
Member

left a comment

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

Show resolved Hide resolved .travis.yml Outdated
Show resolved Hide resolved numpy/distutils/system_info.py Outdated
Show resolved Hide resolved numpy/distutils/system_info.py Outdated
Show resolved Hide resolved numpy/distutils/system_info.py Outdated
Show resolved Hide resolved numpy/distutils/system_info.py Outdated
MAINT: fixed last issues and questions according to #13132
Signed-off-by: Nick Papior <nickpapior@gmail.com>
Show resolved Hide resolved numpy/distutils/system_info.py Outdated

zerothi added some commits Apr 29, 2019

BUG: fixed PYTHONOPTIMIZE run
Signed-off-by: Nick Papior <nickpapior@gmail.com>
MAINT: fixed several PYTHONOPTIMIZE=2 failures
Signed-off-by: Nick Papior <nickpapior@gmail.com>

@mattip mattip requested a review from rgommers Apr 30, 2019

@rgommers
Copy link
Member

left a comment

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 rgommers merged commit 0f19dae into numpy:master May 1, 2019

16 checks passed

LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No new or fixed alerts
Details
Shippable Run 3751 status is SUCCESS.
Details
azure-pipeline numpy.numpy Build #20190430.5 succeeded
Details
azure-pipeline numpy.numpy (Linux_PyPy3) Linux_PyPy3 succeeded
Details
azure-pipeline numpy.numpy (Linux_Python_36_32bit_full_with_asserts) Linux_Python_36_32bit_full_with_asserts succeeded
Details
azure-pipeline numpy.numpy (Windows Python35-64bit-full) Windows Python35-64bit-full succeeded
Details
azure-pipeline numpy.numpy (Windows Python36-32bit-fast) Windows Python36-32bit-fast succeeded
Details
azure-pipeline numpy.numpy (Windows Python36-64bit-full) Windows Python36-64bit-full succeeded
Details
azure-pipeline numpy.numpy (Windows Python37-32bit-fast) Windows Python37-32bit-fast succeeded
Details
azure-pipeline numpy.numpy (Windows Python37-64bit-full) Windows Python37-64bit-full succeeded
Details
azure-pipeline numpy.numpy (macOS) macOS succeeded
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 85.57% (target 1%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rgommers

This comment has been minimized.

Copy link
Member

commented May 1, 2019

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

lijunzh added a commit to lijunzh/numpy that referenced this pull request May 2, 2019

Merge branch 'master' into lijunzh-open_memmap_docs
* master: (25 commits)
  BUG: fix unravel_index when dimension is greater than 'intp'
  MANT: refactor unravel_index for code repetition (numpy#13446)
  BLD, TST: implicit func errors
  DOC: document existance of linalg backends
  BLD: streamlined library names in site.cfg sections (numpy#13157)
  MAINT: fixed typo 'wtihout' from numpy/core/shape_base.py
  BUG: fixing bugs in AVX exp/log while handling special value floats (numpy#13415)
  update sequence
  Add analysis check
  BUG: blindly add TypeError to accepted exceptions
  MAINT: fixed several PYTHONOPTIMIZE=2 failures
  BUG: fixed PYTHONOPTIMIZE run
  MAINT: fixed typo 'Mismacth' from numpy.core.setup_common.py
  MAINT: fixed last issues and questions according to numpy#13132
  MAINT: improve efficiency of pad by avoiding use of apply_along_axis
  DOC: dimension sizes are non-negative, not positive
  BUILD, BUG: fix from review, fix bug in git_version
  MAINT: mention 'make dist' in error messsage
  BUILD: allow version-check to pass if GITVER is Unknown (sdist build)
  BUILD: fail documentation build if numpy version does not match
  ...

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

@adamjstewart adamjstewart referenced this pull request Aug 1, 2019

Merged

Overhaul numpy package #12170

21 of 26 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.