search

sagemath / sage

Main repository of SageMath. Now open for Issues and Pull Requests.
https://www.sagemath.org
Other
1.08k stars 394 forks source link

Replace special handling of optional extensions (bliss, coxeter3, ....) #37857

Open mkoeppe opened 2 weeks ago

mkoeppe commented 2 weeks ago

We relieve the distribution sagemath-standard (in both versions - SAGE_ROOT/pkgs/sagemath-standard and SAGE_ROOT/src) from the duty to build "optional extensions" based on what Sage packages are installed.

The installation is now done uniformly using the modularized distributions sagemath-bliss, sagemath-coxeter3 etc.

We introduce the missing features coxeter3 and sirocco so that the doctester does not have to rely on the sage-the-distro installation records any more.

The wheels of the distributions now build correctly even when not going through building an sdist first, which previously was required to apply MANIFEST-based filtering. This is achieved using the new sage_setup.command.build_py.

User-visible change:

:memo: Checklist

:hourglass: Dependencies

github-actions[bot] commented 2 weeks ago

Documentation preview for this PR (built with commit 414d99bdccda94a285fca401f882be375d26a950; changes) is ready! :tada: This preview will update shortly after each push to this PR.

mkoeppe commented 2 weeks ago

@soehms Here I'm running into a failure of the --hide tests. Would you be able to take a look?

Features detected for doctesting: mcqd,meataxe,networkx,numpy,sage.combinat,sage.geometry.polyhedron,sage.groups,sage.libs.flint,sage.libs.gap,sage.libs.linbox,sage.libs.m4ri,sage.libs.pari,sage.libs.singular,sage.modular,sage.modules,sage.numerical.mip,sage.plot,sage.rings.complex_double,sage.rings.finite_rings,sage.rings.number_field,sage.rings.padics,sage.rings.real_mpfr,sage.symbolic,scipy,sympy
Features that have been hidden: meataxe
soehms commented 2 weeks ago

@soehms Here I'm running into a failure of the --hide tests. Would you be able to take a look?

Features detected for doctesting: mcqd,meataxe,networkx,numpy,sage.combinat,sage.geometry.polyhedron,sage.groups,sage.libs.flint,sage.libs.gap,sage.libs.linbox,sage.libs.m4ri,sage.libs.pari,sage.libs.singular,sage.modular,sage.modules,sage.numerical.mip,sage.plot,sage.rings.complex_double,sage.rings.finite_rings,sage.rings.number_field,sage.rings.padics,sage.rings.real_mpfr,sage.symbolic,scipy,sympy
Features that have been hidden: meataxe

If you mean this one:

sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/doctest/control.py
**********************************************************************
Error: Failed example:: Got: 714

    with open(filename, 'w') as f:
        f.write(test_hide)
        f.close()
Expected:
    729
Got:
    714
**********************************************************************
Error: Failed example:: Got: Running doctests with ID 2024-04-24-03-32-56-91e6ed54.
Running with SAGE_LOCAL='/sage/local' and SAGE_VENV='/sage/local/var/lib/sage/venv-python3.10'

This is because you made changes in test_hide. So this can be fixed by 729 -> 714. However, this is obviously not the test that showed:

Features that have been hidden: meataxe

Where can I find it, or how to reproduce that?

mkoeppe commented 2 weeks ago

Thanks for the fast reaction, and apologies for not providing enough context; it was late in the evening for me.

What I find puzzling is that meataxe is showing both in "Features detected for doctesting" and "Features that have been hidden".

