Update module setup code for free-threaded CPython

[lysnikolaou]

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.

[scoder]

We don't have a nogil-CPython test job, so we're currently running rather blind on this configuration.

[scoder]

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.).

[lysnikolaou]

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.

[scoder]

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.

[lysnikolaou]

CYTHON_COMPILING_IN_CPYTHON_NOGIL sounds good to me.

[ngoldbaum]

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.

[da-woods]

When I compile cython itself on this branch, I see warnings like

That probably is unrelated. 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.

Edit: unrelated

[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.

[colesbury]

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.

[da-woods]

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).

Thanks - I'm happy for it to be dropped then.

[lysnikolaou]

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.

[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).

[ngoldbaum]

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.

[scoder]

Thanks