Support python 3.12 on sagemath-standard #36407
Conversation
|
I was thinking about what was needed to enable 3.12 so it is timely. It is mostly doctest fix apart from that stuff with pylong. Can you expand on that stuff? It mentions some headers from python 3.12, naively I would have thought that was stuff removed from 3.12 that was still needed. If it is supposed to be internal, why do we need to use it? |
As I mentioned in the commit message, the layout for python integers changed in python 3.12. This affects cypari2 and fpylll as well as sagemath, since all three projects implement functions to convert from/to python integers. Since the change is forward and backward incompatible, I created a module I suggest looking at the raw files: in In The only function not taken from python is As for the low level details: the main difference (besides moving the |
The layout for python integers changed in python 3.12. We add a module `sage.cpython.pycore_long` which copies the new (internal) api of PyLong from python 3.12. We also implement fallback version of these functions suitable for python pre-3.12. Note the files implementing the `pycore_long` module (`pycore_long.pxd` and `pycore_long.h`) are shared with fpylll and cypari2.
In python 3.12, the `struct atexit_callback` was renamed to `struct atexit_py_callback`. The easiest workaround is to add `#define atexit_callback atexit_py_callback` in the right place when building using python 3.12 or newer.
Tracing has changed in python 3.12 in such a way that cython doesn't support it properly anymore. This one file sets `profile=true` for cython which won't work anymore (and it fails to build, at least with cython 0.29). We just disable that line. See also: cython/cython#5450
To use some (internal) declarations related to dict type, we have to include `<internal/pycore_dict.h>` which needs `#define Py_BUILD_CORE` to be loaded. This causes trouble when `Python.h` was included before defining `Py_BUILD_CORE`, due to a macro `_PyGC_FINALIZED`. We fix it by undefining said macro.
Some changes in ast, the old `node.n` and `node.s` are deprecated in favour of a common `node.value`. Making this change seems better than just ignoring the deprecation warning.
This adds some filterwarnings that trigger with python 3.12. - deprecation of `datetime.datetime.utcfromtimestamp()` this is triggered by python modules `dateutil` and `sphinx`. - `os.fork()` and `os.ptyfork()` are deprecated when running multi-threaded; I don't see an easy way out of this, so ignore it. - itertools won't support pickling in python 3.14; let's ignore this for now with the hope there's an alternative before 3.14.
Is deprecated, and it can be replaced just fine with `importlib.util.find_spec()`.
Since `importer.find_module(...)` was removed in 3.12. We just follow the suggestion from https://docs.python.org/3/library/importlib.html#importing-a-source-file-directly Note that the last three added lines here could be replaced instead by spec.loader.load_module(module_name) which works; however this is deprecated so it's better to use the recommended way using `importlib.util.module_from_spec(...)` and `spec.loader.execute_module(...)`.
In python < 3.12 we have
sage: a = delta_qexp(1000)
sage: sum(a[n]/float(n)^14 for n in range(1,1000))
0.9985830631627459
This changed in python 3.12 to
sage: sum(a[n]/float(n)^14 for n in range(1,1000))
0.9985830631627461
The latter is the correct one as can be seen using rationals:
sage: float(sum(a[n]/n^14 for n in range(1,1000)))
0.9985830631627461
As a workaround, we do the sum in reverse (from small to large terms),
which gives the correct result in any case:
sage: sum(a[n]/float(n)^14 for n in reversed(range(1,1000)))
0.9985830631627461
In python 3.12 the printing of OrderedDict has been changed. As of Python 3.7, regular dicts are guaranteed to be ordered, so it's safe to replace OrderedDict by dict. Maybe convenient to replace other uses of OrderedDict, although this is out of scope of python 3.12 support.
Running
sage: g = Polyhedron().face_generator()
sage: g.__next__()
A -1-dimensional face of a Polyhedron in ZZ^0
sage: g.__next__()
is supposed to raise `StopIteration`. However in python 3.12 the second
call to `__next__()` leads to a crash.
This is caused by a `return -1` in `next_face_loop()` which is supposed
to mean `raise StopIteration`. Using raise explicitly fixes the crash.
|
rebased to beta6 |
|
Documentation preview for this PR (built with commit 3a7cd3f; changes) is ready! 🎉 |
|
FTR: before I was only testing this with cython 0.29 (this PR applied to sagemath 10.1). Now I've tested this with cython 3:
In summary, when actually updating python to 3.12 something has to be done with cython. Everything seems to work ok when using cython 3.0.2 + cython/cython#5725. A further note: this PR is about supporting using python 3.12. Please review from that point of view: the important thing is that this makes sense and it doesn't cause any problem with older python. I'd appreciate if this can be merged for 10.2 even if it might make more sense to wait a bit more to actually update the python in sage-the-distro. We are already shipping python 3.12 in void linux so this is well tested at least in the architectures we support. |
|
I can reproduce the hang in plain Python The |
Can't reproduce this either. Tried with 3.12.3 (current) and 3.12.2. What's your version of python? |
|
The hangs turned out to be a packaging issue (missing mpdecimal library). Still can reproduce the segfault, though. |
|
_Py_Isimmortal, which we see in the backtrace, is new in Python 3.12. |
But |
|
Calling any libgap function in three arguments triggers the crash. |
|
Can you attach your |
|
Can't reproduce this one either: FWIW, we are on gap 4.12.2, cython 3.0.10, cysignals 1.11.4, ... (what else would be relevant?) We don't have mpdecimal. |
same crash place? Probably, to reproduce one needs to know compiler version, cython version, maybe even glibc and pthreads version, cysignals version... |
|
For me gcc is 13.2.0, glibc is 2.39 (and gap 4.12.2, cython 3.0.10, cysignals 1.11.4, ... ) Unrelated, but how do you deal with #37616 if you are on gap 4.13? |
Yes, same backtrace. What baffles me is that the definition of so this part of the backtrace doesn't make any sense to me: |
I ignore the test failures. None of them is a serious issue. In any case, this is also reproducible with 4.12. |
|
Narrowed this down to GCC optimization. Compiling with Compiling with clang works OK with any optimization level. |
|
Is this compiling libgap? or element.pyx ? If this is compiling libgap with different -O values then it's GAP bug. |
It's compiling the previously attached |
This is our cc line for that file: |
// Static inline functions should use _Py_NULL rather than using directly NULL
// to prevent C++ compiler warnings. On C23 and newer and on C++11 and newer,
// _Py_NULL is defined as nullptr.
#if (defined (__STDC_VERSION__) && __STDC_VERSION__ > 201710L) \
|| (defined(__cplusplus) && __cplusplus >= 201103)
# define _Py_NULL nullptr
#else
# define _Py_NULL NULL
#endif
Something to do with Can you insert prints, or otherwise see the actual values of Also, can you add print in the body of the to see what value of Maybe Of course, sometimes print statements cause sudden changes in how the code operates, but let's try anyway... |
|
something like --- a/src/sage/libs/gap/gap_includes.pxd
+++ b/src/sage/libs/gap/gap_includes.pxd
@@ -29,7 +29,7 @@ cdef extern from "gap/calls.h" nogil:
cdef extern from "gap/libgap-api.h" nogil:
"""
- #define sig_GAP_Enter() {int t = GAP_Enter(); if (!t) sig_error();}
+ #define sig_GAP_Enter() {volatile int t = GAP_Enter(); if (!t) sig_error();}
"""
ctypedef void (*GAP_CallbackFunc)()
void GAP_Initialize(int argc, char ** argv, |
|
This is a regression in the GCC stable branch. I downgraded to the previous Arch snapshot (commit 860b0f0ef787f756c0e293671b4c4622dff63a79) and it works fine. I will try to create a minimal test case and report upstream, so far my naive attempts are not working. |
|
Upstream is understandably having trouble diagnosing the issue, with such a complex setup needed to reproduce it, and I am not able to help much further. If someone more knowledgeable could chime in and help make this more digestable for upstream, that would be helpful. |
|
Let's move the discussion to #37026 since this is not directly caused by Python 3.12 (it only makes the issue manifest itself as a crash, but likely still exists on 3.11) |
I am able to reproduce this on Gentoo (with Python 3.12.3, and gcc version 13.2.1 20240210 (Gentoo 13.2.1_p20240210 p14)). In view of https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114872#c12 it's of course a tough task to produce a "small" example (it could be that gcc code generation only breaks down on huge files like element.c) |
Upstream has concluded it's not a gcc bug. It's actually their optimiser getting more efficient, holding more local variables in registers. So on can either go the route of adding |
This appears to fix sagemath#37026 (segfaults in src/sage/libs/gap/element.pyx with Python 3.12 and gcc 13.2.1) See also sagemath#36407 (comment) The corresponding gcc 13.2.1's bug (or feature) is being dealt with here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114872 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [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. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#37951 Reported by: Dima Pasechnik Reviewer(s): Dima Pasechnik, Gonzalo Tornaría
This appears to fix sagemath#37026 (segfaults in src/sage/libs/gap/element.pyx with Python 3.12 and gcc 13.2.1) See also sagemath#36407 (comment) The corresponding gcc 13.2.1's bug (or feature) is being dealt with here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114872 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [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. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#37951 Reported by: Dima Pasechnik Reviewer(s): Dima Pasechnik, Gonzalo Tornaría
This appears to fix sagemath#37026 (segfaults in src/sage/libs/gap/element.pyx with Python 3.12 and gcc 13.2.1) See also sagemath#36407 (comment) The corresponding gcc 13.2.1's bug (or feature) is being dealt with here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114872 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [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. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#37951 Reported by: Dima Pasechnik Reviewer(s): Dima Pasechnik, Gonzalo Tornaría
This appears to fix sagemath#37026 (segfaults in src/sage/libs/gap/element.pyx with Python 3.12 and gcc 13.2.1) See also sagemath#36407 (comment) The corresponding gcc 13.2.1's bug (or feature) is being dealt with here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114872 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [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. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#37951 Reported by: Dima Pasechnik Reviewer(s): Dima Pasechnik, Gonzalo Tornaría
This PR adds support for running with python 3.12 from system. This has been tested on the python 3.12 branch of void linux (x86_64, x86_64-musl and i686).
The first two commits correspond to #36403. The rest is split is small pieces and I tried to add reasonable explanations in the commit messages. Reviewing by commit may be easier (ignoring the first two, already reviewed).
See also: #36181
⌛ Dependencies
#36403