[RF] Delete fit objects before continuing in rs401d_FeldmanCousins.py#22853
[RF] Delete fit objects before continuing in rs401d_FeldmanCousins.py#22853guitargeek wants to merge 1 commit into
Conversation
Similar to 49fb353. The Python version of this tutorial sporadically crashes on CI with the upcoming cppyy upgrade. It is the same overlapping-heap-ownership problem that was worked around for `rf619_discrete_profiling.py` in 49fb353. The nll/pll pair used for the likelihood surface in pad 3 is not needed after that plot. Deleting it right there tears down the NLL evaluation machinery at a well-defined point instead of at interpreter shutdown, where the order of cleanups is less controlled. With this change the previously deterministic poisoned repro (4/4 crashing) passes 3/3, and plain and ctest invocations pass as well.
Test Results 22 files 22 suites 3d 13h 17m 6s ⏱️ For more details on these failures, see this check. Results for commit 1f829f4. |
| # The profile likelihood and the NLL are not needed anymore. Delete them | ||
| # already here, so that the NLL evaluation machinery is not torn down at | ||
| # interpreter shutdown, where the order of cleanups is less controlled. | ||
| del pll | ||
| del nll |
There was a problem hiding this comment.
Not to block this PR, but in general it's weird/a code smell to have to use del in Python at determined places. Do we have an idea of how this particular scenario could be fixed with e.g. appropriate Pythonizations or changes in the bindings layer?
There was a problem hiding this comment.
It's hard to investigate, because the crash only happens in the cppyy upgrade PR. I guess some different destruction ordering there turns out to be catastrophic for RooFit.
My plan was to first make sure that this works for old and new cppyy by making the destruction order more deterministic with this PR, and then at a later point revisit the dels in the tutorial when there is time.
2 participants
Similar to 49fb353.
The Python version of this tutorial sporadically crashes on CI with the upcoming cppyy upgrade. It is the same overlapping-heap-ownership problem that was worked around for
rf619_discrete_profiling.pyin 49fb353.The nll/pll pair used for the likelihood surface in pad 3 is not needed after that plot. Deleting it right there tears down the NLL evaluation machinery at a well-defined point instead of at interpreter shutdown, where the order of cleanups is less controlled. With this change the previously deterministic poisoned repro (4/4 crashing) passes 3/3, and plain and ctest invocations pass as well.