-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add freethreading_compatible directive to set Py_mod_gil slot #6242
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
Add freethreading_compatible directive to set Py_mod_gil slot #6242
Conversation
Thanks for the quick response on this @da-woods! I've addressed the feedback. |
I'll leave this open for a little bit so people can bikeshed names. But I think it's basically good |
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.
Seems reasonable to add the directive. Suggestions below.
@@ -833,6 +833,16 @@ Cython code. Here is the list of currently supported directives: | ||
appropriate exception is raised. This is off by default for | ||
performance reasons. Default is False. | ||
|
||
``freethreading_compatible`` (True / False) |
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.
From another PR, it seems that it would be helpful to know whether the module targets freethreading Python or not, in order to take performance relevant code generation decisions. Is there an argument for reserving this option to "is compatible" rather than "wants to support"? If not, then I'd just call it freethreading
and give it both meanings when enabled: "I accept slightly less optimal code generation because I want the module to be fully working without the GIL in a free-threading Python".
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.
I think the "do we want to generate slightly less efficient code" is probably a separate question .
The code generation seems like something you should be able to turn on and off locally based on your knowledge of how your own module will be used.
I think it'd be reasonable for this option to affect the default value for the code generation, since if a module isn't designed to be free-threaded then there's no need to slow things down to support it.
But after that, I think they should be separate directives.
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.
Agree with @da-woods here FWIW.
It'd be great to see this get merged soon as it's a prerequisite for building free-threading compatible wheels for many of the projects that depend on Cython. |
@lysnikolaou and I had a brief chat about this. I think we'd both like to get this merged with the current behaviour in the fairly near future. (From my point of view that means "probably sometime after EuroPython") My one suggestion was to explicitly document the option as "experimental" - I think we do want to give ourselves freedom to evolve his we treat free-threading for at least as long as it's experimental in CPython. |
I'm fine with merging this, but yes, please document it as experimental. It's not obvious that there will be changes later, but it's going to take a while until we understand the user experience well enough to give good answers to it.
One comment, there is no need to declare the directive type since that's handled automatically for a bool default value.
|
I tried to compile
Haven't looked further at why this is happening yet. |
Hmm, that ended up being because I had a typo in the directive name, |
Thanks a lot everyone for the feedback. Pushed a new commit that:
|
Thanks |
The new test is failing in the freethreading CPython build:
https://github.com/cython/cython/actions/runs/9903272103/job/27358534710 |
|
Hmm, that's weird. The test succeeds locally on my macOS.
I don't think that's correct. By default, the directive is off, which means that the GIL should be enabled after importing the module. |
Oh, it's because the CI job sets |
Closes #6239.
The reason this is still a draft is that the test should only run under the free-threaded build. Is that something we can do easily? I saw the mechanism that includes limited api tests, but that seems more complex.