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 sage.libs.giac with new optional package sagemath-giac #39376

Draft
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

orlitzky
Copy link
Contributor

As the last step in #38668, we move sage/libs/giac into a separate package 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.

As a prerequisite, I have replaced the old deprecated sage-cython script with a new, much shorter one. The new one simply ensures that we are invoking cython using the python that will be used to run sage. If we invoke it with (say) the system python instead, the search path for *.pxd files will be wrong. (Naturally, this breaks the build process for the new sagemath-giac package).

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.

Finally, this is a draft because the package info is pointing at my own personal GH repo, but that will change soon.

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.
The sage-cython script was deprecated in sage-8.7, so it should be OK
to remove now. There is a new reason for us to have a custom cython
script within sage-the-distro, and it makes sense to reuse this name
if possible.
When Sage is installed in a venv, sagelib gets added to sys.path by
virtue of venv/bin/python, which basically runs whatever python you
were going to run anyway, but with a modified sys.path.

If however you are building an external cython package using the
system cython, this falls apart: the system cython is run under the
system python, which does not have sagelib in its sys.path. This makes
it impossible to build cython SPKGs that depend on sagelib.

To address the issue, we add a tiny wrapper called sage-venv-cython
that essentially runs "python /path/to/cython" rather than"cython".
To use it, simply set

  export CYTHON=sage-cython

in spkg-install.in. (By the time the package is built, the wrapper will
be in $PATH.)
@orlitzky orlitzky marked this pull request as draft January 24, 2025 21:24
Copy link

github-actions bot commented Jan 24, 2025

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

@antonio-rojas
Copy link
Contributor

antonio-rojas commented Jan 24, 2025

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

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

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

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

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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants