Opened 5 years ago

Closed 2 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 jpflori)

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 5 years ago by jpflori

  • Description modified (diff)

comment:2 Changed 5 years ago by jpflori

  • Description modified (diff)

comment:4 Changed 5 years ago by jpflori

comment:5 Changed 5 years ago by fbissey

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 5 years ago by jpflori

  • Authors set to Jean-Pierre Flori
  • 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:

79c712cSimplify BLAS/LAPACK config for IML.
030b90bSimplify BLAS/LAPACK config for numpy.
2a14692Simplify BLAS/LAPACK config for Sage library.
355fff8Let ATLAS install script write cblas_config.
14675a5Remove SAGE_CBLAS documentation.
cf79fc3First try to simplify BLAS/LAPACK configuration for cvxopt.
534ec94Fix cblas_config mechanism.
09e8128Further modifications to numpy/scipy build systems.
37abe37Slight cleanup of cvxopt directory.
b02e159Fixes for cleaner BLAS config.

comment:7 follow-up: Changed 5 years ago by jpflori

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 5 years ago by fbissey

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 5 years ago by jpflori

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 5 years ago by fbissey

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 5 years ago by jpflori

Any chance to get this merged and go on working in a follow up ticket?

comment:12 Changed 5 years ago by fbissey

Missed that. Probably to late for 6.6 but we should be able to rebase for early inclusion in 6.7.

comment:13 Changed 5 years ago by git

  • Commit changed from b02e1593daa51b92401eed7746efcdc665b31388 to 091e941e88d7509f6fffe44785136ae27287aab2

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

091e941Merge remote-tracking branch 'trac/develop' into ticket/17630

comment:14 Changed 5 years ago by fbissey

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 5 years ago by jpflori

Ok, so let's not use it.

comment:16 Changed 5 years ago by jpflori

If you want to add the "libdir" stuff, I'll happily review it.

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

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 5 years ago by jpflori

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 5 years ago by fbissey

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 5 years ago by jpflori

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: Changed 5 years ago by 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.

comment:22 in reply to: ↑ 21 Changed 5 years ago by jpflori

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 5 years ago by git

  • Commit changed from 091e941e88d7509f6fffe44785136ae27287aab2 to 882d72030492da6519301fc8fcadd36093251af4

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

882d720Wrong conflict resolution for numpy.

comment:24 Changed 5 years ago by git

  • Commit changed from 882d72030492da6519301fc8fcadd36093251af4 to 73a21a99499891e5b21552888da9cfa611e53061

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

73a21a9Second try for numpy fixing.

comment:25 Changed 5 years ago by git

  • Commit changed from 73a21a99499891e5b21552888da9cfa611e53061 to a9d2dcf1344c005d6796ec288181451447124718

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

a9d2dcfAnd final fixes.

comment:26 Changed 5 years ago by fbissey

Do you have further things to commit or should I start putting my own comments into action?

comment:27 Changed 5 years ago by git

  • Commit changed from a9d2dcf1344c005d6796ec288181451447124718 to 045266e53354e7f3badb2e2b7dcd4f2c0cea2fa2

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

045266eFinal fix for a correct conflict resolution.

comment:28 Changed 5 years ago by jpflori

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 5 years ago by jdemeyer

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 5 years ago by fbissey

  • 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 5 years ago by jdemeyer

When you think you're done with this ticket, you should really ask Volker to test it on the buildbots.

comment:32 Changed 5 years ago by fbissey

Oh, yes indeed.

comment:33 Changed 5 years ago by jdemeyer

FYI: this will conflict with #18450.

comment:34 Changed 5 years ago by fbissey

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 4 years ago by jpflori

François, any progress on that front?

comment:36 Changed 4 years ago by fbissey

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: Changed 4 years ago by 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.

comment:38 in reply to: ↑ 37 ; follow-up: Changed 4 years ago by fbissey

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 4 years ago by jdemeyer

Replying to fbissey:

Just so I am sure I understand correctly:

