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

Add free-threading support #462

Closed
wants to merge 8 commits into from

Conversation

dairiki
Copy link
Contributor

@dairiki dairiki commented Aug 23, 2024

This is an attempt to address #460. Mostly, I followed the instructions that @AA-Turner linked to there.

I looked over _speedups.c. My non-expert eye did not see anything that didn't look nogil-safe, so I just added the Py_MOD_GIL_NOT_USED declaration to the extension module.

The matrix in the tests.yaml workflow has been expanded to included free-threading versions of python3.13 under Linux and Windows.
The Linux tests work.
The Windows tests do not, for reasons that I do not understand.

I played with enabling free-threading cibuildwheel builds in a separate branch, and that seems to just work. (Though I have not tested any of the resulting wheels to verify that they are usable.)

Fixes #460

@dairiki
Copy link
Contributor Author

dairiki commented Aug 23, 2024

In the Windows free-threading test, pytest appears to fail with

Exception ignored on threading shutdown:
Traceback (most recent call last):
  File "C:\Python313\Lib\threading.py", line 1524, in _shutdown
    if _main_thread._handle.is_done() and _is_main_interpreter():
  py313: FAIL code 32[21](https://github.com/dairiki/markupsafe/actions/runs/10529464771/job/29177296179#step:7:22)225477 (19.61=setup[18.12]+cmd[0.03,1.46] seconds)
SystemError: <method 'is_done' of '_thread._ThreadHandle' objects> returned a result with an exception set

I have no idea what that might be about.
(I have no Windows system available on which to play around locally so further investigation is not easy.)

@dairiki dairiki mentioned this pull request Sep 27, 2024
src/markupsafe/_speedups.c Outdated Show resolved Hide resolved
.github/workflows/tests.yaml Outdated Show resolved Hide resolved
- run: pip install tox
- run: tox run -e ${{ matrix.tox || format('py{0}', matrix.python) }}
--discover C:\Python313\python3.13t.exe

Choose a reason for hiding this comment

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

Should this be restricted to Windows-only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly, though it does not seem to hurt on other platforms.
(The tox docs say that --discover means "for Python discovery first try these Python executables".)

TBH, at this point, I do not remember if the --discover is even necessary (though I suspect that would have removed it if it were not.) There was a lot of flailing involved in getting tox to find the manually-installed version of python on Windows (including, also, the manual addition of the install directories to $PATH).

An alternative to the use of --discover might be to rename the installed python3.13t.exe to python3.13.exe.

This whole PR could, admittedly, use a bit of cleanup.
Someone who knows their way around Windows and PowerShell could probably do a better job on this than I.
(And, hopefully, some time soon, actions/setup-python will support installing no-GIL versions. See actions/setup-python#771 for progress and some hints.)

@AA-Turner
Copy link

(I have no Windows system available on which to play around locally so further investigation is not easy.)

I do have Windows, is there a way to download built artifacts from CIBW?

A

@dairiki
Copy link
Contributor Author

dairiki commented Sep 27, 2024

(I have no Windows system available on which to play around locally so further investigation is not easy.)

I do have Windows, is there a way to download built artifacts from CIBW?

A

@AA-Turner Yes. If one looks at the workflow logs from the actions/upload-artifact step, there is a download URL.

I've just updated my play.free-threading-cibuildwheel to match the latest cibuildwheel config in main.
You can find the workflow log here.

The download URL for the Windows job is https://github.com/dairiki/markupsafe/actions/runs/11072940568/artifacts/1988029015
(That should get you a zip file containing a bunch of Windows wheels.)

@davidism
Copy link
Member

davidism commented Oct 6, 2024

Seems like this was integrated into #461

@davidism davidism closed this Oct 6, 2024
@dairiki
Copy link
Contributor Author

dairiki commented Oct 6, 2024

Seems like this was integrated into #461

Not completely.

As far as I can see, #461 only runs the free-threaded CI tests under Linux.

This PR attempts to run them on Windows as well (though that test is currently failing for undetermined reasons).

Neither PR runs free-threaded tests on macos.

@davidism
Copy link
Member

davidism commented Oct 6, 2024

I'm fine with just Linux for now. MarkupSafe doesn't actually do anything with threading or the GIL, so the tests aren't really doing much besides confirming that it imports. Once setup-python supports free-threading we can decide how much to test.

@davidism
Copy link
Member

davidism commented Oct 6, 2024

I see a segfault locally as well, just from import markupsafe in a 3.13.0rc3 free threaded environment. I can't see any reason for MarkupSafe to cause that, so maybe it's something that needs to be worked out with the Python build?

@dairiki
Copy link
Contributor Author

dairiki commented Oct 6, 2024

Has anyone tried/managed to test a (free-threading) cibuildwheel-built wheel on Windows yet?

E.g. #462 (comment)

@davidism
Copy link
Member

davidism commented Oct 6, 2024

Good call. Installing that wheel and running the tests passed. I'm so confused, but I guess I can release those wheels. What is cibuildwheel doing that I'm not locally?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please could markupsafe._speedups declare support (or lack thereof) for free-threaded Python?
3 participants