-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
prange/OpenMP issues under the free-threading CPython build #6198
Comments
I suggest we ignore 1 and (most of) 2 for now. They're future questions about adding new functionality rather than getting existing functionality to work. For now I'd let
My feeling is that there's basically 4 internal variables that Cython uses to keep trace of state inside a prange loop:
These are assumed to be guarded by the GIL and so we need to put some locking around those in free-threading mode. I think doing that would solve a large chunk of problems (not everything, but possibly everything specific to
refnanny is only an internal testing tool and is disabled while running user code. That means we could do something crude like:
|
I'd go with this option. Having a functional and correct refnanny will be useful in testing the free threading build and avoid introducing reference counting errors. Performance isn't a big issue, as long as the refnanny remains usable and doesn't become a blocker for CI. |
I've had a quick go in #6199. With that said it's untested because I don't have a free-threading build installed anywhere, so I don't expect it to work. |
I think to make things generally thread safe is going to be quite hard. Consider:
What outcome you get is obviously arbitrary (and that isn't Cython's problem), but to make the reference counting of I don't know how far we want to go here? Does it only apply PyObject* or do we need to make anything dealing with integers atomic too? That's a pretty drastic change. For now I'm tempted to add an import-time warning to any module with |
This is correct. The reference counting itself can be considered thread-safe at the CPython level, but the fact that this stores pointers in temporaries and then calls DECREF on them means that the pointers might have changed by the time we update the reference count. Also, locking inside the refnanny is probably not enough, right? If we have a shared variable that gets assigned the result of calling a C API that returns a strong reference and we call |
Locking inside of refnanny is only really intended to make the refnanny internal state consistent. I wasn't imagining that it made the references that it acts on thread-safe. Looking at something simple like
I'm fairly sure the temps (e.g. I think it's just |
I had a go at testing this, and couldn't get it to give me any refnanny errors unfortunately (I did manage to convince myself that it was running multiple threads and it was running without the GIL). So I don't know if my proposed changes make it better or worse. |
I think we still need |
Yes, this is true, although in that case the naming is a bit confusing. It's more like |
I definitely plan to add some documentation for the current state of the freethreading build. I think I'd like to wait for either the Python 3.13 release or a Cython release to do that (and wait to see what we're documenting). I think the main thing we're missing is a way to do gilstate-ensure once per thread rather than once per iteration. I imagine it's a bit expensive so we should eventually provide that. But I'd rather get existing features working first |
Describe your issue
Using multiple threads with
prange
and OpenMP leads to issues under the free-threading build. There's a few test failures, but the easier one to reproduce is this:Running this with the gil enabled is okay:
But, running it with the GIL disabled leads to issues:
There are multiple issues here that need discussion:
with cython.gil
line, Cython starts complaining about operations not being allowed without holding the GIL. Those errors should probably be removed in the context of the free-threading build.with cython.gil
altogether does not make much sense. What do we do in those cases? We continue to supportwith cython.gil
and let it be a no-op when the GIL is disabled? Do we warn against it? Is it an error?PyThreadState_Get
, which needs changing. We should be locking around it as well.The text was updated successfully, but these errors were encountered: