Acquire the GIL in lazy init_extension - #3255
Conversation
Greptile SummaryThis PR fixes a segfault that occurs when a binding registered with
Confidence Score: 5/5Safe to merge — the one-line change correctly acquires the GIL before call_once, fixing the reported segfault and maintaining a consistent lock order across all callers. The change is minimal and mechanically correct: placing gil_scoped_acquire before call_once ensures every thread establishes the same lock order (GIL first, then the call_once internal mutex), eliminating the original segfault. No data races or ordering inversions are introduced. Files Needing Attention: No files require special attention; the single changed function in pybind.cpp is straightforward. Important Files Changed
Sequence DiagramsequenceDiagram
participant PY as Python caller
participant DIS as pybind11 dispatcher
participant FN as fp8_transpose
participant INIT as init_extension
participant ONCE as call_once initializer
PY->>DIS: "tex.fp8_transpose(...)"
note over DIS: "call_guard drops GIL"
DIS->>FN: "(GIL released)"
FN->>INIT: "init_extension()"
INIT->>INIT: "gil_scoped_acquire: re-acquire GIL"
INIT->>ONCE: "std::call_once(flag, lambda)"
alt "First call: flag not set"
ONCE->>ONCE: "import Python modules (needs GIL)"
ONCE-->>INIT: "flag set"
else "Subsequent calls: flag already set"
ONCE-->>INIT: "no-op"
end
INIT->>INIT: "~gil_scoped_acquire: release GIL"
INIT-->>FN: return
FN-->>PY: result
Reviews (3): Last reviewed commit: "Update transformer_engine/pytorch/csrc/e..." | Re-trigger Greptile |
Bindings declared with call_guard<gil_scoped_release> (e.g. fp8_transpose) can be the first transformer_engine_torch call in a process. init_extension() then runs py::module_::import without holding the GIL and segfaults inside PyImport_ImportModule. Reacquire the GIL inside the one-time initializer. Signed-off-by: zhihaow6 <zhihaow6@illinois.edu>
af6afe3 to
ce243b2
Compare
|
Actually, I agree with the Greptile review. @xiuhu17, could you make the suggested change? |
Waiting for the suggested changes to be implemented.
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: Zhihao Wang <101526713+xiuhu17@users.noreply.github.com>
|
/te-ci pytorch |
|
Yes, merged. Thank you for your contribution:-)! |
|
@ptrendx Thanks for reviewing! |
Description
init_extension()imports Python modules viapy::module_::import, which requires the GIL. Several bindings that can trigger this lazy initialization are declared withcall_guard<py::gil_scoped_release>— if one of them is the firsttransformer_engine_torchcall in the process, initialization runs without the GIL and segfaults. Typical runs don't hit this because a GIL-holding call (e.g. a quantize) usually comes first.Deterministic repro on main:
Running any GIL-holding tex call first (e.g.
tex.quantize) and thenfp8_transposeworks, confirming the GIL state is the only difference.Type of change
Changes
init_extension()'sstd::call_onceinitializer before importing Python modules.Checklist: