Opened 5 years ago
Closed 5 years ago
#20157 closed enhancement (fixed)
make numpy and scipy use pkgconfig to find blas/lapack
Reported by:  fbissey  Owned by:  

Priority:  major  Milestone:  sage7.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 )
Since we want the choice of blas/lapack to be configurable by .pc
files we need to have numpy
and scipy
obey to pkgconfig
.
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 autodetections steps 1 to 6.
Change History (43)
comment:1 Changed 5 years ago by
comment:2 Changed 5 years ago by
 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
 Description modified (diff)
comment:4 Changed 5 years ago by
 Branch set to u/fbissey/numpy_scipy_pc
 Commit set to 3237f3805125473f2af55541ca93ddc5147b9807
New commits:
3237f38  Get numpy and scipy to use pc files

comment:5 Changed 5 years ago by
 Status changed from new to needs_review
comment:6 Changed 5 years ago by
 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
 Commit changed from 3237f3805125473f2af55541ca93ddc5147b9807 to 158fb8db9d1a112078ece92a6b33fe84fd9e6825
Branch pushed to git repo; I updated commit sha1. New commits:
158fb8d  Version bump to trigger rebuilds

comment:8 Changed 5 years ago by
 Status changed from needs_work to needs_review
 Work issues version bumps deleted
comment:9 Changed 5 years ago by
 Reviewers set to Volker Braun
 Status changed from needs_review to positive_review
comment:10 Changed 5 years ago by
 Status changed from positive_review to needs_work
