-
Notifications
You must be signed in to change notification settings - Fork 27
build: add freethreaded python ci and wheels #284
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
base: master
Are you sure you want to change the base?
Conversation
|
Nope it's in beta now! |
|
refreshing this PR since there's now beta2 of the freethreaded pydantic |
|
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. |
|
That looks like https://gitlab.kitware.com/cmake/cmake/-/issues/26016, which is supposedly fixed but I'm not sure in which CMake version. |
Ah, sorry, now I see 3.30.3 — so supposedly it should work here. |
|
Sorry for thinking loudly. I see that CMake is doing the right thing: So looks like |
|
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.
|
|
Thanks @henryiii, giving it a try. |
|
3.13t manylinux wheels are dying on tests because of missing awkward 3.13t wheel. Otherwise they build fine! |
|
@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. |
|
Nice - parallel tests seem to go, except on windows. Looks like some issue with parallel processing on windows to begin with. |
|
I don't really understand this failure in macos. Must be a threadsafety thing, but then why not in ubuntu? |
|
Oh wow, it's definitely a thread safety issue (it passed in this latest commit!). Yikes! |
|
@nsmith- Any first ideas on what needs a mutex around it? |
|
For reference the error that crops up in the MT tests is: |
|
My best guess would be something related to this: Lines 106 to 108 in 093ce46
is not threadsafe. |
|
Digging around there seems to be something suspicious w.r.t. the Will continue digging. |
|
Darn, after all that the error is still there! @henryiii any ideas? |
| } | ||
|
|
||
| PYBIND11_MODULE(_core, m) { | ||
| PYBIND11_MODULE(_core, m, py::mod_gil_not_used()) { |
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.
Is this saying there is no need to take the gil for the whole module? I think that's not true.
For example, CorrectionSet.from_file will need to create something reference-counted.
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.
It appears to be bad naming for "free threading support" https://pybind11.readthedocs.io/en/stable/advanced/misc.html#free-threading-support
I can look into it further to see what it implies.
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.
That’s what Python calls it. If you need a lock, take a lock (like std::mutex), but that lock isn’t the GIL when there isn’t one.
|
Should probably use https://github.com/Quansight-Labs/pytest-run-parallel. |
|
I added this in a previous PR which needs changing too: Line 99 in 122f3e5
According to cibuildwheel, 314t are automatically built while 313t need to be requested. |
|
Yep, still getting: and I have absolutely no idea. |
|
and dask still doesn't have freethreaded. |
|
I'll try to do a correctionlib sprint for some of the other feature requests soon. Maybe threaded can be part of that. |
|
@lgray there are a few unrelated improvements, would you be willing to spin those off to a new PR? |
Will add wheels when they're fixed up by #283