Replace sage.libs.giac with new optional package sagemath-giac #39376

orlitzky · 2025-01-24T21:24:19Z

As the last step in #38668, we move sage/libs/giac into a separate package,

https://github.com/sagemath/sagemath-giac

and make it optional. This module is already mostly independent of sage -- there is nothing in sagelib that depends on it unconditionally. Its main use is as an integration backend, where it is technically optional because we try whatever backends are available in succession and expect some to fail.

In fact, the modules themselves are already optional if you are using meson. The sage/libs/giac directory is simply skipped when meson cannot find giac. It is not possible to do the same thing using the build system for sage-the-distro, however, which motivates the separate package. Many reasons are given in #38668 for why you might want to avoid giac, but the main reason for me right now is because it's not portable: it doesn't build and/or run on systems where the rest of sage works fine, including the one I am using most of the time.

The new package uses the namespace sagemath_giac to work around missing namespace support in cython, but sage.libs.giac re-exports everything under the old name for backwards compatibility.

github-actions · 2025-01-24T22:07:54Z

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

antonio-rojas · 2025-01-24T23:20:26Z

In fact, the modules themselves are already optional if you are using meson. The sage/libs/giac directory is simply skipped when meson cannot find giac. It is not possible to do the same thing using the build system for sage-the-distro, however, which motivates the separate package.

How is giac integration different from bliss/meataxe/sirocco that makes it impossible for it to be optional while still living in the main sage source tree?

orlitzky · 2025-01-25T00:53:33Z

How is giac integration different from bliss/meataxe/sirocco that makes it impossible for it to be optional while still living in the main sage source tree?

