sagemath / sage

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

Upgrade: Singular 4.2.0, pysingular 0.9.7 #25993

Closed jdemeyer closed 3 years ago

jdemeyer commented 6 years ago

Tarball: see checksums.ini on the branch

Use make SAGE_SPKG="sage-spkg -o" singular-clean sagelib-clean build to automatically download and install.

"Critical" because it enables supporting newer versions of FLINT.

We use the Singular development branch (spielwiese) + PR https://github.com/Singular/Singular/pull/1058 in order to build documentation. The tarball is made from https://github.com/mkoeppe/Singular/tree/Release-4-2-0-p1%2Bsage

Upstream: Fixed upstream, but not in a stable release.

CC: @simon-king-jena @timokau @slel @isuruf @saraedum @dkrenn @sagetrac-araichev @cheuberg @behackl @dimpase @videlec

Component: packages: standard

Keywords: upgrade, Singular, pysingular

Author: Antonio Rojas, Markus Wageringel, Matthias Koeppe

Branch: ec471e0

Reviewer: Matthias Koeppe, Dima Pasechnik

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

dimpase commented 4 years ago
comment:75

Replying to @mwageringel:

I have merged 9.1.rc0 and removed the (backported) patch from #29438.

With this, I get the segmentation fault in plural.pyx from comment:22 again. I am not sure what to do with this. Maybe we could mark it as not tested and open a new ticket for it.

+1

soehms commented 4 years ago
comment:76

Replying to @mwageringel:

I have merged 9.1.rc0 and removed the (backported) patch from #29438.

With this, I get the segmentation fault in plural.pyx from comment:22 again.

I can't reproduce that.

I checked out the ticket on top of 9.1.rc0 and run make according to the ticket description but all tests of src/sage/rings/polynomial/plural.pyx passed.

Did I miss something? I did that under Windows10 (WSL Ubuntu). The make log-messages said that Singular 4.1.3 had been build but if I start Singular (through Sage) I get:

                     SINGULAR                                 /  Development
 A Computer Algebra System for Polynomial Computations       /   version 4.1.2
                                                           0<
 by: W. Decker, G.-M. Greuel, G. Pfister, H. Schoenemann     \   Mar 2020
FB Mathematik der Universitaet, D-67653 Kaiserslautern        \
mwageringel commented 4 years ago
comment:77

Replying to @soehms:

I can't reproduce that.

I checked out the ticket on top of 9.1.rc0 and run make according to the ticket description but all tests of src/sage/rings/polynomial/plural.pyx passed.

Thanks for taking a look. There is some randomness to this bug. With the current branch, it consistently occurs for me, but has been appearing only from time to time while working on this ticket. If I copy the offending doctest to the top of the file to better isolate the problem, it disappears. I am not even sure if it is really caused by this upgrade – the underlying problem might already exist in Sage without being noticed.

The make log-messages said that Singular 4.1.3 had been build but if I start Singular (through Sage) I get:

                     SINGULAR                                 /  Development
 A Computer Algebra System for Polynomial Computations       /   version 4.1.2
                                                           0<
 by: W. Decker, G.-M. Greuel, G. Pfister, H. Schoenemann     \   Mar 2020
FB Mathematik der Universitaet, D-67653 Kaiserslautern        \

Yes, I get that as well. Apparently upstream forgot to change that version number.

soehms commented 4 years ago
comment:78

Replying to @mwageringel:

Replying to @soehms: There is some randomness to this bug.

That's not good looking!

I tried to reproduce it on another machine (running under Linux Mint 19.1 and Sage with Python 2) but without success, again.

If I copy the offending doctest to the top of the file to better isolate the problem, it disappears.

Does it disappear if you let it in on its position but supress some of the previous doctests, as well?

In my experience such segmentation faults are mostly caused by a previous call of some C-function with no prototype and a wrong number of arguments or some usage of a non initialized variable. Anyway, I think this bug will be very difficult to find!

dimpase commented 4 years ago
comment:79

This looks to me as one of these testing framework Heisenbugs we see elsewhere, as well.

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

Changed commit from 5e496ab to 827aa6c

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

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

827aa6c25993: disable problematic doctest in plural.pyx
mwageringel commented 4 years ago
comment:81

