Opened 5 years ago

Closed 5 years ago

#20157 closed enhancement (fixed)

make numpy and scipy use pkg-config to find blas/lapack

Reported by: fbissey Owned by:
Priority: major Milestone: sage-7.2
Component: packages: standard Keywords:
Cc: jdemeyer Merged in:
Authors: François Bissey Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: af70376 (Commits) Commit: af70376a991d0770ec4a92da8155c33b7702f040
Dependencies: Stopgaps:

Description (last modified by fbissey)

Since we want the choice of blas/lapack to be configurable by .pc files we need to have numpy and scipy obey to pkg-config.

This is complicated by the fact that numpy/scipy's build system will look for

1) mkl 2) openblas 3) threaded atlas 3.10.x (tatlas.so) 4) single thread atlas 3.10.x (satlas.so) 5) threaded atlas (any versions) 6) single thread atlas (any versions) 7) accelerate on OS X 8) blas/lapack configuration provided by the user 9) blas/lapack source provided by the user

In that order from (1) to (9). Note that the user's wishes are ignored unless everything else fails. Getting numpy/scipy to obey the user wishes, whatever they are, means using not well documented variables to disable the auto-detections steps 1 to 6.

Change History (43)

comment:1 Changed 5 years ago by fbissey

Actually

export {ATLAS,PTATLAS,BLAS,LAPACK,OPENBLAS,MKL}=None

shuts down all detection so there is hope things can be taken done selectively.

comment:2 Changed 5 years ago by fbissey

  • Description modified (diff)

Setting everything to None except for BLAS and LAPACK and then giving those some values is not enough. I have got something quite ugly.

comment:3 Changed 5 years ago by fbissey

  • Description modified (diff)

comment:4 Changed 5 years ago by fbissey

  • Authors set to François Bissey
  • Branch set to u/fbissey/numpy_scipy_pc
  • Commit set to 3237f3805125473f2af55541ca93ddc5147b9807

New commits:

3237f38Get numpy and scipy to use pc files

comment:5 Changed 5 years ago by fbissey

  • Status changed from new to needs_review

comment:6 Changed 5 years ago by fbissey

  • Component changed from PLEASE CHANGE to packages: standard
  • Status changed from needs_review to needs_work
  • Work issues set to version bumps

comment:7 Changed 5 years ago by git

  • Commit changed from 3237f3805125473f2af55541ca93ddc5147b9807 to 158fb8db9d1a112078ece92a6b33fe84fd9e6825

Branch pushed to git repo; I updated commit sha1. New commits:

158fb8dVersion bump to trigger rebuilds

comment:8 Changed 5 years ago by fbissey

  • Status changed from needs_work to needs_review
  • Work issues version bumps deleted

comment:9 Changed 5 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

comment:10 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

Full log: http://build.sagedev.org/release/builders/%20%20fast%20Volker%20Desktop%20%28Fedora%2022%20x86_64%29%20incremental/builds/458/steps/compile/logs/stdio

