cython 3.0.11 compatibility #38500

kiwifb · 2024-08-12T01:02:18Z

I recently installed cython 3.0.11 in Gentoo and it caused issue building sage. The issues are not present when building with cython 3.0.10. The fix is just to add some noexcept statements in some of the place cython complains about. More details of the issue and some of the error messages encountered at cschwan/sage-on-gentoo#794

📝 Checklist

The title is concise and informative.
The description explains in detail what this PR is about.
I have linked a relevant issue or discussion.
I have created tests covering the changes.
I have updated the documentation and checked the documentation preview.

github-actions · 2024-08-12T01:45:57Z

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

mkoeppe · 2024-08-12T03:13:43Z

Confirmed the problem with the upgrade PR #38501, https://github.com/sagemath/sage/actions/runs/10345037926/job/28631469828?pr=38501#step:9:12644

mkoeppe · 2024-08-12T04:13:05Z

Fix confirmed using #38501

kiwifb · 2024-08-12T04:17:20Z

It is rather weird that only those cause problems out of the list of warnings I get. And there is a very clear cython 3.0.10 to 3.0.11 boundary as well. Something like that happening with say cython 3.1.0 would not have been so weird.

mkoeppe · 2024-08-12T04:39:51Z

see also #37885 (comment)

tornaria · 2024-08-13T00:56:17Z

It is rather weird that only those cause problems out of the list of warnings I get. And there is a very clear cython 3.0.10 to 3.0.11 boundary as well. Something like that happening with say cython 3.1.0 would not have been so weird.

It's a regression, see: cython/cython#6335

If this PR gets rid of every "implicit noexcept" warning, perhaps legacy_implicit_noexcept can be switched off?

kiwifb · 2024-08-13T01:54:13Z

If this PR gets rid of every "implicit noexcept" warning, perhaps legacy_implicit_noexcept can be switched off?

It does not, but I would consider finishing the job if there is not too much more to do.

kiwifb · 2024-08-13T01:55:15Z

Actually, I should properly check what's left, maybe it will be a nice surprise.

kiwifb · 2024-08-13T02:04:18Z

The whole of the cypari2 folder still need to be done. And there are those two bits that may need to be dealt with

warning: sage/categories/morphism.pyx:801:26: noexcept clause is ignored for function returning Python object
warning: sage/categories/morphism.pyx:826:22: noexcept clause is ignored for function returning Python object

kiwifb · 2024-08-13T02:06:32Z

Once you remove duplicate warnings, it amounts to 15 lines, not too onerous

warning: /usr/lib/python3.12/site-packages/cypari2/convert.pxd:25:30: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
warning: /usr/lib/python3.12/site-packages/cypari2/convert.pxd:45:34: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
warning: /usr/lib/python3.12/site-packages/cypari2/convert.pxd:60:28: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
warning: /usr/lib/python3.12/site-packages/cypari2/convert.pxd:63:22: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
warning: /usr/lib/python3.12/site-packages/cypari2/convert.pxd:65:30: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
warning: /usr/lib/python3.12/site-packages/cypari2/convert.pxd:68:32: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
warning: /usr/lib/python3.12/site-packages/cypari2/gen.pxd:27:26: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
warning: /usr/lib/python3.12/site-packages/cypari2/pari_instance.pxd:6:29: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
warning: /usr/lib/python3.12/site-packages/cypari2/pari_instance.pxd:7:29: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
warning: /usr/lib/python3.12/site-packages/cypari2/pari_instance.pxd:8:26: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
warning: /usr/lib/python3.12/site-packages/cypari2/paridecl.pxd:5330:37: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
warning: /usr/lib/python3.12/site-packages/cypari2/stack.pxd:11:21: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
warning: /usr/lib/python3.12/site-packages/cypari2/stack.pxd:12:20: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
warning: /usr/lib/python3.12/site-packages/cypari2/stack.pxd:14:32: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
warning: /usr/lib/python3.12/site-packages/cypari2/stack.pxd:19:22: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.

kiwifb · 2024-08-13T02:11:29Z

Actually this is not inside sage but the installed copy of cypari2. Does that prevent us from removing legacy_implicit_noexcept? Do we need a new release of cypari2 to get rid of it?

mkoeppe · 2024-08-13T02:16:21Z