Replying to @soehms:

Does it disappear if you let it in on its position but supress some of the previous doctests, as well?

Yes, if I remove the previous block of tests from the previous method, it disappears, too. Even if I move that block of tests from the previous method to the affected method, and thus preserve all tests and their order, everything works fine. Heisenbug indeed. As you cannot replicate it, this problem might not be too serious. Thanks for looking into it, though.

I have disabled the test on the current branch and opened #29528 for it.

soehms commented 4 years ago
comment:82

Replying to @mwageringel:

Replying to @soehms:

Does it disappear if you let it in on its position but supress some of the previous doctests, as well?

Yes, if I remove the previous block of tests from the previous method, it disappears, too. Even if I move that block of tests from the previous method to the affected method, and thus preserve all tests and their order, everything works fine. Heisenbug indeed. As you cannot replicate it, this problem might not be too serious.

Now, I tried again using the doctest option --global-iterations. With this I was able to reproduce all of your observations (repeatable after 9 loops in the WSL case and 11 loops in the Linux Mint case).

On the other hand, on a 9.1.rc0 installation without that Singular upgrade the segmentation fault didn't occur until loop 500.

mwageringel commented 4 years ago
comment:83

This is just a guess, but this might be related to the problem that the NCPolynomial_plural elements are sometimes not properly deallocated because the parent ring is not defined anymore, for some reason. Indeed, this change produces a warning in exactly the doctest in __reduce__ which preceeds the problematic one.

--- a/src/sage/rings/polynomial/plural.pyx
+++ b/src/sage/rings/polynomial/plural.pyx
@@ -1431,10 +1431,12 @@ cdef class NCPolynomial_plural(RingElement):
     def __dealloc__(self):
         # TODO: Warn otherwise!
         # for some mysterious reason, various things may be NULL in some cases
         if self._parent is not None and (<NCPolynomialRing_plural>self._parent)._ring != NULL and self._poly != NULL:
             p_Delete(&self._poly, (<NCPolynomialRing_plural>self._parent)._ring)
