build: add freethreaded python ci and wheels #284
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
Nope it's in beta now! |
lgray
changed the title
Contributor
Author
|
refreshing this PR since there's now beta2 of the freethreaded pydantic |
Contributor
Author
|
I noticed when it's doing the freethreaded windows build that the MS linker is looking for Results in the error (I think): I think that's the only blocker on windows. |
Contributor
|
That looks like https://gitlab.kitware.com/cmake/cmake/-/issues/26016, which is supposedly fixed but I'm not sure in which CMake version. |
Contributor
Ah, sorry, now I see 3.30.3 — so supposedly it should work here. |
Contributor
|
Sorry for thinking loudly. I see that CMake is doing the right thing: So looks like |
Contributor
Author
|
No problem - think out loud all you want. I'm just not sure where to look :-) |
|
You need to use the modern FindPython, not the old one. FindPythonLibs / FindPythonInterp was "removed" (sort of) in CMake 3.27, so that's not what 3.30.3 is referring to. |
|
Right above here Lines 16 to 21 in ea10902 set(PYBIND11_FINDPYTHON ON). pybind11 3.0 will change the default.
|
Contributor
Author
|
Thanks @henryiii, giving it a try. |
Contributor
Author
|
3.13t manylinux wheels are dying on tests because of missing awkward 3.13t wheel. Otherwise they build fine! |
Contributor
Author
|
@henryiii I guess the best way to test this in freethreaded mode for races and such would to first just try pytest-parallel? Probably mark the dask tests to not be run in parallel? |
|
Yes, I believe so. |
Contributor
Author
|
Nice - parallel tests seem to go, except on windows. Looks like some issue with parallel processing on windows to begin with. |
Contributor
Author
|
I don't really understand this failure in macos. Must be a threadsafety thing, but then why not in ubuntu? |
Contributor
Author
|
Oh wow, it's definitely a thread safety issue (it passed in this latest commit!). Yikes! |
Contributor
Author
|
@nsmith- Any first ideas on what needs a mutex around it? |
Contributor
Author
|
For reference the error that crops up in the MT tests is: |
Collaborator
|
My best guess would be something related to this: Lines 106 to 108 in 093ce46 is not threadsafe. |
Contributor
Author
|
Digging around there seems to be something suspicious w.r.t. the Will continue digging. |
Collaborator
|
@lgray there are a few unrelated improvements, would you be willing to spin those off to a new PR? |
Contributor
|
With a thread-sanitized python build, you can see there is a data race. I don't understand yet whether this is a cpython bug or a pybind11. Here is the log: |
Contributor
|
I have a feeling that it has something to do with when the iterator type is being created? Pybind11 creates the iterator lazily? diff --git a/src/python.cc b/src/python.cc
index f7ffb0d..71d9475 100644
--- a/src/python.cc
+++ b/src/python.cc
@@ -85,7 +85,7 @@ namespace {
return output;
}
}
-PYBIND11_MODULE(_core, m) {
+PYBIND11_MODULE(_core, m, py::mod_gil_not_used()) {
m.doc() = "python binding for corrections evaluator";
py::class_<Variable>(m, "Variable")
@@ -183,4 +183,10 @@ PYBIND11_MODULE(_core, m) {
.value("ACOSH", FormulaAst::UnaryOp::Acosh)
.value("ASINH", FormulaAst::UnaryOp::Asinh)
.value("ATANH", FormulaAst::UnaryOp::Atanh);
+
+ {
+ auto dummy = CorrectionSet::from_string(R"({"schema_version": 2, "corrections": []})");
+ auto it = py::make_key_iterator(dummy->begin(), dummy->end());
+ (void)it;
+ }
} |
Contributor
Author
|
I've added a static mutex and a lock guard here, it's not (or shouldn't be) performance critical code. It's a bit of a sledgehammer but maybe it works. |
Contributor
Author
|
A bunch of stuff to clean up too since this was a heavy rebasing of the changes. We'll see how far it gets. |
Contributor
Author
|
Oh it's some initialization thing? weird! Let's see if the global mutex fixes too. |
Contributor
Author
|
@henryiii could you quickly explain what's going on here such that a global dummy initialization makes things work? I am happy for fewer locks but it's a bit too much faith-based programming for me. |
Contributor
Author
|
The mutex approach also seems to fix it. I suppose, if it's a one-time initialization thing in pybind I could change it to use an |
Contributor
|
Yeah you can also get other errors from this line like That made me have a feeling that multiple threads call |
Contributor
I think you are missing the |
Contributor
Author
|
I'm loading without the nogil checks in python using the commandline args, so it's doing the right thing testing-wise but let me mark it to it always loads properly. |
Contributor
|
I don't know if the mutex is safe. Even though it passes the tests for me locally too under a release build of cpython 3.14, I get a deadlock if I cherry-pick this PR python/cpython#133177 on top of 3.14 which fixes the original TSAN I was seeing. Meaning that it will be a problem in python 3.15 or in 3.14 if this PR gets backported. |
Contributor
|
I have created this reproducer |
Contributor
Author
|
Sweet! thanks. |
Contributor
Author
|
@nsmith- so we now have something that works but it requires quite a bit of testing infrastructure update. I'll start making PRs to get everything else updated and then probably make a clean PR with the freethreading update alone. |
henryiii
reviewed
Jan 21, 2026
| - uses: pypa/[email protected] | ||
| env: | ||
| CIBW_ARCHS: ${{ matrix.arch }} | ||
| CIBW_ENABLE: cpython-freethreading |
There was a problem hiding this comment.
You only need this for 3.13t, 3.14t is not experimental so it's on by default.
Contributor
|
@lgray @nsmith- once pybind/pybind11#5971 is merged, we can just update the pybind11 submodule to point to master and we don't need any workaround for thread-safety. |
Collaborator
|
Great! I'd slightly prefer to wait for a pybind11 release tag. |
Contributor
Author
|
Same - I'll wait for the next pybind release with that fix in. Better way to do it, fewer required changes here. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Will add wheels when they're fixed up by #283