Fix Python module thread model to allow for reusing interpreters #26857
Conversation
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
| entirety of each task, these examples will be no faster than running the tasks | ||
| serially. | ||
| Although these examples use Chapel's task parallelism constructs, | ||
| they will be no faster than running the tasks serially due to the GIL. |
There was a problem hiding this comment.
Does acquiring the GIL slow down non-Python code? My expectation would be that interleaving Python calls with regular Chapel code will be okay until the Python code segments are reached, at which point they will behave like a barrier was used. Do we know if that's the case?
There was a problem hiding this comment.
No, it should only effect Python code. So this statement of "no faster than running serially" is mostly about these specific examples, where the only work done in the loop is the Python function call
| PySequence_SetItem(this.getPyObject(), idx.safeCast(Py_ssize_t), pyItem); | ||
| this.check(); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| // TODO: now that we handle this on every call to the python interpreter, | ||
| // is this still needed as a user facing type? |
There was a problem hiding this comment.
I would argue no. But I've of two minds whether I think it should be handled in this PR:
- Pros:
- All the related changes are in one place, which makes it easier to see the link between actions. If we do another full overhaul, we'll know to look at this type again if we start by looking at this PR
- Cons:
- If we do end up backing away from this change, we might not remember to revisit whether this type should be public again, so users might not have documentation for any strategy.
There was a problem hiding this comment.
Seeing that fromPickle still needs the GIL makes me lean more towards "don't hide it yet". But that probably means we should update the documentation for it to point back to the earlier docs about the more recommended strategy
There was a problem hiding this comment.
fromPickle only needs it because its an internal function that is meant to be called with the GIL already held. Those tests are testing undocumented/private API details.
But I take your point that we should not hide it yet, which is also my inclination.
I'll note that GIL can be used freely by users if they so choose, threadState is likely to crash their program
Signed-off-by: Jade Abraham <[email protected]>
|
Woohoo! That was a tough nut to crack, congrats on landing it (and by feature freeze as well!) |
|
(And thanks to @mppf for the assist yesterday!) |
Fixes an issue where using a Python interpreter in disjoint
onblocks would segfault.The use case fixed by this PR is captured in the new test,
test/library/packages/Python/correctness/reuseInterpreter.chpl. This test creates a block distributed array of interpreters in one function and then uses them in another function. Prior to this PR, this would segfault due to the following scenario. I have simplified to a single localeinitPythonInterpreterscreates a new comm task and initializes the Python interpreter in that thread.distributedApplycreates a separate comm task and tries to use the Python interpreter.This problem can be fixed by following https://docs.python.org/3/c-api/init.html#non-python-created-threads. At interpreter initialization, we must call
PyGILState_EnsureandPyEval_SaveThread. At interpreter finalization, we must callPyEval_RestoreThreadandPyGILState_Release. Then, all Python operations in between must be wrapped in matching calls toPyGILState_[Ensure|Release].However, implementing this then introduces a new problem, the Python interpreter must be started and stopped from the same thread (see https://docs.python.org/3/c-api/init.html#c.Py_FinalizeEx). This is solved in this PR by only starting and stopping the Python interpreter a daemon thread inside the
Interpreter's init. Note that due to #26855, this is implemented as apthread, not abegin.Since all Python operations as now wrapped in calls to the GIL, for the most part user code no longer needs to manually handle the GIL. This PR updates the examples and docs accordingly. Future work should investigate if we should still expose the
GILandthreadStateobjects.start_test test/library/packages/Pythonon Mac with Python 3.13start_test test/library/packages/Pythonon Mac with Python 3.9start_test test/library/packages/Pythonon Linux with Python 3.10[reviewed by @lydia-duncan and @mppf]