# distutils: libraries = gmp pari

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

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:40 Changed 4 years ago by jdemeyer

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 = [ 
    10241024              sources = ['sage/matrix/echelon_matrix.pyx']),
    10251025
    10261026    Extension('sage.matrix.change_ring',
    1027               sources = ['sage/matrix/change_ring.pyx'],
    1028               libraries=[BLAS, BLAS2]),
     1027              sources = ['sage/matrix/change_ring.pyx']),
    10291028
    10301029    Extension('sage.matrix.matrix',
    10311030              sources = ['sage/matrix/matrix.pyx']),
    ext_modules = [ 
    10401039              sources = ['sage/matrix/matrix2.pyx']),
    10411040
    10421041    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']),
    10451043
    10461044    Extension('sage.matrix.matrix_cyclo_dense',
    10471045              sources = ['sage/matrix/matrix_cyclo_dense.pyx'],
    ext_modules = [ 
    10521050              sources = ['sage/matrix/matrix_dense.pyx']),
    10531051
    10541052    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']),
    10571054
    10581055    Extension('sage.matrix.matrix_generic_dense',
    10591056              sources = ['sage/matrix/matrix_generic_dense.pyx']),
    ext_modules = [ 
    11161113              sources = ['sage/matrix/matrix_rational_sparse.pyx']),
    11171114
    11181115    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']),
    11211117
    11221118    Extension('sage.matrix.matrix_sparse',
    11231119              sources = ['sage/matrix/matrix_sparse.pyx']),
    ext_modules = [ 
    12541250              sources = ['sage/modules/module.pyx']),
    12551251
    12561252    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']),
    12591254
    12601255    Extension('sage.modules.vector_double_dense',
    1261               ['sage/modules/vector_double_dense.pyx'],
    1262               libraries = [BLAS, BLAS2]),
     1256              ['sage/modules/vector_double_dense.pyx']),
    12631257
    12641258    Extension('sage.modules.vector_integer_dense',
    12651259              sources = ['sage/modules/vector_integer_dense.pyx']),
    ext_modules = [ 
    12781272              sources = ['sage/modules/vector_rational_dense.pyx']),
    12791273
    12801274    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']),
    12831276
    12841277    ################################
    12851278    ##

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:41 Changed 4 years ago by jdemeyer

  • Dependencies set to #18777

See #18777.

comment:42 Changed 4 years ago by jdemeyer

  • Dependencies changed from #18777 to #18777, #18778

comment:43 Changed 4 years ago by jdemeyer

I did a serious clean-up of the Cython interface to GSL, see #18778.

comment:44 Changed 4 years ago by fbissey

  • 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:

217c051Merge branch 'develop' into blas

comment:45 Changed 4 years ago by git

  • Commit changed from 217c051edd4d06957cf82893f5c4d97e9ac1c654 to d448831b044c43b1190ece92ac6c4b60eb332d7c

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

d448831Forgot a very important "+" in module_list.py

comment:46 Changed 4 years ago by jdemeyer

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 4 years ago by fbissey

That's good suggestions. I will do that once I finished my current round of nitpicking.

comment:48 Changed 4 years ago by jdemeyer

One more nitpicking: replace

cat x | sed ...

by

sed ... x

comment:49 follow-ups: Changed 4 years ago by 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

comment:50 in reply to: ↑ 49 Changed 4 years ago by fbissey

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: Changed 4 years ago by 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)

comment:52 Changed 4 years ago by jdemeyer

Also please use $IML_CONFIGURE instead of "$IML_CONFIGURE" (such variables are usually not quoted).

comment:53 follow-up: Changed 4 years ago by jdemeyer

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: Changed 4 years ago by jpflori

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: Changed 4 years ago by 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.

comment:56 follow-up: Changed 4 years ago by jdemeyer

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 4 years ago by jdemeyer

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: Changed 4 years ago by jpflori

Replying to jdemeyer:

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.

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 4 years ago by jpflori

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 4 years ago by jdemeyer

Replying to jpflori:

Replying to jdemeyer:

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.

This patch is a dirty hack to configure cvxopt to use BLAS related stuff.

There is also GSL-related stuff in that patch.

comment:61 Changed 4 years ago by fbissey

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: Changed 4 years ago by jdemeyer

Now that you explain the cvxopt thing, it's clearer.

comment:63 Changed 4 years ago by git

  • Commit changed from d448831b044c43b1190ece92ac6c4b60eb332d7c to 5c9e7a50cbc9b97db2c560e18d08294b33b768e7

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

5c9e7a5Proper insertion in library_order

comment:64 Changed 4 years ago by fbissey

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 4 years ago by jdemeyer

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.

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:66 in reply to: ↑ 62 Changed 4 years ago by fbissey

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 4 years ago by fbissey

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.

