MNT Mark cython extensions as free-threaded compatible #31342
Conversation
…as free-threaded compatible
sklearn/meson.build
Outdated
| @@ -1,6 +1,9 @@ | |||
| fs = import('fs') | |||
|
|
|||
| cython_args = [] | |||
| if cython.version().version_compare('>=3.1.0') | |||
| cython_args += ['-Xfreethreading_compatible=True'] | |||
There was a problem hiding this comment.
Setting this means that the C extensions that come with scikit-learn are marked as working with free threaded Python right? One question I've had about marking extensions as compatible: how do we know? With the GIL there are a lot of things you could do that without the GIL enabled are bugs because they lead to race conditions. How the heck would we find all of them?
There was a problem hiding this comment.
How the heck would we find all of them?
We do not need to find them all to declare that as a project we would consider such race conditions as bugs that we are willing to get fixed.
But I agree, it's better to try to catch as many as possible ahead of time. In the past (e.g. #30007) we started to take a look at pytest extensions that should help us catch those bugs before making a release. We need to give those tools a second look to see which one is the easiest to adopt for scikit-learn, either for manual checks of a particular submodule or automated check on the CI for the full code base.
There was a problem hiding this comment.
True, we don't have to find all of them ahead of time. We could attach a note to free-threaded usage mentioning that it is not yet ready for production use?
I'll take a look at #30007.
There was a problem hiding this comment.
We could attach a note to free-threaded usage mentioning that it is not yet ready for production use?
It would be a good idea to include such a note in the release notes for 1.7. cc @jeremiedbb.
There was a problem hiding this comment.
Reading the docs for freethreading_compatible=True: https://docs.cython.org/en/latest/src/userguide/freethreading.html#status it states:
When you specify this directive, importing the module will not cause the interpreter to re-enable the GIL. The directive itself does not do anything to ensure compatibility - it is simply a way for you to indicate that you have tested your module and are confident that it works.
If we are not confident enough that it works, I do not see the value in setting the flag. It could even be a regression:
- Currently, with
freethreading_compatible=Falsemeans the GIL is reenabled. So the code works today. - If we change it to
freethreading_compatible=Trueand the module is not compatible with free threading, then it could break.
There was a problem hiding this comment.
But we cannot run such tests if we do not release the GIL when importing the native extension modules, right?
There was a problem hiding this comment.
Maybe we should finalize and merge #30041 to main and then resync that branch with main to actually be able to run such tests using pytest-run-parallel.
|
I noticed a few other missing cython generator in the meson.build file and ended up opening a separate PR for it: #31346. |
|
I opened #31357 to do the simpler part of using released Cython 3.1 for free-threaded rather than the dev cython version. Marking our extension as free-threaded compatible will be left for later. |
Cython 3.1 has been released on May 8 2025.
Following scipy PR scipy/scipy#22658 to use
-Xfreethreading_compatible=Truecython argument if cython >= 3.1This cleans up the lock-file and install.sh script as well because cython and scipy are available through conda-forge.
I realized I forgot a
cython_gen.processin #31212 for murmurhash and I was getting a warning about it locally ...