/mnt/disk/home/buildslave-sage/slave/sage_git/build/local/var/tmp/sage/build/numpy-1.10.4.p1/src/numpy/distutils/system_info.py:1651: UserWarning: 
    Atlas (http://math-atlas.sourceforge.net/) libraries not found.
    Directories to search for the libraries can be specified in the
    numpy/distutils/site.cfg file (section [atlas]) or by setting
    the ATLAS environment variable.
  warnings.warn(AtlasNotFoundError.__doc__)
blas_info:
/mnt/disk/home/buildslave-sage/slave/sage_git/build/local/var/tmp/sage/build/numpy-1.10.4.p1/src/numpy/distutils/system_info.py:635: UserWarning: Specified path  is invalid.
  warnings.warn('Specified path %s is invalid.' % d)
C compiler: cc

creating /tmp/tmpX0K4bQ/tmp
creating /tmp/tmpX0K4bQ/tmp/tmpX0K4bQ
compile options: '-c'
cc: /tmp/tmpX0K4bQ/source.c
cc /tmp/tmpX0K4bQ/tmp/tmpX0K4bQ/source.o -L/mnt/disk/home/buildslave-sage/slave/sage_git/build/local/lib -lcblas -o /tmp/tmpX0K4bQ/a.out
/usr/bin/ld: warning: libatlas.so.3, needed by /mnt/disk/home/buildslave-sage/slave/sage_git/build/local/lib/libcblas.so, not found (try using -rpath or -rpath-link)

It is missing the -latlas for some reason, though it is in the cblas.pc

$ pkg-config --libs cblas
-L/mnt/disk/home/buildslave-sage/slave/sage_git/build/local/lib -lcblas -latlas 

comment:11 Changed 5 years ago by fbissey

Can you look at the generated site.cfg file?

comment:12 Changed 5 years ago by fbissey

I notice poseidon also has a problem but look slightly different.

comment:13 Changed 5 years ago by vbraun

$ cat /mnt/disk/home/buildslave-sage/slave/sage_git/build/local/var/tmp/sage/build/numpy-1.10.4.p1/src/site.cfg
[DEFAULT]
library_dirs = /mnt/disk/home/buildslave-sage/slave/sage_git/build/local/lib
include_dirs = /mnt/disk/home/buildslave-sage/slave/sage_git/build/local/include
[blas]
include_dirs = 
library_dirs = /mnt/disk/home/buildslave-sage/slave/sage_git/build/local/lib
blas_libs = atlas,f77blas,cblas
[lapack]
library_dirs = /mnt/disk/home/buildslave-sage/slave/sage_git/build/local/lib
lapack_libs = lapack,f77blas,cblas,atlas

comment:14 Changed 5 years ago by fbissey

OK I think there may be niceties in the linker. Here I get:

creating /tmp/tmpRM4d1X/tmp
creating /tmp/tmpRM4d1X/tmp/tmpRM4d1X
compile options: '-c'
cc: /tmp/tmpRM4d1X/source.c
cc /tmp/tmpRM4d1X/tmp/tmpRM4d1X/source.o -L/home/fbissey/sandbox/git-fork/sage/local/lib -lcblas -o /tmp/tmpRM4d1X/a.out
  FOUND:
    libraries = ['cblas']
    library_dirs = ['/home/fbissey/sandbox/git-fork/sage/local/lib']
    language = c
    define_macros = [('HAVE_CBLAS', None)]

  FOUND:
    libraries = ['cblas']
    library_dirs = ['/home/fbissey/sandbox/git-fork/sage/local/lib']
    define_macros = [('NO_ATLAS_INFO', 1), ('HAVE_CBLAS', None)]
    language = c

So we have only cblas again. But my linker still resolves it from the information in the cblas library.

I think there is a small bug (feature) in numpy that causes it to only use a single library. I think I now which part of the Gentoo patch deals with that,

comment:15 Changed 5 years ago by git

  • Commit changed from 158fb8db9d1a112078ece92a6b33fe84fd9e6825 to dfddf074dd2ffab07030451f6c715483bb8fad25

Branch pushed to git repo; I updated commit sha1. New commits:

45487f9cosmetics in lapack_conf.py. Add a patch to system_info.py to accept more than one library in blas_info (taken from gentoo).
dfddf07Properly setup (by setting the variable for various BLAS/LAPACK to None) scipy will look for its BLAS/LAPACK configuration in

comment:16 Changed 5 years ago by fbissey

  • Status changed from needs_work to needs_review

OK I added part of a patch from Gentoo that will let numpy's system_info.py take more that one library. I beautified the lapack_conf.py a bit. And revisited scipy's case. It turns out we can dispense with site.cfg.

comment:17 Changed 5 years ago by fbissey

poseidon is stumping me. There is not even a compilation line

blas_info:
/home/kevin/sage-patchbot/local/var/tmp/sage/build/numpy-1.10.4.p1/src/numpy/distutils/system_info.py:635: UserWarning: Specified path  is invalid.
  warnings.warn('Specified path %s is invalid.' % d)
  libraries atlas,f77blas,cblas not found in []
  NOT AVAILABLE

comment:18 follow-up: Changed 5 years ago by fbissey

While the absence of compilation output is annoying it may be due to a weakness of pkgconfig which can change the order of the libraries. Library order is important if your ld has --as-needed by default. We could try to pass --no-as-needed but it would be better if the order could be fixed from pkgconfig.

comment:19 in reply to: ↑ 18 Changed 5 years ago by fbissey

Replying to fbissey:

While the absence of compilation output is annoying it may be due to a weakness of pkgconfig which can change the order of the libraries. Library order is important if your ld has --as-needed by default. We could try to pass --no-as-needed but it would be better if the order could be fixed from pkgconfig.

There must something else otherwise building cvxopt should fail on the same machine.

comment:20 Changed 5 years ago by vbraun

I don't understand, its is a strong point of pkg-config that it gets library order correct regardless of the argument order.

Of course only if the pc files have the appropriate Requires: dependency information. E.g. right now our lapack.pc does this wrong, though I don't think that is the issue here.

comment:21 Changed 5 years ago by vbraun

  • Status changed from needs_review to positive_review

In any case I'm going to run this on the build bot...

comment:22 Changed 5 years ago by fbissey

I am fairly sure that's not the issue. As for the order I am fairly sure that it is due to the fact that pkgconfig (python interface) uses sets. Because the set is not ordered, order can be be changed. For the library field it would have been better to use a list I think.

comment:23 Changed 5 years ago by vbraun

Possibly, though really the order of all compiler switches potentially matters not just the libraries. So the goal should be to a) always use pc files and then b) directly use the output string from pkgconfig.libs(...) instead of pkgconfig.parse

comment:24 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

Fails to build scipy on some platforms:

Possibly due to this:

/mnt/highperf/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/numpy/distutils/system_info.py:635: UserWarning: Specified path  is invalid.
  warnings.warn('Specified path %s is invalid.' % d)
  libraries lapack,f77blas,cblas,atlas not found in []
  NOT AVAILABLE

comment:25 Changed 5 years ago by fbissey

OK. I think I see a pattern now. I have to check the system_info.py code again. The real breaking point is

libraries lapack,f77blas,cblas,atlas not found in []

