sage icon indicating copy to clipboard operation
sage copied to clipboard

Demote brial (= polybori) from standard to optional, add distribution sagemath-brial, enlarge sagemath-objects, sagemath-categories

Open mkoeppe opened this issue 2 years ago • 9 comments
trafficstars

as proposed in https://groups.google.com/g/sage-devel/c/ewHP20bmskQ, because (among other things): It has been dropped from Debian testing (debian-trixie) and ubuntu-noble, where it seems to block SageMath upgrades (Sage is stuck at 9.5 in Debian/Ubuntu). See https://repology.org/project/brial/versions

  • Doctests are already conditional on the dynamically detected feature sage.rings.polynomial.pbori
  • We create a distribution sagemath-brial that packages the corresponding extension modules (cherry-picked from #35095).
  • Documentation is conditionalized by using Sphinx tags

Also includes style changes to all*.py files cherry-picked from #37900.

:memo: Checklist

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

:hourglass: Dependencies

  • Depends on #36495 (split out from here)
  • ~~Depends on #36566 (split out from here)~~
  • Depends on #37857 (merged here)

mkoeppe avatar Oct 01 '23 20:10 mkoeppe

docs don't build. I think one can't build boolean polynomials' docs without brial installed, so this has to be moved out:

--- a/src/doc/en/reference/polynomial_rings/index.rst
+++ b/src/doc/en/reference/polynomial_rings/index.rst
@@ -62,12 +62,4 @@ Infinite Polynomial Rings
    sage/rings/polynomial/symmetric_ideal
    sage/rings/polynomial/symmetric_reduction
 
-Boolean Polynomials
--------------------
-
-.. toctree::
-   :maxdepth: 1
-
-   sage/rings/polynomial/pbori/pbori
-
 .. include:: ../footer.txt

dimpase avatar Oct 02 '23 21:10 dimpase

Right. We will need to find a solution for this more general problem of building the manual with optional parts...

mkoeppe avatar Oct 02 '23 21:10 mkoeppe

also:

src/sage/features/sagemath.py:867: UserWarning: Feature sage.rings.polynomial.pbori is declared standard, but it is provided by sagemath_brial, which is declared experimental in SAGE_ROOT/build/pkgs
  JoinFeature.__init__(self, 'sage.rings.polynomial.pbori',

dimpase avatar Oct 02 '23 22:10 dimpase

there are several unconditional imports of BooleanPolynomials left, e.g. in src/sage/rings/polynomial/multi_polynomial_libsingular.pyx

Should it go via an abstract class, etc, to achieve modularity ?

dimpase avatar Oct 03 '23 09:10 dimpase

there are several unconditional imports of BooleanPolynomials left, e.g. in src/sage/rings/polynomial/multi_polynomial_libsingular.pyx

Should it go via an abstract class, etc, to achieve modularity ?

I think I have already done all necessary changes of this kind in #35095. I'll cherry-pick them to this branch later today.

mkoeppe avatar Oct 03 '23 16:10 mkoeppe

Actually I created the ABC BooleanPolynomialRing_base for the parent, but not for the element class. So if you have chance, a modularizing fix here would be welcome

Edit: Done already in f5c6196

mkoeppe avatar Oct 03 '23 18:10 mkoeppe

I get

$ ./sage -sh -c '(cd pkgs/sagemath-brial && tox -v -v -v -e sagepython)'
...
      
            cypari2/closure.pyx:139:4: 'sig_on' is not a constant, variable or function identifier
      
            Error compiling Cython file:
            ------------------------------------------------------------
            ...
      
            cdef int _pari_init_closure() except -1:
                sig_on()
                global ep_call_python
                ep_call_python = install(<void*>call_python, "call_python", 'DGDGDGDGDGD5,U,U')
                sig_off()
                ^
            ------------------------------------------------------------
      
            cypari2/closure.pyx:142:4: 'sig_off' is not a constant, variable or function identifier
      
            Error compiling Cython file:
            ------------------------------------------------------------
            ...
                # Only 5 arguments are supported for now...
                if nargs > 5:
                    nargs = 5
      
                # Fill in default arguments of PARI function
                sig_on()
                ^
            ------------------------------------------------------------
      
            cypari2/closure.pyx:230:4: 'sig_on' is not a constant, variable or function identifier
            Traceback (most recent call last):
...
              File "/private/var/folders/td/fw1q9ljs311ggyph77rs53_40000gn/T/pip-install-h6yasewl/cypari2_0d737fa76b3f479ea5e1b6ab73dffe77/.eggs/Cython-3.0.4-py3.11.egg/Cython/Build/Dependencies.py", line 1321, in cythonize_one
                raise CompileError(None, pyx_file)
            Cython.Compiler.Errors.CompileError: cypari2/closure.pyx
            [end of output]
      
        note: This error originates from a subprocess, and is likely not a problem with pip.
      error: legacy-install-failure
      
      × Encountered error while trying to install package.
      ╰─> cypari2
      
      note: This is an issue with the package mentioned above, not pip.
      hint: See above for output from the failure.
      [end of output]
  
  note: This error originates from a subprocess, and is likely not a problem with pip.
error: subprocess-exited-with-error

× pip subprocess to install build dependencies did not run successfully.
│ exit code: 1
╰─> See above for output.

note: This error originates from a subprocess, and is likely not a problem with pip.
sagepython: 67309 C exit 1 (66.94 seconds) /Users/kwankyu/GitHub/sage-dev/pkgs/sagemath-brial> python -I -m pip install -r requirements.txt pid=54141 [tox/execute/api.py:275]
  sagepython: FAIL code 1 (66.95 seconds)
  evaluation failed :( (67.15 seconds)

kwankyu avatar Oct 18 '23 04:10 kwankyu

This src/sage_setup/__init__.py seems new (or copied from somewhere?) What is the intention? Should this

sage: sage_setup.sage_setup(['sagemath-modules'])

work? There's no doc for it...

kwankyu avatar Oct 18 '23 06:10 kwankyu

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

github-actions[bot] avatar Oct 23 '23 06:10 github-actions[bot]

This PR is quite big, it touches a lot of files (the vast majority in a very small way). Is it necessary to do brial and objects and categories all at once? Is there a technical reason this is required?

kiwifb avatar Jun 10 '24 01:06 kiwifb

One direction is technical: Getting sagemath-brial to build needs the enlargement of the other distributions.

The other direction is cultural: To demonstrate that the larger sagemath-categories is more useful, I am using sagemath-brial as the example.

mkoeppe avatar Jun 10 '24 01:06 mkoeppe

I was essentially after technical justification. It is not trivial to review changes to 616 files. Relying on automation is unavoidable in that case.

kiwifb avatar Jun 10 '24 01:06 kiwifb

Of course the changes that add the sage_setup: distribution lines have been made with an automated tool. The commit messages indicate this, for example

mkoeppe avatar Jun 10 '24 02:06 mkoeppe

If it helps, I can rebase so that all these are added in the first or in the last commit, or something like that, let me know

mkoeppe avatar Jun 10 '24 03:06 mkoeppe

If it helps, I can rebase so that all these are added in the first or in the last commit, or something like that, let me know

Rebased. I haven't added the sage_setup: distributions for now, for ease of reviewing.

mkoeppe avatar Jun 11 '24 02:06 mkoeppe

git diff 7ef824de14a3b31462ba97fb1d2076642ed6cf4a...a0196331926 will show the changes on top of the two merged dependencies.

mkoeppe avatar Jun 11 '24 02:06 mkoeppe

Ready for review...

mkoeppe avatar Jun 19 '24 17:06 mkoeppe

Dependencies are all merged, so let's get this in please.

mkoeppe avatar Jul 27 '24 17:07 mkoeppe

@kiwifb ?

mkoeppe avatar Aug 21 '24 05:08 mkoeppe

Thank you!

mkoeppe avatar Aug 21 '24 16:08 mkoeppe

Building fails with pbori.h import

vbraun avatar Aug 22 '24 12:08 vbraun

If it helps, I can rebase so that all these are added in the first or in the last commit, or something like that, let me know

Rebased. I haven't added the sage_setup: distributions for now, for ease of reviewing.

I have added it back now; only for sagemath-brial, not the other distributions (for which it is only decoration). I will add this decoration for the other distributions in a follow-up PR.

mkoeppe avatar Aug 22 '24 16:08 mkoeppe

Please only set it back to positive review after sucessfully building it from scratch on a system that does not have polybory pre-installed

vbraun avatar Aug 22 '24 19:08 vbraun

I did test the build after make brial-uninstall sagelib-clean, but I'll test again with make distclean.

mkoeppe avatar Aug 22 '24 19:08 mkoeppe

Yes, it builds, as expected.

mkoeppe avatar Aug 22 '24 20:08 mkoeppe

Still doesn't build for me, I guess there is some incompatible change in 10.5.beta3

[sagemath_doc_html-none] [spkg-install] [games    ] The inventory file is in ../../local/share/doc/sage/inventory/en/reference/games.
[sagemath_doc_html-none] [spkg-install] [finite_ri] The inventory file is in ../../local/share/doc/sage/inventory/en/reference/finite_rings.
[sagemath_doc_html-none] [spkg-install] [polynomia] The inventory file is in ../../local/share/doc/sage/inventory/en/reference/polynomial_rings.
[sagemath_doc_html-none] [spkg-install] Error building the documentation.
[sagemath_doc_html-none] [spkg-install] Traceback (most recent call last):
[sagemath_doc_html-none] [spkg-install]   File "<frozen runpy>", line 198, in _run_module_as_main
[sagemath_doc_html-none] [spkg-install]   File "<frozen runpy>", line 88, in _run_code
[sagemath_doc_html-none] [spkg-install]   File "/home/release/Sage/src/sage_docbuild/__main__.py", line 532, in <module>
[sagemath_doc_html-none] [spkg-install]     sys.exit(main())
[sagemath_doc_html-none] [spkg-install]              ^^^^^^
[sagemath_doc_html-none] [spkg-install]   File "/home/release/Sage/src/sage_docbuild/__main__.py", line 528, in main
[sagemath_doc_html-none] [spkg-install]     build()
[sagemath_doc_html-none] [spkg-install]   File "/home/release/Sage/src/sage_docbuild/builders.py", line 827, in _wrapper
[sagemath_doc_html-none] [spkg-install]     getattr(DocBuilder, build_type)(self, *args, **kwds)
[sagemath_doc_html-none] [spkg-install]   File "/home/release/Sage/src/sage_docbuild/builders.py", line 163, in f
[sagemath_doc_html-none] [spkg-install]     runsphinx()
[sagemath_doc_html-none] [spkg-install]   File "/home/release/Sage/src/sage_docbuild/sphinxbuild.py", line 319, in runsphinx
[sagemath_doc_html-none] [spkg-install]     sys.stderr.raise_errors()
[sagemath_doc_html-none] [spkg-install]   File "/home/release/Sage/src/sage_docbuild/sphinxbuild.py", line 255, in raise_errors
[sagemath_doc_html-none] [spkg-install]     raise OSError(self._error)
[sagemath_doc_html-none] [spkg-install] OSError: WARNING: autodoc: failed to import module 'pbori.pbori' from module 'sage.rings.polynomial'; the following exception was raised:

vbraun avatar Sep 04 '24 22:09 vbraun

I'll check with the merged branch again.

mkoeppe avatar Sep 04 '24 22:09 mkoeppe

Still doesn't build for me, I guess there is some incompatible change in 10.5.beta3

Confirmed

mkoeppe avatar Sep 05 '24 00:09 mkoeppe

It's correctly displaying

[sagemath_doc_html-none] [spkg-install] Warning: Could not import sage.rings.polynomial.pbori.pbori No module named 'sage.rings.polynomial.pbori.pbori'

but somehow it's still generating the file src/doc/en/reference/polynomial_rings/sage/rings/polynomial/pbori/pbori.rst with contents

.. nodoctest

.. _sage.rings.polynomial.pbori.pbori:

UNABLE TO IMPORT MODULE
=======================

.. This file has been autogenerated.


.. automodule:: sage.rings.polynomial.pbori.pbori
   :members:
   :undoc-members:
   :show-inheritance:

mkoeppe avatar Sep 06 '24 03:09 mkoeppe

somehow it's still generating the file src/doc/en/reference/polynomial_rings/sage/rings/polynomial/pbori/pbori.rst

It's gone now.

mkoeppe avatar Sep 06 '24 04:09 mkoeppe