Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates to linbox-1.7.0, givaro-4.2.0, fflas-ffpack-2.5.0 #35148

Closed
wants to merge 5 commits into from

Conversation

tornaria
Copy link
Contributor

@tornaria tornaria commented Feb 17, 2023

📚 Description

Draft PR taken from #32959. I rebased on top of 9.8 and squashed together so there's one commit per update, plus the two commits that adapt sage to changes in linbox api. I left out sagemath/sagetrac-mirror@4006e10 since I understand it's not needed with singular 4.3+.

I'm testing on void, but note I needed to include a few patches for this to work on i686:

I'm making this draft PR to get the ball rolling...

@ClementPernet

Resolves #32959

📝 Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion.

⌛ Dependencies

@mkoeppe
Copy link
Member

mkoeppe commented Feb 17, 2023

I left out sagemath/sagetrac-mirror@4006e10 since I understand it's not needed with singular 4.3+.

In this case, we'll have to tighten the Singular requirements in build/pkgs/singular/spkg-configure.m4

@tornaria
Copy link
Contributor Author

The problem here is that configure decided to use givaro and fflas-ffpack from system, and rebuilding linbox to 1.7.0 failed. I guess this is related to #34359. I don't know what is the way forward.

Also, there seems to be other issues:
a. looking at the patch for sage, it seems the API for linbox has changed: I don't know if it is straightforward to support 1.6.3 at the same time as 1.7.0 (I didn't try).
b. the ABI has also changed but the soname didn't, this is a problem, e.g. I get a lot of crashes when running sage built with the old versions after upgrading linbox et al.

@mkoeppe
Copy link
Member

mkoeppe commented May 3, 2023

What versions of packages do we want to accept from the system? #34359 is relevant

@tornaria
Copy link
Contributor Author

tornaria commented May 3, 2023

What versions of packages do we want to accept from the system? #34359 is relevant

Even debian (testing) has already moved to linbox 1.7.0. Maybe we can just require givaro 4.2.0, fflas 2.5.0, and linbox 1.7.0?

The important bit is that linbox 1.7.0 cannot be built with older givaro & fflas so either you force everything to the new versions or you have to add logic in givaro and fflas to accept old versions only if system linbox is available.

@mkoeppe
Copy link
Member

mkoeppe commented May 3, 2023

Maybe we can just require givaro 4.2.0, fflas 2.5.0, and linbox 1.7.0?

Sounds reasonable.

@tornaria
Copy link
Contributor Author

tornaria commented May 4, 2023

I don't see an easy way to support both 1.6.3 and 1.7.0. Three lines have to be changed:

$ git diff
diff --git a/src/sage/libs/linbox/linbox.pxd b/src/sage/libs/linbox/linbox.pxd
index dcc482960cc..6db69735abc 100644
--- a/src/sage/libs/linbox/linbox.pxd
+++ b/src/sage/libs/linbox/linbox.pxd
@@ -32,7 +32,7 @@ cdef extern from "linbox/matrix/dense-matrix.h":
         ctypedef Modular_double Field
         ctypedef double Element
         DenseMatrix_Modular_double(Field F, size_t m, size_t n)
-        DenseMatrix_Modular_double(Field F, size_t m, size_t n, Element*)
+        DenseMatrix_Modular_double(Field F, Element*, size_t m, size_t n)
         void setEntry(size_t i, size_t j, Element& a)
         Element &getEntry(size_t i, size_t j)
 
@@ -42,7 +42,7 @@ cdef extern from "linbox/matrix/dense-matrix.h":
         ctypedef Modular_float Field
         ctypedef float Element
         DenseMatrix_Modular_float(Field F, size_t m, size_t n)
-        DenseMatrix_Modular_float(Field F, size_t m, size_t n, Element*)
+        DenseMatrix_Modular_float(Field F, Element*, size_t m, size_t n)
         void setEntry(size_t i, size_t j, Element& a)
         Element &getEntry(size_t i, size_t j)
 
