-
Notifications
You must be signed in to change notification settings - Fork 36
ENH: Free threading support #131
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
7a29b98
to
0f46bb7
Compare
Unlike #130, tests pass here https://github.com/bodono/scs-python/actions/runs/15374887139/job/43258511781?pr=1312 |
06fb4a8
to
0f46bb7
Compare
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.
Some comments inline - I think you probably want to use the critical section API rather than adding a mutex to the wrapper object since the wrapper object extends PyObject and you need to call into the C API while working with it. Otherwise you're going to need to refactor things so that you don't have to call into the C API while holding the per-object lock.
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.
Comments inline. I didn't read over the new tests since I'm not familiar with this library.
Pending on bodono#132
Assuming SCS is indeed reentrant
@@ -563,10 +568,16 @@ static int SCS_init(SCS *self, PyObject *args, PyObject *kwargs) { | |||
self->sol->s = (scs_float *)scs_calloc(self->m, sizeof(scs_float)); | |||
|
|||
/* release the GIL */ | |||
#ifdef Py_GIL_DISABLED |
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.
Does the GIL protect against the race you're worried about here? If not then you should lock on the GIL-enabled build too. It'd be nice to have an explicit test case for this fix if you don't already.
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 general idea here was to lock before Py_BEGIN_ALLOW_THREADS
.
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.
Since its the initialization call, yes, it is covered by tests :)
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.
Oh oops, I completely missed that this is happening inside SCS_init
. In that case locking probably isn't necessary, unless it's somehow possible for two threads to simultaneously see the same SCS
object before SCS_init
returns an initialized object.
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 still a bit unclear about this. The guarantee with the lock is that there can be no two python threads trying to call scs_init
(the external library) at the same time...
This is the general pattern of locks around calls to scs
.
Under the free threading build without the lock isn't it possible for two threads to try to create scs
objects at the same time thereby simultaneously calling into the external library? Or is the point that since the lock is on the object which hasn't been initialized yet it is meaningless anyway (or needs a global lock)?
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.
So we need to make a distinction between scs_init
and SCS_init
. The latter is the tp_init
slot for SCS_Type
- it's the function that we're inside of in this comment thread.
A Python object is effectively thread-local while it's under construction. Said another way, there's no way for another thread to have a handle on the SCS
instance before SCS_init
completes.
Thanks for taking this on! Let me know when you think it is ready to merge! |
Closes #130.
Local tests via:
Basically follows the porting extensions advice:
pythoncapi-compat
Get*
calls to useGet*Ref
here since they're uses of argument parsing (xref C-API documentation)EDIT: After the split into #132 and #133 to test this for now one needs to manually setup the pythoncapi-compat bit.