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

sagemath: use cython 3 #45887

Closed
wants to merge 3 commits into from
Closed

Conversation

tornaria
Copy link
Contributor

@tornaria tornaria commented Sep 2, 2023

Testing the changes

  • I tested the changes in this PR: briefly

Since sagemath/sage#36109 was merged upstream, it's now almost trivial to build sagemath with cython 3. This PR is pretty straightforward to understand:

  • commit 7f8e651 rebuilds python3-pplpy using cython 3. Nothing fancy here and this commit was already tested and only dropped since it turned out it had to be built with the same version of cython as sagemath itself.
  • commit df0d218 is just adding a few patches to sagemath, switching to cython 3, and bumping.

Description of added patches:

  1. patch 36109-00pre.patch: this is hand picked trivial patch that allows us to apply Prepare for updating Cython to 3.0.0 sagemath/sage#36109 cleanly on 10.1 (since that PR is based on 10.1.beta0). All changes are comments!
  2. patch 36109-prepare_for_cython_3.0.patch: this is long, taken verbatim from https://github.com/sagemath/sage/pull/36109.diff, and is already merged.
  3. patch build-cython3.patch: this is 2 lines changed plus 4 legacy build options added to cython setup.
  4. patch fix-doctest-cython3.patch: this is a few fixes in doctests to accomodate minor changes in cython output (__repr__ for cython classes, functions, and generators).

CC: @ahesford

@ahesford
Copy link
Member

ahesford commented Sep 2, 2023

What is the benefit of building with Cython 3 in advance of an official Sage release that includes these changes?

@tornaria
Copy link
Contributor Author

tornaria commented Sep 3, 2023

What is the benefit of building with Cython 3 in advance of an official Sage release that includes these changes?

Mainly that I can install cython 3 together with sagemath, which is quite useful for me to test beta releases of sagemath and proposed PRs in advance. But that's just me and I can build this locally myself.

Running this PR through CI is a way to make sure everything works fine for us, which is good feedback to give upstream before they do the next release, in particular sagemath/sage#36110 is still not given positive review and the fact that we have tested it onthree different architectures is useful (unfortunately some deps are nocross).

I noticed that you made it so cython 3 can replace cython 0.29, but some I'm now runing tests under this scenario, and there are a couple of failures (mainly because a mismatch in the type of cython functions between version 0.29 and 3.x).

@ahesford
Copy link
Member

ahesford commented Sep 3, 2023

You can ignorepkg=python3-Cython0.29 and install python3-Cython even if sagemath still requires the former.

Both Cython packages replace each other because that is needed to allow a clean update path to the current sagemath from an older package, and will be required in the future when sagemath moves to Cython 3.0; otherwise, the update would try to install the new Cython required for the Sage update, find that it conflicts with the version required by the old Sage, and abort. Unfortunately, there is also some unexpected behavior that will allow users to break package consistency by replacing the required Cython package with the other version. (This might be helpful for testing, because it allows quick swapping between Cython packages without having to replace the installed sagemath.)

@tornaria
Copy link
Contributor Author

tornaria commented Sep 4, 2023

You can ignorepkg=python3-Cython0.29 and install python3-Cython even if sagemath still requires the former.

Both Cython packages replace each other because that is needed to allow a clean update path to the current sagemath from an older package, and will be required in the future when sagemath moves to Cython 3.0; otherwise, the update would try to install the new Cython required for the Sage update, find that it conflicts with the version required by the old Sage, and abort. Unfortunately, there is also some unexpected behavior that will allow users to break package consistency by replacing the required Cython package with the other version. (This might be helpful for testing, because it allows quick swapping between Cython packages without having to replace the installed sagemath.)

All of that is pointless, since sagemath built with cython 0.29 is broken with cython 3.0.

@ahesford
Copy link
Member

ahesford commented Sep 6, 2023

[I thought I already posted a response, but it seems lost to the ether.]

I understand that Sage built with Cython 0.29 is broken with Cython 3.0, but my point was that you can easily swap between installed Cython versions to test a locally built Sage with these patches.

The general policy for patches is that they should be limited to critical fixes in advance of an official release or correct an incompatibility introduced by our own packaging choices (e.g., to support special aspects of our cross building). New features, even when merged upstream, should wait until an official release introduces them. This is particularly true here, where the patches to add support are more than 9k lines long.

@ahesford
Copy link
Member

ahesford commented Sep 7, 2023

After an out-of-band discussion about this PR, I am less concerned about carrying these patches. That said, is there any argument in favor of just bumping sagemath to 10.2.beta1 rather than patching 10.1?

@tornaria
Copy link
Contributor Author

tornaria commented Sep 9, 2023

After an out-of-band discussion about this PR, I am less concerned about carrying these patches.

After thinking it thoroughly I don't think there is a rush to have sagemath with cython 3 merged (even if I believe the patches to be pretty safe). Since nothing in the repo depends on python-Cython, there is no conflict at all in that sagemath depends on python-Cython0.29. Functionally there should be no difference (for the time being... cython 3 unlocks some features that will eventually be used in sagemath).

I'm happy to either close or move to draft this PR. In any case this will stay available for whoever needs to run sagemath on cython3.

That said, is there any argument in favor of just bumping sagemath to 10.2.beta1 rather than patching 10.1?

We've tried to stick with releases rather than betas. The problem with betas is that sometimes there are features that are "half-merged" so I wouldn't be confident. I much prefer cherry-picking PRs that have been already merged and beta-released. I also try to only use patches I personally review (the 9k lines patch I actually read line by line and positive review myself).

OTOH, sometimes I consider having some package "sagemath-next" which previews the future sagemath (carrying the latest beta / rc) in a way that it can coexist with "sagemath".

@ahesford
Copy link
Member

In that case, we can mark this draft and move it to 10.2 when ready. In the meantime, if a compelling need to jump to Cython 3 arises sooner, we will have this ready to go in its current state.

@tornaria
Copy link
Contributor Author

tornaria commented Oct 9, 2023

Updated for python 3.12.

Note: in principle we won't be merging this. When sagemath 10.2 is released, it will move to cython 3. But the current betas of sagemath 10.2 need cython 3 to build so it's easier if the system sagemath also runs with cython 3.

@tornaria
Copy link
Contributor Author

It turns out #46553 is needed for sagemath to pass check with cython 3.

@tornaria
Copy link
Contributor Author

I won't keep up this PR anymore. If you need sagemath with cython 3, use #46832.

@tornaria tornaria closed this Oct 22, 2023
@tornaria tornaria deleted the sagemath-cython3 branch October 22, 2023 14:51
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