Prepare for updating Cython to 3.0.0 #36109
Conversation
|
Tests pass (the failures in |
There was a problem hiding this comment.
I have built and doctested this PR, using cython 0.29.36 in void linux (glibc, 64 bit). Everything passes (normal and --long).
I made a few comments in the changes, but mostly it seems good.
Attribute __custom_name seems to be used mostly in *.py files so it was already class private. Maybe that one is not intended to be inherited, but I didn't check the uses in *.pyx files (which are the ones where cython 0.29.x makes inherited and cython 3.0 makes not inherited). I wonder if that's the reason why some had to be renamed from _custom_name to _name, because making them inheritable breaks something?
I'm yet to try cython 3.0 itself, but I'll report about that in #36110. It'd be important to get this PR in as soon as possible in the 10.2 beta cycle so it gets time to be sorted out in case of any trouble. The actual switch to cython 3.0 is then quite small I think.
src/sage/combinat/cluster_complex.py
Outdated
| self.__custom_name = 'Cluster complex' | ||
| self._name = 'Cluster complex' |
There was a problem hiding this comment.
Why is this change necessary?
There was a problem hiding this comment.
The story is a bit complicated. First of all, __custom_name was used for object renaming. In src/sage/structure/sage_object.pyx, SageObject.rename:
The reason C-extension types are not supported by default
is if they were then every single one would have to carry
around an extra attribute, which would be slower and waste
a lot of memory.
To support them for a specific class, add a
``cdef public __custom_name`` attribute.
This attribute is used for both Cython objects and plain Python objects, so it is necessary to perform the __custom_name -> _custom_name change in both pyx and py sources.
The case in this file is different. This self.__custom_name is unrelated to the one for object renaming, as it is in plain Python code and the attribute name is mangled, while the object renaming code is in Cython and expects unmangled names. However, it was changed in the __custom_name -> _custom_name rename as a result of simple find and replace, and after the change it collides with object renaming, so I changed it to a different name.
Technically this change is not necessary, but I find it clearer to use a different name from the object renaming case here.
There was a problem hiding this comment.
I see, but IMO this should be a private attribute. As a matter of fact, I would just get rid of it and move this code down to the _repr_() method as I suggest below (it is the only place using this...)
src/sage/cpython/cython_metaclass.h
Outdated
| // and resets just after the call. We need to do the same, or it may | ||
| // end up with illegal memory accesses. |
There was a problem hiding this comment.
Here Py_TPFLAGS_HEAPTYPE is reset after calling PyTypeReady, but it's not set before. Nothing seems to break, is the reset really necessary? Why is setting it before not needed?
Do you have an example where not having this produces an illegal memory access that we can add as a test?
There was a problem hiding this comment.
This Sage_PyType_Ready function is supposed to be used in place of PyType_Ready using macros:
#define PyType_Ready(t) Sage_PyType_Ready(t)
As a result, when Cython calls PyType_Ready in their code, it is actually calling Sage_PyType_Ready here. As seen in the Cython issue, Py_TPFLAGS_HEAPTYPE should only be set for the duration of PyType_Ready, and all the following code in Sage_PyType_Ready should not run with it set.
Not having this results in segfaults for the first example in src/sage/cpython/cython_metaclass.pyx. In particular, the print line tries to print the string representation of MyCustomType, which have illegal memory accesses.
sage: cython( # optional - sage.misc.cython
....: '''
....: cimport cython
....: cimport sage.cpython.cython_metaclass
....: cdef class MyCustomType():
....: @cython.always_allow_keywords(False)
....: def __getmetaclass__(_):
....: class MyMetaclass(type):
....: def __init__(*args):
....: print("Calling MyMetaclass.__init__{}".format(args))
....: return MyMetaclass
....:
....: cdef class MyDerivedType(MyCustomType):
....: pass
....: ''')
Unfortunately, I think it would be unconvenient to add a test without copying the entire header file.
There was a problem hiding this comment.
I see. Maybe rephrase to something like "We need to reset it earlier, since otherwise it may end up with illegal memory accesses."
Note that in this comment you said Py_TPFLAGS_HEAPSIZE but you want to say Py_TPFLAGS_HEAPTYPE (I think).
| unused argument. This method should return a type to be used as | ||
| unused argument, and turn off the Cython directive | ||
| ``always_allow_keywords``. This method should return a type to be used as |
There was a problem hiding this comment.
Is this a bugfix or a performance hack? If the former, what happens if always_allow_keywords(False) is not used?
For the record, this commit (55b088a) is only changing documentation and doctests, the actual change happens in src/sage/structure/element.pyx in commit 7e011a7, which is the only place where this metaclass magic is used in sagemath.
There was a problem hiding this comment.
The code currently requires a METH_NOARGS method (in src/sage/cpython/cython_metaclass.h), and this is the way make one in Cython 3. Not having this results in the error PyMethodDescr_CallSelf requires a method without arguments. I think the code could be changed to handle other types of methods, but I did not look deeper into that.
Relevant Cython changelog item:
No/single argument functions now accept keyword arguments by default in order to comply with Python semantics. The marginally faster calling conventions METH_NOARGS and METH_O that reject keyword arguments are still available with the directive @cython.always_allow_keywords(False). (Github issue #3090)
| #undef T_CHAR | ||
| #undef T_BOOL | ||
| """ | ||
| pass |
There was a problem hiding this comment.
Ugly hack, but the comment is very clear.
It seems stupid that this is added to cython 3 since structmember.h has this comment
* This header is deprecated: new code should not use stuff from here.
* New definitions are in descrobject.h.
To be fair, I couldn't find anything in cython source code that would make structmember.h to be included. Is it included indirectly?
There was a problem hiding this comment.
The relevant Cython code is here. From my tests, defining basically any functions or extension classes in a pyx file causes this header to be included.
There was a problem hiding this comment.
So there's no way to make cython include descrobject.h instead as suggested... 😞
src/sage/misc/fpickle.pyx
Outdated
| # Note: On Python 3 there is no .im_class but we can get the instance's | ||
| # class through .__self__.__class__ | ||
| cls = getattr(method, 'im_class', method.__self__.__class__) |
There was a problem hiding this comment.
| # Note: On Python 3 there is no .im_class but we can get the instance's | |
| # class through .__self__.__class__ | |
| cls = getattr(method, 'im_class', method.__self__.__class__) | |
| cls = method.__self__.__class__ |
We don't support python 2 anymore, right?
There was a problem hiding this comment.
To be honest, I do not quite understand the need for this function at all as Python seems to do well with method pickling, so I tried to keep the changes minimal. I think the Python 2 bits can be safely removed, though.
| try: | ||
| value = self.callable(len(self.cache)) | ||
| except StopIteration: | ||
| return 1 | ||
| self.cache.append(value) |
There was a problem hiding this comment.
Can we add an example showing this fails to work without / works with?
This is what I could come up with:
sage: from sage.misc.lazy_list import lazy_list
sage: def f(x):
....: if x<5:
....: return x
....: else:
....: raise StopIteration
....:
sage: L = lazy_list(f)
sage: L._update_cache_up_to(10)
1
sage: L._info()
cache length 5
start 0
stop 9223372036854775807
step 1
but it's not clear to me this is / should be supported (using a function that raises StopIteration). Where is this triggered? Is it triggered just with cython 3?
Related issue (not solved by the change above, and not related to cython 3):
sage: L = lazy_list([1,2,3])
sage: L._update_cache_up_to(10)
---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
Cell In[26], line 1
----> 1 L._update_cache_up_to(Integer(10))
File ~/src/sage/sage-git/develop/pkgs/sagemath-standard/build/lib.linux-x86_64-cpython-311/sage/misc/lazy_list.pyx:844, in sage.misc.lazy_list.lazy_list_generic._update_cache_up_to (build/cythonized/sage/misc/lazy_list.c:5944)()
842 return l
843
--> 844 cpdef int _update_cache_up_to(self, Py_ssize_t i) except -1:
845 r"""
846 Update the cache up to ``i``.
File ~/src/sage/sage-git/develop/pkgs/sagemath-standard/build/lib.linux-x86_64-cpython-311/sage/misc/lazy_list.pyx:876, in sage.misc.lazy_list.lazy_list_generic._update_cache_up_to (build/cythonized/sage/misc/lazy_list.c:5809)()
874 cdef list l
875 while len(self.cache) <= i:
--> 876 l = self._new_slice()
877 if not l:
878 return 1
AttributeError: 'sage.misc.lazy_list.lazy_list_generic' object has no attribute '_new_slice'
There was a problem hiding this comment.
I am not sure why, but it seems to be intended as seen in the doctests of lazy_list_from_function.__init__:
sage: def f(n):
....: if n >= 5: raise StopIteration
....: return 5 - n
sage: list(lazy_list_from_function(f))
[5, 4, 3, 2, 1]
Before Cython 3 it works because StopIteration in generators (lazy_list_generic.__iter__ here) simply stops the iteration, while in Cython 3 it becomes RuntimeError.
Relevant Cython changelog item:
PEP-479 (generator_stop) is now enabled by default with language level 3. (Github issue #2580)
| rhs = next(term_iter) | ||
| try: | ||
| rhs = next(term_iter) | ||
| except StopIteration: | ||
| return |
There was a problem hiding this comment.
Why this works ok with 0.29.x but not with 3.0?
There was a problem hiding this comment.
The same as above, StopIteration in generators becomes RuntimeError in Cython 3.
| def __rtruediv__(self, left): | ||
| r""" | ||
| Return the quotient of left with ``self``, that is: | ||
|
|
There was a problem hiding this comment.
Note __rtruediv__ is called explicitly in RealIntervalFieldElement.__truediv__ (src/sage/rings/real_mpfi.pyx) and in RealNumber.__truediv__ (src/sage/rings/real_mpfr.pyx).
I really fail to understand the logic behind those methods and why they sometimes use __rtruediv__; those methods are also deprecated (cf #15114) so maybe this doesn't matter at all.
Moreover, the examples in the doctests of the removed methods still work fine, so these methods seem really unnecessary.
There was a problem hiding this comment.
I think this is what happened. Only __truediv__ is special for Cython code (with c_api_binop_methods=True, which is the case both before Cython 3 and for our settings in Cython 3), and both x.__truediv__ and x.__rtruediv__ are available because they are wrappers of the actual division code. Here, __rtruediv__ is simply a normal method that shadows the name of the special wrapper, so x.__rtruediv__ is still available after removing this code.
RealIntervalFieldElement and RealNumber only call __rtruediv__ on Element and RealIntervalFieldElement, so they are irrelevant here.
|
As a general comment let me say that your answers show that you have put a lot of thought into this, and everything looks great. I'll be making some particular comments as time permits, but please don't take them too strongly; they are mostly suggestions. I rather get this in as-is than get into bike-shed territory. |
| @@ -36,7 +36,7 @@ class HopfAlgebrasWithBasis(CategoryWithAxiom_over_base_ring): | |||
| sage: A = C.example(); A # optional - sage.groups | |||
| An example of Hopf algebra with basis: the group algebra of the | |||
| Dihedral group of order 6 as a permutation group over Rational Field | |||
| sage: A.__custom_name = "A" # optional - sage.groups | |||
| sage: A._custom_name = "A" # optional - sage.groups | |||
There was a problem hiding this comment.
| sage: A._custom_name = "A" # optional - sage.groups | |
| sage: A.rename("A") # optional - sage.groups |
| sage: X = CombinatorialFreeModule(QQ, [1,2]); X._custom_name = "X" # optional - sage.modules | ||
| sage: Y = CombinatorialFreeModule(QQ, [3,4]); Y._custom_name = "Y" # optional - sage.modules |
There was a problem hiding this comment.
| sage: X = CombinatorialFreeModule(QQ, [1,2]); X._custom_name = "X" # optional - sage.modules | |
| sage: Y = CombinatorialFreeModule(QQ, [3,4]); Y._custom_name = "Y" # optional - sage.modules | |
| sage: X = CombinatorialFreeModule(QQ, [1,2]); X.rename("X") # optional - sage.modules | |
| sage: Y = CombinatorialFreeModule(QQ, [3,4]); Y.rename("Y") # optional - sage.modules |
src/sage/combinat/cluster_complex.py
Outdated
| self.__custom_name = 'Cluster complex' | ||
| self._name = 'Cluster complex' |
There was a problem hiding this comment.
I see, but IMO this should be a private attribute. As a matter of fact, I would just get rid of it and move this code down to the _repr_() method as I suggest below (it is the only place using this...)
src/sage/combinat/cluster_complex.py
Outdated
| @@ -271,7 +271,7 @@ def _repr_(self): | |||
| sage: ClusterComplex(['A', 2])._repr_() | |||
| "Cluster complex of type ['A', 2] with 5 vertices and 5 facets" | |||
| """ | |||
| name = self.__custom_name | |||
| name = self._name | |||
There was a problem hiding this comment.
| name = self._name | |
| if k == 1: | |
| name = 'Cluster complex' | |
| else: | |
| name = 'Multi-cluster complex' |
src/sage/combinat/subword_complex.py
Outdated
| @@ -1131,7 +1131,7 @@ def __init__(self, Q, w, algorithm="inductive"): | |||
| SimplicialComplex.__init__(self, maximal_faces=Fs, | |||
| maximality_check=False, | |||
| category=cat) | |||
| self.__custom_name = 'Subword complex' | |||
| self._name = 'Subword complex' | |||
There was a problem hiding this comment.
| self._name = 'Subword complex' |
Remove as this is unused.
| cdef int _nb_slackvars | ||
| cdef object __monoid | ||
| cdef public object __custom_name | ||
| cdef public object _custom_name |
There was a problem hiding this comment.
Doesn't seem to be used, is it?
| @@ -22,7 +22,7 @@ cdef class PermutationGroupElement(MultiplicativeGroupElement): | |||
| cpdef PermutationGroupElement _generate_new_GAP(self, old) | |||
| cpdef _gap_list(self) | |||
| cpdef domain(self) | |||
| cdef public __custom_name | |||
| cdef public _custom_name | |||
There was a problem hiding this comment.
In permgroup_element.pyx it is documented that Permutation group elements support renaming them so this one is indeed used.
src/sage/interfaces/interface.py
Outdated
| if self._name in s: | ||
| try: | ||
| s = s.replace(self._name, getattr(self, '__custom_name')) | ||
| s = s.replace(self._name, getattr(self, '_custom_name')) | ||
| except AttributeError: | ||
| pass |
There was a problem hiding this comment.
I've no clue why this is needed or what for...
src/sage/structure/sage_object.pyx
Outdated
| To support them for a specific class, add a | ||
| ``cdef public __custom_name`` attribute. | ||
| ``cdef public _custom_name`` attribute. | ||
| """ |
There was a problem hiding this comment.
I give up. I was going to say that __custom_name indeed looks like it should be private to class SageObject, so it should be kept double underscore. However, this encapsulation has been perforated all over the place (cython fault since it didn't mangle).
My preference would be to rename __custom_name to _SageObject__custom_name, that is explicitly mangling (so it works the same in cython 0.29 and cython 3, with a note to change back to __custom_name once cython <3 support is dropped.
Then every use of this private attribute in a different class would be explicit. The only ok usage would be to add cdef public _SageObject__custom_name attribute to support renaming of C-extension types. Everything else should be deprecated IMO (I suggested a few places where it is a simple matter of using rename(..).
But of course, all of this is silly, because you should not be responsible of all this technical debt. Whatever, maybe the easier way is really to rename everything to _SageObject__custom_name and let the future fix this in due time.
|
I think this is really good. If you want to do something about I fear of looking in detail into the other attributes to figure out if there are others that should really be private to the class instead of shared with subclasses. |
|
I added |
Great! Everything looks good. I will test it later today and if everything works approve it. Then I'll move to #36110 which looks much simpler (at least from the POV of the sagelib). We want to push this into void linux soon (void-linux/void-packages#45086). |
src/sage/combinat/cluster_complex.py
Outdated
| @@ -271,7 +267,10 @@ def _repr_(self): | |||
| sage: ClusterComplex(['A', 2])._repr_() | |||
| "Cluster complex of type ['A', 2] with 5 vertices and 5 facets" | |||
| """ | |||
| name = self.__custom_name | |||
| if k == 1: | |||
There was a problem hiding this comment.
| if k == 1: | |
| if self._k == 1: |
| sage: P.rename('A polynomial ring') | ||
| sage: P.get_custom_name() | ||
| 'A polynomial ring' |
There was a problem hiding this comment.
| sage: P.rename('A polynomial ring') | |
| sage: P.get_custom_name() | |
| 'A polynomial ring' | |
| sage: P.rename('A polynomial ring') | |
| sage: P.get_custom_name() | |
| 'A polynomial ring' | |
| sage: P.reset_name() | |
| sage: P.get_custom_name() is None | |
| True |
|
I think with the two fixes I suggested above, this is ready. Let's make sure that CI passes, then I can give positive review. I think there is a conflict with current develop, so you'll have to merge or rebase to 10.2.beta0. Let's try to get this merged aSAP so we can move forward with cython 3.0. |
infmagic2047
force-pushed
the
prepare-cython-3
branch
from
e2537e6
to
4b85613
Compare
|
I just fixed the test failures and rebased on current develop. |
|
Documentation preview for this PR (built with commit 4b85613; changes) is ready! 🎉 |
@tobiasdiez is it possible to stop spamming the PRs (and our mailboxes) with this useless (non-)warning ? |
|
@vbraun we are getting which AFAICS is unrelated. Also: and Can you please fix develop so CI works? Isn't it a good idea to run (pre-)releases through CI to make sure CI passes? |
for the linter, please review #36148 |
@tornaria Perhaps a PR to https://github.com/sagemath/sage-release-management/blob/main/git_sage/cmdline/merge_command.py, to run |
No, that's just the release management running behind CI. I mean pushing the release branch to gh and let it run CI exactly as it will do for everybody when it's released. I mean making a rule that CI is (almost) never broken, and that PRs that don't pass CI are not merged except in exceptional circumstances. I mean that I only get a failing CI when it's (almost certainly) my fault, so I know looking into the details will give me some insight. As things are, looking into CI is a waste of time since CI is broken on a noop PR more often than not. |
|
#35062 fixes this (using the relatively new github merge queue feature). It would prevent that PRs with failing ci are merged, and that even merging multiple PRs at the same time don't result in failing ci for the main branch... Concerning the documentation comment, do you have a proposal on how to make the compiled documentation easily accessible in another way? |
Sounds great!
Just don't make it accessible, what's the use? I guess for at least 90-95% of the PR there is no need (I'm sure for 100% of my PR this is useless and it spams me and my PR). If it's a feature some PRs may find useful, maybe have a magic keyword one can add to the PR so that this is run only when the keyword is present. |
FWIW, I find it useful and look at the generated documentation all the time. |
I'm sure its useful for PRs that affect documentation. But most PRs are not about documentation, I think... That's why I suggested an opt-in switch for this. |
Many PRs involve docstring changes, and we want to make it as easy as possible for people to check the formatted documentation. |
Documentation previews are extremely useful as most PRs touch docstrings, which should be checked in the documentation preview. I try to check it as often as I can for reviewing PRs. On the other hand, I am curious why you describe it as "spamming". There is always one such comment (of one line) for a PR. Why do you mention mailboxes? Is the the comment notified to you whenever it is created, so you find many of them in your mailbox? |
For the PRs on which I participate, or where I'm tagged, updates are sent to my mail. It's completely annoying to receive (often several hours after I pushed to a branch) an email which only has a completely useless comment. This is only making it as easy as possible for me to finally pull the trigger and disable email notifications for sagemath/sage, after which tagging me or commenting on my PRs will be good for nothing. |
|
@tornaria Just filter out these messages in your email system. That would be more productive. |
Sure, whatever... It's still annoying. Could the bot edit the PR description instead? I don't think edits send email notifications. That might even be more useful as the link for the documentation would appear in the top of the PR, and even perhaps other useful info... Anyway, I'll add |
The email notifications are a feature. PR authors don't have to build the documentation themselves. When it has been built, one receives a friendly email with the useful links to the built documentation. This enables an asynchronous workflow that is more productive than waiting for the documentation to build locally, which often takes too long, occupying a worktree in which one cannot work on a different PR at this time. |
|
I will investigate why i am not receiving such email notifications for built doc. I am getting notified of other comments posted to PRs i am involved though. |
|
Ah, my Gmail has the filter that deletes all mails from |
So you acknowledge that it's annoying that the bot spams our mailboxes... 🤔 |
<!-- ^^^^^ 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 --> This PR builds on sagemath#36109 and contains the rest of changes needed to update Cython to 3.0.0. The actual changes needed for building Sage are a632238 for the code, 67ad2e4 for the build system, and 7195b8f for the doctests. The rest of commits are either from sagemath#36109 or for patching dependencies. Closes sagemath#29863. Ideally we should wait until numpy and scipy are upgraded to 1.25 and 1.11 respectively before merging this, so we can drop the patches for them. <!-- 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. --> ### 📝 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: ... --> - Depends on sagemath#36109 - Depends on sagemath#36112 (merged here) <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36110 Reported by: Yutao Yuan Reviewer(s): Gonzalo Tornaría, Kwankyu Lee, Matthias Köppe, Tobias Diez
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 #29863.
📝 Checklist
⌛ Dependencies