It's not. Matthias has a sagemath_giac in his fork of Sage along those same lines, but however you go about it, it's a good amount of work. (You can't just add a --disable-giac flag that skips sage/libs/giac.)

What you'll find at https://github.com/orlitzky/sagemath-giac is a normal cython package. You can clone it, build it, test it, package it, etc. independent of sage-the-distro, using what you already know as a python developer. There's no magic, and even the doctests work with a simple python -m doctest or pytest. Considering that both approaches involve a significant amount of work, I find this to be a much friendlier design.

antonio-rojas · 2025-01-25T09:54:39Z

What you'll find at https://github.com/orlitzky/sagemath-giac is a normal cython package. You can clone it, build it, test it, package it, etc. independent of sage-the-distro, using what you already know as a python developer. There's no magic, and even the doctests work with a simple python -m doctest or pytest. Considering that both approaches involve a significant amount of work, I find this to be a much friendlier design.

I disagree - thinking outside the sage-the-distro framework I see a few disadvantages of this approach:

This makes sagemath-giac a de-facto third-party package. When there are breaking changes in sagelib, you need to port stuff in two different places instead of having all code centralized in a single repo (adds burden to developers), and make sure everything is released in sync (adds burden to upstream maintainers) - we're seeing this now with the pari 2.17 upgrade and SnapPy.
You can no longer build and package all components (standard and optional) from a single repo - you have to either combine several sources at build time or create separate packages (adds burden to downstream packagers).

Since, as you said, this works properly in meson and we're moving towards making meson the default buildsystem, won't this eventually fix itself?

orlitzky · 2025-01-25T11:27:11Z

This makes sagemath-giac a de-facto third-party package. When there are breaking changes in sagelib, you need to port stuff in two different places instead of having all code centralized in a single repo (adds burden to developers), and make sure everything is released in sync (adds burden to upstream maintainers) - we're seeing this now with the pari 2.17 upgrade and SnapPy.

You can no longer build and package all components (standard and optional) from a single repo - you have to either combine several sources at build time or create separate packages (adds burden to downstream packagers).

Fair concerns, but in this case I would not be too worried:

These modules are very lightly integrated with sage and should not break when sage upgrades. If you look through the git log, you'll see a few module names changing in import statements, but nothing else. (And we should have a year to sync those while the old names are deprecated.)
On the contrary, because it is largely independent of sage, it will be less work to package it once than it would be to package (the same exact code) multiple times with each release of sage, like we do with sagemath_bliss and friends. The most common changes are updates to the doctest output due to changes in giac, which become much easier to fix and release in a minor bump to sagemath-giac without having to wait 6 months for the next version of sage while your tests fail.
It's becoming an optional package, so it requires no extra work. You can do nothing and upgrade never :)

Since, as you said, this works properly in meson and we're moving towards making meson the default buildsystem, won't this eventually fix itself?

Absolutely, this was a tempting approach. But giac is completely broken on both types of systems I have. It doesn't build on anything other than x86_64/arm64, and even on x86_64, it crashes under a hardened glibcxx. How long is eventually?

orlitzky · 2025-01-25T11:42:12Z

It is maybe worth mentioning that, once upon a time, this was already a separate package. It was bundled with sagelib when it was made a standard package to avoid a circular dependency sagelib -> sagemath_giac -> sagelib. But today, there is no sagelib -> sagemath_giac dependency. All of the integration backends are drop-ins and can fail without issue.

tobiasdiez · 2025-02-02T12:08:23Z

Can you explain the problem with the cython compilation in more detail please. You mentioned in the other PR:

Basically, you can't compile external cython packages against sage-the-distro because cython will use sys.path from the system python, and thus all of sage is hidden from it.

So, you have sage installed in venv A, but cython in venv B; and try to build sagemath-giac in venv A?

I think the reason why this is not working is because you don't declare sage as a dependency here:
https://github.com/orlitzky/sagemath-giac/blob/23d0d7d6073d9034d7fd87bd5aca0657c98cdd89/src/sagemath_giac/meson.build#L52
You probably need to add a similar construction for sage as you do for gmpy2.

To make this easier we may add a sage-config tool similar to what numpy did: numpy/numpy#25730

orlitzky · 2025-02-02T17:10:03Z

So, you have sage installed in venv A, but cython in venv B; and try to build sagemath-giac in venv A?

Almost. The system cython is being used to build sagemath-giac against sagelib installed in a venv (I am building sage-the-distro "normally" to make sure it works). Without the sage-cython wrapper, this is what happens:

[sagemath_giac-0.1.0] [spkg-install] [1/3] Compiling Cython source /home/mjo/src/sage.git/local/var/tmp/sage/build/sagemath_giac-0.1.0/src/src/sagemath_giac/giac.pyx
[sagemath_giac-0.1.0] [spkg-install] FAILED: src/sagemath_giac/giac.cpython-312-x86_64-linux-gnu.so.p/src/sagemath_giac/giac.pyx.cpp 
[sagemath_giac-0.1.0] [spkg-install] cython -M --fast-fail -3 --cplus /home/mjo/src/sage.git/local/var/tmp/sage/build/sagemath_giac-0.1.0/src/src/sagemath_giac/giac.pyx -o src/sagemath_giac/giac.cpython-312-x86_64-linux-gnu.so.p/src/sagemath_giac/giac.pyx.cpp
[sagemath_giac-0.1.0] [spkg-install] 
[sagemath_giac-0.1.0] [spkg-install] Error compiling Cython file:
[sagemath_giac-0.1.0] [spkg-install] ------------------------------------------------------------
[sagemath_giac-0.1.0] [spkg-install] ...
[sagemath_giac-0.1.0] [spkg-install] from sys import maxsize as Pymaxint, version_info as Pyversioninfo
[sagemath_giac-0.1.0] [spkg-install] import os
[sagemath_giac-0.1.0] [spkg-install] import math
[sagemath_giac-0.1.0] [spkg-install] 
[sagemath_giac-0.1.0] [spkg-install] # sage includes
[sagemath_giac-0.1.0] [spkg-install] from sage.ext.stdsage cimport PY_NEW
[sagemath_giac-0.1.0] [spkg-install] ^
[sagemath_giac-0.1.0] [spkg-install] ------------------------------------------------------------
[sagemath_giac-0.1.0] [spkg-install] 
[sagemath_giac-0.1.0] [spkg-install] /home/mjo/src/sage.git/local/var/tmp/sage/build/sagemath_giac-0.1.0/src/src/sagemath_giac/giac.pyx:144:0: 'sage/ext/stdsage.pxd' not found

That's a "cythonize" failure that occurs because the sage.* namespace is not visible to cython, which gets its search path from python's sys.path. Meson is run using the venv python where everything looks OK because the venv is magically added to its sys.path. But then the system cython is ultimately run (from its shebang) using /usr/bin/python, which does not include the venv in its sys.path. As a result, all cimport statements for modules in the venv fail.

I think the reason why this is not working is because you don't declare sage as a dependency here: https://github.com/orlitzky/sagemath-giac/blob/23d0d7d6073d9034d7fd87bd5aca0657c98cdd89/src/sagemath_giac/meson.build#L52 You probably need to add a similar construction for sage as you do for gmpy2.

If you look further down, I am including a similar thing for sage.cpython. This addresses the compilation stage (where some headers are required), but I don't think it affects the cythonize phase?

tobiasdiez · 2025-02-02T17:36:09Z

If the issue is at the cythonize stage, then adding something like this should work (with the path to the root of sage):

sage/src/meson.build

Lines 128 to 131 in dc99dc8

    
           # Meson currently ignores include_directories for Cython modules, so we 
        
           # have to add them manually. 
        
           # https://github.com/mesonbuild/meson/issues/9562 
        
           add_project_arguments('-I', meson.current_source_dir(), language: 'cython')

orlitzky · 2025-02-02T18:34:07Z

If the issue is at the cythonize stage, then adding something like this should work (with the path to the root of sage):

Sure, but this is just one of potentially many external cython packages, and the issue only arises because sage-the-distro hides sagelib in a venv. Wouldn't it be better to address the problem once, in sage-the-distro?

sagemath-standard is declared as a dependency of sagemath-giac (in pyproject.toml), so it is expected to be available to python. As with any build system, you can put it in an unusual place, but then it is up to you to inform the build system where to find it (at build time) and eventually ensure that it is available at run time. I am thinking along the lines of -L/usr/local/lib (build time) and LD_LIBRARY_PATH=/usr/local/lib (run time).

At build time, sage-the-distro is using the venv python, which has all of the dependencies in its path. At run time, sage-the-distro uses the venv python for the same reason. It's only the call to the system cython with an un-fudged path (at build time) that is a problem. And IMO sage-the-distro should be responsible for ensuring that the cython it's using can find the dependencies of the packages it is installing. We could provide include directories by hand in sage-the-distro, but the simplest way I could think of was just to launch cython using the venv python which makes everything consistent.

orlitzky · 2025-02-19T23:06:00Z

Reviewers should take a look at https://github.com/sagemath/sagemath-giac too since obviously a big part of this was separating the package.

orlitzky · 2025-02-19T23:30:33Z

@dimpase I don't know why but you aren't in the reviewer dropdown

dimpase · 2025-02-19T23:45:03Z

I screamed too loudly on #39467.
Please get @tobiasdiez to review.

orlitzky · 2025-02-20T00:04:42Z

Since, as you said, this works properly in meson and we're moving towards making meson the default buildsystem, won't this eventually fix itself?

Thinking more about this recently, there is another reason why "wait for meson" isn't as nice of a solution. On Gentoo (and on sage-the-distro), sagelib usually "breaks" whenever giac is updated. And, to fix it, you have to rebuild all of sagelib. With sagemath-giac separate, you can instead do a 30s rebuild of the affected modules only.

dimpase · 2025-02-20T00:32:04Z

"wait for meson" is worse than doing a proper Pythonic modularisation, IMHO.

tobiasdiez · 2025-02-20T10:08:50Z

If the issue is at the cythonize stage, then adding something like this should work (with the path to the root of sage):

Sure, but this is just one of potentially many external cython packages, and the issue only arises because sage-the-distro hides sagelib in a venv. Wouldn't it be better to address the problem once, in sage-the-distro?

At build time, sage-the-distro is using the venv python, which has all of the dependencies in its path. At run time, sage-the-distro uses the venv python for the same reason. It's only the call to the system cython with an un-fudged path (at build time) that is a problem. And IMO sage-the-distro should be responsible for ensuring that the cython it's using can find the dependencies of the packages it is installing. We could provide include directories by hand in sage-the-distro, but the simplest way I could think of was just to launch cython using the venv python which makes everything consistent.

Sorry for the late reply. This makes a lot more sense now. So, I agree, having a "sage cython" in sage-the-distro as a wrapper around cython with the correct path/deps handling looks like a good solution. But this should then be a script specific to sage-the-distro, and not something that is installed as part of sagelib, right?

orlitzky · 2025-02-20T13:13:37Z

But this should then be a script specific to sage-the-distro, and not something that is installed as part of sagelib, right?

Right.

orlitzky · 2025-02-20T13:34:32Z

(The Gentoo CI failures related to giac are a pre-existing condition)

kiwifb · 2025-02-20T20:24:34Z

Regardless of modularisation, having to recompile sage to enable an option (in the sage packaging context - I am aware that if you are pottering in your local tree install, it is not the same) has always been a bit of a downer. Having it being a plug and play thing is much more effective and easy to turn on and off.
I do not care so much about the rest of modularisation, but I have been packaging sage for 15+ years now and separating optional stuff makes my life easier and align better with best practice in the python ecosystem where functionality is available at runtime if the right package is installed.

tobiasdiez · 2025-02-21T20:36:29Z

But this should then be a script specific to sage-the-distro, and not something that is installed as part of sagelib, right?

Right.

Could you then please create this sage-cython wrapper in build and not reuse the one in src that is still shipped to all users? Thanks!

tobiasdiez

Apart from the sage-cython script and some minor comments, things are looking good to me.

(Sorry for the late review)

tobiasdiez · 2025-02-21T20:42:50Z

src/sage/features/sagemath.py

@@ -560,7 +560,7 @@ def __init__(self):
        """
        JoinFeature.__init__(self, 'sage.libs.giac',
                             [PythonModule('sage.libs.giac.giac')],
-                             spkg='sagemath_giac', type='standard')
+                             spkg='sagemath_giac', type='optional')


Should this feature not better now use the new sagemath-giac package to test it? sage.libs.giac.giac is always available.

Alternatively, if we no longer need the giac tests (because all tests are now in the new giac repo), then please remove it.

This still works because the PythonModule test tries to import the module and it will fail if sagemath-giac is not installed:

sage: from sage.features.sagemath import sage__libs__giac sage: sage__libs__giac().is_present() FeatureTestResult('sage.libs.giac.giac', False)

There are indeed some integral tests in sagelib that have # needs sage.libs.giac. I did some work a month or two ago to carefully separate them from the # needs giac tests.

tobiasdiez · 2025-02-21T20:45:05Z

src/sage/libs/giac/__init__.py

-        gb_giac = F.eliminate(list(elim_variables), 'gbasis')
-
-    return PolynomialSequence(gb_giac, P, immutable=True)
+# sagemath_giac split its __init__.py into these two


Not utterly important, but could we add a pytest that checks if this integration is working at least in principle by, say, calculating something simple.

Can you elaborate?

In sagemath-giac, I have taken the contents of the old __init__.py and moved them into two separate files, gb.py and context.py. In sagelib I am importing those two modules inside of __init__.py so that, if sagemath-giac is installed, all of the old stuff will be available where you expect it using from sage.libs.giac import foo. If, on the other hand, sagemath-giac is not installed, then import sage.libs.giac will fail with an ImportError.

There are doctests that have # needs sage.libs.giac that will test this when sagemath-giac is installed. Or did you just want something for pytest specifically?

I wanted a test that checks that these simple reexports work as expected (as a kind of silly integration test). But, as you write above, there are still tests in sage using giac, so this is not really needed.

orlitzky · 2025-02-22T01:28:38Z

Gone again, no rebuild. And sagelib doesn't need an := dependency on giac any more.

for := to be useful, we would need giac to start using soname that are not 0.0.0.

We can set SLOT=0/${PV} on giac now to indicate that all upgrades potentially break ABI. In the past this would have been too painful, but now you can put the := dep in sagemath-giac (which rebuilds quickly) rather than in sagemath-standard.

orlitzky · 2025-02-22T02:04:56Z

Could you then please create this sage-cython wrapper in build and not reuse the one in src that is still shipped to all users? Thanks!

This one is not so easy because sage-cython needs to be inside the venv for it to magically pick up the venv python/sitedir when it is launched.

src/setup.cfg already contains one line,

# Only makes sense in sage-the-distribution. TODO: Move to another installation script.
bin/sage-list-packages

but I've no idea what such an installation script might look like.

This package has been split off into sagemath-giac.

SageMath no longer links against libgiac directly.

This directory has been removed (into a separate package).

The sage library can now be built without giac.

The sage library no longer requires giac.

This helper script has been moved to the sagemath-giac package.

The src/sage/libs/giac tree has been split off into a separate package, sagemath-giac.

This interface is now contained in a separate package, but until cython supports namespace packages, that interface must have a different name. We bring back sage.libs.giac as a wrapper around it to avoid breaking existing code.

This optional sage/giac integration package replaces the old hard dependency on giac that was required to build sage.libs.giac.

This is now strictly required only by the (optional) sagemath_giac package. There is a feature test guarding the doctests in the old pexpect interface under sage.interfaces.giac, and nothing else in sagelib imports that pexpect interface, so if you do not use it explicitly, giac is optional.

orlitzky · 2025-02-24T01:39:36Z

Force push:

Rebase onto develop
Update sagemath-giac to v0.1.1
Eliminate the sage-cython hack (we still need the hack, but it happens in meson now)

tobiasdiez

Thanks for all the work you put into this!

orlitzky · 2025-02-25T14:04:39Z

Thanks! I've updated the release tour, and with this, sage now runs on risc-v hardware out-of-the-box:

https://github.com/sagemath/sage/wiki/Sage-10.6-Release-Tour

orlitzky marked this pull request as draft January 24, 2025 21:24

orlitzky mentioned this pull request Oct 27, 2024

Support for --disable-giac #38668

Open

orlitzky force-pushed the sagemath-giac branch from 8302d6c to 454c025 Compare January 26, 2025 13:55

orlitzky mentioned this pull request Feb 2, 2025

Remove deprecated sage-cython script #39427

Merged

5 tasks

orlitzky force-pushed the sagemath-giac branch from 454c025 to 37eba1c Compare February 19, 2025 22:50

orlitzky requested review from kiwifb, tornaria, tobiasdiez and antonio-rojas February 19, 2025 23:00

orlitzky marked this pull request as ready for review February 19, 2025 23:02

github-actions bot added the s: needs review label Feb 19, 2025

kiwifb removed the s: needs review label Feb 21, 2025

tobiasdiez suggested changes Feb 21, 2025

View reviewed changes

tobiasdiez added s: needs work and removed s: positive review labels Feb 21, 2025

orlitzky added 12 commits February 22, 2025 23:25

src/sage/libs/giac: remove

4549c4c

This package has been split off into sagemath-giac.

src/sage_setup/library_order.py: drop giac from library_order_list

9269c3c

SageMath no longer links against libgiac directly.

src/doc/en/reference/libs/index.rst: remove sage/libs/giac

3734d30

This directory has been removed (into a separate package).

build/pkgs/sagelib/dependencies: remove giac

84c71f2

The sage library can now be built without giac.

src/pyproject.toml: drop giac dependency

007a1a5

The sage library no longer requires giac.

src/sage_setup/autogen/giacpy-mkkeywords.py: remove

60a42a3

This helper script has been moved to the sagemath-giac package.

src/sage/libs/meson.build: no more giac subdirectory

1e344f9

The src/sage/libs/giac tree has been split off into a separate package, sagemath-giac.

build/pkgs/sagemath_giac: new optional package

c023a2d

This optional sage/giac integration package replaces the old hard dependency on giac that was required to build sage.libs.giac.

src/sage/features/giac.py: standard -> optional

65ff51e

src/sage/features/sagemath.py: mark sage.libs.giac as optional

d7a5f54

orlitzky force-pushed the sagemath-giac branch from 37eba1c to d7a5f54 Compare February 24, 2025 01:33

github-actions bot added s: needs review and removed s: needs work labels Feb 24, 2025

tobiasdiez approved these changes Feb 25, 2025

View reviewed changes

tobiasdiez added s: positive review and removed s: needs review labels Feb 25, 2025

jhpalmieri mentioned this pull request Feb 27, 2025

downgrade giac to optional #38712

Closed

2 tasks

vbraun merged commit 3cf106e into sagemath:develop Feb 28, 2025
24 of 49 checks passed

github-actions bot removed the s: positive review label Feb 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace sage.libs.giac with new optional package sagemath-giac #39376

Replace sage.libs.giac with new optional package sagemath-giac #39376

orlitzky commented Jan 24, 2025 •

edited

Loading

github-actions bot commented Jan 24, 2025 •

edited

Loading

antonio-rojas commented Jan 24, 2025 •

edited

Loading

orlitzky commented Jan 25, 2025

antonio-rojas commented Jan 25, 2025

orlitzky commented Jan 25, 2025

orlitzky commented Jan 25, 2025

tobiasdiez commented Feb 2, 2025

orlitzky commented Feb 2, 2025

tobiasdiez commented Feb 2, 2025

orlitzky commented Feb 2, 2025

orlitzky commented Feb 19, 2025

orlitzky commented Feb 19, 2025

dimpase commented Feb 19, 2025

orlitzky commented Feb 20, 2025

dimpase commented Feb 20, 2025

tobiasdiez commented Feb 20, 2025

orlitzky commented Feb 20, 2025

orlitzky commented Feb 20, 2025

kiwifb commented Feb 20, 2025 •

edited

Loading

tobiasdiez commented Feb 21, 2025

tobiasdiez left a comment

tobiasdiez Feb 21, 2025

orlitzky Feb 22, 2025

tobiasdiez Feb 21, 2025

orlitzky Feb 22, 2025

tobiasdiez Feb 25, 2025

orlitzky commented Feb 22, 2025

orlitzky commented Feb 22, 2025

orlitzky commented Feb 24, 2025

tobiasdiez left a comment

orlitzky commented Feb 25, 2025

Replace sage.libs.giac with new optional package sagemath-giac #39376

Replace sage.libs.giac with new optional package sagemath-giac #39376

Conversation

orlitzky commented Jan 24, 2025 • edited Loading

github-actions bot commented Jan 24, 2025 • edited Loading

antonio-rojas commented Jan 24, 2025 • edited Loading

orlitzky commented Jan 25, 2025

antonio-rojas commented Jan 25, 2025

orlitzky commented Jan 25, 2025

orlitzky commented Jan 25, 2025

tobiasdiez commented Feb 2, 2025

orlitzky commented Feb 2, 2025

tobiasdiez commented Feb 2, 2025

orlitzky commented Feb 2, 2025

orlitzky commented Feb 19, 2025

orlitzky commented Feb 19, 2025

dimpase commented Feb 19, 2025

orlitzky commented Feb 20, 2025

dimpase commented Feb 20, 2025

tobiasdiez commented Feb 20, 2025

orlitzky commented Feb 20, 2025

orlitzky commented Feb 20, 2025

kiwifb commented Feb 20, 2025 • edited Loading

tobiasdiez commented Feb 21, 2025

tobiasdiez left a comment

Choose a reason for hiding this comment

tobiasdiez Feb 21, 2025

Choose a reason for hiding this comment

orlitzky Feb 22, 2025

Choose a reason for hiding this comment

tobiasdiez Feb 21, 2025

Choose a reason for hiding this comment

orlitzky Feb 22, 2025

Choose a reason for hiding this comment

tobiasdiez Feb 25, 2025

Choose a reason for hiding this comment

orlitzky commented Feb 22, 2025

orlitzky commented Feb 22, 2025

orlitzky commented Feb 24, 2025

tobiasdiez left a comment

Choose a reason for hiding this comment

orlitzky commented Feb 25, 2025

orlitzky commented Jan 24, 2025 •

edited

Loading

github-actions bot commented Jan 24, 2025 •

edited

Loading

antonio-rojas commented Jan 24, 2025 •

edited

Loading

kiwifb commented Feb 20, 2025 •

edited

Loading