Skip to content
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

Document Cython's freethreading_compatible directive #35

Merged
merged 4 commits into from
Jul 16, 2024

Conversation

lysnikolaou
Copy link
Member

No description provided.

@lysnikolaou lysnikolaou added the documentation Improvements or additions to documentation label Jul 15, 2024
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lysnikolaou! A few suggested tweaks

docs/porting.md Outdated Show resolved Hide resolved
docs/porting.md Outdated Show resolved Hide resolved
docs/porting.md Show resolved Hide resolved
Co-authored-by: Ralf Gommers <ralf.gommers@gmail.com>
@lysnikolaou
Copy link
Member Author

Thanks for the review @rgommers! Feedback addressed.

(`# cython: freethreading_compatible=True`) in `.pyx` files, or globally by adding
`-Xfreethreading_compatible=True` to the Cython arguments via the project's
build system.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's some example code to do this via build configuration with setuptools and meson:

compiler_directives = {'freethreading_compatible': True}

setup(
    ext_modules=cythonize(
        extensions,
        compiler_directives=compiler_directives)
)
cython_args = []
if cy.version().version_compare('>=3.1.0')
  cython_args += ['-Xfreethreading_compatible=True']
endif

Meson compiler versions don't include alpha or beta qualifiers, so no need to check that for cython.

For the setuptools one, you could note that you can use e.g. packaging.version to check for a minimum Cython version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of including example build config code - that'll definitely be helpful for users.

The Meson code should include an extension_module call as well I think, something like:

py.extension_module('modulename'
    'source.pyx',
    cython_args: cython_args,
    ...
)

Even nicer if we had a CMake/scikit-build-core version of it as well. Maybe @henryiii could suggest an idiomatic snippet?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks both for the suggestions! I added setuptools and Meson examples, but I don't know enough about CMake/scikit-build-core to do that as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re scikit-build-core, I think this is a good example: https://github.com/scikit-build/scikit-build-sample-projects/tree/main/projects/hello-free-threading (or the hello-cython next to it). However, it doesn't have the code yet to mark extension modules as compatible and I'm not sure what the idiomatic way to do it is, so let's do this as a follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be better once we finish scikit-build/cython-cmake#19. Will try to do that this week and then update here.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there!

docs/porting.md Outdated Show resolved Hide resolved
docs/porting.md Outdated Show resolved Hide resolved
docs/porting.md Outdated Show resolved Hide resolved
Co-authored-by: Ralf Gommers <ralf.gommers@gmail.com>
@rgommers rgommers merged commit c12572b into Quansight-Labs:main Jul 16, 2024
1 check passed
@rgommers
Copy link
Member

Merged, thanks @lysnikolaou and @ngoldbaum.

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

Successfully merging this pull request may close these issues.

4 participants