feat(pybind): release the GIL around expensive linalg/contract calls#987
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #987 +/- ##
==========================================
+ Coverage 32.81% 32.99% +0.17%
==========================================
Files 230 230
Lines 33039 33069 +30
Branches 13769 13781 +12
==========================================
+ Hits 10841 10910 +69
+ Misses 14904 14795 -109
- Partials 7294 7364 +70
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces GIL-release guards (py::call_guard<py::gil_scoped_release>) to several computationally expensive C++ bindings in the pybind/ layer, including SVD, Lanczos, Arnoldi, and tensor contraction operations, allowing other Python threads to run concurrently. It also adds a test suite in pytests/binding_gil_test.py to verify GIL release and callback functionality. The reviewer suggests dynamically scaling the matrix size in the SVD GIL-release test to prevent flaky CI failures on fast machines where a fixed 900x900 matrix might compute too quickly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| a = cytnx.random.normal([900, 900], mean=0.0, std=1.0) | ||
|
|
||
| # Measure a single serial Svd call to calibrate the "GIL held too long" | ||
| # threshold to this machine's speed instead of a brittle fixed constant. | ||
| serial_duration = _time_one_svd(a) | ||
| # If a single call is not clearly above the 0.2s jitter floor, the test | ||
| # would pass vacuously even with the GIL held; force a resize instead. | ||
| assert serial_duration > 0.25, ( | ||
| "matrix too small to discriminate GIL hold from floor" | ||
| ) |
There was a problem hiding this comment.
On highly optimized BLAS/LAPACK configurations (such as multi-threaded Intel MKL or Apple Accelerate on modern hardware), a 900x900 SVD can execute extremely quickly (well under 0.25 seconds). This will cause the assertion assert serial_duration > 0.25 to fail, leading to flaky or broken CI builds on fast machines.
To make the test robust across all hardware and BLAS configurations, we should dynamically scale the matrix size until the SVD takes longer than 0.25 seconds.
# Dynamically find a matrix size that takes long enough (> 0.25s) on this machine
# to reliably distinguish GIL release from scheduling jitter.
size = 600
a = None
serial_duration = 0.0
while size <= 2400:
a = cytnx.random.normal([size, size], mean=0.0, std=1.0)
serial_duration = _time_one_svd(a)
if serial_duration > 0.25:
break
size += 300
assert serial_duration > 0.25, (
f"Even at {size}x{size}, SVD took only {serial_duration:.3f}s, "
"which is too small to discriminate GIL hold from floor."
)There was a problem hiding this comment.
Addressed in 9553e00d. The test no longer hard-asserts on a fixed size — it scales the matrix up (1.5x per step from 900, capped at 2000) until a single serial Svd clears a 0.3s discrimination floor. It differs from the suggestion in one way: instead of a hard assert when even the largest size stays under the floor, it pytest.skip()s ("machine too fast to discriminate"), so a very fast LAPACK backend yields a clean skip rather than a red build. Since Svd is O(n^3), one 1.5x bump is ~3.4x the work, so the fast runner that measured 0.22s at 900 clears the floor at 1350. Fresh CI is green everywhere, including BuildAndTest running this test.
Code ReviewReleases the GIL ( Correctness — verifiedGIL release is only safe under specific conditions; each is handled:
Notes
CI statusThe current red checks are stale, not a code failure: the fast Linux failures ( Verdict: correct, well-reasoned, well-documented concurrency win — pure-C++ guarded bodies, the Posted by Claude Code on behalf of @pcchen |
c07f1fe to
fae306d
Compare
CI update: the one red job is a flaky test threshold, not a GIL bugFresh CI after the rebase is green everywhere except one job — The test times a single serial 900×900 So the GIL-release change itself looks correct; the new test is just environment-sensitive — 900×900 isn't reliably above the 0.25s floor on fast runners. Suggest hardening the calibration before merge:
Low-severity (test-only), but a test that fails CI intermittently shouldn't merge as-is. Everything else (code review above) stands: LGTM on the GIL fix once the calibration is made robust. Posted by Claude Code on behalf of @pcchen |
|
Pushed a fix for the flaky calibration in Instead of hard-asserting that a single serial Svd on a fixed 900×900 matrix exceeds 0.25s (which failed on a fast runner at 0.22s), the test now scales the matrix up (1.5× dim per step, capped at 2000) until the serial Svd clears a 0.3s discrimination floor, and Couldn't run it locally (needs a full cytnx build), so it's syntax-checked and relies on the fresh CI to confirm — the discrimination logic is straightforward. The GIL-release code review above is unchanged (LGTM). Posted by Claude Code on behalf of @pcchen |
Long-running C++ compute (Svd/Rsvd/Hosvd, Eigh/Eig, Qr/Qdr, InvM/InvM_, Lanczos/Arnoldi, Tensordot/Kron/Matmul/Dot/Gemm/Det/Directsum/Lstsq, UniTensor Contract(s)/.contract, Network.Launch) used to hold the GIL for the whole call, blocking every other Python thread. Each of these bindings now carries py::call_guard<py::gil_scoped_release>(), constructed only around the already-materialized C++ invocation (call_guard's semantics: guard is constructed after argument conversion, per pybind11/attr.h "T scope_guard; return foo(args...)"), so Python objects are never touched while the GIL is released. Scope: all O(n^3)-class linalg entry points EXCEPT ExpH/ExpM (rewritten by open PR #915; guards there would conflict). The elementwise operator sweep (tensor_py.cpp arithmetic) is deliberately deferred. A guard discipline comment block above the first guard in linalg_py.cpp documents the safety rules (pure-C++ bodies only; left-to-right guard composition vs scoped_ostream_redirect; trampoline reacquisition); network_py.cpp and unitensor_py.cpp guard sites point to it. Guarded bindings (45 sites in linalg_py.cpp, 4 in unitensor_py.cpp, 1 in network_py.cpp): - linalg_py.cpp: Svd (x2), Gesvd (x2), Rsvd (x3), Gesvd_truncate (x3), Svd_truncate (x3), Eigh (x2), Eig (x2), Qr (x2), Qdr (x2), InvM (x2), InvM_ (x2), Matmul, Matmul_dg, Det, Tensordot, Tensordot_dg, Kron, Dot, Gemm, Gemm_, Directsum, Hosvd (x2), Lstsq, Arnoldi (x2), Lanczos (x4), Lanczos_Exp. - unitensor_py.cpp: UniTensor.contract method, free functions Contract (x2 overloads) and Contracts. - network_py.cpp: Network.Launch. The Rsvd/Hosvd/Lstsq/InvM/InvM_/Matmul_dg/Tensordot_dg/Gemm_ additions came from a review-consistency sweep (Gemm was guarded but Gemm_ was not, etc.); each lambda body was re-audited as pure C++ before guarding (the only non-trivial case is Rsvd's seed defaulting through cytnx::random::__static_random_device, a plain std::random_device - include/random.hpp:25 - with no CPython involvement). Skipped per plan (not touched, to avoid conflict with open PR #915): ExpH/ExpM (all overloads) - left untouched. Not present in this codebase, so not added: Gemm_Batch (no such binding/overload exists in linalg_py.cpp); Lanczos_Gnd as a separate top-level binding (only reachable through the "Gnd" method string on the existing Lanczos overloads, which are already guarded); the Lanczos_ER/Lanczos_Gnd/Lanczos_Gnd_Ut bindings are commented out (dead code) in linalg_py.cpp and were left as-is. Ostream-redirect caution (investigated, does not apply here): none of the guarded bindings carry a py::scoped_ostream_redirect/ scoped_estream_redirect call_guard, so there is no risk of gil_scoped_release racing a redirect's construction in this change. For completeness, verified that IF such a combination were ever needed, writes would in fact be safe: pybind11's pythonbuf::_sync() (build_py/_deps/pybind11_sources-src/include/pybind11/iostream.h:96, inside the `int _sync()` override used by both overflow() and the destructor) opens with `gil_scoped_acquire tmp;` before touching any Python object, so C++ writes to a redirected stream reacquire the GIL themselves. The *construction* of scoped_ostream_redirect itself (iostream.h:166-171) does touch a Python object (sys.stdout) via a default argument evaluated at call time in the guard's default constructor, which is why call_guard's left-to-right construction order matters if such a combination is ever introduced - gil_scoped_release must not be constructed first in the chain, or that default-arg Python attribute lookup would execute without the GIL. This rule is now captured in the in-code discipline comment. PYBIND11_OVERLOAD trampoline safety (verified): PyLinOp::matvec (pybind/linop_py.cpp:29-42) uses PYBIND11_OVERLOAD, which expands via PYBIND11_OVERRIDE -> PYBIND11_OVERRIDE_NAME -> PYBIND11_OVERRIDE_IMPL (build_py/_deps/pybind11_sources-src/include/pybind11/pybind11.h:3530-3547, see also the PYBIND11_OVERLOAD macro itself at line 3640-3641). Line 3532 opens the macro body with `pybind11::gil_scoped_acquire gil;` before calling get_override() and invoking the Python override. This means a Python-subclassed LinOp's matvec() can be safely invoked by the solver even while the surrounding Lanczos/Arnoldi call has released the GIL - the trampoline reacquires it for the duration of the Python callback. This justifies guarding the LinOp-taking solvers. Tests: pytests/binding_gil_test.py - test_svd_releases_gil: measures a serial single-Svd duration in-test and asserts the max ticker gap during 3 concurrent Svd(900x900) calls stays under max(0.2s, 0.5 * serial_duration), which is robust across machine speeds (a fixed constant threshold was unstable: on this machine a 400x400 Svd only takes ~0.08s, under the 0.2s floor, so the matrix had to be sized up to ~900x900 to get a single-call duration long enough to make GIL-holding observably different from GIL-releasing; confirmed red on the baseline build, max_gap (~0.645s) matched the measured serial duration (~0.602s) almost exactly, i.e. the GIL was fully held). Guards against vacuous passes on faster future hardware via an explicit serial_duration > 0.25s precondition assert. The ticker thread is daemon=True and stopped in a try/finally so a raising Svd cannot leak a thread that would hang the run and mask the real error. - test_lanczos_with_python_linop_still_works: precondition smoke test that python LinOp subclassing + matvec() override works at all (no solver involved). - test_lanczos_gnd_with_python_linop: a cheap (<1s) real cytnx.linalg.Lanczos(method="Gnd") call through a python-subclassed LinOp, mirroring example/DMRG/dmrg_two_sites_dense.py's Hxx pattern, exercising the guarded Lanczos binding with the trampoline callback under a released GIL. Full pytest suite: 28 baseline + 3 new = 31 passed, 0 failed. Co-Authored-By: Claude <noreply@anthropic.com>
…bration test_svd_releases_gil hard-required a single serial Svd on a fixed 900x900 matrix to exceed 0.25s so the ticker-starvation signal is discriminating. On fast LAPACK backends a 900x900 Svd can finish in ~0.22s, tripping the calibration assert (observed on CI: 0.2198s > 0.25 -> AssertionError) even though the GIL-release behavior itself is correct. Scale the matrix up (1.5x dim per step, capped at 2000) until the serial Svd clears a 0.3s discrimination floor, and pytest.skip() if even the cap stays under the floor (machine too fast to discriminate) rather than failing. Keeps the assertion meaningful across fast/slow runners without a brittle fixed size + threshold. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9553e00 to
99ad0a1
Compare
pcchen
left a comment
pcchen
left a comment
There was a problem hiding this comment.
Approving. The GIL-release change is verified safe: every guarded site is a function-pointer binding or pure-C++ lambda (argument conversion before the guard, return conversion after), no site composes scoped_ostream_redirect with the release (and none dropped an existing redirect), and the Lanczos/Arnoldi Python-callback path is safe via PYBIND11_OVERRIDE's gil_scoped_acquire — pinned by the end-to-end Lanczos-through-a-Python-LinOp test. The in-code guard-discipline note is a valuable durable artifact.
The one CI failure along the way was the PR's own calibration test being environment-sensitive (0.22s Svd on a fast runner vs a 0.25s hard-assert); the adaptive-scaling fix (grow to a 0.3s discrimination floor, skip if the machine is too fast) is now CI-validated — current head is rebased on master with 16/16 checks green, including BuildAndTest running the new binding_gil_test.py.
Posted by Claude Code on behalf of @pcchen
2 participants
Problem
No binding released the GIL, so any long-running call (SVD, eigensolver, contraction,
Network.Launch) blocked every other Python thread for its full duration.Fix
py::call_guard<py::gil_scoped_release>()on 50 binding sites: all O(n³)-class linalg entry points (Svd/Gesvd/±truncate, Rsvd, Eigh/Eig, Qr/Qdr, Hosvd, Lstsq, InvM/InvM_, Matmul(+_dg), Tensordot(+dg), Kron, Dot, Gemm/Gemm, Directsum, Det, Arnoldi, Lanczos, Lanczos_Exp),Contract/Contracts/.contract, andNetwork.Launch. ExpH/ExpM are deliberately excluded — open PR #915 rewrites those bindings; guarding them here would conflict.Safety was verified against the fetched pybind11 3.0.1 source, and the rules are recorded in an in-code discipline note at the first guard site: guarded lambda bodies are pure C++ (argument conversion happens before the guard, return conversion after it);
py::scoped_ostream_redirectneeds the GIL so guard composition order matters; PythonLinOp.matveccallbacks are safe becausePYBIND11_OVERRIDEreacquires the GIL (verified at pybind11.h:3532) — pinned by an end-to-end test running a real Lanczos ground-state solve through a Python-subclassedLinOp.Testing
New
pytests/binding_gil_test.py: a self-calibrating threading test (asserts a ticker thread keeps running during Svd; red on the unguarded build with gap ≈ full Svd duration), a LinOp smoke test, and the Lanczos-through-Python-LinOp integration test. Elementwise/Tensor-operator sweep deliberately deferred (documented in the in-code note).🤖 Generated with Claude Code