the rest before is in common with successful build. It has to be a failure in finding at [] instead of the given library_dirs why it happens some times and not some other will be interesting to untangle.

comment:26 Changed 5 years ago by git

  • Commit changed from dfddf074dd2ffab07030451f6c715483bb8fad25 to fea3a2a429bdc015670a7b8726b7974d8d818689

Branch pushed to git repo; I updated commit sha1. New commits:

7808e83Small improvement and cleaning in lapack_conf.py
38007aeMerge branch 'develop' into numpy_scipy_pc
fea3a2aNot having access to a failing machine that's a bit of a stab in the dark

comment:27 Changed 5 years ago by fbissey

  • Status changed from needs_work to needs_review

Let's see what the bots make of that.

comment:28 Changed 5 years ago by fbissey

  • Branch changed from u/fbissey/numpy_scipy_pc to u/fbissey/numpy_scipy_pc_dbg
  • Commit changed from fea3a2a429bdc015670a7b8726b7974d8d818689 to f9658bac915660e2c8c680dd91d188fd20f06bff

I tried to get a machine reproducing poseidon's problems and failed. So I am switching to a debug branch in the hope that poseidon will spit out a clue for me. Do not review this branch.


New commits:

f9658baAdd debugging info in the hope to get poseidon to spit something useful

comment:29 Changed 5 years ago by fbissey

Well it spat something useful

[ALL]
library_dirs = /home/kevin/sage-patchbot/local/lib
include_dirs = /home/kevin/sage-patchbot/local/include
[blas]
library_dirs = 
blas_libs    = atlas, f77blas, cblas
[lapack]
library_dirs = 
lapack_libs  = lapack, f77blas, cblas, atlas

Back to the atlas spkg for inspection.

comment:30 Changed 5 years ago by git

  • Commit changed from f9658bac915660e2c8c680dd91d188fd20f06bff to 050dff945cd01a314975828c1ce06fb7c81935bd

Branch pushed to git repo; I updated commit sha1. New commits:

050dff9more targeted output

comment:31 Changed 5 years ago by fbissey

Still no review please.

comment:32 Changed 5 years ago by fbissey

  • Branch changed from u/fbissey/numpy_scipy_pc_dbg to u/fbissey/numpy_scipy_pc
  • Commit changed from 050dff945cd01a314975828c1ce06fb7c81935bd to fea3a2a429bdc015670a7b8726b7974d8d818689
  • Dependencies set to 20208

Random failures should now be solved by #20208.

comment:33 Changed 5 years ago by vbraun

  • Dependencies changed from 20208 to #20208

comment:34 Changed 5 years ago by fbissey

  • Milestone changed from sage-7.1 to sage-7.2

comment:35 Changed 5 years ago by git

  • Commit changed from fea3a2a429bdc015670a7b8726b7974d8d818689 to 38007ae520f8535e512aa29082681d3300b75c47

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

comment:36 Changed 5 years ago by git

  • Commit changed from 38007ae520f8535e512aa29082681d3300b75c47 to b2c95cb2f24d369703483148b6258ad7486713f2

Branch pushed to git repo; I updated commit sha1. New commits:

20e1893Do not strip out system libs in pkg-config
b2c95cbMerge branch 'keep_system_libraries_in_pkgconf' into numpy_scipy_pc

comment:37 Changed 5 years ago by fbissey

Clean up and merge #20208 so it does build on poseidon out of the box. Cross fingers.

comment:38 Changed 5 years ago by fbissey

And poseidon still fails because pkgconf hasn't been rebuild before numpy..... I guess I will have to wait until #20208 is in a beta.

comment:39 Changed 5 years ago by git

  • Commit changed from b2c95cb2f24d369703483148b6258ad7486713f2 to 41a7bdec73bb71b47d36ccccf7b073543e3fced2

Branch pushed to git repo; I updated commit sha1. New commits:

41a7bdeMerge branch 'develop' into numpy_scipy_pc

comment:40 Changed 5 years ago by fbissey

  • Dependencies #20208 deleted

Rebased on 7.2.beta0 which includes #20208.

comment:41 Changed 5 years ago by git

  • Commit changed from 41a7bdec73bb71b47d36ccccf7b073543e3fced2 to af70376a991d0770ec4a92da8155c33b7702f040

Branch pushed to git repo; I updated commit sha1. New commits:

a7869d8Merge branch 'develop' into numpy_scipy_pc
af70376try another strategy to get things going on poseidon

comment:42 Changed 5 years ago by fbissey

  • Status changed from needs_review to positive_review

Finally building on poseidon. Theory: #20208 fixes the case where there is no global pkg-config installed and sage's pkgconf is used and in that case having it not strip default library directories when present in the .pc file. But poseidon still failed, probably because it has a global pkg-config command available and it happens to be pkgconf. Because it is provided by the system #20208 doesn't fix it.

The lapack.conf script is much more robust as a result of the latest commit.

Putting back to positive review.

comment:43 Changed 5 years ago by vbraun

  • Branch changed from u/fbissey/numpy_scipy_pc to af70376a991d0770ec4a92da8155c33b7702f040
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.