-
Notifications
You must be signed in to change notification settings - Fork 22
feat(arro3): Freethreading - module markers + CI #275
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
Conversation
|
I was hitting this #263. If you find a way to use uv on Windows I'd be happy to use it. Maybe it's an issue with PATH? |
|
It's also an issue that the buffer protocol is even less sound in Python 3.13t. See discussion: https://discord.com/channels/1209263839632424990/1310572181138178129/1310572181138178129, https://alexgaynor.net/2022/oct/23/buffers-on-the-edge/, PyO3/pyo3#2824 I'm not sure what to do here. It's very beneficial to have the buffer protocol as an option. So perhaps just document to Python users that it's unsafe to mutate the Python buffer after passing it to rust? |
|
Yeah - I'm actually having a great deal of difficulty thinking of buffer protocol scenarios where freethreading would be unsound but not GIL-enabled; there's definitely a difference, but the complete lack of immutability guarantees is 99% of the problem (and serialized python execution (the GIL) addresses the remaining 1%). I think the safety note on By the sounds of (amongst other parts of that document) https://py-free-threading.github.io/porting/#dealing-with-thread-unsafe-objects and the comments on how numpy is going ahead with this, pathological cases (like resizing an array while it's being read) can just be left as "you're holding it wrong, enjoy the segfault", right up until the first bug report. |
|
Hmm it looks like I can't trigger the wheel builds on this repo from your PR.
Ideally this would also be surfaced to Python to be seen by Python users, but I'm not sure the right level of obtrusiveness for that. |
|
It would be nice if we didn't have to use a fork of |
Yeah, I noticed that in my own fork, though that seems to be fine now 🤔. It's likely a matter of: jobs can be triggered manually via workflow_dispatch, but not on pull requests (the wheels would need on: pull_request, which might be ok with cancel-in-progress); there doesn't seem to be a non-convoluted way to do manually triggered PR checks. |
|
Mm, not yet, probably mid-January (given the whole Christmas/NYE period is upon us) - it looks like it's down to a single mutex, probably test reconfiguration too. Obtrusiveness-wise, I can stick a (concise) spiel in the docstring of Array::frombuffer; I'd be hesitant to log anything at runtime higher than trace or debug level. |
|
I'm inclined not to merge this until numpy gets a stable release including the |
|
Yep, I'll stick this one (and related) in draft. |
|
Numpy merged its free-threading pr: PyO3/rust-numpy#471 (comment) |
|
Excellent, hopefully shouldn't be too much longer until that gets a release. I'll re-pin to the tip of |
|
Hrm, blocked by the pyO3 breaking change in 0.23.4 involving the |
…ompile without subsequent commit (avoid keeping it though, since it erases timezone info))
Can you merge in latest main? It should be fixed (with a temporary fixed offset workaround) |
b4caf36 to
fd1726b
Compare
|
With #304 it looks like this might actually be unblocked? |
| } | ||
|
|
||
| #[pymodule] | ||
| #[pymodule(gil_used = false)] |
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.
I'm not sure this will always be true for the IO module. E.g. I might want to allow a Python HTTP library like aiohttp to be used for making the parquet file reads
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.
True, if unmodified (as in, rust locking structures cfg-gated on Py_GIL_DISABLED) - there isn't code that allows that quite yet, right?
I should definitely take a crack at pytest-freethread in general before this is merged (at the very least CI for a single test in pyo3-arrow or core).
Thinking about it, pretty much all users of the freethreading build at the moment are other library maintainers porting their package to freethreading - this is probably a rare instance (especially given the main dependant is geoarrow.rust.io) where hard concurrency errors are preferred over RuntimeWarning .
Oh, excellent - I gather the obstore py03 0.24.0 + object store 0.11 release unblocks this too (via #304 probably), right? That was my last blocker on this (the parquet crate's dependency on object store was involved in there somewhere). Thought I'd have to wait for the April parquet release. |
|
Yeah I made compatibility releases https://github.com/developmentseed/obstore/tree/main/pyo3-object_store#version-compatibility |
|
I can take a closer look if anything is still depending on the GIL if you want.
FYI, Using the |
pyproject.toml
Outdated
| description = "Add your description here" | ||
| readme = "README.md" | ||
| requires-python = ">=3.11" | ||
| requires-python = ">=3.13" |
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.
This should be reverted before merging.
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.
The top-level pyproject.toml is only for development. This change only means that to run the tests you need to be using 3.13+.
I renamed this name to "dev-environment" to make that clearer.
Regardless, might not need to pin 3.13 here.
|
According to pytest-freethreaded it's also important to run with When I run this locally with the GIL disabled I get a segfault, so something is wrong? @bschoenmaeckers do you have an idea? |
|
Now I'm able to reproduce the segfault on CI as well https://github.com/kylebarron/arro3/actions/runs/17590097733/job/49968516771?pr=275 cc @H-Plus-Time as well |
|
I’m not entirely sure what happens here without a debug build of python. Does this error also occur on 3.14? |
I tried But that doesn't work out of the box because pyarrow doesn't have a built wheel for 3.14t and I don't want to figure out how to build it. We could split out the tests to not require pyarrow but I don't have time to do that now. |
|
I updated package versions (esp maturin) and now CI seems to pass 🤷♂️. Thanks all for your work here! |
Yet another fork (this time in setup-python), what exactly was it about setup-uv in windows that broke builds?
Verified wheel building run over at https://github.com/H-Plus-Time/arro3/actions/runs/12349504140.