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

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

Merged

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Apr 23, 2024

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:

  • To install these options, use ./configure --enable-sagemath_bliss before building, or use ./sage -i sagemath_bliss or make sagemath_bliss.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link

github-actions bot commented Apr 23, 2024

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 24, 2024

@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
Copy link
Member

soehms commented Apr 24, 2024

@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
Copy link
Contributor Author

mkoeppe commented Apr 24, 2024

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
Copy link
Contributor Author

mkoeppe commented Apr 24, 2024

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

@soehms
Copy link
Member

soehms commented Apr 26, 2024

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
Copy link
Member

kiwifb commented Apr 30, 2024

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
Copy link
Member

soehms commented May 2, 2024

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
Copy link
Member

kiwifb commented May 2, 2024

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
Copy link
Member

soehms commented May 6, 2024

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
Copy link
Member

soehms commented May 9, 2024

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 #37737 (comment) and commit f881e2b).

@mkoeppe mkoeppe force-pushed the sagemath_standard_remove_optional_build branch from 0e7cd3d to 6a25b74 Compare May 9, 2024 20:28
@mkoeppe mkoeppe force-pushed the sagemath_standard_remove_optional_build branch from 6a25b74 to 2601dc0 Compare May 9, 2024 20:48
@kiwifb
Copy link
Member

kiwifb commented May 9, 2024

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

@mkoeppe mkoeppe force-pushed the sagemath_standard_remove_optional_build branch 2 times, most recently from 0e9e031 to 414d99b Compare May 12, 2024 02:01
@mkoeppe mkoeppe force-pushed the sagemath_standard_remove_optional_build branch from 414d99b to 7ab8171 Compare May 14, 2024 06:15
@mkoeppe mkoeppe requested a review from kiwifb May 16, 2024 01:36
@kiwifb
Copy link
Member

kiwifb commented May 16, 2024

I have started a private test workflow on my computer, I will let you know the results once it is done.

@kiwifb
Copy link
Member

kiwifb commented May 22, 2024

The workflow thing did not go well because I had not taken into some "interference". Will try a different way.

@kiwifb
Copy link
Member

kiwifb commented May 23, 2024

I'll also further note that I have been impacted by cschwan/sage-on-gentoo#783 which is like the kind of stuff we see with setuptools_scm (but worse) and will only stop when either sagemath moves to meson or people bloody stop injecting their stuff in setuptools.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 23, 2024

... or when downstream architects stop trying to circumvent build isolation ....

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 23, 2024

My modest proposal for this whole problem: A site-packages slurper that creates a PEP-660 editable wheel from a package installed in site-packages. Then build with proper build isolation instead of gpep517 and use a PIP_CONSTRAINT that forces the use of these wheels.

@mkoeppe mkoeppe force-pushed the sagemath_standard_remove_optional_build branch from f10abcd to d4864f8 Compare May 25, 2024 18:11
Copy link
Member

@kiwifb kiwifb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could build and test the thing. I think we need to move it forward now.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 29, 2024

Thank you!

vbraun pushed a commit to vbraun/sage that referenced this pull request May 29, 2024
…iss, coxeter3, ....)

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

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:
- To install these options, use `./configure --enable-sagemath_bliss`
before building, or use `./sage -i sagemath_bliss` or `make
sagemath_bliss`.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

- Depends on sagemath#37737 (merged here)
- Depends on sagemath#37973 (merged here)
    
URL: sagemath#37857
Reported by: Matthias Köppe
Reviewer(s): François Bissey
@vbraun vbraun merged commit 2d5477e into sagemath:develop Jun 1, 2024
13 of 15 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Jun 1, 2024
@mkoeppe mkoeppe deleted the sagemath_standard_remove_optional_build branch September 27, 2024 02:09
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.

4 participants