Repository navigation
[df] Release GIL around parallel event loop - #24148
SamdaniSayam wants to merge 1 commit into
Conversation
Pure-Python callables passed to Define/Filter deadlock when EnableImplicitMT() is used: the Python thread holds the GIL while waiting in TThreadExecutor, and worker threads block in PyGILState_Ensure when they call the Python callback. Release the GIL before entering the parallel loop (and reacquire it afterwards) so that worker threads can execute Python callables. Fixes root-project#24140
vepadulano
left a comment
vepadulano
left a comment
There was a problem hiding this comment.
Dear @SamdaniSayam ,
Thanks for taking interest in the project.
I was not able to run the issue's reproducer end-to-end here, so CI will be the first full verification.
Please, try to be more mindful when submitting PRs. Do not hope for the CI to be some sort of magic barrier.
See also comments inline
| // Python.h must come before any other headers, it must be included before any standard header. | ||
| #if defined(R__USE_IMT) && defined(R__HAS_PYTHON_GIL) | ||
| #include <Python.h> | ||
| #endif |
There was a problem hiding this comment.
This is absolutely a no-go, without even looking further, it would mean that the RDataFrame library has a build-time dependency on the Python system headers
There was a problem hiding this comment.
Ok I looked until the end of the code changes and I can see what I feared in CMakeLists.txt
| #ifdef R__HAS_PYTHON_GIL | ||
| // The calling Python thread holds the GIL while waiting in TThreadExecutor. | ||
| // Release it so that worker threads can invoke Python callables without deadlock. | ||
| PyThreadState *gilSave = nullptr; | ||
| bool const hasGIL = Py_IsInitialized() && PyGILState_Check(); | ||
| if (hasGIL) | ||
| gilSave = PyEval_SaveThread(); |
There was a problem hiding this comment.
None of this can go in RDataFrame, at all. Usually these type of changes are delegated to the Python bindings, although in this particular case I question if they could even go there.
|
|
||
| #include "RConfigure.h" // R__USE_IMT | ||
|
|
||
| // Python.h must come before any other headers, it must be included before any standard header. |
There was a problem hiding this comment.
Why? This sentence given without context underlines a lack of understanding of the problem.
In RDataFrame, when Python callables are used inside
Define/FilterandROOT.EnableImplicitMT()is active, the main Python thread holds the GIL while waiting inTThreadExecutor::ParallelFor. TBB worker threads therefore block forever inPyGILState_Ensurewhen invoking the Python callback. The event loop never completes, as reported in #24140.This PR releases the GIL before entering the parallel loop (
RunEmptySourceMT,ROOT::Internal::RDF::ProcessMT) and reacquires it afterwards.RDF::RDF::RunGraphsis adjusted the same way for the case where multiple unique computation graphs are triggered in parallel from Python. In a pure C++ context the release is a no-op (the current thread does not hold the GIL).Validation
AI disclosure
This fix was drafted with LLM assistance and has been thoroughly reviewed and understood by me. I take full responsibility for the change, as required by CONTRIBUTING.md.
Fixes #24140