diff --git a/src/sage/matrix/matrix_modn_dense_template.pxi b/src/sage/matrix/matrix_modn_dense_template.pxi
index e45e05f3641..abf29badce6 100644
--- a/src/sage/matrix/matrix_modn_dense_template.pxi
+++ b/src/sage/matrix/matrix_modn_dense_template.pxi
@@ -221,7 +221,7 @@ cdef inline linbox_echelonize_efd(celement modulus, celement* entries, Py_ssize_
         return 0,[]
 
     cdef ModField *F = new ModField(<long>modulus)
-    cdef DenseMatrix *A = new DenseMatrix(F[0], <Py_ssize_t>nrows, <Py_ssize_t>ncols, <ModField.Element*>entries)
+    cdef DenseMatrix *A = new DenseMatrix(F[0], <ModField.Element*>entries,<Py_ssize_t>nrows, <Py_ssize_t>ncols)
     cdef Py_ssize_t r = reducedRowEchelonize(A[0])
     cdef Py_ssize_t i,j
     for i in range(nrows):

The - lines support 1.7.0, the - lines support 1.6.3. I don't know how conditional compile one or the other. Better yet woudl be to have cython "change" the calls to the constructor when 1.6.3 is in use.

Of course, the easiest way is to just migrate to 1.7.0 altogether.

@tornaria
Copy link
Contributor Author

tornaria commented May 4, 2023

In #35612 I removed the use of the changed api, that was only in a single place and easy to replace. Now sagelib uses only part of the API that is unchanged between 1.6.3 and 1.7.0.

I rebased this PR on top of #35612. If possible, that one should be merged quickly for 10.0, and this one can be worked out together with #34359.

I would say sagemath should update linbox et al soon after 10.0, and in terms of system support: accept any version of linbox starting with 1.6.3. As for givaro and fflas-ffpack, accept them from system only if either linbox is installed, or if they are at least 4.2.0 and 2.5.0. It should not be allowed to use older givaro and/or fflas-ffpack when linbox will be built by sage.

@tornaria
Copy link
Contributor Author

Rebased to 10.0.rc2 without any change (removes spurious "fix the linter commits")

vbraun pushed a commit that referenced this pull request May 28, 2023
    
### 📚 Description

The constructor `DenseMatrix(field, entries, m, n)` in 1.6.3 was changed
to `DenseMatrix(field, m, n, entries)` in 1.7.0.

We replace use of this constructor by a different way to initialize a
matrix from entries using part of the API that works both in 1.6.3 and
in 1.7.0.

NOTE:  the two versions of the library are ABI-incompatible so changing
linbox requires recompiling sagelib. To complicate matters, linbox
doesn't change soname so the dynamic linker will let this go and
sagemath will eventually crash.

See:  #35148 for the actual update of linbox -- this PR should make that
one easier as well as system support of linbox, since both linbox 1.6.3
and 1.7.0 wil be supported.

### 📝 Checklist

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
    
URL: #35612
Reported by: Gonzalo Tornaría
Reviewer(s): Dima Pasechnik
@tornaria
Copy link
Contributor Author

tornaria commented Nov 7, 2023

I've just rebased this PR to 10.2.rc0. We've been shipping these updates for a while in void linux without any issue that I know of. There are some patches we use that I added here (IIRC they are all from upstream), and this is regularly tested on x86_64 (glibc and musl) and on i686 (glibc only). I think it's been tested on aarch64 as well.

I'm no longer interested in this PR since I'm unable to test. If someone else wants to take over, feel free...

@tornaria
Copy link
Contributor Author

Rebased to 10.2.rc1

@mkoeppe
Copy link
Member

mkoeppe commented Nov 11, 2023

On debian-bullseye-i386-standard (https://github.com/sagemath/sage/actions/runs/6836592787/job/18591708478?pr=35148#step:14:397):

 ../../linbox/algorithms/polynomial-matrix/fft-integral.inl: In member function 'void LinBox::FFT_base<Field, Simd, typename std::enable_if<(Field::is_elt_integral_v && typename Simd::is_same_element<Field>::value)>::type>::DIF_core_laststeps(LinBox::FFT_base<Field, Simd, typename std::enable_if<(Field::is_elt_integral_v && typename Simd::is_same_element<Field>::value)>::type>::Element*, size_t, size_t, const Element*, const Element*, const simd_vect_t&, const simd_vect_t&, LinBox::FFTSimdHelper<4>) const':
  ../../linbox/algorithms/polynomial-matrix/fft-integral.inl:703:49: error: incomplete type 'Simd128<long long unsigned int>' {aka 'Simd128_impl<true, true, false, 8>'} used in nested name specifier
    703 |                         V7 = Simd128<uint64_t>::mulx (V4, Wp);
        |                                                 ^~~~

@tornaria
Copy link
Contributor Author

On debian-bullseye-i386-standard (https://github.com/sagemath/sage/actions/runs/6836592787/job/18591708478?pr=35148#step:14:397):

 ../../linbox/algorithms/polynomial-matrix/fft-integral.inl: In member function 'void LinBox::FFT_base<Field, Simd, typename std::enable_if<(Field::is_elt_integral_v && typename Simd::is_same_element<Field>::value)>::type>::DIF_core_laststeps(LinBox::FFT_base<Field, Simd, typename std::enable_if<(Field::is_elt_integral_v && typename Simd::is_same_element<Field>::value)>::type>::Element*, size_t, size_t, const Element*, const Element*, const simd_vect_t&, const simd_vect_t&, LinBox::FFTSimdHelper<4>) const':
  ../../linbox/algorithms/polynomial-matrix/fft-integral.inl:703:49: error: incomplete type 'Simd128<long long unsigned int>' {aka 'Simd128_impl<true, true, false, 8>'} used in nested name specifier
    703 |                         V7 = Simd128<uint64_t>::mulx (V4, Wp);
        |                                                 ^~~~

No idea, I'm building on i686 without trouble (needs 292.patch and 294.patch which I added here). May be because it's being built without sse2. Do we really need to support pentium 3?

Does it work now?

@tornaria
Copy link
Contributor Author

It seems all tests passed here (I only looked in debian-bullseye-i386-standard, but the failure seems to be something else).

@mkoeppe
Copy link
Member

mkoeppe commented Nov 14, 2023

Same "incomplete type" errors are still present

@tornaria
Copy link
Contributor Author

Same "incomplete type" errors are still present

I give up. How should I know this is a 32 bit problem if ALL builders fail, etc.

There's something very weird going on here. The lines that use Simd128<uint64_t> are all guarded by

#ifdef __FFLASFFPACK_HAVE_SSE4_1_INSTRUCTIONS

so it makes sense that they shouldn't be compiled, but for some reason are.

@tornaria
Copy link
Contributor Author

Also in fact

configure: WARNING: unrecognized options: --disable-maintainer-mode, --disable-openmp, --disable-sse3, --disable-ssse3, --disable-sse41, --disable-sse42, --disable-fma, --disable-fma4, --disable-avx, --disable-avx2

so all these flags (even the two that I removed) are actually unused.

I'm looking back at my templates and I see I'm using --without-archnative in configure flags because otherwise they will use -march=native to build.

Copy link

Documentation preview for this PR (built with commit 66e9d15; changes) is ready! 🎉

@mkoeppe
Copy link
Member

mkoeppe commented Jan 2, 2024

@mkoeppe
Copy link
Member

mkoeppe commented Jan 2, 2024

It looks like more work with upstream is needed.

(that's a very good, on-topic PR number;)

@vbraun
Copy link
Member

vbraun commented May 4, 2024

Also needs this patch linbox-team/givaro#226 (comment) to build givaro on Fedora 40

@vbraun
Copy link
Member

vbraun commented May 5, 2024

And part of the commit referenced in linbox-team/linbox#310 ...

@vbraun
Copy link
Member

vbraun commented May 5, 2024

I've rebased the branch at https://github.com/vbraun/sage/tree/linbox-flas-givaro-4.2.0 and added the two additional upstream patches, now at least the linbox-flas-givaro builds on gcc-14

@vbraun
Copy link
Member

vbraun commented May 5, 2024

I've created a new PR at #37938

vbraun pushed a commit to vbraun/sage that referenced this pull request May 5, 2024
…-2.5.0

### 📚 Description

Draft PR taken from sagemath#35148. I rebased on top of 10.4 and added two more
patches taken from upstream

@ClementPernet

closes sagemath#32959
closes sagemath#35148

URL: sagemath#37938
Reported by: Volker Braun
Reviewer(s):
@tornaria tornaria closed this May 5, 2024
@tornaria tornaria deleted the linbox branch May 5, 2024 23:43
vbraun pushed a commit to vbraun/sage that referenced this pull request May 9, 2024
…-2.5.0

### 📚 Description

Draft PR taken from sagemath#35148. I rebased on top of 10.4 and added two more
patches taken from upstream

@ClementPernet

closes sagemath#32959
closes sagemath#35148

URL: sagemath#37938
Reported by: Volker Braun
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request May 11, 2024
…-2.5.0

### 📚 Description

Draft PR taken from sagemath#35148. I rebased on top of 10.4 and added two more
patches taken from upstream

@ClementPernet

closes sagemath#32959
closes sagemath#35148

URL: sagemath#37938
Reported by: Volker Braun
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request May 11, 2024
…-2.5.0

    
### 📚 Description

Draft PR taken from sagemath#35148. I rebased on top of 10.4 and added two more
patches taken from upstream

@ClementPernet

closes sagemath#32959
closes sagemath#35148
    
URL: sagemath#37938
Reported by: Volker Braun
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request May 12, 2024
…-2.5.0

    
### 📚 Description

Draft PR taken from sagemath#35148. I rebased on top of 10.4 and added two more
patches taken from upstream

@ClementPernet

closes sagemath#32959
closes sagemath#35148
    
URL: sagemath#37938
Reported by: Volker Braun
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request May 12, 2024
…-2.5.0

    
### 📚 Description

Draft PR taken from sagemath#35148. I rebased on top of 10.4 and added two more
patches taken from upstream

@ClementPernet

closes sagemath#32959
closes sagemath#35148
    
URL: sagemath#37938
Reported by: Volker Braun
Reviewer(s):
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to givaro-4.2.0 fflas-ffpack-2.5.0 linbox-1.7.0
3 participants