For cypari2 see #38183

kiwifb · 2024-08-13T02:35:50Z

I'd rather have proper release rather than rc, but that could be OK so long as we have final releases in time for 10.5.

mkoeppe · 2024-08-13T02:36:38Z

I held off with cutting the releases when I ran into #38183 (comment)

gmou3

I had noticed warnings about these files during building, which specifically mentioned a future lack of support of implicit noexcepts.

Most such removals happened in #37667. After that, #38057 added many function return types. This combination for C return types (int, bint, void) seems to yield an issue.

mkoeppe · 2024-08-13T19:10:44Z

I'd rather have proper release rather than rc

https://pypi.org/project/cypari2/2.2.0/

tornaria · 2024-08-13T21:03:29Z

Once you remove duplicate warnings, it amounts to 15 lines, not too onerous

warning: /usr/lib/python3.12/site-packages/cypari2/convert.pxd:25:30: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
...

Afaict, this was already fixed on version 2.1.5 of cypari2 (march 2024).

kiwifb · 2024-08-13T21:20:39Z

Once you remove duplicate warnings, it amounts to 15 lines, not too onerous
warning: /usr/lib/python3.12/site-packages/cypari2/convert.pxd:25:30: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
...
Afaict, this was already fixed on version 2.1.5 of cypari2 (march 2024).

Right, I am still on 2.1.4. I have let things slip a bit since some packages have been moved to the main gentoo tree. Time to put in some PR for Gentoo.

tornaria · 2024-08-13T21:21:02Z

I had noticed warnings about these files during building, which specifically mentioned a future lack of support of implicit noexcepts.

Most such removals happened in #37667. After that, #38057 added many function return types. This combination for C return types (int, bint, void) seems to yield an issue.

But see, you have a problem here. As an example see https://github.com/sagemath/sage/blob/develop/src/sage/matroids/matroid.pyx#L495:

...
    cpdef int _rank(self, frozenset X):
        r"""
        Return the rank of a set ``X``.

        This method does no checking on ``X``, and ``X`` may be assumed to
        have the same interface as ``frozenset``.

        INPUT:

        - ``X`` -- an object with Python's ``frozenset`` interface

        OUTPUT: integer

        .. NOTE::

            Subclasses should implement this method.

        EXAMPLES::

            sage: M = sage.matroids.matroid.Matroid()
            sage: M._rank(frozenset([0, 1, 2]))
            NotImplementedError: subclasses need to implement this
            ...
        """
        raise NotImplementedError("subclasses need to implement this")
...

Since this function is indeed raising an exception, I don't think it should be noexcept (neither implicit nor explicit).

Functions that return python types are except NULL or something like that (because a python type is a pointer so NULL is always there to flag an exception). When you switch the return type to a C type, this is no longer the case. You can tell cython what is a good value to use to flag an exception (e.g. rank is >= 0 so -1 seems a good choice).

gmou3

Got it, thanks @tornaria!

gmou3 · 2024-08-13T21:56:46Z

src/sage/matroids/matroid.pxd


    # internal methods, assuming verified input
    cpdef frozenset _max_independent(self, frozenset X)
    cpdef frozenset _circuit(self, frozenset X)
    cpdef frozenset _fundamental_circuit(self, frozenset B, e)
    cpdef frozenset _closure(self, frozenset X)
-    cpdef int _corank(self, frozenset X)
+    cpdef int _corank(self, frozenset X) noexcept


Suggested change

cpdef int _corank(self, frozenset X) noexcept

cpdef int _corank(self, frozenset X) except -1

Did you forget that one in your PR?

This function does not raise an error, so I think better leave it as is.

gmou3 · 2024-08-13T21:57:13Z

src/sage/matroids/matroid.pyx