/mnt/disk/home/buildslavesage/slave/sage_git/build/local/var/tmp/sage/build/numpy1.10.4.p1/src/numpy/distutils/system_info.py:1651: UserWarning: Atlas (http://mathatlas.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/buildslavesage/slave/sage_git/build/local/var/tmp/sage/build/numpy1.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/buildslavesage/slave/sage_git/build/local/lib lcblas o /tmp/tmpX0K4bQ/a.out /usr/bin/ld: warning: libatlas.so.3, needed by /mnt/disk/home/buildslavesage/slave/sage_git/build/local/lib/libcblas.so, not found (try using rpath or rpathlink)
It is missing the latlas
for some reason, though it is in the cblas.pc
$ pkgconfig libs cblas L/mnt/disk/home/buildslavesage/slave/sage_git/build/local/lib lcblas latlas
comment:11 Changed 5 years ago by
Can you look at the generated site.cfg
file?
comment:12 Changed 5 years ago by
I notice poseidon also has a problem but look slightly different.
comment:13 Changed 5 years ago by
$ cat /mnt/disk/home/buildslavesage/slave/sage_git/build/local/var/tmp/sage/build/numpy1.10.4.p1/src/site.cfg [DEFAULT] library_dirs = /mnt/disk/home/buildslavesage/slave/sage_git/build/local/lib include_dirs = /mnt/disk/home/buildslavesage/slave/sage_git/build/local/include [blas] include_dirs = library_dirs = /mnt/disk/home/buildslavesage/slave/sage_git/build/local/lib blas_libs = atlas,f77blas,cblas [lapack] library_dirs = /mnt/disk/home/buildslavesage/slave/sage_git/build/local/lib lapack_libs = lapack,f77blas,cblas,atlas
comment:14 Changed 5 years ago by
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/gitfork/sage/local/lib lcblas o /tmp/tmpRM4d1X/a.out FOUND: libraries = ['cblas'] library_dirs = ['/home/fbissey/sandbox/gitfork/sage/local/lib'] language = c define_macros = [('HAVE_CBLAS', None)] FOUND: libraries = ['cblas'] library_dirs = ['/home/fbissey/sandbox/gitfork/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
 Commit changed from 158fb8db9d1a112078ece92a6b33fe84fd9e6825 to dfddf074dd2ffab07030451f6c715483bb8fad25
Branch pushed to git repo; I updated commit sha1. New commits:
45487f9  cosmetics in lapack_conf.py. Add a patch to system_info.py to accept more than one library in blas_info (taken from gentoo).

dfddf07  Properly 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
 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
poseidon is stumping me. There is not even a compilation line
blas_info: /home/kevin/sagepatchbot/local/var/tmp/sage/build/numpy1.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 followup: ↓ 19 Changed 5 years ago by
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 asneeded
by default. We could try to pass noasneeded
but it would be better if the order could be fixed from pkgconfig
.
comment:19 in reply to: ↑ 18 Changed 5 years ago by
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 yourld
hasasneeded
by default. We could try to passnoasneeded
but it would be better if the order could be fixed frompkgconfig
.
There must something else otherwise building cvxopt
should fail on the same machine.
comment:20 Changed 5 years ago by
I don't understand, its is a strong point of pkgconfig 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
 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
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
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
 Status changed from positive_review to needs_work
Fails to build scipy on some platforms:
 http://build.sagedev.org/release/builders/%20%20slow%20Oxford%20ARM%20%28Ubuntu%2012.10%29%20incremental/builds/529
 http://build.sagedev.org/release/builders/%20%20slow%20AIMS%20%20%28Debian%208%2032%20bit%29%20incremental/builds/423
 http://build.sagedev.org/release/builders/%20%20slow%20AIMS%20%20%28Debian%207%2064%20bit%29%20incremental/builds/395
Possibly due to this:
/mnt/highperf/buildbot/slave/sage_git/build/local/lib/python2.7/sitepackages/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
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
 Commit changed from dfddf074dd2ffab07030451f6c715483bb8fad25 to fea3a2a429bdc015670a7b8726b7974d8d818689
comment:27 Changed 5 years ago by
 Status changed from needs_work to needs_review
Let's see what the bots make of that.
comment:28 Changed 5 years ago by
 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:
f9658ba  Add debugging info in the hope to get poseidon to spit something useful

comment:29 Changed 5 years ago by
Well it spat something useful
[ALL] library_dirs = /home/kevin/sagepatchbot/local/lib include_dirs = /home/kevin/sagepatchbot/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
 Commit changed from f9658bac915660e2c8c680dd91d188fd20f06bff to 050dff945cd01a314975828c1ce06fb7c81935bd
Branch pushed to git repo; I updated commit sha1. New commits:
050dff9  more targeted output

comment:31 Changed 5 years ago by
Still no review please.
comment:32 Changed 5 years ago by
 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
 Dependencies changed from 20208 to #20208
comment:34 Changed 5 years ago by
 Milestone changed from sage7.1 to sage7.2
comment:35 Changed 5 years ago by
 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
 Commit changed from 38007ae520f8535e512aa29082681d3300b75c47 to b2c95cb2f24d369703483148b6258ad7486713f2
comment:37 Changed 5 years ago by
Clean up and merge #20208 so it does build on poseidon out of the box. Cross fingers.
comment:38 Changed 5 years ago by
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
 Commit changed from b2c95cb2f24d369703483148b6258ad7486713f2 to 41a7bdec73bb71b47d36ccccf7b073543e3fced2
Branch pushed to git repo; I updated commit sha1. New commits:
41a7bde  Merge branch 'develop' into numpy_scipy_pc

comment:40 Changed 5 years ago by
 Dependencies #20208 deleted
Rebased on 7.2.beta0 which includes #20208.
comment:41 Changed 5 years ago by
 Commit changed from 41a7bdec73bb71b47d36ccccf7b073543e3fced2 to af70376a991d0770ec4a92da8155c33b7702f040
comment:42 Changed 5 years ago by
 Status changed from needs_review to positive_review
Finally building on poseidon. Theory: #20208 fixes the case where there is no global pkgconfig
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 pkgconfig
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
 Branch changed from u/fbissey/numpy_scipy_pc to af70376a991d0770ec4a92da8155c33b7702f040
 Resolution set to fixed
 Status changed from positive_review to closed
Actually
shuts down all detection so there is hope things can be taken done selectively.