[cython/cython] Update module setup code for free-threaded CPython (PR #6137)
Lysandros Nikolaou
Trying to build Cython (and run the test suite) with the latest CPython main with --disable-gil leads to segfaults. The following patch fixes the segfaults. Running the test suite with -X gil=1 (GIL enabled, default) succeeds locally on my machine, while running it with -X gil=0 (GIL disabled) fails with 8 test failures, all related to refnanny.
I'm opening this PR to see whether these changes really make sense (probably first time where there's two CYTHON_COMPILING_IN_* flags enabled) and I'll look into the failures as well, if that's okay. Also, NumPy builds under the free-threaded build with these changes.
You can view, comment on, or merge this pull request online at:
https://github.com/cython/cython/pull/6137
Commit Summary
- a0cff72 Update Cython build flags for nogil
File Changes
(1 file)
Patch Links:
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
scoder
@scoder commented on this pull request.
We don't have a nogil-CPython test job, so we're currently running rather blind on this configuration.
In Cython/Utility/ModuleSetupCode.c:
> -#elif defined(Py_GIL_DISABLED) || defined(Py_NOGIL) +#elif defined(Py_NOGIL)
Now that the nogil fork is merged, it probably makes sense to treat it as a special case of CPython. The risky sections that we used to hide under the …IN_CPYTHON macro should long have received their own special macro (like USE_BORROWED_REFS etc.).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Lysandros Nikolaou
We don't have a nogil-CPython test job, so we're currently running rather blind on this configuration.
Correct. I can work on adding CI for nogil-CPython if you want, but that will probably fail for a while. Also, setup-python currently does not support nogil (actions/setup-python#771), but there' a similar action we can use.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
scoder
@scoder commented on this pull request.
In Cython/Utility/ModuleSetupCode.c:
> + #ifdef Py_GIL_DISABLED + #define CYTHON_COMPILING_IN_NOGIL 1 + #else + #define CYTHON_COMPILING_IN_NOGIL 0 + #endif
This is a bit risky. Currently, there is only ever one CYTHON_COMPILING_IN… macro set to 1, which makes "if-elif-else" special cases easy to write and follow. I'd rather rename this to distinguish it from the "main" target environment (which continues to be CPython here).
OTOH, "compiling in" is pretty accurate, and seeing a name like CYTHON_IN_CPYTHON_NOGIL would make me wonder if someone just forgot the COMPILING part there.
Maybe CYTHON_COMPILING_IN_CPYTHON_NOGIL would be ok as a compromise? That would at least link it to the CPYTHON macro. I'll be happy to read better suggestions.
In Cython/Utility/ModuleSetupCode.c:
> + #ifdef Py_GIL_DISABLED + #define CYTHON_USE_PYLONG_INTERNALS 0 + #else + #define CYTHON_USE_PYLONG_INTERNALS 1 + #endif
I think we should collapse these tivial definitions to
#define CYTHON_USE_PYLONG_INTERNALS (!CYTHON_COMPILING_IN_NOGIL)
(or however we name that macro in the end). We already do that for version checks in a couple of other macros and it's simple enough.
In Cython/Utility/ModuleSetupCode.c:
> + #ifdef Py_GIL_DISABLED + #undef CYTHON_USE_PYLIST_INTERNALS + #define CYTHON_USE_PYLIST_INTERNALS 0 + #else + #ifndef CYTHON_USE_PYLIST_INTERNALS + #define CYTHON_USE_PYLIST_INTERNALS 1 + #endif #endif
Let's just chain these cases like this:
⬇️ Suggested change- #ifdef Py_GIL_DISABLED - #undef CYTHON_USE_PYLIST_INTERNALS - #define CYTHON_USE_PYLIST_INTERNALS 0 - #else - #ifndef CYTHON_USE_PYLIST_INTERNALS - #define CYTHON_USE_PYLIST_INTERNALS 1 - #endif - #endif + #ifdef Py_GIL_DISABLED + #undef CYTHON_USE_PYLIST_INTERNALS + #define CYTHON_USE_PYLIST_INTERNALS 0 + #elif !defined(CYTHON_USE_PYLIST_INTERNALS) + #define CYTHON_USE_PYLIST_INTERNALS 1 + #endif
In Cython/Utility/ModuleSetupCode.c:
> #ifndef CYTHON_METH_FASTCALL
// CPython 3.6 introduced METH_FASTCALL but with slightly different
// semantics. It became stable starting from CPython 3.7.
+ #ifdef Py_GIL_DISABLED
+ #define CYTHON_METH_FASTCALL 0
+ #else
#define CYTHON_METH_FASTCALL 1
+ #define CYTHON_METH_FASTCALL 1
+ #define CYTHON_METH_FASTCALL 1
+ #endif
#endif
This looks a bit messed up. You'd probably want the same structure as for the other cases: test the gil-disabled case first.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Nathan Goldbaum
When I compile cython itself on this branch, I see warnings like
/Users/goldbaum/Documents/cython/Cython/Plex/DFA.c:7321:13: warning: code will never be executed [-Wunreachable-code]
7321 | goto bad;
| ^~~~~~~~
/Users/goldbaum/Documents/cython/Cython/Plex/DFA.c:10197:79: warning: code will never be executed [-Wunreachable-code]
10197 | if (unlikely(__Pyx_PyTuple_SET_ITEM(varnames_tuple, i, varnames[i]))) goto done;
| ^~~~~~~~~
2 warnings generated.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
da-woods
When I compile cython itself on this branch, I see warnings like
That probably is related. The Limited API work introduces some error checking that's skipped for non-limited API operations. That'll add some unused paths on the non-limited API mode. At some point we're going to need to clean up the warnings from those.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
da-woods
#elif defined(Py_GIL_DISABLED) || defined(Py_NOGIL)
A few months ago @colesbury asked to keep the old Py_GIL_DISABLED define because apparently some people are using his old prototype fork (#5852 (comment)).
My proposal would be to treat them as two unrelated products and have a separate define for each. So leave the old code as is (maybe renaming to CYTHON_COMPILING_IN_NOGIL_PROTOTYPE or something like that) and treat the new official free-threating CPython as @scoder suggests above.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Sam Gross
FWIW, I think we can now drop the old Py_NOGIL support (i.e., nogil-3.9) and just include Py_GIL_DISABLED (i.e., upstream CPython 3.13 with the ---disable-gil` build configuration).
I think we should be able update scikit-learn's continuous build soon, and in the meantime I'm already maintaining out-of-tree patches and can continue to do so.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
da-woods
FWIW, I think we can now drop the old
Py_NOGILsupport (i.e., nogil-3.9) and just includePy_GIL_DISABLED(i.e., upstream CPython 3.13 with the--disable-gilbuild configuration).
Thanks - I'm happy for it to be dropped then.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Lysandros Nikolaou
@lysnikolaou pushed 1 commit.
- eb497e2 Cleanup defines structure
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Lysandros Nikolaou
@lysnikolaou commented on this pull request.
In Cython/Utility/ModuleSetupCode.c:
> + #ifdef Py_GIL_DISABLED + #define CYTHON_COMPILING_IN_NOGIL 1 + #else + #define CYTHON_COMPILING_IN_NOGIL 0 + #endif
CYTHON_COMPILING_IN_CPYTHON_NOGIL sounds good to me.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Lysandros Nikolaou
Regarding the CI job, it probably makes more sense to keep it in a separate fork, right? I assume it'll fail a lot over the next few weeks, and it probably wouldn't be a good idea to always have a red CI.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
scoder
Regarding the CI job, it probably makes more sense to keep it in a separate fork, right? I assume it'll fail a lot over the next few weeks, and it probably wouldn't be a good idea to always have a red CI.
The Py3.13 jobs have allow_failures=true, and so would the nogil jobs. CPython's C-API has been under heavy changes for the last 2-3 releases, so we regularly end up with failing "latest Python" CI jobs for a while, until the problems either fix themselves (rarely) or we find the time to look into the failures and then either fix them on our side (sometimes) or start a discussion in their bug tracker (usually).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Lysandros Nikolaou
@lysnikolaou pushed 1 commit.
- 2545b5c Fix typo in module setup code
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Lysandros Nikolaou
@lysnikolaou pushed 1 commit.
- 39890cf Fix flags again
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Nathan Goldbaum
Any further comments on this? Lysandros is on vacation for the next week or two but he's asked me to help make sure this gets merged.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
scoder
@scoder commented on this pull request.
In Cython/Utility/ModuleSetupCode.c:
> #endif - #ifndef CYTHON_USE_PYLIST_INTERNALS + #ifdef Py_GIL_DISABLED
We should have one place where we decide about internal configuration macros, and then use only them.
⬇️ Suggested change- #ifdef Py_GIL_DISABLED + #if CYTHON_COMPILING_IN_CPYTHON_NOGIL
In Cython/Utility/ModuleSetupCode.c:
> #define CYTHON_USE_PYLIST_INTERNALS 1
#endif
#ifndef CYTHON_USE_UNICODE_INTERNALS
#define CYTHON_USE_UNICODE_INTERNALS 1
#endif
- #if PY_VERSION_HEX >= 0x030B00A2
+ #if defined(Py_GIL_DISABLED) || PY_VERSION_HEX >= 0x030B00A2
⬇️ Suggested change
- #if defined(Py_GIL_DISABLED) || PY_VERSION_HEX >= 0x030B00A2 + #if CYTHON_COMPILING_IN_CPYTHON_NOGIL || PY_VERSION_HEX >= 0x030B00A2
In Cython/Utility/ModuleSetupCode.c:
> @@ -382,20 +318,29 @@
#ifndef CYTHON_UNPACK_METHODS
#define CYTHON_UNPACK_METHODS 1
#endif
- #ifndef CYTHON_FAST_THREAD_STATE
+ #ifdef Py_GIL_DISABLED
⬇️ Suggested change
- #ifdef Py_GIL_DISABLED + #if CYTHON_COMPILING_IN_CPYTHON_NOGIL
In Cython/Utility/ModuleSetupCode.c:
> #define CYTHON_FAST_THREAD_STATE 1 #endif - #ifndef CYTHON_FAST_GIL + #ifdef Py_GIL_DISABLED⬇️ Suggested change
- #ifdef Py_GIL_DISABLED + #if CYTHON_COMPILING_IN_CPYTHON_NOGIL
In Cython/Utility/ModuleSetupCode.c:
> #endif - #ifndef CYTHON_FAST_PYCALL + #ifdef Py_GIL_DISABLED⬇️ Suggested change
- #ifdef Py_GIL_DISABLED + #if CYTHON_COMPILING_IN_CPYTHON_NOGIL
In Cython/Utility/ModuleSetupCode.c:
> @@ -412,7 +357,10 @@
#ifndef CYTHON_USE_TP_FINALIZE
#define CYTHON_USE_TP_FINALIZE 1
#endif
- #ifndef CYTHON_USE_DICT_VERSIONS
+ #ifdef Py_GIL_DISABLED
⬇️ Suggested change
- #ifdef Py_GIL_DISABLED + #if CYTHON_COMPILING_IN_CPYTHON_NOGIL
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
scoder
@scoder pushed 1 commit.
- 421cd61 Use Cython configuration macro instead of external macro.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
scoder
Merged #6137 into master.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
scoder
Thanks
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Henry Schreiner
FYI, cibuildwheel will be shipping free-threaded support soon.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.