search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.43k stars 479 forks source link

Update Givaro, FFLAS-FFPACK and LinBox #17635

Closed jpflori closed 8 years ago

jpflori commented 9 years ago

Tarballs of released packages (with appropriate names) can be found at:

CC: @kiwifb @ClementPernet

Component: packages: standard

Keywords: linbox, givaro, fflas-ffpack, sd75

Author: Clément Pernet, Jeroen Demeyer

Branch: 7e8eb3a

Reviewer: François Bissey, Jeroen Demeyer, Dima Pasechnik

Issue created by migration from https://trac.sagemath.org/ticket/17635

jpflori commented 9 years ago
comment:1

Requires to update:

kiwifb commented 9 years ago
comment:2

So we have to wait for linbox? Do we have an idea of when it will happen?

jpflori commented 9 years ago
comment:3

Replying to @kiwifb:

So we have to wait for linbox? Do we have an idea of when it will happen?

Yes we have to wait for a new Linbox, expected about the end of february, Clément Pernet wrote me, together with updated Givaro and fflas-ffpack.

By the way, I started this for #17630 which might interest you as well, I'll push code there tomorrow.

jdemeyer commented 9 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,3 @@
-Tarball at:
+Tarballs at:
 * https://forge.imag.fr/frs/download.php/592/givaro-3.8.0.tar.gz
-
+* http://linalg.org/fflas-ffpack-2.0.0.tar.gz (needs to be renamed because of the dash)
jdemeyer commented 9 years ago
comment:4

Replying to @jpflori:

Replying to @kiwifb:

So we have to wait for linbox? Do we have an idea of when it will happen?

Yes we have to wait for a new Linbox, expected about the end of february, Clément Pernet wrote me, together with updated Givaro and fflas-ffpack.

It's June already...

Since givaro, LinBox and FFLAS-FFPACK all depend on each other, we should make one ticket.

jdemeyer commented 9 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,3 @@
 Tarballs at:
-* https://forge.imag.fr/frs/download.php/592/givaro-3.8.0.tar.gz
-* http://linalg.org/fflas-ffpack-2.0.0.tar.gz (needs to be renamed because of the dash)
+* https://forge.imag.fr/frs/download.php/672/givaro-4.0.0.tar.gz
+* http://linalg.org/fflas-ffpack-2.1.0.tar.gz (needs to be renamed because of the dash)
ClementPernet commented 9 years ago
comment:6

Sorry for the delay in releasing the fflas-ffpack/Givaro/LinBox environment. We're working on it now, and I'll keep you posted about the status. It should happen before the end of the year and hopefully much earlier.

I also meant to propose to replace ATLAS by OpenBLAS in sage. Looking at your clean-up work of the BLAS in #17630, I understand that we should proceed in the following order:

  1. release linbox
  2. upgrade linbox fflas-ffpack Givaro in Sage (this ticket #17635)
  3. clean-up the BLAS usage #17630
  4. replace ATLAS by OpenBLAS (ticket to be created)

I'll be happy to help in these tasks.

jdemeyer commented 9 years ago
comment:7

Let me remind you also about #15535, which pops up regularly. I don't know if it's a bug in Linbox or in the way that Sage uses Linbox, but it would be good to have a Linbox developer look at it.

kiwifb commented 9 years ago
comment:8

Replying to @ClementPernet:

Sorry for the delay in releasing the fflas-ffpack/Givaro/LinBox environment. We're working on it now, and I'll keep you posted about the status. It should happen before the end of the year and hopefully much earlier.

I also meant to propose to replace ATLAS by OpenBLAS in sage. Looking at your clean-up work of the BLAS in #17630, I understand that we should proceed in the following order:

  1. release linbox
  2. upgrade linbox fflas-ffpack Givaro in Sage (this ticket #17635)
  3. clean-up the BLAS usage #17630
  4. replace ATLAS by OpenBLAS (ticket to be created)

I'll be happy to help in these tasks.

I'll admit to have been a bit demotivated on the blas cleaning front. The tickets needs re-organizing and have good descriptions so we know what is supposed to go in them. Getting ATLAS to produce .pc file and using them is probably a priority task. Once that's done someone could create an optional openblas spkg or something.

ClementPernet commented 9 years ago
comment:9

I'm working on it, and will release a new version of the 3 libs for the occasion, so as to ensure a smoother integration upstream. This will very likely be givaro-4.0.1 fflas-ffpack-2.2.0 linbox-1.4.1

These libraries now require c++11 support and this tickets therefore depends on #18323.

Inspired by what's propose in there, I suggest to add lines of the form

+# distutils: extra_compile_args = -std=c++11

to libs/linbox/linbox.pxd lib/linbox/fflas.pxd lib/linbox/echelonform.pxd but I understand that this is subject to a debate on sage-devel https://groups.google.com/forum/#!topic/sage-devel/aGwotQA9to8

If this option happens to be accepted, we would then need to add more cflags in the list: namely those produced by fflas-ffpack configure related to the target architecture: -mavx -msse4.1 and others like -fabi-version=6 -fopenmp.

I could not figure a way to append the string returned by bin/fflas-ffpack-config --cflags to these pxd files. Any hint?

ClementPernet commented 9 years ago

Dependencies: 18323

ClementPernet commented 9 years ago

Changed dependencies from 18323 to #18323

kiwifb commented 9 years ago
comment:11

Replying to @ClementPernet:

I could not figure a way to append the string returned by bin/fflas-ffpack-config --cflags to these pxd files. Any hint?

Add a definition in module_list.py, you define a LINBOX_FLAGS. It would be easier if you had a .pc file as we have a python interface to pkgconfig that we can use easily. Here you have to do your own parsing from python. Anyway you can then use LINBOX_FLAGS in the .pxd file.

jdemeyer commented 9 years ago
comment:12

Replying to @ClementPernet:

-mavx -msse4.1

I understand that the libraries themselves want these, but why the Cython files?

and others like -fabi-version=6

This looks like it should be used always for all C++ files then. Mixing ABI's doesn't look like a good idea.

-fopenmp

Same question as before, is this really needed for the Cython interface?

ClementPernet commented 9 years ago
comment:13

Replying to @jdemeyer:

Replying to @ClementPernet:

-mavx -msse4.1

I understand that the libraries themselves want these, but why the Cython files?

fflas-ffpack and linbox are essentially source libraries, so when you include the headers, you include the whole code. This is by design, due to the ubiquity of template programming in these libraries.

Now the old linbox-sage interface compiles some specializations and offer them through a C api. These extra flags would not be required if we would stick to using this interface.

But, since then, Cython started to offer the capability to hook C++ code directly and more functionnalities have been added in sage where fflas-ffpack code is called directly, like in libs/linbox/fflas.pxd where the C++ routine FFLAS::fgemm is called.

and others like -fabi-version=6

This looks like it should be used always for all C++ files then. Mixing ABI's doesn't look like a good idea.

Sure. Maybe one should change the ABI higher up then.

-fopenmp

Same question as before, is this really needed for the Cython interface?

Same answer as above.

jdemeyer commented 9 years ago

Changed dependencies from #18323 to #19298

ClementPernet commented 9 years ago

Branch: u/cpernet/givaro_fflasffpack_linbox

ClementPernet commented 9 years ago
comment:16

I added my working branch to the ticket. I could not find if and where I should store the tarballs of the new spkg?

As of today (sept 28 2015); they correspond to the upstream version of the 3 libs: givaro, fflas-ffpack and linbox.

I added .pc files for givaro and fflas-ffpack (coming soon for linbox), and defined the extra_compile_flages in modules_list.py as suggested by fbissey.

This is still work in progress:

New commits:

dd81c26Update givaro to v4.0.1 fflas-ffpack to v2.2.0 and linbox to v1.4.1rc0.
ClementPernet commented 9 years ago

Commit: dd81c26

jdemeyer commented 9 years ago
comment:17

Replying to @ClementPernet:

I could not find if and where I should store the tarballs of the new spkg?

Just add a link to the upstream tarballs on this ticket. If any renaming is needed, you should rename the tarball and add a link to the renamed tarball.

ClementPernet commented 9 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,4 @@
 Tarballs at:
-* https://forge.imag.fr/frs/download.php/672/givaro-4.0.0.tar.gz
-* http://linalg.org/fflas-ffpack-2.1.0.tar.gz (needs to be renamed because of the dash)
+* http://lig-membres.imag.fr/pernet/prereleases/givaro-4.0.1rc0.tar.gz
+* http://lig-membres.imag.fr/pernet/prereleases/linbox-1.4.0rc0.tar.gz
+* http://lig-membres.imag.fr/pernet/prereleases/fflas_ffpack-2.2.0rc0.tar.bz2
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from dd81c26 to c97f547

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

c97f547update fflas-givaro-linbox spkg version and minor chnages to configuration files
jdemeyer commented 9 years ago
comment:20

Instead of adding flags to individual extensions, they should be added to the .pxd or .pyx files.

ClementPernet commented 9 years ago
comment:21

Ok, now I get confused: fbissey suggested to define all compilation flags in module_list.py. I saw there that most spkg related extension had their flags set in this file: e.g. flint, fplll, Lfunction, m4ri, m4rie... Hence I moved all flags that I originally wrote in the linbox.pxd fflas-ffpack.pxd givaro.pxd files to module_list.py which has the advantage of defining everything at the same place.

Jeroen or François are you suggesting that the proper way to proceed is to

+# distutils: extra_compile_args = ${givaro_extra_compile_args}

? Will do, if that's the way to go.

Also, I did not find how to invoque the python interface to pkgconfig, mentionned by françois.

kiwifb commented 9 years ago
comment:22

Sorry I have been of the net for most of the week. For how to use pkgconfig you can follow the way I use it for blas in sage-on-gentoo at https://github.com/cschwan/sage-on-gentoo/blob/master/sci-mathematics/sage/files/sage-6.8-blas-r1.patch. Note that you probably don't need to parse the library_dir, I do it for blas in unusual location, which is expected for intel MKL or the AMD equivalent.

jdemeyer commented 9 years ago
comment:23

Replying to @ClementPernet:

I moved all flags that I originally wrote in the linbox.pxd fflas-ffpack.pxd givaro.pxd files to module_list.py

Please don't!

which has the advantage of defining everything at the same place.

...but the disadvantage of requiring to add the flags to every single extension, which is much less scalable.

Jeroen or François are you suggesting that the proper way to proceed is to

  • define the flags givaro_extra_compile_args, fflas_ffpack_extra_compile_args, linbox_extra_compile_args in module_list.py
  • use them in the pxd files with something like 
+# distutils: extra_compile_args = ${givaro_extra_compile_args}

Essentially yes. You can grep for GSL_LIBRARIES to see how it's supposed to be done.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

fb16751Pass the library specific cflags in the pxd files instead of extensions in module_list.py
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from c97f547 to fb16751

jdemeyer commented 9 years ago
comment:25

This is still not quite what I meant.

For example, why does src/sage/rings/polynomial/multi_polynomial_ideal_libsingular.pxd need GIVARO_CFLAGS? Looking at that file, there is nothing related to givaro.

I assume you added GIVARO_CFLAGS because it still somehow indirectly includes Givaro code. But then you should add GIVARO_CFLAGS in the place where the Givaro headers are actually included and Cython's dependency tracking should pick it up.

See again the GSL_LIBRARIES example: I don't add GSL_LIBRARIES to every module which somehow indirectly uses GSL, only where the library functions are actually declared.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

c25de33Remove all unecessary extra_compile_args for GIVARO and FFLASFFPACK.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from fb16751 to c25de33

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

73cc6b3Finds GIVARO and FFLASFFPACK CFLAGS in the .pc pkgconfig file.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from c25de33 to 73cc6b3

ClementPernet commented 9 years ago
comment:28

Replying to @jdemeyer:

I assume you added GIVARO_CFLAGS because it still somehow indirectly includes Givaro code. But then you should add GIVARO_CFLAGS in the place where the Givaro headers are actually included and Cython's dependency tracking should pick it up.

Yes. It was necessary to add these flags to extensions when everything was gathered in module_list.py, but no longer now that it is in each pxd files. I now see the point! Sorry for messing this up, it's fixed. I also made the CFLAGS variables set by the pkgconfig .pc files, as suggested by François.

One pb remains: there is no gmp.pc for the moment, and the givaro.pc lists gmp as a dependency. Similarly fflas-ffpack.pc should list blas as dependency, but we still don't have a blas.pc file (see #17075). I suggest to remove these dependencies for the moment, and I'll add them once we have the proper .pc files. If you confirm, I'll produce new tar.gz for givaro and fflas-ffpack accordingly.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 73cc6b3 to 7fd9523

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

7fd9523Add link flags for fflas-ffpack.
jdemeyer commented 8 years ago
comment:30

Are there any significant API changes in the new linbox? I would like to add a linbox interface for sparse matrices over ZZ. I could do that using the current linbox-1.3.2 but I'd like to know that it will still work after upgrading linbox.

jdemeyer commented 8 years ago

Changed branch from u/cpernet/givaro_fflasffpack_linbox to u/jdemeyer/givaro_fflasffpack_linbox

jdemeyer commented 8 years ago

Changed commit from 7fd9523 to c447352

jdemeyer commented 8 years ago

Changed dependencies from #19298 to none

jdemeyer commented 8 years ago
comment:32

Merged latest beta.

New commits:

c447352Merge tag '7.1.beta3' into t/17635/givaro_fflasffpack_linbox
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from c447352 to f31752c

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

f31752cMerge tag '7.1.beta3' into t/17635/givaro_fflasffpack_linbox
jdemeyer commented 8 years ago
comment:34

fflas_ffpack doesn't build:

libtool: compile:  g++ -DHAVE_CONFIG_H -I. -I../.. -I../.. -I../../fflas-ffpack/utils/ -I../../fflas-ffpack/fflas/ -I../../fflas-ffpack/ffpack -I../../fflas-ffpack/field -I/usr/local/src/sage-git/local/include -I/usr/local/src/sage-git/local//include -D__FFLASFFPACK_HAVE_CBLAS -msse4.1 -mavx -fabi-version=6 -fopenmp -O2 -Wall -DNDEBUG -UFFLASFFPACK_DEBUG -g -O2 -fPIC -std=gnu++11 -c fflas_L1_inst.C  -fPIC -DPIC -o .libs/fflas_L1_inst.o
libtool: compile:  g++ -DHAVE_CONFIG_H -I. -I../.. -I../.. -I../../fflas-ffpack/utils/ -I../../fflas-ffpack/fflas/ -I../../fflas-ffpack/ffpack -I../../fflas-ffpack/field -I/usr/local/src/sage-git/local/include -I/usr/local/src/sage-git/local//include -D__FFLASFFPACK_HAVE_CBLAS -msse4.1 -mavx -fabi-version=6 -fopenmp -O2 -Wall -DNDEBUG -UFFLASFFPACK_DEBUG -g -O2 -fPIC -std=gnu++11 -c fflas_L3_inst.C  -fPIC -DPIC -o .libs/fflas_L3_inst.o
In file included from ../../fflas-ffpack/fflas/fflas_sparse/csr.h:89:0,
                 from ../../fflas-ffpack/fflas/fflas_sparse.h:126,
                 from ../../fflas-ffpack/fflas/fflas.h:164,
                 from fflas_L1_inst.C:34:
../../fflas-ffpack/fflas/fflas_sparse/csr/csr_pspmm.inl:48:2: error: stray '#' in program
   PAR_BLOCK{
  ^
ClementPernet commented 8 years ago

Changed branch from u/jdemeyer/givaro_fflasffpack_linbox to u/cpernet/givaro_fflasffpack_linbox

ClementPernet commented 8 years ago
comment:36

I've updated the tarballs of the 3 libraries to the latests rc1 (the failing code in longer there) and updated the checksums. Everything builds on my box.

New commits:

db82895Patch merged upstream in fflas-ffpack
a45e379missing coma
4cd2289updating givaro, fflas-ffpack and linbox to latest rc1 version
ClementPernet commented 8 years ago

Changed commit from f31752c to 4cd2289

ClementPernet commented 8 years ago

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
 Tarballs at:
-* http://lig-membres.imag.fr/pernet/prereleases/givaro-4.0.1rc0.tar.gz
-* http://lig-membres.imag.fr/pernet/prereleases/linbox-1.4.0rc0.tar.gz
-* http://lig-membres.imag.fr/pernet/prereleases/fflas_ffpack-2.2.0rc0.tar.bz2 
+* http://lig-membres.imag.fr/pernet/prereleases/givaro-4.0.1rc1.tar.gz
+* http://lig-membres.imag.fr/pernet/prereleases/linbox-1.4.0rc1.tar.gz
+* http://lig-membres.imag.fr/pernet/prereleases/fflas_ffpack-2.2.0rc1.tar.bz2
ClementPernet commented 8 years ago

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
 Tarballs at:
 * http://lig-membres.imag.fr/pernet/prereleases/givaro-4.0.1rc1.tar.gz
 * http://lig-membres.imag.fr/pernet/prereleases/linbox-1.4.0rc1.tar.gz
-* http://lig-membres.imag.fr/pernet/prereleases/fflas_ffpack-2.2.0rc1.tar.bz2 
+* http://lig-membres.imag.fr/pernet/prereleases/fflas_ffpack-2.2.0rc1.tar.gz