[PyROOT] Fix a GIL deadlock in THx.Fit #21129
Conversation
hageboeck
requested review from
bellenot,
guitargeek and
vepadulano
as code owners
|
Nice, and compute intense functions like these is exactly when the GIL should be released to enable Python multithreading. Did you check the Pythonization also works for TH1-derived classes? |
We checked in the interpreter: >>> import ROOT
>>> ROOT.TH2.Fit.__release_gil__
True |
Test Results 22 files 22 suites 3d 21h 25m 16s ⏱️ Results for commit 2232fb9. ♻️ This comment has been updated with latest results. |
|
I've taken a look at the MacOS failures. There seems to be some other kind of interference on that platform for two different types of functions that release the GIL, namely the distributed RDataFrame execution and now TH1.Fit. Needs investigation |
vepadulano
left a comment
vepadulano
left a comment
There was a problem hiding this comment.
Thank you @siliataider for investigating and fixing the failures on MacOS!
The changes LGTM, maybe consider the ruff format suggestions.
|
Some of the |
TH1.Fit might call into Python functions, which would require the GIL to be held. If TH1.Fit holds it, though, the process deadlocks. Here, the GIL is released before TH1.Fit, which runs fully in C++ anyway. Fix root-project#21080.
This saves startup times and collects TH1 tests in one place.
The timeout of the test is reduced because the deadlock would otherwise show only after 1500s.
hageboeck
force-pushed
the
TH1_GIL_problem
branch
from
0319cda to
2232fb9
Compare
|
Thanks for this fix! maybe something to |
|
/backport to 6.38 |
|
/backport to 6.38 |
|
This PR has been backported to branch 6.38: |
|
/backport to 6.38 |
1 similar comment
|
/backport to 6.38 |
|
This PR has been backported to branch 6.38: #21198 |
6 participants
TH1.Fit might call into Python functions, which would require the GIL to
be held. If TH1.Fit holds it, though, the process deadlocks.
Here, the GIL is released before TH1.Fit, which runs fully in C++
anyway.
A test was added with a 30s timeout. It completes in about 1s, unless the
GIL deadlock is maintained.
Fix #21080.