Last edited 4 years ago by fbissey (previous) (diff)

comment:68 Changed 4 years ago by git

  • Commit changed from 5c9e7a50cbc9b97db2c560e18d08294b33b768e7 to eda2eb638d7c10f36053aa43474bb2fd21cd4db2

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

cc5fefcIntroduce 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.
57cbf6fClean 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
eda2eb6Centralize 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: Changed 4 years ago by fbissey

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: Changed 4 years ago by 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.

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: Changed 4 years ago by 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 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.

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 course get_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: Changed 4 years ago by jpflori

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

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 course get_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 4 years ago by git

  • Commit changed from eda2eb638d7c10f36053aa43474bb2fd21cd4db2 to 83fbfcf79a435d04c1bc7c90b1ad7ff4510797db

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

83fbfcfFix indent in ATLAS spkg-install, add a doctest for get_libs_config.

comment:74 in reply to: ↑ 72 Changed 4 years ago by fbissey

  • Authors changed from Jean-Pierre Flori to Jean-Pierre Flori, François Bissey
  • Status changed from needs_work to needs_review

Replying to jpflori:

I also spotted a few indentation issues in atlas/spkg-install and src/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: Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

49 hasn't been addressed.

comment:76 in reply to: ↑ 75 ; follow-up: Changed 4 years ago by fbissey

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 4 years ago by jpflori

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: Changed 4 years ago by jdemeyer

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 4 years ago by git

  • Commit changed from 83fbfcf79a435d04c1bc7c90b1ad7ff4510797db to 2e780270799ebe985039590431189dd5f3bde730

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

2e78027typo in atlas spkg-install, manifest itself on OS X

comment:80 in reply to: ↑ 78 ; follow-up: Changed 4 years ago by jpflori

Replying to jdemeyer:

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

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 4 years ago by jdemeyer

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 4 years ago by jdemeyer

In any case, can we please in this ticket remove the changes to sage-env and discuss this somewhere else?

comment:83 Changed 4 years ago by fbissey

Ticket causes problems on OS X, so not ready yet.

comment:84 Changed 4 years ago by fbissey

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 4 years ago by jdemeyer

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 4 years ago by git

  • Commit changed from 2e780270799ebe985039590431189dd5f3bde730 to beeb264e9929e335f64975028c95bc03cc4b7d81

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

beeb264Revert changes to sage-env

comment:87 Changed 4 years ago by git

  • Commit changed from beeb264e9929e335f64975028c95bc03cc4b7d81 to 764c3b574a438ef36b3a8a29552a7691514a37fb

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

b2495ddImprove portability of sed script so that it works on OS X.
764c3b5Forgot to close the conditional when reversing LDFLAGS changes in sage-env

comment:88 Changed 4 years ago by fbissey

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 4 years ago by jdemeyer

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: Changed 4 years ago by jpflori

Using pkgconfig is actually the final destination. This ticket was just a cleanup ticket before #17075.

comment:91 in reply to: ↑ 90 Changed 4 years ago by jdemeyer

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 4 years ago by jdemeyer

  • Dependencies #18777, #18778 deleted

comment:93 Changed 4 years ago by jpflori

I guess that at the time this ticket was opened, pkgconfig was not standard though I could not swear it.

comment:94 follow-up: Changed 4 years ago by fbissey

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.

Last edited 4 years ago by fbissey (previous) (diff)

comment:95 in reply to: ↑ 94 Changed 4 years ago by jpflori

Replying to fbissey:

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.

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 4 years ago by jpflori

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 4 years ago by jdemeyer

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 4 years ago by fbissey

I approve the motion and that will enable me to move a bit faster and not to deal with the R bump. I would very much like to deal with #17642 next. That and making cython() use distutils as talked about in #18825

comment:99 Changed 4 years ago by jpflori

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 4 years ago by fbissey

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 4 years ago by jdemeyer

  • Dependencies set to #17075

comment:102 Changed 4 years ago by jdemeyer

  • Dependencies #17075 deleted

Is this ticket still relevant now that we have #17075?

comment:103 Changed 3 years ago by jpflori

I think we can close this one.

comment:104 Changed 3 years ago by jpflori

  • 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 2 years ago by embray

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

Note: See TracTickets for help on using tickets.