-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Don't define Py_LIMITED_API and don't re-enable GIL under free-threading #3482
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
Conversation
This needs a rebase on top of #3484 to pass CI. I previously found the build to work when Even though the module is thread-safe, I'm reluctant to call Because Tornado is a single-threaded framework, I'd prefer to avoid anything that implies future changes while free-threading is in its experimental phase. Minimizing churn here is more important to me than maximizing support for free-threading. (I'd be OK if the call to |
On macOS, I'm getting the following build error (which I find expected since the
Agreed. Since the |
Hmm, that error is clear and has been there since 3.13.0b1. Did I make a mistake in my earlier testing and not actually run a free-threaded build? The init changes still need some ifdefs because they use symbols introduced in python 3.12 and 3.13 but we support back to 3.9. |
Sorry for not making it clear in my last comment, but I had to use
Yup, missed that. On it. |
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.
Ah yes, the TORNADO_EXTENSION option probably wasn't set right in some of my tests (but I did have it sometimes because I did have at least some failures).
This got more complicated than anticipated but I think it's almost ready to go.
tornado/speedups.c
Outdated
#if (!defined(Py_LIMITED_API) && PY_VERSION_HEX >= 0x030c0000) || Py_LIMITED_API >= 0x030c0000 | ||
{Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED}, | ||
#endif | ||
#if !defined(Py_LIMITED_API) && PY_VERSION_HEX >= 0x030d0000 |
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.
Why are these two #if
s not the same structure? The definitions are the same. I think the second one also needs a Py_LIMITED_API >= 0x030d0000
clause.
It would be simpler to do #ifdef Py_mod_gil
, although I don't think it's mandated that these are #define
s instead of global variables.
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.
Yeah, I'd prefer to not go with #ifdef Py_mod_gil
. These would probably not change, but it's still more stable to do a version check.
Also, there's only wheels for the stable ABI on PyPI. Would you be open to uploading wheels for |
I'm open to uploading cp313t wheels to pypi if someone else does the work ;) From my prior testing the blocker is that auditwheel was failing for unclear reasons so I assume the wheels aren't releasable until that's fixed (maybe just needs a version bump?) See tornado/.github/workflows/test.yml Line 152 in bfe7489
|
Anyway, this looks good so I'm going to go ahead and merge it. |
Py_LIMITED_API
macro leads to build failurestornado.speedups
since it's thread-safe.