Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged

Conversation

lysnikolaou
Copy link
Contributor

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.

tests/run/freethreading_compatible.srctree Show resolved Hide resolved
@lysnikolaou lysnikolaou marked this pull request as ready for review June 12, 2024 20:04
@lysnikolaou
Copy link
Contributor Author

Thanks for the quick response on this @da-woods! I've addressed the feedback.

Cython/Compiler/Options.py Outdated Show resolved Hide resolved
@da-woods
Copy link
Contributor

I'll leave this open for a little bit so people can bikeshed names. But I think it's basically good

@scoder scoder added this to the 3.1 milestone Jun 15, 2024
Copy link
Contributor

@scoder scoder left a 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.

Cython/Compiler/ModuleNode.py Outdated Show resolved Hide resolved
Cython/Compiler/ModuleNode.py Outdated Show resolved Hide resolved
Cython/Compiler/ModuleNode.py Outdated Show resolved Hide resolved
Cython/Compiler/ModuleNode.py Outdated Show resolved Hide resolved
@@ -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)
Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@colesbury
Copy link
Contributor

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.

@da-woods
Copy link
Contributor

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

@scoder
Copy link
Contributor

scoder commented Jul 11, 2024 via email

@ngoldbaum
Copy link
Contributor

I tried to compile _pcg64.pyx in numpy with this directive inserted at the top of the file and hit this compilation error when building numpy:

numpy/random/_pcg64.cpython-313t-darwin.so.p/numpy/random/_pcg64.pyx.c:9310:3: error: expected '}'
  {0, NULL}
  ^
numpy/random/_pcg64.cpython-313t-darwin.so.p/numpy/random/_pcg64.pyx.c:9304:51: note: to match this '{'
static PyModuleDef_Slot __pyx_moduledef_slots[] = {
                                                  ^
1 error generated.

Haven't looked further at why this is happening yet.

@ngoldbaum
Copy link
Contributor

Hmm, that ended up being because I had a typo in the directive name, frethreading_compatible=True. Not sure why the miscompilation happened with the typo given the code change in this PR.

Cython/Compiler/ModuleNode.py Outdated Show resolved Hide resolved
@lysnikolaou
Copy link
Contributor Author

Thanks a lot everyone for the feedback. Pushed a new commit that:

  • Fixes the bug that @ngoldbaum discovered (syntax error in single-phase init)
  • Adds tests for this under single-phase init
  • Mentions in the documentation that this is still experimental
  • Removes the directive type declaration like @scoder suggested

@da-woods da-woods merged commit 746627f into cython:master Jul 12, 2024
59 of 65 checks passed
@da-woods
Copy link
Contributor

Thanks

@scoder
Copy link
Contributor

scoder commented Jul 16, 2024

The new test is failing in the freethreading CPython build:

Errors from shard 6:
======================================================================
FAIL: runTest (__main__.EndToEndTest.runTest)
[6] End-to-end freethreading_compatible
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/cython/cython/runtests.py", line 2040, in runTest
    self.assertEqual(0, res, "non-zero exit status, last output was:\n%r\n-- stdout:%s\n-- stderr:%s\n" % (
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        ' '.join(command), out[-1], err[-1]))
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 0 != 1 : non-zero exit status, last output was:
'/home/runner/venv-3.13/bin/python -c import default; default.test()'
-- stdout:
-- stderr:Traceback (most recent call last):
  File "<string>", line 1, in <module>
    import default; default.test()
                    ~~~~~~~~~~~~^^
  File "default.py", line 7, in default.test
    assert sys._is_gil_enabled()
AssertionError

https://github.com/cython/cython/actions/runs/9903272103/job/27358534710

@da-woods
Copy link
Contributor

default.py should be assert not sys._is_gil_enabled() instead I think. So it's the test that's wrong rather than the behaviour.

@lysnikolaou
Copy link
Contributor Author

lysnikolaou commented Jul 17, 2024

Hmm, that's weird. The test succeeds locally on my macOS.

default.py should be assert not sys._is_gil_enabled() instead I think. So it's the test that's wrong rather than the behaviour.

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.

@lysnikolaou
Copy link
Contributor Author

lysnikolaou commented Jul 17, 2024

Oh, it's because the CI job sets PYTHON_GIL=0 so that GIL is not enabled, even if a non-compatible modules is imported. Not sure what the correct fix is here, since we probably don't want to enable the GIL while running the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH] Add a way to set Py_mod_gil slot for modules defined in cython
5 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.