@@ -492,7 +492,7 @@ cdef class Matroid(SageObject):
        """
        raise NotImplementedError("subclasses need to implement this")

-    cpdef int _rank(self, frozenset X):
+    cpdef int _rank(self, frozenset X) noexcept:


Suggested change

cpdef int _rank(self, frozenset X) noexcept:

cpdef int _rank(self, frozenset X) except -1:

gmou3 · 2024-08-13T21:57:40Z

src/sage/sets/disjoint_set.pxd

@@ -19,8 +19,8 @@ cdef class DisjointSet_class(SageObject):
    cpdef number_of_subsets(self)

 cdef class DisjointSet_of_integers(DisjointSet_class):
-    cpdef int find(self, int i)
-    cpdef void union(self, int i, int j)
+    cpdef int find(self, int i) noexcept


Suggested change

cpdef int find(self, int i) noexcept

cpdef int find(self, int i) except -1

gmou3 · 2024-08-13T21:57:53Z

src/sage/sets/disjoint_set.pxd

-    cpdef int find(self, int i)
-    cpdef void union(self, int i, int j)
+    cpdef int find(self, int i) noexcept
+    cpdef void union(self, int i, int j) noexcept


Suggested change

cpdef void union(self, int i, int j) noexcept

cpdef void union(self, int i, int j) except *

gmou3 · 2024-08-13T21:58:18Z

src/sage/sets/disjoint_set.pyx

@@ -448,7 +448,7 @@ cdef class DisjointSet_of_integers(DisjointSet_class):
        for i, parent in enumerate(l):
            self.union(parent, i)

-    cpdef int find(self, int i):
+    cpdef int find(self, int i) noexcept:


Suggested change

cpdef int find(self, int i) noexcept:

cpdef int find(self, int i) except -1:

gmou3 · 2024-08-13T21:58:38Z

src/sage/sets/disjoint_set.pyx

@@ -492,7 +492,7 @@ cdef class DisjointSet_of_integers(DisjointSet_class):
            raise ValueError('i must be between 0 and %s (%s given)' % (card - 1, i))
        return OP_find(self._nodes, i)

-    cpdef void union(self, int i, int j):
+    cpdef void union(self, int i, int j) noexcept:


Suggested change

cpdef void union(self, int i, int j) noexcept:

cpdef void union(self, int i, int j) except *:

kiwifb · 2024-08-14T04:40:26Z

@gmou3 are those all the changes we can do to follow @tornaria's suggestion? If so, I will accept all of them and we can put things back into positive review. For now I am switching back to need review.

gmou3 · 2024-08-14T10:12:11Z

Yes, these are all (for the files in question). However, in order to make things build, the signatures of the heirs of the _rank function must be consistent. A few doctest adaptations are also required.

@kiwifb I opened a PR to your branch that settles everything.

For C-return-type functions that raise exceptions (and their heirs).

kiwifb · 2024-08-14T10:16:46Z

Yes, these are all (for the files in question). However, in order to make things build, the signatures of the heirs of the _rank function must be consistent. A few doctest adaptations are also required.

@kiwifb I opened a PR to your branch that settles everything.

Thank you for that.

gmou3 · 2024-08-14T11:41:50Z

After adding except -1 to _rank and playing around with -1 rank values you get some Exception ignored messages:

sage: def r(X):
....:     return -1
sage: M = Matroid(groundset=[0,1,2], rank_function=r)
sage: M.is_valid()
Exception ignored in: 'sage.matroids.matroid.Matroid.is_valid'
False
sage: M._rank(frozenset())
-1
sage: M._corank(frozenset())
Exception ignored in: 'sage.matroids.matroid.Matroid._corank'
0

I can personally ignore such ignorations. I am mostly insecure about whether these changes slow down _rank, which is a core function that is often called ad nauseam. (Some basic testing doesn't show a difference.)

mkoeppe · 2024-08-14T22:45:43Z

Test with the update PR #38500 looks OK.

mkoeppe

LGTM

tornaria · 2024-08-15T01:06:04Z

After adding except -1 to _rank and playing around with -1 rank values you get some Exception ignored messages:
sage: def r(X):
....:     return -1
sage: M = Matroid(groundset=[0,1,2], rank_function=r)
sage: M.is_valid()
Exception ignored in: 'sage.matroids.matroid.Matroid.is_valid'
False
sage: M._rank(frozenset())
-1
sage: M._corank(frozenset())
Exception ignored in: 'sage.matroids.matroid.Matroid._corank'
0
I can personally ignore such ignorations. I am mostly insecure about whether these changes slow down _rank, which is a core function that is often called ad nauseam. (Some basic testing doesn't show a difference.)

When you tag a function as except -1 you are making a promise to cython that your function will never return -1 on its own, so the value is used to indicate an exception was raised. The caller will assume an exception has been raised without checking it first (to save time).

If you want cython to use -1 to indicate an exception but your function might return -1, you should tag the function as except? -1. In this case, when the caller sees a -1 return value it will check first before assuming an exception has happened.

So, the warning you see means: the function returned -1 and so cython assumed there an exception was raised, but in fact there was no exception to be found anywhere.

I suggest reading http://docs.cython.org/en/latest/src/userguide/language_basics.html#error-return-values (click on "cython" to see the cython syntax which is used in sagemath .pyx files)

If you want to support the possibility of having an implementation of these methods to return -1 then I think except? -1 seems the way to go. This should not slowdown anything except when the function indeed returns -1 (rare) or when the function raises an exception (speed not really critical). IOW, I don't think there's any real harm in adding the ? just in case.

gmou3 · 2024-08-15T05:41:59Z

@tornaria Thanks for the reference. Exactly what I needed.

@kiwifb After merging the question marks we're good to go.

gmou3 · 2024-08-19T08:54:15Z

did you maybe miss my PR Mr. 🥝? kiwifb#2

* Add some `except`s For C-return-type functions that raise exceptions (and their heirs). * Use `except?` for `_rank`

kiwifb · 2024-08-19T09:15:22Z

did you maybe miss my PR Mr. 🥝? kiwifb#2

I completely missed it. Good job poking me about it.
The fruit "brand" was named after the nickname for new zealander which itself comes from the name of one iconic New Zealand animal https://www.doc.govt.nz/nature/native-animals/birds/birds-a-z/kiwi/

gmou3 · 2024-08-19T09:19:53Z

Cool, didn't know that. Cute animal!

…TRAINT=$SAGE_ROOT/constraints_pkgs.txt`   This is aimed at the intermediate level, for users who wish to build a set of distribution packages as supplied in `pkgs/` but without using the wheelhouse infrastructure of Sage-the-distribution. Documentation included in the files. It can also be tested using new tox environments defined in `pkgs/sagemath-standard/tox.ini`, and is tested on GH Actions: https://github.com/mkoeppe/sage/actions/workflows/ci-linux- incremental.yml?query=branch%3Aconstraints_pkgs - this also provides the missing automatic test for the **sagemath- standard** distribution built out of `pkgs/sagemath-standard/` Marked as critical because it is hoped to ease the adoption of modularized distribution packages by downstream packagers.    ### 📝 Checklist     - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies   - Depends on sagemath#38515 (merged here) - Depends on sagemath#38500 (merged here) URL: sagemath#37434 Reported by: Matthias Köppe Reviewer(s): François Bissey, Kwankyu Lee, Matthias Köppe

I recently installed cython 3.0.11 in Gentoo and it caused issue building sage. The issues are not present when building with cython 3.0.10. The fix is just to add some `noexcept` statements in some of the place cython complains about. More details of the issue and some of the error messages encountered at https://github.com/cschwan/sage-on- gentoo/issues/794 ### 📝 Checklist  - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. URL: sagemath#38500 Reported by: François Bissey Reviewer(s): François Bissey, gmou3, Matthias Köppe

…TRAINT=$SAGE_ROOT/constraints_pkgs.txt`   This is aimed at the intermediate level, for users who wish to build a set of distribution packages as supplied in `pkgs/` but without using the wheelhouse infrastructure of Sage-the-distribution. Documentation included in the files. It can also be tested using new tox environments defined in `pkgs/sagemath-standard/tox.ini`, and is tested on GH Actions: https://github.com/mkoeppe/sage/actions/workflows/ci-linux- incremental.yml?query=branch%3Aconstraints_pkgs - this also provides the missing automatic test for the **sagemath- standard** distribution built out of `pkgs/sagemath-standard/` Marked as critical because it is hoped to ease the adoption of modularized distribution packages by downstream packagers.    ### 📝 Checklist     - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies   - Depends on sagemath#38515 (merged here) - Depends on sagemath#38500 (merged here) URL: sagemath#37434 Reported by: Matthias Köppe Reviewer(s): François Bissey, Kwankyu Lee, Matthias Köppe

Various bug fixes, preparation for Python 3.13 - https://cython.readthedocs.io/en/latest/src/changes.html#id2 ### 📝 Checklist  - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies    - Depends on sagemath#38500 (merged here) URL: sagemath#38501 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee

Add noexcept in matroids and set for cython 3.0.11 compatibility

c4d3b66

github-actions bot added the s: needs review label Aug 12, 2024

kiwifb requested a review from antonio-rojas August 12, 2024 01:04

mkoeppe requested a review from gmou3 August 12, 2024 03:18

mkoeppe added s: positive review and removed s: needs review labels Aug 12, 2024

mkoeppe added the p: blocker / 1 label Aug 12, 2024

gmou3 approved these changes Aug 13, 2024

View reviewed changes

mkoeppe added this to the sage-10.5 milestone Aug 13, 2024

gmou3 suggested changes Aug 13, 2024

View reviewed changes

kiwifb removed the s: positive review label Aug 14, 2024

Add some excepts (#1)

31d6a6b

For C-return-type functions that raise exceptions (and their heirs).

github-actions bot added s: needs review and removed s: needs work labels Aug 14, 2024

mkoeppe approved these changes Aug 14, 2024

View reviewed changes

mkoeppe added s: positive review and removed s: needs review labels Aug 14, 2024

gmou3 self-assigned this Aug 15, 2024

Use except? for _rank (#2)

d943edc

* Add some `except`s For C-return-type functions that raise exceptions (and their heirs). * Use `except?` for `_rank`

github-actions bot added s: needs review and removed s: positive review labels Aug 19, 2024

gmou3 approved these changes Aug 19, 2024

View reviewed changes

gmou3 added s: positive review and removed s: needs review labels Aug 19, 2024

collares mentioned this pull request Sep 2, 2024

Sage: Fix on Darwin NixOS/nixpkgs#264126

Merged

13 tasks

vbraun merged commit 14c0b0c into sagemath:develop Sep 3, 2024
17 of 20 checks passed

github-actions bot removed the s: positive review label Sep 4, 2024

	cpdef int _corank(self, frozenset X) noexcept
	cpdef int _corank(self, frozenset X) except -1

	cpdef int _rank(self, frozenset X) noexcept:
	cpdef int _rank(self, frozenset X) except -1:

	cpdef int find(self, int i) noexcept
	cpdef int find(self, int i) except -1

	cpdef void union(self, int i, int j) noexcept
	cpdef void union(self, int i, int j) except *

	cpdef int find(self, int i) noexcept:
	cpdef int find(self, int i) except -1:

cython 3.0.11 compatibility #38500

cython 3.0.11 compatibility #38500

Conversation

kiwifb commented Aug 12, 2024

📝 Checklist

github-actions bot commented Aug 12, 2024 • edited Loading

mkoeppe commented Aug 12, 2024

mkoeppe commented Aug 12, 2024

kiwifb commented Aug 12, 2024 • edited Loading

mkoeppe commented Aug 12, 2024

tornaria commented Aug 13, 2024

kiwifb commented Aug 13, 2024

kiwifb commented Aug 13, 2024

kiwifb commented Aug 13, 2024

kiwifb commented Aug 13, 2024

kiwifb commented Aug 13, 2024

mkoeppe commented Aug 13, 2024

kiwifb commented Aug 13, 2024

mkoeppe commented Aug 13, 2024

gmou3 left a comment • edited Loading

Choose a reason for hiding this comment

mkoeppe commented Aug 13, 2024

tornaria commented Aug 13, 2024

kiwifb commented Aug 13, 2024

tornaria commented Aug 13, 2024

gmou3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiwifb commented Aug 14, 2024

gmou3 commented Aug 14, 2024

kiwifb commented Aug 14, 2024

gmou3 commented Aug 14, 2024 • edited Loading

mkoeppe commented Aug 14, 2024

mkoeppe left a comment

Choose a reason for hiding this comment

tornaria commented Aug 15, 2024

gmou3 commented Aug 15, 2024

gmou3 commented Aug 19, 2024

kiwifb commented Aug 19, 2024

gmou3 commented Aug 19, 2024

github-actions bot commented Aug 12, 2024 •

edited

Loading

kiwifb commented Aug 12, 2024 •

edited

Loading

gmou3 left a comment •

edited

Loading

gmou3 commented Aug 14, 2024 •

edited

Loading