-
Notifications
You must be signed in to change notification settings - Fork 747
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
Tracking issue for no-gil/freethreaded work #4265
Comments
As a tiny piece of this and to try to learn the library better, I'm working on adding wrappers for the new |
Just to update the current state of things: pyo3 builds against the free-threaded build if you do:
If you use pyenv, you'll also need to locally delete or modify the This very quicky crashes inside of mimalloc internals, ultimately inside of
Just to make sure all of this is reproducible and we have some feedback on CI, I think I'm going to add a free-threaded CI job marked with |
That sounds great to me, thanks! |
* Update dict.get_item binding to use PyDict_GetItemRef Refs #4265 * test: add test for dict.get_item error path * test: add test for dict.get_item error path * test: add test for dict.get_item error path * fix: fix logic error in dict.get_item bindings * update: apply david's review suggestions for dict.get_item bindings * update: create ffi::compat to store compatibility shims * update: move PyDict_GetItemRef bindings to spot in order from dictobject.h * build: fix build warning with --no-default-features * doc: expand release note fragments * fix: fix clippy warnings * respond to review comments * Apply suggestion from @mejrs * refactor so cfg is applied to functions * properly set cfgs * fix clippy lints * Apply @davidhewitt's suggestion * deal with upstream deprecation of new_bound
I added a new checkbox for " Adopt new owned-reference-friendly C APIs". If we have a list of all the ones we need, I can make those sub-checkboxes. |
I think I also had a chat with @davidhewitt today and in addition to Our first idea is to make GILProtected a no-op on In addition we need to use I looked at adding a failing CI job, but that won't work right now because of if you run the tests on a free-threaded build with |
Ok, updated the tracking list.
|
I'm still learning the library and it shows... I think David meant |
My guess is it's a reference to |
My mistake, yes we removed the |
See #4421 which updates the FFI bindings for the free-threaded build. That's enough to get the tests to pass without deadlocking, so I added a CI config as well. |
Added a checkbox for |
I see Some more updates for the check boxes:
I also just opened a PR for |
|
I spent some time today trying to understand this more and now I have a test case I think that demonstrates the issue. Rust code: #[pyclass]
#[derive(Default)]
struct PyClassThreadIter {
count: usize,
}
#[pymethods]
impl PyClassThreadIter {
#[new]
pub fn new() -> Self {
Default::default()
}
fn __next__(&mut self) -> PyResult<usize> {
self.count += 1;
Ok(self.count)
}
} and corresponding Python test code: def run_threaded(func, iters):
with concurrent.futures.ThreadPoolExecutor(max_workers=1000) as tpe:
futures = [tpe.submit(func) for _ in range(iters)]
for f in futures:
f.result()
def test_parallel_iter():
i = pyclasses.PyClassThreadIter()
def func():
next(i)
run_threaded(func, 500000)
assert next(i) == 500001 Running this on my M3 Mac with
To answer my question above, this doesn't really have anything to do with the use of Instead if we want to prevent that, I think we would need to refactor the pyclass internals so accessing the instance state is protected by e.g. a |
Well, I think there's a question of whether it should be a lock, or just an
atomic.
…On Tue, Sep 10, 2024, 1:10 PM Nathan Goldbaum ***@***.***> wrote:
I spent some time today trying to understand this more and now I have a
test case I think that demonstrates the issue:
#[pyclass]#[derive(Default)]struct PyClassThreadIter {
count: usize,}
#[pymethods]impl PyClassThreadIter {
#[new]
pub fn new() -> Self {
Default::default()
}
fn __next__(&mut self) -> PyResult<usize> {
self.count += 1;
Ok(self.count)
}}
and corresponding Python test code:
def run_threaded(func, iters):
with concurrent.futures.ThreadPoolExecutor(max_workers=1000) as tpe:
futures = [tpe.submit(func) for _ in range(iters)]
for f in futures:
f.result()
def test_parallel_iter():
i = pyclasses.PyClassThreadIter()
def func():
next(i)
run_threaded(func, 500000)
assert next(i) == 500001
Running this on my M3 Mac with PYTHON_GIL=0 set eventually triggers an
exception:
def func():
> next(i)
E RuntimeError: Already borrowed
To answer my question above, this doesn't really have anything to do with
the use of Cell in PyClassBorrowChecker. That isn't thread-safe, but even
if I replaced the Cell with a std::syc::Mutex, that would only
synchronize the borrow checking, it wouldn't prevent creating two
simultaneous borrowed references in the first place.
Instead if we want to prevent that, I think we would need to refactor the
pyclass internals so accessing the instance state is protected by e.g. a
std::sync::RWLock.
—
Reply to this email directly, view it on GitHub
<#4265 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAGBDJ5MKTMOFBU3BRLNDZV4RZ3AVCNFSM6AAAAABJTCDPMSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBRGUYTKMRQGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
So instead of |
Not spin lock, just raise an exception on an attempted concurrent borrow
…On Tue, Sep 10, 2024, 1:30 PM Nathan Goldbaum ***@***.***> wrote:
So instead of struct BorrowFlag(usize), you'd have struct
BorrowFlag(AtomicUsize). If we read the borrow flag and it's
HAS_MUTABLE_BORROW, we enter a spinlock and block until the thread that
owns the mutable reference releases it. Otherwise everything else is the
same, except increment and decrement are atomic operations.
—
Reply to this email directly, view it on GitHub
<#4265 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAGBARBGI4PIF6XIJA3QLZV4UETAVCNFSM6AAAAABJTCDPMSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBRGU2TMOJTGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Ah well that's how it works right now! Except we're not tracking the references in a thread-safe manner. It's not clear to me if it would be easier in the long run for the ecosystem of extensions written using PyO3 to need to add locking to avoid creating multiple simultaneous mutable borrows coming from Python code or to put the locking in PyO3 itself. That's where my suggestion to add locking comes from. Maybe instead we could add a way to optionally add locking around acquiring simultaneous mutable references to pyclass state, to ease porting for extensions that are implicitly relying on the GIL today. |
Optional locking seems reasonable to me, I'm just opposed to it being the
default
…On Tue, Sep 10, 2024, 1:54 PM Nathan Goldbaum ***@***.***> wrote:
Ah well that's how it works right now! Except we're not tracking the
references in a thread-safe manner.
It's not clear to me if it would be easier in the long run for the
ecosystem of extensions written using PyO3 to need to add locking to avoid
creating multiple simultaneous mutable borrows coming from Python code or
to put the locking in PyO3 itself. That's where my suggestion to add
locking comes from.
Maybe instead we could add a way to optionally add locking around
acquiring simultaneous mutable references to pyclass state, to ease porting
for extensions that are implicitly relying on the GIL today.
—
Reply to this email directly, view it on GitHub
<#4265 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAGBEYGM3RWZ3P6G2D7RLZV4W6ZAVCNFSM6AAAAABJTCDPMSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBRGYYDKNZRGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I don't agree with this, if we implement this it's basically impossible to write a pyclass using this. The problem, as I see it, is that trying to lock something that is already locked (on a different thread) is expected behavior, but doing so with a refcell is a logic error and thus a bug. In the latter case panicking or returning an error is fine, in the former we should probably just wait. |
I don't see why raising an error on concurrent attempts to mutate makes it
impossible to write a pyclass. a) I think this is likely to be an
incredibly rare scenario, b) it's the existing behavior for two threads
attempting to take mutable references to the same value, c) users can
always accomplish shared behavior by taking a shared reference and using
interior mutability, d) this behavior doesn't preclude us offering an
RwLock based option.
…On Thu, Sep 12, 2024 at 3:36 PM Bruno Kolenbrander ***@***.***> wrote:
Not spin lock, just raise an exception on an attempted concurrent borrow
I don't agree with this, if we implement this it's basically impossible to
write a pyclass using this. The problem, as I see it, is that trying to
lock something that is already locked (on a different thread) is expected
behavior, but doing so with a refcell is a logic error and thus a bug. In
the latter case panicking or returning an error is fine, in the former we
should probably just wait.
—
Reply to this email directly, view it on GitHub
<#4265 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAGBCE3UWJCI6GFOJIUYLZWHUKJAVCNFSM6AAAAABJTCDPMSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBXGA4TCNZZGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
All that is necessary for evil to succeed is for good people to do nothing.
|
Sorry to be slow to engage with this part of the discussion; I've been travelling with family and then sick 🤮. Better now! I think the new factor here is that previously if you had a The Overall, I think we are forced to push a pretty tough change on users here. My instinct is that this is a good time to make
I want to start looking into the |
Defaulting to frozen, and then requiring users to pick a strategy for mutability seems good to me. In my experience, a lot of types don't have particularly useful concurrent semantics, and while a lock can make them safe, it can't make it sensible. FWIW my concern is much less that locks have overhead, it's that rw-locks are full of performance cliffs and priority inversion issues (https://blog.nelhage.com/post/rwlock-contention/). |
* Update dict.get_item binding to use PyDict_GetItemRef Refs #4265 * test: add test for dict.get_item error path * test: add test for dict.get_item error path * test: add test for dict.get_item error path * fix: fix logic error in dict.get_item bindings * update: apply david's review suggestions for dict.get_item bindings * update: create ffi::compat to store compatibility shims * update: move PyDict_GetItemRef bindings to spot in order from dictobject.h * build: fix build warning with --no-default-features * doc: expand release note fragments * fix: fix clippy warnings * respond to review comments * Apply suggestion from @mejrs * refactor so cfg is applied to functions * properly set cfgs * fix clippy lints * Apply @davidhewitt's suggestion * deal with upstream deprecation of new_bound
As I commented in #4265, I want to suggest that a realistic goal here for the 0.23 release is to transition from "unusable and unsound" to "unusable but sound for testing". Given that we have breaking changes already landing in 0.23, I think much better to delay the breaking changes needed above in #4265 (comment) to 0.23 (this is primarily immutable by default with the opt-in). Thus for 0.23 I'd like to merge the PRs which make PyO3 sound under free threading but don't change existing semantics, even if those semantics are unhelpful under free threading. This is (as far as I'm aware)
I think that would be good enough to unblock the downstream ecosystem to start testing their own projects under free threading. The focus of the 0.24 release can then be the breaking changes needed to make the semantics of PyO3 actually sensible under free threading. |
That makes sense to me, given that this is clearly documented of course. How are other libraries like Boost and Pybind11 handling this? (If they are even handling nogil at all...) |
I've been experimenting with
(here I'm using a slightly modified version of cargo-stress to get better error reporting, see danhhz/cargo-stress#6). I don't understand how we could be getting a UTF-8 decode error while defining a class. This could be a sign of some thread-safety issue in |
Hmm, we just had the same issue in https://github.com/PyO3/pyo3/actions/runs/11054116502/job/30710180455#step:9:5074 |
Ohhh, I get it, it's because |
Good point, I will try to fix that PR up next time I type a line of code! |
Ah, just realised #4584 - I've added a checkbox for |
really looking forward to this! |
#4298 might imply |
As per python/cpython#125243 (comment) I've added a bullet to the top for datetime bindings. |
We didn't have a dedicated issue for this, so now there's one.
TODO:
cfg
for no-gil, but only allowed behind an experimental featureffi-check
passing with a no-GIL buildPyDict_GetItemRef
PyList_GetItemRef
PyDict_Next
PyWeakref_GetRef
PyImport_AddModuleRef
Python<'_>
indicates only a single thread is executing:pyo3::sync::GILOnceCell
PyClassBorrowChecker
GILProtected
PyErrState::normalize
Py_mod_gil
slot should be setdatetime
bindings are not thread safe (?)The text was updated successfully, but these errors were encountered: