Opened 6 years ago
Closed 4 years ago
#17630 closed defect (wontfix)
Further clean up of ATLAS (or equivalent BLAS) linking
Reported by: | jpflori | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | packages: standard | Keywords: | |
Cc: | jdemeyer, vbraun, fbissey | Merged in: | |
Authors: | Jean-Pierre Flori, François Bissey | Reviewers: | |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Different pkgs find ATLAS (or equivalent libs e.g. on OS X and Cygwin) libs in different ways. In particular the situation is broken on Cygwin.
The one we tweak ourselves are:
- ATLAS when we pass SAGE_ATLAS_LIB to decide what to copy/symlink
- fflas-ffpack
- iml
- R
- the Sage library through SAGE_BLAS and module_list.py and its BLAS/BLAS2 madness
The ones we hack ourselves:
- cvxopt (we patch and hardcode in setup.py).
Other pkgs depend on a BLAS implem but are autonomous(?):
- gsl,
- scipy (envvars are unset...),
- numpy (same same as scipy).
My proposal before a pkgconfig solution gets available at #17075 is that when ATLAS spkg-install script is run it decides what should be used once and for all and other packages use this information (like used to be done a little bit when the linbox spkg-install script wrote echo $LINBOX_BLAS > $SAGE_LOCAL/share/cblas_config
)
Change History (105)
comment:1 Changed 6 years ago by
- Description modified (diff)
comment:2 Changed 6 years ago by
- Description modified (diff)
comment:3 Changed 6 years ago by
comment:4 Changed 6 years ago by
Fot cvxopt: http://cvxopt.org/install/
comment:5 Changed 6 years ago by
Yes definitely of interest. I think cvxopt 1.1.7 is better at handling things with environment variables so we shouldn't patch anymore, I should update the cvxopt ebuild for gentoo. From cvxopt setup.py
BLAS_NOUNDERSCORES = int(os.environ.get("CVXOPT_BLAS_NOUNDERSCORES",BLAS_NOUNDERSCORES)) == True BLAS_LIB = os.environ.get("CVXOPT_BLAS_LIB",BLAS_LIB) LAPACK_LIB = os.environ.get("CVXOPT_LAPACK_LIB",LAPACK_LIB) BLAS_LIB_DIR = os.environ.get("CVXOPT_BLAS_LIB_DIR",BLAS_LIB_DIR) BLAS_EXTRA_LINK_ARGS = os.environ.get("CVXOPT_BLAS_EXTRA_LINK_ARGS",BLAS_EXTRA_LINK_ARGS) if type(BLAS_LIB) is str: BLAS_LIB = BLAS_LIB.strip().split(',') if type(LAPACK_LIB) is str: LAPACK_LIB = LAPACK_LIB.strip().split(',') if type(BLAS_EXTRA_LINK_ARGS) is str: BLAS_EXTRA_LINK_ARGS = BLAS_EXTRA_LINK_ARGS.strip().split(',') BUILD_GSL = int(os.environ.get("CVXOPT_BUILD_GSL",BUILD_GSL)) GSL_LIB_DIR = os.environ.get("CVXOPT_GSL_LIB_DIR",GSL_LIB_DIR) GSL_INC_DIR = os.environ.get("CVXOPT_GSL_INC_DIR",GSL_INC_DIR) BUILD_FFTW = int(os.environ.get("CVXOPT_BUILD_FFTW",BUILD_FFTW)) FFTW_LIB_DIR = os.environ.get("CVXOPT_FFTW_LIB_DIR",FFTW_LIB_DIR) FFTW_INC_DIR = os.environ.get("CVXOPT_FFTW_INC_DIR",FFTW_INC_DIR) BUILD_GLPK = int(os.environ.get("CVXOPT_BUILD_GLPK",BUILD_GLPK)) GLPK_LIB_DIR = os.environ.get("CVXOPT_GLPK_LIB_DIR",GLPK_LIB_DIR) GLPK_INC_DIR = os.environ.get("CVXOPT_GLPK_INC_DIR",GLPK_INC_DIR) BUILD_DSDP = int(os.environ.get("CVXOPT_BUILD_DSDP",BUILD_DSDP)) DSDP_LIB_DIR = os.environ.get("CVXOPT_DSDP_LIB_DIR",DSDP_LIB_DIR) DSDP_INC_DIR = os.environ.get("CVXOPT_DSDP_INC_DIR",DSDP_INC_DIR)
gsl makes its own cblas (but no fortran blas) and by default libgsl is not linked with any cblas, you are supposed to link with a cblas when you use the library. Setting blas/lapack in numpy/scipy makes me crazy half the time. Environment variable or setup.cfg can be used to change the default (currently looking for MKL, ATLAS, netlib and netlib source in that order if I am not mistaken). Making a setup.cfg from pkg-config is hard
pc_incdir() { $(tc-getPKG_CONFIG) --cflags-only-I $@ | \ sed -e 's/^-I//' -e 's/[ ]*-I/:/g' -e 's/[ ]*$//' -e 's|^:||' } pc_libdir() { $(tc-getPKG_CONFIG) --libs-only-L $@ | \ sed -e 's/^-L//' -e 's/[ ]*-L/:/g' -e 's/[ ]*$//' -e 's|^:||' } pc_libs() { $(tc-getPKG_CONFIG) --libs-only-l $@ | \ sed -e 's/[ ]-l*\(pthread\|m\)\([ ]\|$\)//g' \ -e 's/^-l//' -e 's/[ ]*-l/,/g' -e 's/[ ]*$//' \ | tr ',' '\n' | sort -u | tr '\n' ',' | sed -e 's|,$||' } local libdir="${EPREFIX}"/usr/$(get_libdir) # local tree # make sure _dotblas.so gets built sed -i -e '/NO_ATLAS_INFO/,+1d' numpy/core/setup.py || die cat >> site.cfg <<-EOF [blas] include_dirs = $(pc_incdir cblas) library_dirs = $(pc_libdir cblas blas):${libdir} blas_libs = $(pc_libs cblas blas) [lapack] library_dirs = $(pc_libdir lapack):${libdir} lapack_libs = $(pc_libs lapack) EOF
but with the python binding it should be easier.
comment:6 Changed 6 years ago by
- Branch set to u/jpflori/ticket/17630
- Cc jdemeyer vbraun fbissey added
- Commit set to b02e1593daa51b92401eed7746efcdc665b31388
- Status changed from new to needs_review
This is definitely not really needs_reviex, in particular, OS X surely needs further tweaking, not sure that passing -lblas
or lcblas
would always work, or if one sometines needs to pass the -framework
stuff...
I've basically tested it on Linux and Cygwin and it looks ok.
Anyway, I won't be able to do anything for at leasr one month, so here is the current state of affair. Feel free to finalize and merge it!
Last 10 new commits:
79c712c | Simplify BLAS/LAPACK config for IML.
|
030b90b | Simplify BLAS/LAPACK config for numpy.
|
2a14692 | Simplify BLAS/LAPACK config for Sage library.
|
355fff8 | Let ATLAS install script write cblas_config.
|
14675a5 | Remove SAGE_CBLAS documentation.
|
cf79fc3 | First try to simplify BLAS/LAPACK configuration for cvxopt.
|
534ec94 | Fix cblas_config mechanism.
|
09e8128 | Further modifications to numpy/scipy build systems.
|
37abe37 | Slight cleanup of cvxopt directory.
|
b02e159 | Fixes for cleaner BLAS config.
|
comment:7 follow-up: ↓ 8 Changed 6 years ago by
Also note that part of the changes here have been shamelessly stolen from sage-on-gentoo patches posted on #17075.
comment:8 in reply to: ↑ 7 Changed 6 years ago by
Replying to jpflori:
Also note that part of the changes here have been shamelessly stolen from sage-on-gentoo patches posted on #17075.
You didn't go all the way with libdir, but you certainly stole the spirit of it. The libdir part is important if we use an external blas that is not in a standard directory. Hum.... may be I should add a runpath for that case, at the very least LD_LIBRARY_PATH or its equivalent needs to be set.
comment:9 Changed 6 years ago by
Please note that the idea here is just to clean up how we use our default (and unique) BLAS. We should not try to do everything at once!
comment:10 Changed 6 years ago by
Well, I'd still like to include libdir as forward thinking - that and it makes my life easier sage-on-gentoo side - of course lots of things land in sage that make my life ugly but I'd like to minimize it if I can.
comment:11 Changed 6 years ago by
Any chance to get this merged and go on working in a follow up ticket?
comment:12 Changed 6 years ago by
Missed that. Probably to late for 6.6 but we should be able to rebase for early inclusion in 6.7.
comment:13 Changed 6 years ago by
- Commit changed from b02e1593daa51b92401eed7746efcdc665b31388 to 091e941e88d7509f6fffe44785136ae27287aab2
Branch pushed to git repo; I updated commit sha1. New commits:
091e941 | Merge remote-tracking branch 'trac/develop' into ticket/17630
|
comment:14 Changed 6 years ago by
Here is what is happening when you try to use Accelerate in R on OS X
gcc -std=gnu99 -dynamiclib -Wl,-headerpad_max_install_names -undefined dynamic_lookup -single_module -multiply_defined suppress -L../../../lib -L/Users/fbissey/build/sage-6.5.beta1/local/lib/ -o internet.so Rhttpd.o Rsock.o internet.o nanoftp.o nanohttp.o sock.o sockconn.o -lR -Wl,-framework -Wl,CoreFoundation mkdir /Users/fbissey/build/sage-6.5.beta1/local/var/tmp/sage/build/r-3.1.2.p0/src/modules making Lapack.d from Lapack.c making vecLibg95c.d from vecLibg95c.c <built-in>: warning: subframework include vecLib/vecLib.h conflicts with framework include <built-in>: warning: subframework include vecLib/vecLibTypes.h conflicts with framework include <built-in>: warning: subframework include vecLib/vBasicOps.h conflicts with framework include <built-in>: warning: subframework include vecLib/vBigNum.h conflicts with framework include <built-in>: warning: subframework include vecLib/vectorOps.h conflicts with framework include <built-in>: warning: subframework include vecLib/vfp.h conflicts with framework include <built-in>: warning: subframework include vecLib/vDSP.h conflicts with framework include <built-in>: warning: subframework include vecLib/cblas.h conflicts with framework include <built-in>: warning: subframework include vecLib/clapack.h conflicts with framework include <built-in>: warning: subframework include vecLib/LinearAlgebra/LinearAlgebra.h conflicts with framework include <built-in>: warning: subframework include vecLib/LinearAlgebra/base.h conflicts with framework include In file included from /usr/include/os/object.h:27:0, from /System/Library/Frameworks/Accelerate.framework/Frameworks/vecLib.framework/Headers/LinearAlgebra/base.h:6, from /System/Library/Frameworks/Accelerate.framework/Frameworks/vecLib.framework/Headers/LinearAlgebra/LinearAlgebra.h:10, from /System/Library/Frameworks/Accelerate.framework/Frameworks/vecLib.framework/Headers/vecLib.h:65, from /System/Library/Frameworks/Accelerate.framework/Headers/Accelerate.h:20, from vecLibg95c.c:8: /usr/include/os/base.h:113:20: error: missing binary operator before token "(" #if __has_extension(attribute_overloadable) ^ /usr/include/os/base.h:119:54: error: missing binary operator before token "(" #if __has_feature(objc_fixed_enum) || __has_extension(cxx_strong_enums) ^ <built-in>: warning: subframework include vecLib/LinearAlgebra/object.h conflicts with framework include <built-in>: warning: subframework include vecLib/LinearAlgebra/matrix.h conflicts with framework include <built-in>: warning: subframework include vecLib/LinearAlgebra/vector.h conflicts with framework include <built-in>: warning: subframework include vecLib/LinearAlgebra/splat.h conflicts with framework include <built-in>: warning: subframework include vecLib/LinearAlgebra/arithmetic.h conflicts with framework include <built-in>: warning: subframework include vecLib/LinearAlgebra/linear_systems.h conflicts with framework include <built-in>: warning: subframework include vecLib/LinearAlgebra/norms.h conflicts with framework include <built-in>: warning: subframework include vecLib/vForce.h conflicts with framework include make[3]: *** [vecLibg95c.d] Error 1 make[2]: *** [make.lapack] Error 2 make[1]: *** [R] Error 1 make: *** [R] Error 1 Error building R.
BOOOM!
comment:15 Changed 6 years ago by
Ok, so let's not use it.
comment:16 Changed 6 years ago by
If you want to add the "libdir" stuff, I'll happily review it.
comment:17 follow-up: ↓ 18 Changed 6 years ago by
Other than that little details:
- I would prefer cblas (C) and blas (f77) to be stored separately.
- Do you really want to enable lapack in fflas-ffpack/linbox? Is there a real point to it?
I would like to add the libdir stuff that would make my life easier thank you.
The point about accelerate on OS X is there was a question in the spkg-install so I set out to find the answer.
comment:18 in reply to: ↑ 17 Changed 6 years ago by
Replying to fbissey:
Other than that little details:
- I would prefer cblas (C) and blas (f77) to be stored separately.
Sure, that makes sense.
- Do you really want to enable lapack in fflas-ffpack/linbox? Is there a real point to it?
In linbox itself it is not used. I don't remember about the fflas-ffpack part...
I would like to add the libdir stuff that would make my life easier thank you.
The point about accelerate on OS X is there was a question in the spkg-install so I set out to find the answer.
Ah, thanks!
comment:19 Changed 6 years ago by
Well, you added lapack to fflas-ffpack and added lapack for linbox in module_list.py. If that doesn't enable any extra functionality or anything we should do away with it.
comment:20 Changed 6 years ago by
It might enhance performance, I did not check. I only know for sure that fflas-ffpack can be configure to (potentially) use LAPACK. If you don't feel safe with this, we can postpone such an inclusion and deal with it in #14390.
comment:21 follow-up: ↓ 22 Changed 6 years ago by
I would postpone the fflas-ffpack stuff, don't you have a series of ticket for givaro/fflas-ffpack/linbox update? We could also do that when we get around it.
comment:22 in reply to: ↑ 21 Changed 6 years ago by
Replying to fbissey:
I would postpone the fflas-ffpack stuff, don't you have a series of ticket for givaro/fflas-ffpack/linbox update? We could also do that when we get around it.
Yes: #17635 and friends. Note we'll have to wait for a fflas-ffpack 2.1 release and linbox x.y release as the current versions of the three libs are not compatible. They should have come out in march, but didn't and Clément did not answer my latest mail.
comment:23 Changed 6 years ago by
- Commit changed from 091e941e88d7509f6fffe44785136ae27287aab2 to 882d72030492da6519301fc8fcadd36093251af4
Branch pushed to git repo; I updated commit sha1. New commits:
882d720 | Wrong conflict resolution for numpy.
|
comment:24 Changed 6 years ago by
- Commit changed from 882d72030492da6519301fc8fcadd36093251af4 to 73a21a99499891e5b21552888da9cfa611e53061
Branch pushed to git repo; I updated commit sha1. New commits:
73a21a9 | Second try for numpy fixing.
|
comment:25 Changed 6 years ago by
- Commit changed from 73a21a99499891e5b21552888da9cfa611e53061 to a9d2dcf1344c005d6796ec288181451447124718
Branch pushed to git repo; I updated commit sha1. New commits:
a9d2dcf | And final fixes.
|
comment:26 Changed 6 years ago by
Do you have further things to commit or should I start putting my own comments into action?
comment:27 Changed 6 years ago by
- Commit changed from a9d2dcf1344c005d6796ec288181451447124718 to 045266e53354e7f3badb2e2b7dcd4f2c0cea2fa2
Branch pushed to git repo; I updated commit sha1. New commits:
045266e | Final fix for a correct conflict resolution.
|
comment:28 Changed 6 years ago by
I think I've sanitized the situation after my miserable failure to correctly resolve the conflict for numpy spkg-install
script between this ticket and #18142.
Please go ahead.
comment:29 Changed 6 years ago by
Is this ticket really needs_review
or are further changes planned (in the latter case, set the status to needs_work
please).
comment:30 Changed 6 years ago by
- Status changed from needs_review to needs_work
There are a couple more changes we are/ I am planning. I should have put it back to "needs_work" earlier. I should be able to further work on this ticket tomorrow.
comment:31 Changed 6 years ago by
When you think you're done with this ticket, you should really ask Volker to test it on the buildbots.
comment:32 Changed 6 years ago by
Oh, yes indeed.
comment:33 Changed 6 years ago by
FYI: this will conflict with #18450.
comment:34 Changed 6 years ago by
I think it will be better to rebase on top of #18450 I will have to rebase the corresponding sage-on-gentoo patch anyway and it will be less patching in the end.
comment:35 Changed 6 years ago by
François, any progress on that front?
comment:36 Changed 6 years ago by
Sorry but no, I have got side-tracked by the upgrade of matplotlib ticket #17618 and I was thinking bumping numpy to 1.9.2 at #17642 would be trivial but it isn't and I am kind of stuck on it. Working on this ticket could actually be useful distraction but I am busy right now trying to get something scheduling properly on an AIX cluster.
comment:37 follow-up: ↓ 38 Changed 6 years ago by
Whenever you work on this ticket, please do it in the spirit of #18450: it is better to define libraries in the .pxd
files of the libraries if possible.
comment:38 in reply to: ↑ 37 ; follow-up: ↓ 39 Changed 6 years ago by
Replying to jdemeyer:
Whenever you work on this ticket, please do it in the spirit of #18450: it is better to define libraries in the
.pxd
files of the libraries if possible.
Just so I am sure I understand correctly:
# distutils: libraries = gmp pari
for example will add "-lgmp -lpari" to the linking line (possibly not in that order since it could break)? Is it possible to read the correct library or import from another file? I am asking because our forward thinking is to move the definition of the blas/lapack stuff to .pc
files and be able to choose between several implementations that won't all have the same name.
comment:39 in reply to: ↑ 38 Changed 6 years ago by
Replying to fbissey:
Just so I am sure I understand correctly:
# distutils: libraries = gmp parifor example will add "-lgmp -lpari" to the linking line
Yes indeed.
possibly not in that order since it could break
There is a global order library_order_list
defined in src/module_list.py
. Libraries are sorted according to that order, so the order given in the # distutils
declaration is not relevant (unless the library is not listed in library_order_list
).
Is it possible to read the correct library or import from another file?
Yes, see the aliases
variable in src/module_list.py
Just define whatever you need in that variable and then use the alias name in the # distutils
declaration.
comment:40 Changed 6 years ago by
As far as I know, nothing in Sage directly uses BLAS, it is only used through external libraries (linbox, numpy, ...)
So it's a mystery why some extensions in src/module_list.py
list the BLAS libraries without any other library. And indeed, on my Linux box, Sage still works after the following change:
-
src/module_list.py
diff --git a/src/module_list.py b/src/module_list.py index e01cd4b..7d818d6 100755
a b ext_modules = [ 1024 1024 sources = ['sage/matrix/echelon_matrix.pyx']), 1025 1025 1026 1026 Extension('sage.matrix.change_ring', 1027 sources = ['sage/matrix/change_ring.pyx'], 1028 libraries=[BLAS, BLAS2]), 1027 sources = ['sage/matrix/change_ring.pyx']), 1029 1028 1030 1029 Extension('sage.matrix.matrix', 1031 1030 sources = ['sage/matrix/matrix.pyx']), … … ext_modules = [ 1040 1039 sources = ['sage/matrix/matrix2.pyx']), 1041 1040 1042 1041 Extension('sage.matrix.matrix_complex_double_dense', 1043 sources = ['sage/matrix/matrix_complex_double_dense.pyx'], 1044 libraries=[BLAS, BLAS2]), 1042 sources = ['sage/matrix/matrix_complex_double_dense.pyx']), 1045 1043 1046 1044 Extension('sage.matrix.matrix_cyclo_dense', 1047 1045 sources = ['sage/matrix/matrix_cyclo_dense.pyx'], … … ext_modules = [ 1052 1050 sources = ['sage/matrix/matrix_dense.pyx']), 1053 1051 1054 1052 Extension('sage.matrix.matrix_double_dense', 1055 sources = ['sage/matrix/matrix_double_dense.pyx'], 1056 libraries=[BLAS, BLAS2]), 1053 sources = ['sage/matrix/matrix_double_dense.pyx']), 1057 1054 1058 1055 Extension('sage.matrix.matrix_generic_dense', 1059 1056 sources = ['sage/matrix/matrix_generic_dense.pyx']), … … ext_modules = [ 1116 1113 sources = ['sage/matrix/matrix_rational_sparse.pyx']), 1117 1114 1118 1115 Extension('sage.matrix.matrix_real_double_dense', 1119 sources = ['sage/matrix/matrix_real_double_dense.pyx'], 1120 libraries=[BLAS, BLAS2]), 1116 sources = ['sage/matrix/matrix_real_double_dense.pyx']), 1121 1117 1122 1118 Extension('sage.matrix.matrix_sparse', 1123 1119 sources = ['sage/matrix/matrix_sparse.pyx']), … … ext_modules = [ 1254 1250 sources = ['sage/modules/module.pyx']), 1255 1251 1256 1252 Extension('sage.modules.vector_complex_double_dense', 1257 ['sage/modules/vector_complex_double_dense.pyx'], 1258 libraries = [BLAS, BLAS2]), 1253 ['sage/modules/vector_complex_double_dense.pyx']), 1259 1254 1260 1255 Extension('sage.modules.vector_double_dense', 1261 ['sage/modules/vector_double_dense.pyx'], 1262 libraries = [BLAS, BLAS2]), 1256 ['sage/modules/vector_double_dense.pyx']), 1263 1257 1264 1258 Extension('sage.modules.vector_integer_dense', 1265 1259 sources = ['sage/modules/vector_integer_dense.pyx']), … … ext_modules = [ 1278 1272 sources = ['sage/modules/vector_rational_dense.pyx']), 1279 1273 1280 1274 Extension('sage.modules.vector_real_double_dense', 1281 ['sage/modules/vector_real_double_dense.pyx'], 1282 libraries = [BLAS, BLAS2]), 1275 ['sage/modules/vector_real_double_dense.pyx']), 1283 1276 1284 1277 ################################ 1285 1278 ##
Perhaps those libraries are needed for some reason (I know Linux in particular is very flexible with defining libraries) but it's something to be investigated...
comment:42 Changed 6 years ago by
- Dependencies changed from #18777 to #18777, #18778
comment:43 Changed 6 years ago by
I did a serious clean-up of the Cython interface to GSL, see #18778.
comment:44 Changed 6 years ago by
- Branch changed from u/jpflori/ticket/17630 to u/fbissey/blas
- Commit changed from 045266e53354e7f3badb2e2b7dcd4f2c0cea2fa2 to 217c051edd4d06957cf82893f5c4d97e9ac1c654
This is just merging the previous work of Jean-Pierre into 6.8.beta5 and solving the conflict. There are a couple more things I want to do. Then I should explore transferring libraries to .pxd
files.
New commits:
217c051 | Merge branch 'develop' into blas
|
comment:45 Changed 6 years ago by
- Commit changed from 217c051edd4d06957cf82893f5c4d97e9ac1c654 to d448831b044c43b1190ece92ac6c4b60eb332d7c
Branch pushed to git repo; I updated commit sha1. New commits:
d448831 | Forgot a very important "+" in module_list.py
|
comment:46 Changed 6 years ago by
I would move cblas_libs()
to sage/env.py
, analogous to sage_include_directories()
.
And this
f = open(os.path.join(SAGE_LOCAL, 'share/cblas_config'), 'r') libs = f.readline().split() f.close() return libs
can be written as
with open(os.path.join(SAGE_SHARE, 'cblas_config')) as f: return f.readline().split()
comment:47 Changed 6 years ago by
That's good suggestions. I will do that once I finished my current round of nitpicking.
comment:48 Changed 6 years ago by
One more nitpicking: replace
cat x | sed ...
by
sed ... x
comment:49 follow-ups: ↓ 50 ↓ 54 Changed 6 years ago by
I would also like to know the justification for this in sage-env
:
-if [ "$LDFLAGS" = "" ]; then - LDFLAGS="" && export LDFLAGS +if [ -z "$LDFLAGS" ]; then + unset LDFLAGS fi
comment:50 in reply to: ↑ 49 Changed 6 years ago by
Replying to jdemeyer:
I would also like to know the justification for this in
sage-env
:-if [ "$LDFLAGS" = "" ]; then - LDFLAGS="" && export LDFLAGS +if [ -z "$LDFLAGS" ]; then + unset LDFLAGS fi
That must have come from one of Jean-Pierre's commit. Difference is very subtle and I am not sure it matters but I am wondering about the whole point of the test.
comment:51 follow-up: ↓ 55 Changed 6 years ago by
Instead of
IML_BLAS=--with-cblas="$cblas_libs" ./configure "$IML_BLAS"
with awkward quoting, why not simply
./configure --with-cblas="$cblas_libs"
(same for FFLAS_FFPACK_BLAS
)
comment:52 Changed 6 years ago by
Also please use $IML_CONFIGURE
instead of "$IML_CONFIGURE"
(such variables are usually not quoted).
comment:53 follow-up: ↓ 59 Changed 6 years ago by
I would also like to know the justification for the removal of
build/pkgs/numpy/patches/cygwin-lapack_lite-setup.py.diff
comment:54 in reply to: ↑ 49 ; follow-up: ↓ 57 Changed 6 years ago by
Replying to jdemeyer:
I would also like to know the justification for this in
sage-env
:-if [ "$LDFLAGS" = "" ]; then - LDFLAGS="" && export LDFLAGS +if [ -z "$LDFLAGS" ]; then + unset LDFLAGS fi
Because this does not do the same thing :).
IIRC the first option exports LDFLAGS = ""
when LDFLAGS is not set.
Or something in this spirit.
This can cause trouble later on (and I thing I was hit by that).
Whatsoever IIRC once again, the "new" treatment for LDFLAGS is similar to the one for CFLAGS and CPPFLAGS.
comment:55 in reply to: ↑ 51 ; follow-up: ↓ 67 Changed 6 years ago by
Replying to jdemeyer:
Instead of
IML_BLAS=--with-cblas="$cblas_libs" ./configure "$IML_BLAS"with awkward quoting, why not simply
./configure --with-cblas="$cblas_libs"(same for
FFLAS_FFPACK_BLAS
)
I don't really remember, but both might work as well. IIRC FFLAS-FFPACK configure scripts is very sensitive to quoting and spaces and it was a nightmare to make it accept multiple libraries to be linked separated by spaces.
comment:56 follow-up: ↓ 58 Changed 6 years ago by
Same for the removal of
build/pkgs/cvxopt/patches/setup.py.patch
It's not at all clear whether these changes are related to BLAS setup. Perhaps all non-BLAS-related changes should be factored out to a different ticket, that would make reviewing easier.
comment:57 in reply to: ↑ 54 Changed 6 years ago by
Replying to jpflori:
Because this does not do the same thing :).
That's exactly why such changes shouldn't be smuggled inside a ticket dealing with BLAS.
comment:58 in reply to: ↑ 56 ; follow-up: ↓ 60 Changed 6 years ago by
Replying to jdemeyer:
Same for the removal of
build/pkgs/cvxopt/patches/setup.py.patchIt's not at all clear whether these changes are related to BLAS setup.
This patch is a dirty hack to configure cvxopt to use BLAS related stuff.
Perhaps all non-BLAS-related changes should be factored out to a different ticket, that would make reviewing easier.
Sure that would be the best way to go.
comment:59 in reply to: ↑ 53 Changed 6 years ago by
Replying to jdemeyer:
I would also like to know the justification for the removal of
build/pkgs/numpy/patches/cygwin-lapack_lite-setup.py.diff
I just remember this patch looked fishy and useless and I had no problem not applying it.
comment:60 in reply to: ↑ 58 Changed 6 years ago by
comment:61 Changed 6 years ago by
I guess we can split some of the cvxopt stuff to make things clearer. Prior to 1.1.7 it was difficult to set any library properly without patching. setup.py
was requiring manual edit before installation unless you setup was closely matching the author. 1.1.7 introduced some mechanism to pass library settings from the environment and we just decided to do that at the same time. But a separate ticket is no problem.
comment:62 follow-up: ↓ 66 Changed 6 years ago by
Now that you explain the cvxopt
thing, it's clearer.
comment:63 Changed 6 years ago by
- Commit changed from d448831b044c43b1190ece92ac6c4b60eb332d7c to 5c9e7a50cbc9b97db2c560e18d08294b33b768e7
Branch pushed to git repo; I updated commit sha1. New commits:
5c9e7a5 | Proper insertion in library_order
|
comment:64 Changed 6 years ago by
I'll do some more work over the week end, the last commit is just so that sage with that branch is in a building state.
comment:65 Changed 6 years ago by
For this to work properly, you will need to increase the patchlevel of the ATLAS package, forcing it to be rebuilt. For the other packages, I don't think it's needed if they depend on ATLAS.
comment:66 in reply to: ↑ 62 Changed 6 years ago by
Replying to jdemeyer:
Now that you explain the
cvxopt
thing, it's clearer.
Do you still want a separate ticket?
comment:67 in reply to: ↑ 55 Changed 6 years ago by
Replying to jpflori:
Replying to jdemeyer:
Instead of
IML_BLAS=--with-cblas="$cblas_libs" ./configure "$IML_BLAS"with awkward quoting, why not simply
./configure --with-cblas="$cblas_libs"(same for
FFLAS_FFPACK_BLAS
)I don't really remember, but both might work as well. IIRC FFLAS-FFPACK configure scripts is very sensitive to quoting and spaces and it was a nightmare to make it accept multiple libraries to be linked separated by spaces.
In both case the variable is only defined on cygwin/darwin and we rely on autodetection in other cases. I guess we could throw the autodetection throw the window and put the stuff in every case. In fact if we want to be able to use something else than ATLAS in the future it is prudent to do so. So I will throw the particular cases out. If there is a strong case to still keep something for cygwin I am all ears.
comment:68 Changed 6 years ago by
- Commit changed from 5c9e7a50cbc9b97db2c560e18d08294b33b768e7 to eda2eb638d7c10f36053aa43474bb2fd21cd4db2
Branch pushed to git repo; I updated commit sha1. New commits:
cc5fefc | Introduce a finer grain of configuration between blas/cblas/lapack. In the future we want to move to .pc file and the configuration will have the same granularity.
|
57cbf6f | Clean up spkg-install of all packages affected by the changes in the ATLAS spkg. The new model in anticipation of future changes is to not rely on autodetection of
|
eda2eb6 | Centralize the reading of SAGE_SHARE/*_config files in sage/env.py where it can be accessed by setup.py/module_list.py and sage/misc/cython.py.
|
comment:69 follow-up: ↓ 70 Changed 6 years ago by
I'll happily field comments on the new changes but I will otherwise start moving libraries to the .pxd
files. After Jeroen clean up of the other tickets that's a total of 5 files (plus one optional file) so not a big job but I'll probably end with some redundancies on my first go (i.e. stuff that's not needed thanks to other declarations in .pxd
).
comment:70 in reply to: ↑ 69 ; follow-up: ↓ 71 Changed 6 years ago by
Replying to fbissey:
I'll happily field comments on the new changes but I will otherwise start moving libraries to the
.pxd
files.
I'd do that in a new ticket. There is already a lot of stuff on this ticket and I wouldn't add anything more.
I might not have much time these days to look at this ticket, so you can just go ahead. I am still very suspicious about the change to LDFLAGS
in sage-env
: it's a seemingly innocent change but with so much potential to break stuff. For handling CFLAGS
, LDFLAGS
,... in Sage we have always treated "unset" and "set to empty" equivalently. So such a change shouldn't be made lightly.
Two other small things I noticed now: there is some strange indentation in atlas/spkg-install
, be sure to use mutiples of 4 spaces everywhere. And of course get_libs_config()
needs a doctest.
comment:71 in reply to: ↑ 70 ; follow-up: ↓ 72 Changed 6 years ago by
Replying to jdemeyer:
Replying to fbissey:
I'll happily field comments on the new changes but I will otherwise start moving libraries to the
.pxd
files.I'd do that in a new ticket. There is already a lot of stuff on this ticket and I wouldn't add anything more.
That's a fair comment we touched things all over the place and some more will need to be "aligned" eventually.
I might not have much time these days to look at this ticket, so you can just go ahead. I am still very suspicious about the change to
LDFLAGS
insage-env
: it's a seemingly innocent change but with so much potential to break stuff. For handlingCFLAGS
,LDFLAGS
,... in Sage we have always treated "unset" and "set to empty" equivalently. So such a change shouldn't be made lightly.
I am a bit concerned that there is even a need for these tests and setting/unsetting. It just feel wrong so I am probably not the best person to pass a judgement on that particular change, my own reaction is to make the whole thing disappear.
Two other small things I noticed now: there is some strange indentation in
atlas/spkg-install
, be sure to use mutiples of 4 spaces everywhere. And of courseget_libs_config()
needs a doctest.
I will check the indent ASAP and I was afraid you would ask for that doctest, but I know what to do.
@Jean-Pierre, any comments on the last few commits? As can be surmised by comments we'll eventually touch again some packages using blas/cblas/lapack
to enforce settings in the future. But as Jeroen says there is enough here already.
comment:72 in reply to: ↑ 71 ; follow-up: ↓ 74 Changed 6 years ago by
Replying to fbissey:
Replying to jdemeyer:
Replying to fbissey:
I'll happily field comments on the new changes but I will otherwise start moving libraries to the
.pxd
files.I'd do that in a new ticket. There is already a lot of stuff on this ticket and I wouldn't add anything more.
That's a fair comment we touched things all over the place and some more will need to be "aligned" eventually.
I might not have much time these days to look at this ticket, so you can just go ahead. I am still very suspicious about the change to
LDFLAGS
insage-env
: it's a seemingly innocent change but with so much potential to break stuff. For handlingCFLAGS
,LDFLAGS
,... in Sage we have always treated "unset" and "set to empty" equivalently. So such a change shouldn't be made lightly.I am a bit concerned that there is even a need for these tests and setting/unsetting. It just feel wrong so I am probably not the best person to pass a judgement on that particular change, my own reaction is to make the whole thing disappear.
My only point is that now LDFLAGS
just follows what CFLAGS
does and gave me a better behavior.
So I don't agree that we've always treated LDFLAGS and CFLAGS (and CXXFLAGS) in the same way when they are unset and empty.
The problem with the current behavior of LDFLAGS, is that if LDFLAGS
is not set, then whe get LDFLAGS=""
.
And some packages won't touch LDFLAGS if LFDLAGS=""
.
If you feel worried about this, then just remove this change.
For the current behavior of CFLAGS, what you get is that if CFLAGS is empty, then it's unset, that might not be the best behavior as well...
Two other small things I noticed now: there is some strange indentation in
atlas/spkg-install
, be sure to use mutiples of 4 spaces everywhere. And of courseget_libs_config()
needs a doctest.I will check the indent ASAP and I was afraid you would ask for that doctest, but I know what to do.
@Jean-Pierre, any comments on the last few commits? As can be surmised by comments we'll eventually touch again some packages using
blas/cblas/lapack
to enforce settings in the future. But as Jeroen says there is enough here already.
Yeah I generally agree with going step by step even if the current solution is not perfect.
I also spotted a few indentation issues in atlas/spkg-install
and src/sage/env.py
comment:73 Changed 6 years ago by
- Commit changed from eda2eb638d7c10f36053aa43474bb2fd21cd4db2 to 83fbfcf79a435d04c1bc7c90b1ad7ff4510797db
Branch pushed to git repo; I updated commit sha1. New commits:
83fbfcf | Fix indent in ATLAS spkg-install, add a doctest for get_libs_config.
|
comment:74 in reply to: ↑ 72 Changed 6 years ago by
- Status changed from needs_work to needs_review
Replying to jpflori:
I also spotted a few indentation issues in
atlas/spkg-install
andsrc/sage/env.py
Found some stuff in spkg-install
but cannot see anything in env.py
, if there is something you'll have to provide a line number. I am putting this for review now.
comment:75 follow-up: ↓ 76 Changed 6 years ago by
- Status changed from needs_review to needs_work
49 hasn't been addressed.
comment:76 in reply to: ↑ 75 ; follow-up: ↓ 78 Changed 6 years ago by
Replying to jdemeyer:
49 hasn't been addressed.
While I find the argument that LDFLAGS
should be treated just the same as {C,CXX}FLAGS
persuasive, I'll surrender to the fact that it belongs to another ticket that I'll happily review positively. Waiting for my OS X build to finish before committing that change.
comment:77 Changed 6 years ago by
Yeah, just postpone it to another ticket (but also make sure that our treatment of LDFLAGS does not break CVXOPT or other package linking to BLAS which need to be fed stuff through LDFLAGS).
comment:78 in reply to: ↑ 76 ; follow-up: ↓ 80 Changed 6 years ago by
Replying to fbissey:
While I find the argument that
LDFLAGS
should be treated just the same as{C,CXX}FLAGS
persuasive
The treating of {C,CXX}FLAGS
changed recently in #16149 and I don't understand why that change was needed.
Personally, I would prefer to not treat empty and non-empty FOOFLAGS
fundamentally different (i.e. I would revert #16149 and leave LDFLAGS
as they are).
comment:79 Changed 6 years ago by
- Commit changed from 83fbfcf79a435d04c1bc7c90b1ad7ff4510797db to 2e780270799ebe985039590431189dd5f3bde730
Branch pushed to git repo; I updated commit sha1. New commits:
2e78027 | typo in atlas spkg-install, manifest itself on OS X
|
comment:80 in reply to: ↑ 78 ; follow-up: ↓ 81 Changed 6 years ago by
Replying to jdemeyer:
Replying to fbissey:
While I find the argument that
LDFLAGS
should be treated just the same as{C,CXX}FLAGS
persuasiveThe treating of
{C,CXX}FLAGS
changed recently in #16149 and I don't understand why that change was needed.Personally, I would prefer to not treat empty and non-empty
FOOFLAGS
fundamentally different (i.e. I would revert #16149 and leaveLDFLAGS
as they are).
Having *FLAGS
empty or unset are two different things.
Maybe it should be the same, but some libraries deal with it differently.
E.g. when it's unset it means do whatever you want with *FLAGS
and if it is empty, it means leave it empty.
Of course, this is not a universal truth and different libraries behaves differently.
So as far as I'm concerned, I would just not touch *FLAGS
at all.
comment:81 in reply to: ↑ 80 Changed 6 years ago by
Replying to jpflori:
Having
*FLAGS
empty or unset are two different things.
I know and understand that, but my point is that it shouldn't be two different things.
comment:82 Changed 6 years ago by
In any case, can we please in this ticket remove the changes to sage-env
and discuss this somewhere else?
comment:83 Changed 6 years ago by
Ticket causes problems on OS X, so not ready yet.
comment:84 Changed 6 years ago by
The sed
command is not doing its job
Mirage:sage-6.8.beta5 fbissey$ cat local/share/blas_config|sed 's/\([^ ]\+\)/-l\1/g' blas
comment:85 Changed 6 years ago by
I believe that \+
in sed
regexps for "match 1 or more times" is not portable. But you can replace X\+
by XX*
for any value of X
.
comment:86 Changed 6 years ago by
- Commit changed from 2e780270799ebe985039590431189dd5f3bde730 to beeb264e9929e335f64975028c95bc03cc4b7d81
Branch pushed to git repo; I updated commit sha1. New commits:
beeb264 | Revert changes to sage-env
|
comment:87 Changed 6 years ago by
- Commit changed from beeb264e9929e335f64975028c95bc03cc4b7d81 to 764c3b574a438ef36b3a8a29552a7691514a37fb
comment:88 Changed 6 years ago by
Thanks for the portability tip Jeroen, that got me through the stuff using autotools. OS X stuff has been compromised in numpy
and very likely scipy
, working on that.
comment:89 Changed 6 years ago by
I just got an idea: why aren't we using a pkgconfig
file instead of local/share/blas_config
? I seems like pkgconfig
is exactly meant to solve the problems we're having on this ticket.
A typical file looks like
$ cat local/lib/pkgconfig/m4ri.pc SAGE_ROOT=/usr/local/src/sage-config prefix=${SAGE_ROOT}/local exec_prefix=${prefix} libdir=${SAGE_ROOT}/local/lib includedir=${prefix}/include Name: M4RI Description: Dense linear algebra over GF(2). Version: 20140914 Libs: -L${libdir} -lm4ri -lm Cflags: -I${includedir}/m4ri -I${SAGE_ROOT}/local/include -g -fPIC -Wall -pedantic -O2 -mmmx -msse -msse2 -msse3
I am sure we can make a blas.pc
file using this format.
From the shell, you can do for example
(sage-sh) $ pkgconf --libs m4ri -lm4ri -lm
which would replace the ugly sed
script.
There is also a Python interface:
sage: import pkgconfig sage: pkgconfig.libs("m4ri") u'-L/usr/local/src/sage-config/local/lib -lm4ri -lm'
comment:90 follow-up: ↓ 91 Changed 6 years ago by
Using pkgconfig
is actually the final destination.
This ticket was just a cleanup ticket before #17075.
comment:91 in reply to: ↑ 90 Changed 6 years ago by
What you're doing here on this ticket with $SAGE_SHARE/blas_config
is very close to pkgconfig
. So why not just use pkgconfig
instead?
comment:92 Changed 6 years ago by
- Dependencies #18777, #18778 deleted
comment:93 Changed 6 years ago by
I guess that at the time this ticket was opened, pkgconfig was not standard though I could not swear it.
comment:94 follow-up: ↓ 95 Changed 6 years ago by
When I started talking about #17075 and associated work to be able to use any blas/lapack
our goal was to use pkgconfig
and https://github.com/matze/pkgconfig
as a python interface to it. This is what sage-on-gentoo
is currently using. The fact that the python interface was also standard escaped me.
We included pkgconfig
in sage when we upgraded to matplotlib-1.3.x
after I determined that it was the only to build it on OS X while reviewing that ticket. That would have been after #17075 but I am not sure if it was before this ticket.
For reference this is the current sage-on-gentoo patch for 6.8.beta6 https://github.com/cschwan/sage-on-gentoo/blob/master/sci-mathematics/sage/files/sage-6.8-blas-r1.patch
The librarydir part is for dealing with BLAS/LAPACK installed in less standard location (/opt
) where they may be out of the library search path when linking. It still relies on said path to be in ld.so.config
or equivalent. Typical cases are intel MKL and AMD acml. The ability to compile against MKL is one of the key motivation here.
Since we have all the elements, going .pc
all the way is fine. I'll rewrite the whole thing.
As mentioned before I spent some time working on OS X and that's also one of my motivation to rewrite some stuff. That's an interesting little story that touch sage-env
(surprise)....
So Jean-Pierre put this in numpy
and scipy
if [ -n "${LDFLAGS+set}" ]; then if [ "$UNAME" != "Darwin" ]; then export LDFLAGS="-bundle -undefined dynamic_lookup $LDFLAGS" export CPPFLAGS="-D__ACCELERATE__ $CPPFLAGS" else export LDFLAGS="-shared $LDFLAGS" fi fi
Which definitely looked wrong on first glance but I trusted Jean-Pierre. Well it is wrong and compilation will fail on OS X because somehow you'll have -bundle
and -shared
in LDFLAGS
at the same time. Unless you include Jean-Pierre changes to sage-env
in which case numpy
at least compile. I'll hazard that the change to sage-env
enable compilation under linux as well somehow.
So there we are the change to sage-env
was allowing wrong stuff in numpy
and scipy
(not on OS X) to work to some extent.
My local branch is in a bit of a mess and I think I pushed too much stuff on trac so I'll get a clean branch in.
comment:95 in reply to: ↑ 94 Changed 6 years ago by
Replying to fbissey:
When I started talking about #17075 and associated work to be able to use any
blas/lapack
our goal was to usepkgconfig
andhttps://github.com/matze/pkgconfig
as a python interface to it. This is whatsage-on-gentoo
is currently using. The fact that the python interface was also standard escaped me.We included
pkgconfig
in sage when we upgraded tomatplotlib-1.3.x
after I determined that it was the only to build it on OS X while reviewing that ticket. That would have been after #17075 but I am not sure if it was before this ticket.For reference this is the current sage-on-gentoo patch for 6.8.beta6 https://github.com/cschwan/sage-on-gentoo/blob/master/sci-mathematics/sage/files/sage-6.8-blas-r1.patch
The librarydir part is for dealing with BLAS/LAPACK installed in less standard location (
/opt
) where they may be out of the library search path when linking. It still relies on said path to be inld.so.config
or equivalent. Typical cases are intel MKL and AMD acml. The ability to compile against MKL is one of the key motivation here.Since we have all the elements, going
.pc
all the way is fine. I'll rewrite the whole thing.As mentioned before I spent some time working on OS X and that's also one of my motivation to rewrite some stuff. That's an interesting little story that touch
sage-env
(surprise)....So Jean-Pierre put this in
numpy
andscipy
if [ -n "${LDFLAGS+set}" ]; then if [ "$UNAME" != "Darwin" ]; then export LDFLAGS="-bundle -undefined dynamic_lookup $LDFLAGS" export CPPFLAGS="-D__ACCELERATE__ $CPPFLAGS" else export LDFLAGS="-shared $LDFLAGS" fi fiWhich definitely looked wrong on first glance but I trusted Jean-Pierre.
I wouldn't it definitely looks wrong on non-OS Xes :)
Read the comment above this part of code.
If LDFLAGS
is set, you need to add -shared
yourself.
This is exactly what is done here.
And yes you are right, this must be the reason for my changes to sage-env
, as if LDFLAGS
is set but empty, then shared
won't be added and the compilation will fail.
The easy solution is to unconditionnally add -shared
to LDFLAGS
on non-OS X.
As far as OS X is concerned, I guess I got this from "the web" as I don't have access to any OS X machine and cried for testing here before.
comment:96 Changed 6 years ago by
In fact the OS X stuff comes from the previous scipy spkg-install
.
If think that my idea to test whether LDFLAGS
is set and only modify it if it is not (differently on OS X and non-OS Xes), is that if LDFLAGS
is set then the user did it and knows what he did and will know how to fix it if compilation fails.
comment:97 Changed 6 years ago by
Just a suggestion, feel free to ignore:
do we really need to change all packages using BLAS at the same time? In order to get something in Sage more quickly, could we for example just change ATLAS and the Sage library in this ticket and postpone the other packages to a new ticket?
comment:98 Changed 6 years ago by
comment:99 Changed 6 years ago by
If possible, let's modularize (though I won't personally have time to do anything serious before the 16th or 20th :( ). I think we could even make this ticket a task and:
- write pkgconfig files for ATLAS in #17075,
- clean up packages using BLAS in dedicated tickets.
Also note that the new LinBox?/FFLAS-FFPACK/Givaro compatible versions should be out very soon (only LinBox? 2.x is missing but there is currently a workshop taking place in Grenoble from what I understood).
comment:100 Changed 6 years ago by
I have, I believe, finished the rewrite of ATLAS' spkg-install
to create .pc
files. I can push it first thing after breakfast to the right ticket.
comment:101 Changed 6 years ago by
- Dependencies set to #17075
comment:102 Changed 5 years ago by
- Dependencies #17075 deleted
Is this ticket still relevant now that we have #17075?
comment:103 Changed 4 years ago by
I think we can close this one.
comment:104 Changed 4 years ago by
- Branch u/fbissey/blas deleted
- Commit 764c3b574a438ef36b3a8a29552a7691514a37fb deleted
- Milestone changed from sage-6.5 to sage-duplicate/invalid/wontfix
- Status changed from needs_work to positive_review
comment:105 Changed 4 years ago by
- Resolution set to wontfix
- Status changed from positive_review to closed
Closing tickets in the sage-duplicate/invalid/wontfix module with positive_review (i.e. someone has confirmed they should be closed).
Info for scipy/numpy: http://www.scipy.org/scipylib/building/linux.html