In these tests (which you can see in the CI logs, or in the annotations shon in the "Files changed" tab here, the packages meataxe and sagemath_meataxe are installed.

You can reproduce it locally with this branch by doing make sagemath_meataxe.

mkoeppe commented 2 weeks ago

Meanwhile I'll fix the trivial failure 729 -> 714.

soehms commented 2 weeks ago

Thanks for the fast reaction, and apologies for not providing enough context; it was late in the evening for me.

No problem! BTW, thank you for your hard work on Sage. It really seems that you work day and night and never get tired!

What I find puzzling is that meataxe is showing both in "Features detected for doctesting" and "Features that have been hidden".

In these tests (which you can see in the CI logs, or in the annotations shon in the "Files changed" tab here, the packages meataxe and sagemath_meataxe are installed.

You can reproduce it locally with this branch by doing make sagemath_meataxe.

Thanks! The issue seems to be caused by this change in control.py. If I disable it the issue disappears. The failure is thrown because the last line from test_hide

{prompt}: get_matrix_class(GF(25,'x'), 4, 4, False, 'meataxe')  # optional - meataxe

It is tested, even though meataxe should not be in options.optional. This gives [5 tests, ...] instead of expected [4 tests, ...]. The behavior is not reproducible when running the according tests in the command-line:

cat test_hide.py

r"""
sage: next(graphs.fullerenes(20))
Traceback (most recent call last):
 ...
FeatureNotPresentError: buckygen is not available.
...
sage: next(graphs.fullerenes(20))   # optional - buckygen
Graph on 20 vertices

sage: len(list(graphs.fusenes(2)))
Traceback (most recent call last):
 ...
FeatureNotPresentError: benzene is not available.
...
sage: len(list(graphs.fusenes(2)))  # optional - benzene
1
sage: from sage.matrix.matrix_space import get_matrix_class
sage: get_matrix_class(GF(25,'x'), 4, 4, False, 'meataxe')
Failed lazy import:
meataxe is not available.
...
sage: get_matrix_class(GF(25,'x'), 4, 4, False, 'meataxe')  # optional - meataxe
<class 'sage.matrix.matrix_gfpn_dense.Matrix_gfpn_dense'>
"""
sage$ sage -tp --hide=buckygen,all test_hide.py
Running doctests with ID 2024-04-25-23-45-21-7b41d91a.
Git branch: sagemath_standard_remove_optional_build
Git ref: 10.4.beta3-11-g0e7cd3d70e-dirty
Running with SAGE_LOCAL='/home/sebastian/devel/sage/local' and SAGE_VENV='/home/sebastian/devel/sage/local/var/lib/sage/venv-python3.10'
Using --optional=debian,pip,sage,sage_spkg
Features to be detected: 4ti2,benzene,bliss,buckygen,conway_polynomials,coxeter3,csdp,cvxopt,cvxopt,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_cubic_hecke,database_ellcurves,database_graphs,database_jones_numfield,database_knotinfo,dvipng,ecm,fpylll,fricas,gap_package_atlasrep,gap_package_design,gap_package_grape,gap_package_guava,gap_package_hap,gap_package_polycyclic,gap_package_qpa,gap_package_quagroup,gfan,graphviz,imagemagick,ipython,jmol,jupymake,kenzo,latte_int,lrcalc_python,lrslib,matroid_database,mcqd,meataxe,mpmath,msolve,nauty,networkx,numpy,palp,pandoc,pdf2svg,pdftocairo,pexpect,phitigra,pillow,plantri,polytopes_db,polytopes_db_4d,pplpy,primecountpy,ptyprocess,pynormaliz,pyparsing,python_igraph,requests,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.groups,sage.libs.braiding,sage.libs.ecl,sage.libs.flint,sage.libs.gap,sage.libs.linbox,sage.libs.m4ri,sage.libs.ntl,sage.libs.pari,sage.libs.singular,sage.misc.cython,sage.modular,sage.modules,sage.numerical.mip,sage.plot,sage.rings.complex_double,sage.rings.finite_rings,sage.rings.function_field,sage.rings.number_field,sage.rings.padics,sage.rings.polynomial.pbori,sage.rings.real_double,sage.rings.real_mpfr,sage.sat,sage.schemes,sage.symbolic,sage_numerical_backends_coin,sagemath_doc_html,scipy,singular,sirocco,sphinx,symengine_py,sympy,tdlib,threejs
Doctesting 1 file using 4 threads.
sage -t --warn-long 68.3 --random-seed=71802437639660612944812824411654478714 test_hide.py
    [4 tests, 0.13 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 0.2 seconds
    cpu time: 0.1 seconds
    cumulative wall time: 0.1 seconds
Features detected for doctesting:
Features that have been hidden: meataxe,buckygen
pytest is not installed in the venv, skip checking tests that rely on it

sage$ sage -tp --hide=benzene,optional test_hide.py
Running doctests with ID 2024-04-25-23-45-50-3b887d69.
Git branch: sagemath_standard_remove_optional_build
Git ref: 10.4.beta3-11-g0e7cd3d70e-dirty
Running with SAGE_LOCAL='/home/sebastian/devel/sage/local' and SAGE_VENV='/home/sebastian/devel/sage/local/var/lib/sage/venv-python3.10'
Using --optional=debian,pip,sage,sage_spkg
Features to be detected: 4ti2,benzene,bliss,buckygen,conway_polynomials,coxeter3,csdp,cvxopt,cvxopt,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_cubic_hecke,database_ellcurves,database_graphs,database_jones_numfield,database_knotinfo,dvipng,ecm,fpylll,fricas,gap_package_atlasrep,gap_package_design,gap_package_grape,gap_package_guava,gap_package_hap,gap_package_polycyclic,gap_package_qpa,gap_package_quagroup,gfan,graphviz,imagemagick,ipython,jmol,jupymake,kenzo,latte_int,lrcalc_python,lrslib,matroid_database,mcqd,meataxe,mpmath,msolve,nauty,networkx,numpy,palp,pandoc,pdf2svg,pdftocairo,pexpect,phitigra,pillow,plantri,polytopes_db,polytopes_db_4d,pplpy,primecountpy,ptyprocess,pynormaliz,pyparsing,python_igraph,requests,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.groups,sage.libs.braiding,sage.libs.ecl,sage.libs.flint,sage.libs.gap,sage.libs.linbox,sage.libs.m4ri,sage.libs.ntl,sage.libs.pari,sage.libs.singular,sage.misc.cython,sage.modular,sage.modules,sage.numerical.mip,sage.plot,sage.rings.complex_double,sage.rings.finite_rings,sage.rings.function_field,sage.rings.number_field,sage.rings.padics,sage.rings.polynomial.pbori,sage.rings.real_double,sage.rings.real_mpfr,sage.sat,sage.schemes,sage.symbolic,sage_numerical_backends_coin,sagemath_doc_html,scipy,singular,sirocco,sphinx,symengine_py,sympy,tdlib,threejs
Doctesting 1 file using 4 threads.
sage -t --warn-long 68.3 --random-seed=129807666119759088366436594506632345285 test_hide.py
    [4 tests, 0.14 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 0.2 seconds
    cpu time: 0.1 seconds
    cumulative wall time: 0.1 seconds
Features detected for doctesting:
Features that have been hidden: meataxe,buckygen
pytest is not installed in the venv, skip checking tests that rely on it

At the moment I've no idea, what is causing that! I can continue on that, but not before next Thursday.

kiwifb commented 1 week ago

Thanks for pointing me here from #37905 I searched issues but not PR. It is rather odd and I cannot see at which point the hiding is done (we have some hidden hiding?).

soehms commented 1 week ago

Thanks for pointing me here from #37905 I searched issues but not PR. It is rather odd and I cannot see at which point the hiding is done (we have some hidden hiding?).

On which branch did you observe the failure? I can only reproduce it on the branch of this PR. As mentioned in my previous comment, it disappears if the following lines from it (in control.py) are omitted:

                        if pkg.name in ['bliss', 'coxeter3', 'mcqd', 'meataxe', 'sirocco', 'tdlib']:
                            continue

So what could you have had in your test that replaced what those two lines do?

kiwifb commented 1 week ago

As I mentioned in #37905 it has a sage-on-gentoo self inflicted component. I only ship a very limited version of sage/misc/package.py and apply this patch https://github.com/cschwan/sage-on-gentoo/blob/master/sci-mathematics/sagemath-standard/files/sagemath-standard-10.4-neutering.patch which removes the part of control.py that depends on list_packages - and that's exactly where your own patch to control.py is located.

soehms commented 1 week ago

As I mentioned in #37905 it has a sage-on-gentoo self inflicted component. I only ship a very limited version of sage/misc/package.py and apply this patch https://github.com/cschwan/sage-on-gentoo/blob/master/sci-mathematics/sagemath-standard/files/sagemath-standard-10.4-neutering.patch which removes the part of control.py that depends on list_packages - and that's exactly where your own patch to control.py is located.

Thanks for the explanation. I was now able to locate the cause of the error.

This is because AvailableSoftware.__contains__ caches its result via the _seen array. Here's what's happening: Due to the changes in this PR or the changes in the Gentoo patch, meataxe was not in the options.optional set. On the other hand there is a line in control.py with the tag # optional - meataxe. Therefore, in SageDocTestParser.parse, the test 'meataxe' in available_software is called before the feature is set to hidden. This results in _seen being set to 1 for Meataxe. When later parsing the tests in the test_hide file, 'meataxe' in available_software shows True again, even though the feature has now been hidden.

We currently have PR #37737 for an improvement to the implementation of the hide option. I'll see how to integrate a solution to the problem into that PR. For now, I would suggest adding the following two lines to AvailableSoftware.__contains__ to fix the bug immediately:

diff --git a/src/sage/doctest/external.py b/src/sage/doctest/external.py
index 5dc1ed7b82..c396fbab38 100644
--- a/src/sage/doctest/external.py
+++ b/src/sage/doctest/external.py
@@ -445,6 +445,8 @@ class AvailableSoftware():
             idx = self._indices[item]
         except KeyError:
             return False
+        if self._features[idx]._hidden:
+            return False
         if not self._seen[idx]:
             if not self._allow_external and self._features[idx] in self._external_features:
                 self._seen[idx] = -1 # not available
soehms commented 4 days ago

We currently have PR #37737 for an improvement to the implementation of the hide option. I'll see how to integrate a solution to the problem into that PR.

I've implemented a solution to the issue there (see https://github.com/sagemath/sage/pull/37737#issuecomment-2102989850 and commit https://github.com/sagemath/sage/pull/37737/commits/f881e2b3fec578fbeeb29dfaf2e8badef7bd8900).

kiwifb commented 4 days ago

Will test as soon as possible, my Friday is already marked has gone so I am not sure when it will be.