-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Support Python 3.11a4 _PyErr_StackItem #4672
Conversation
Python 3.11 alpha 4 removed exc_type and exc_traceback fields the the _PyErr_StackItem structure: * python/cpython@396b583 * https://bugs.python.org/issue45711 * #4500
So my belief was that I essentially fixed this in 0.29.x with #4610 (by just turning off a couple of defines). The master branch has a more thorough fix. Looking at the most recent CI run for 0.29.x (https://github.com/cython/cython/runs/5323367319?check_suite_focus=true) there's one tracing test failing, (and a few division tests failing because the exception message has changed) but nothing I can see that suggests serious issues. |
The original PR for the master branch is here: #4584 We disabled the two feature flags because I'd rather try to avoid backporting the whole thing to 0.29.x. It's a rather big change for a legacy release branch. I haven't looked into what's failing in lxml yet. Do you have the compiler output? |
The PR looks ok, though. Since we have it right there, let's just get it merged into 0.29.x. |
All the C++ checks seem to be failing at the moment though...
is a fairly typical example of what's going wrong |
#if PY_VERSION_HEX >= 0x030B00A4 | ||
tb = __Pyx__ExceptionGetTraceback(exc_state->exc_value); | ||
#else | ||
tb = exc_state->exc_traceback; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs a cast in C++
/////////////// _ExceptionGetTraceback.proto /////////////// | ||
|
||
// Used by __Pyx__ExceptionSave() and __Pyx__ExceptionSwap() on Python 3.11a6 | ||
static CYTHON_INLINE PyObject* __Pyx__ExceptionGetTraceback(PyObject *value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this needs to be defined on Python 3.4+ only. On C++ the lack of declaration for PyException_GetTraceback
is causing it to fail
#if PY_VERSION_HEX >= 0x030B00A4 | ||
tb = __Pyx__ExceptionGetTraceback(exc_state->exc_value); | ||
#else | ||
tb = exc_state->exc_traceback; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tb = exc_state->exc_traceback; | |
tb = (PyTracebackObject *) exc_state->exc_traceback; |
PyTracebackObject *tb = (PyTracebackObject *) exc_state->exc_traceback; | ||
PyTracebackObject *tb; | ||
#if PY_VERSION_HEX >= 0x030B00A4 | ||
tb = __Pyx__ExceptionGetTraceback(exc_state->exc_value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tb = __Pyx__ExceptionGetTraceback(exc_state->exc_value); | |
tb = (PyTracebackObject *) __Pyx__ExceptionGetTraceback(exc_state->exc_value); |
/////////////// _ExceptionGetType.proto /////////////// | ||
|
||
static CYTHON_INLINE PyObject* __Pyx__ExceptionGetType(PyObject *value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming convention is to use double underscores for internal variants of "regular" helper functions, e.g. after handling special cases in an inline function. These functions should just have a single underscore as name separator.
#undef CYTHON_FAST_THREAD_STATE | ||
#define CYTHON_FAST_THREAD_STATE 0 | ||
#elif !defined(CYTHON_FAST_THREAD_STATE) | ||
#if !defined(CYTHON_FAST_THREAD_STATE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#if !defined(CYTHON_FAST_THREAD_STATE) | |
#ifndef CYTHON_FAST_THREAD_STATE |
#undef CYTHON_USE_EXC_INFO_STACK | ||
#define CYTHON_USE_EXC_INFO_STACK 0 | ||
#elif !defined(CYTHON_USE_EXC_INFO_STACK) | ||
#if !defined(CYTHON_USE_EXC_INFO_STACK) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#if !defined(CYTHON_USE_EXC_INFO_STACK) | |
#ifndef CYTHON_USE_EXC_INFO_STACK |
I'm aware of this change, it's now possible to build numpy and lxml, but lxml does crash on Python 3.11 :-( It seems like exc_info must be handled on Python 3.11, that's why I wrote that PR. |
I can address all of your remarks, but first: do you prefer that I backport the change from master, or to have a different change (this current PR) in the 0.29.x branch? |
In Python, testing In |
I was thinking that this was based on the change in master, but apparently, it's not. What I would prefer is the minimal change to make this work. I thought that this was disabling the feature flags, but it seems that that was not enough. In any case, having two different sets of changes in master and legacy branch is the worst choice, IMHO, because it increases the chance of independent breakage of either of the two and makes later bug fixing unnecessarily difficult. |
Do we have any details about what's crashing on lxml/what we need to do to reproduce the crash? Is it https://bugzilla.redhat.com/show_bug.cgi?id=2051510 ? The main reason I think it's worth looking at is that the code paths in 0.29.x are largely the fallbacks that are used on PyPy I think. So if there's a reference counting error there it's probably worth fixing, irrespective of what we do with 0.29.x and Python 3.11. (Plus there's clearly something that isn't tested properly in Cython...) |
Not sure. The issue reported was the there was an issue with lxml+Cython 0.29. But it was never clear to me what that issue was (or whether that issue was related to As far as I can see the vast majority of Cython code is working fine. But obviously there may be little issues remaining that haven't yet been found/reported in sufficient detail. Cython's own test suite is running pretty well (but not absolutely perfectly) in Py3.11. Line tracing seems to be broken on the 0.29.x branch (which isn't a core feature but will need fixing eventually). But beyond that the errors look pretty minor (e.g. exception messages have changed slightly so doctests don't write match) |
It seems like this change is no longer needed. I don't understand why previously, lxml required it and now it just works, but at the end, I'm happy that it just works :-) |
Over at statsmodels we experience an issue with Python 3.11 and
This was using Cython 0.29.32 on Python 3.11.0rc2 The error log from the CI is attached below. Could these errors be related to the PR above? Error log building Statsmodels with Python 3.11 on Ubuntu:
|
There looks to be a change in What's slightly puzzling is that our CI seems to pass all the tests for 3.11. So that suggests this bit of code is pretty much untested. I'll have an investigate |
I've replied on the other issue, but the fix for 0.29.x was to disable the code via cython/Cython/Utility/ModuleSetupCode.c Lines 257 to 260 in f2e8b2f
As far as I can see all of it is guarded so really should be being compiled |
@EwoutH See statsmodels/statsmodels#8287 (comment) - I think this is outdated c files rather than a current Cython bug |
Python 3.11 alpha 4 removed exc_type and exc_traceback fields the the
_PyErr_StackItem structure: