Add Py_mod_gil slot to C extension modules #59135
Conversation
|
Is this slot only available in 3.13?
Do you know what makes it not thread unsafe? I'm surprised given how small of a module it is. Definitely more of a nice-to-have on the parser though. Understood its probably a very large undertaking to make the CSV reader overall multi-threaded, and most likely not even worth it with pyarrow |
| @@ -245,7 +245,10 @@ static int pandas_datetime_exec(PyObject *Py_UNUSED(module)) { | |||
| } | |||
|
|
|||
| static PyModuleDef_Slot pandas_datetime_slots[] = { | |||
| {Py_mod_exec, pandas_datetime_exec}, {0, NULL}}; | |||
| {Py_mod_exec, pandas_datetime_exec}, | |||
| {Py_mod_gil, Py_MOD_GIL_NOT_USED}, | |||
There was a problem hiding this comment.
As @WillAyd mentioned, this is new in 3.13:
#if PY_VERSION_HEX >= 0x030D0000
{Py_mod_gil, Py_MOD_GIL_NOT_USED},
#endif
https://docs.python.org/3.14/howto/free-threading-extensions.html#multi-phase-initialization
There was a problem hiding this comment.
Thanks for this. Friday was not a good day, apparently. 🤕
One question: In the free-threaded repo's README we recomment guarding with #ifdef Py_GIL_DISABLED. Should we change that?
There was a problem hiding this comment.
I haven't kept tabs on the CPython implementation but seems like what the free-threaded repo suggests might be better than checking the version hex - there will be both GIL and GIL-less Python compilations for a few versions right?
There was a problem hiding this comment.
@WillAyd That's correct, however Py_mod_gil is just ignored in 3.13+ non-free-threaded builds. Effectively, both guards lead to correct behavior.
There was a problem hiding this comment.
As a matter of style I think checking for Py_GIL_DISABLED would make more sense if you were evaluating whether that was true/false, not just checking if the symbol was defined.
So happy with what we have here, but probably worth asking for input from other projects for the free-threaded repo standard.
We can merge this as is for now and follow up with a different approach if a more canonical one becomes clearer
I mean that it's thread-unsafe in the sense that there's global state in the |
|
Sorry for the typo - I mean "not thread safe" but I think you got what I was saying. And cool thanks for the response - agree with you on that |
|
Thanks @lysnikolaou |
This is the Cython equivalent of adding a `Py_mod_gil` slot with `Py_MOD_GIL_NOT_USED` like we did in pandas-dev#59135.
* ENH: Globally enable Cython free-threading directive This is the Cython equivalent of adding a `Py_mod_gil` slot with `Py_MOD_GIL_NOT_USED` like we did in #59135. * Use add_project_arguments * Mark json with Py_MOD_GIL_NOT_USED & remove PYTHON_GIL env var from ci test job
x-ref #59057.
The
Py_mod_gilslot is needed so that the GIL is not automatically enabled when these C extension modules get imported.pandas_datetimefor thread-safety and it appears to be okay.pandas_parseris not thread-safe. A discussion is needed on whether we should care about it though, since thecengine toread_csvand friends is already documented as being thread-unsafe and the thread-safepyarrowalternative is already there.