+        elif self._poly != NULL:
+            print("warning in NCPolynomial_plural.__dealloc__")

     def __reduce__(self):
         """
         TESTS::
simon-king-jena commented 4 years ago
comment:84

Replying to @mwageringel:

This is just a guess, but this might be related to the problem that the NCPolynomial_plural elements are sometimes not properly deallocated because the parent ring is not defined anymore, for some reason.

There was #13447 dedicated to having proper refcount for libsingular rings. If I recall correctly, the problem is that refcounting in Singular ONLY happens for rings in the interface, i.e. there is no refcount when polynomials are created. However, there is an internal ref counter in Singular that sometimes in very few places in the Singular code is used.

Anyway, perhaps it makes sense to revive that other ticket and finally try and get the refcount problem done.

antonio-rojas commented 4 years ago
comment:85

9.1.rc2 introduces one new test failure:

File "/usr/lib/python3.8/site-packages/sage/rings/polynomial/multi_polynomial_ideal.py", line 3969, in sage.rings.polynomial.multi_polynomial_ideal.NCPolynomialIdeal.groebner_basis
Failed example:
    ideal(J.transformed_basis()).change_ring(P).interreduced_basis()  # testing trac 21884
Expected:
    [a - 60*c^3 + 158/7*c^2 + 8/7*c - 1, b + 30*c^3 - 79/7*c^2 + 3/7*c, c^4 - 10/21*c^3 + 1/84*c^2 + 1/84*c]
Got:
    // ** redefining xR (   keepring basering;)
    [a - 60*c^3 + 158/7*c^2 + 8/7*c - 1, b + 30*c^3 - 79/7*c^2 + 3/7*c, c^4 - 10/21*c^3 + 1/84*c^2 + 1/84*c]
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

4199b0c25993: Merge tag '9.2.beta0' into #25993
5e6a38325993: fix ntl patch
301a55425993: fix doctest with flaky singular pexpect output
6f57ea525993: replace _cycle by permutation action on polynomials
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 827aa6c to 6f57ea5

mwageringel commented 4 years ago

Changed dependencies from #29465 to none

mwageringel commented 4 years ago
comment:87

Replying to @antonio-rojas:

9.1.rc2 introduces one new test failure:

This is some side-effect in the Singular pexpect interface that appears since #29543 and seems to be harmless. I have updated the doctest.

I have also removed the shift/_cycle method, as it can be replaced by the action of permutations on polynomials.

This ticket is now ready for review. It would be nice to get this merged into one of the next betas.

kiwifb commented 4 years ago
comment:88

Should we try for 4.1.3_p2 instead of 4.1.3? Or _p3 if it has been released since the last time I looked.

mwageringel commented 4 years ago
comment:89

Well, this is a moving target. I can try it tomorrow and, if it does not break anything, I guess it would not hurt to include it. Otherwise, it probably does not make this ticket simpler, so could also be done on a new ticket.

kiwifb commented 4 years ago
comment:90

That's a good plan.

antonio-rojas commented 4 years ago
comment:91

Anything more recent than 4.1.3 has a bug that makes the test suite hang in schemes/curves/affine_curve.py

http://www.singular.uni-kl.de:8002/trac/ticket/865

kiwifb commented 4 years ago
comment:92

You say 4.1.2p1 in that ticket. Was that meant to be 4.1.3p1?

antonio-rojas commented 4 years ago
comment:93

Yes, there's no way to modify a ticket if you file it without an account

antonio-rojas commented 4 years ago
comment:94

Besides the hang, with singular>=4.1.3p1 there are two more trivial test failures due to changes in ring notation display. Perhaps these could be fixed here so they pass on distros that ship a newer singular

**********************************************************************
File "/usr/lib/python3.8/site-packages/sage/rings/polynomial/polynomial_singular_interface.py", line 166, in sage.rings.polynomial.polynomial_singular_interface.PolynomialRing_singular_repr._singular_
Failed example:
    singular(R)
Expected:
    polynomial ring, over a ring (with zero-divisors), global ordering
    //   coefficients: ZZ/bigint(15)
    //   number of vars : 2
    //        block   1 : ordering dp
    //                  : names    x y
    //        block   2 : ordering C
Got:
    polynomial ring, over a ring (with zero-divisors), global ordering
    // coefficients: ZZ/(15)
    // number of vars : 2
    //        block   1 : ordering dp
    //                  : names    x y
    //        block   2 : ordering C
**********************************************************************
File "/usr/lib/python3.8/site-packages/sage/rings/polynomial/multi_polynomial_libsingular.pyx", line 1349, in sage.rings.polynomial.multi_polynomial_libsingular.MPolynomialRing_libsingular._singular_init_
Failed example:
    singular(R)
Expected:
    polynomial ring, over a ring (with zero-divisors), global ordering
    //   coefficients: ZZ/bigint(15)
    //   number of vars : 2
    //        block   1 : ordering dp
    //                  : names    x y
    //        block   2 : ordering C
Got:
    polynomial ring, over a ring (with zero-divisors), global ordering
    // coefficients: ZZ/(15)
    // number of vars : 2
    //        block   1 : ordering dp
    //                  : names    x y
    //        block   2 : ordering C
**********************************************************************
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

edd946e25993: make tests compatible with Singular ≥ 4.1.3p1
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 6f57ea5 to edd946e

mwageringel commented 4 years ago
comment:96

Replying to @antonio-rojas:

Besides the hang, with singular>=4.1.3p1 there are two more trivial test failures due to changes in ring notation display. Perhaps these could be fixed here so they pass on distros that ship a newer singular

Thanks for the infos. I have updated these doctests.

mkoeppe commented 4 years ago
comment:97

What's missing on this ticket?

mkoeppe commented 4 years ago
comment:98

There's now ftp://jim.mathematik.uni-kl.de/pub/Math/Singular/SOURCES/4-1-3/singular-4.1.3p2.tar.gz

mwageringel commented 4 years ago
comment:99

Replying to @mkoeppe:

What's missing on this ticket?

It just needs a review. The problem in comment:91 appears to be fixed upstream, but the fix is not in 4.1.3p2.

mkoeppe commented 4 years ago
comment:100

Then I guess we need a patch on top of 4.1.3p2

mkoeppe commented 4 years ago

Upstream: Fixed upstream, but not in a stable release.

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

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

3bc9caa25993: add patch fixing hang in primdecSY and upgrade to 4.1.3p2
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from edd946e to 3bc9caa

mwageringel commented 4 years ago
comment:104

I have added the patch.

For some reason, this now requires running make sagelib-clean (otherwise, Sage does not start), so this might break incremental builds. Is there a way around this?

mwageringel commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,3 @@
-**Tarball 4.1.3**: ftp://jim.mathematik.uni-kl.de/pub/Math/Singular/SOURCES/4-1-3/singular-4.1.3.tar.gz
+**Tarball 4.1.3p2**: ftp://jim.mathematik.uni-kl.de/pub/Math/Singular/SOURCES/4-1-3/singular-4.1.3p2.tar.gz

-Use `make SAGE_SPKG="sage-spkg -o" singular-clean build` to automatically download and install.
+Use `make SAGE_SPKG="sage-spkg -o" singular-clean sagelib-clean build` to automatically download and install.
mkoeppe commented 4 years ago
comment:105

Tests run at https://github.com/mkoeppe/sage/pull/40/checks?check_run_id=839092163

mkoeppe commented 4 years ago

Reviewer: Matthias Koeppe, ...

mkoeppe commented 4 years ago
comment:106

Looking good. The failures on homebrew-standard, conda-forge-ubuntu, debian-bullseye-standard, and debian-sid-standard have nothing to do with this ticket.

Other reviewers should comment on the changes on this ticket.

mkoeppe commented 4 years ago
comment:107

However, the builds for cygwin-standard (https://github.com/mkoeppe/sage/runs/839373484) and cygwin-minimal (https://github.com/mkoeppe/sage/runs/839558776) fail.

Unfortunately the detailed logs are not available because an error happened during the log archiving operation.

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

Changed commit from 3bc9caa to 06e2227

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

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

06e2227Merge tag '9.2.beta3' into t/25993/public/singular4-1-3
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

e0748b6build/pkgs/singular: Remove python dependency, configure --without-python (singular only supports python2)
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 06e2227 to e0748b6

mkoeppe commented 4 years ago

Changed author from Antonio Rojas, Markus Wageringel to Antonio Rojas, Markus Wageringel, Matthias Koeppe

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

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

539c182build/make/install: Do not depend on src/bin/sage-version.sh
761092cMerge branch 't/29987/build_make_install__do_not_depend_on_src_bin_sage_version_sh' into t/30064/fix_tox_docker_builds_broken_by__29884
f2efa6asrc/doc/bootstrap: Create the directory src/doc/en/reference/repl if it does not exist
b7bf43bbuild/bin/write-dockerfile.sh: ADD src/bin for bootstrapping, needed by src/doc/bootstrap after #29884
365ce61Merge branch 'u/mkoeppe/fix_tox_docker_builds_broken_by__29884' of git://trac.sagemath.org/sage into HEAD
1e7becctox.ini [debian-buster, -sid]: IGNORE_MISSING_SYSTEM_PACKAGES=yes because of libpython3.7-dev
fb61a31Merge branch 'u/mkoeppe/tox_ini__debian_bullseye___sid_have_python3_8_instead_of_3_7' of git://trac.sagemath.org/sage into 9.2.beta3+ci-fixes
4e99a00Merge branch '9.2.beta3+ci-fixes' into t/25993/public/singular4-1-3
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from e0748b6 to 4e99a00

mkoeppe commented 4 years ago
comment:112

New tests run at https://github.com/mkoeppe/Sources/actions/runs/159730297

mkoeppe commented 4 years ago
comment:113

Replying to @mkoeppe:

However, the builds for cygwin-standard (https://github.com/mkoeppe/sage/runs/839373484) and cygwin-minimal (https://github.com/mkoeppe/sage/runs/839558776) fail.

Reported at https://github.com/Singular/Singular/issues/1017

mkoeppe commented 4 years ago

Changed upstream from Fixed upstream, but not in a stable release. to Reported upstream. No feedback yet.

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

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

3e991c7build/bin/sage-system-python: Improve check for a suitable python
62dfd9aMerge branch 'u/mkoeppe/build_bin_sage_system_python__improve_check_for_a_suitable_python' of git://trac.sagemath.org/sage into t/25993/public/singular4-1-3