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

Meta-ticket: Cython 3.0.0 (released July 2023) #29863

Closed
mkoeppe opened this issue Jun 15, 2020 · 33 comments · Fixed by #36110
Closed

Meta-ticket: Cython 3.0.0 (released July 2023) #29863

mkoeppe opened this issue Jun 15, 2020 · 33 comments · Fixed by #36110

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Jun 15, 2020

This ticket is for experimenting with an update to Cython 3, which entered the beta stage on 2023-02-25 after several years in alpha. (No release schedule has been published.)

https://github.com/cython/cython/blob/master/CHANGES.rst

See also:

Depends on #26254
Depends on #32832
Depends on #34257

CC: @kwankyu @williamstein @videlec @tscrim @tobiasdiez

Component: packages: standard

Keywords: upgrade

Branch/Commit: u/mkoeppe/meta_ticket__cython_3_0_0__unreleased_ @ 6c338f8

Issue created by migration from https://trac.sagemath.org/ticket/29863

@mkoeppe mkoeppe added this to the sage-9.2 milestone Jun 15, 2020
@slel
Copy link
Member

slel commented Jun 18, 2020

Changed keywords from none to upgrade

@slel slel added the c: cython label Jun 18, 2020
@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Aug 13, 2020
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 13, 2021

comment:3

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Feb 13, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Jul 19, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 14, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Mar 5, 2022
@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Upgrade Cython to 3.0.0 (unreleased) Meta-ticket: Cython 3.0.0 (unreleased) Jul 24, 2022
@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 25, 2022

Dependencies: #26254, #32832

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 25, 2022

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 25, 2022

Commit: 02ce997

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 25, 2022

comment:13

pplpy build fails:

    tree = Parsing.p_module(s, pxd, full_module_name)

  Error compiling Cython file:
  ------------------------------------------------------------
  ...
  from ppl_decl cimport *
  ^
  ------------------------------------------------------------

  ppl/bit_arrays.pxd:1:0: 'ppl_decl.pxd' not found

  Error compiling Cython file:

Last 10 new commits:

bea228bTry with localized import
89b8081Inline all other imports from expression
9bbf574Fix a few tests
803804fand one more test
a23ffe9Tiny edits
570058cMerge remote-tracking branch 'origin/u/mkoeppe/update_cython_to_0_29_29' into public/26254
02d6282Merge remote-tracking branch 'origin/develop' into public/26254
bca67b0Merge #26254
f537175build/pkgs/cython: Use 3.0.0a10
02ce997build/pkgs/cython/patches/trashcan.patch: Remove

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 25, 2022

comment:14
[sagelib-9.7.beta6]   Error compiling Cython file:
[sagelib-9.7.beta6]   ------------------------------------------------------------
[sagelib-9.7.beta6]   ...
[sagelib-9.7.beta6]               sage: p = next_prime(2^32)
[sagelib-9.7.beta6]               sage: GF(p)(int(p+1))
[sagelib-9.7.beta6]               1
[sagelib-9.7.beta6]           """
[sagelib-9.7.beta6]           mpz_set_si(self.value, value)
[sagelib-9.7.beta6]           mpz_mod(self.value, self.value, self.__modulus.sageInteger.value)
[sagelib-9.7.beta6]                                                                    ^
[sagelib-9.7.beta6]   ------------------------------------------------------------
[sagelib-9.7.beta6] 
[sagelib-9.7.beta6]   sage/rings/finite_rings/integer_mod.pyx:1986:66: Cannot convert Python object to '__mpz_struct *'

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 25, 2022

comment:15
[sagelib-9.7.beta6]   Error compiling Cython file:
[sagelib-9.7.beta6]   ------------------------------------------------------------
[sagelib-9.7.beta6]   ...
[sagelib-9.7.beta6]           """
[sagelib-9.7.beta6]           Lfunction.__init__(self, name, what_type_L, dirichlet_coefficient,  period, Q, OMEGA, gamma,lambd, pole,residue)
[sagelib-9.7.beta6]           self._repr += " with integer Dirichlet coefficients"
[sagelib-9.7.beta6] 
[sagelib-9.7.beta6]       # override
[sagelib-9.7.beta6]       cdef void __init_fun(self, char *NAME, int what_type, dirichlet_coeff, long long Period, double q,  c_Complex w, int A, double *g, c_Complex *l, int n_poles, c_Complex *p, c_Complex *r):
[sagelib-9.7.beta6]           ^
[sagelib-9.7.beta6]   ------------------------------------------------------------
[sagelib-9.7.beta6] 
[sagelib-9.7.beta6]   sage/libs/lcalc/lcalc_Lfunction.pyx:500:9: C method '_Lfunction_I__init_fun' not previously declared in definition part of extension type 'Lfunction_I'

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 25, 2022

comment:16
[sagelib-9.7.beta6]   Error compiling Cython file:
[sagelib-9.7.beta6]   ------------------------------------------------------------
[sagelib-9.7.beta6]   ...
[sagelib-9.7.beta6]           """
[sagelib-9.7.beta6]           return Matroid._repr_(self) + " with " + str(self.bases_count()) + " bases"
[sagelib-9.7.beta6] 
[sagelib-9.7.beta6]       # support for parent BasisExchangeMatroid
[sagelib-9.7.beta6] 
[sagelib-9.7.beta6]       cdef bint __is_exchange_pair(self, long x, long y) except -1:      # test if current_basis-x + y is a basis
[sagelib-9.7.beta6]           ^
[sagelib-9.7.beta6]   ------------------------------------------------------------
[sagelib-9.7.beta6] 
[sagelib-9.7.beta6]   sage/matroids/basis_matroid.pyx:268:9: C method '_BasisMatroid__is_exchange_pair' not previously declared in definition part of extension type 'BasisMatroid'

[sagelib-9.7.beta6]   Error compiling Cython file:
[sagelib-9.7.beta6]   ------------------------------------------------------------
[sagelib-9.7.beta6]   ...
[sagelib-9.7.beta6]                   b = frozenset(B)
[sagelib-9.7.beta6]                   if len(b) != self._matroid_rank:
[sagelib-9.7.beta6]                       raise ValueError("basis has wrong cardinality.")
[sagelib-9.7.beta6]                   if not b.issubset(self._groundset):
[sagelib-9.7.beta6]                       raise ValueError("basis is not a subset of the groundset")
[sagelib-9.7.beta6]                   self.__pack(self._b, b)
[sagelib-9.7.beta6]                                  ^
[sagelib-9.7.beta6]   ------------------------------------------------------------
[sagelib-9.7.beta6] 
[sagelib-9.7.beta6]   sage/matroids/basis_matroid.pyx:224:32: Cannot convert 'bitset_t' to Python object
[sagelib-9.7.beta6] 

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 25, 2022

comment:17
[sagelib-9.7.beta6]   Error compiling Cython file:
[sagelib-9.7.beta6]   ------------------------------------------------------------
[sagelib-9.7.beta6]   ...
[sagelib-9.7.beta6] 
[sagelib-9.7.beta6]       AUTHOR:
[sagelib-9.7.beta6] 
[sagelib-9.7.beta6]       - Xavier Caruso (2013-03)
[sagelib-9.7.beta6]       """
[sagelib-9.7.beta6]       cdef int __normalize(self) except -1:
[sagelib-9.7.beta6]           ^
[sagelib-9.7.beta6]   ------------------------------------------------------------
[sagelib-9.7.beta6] 
[sagelib-9.7.beta6]   sage/rings/polynomial/polynomial_element.pyx:11912:9: C method '_Polynomial_generic_dense_inexact__normalize' not previously declared in definition part of extension type 'Polynomial_generic_dense_inexact'

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 25, 2022

Changed commit from 4e14632 to 46af90e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 25, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

46af90esrc/sage/rings/polynomial/polynomial_element.pxd: Add missing cdef method declaration

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 25, 2022

comment:21

Help wanted with Cannot convert 'bitset_t' to Python object and Cannot convert Python object to '__mpz_struct *'

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 31, 2022

Changed dependencies from #26254, #32832 to #26254, #32832, #34257

@mkoeppe

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 31, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

1f9ee80src/sage/libs/lcalc/lcalc_Lfunction.pxd: Add missing cdef method declarations
034094csrc/sage/matroids/{basis,linear}_matroid.pxd: Add missing cdef method declarations
9d89bdfsrc/sage/rings/polynomial/polynomial_element.pxd: Add missing cdef method declaration
727d101Merge #34257
6c338f8build/pkgs/pplpy/patches: Add https://gitlab.com/videlec/pplpy/-/merge_requests/20

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 31, 2022

Changed commit from 46af90e to 6c338f8

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 31, 2022

comment:25

Replying to @mkoeppe:

pplpy build fails:

    tree = Parsing.p_module(s, pxd, full_module_name)

  Error compiling Cython file:
  ------------------------------------------------------------
  ...
  from ppl_decl cimport *
  ^
  ------------------------------------------------------------

  ppl/bit_arrays.pxd:1:0: 'ppl_decl.pxd' not found

  Error compiling Cython file:

PR: https://gitlab.com/videlec/pplpy/-/merge_requests/20

@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Aug 31, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.8, sage-9.9 Jan 7, 2023
@mkoeppe mkoeppe changed the title Meta-ticket: Cython 3.0.0 (unreleased) Meta-ticket: Cython 3.0.0 (entered beta stage) Feb 26, 2023
@mkoeppe mkoeppe removed this from the sage-10.0 milestone Feb 26, 2023
@mkoeppe mkoeppe changed the title Meta-ticket: Cython 3.0.0 (entered beta stage) Meta-ticket: Cython 3.0.0 (entered rc stage) Jul 14, 2023
@tornaria
Copy link
Contributor

Now that 3.0.0 is released, could we have this rebased and in a PR, please?

@mkoeppe mkoeppe changed the title Meta-ticket: Cython 3.0.0 (entered rc stage) Meta-ticket: Cython 3.0.0 (released July 2023) Jul 17, 2023
@tornaria
Copy link
Contributor

Related: cython/cython#5552

@tornaria
Copy link
Contributor

Cf: https://github.com/tornaria/sage/tree/cython3-legacy, I've got as far as being able to cythonize everything, but some C files still fail to compile.

First five commits are git cherry-pick f36275a7869 36ec4930832 1f9ee80cded 034094c849e 9d89bdf776 from the sagetrac mirror on top of 10.1.beta7.

Note that cython/cython#5552 means cythonize fails if python package lxml is available. Can use SAGE_DEBUG=no to avoid the issue which only happens with debug enabled.

Also: void-linux/void-packages#45086 includes patches for cysignals, cypari2, fpylll and pplpy that might be useful.

@tornaria
Copy link
Contributor

After adding a commit fixing inheritance in lcalc_Lfunction (see 10.1.beta7...tornaria:sage:d0ad68ed9d4) I've got the whole of sagelib to compile.

Now it fails at import sage.structure.element as follows:

$ PYTHONPATH=pkgs/sagemath-standard/build/lib.linux-x86_64-cpython-311 python
Python 3.11.4 (main, Jun  8 2023, 02:02:15) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sage.structure.element
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../pkgs/sagemath-standard/build/lib.linux-x86_64-cpython-311/sage/structure/__init__.py", line 2, in <module>
    import sage.structure.element
  File "sage/structure/element.pyx", line 1, in init sage.structure.element (build/cythonized/sage/structure/element.c:44218)
TypeError: PyMethodDescr_CallSelf requires a method without arguments

Please note: I'll probably keep rebasing and/or changing commits in my branch https://github.com/tornaria/sage/tree/cython3-legacy so don't take them as public commits unless you are willing to rebase.

@tornaria
Copy link
Contributor

Branch pushed to git repo; I updated commit sha1. New commits:
fd2e122 src/sage/libs/lcalc/lcalc_Lfunction.pxd: Add missing cdef method declarations

This is incorrect and it stems from a fundamental change in cython 3 that will probably bite us all over the place (and this one cannot be switched off afaict).

I better link to the relevant section of the migration guide: https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#class-private-name-mangling

This means sagelib has been (mis)using __name members (methods and attributes) that (a) in python are class-private via name mangling (b) in cython before 3.0.0 they are inherited due to mangling not implemented (c) in cython 3.0.0 they are class-private via the exact same name mangling as in python.

So the way to fix all these "missing cdef ..." errors is not by adding the missing methods but renaming the members to use a single underscore which is the python way to declare a "private" member that is still inherited by subclasses (think "protected" in c++).

vbraun pushed a commit to vbraun/sage that referenced this issue Sep 1, 2023
    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

Here are the changes needed to get Sage compiled with Cython 3. This PR
contains changes that do not break compatibility with old Cython, so
they can be merged without actually upgrading Cython.

Part of sagemath#29863.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.

### ⌛ Dependencies

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

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36109
Reported by: Yutao Yuan
Reviewer(s): Gonzalo Tornaría, Yutao Yuan
@vbraun vbraun closed this as completed in 05d7d1d Sep 27, 2023
@mkoeppe mkoeppe added this to the sage-10.2 milestone Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants