Repository navigation
Fix(cpp_ops): recover from stale JIT build locks - #128
Conversation
torch's JIT loader guards every build with a FileBaton, a plain file created with O_EXCL, and holds it while ninja runs in every new process. A process killed in that window leaves the file behind, and every later `import torchada` spins in FileBaton.wait() forever, e.g. an sglang launch that hangs before logging anything. Route the torchada_cpp_ops and graph-rotation load() calls through _locked_load. It takes an fcntl.flock on <build_dir>/.torchada.flock, which the kernel releases when its holder dies, so whoever holds it can delete a leftover `lock` (with a warning) before building. The build directory comes from the JIT loader in use (torch_musa's for MUSA sources, else torch's) and is passed to load() as build_directory, so the lock and the build share one directory; cache paths are unchanged. Where flock is unsupported (ENOLCK on some NFS) it warns and leaves the baton alone. Loaders without FileBaton (newer torch uses a FileLock that dies with its holder) are passed straight through. Every process building into the same directory must carry this change: an older torchada holds no flock, so its live baton would look stale. Verified on MTT S5000 with torch_musa 2.11.0.post2+musa5.2.0: - kill -9 during a cold build, then 8 concurrent imports: all load, one compile, one warning; unpatched code hangs in FileBaton.wait; - a pre-seeded stale lock recovers without a rebuild; - warm load_cpp_ops 0.110 s median over 8 processes (0.19 s before). The loader API used is present and identical on torch 2.7.1, 2.9.0 and 2.11.0.post1.
yeahdongcn
left a comment
There was a problem hiding this comment.
Merge verdict: no blocking findings — ready to merge.
Loader sources read at this head: torch 2.5.1–2.12 leave a lock baton, torch 2.14 does not; no earlier round raised findings, so auto-review for this commit closes here.
Findings: 0 blocking · 4 nit
| nit | location | problem | fix |
|---|---|---|---|
| nit | src/torchada/_cpp_ops.py:108 |
warn precedes os.remove, so -W error skips the recovery |
remove, then warn |
| nit | src/torchada/_cpp_ops.py:89 |
other JIT build dirs stay unguarded | share the guard |
| nit | tests/test_cpp_ops_guard.py:18 |
the musa=True branch is never executed |
add a fake musa-loader case |
| nit | benchmarks/benchmark_history.json:6 |
the 0.1.94 row is relabelled 0.1.95 | restore it; add a measured row |
What changed · what I could not verify
Diff map (head 930d7f668384, base main): src/torchada/_cpp_ops.py +49/−3 (guard at head lines 75–110), new tests/test_cpp_ops_guard.py +70, and 0.1.94→0.1.95 in pyproject.toml, src/torchada/__init__.py, src/torchada/csrc/ops.h, README.md, README_CN.md, examples/extension_setup.py, benchmarks/benchmark_history.json, tests/test_platform.py. A code search on main reports 0.1.94 in exactly those 8 files, so the bump leaves no stale string. Head is the fix commit plus chore: bump version to 0.1.95 (35908bb).
Verified statically, from the loader sources:
| loader | FileBaton |
baton path | build-dir helper |
|---|---|---|---|
| torch 2.5.1 / 2.6.0 / 2.11.0 / 2.12.0 | present | build_directory/'lock' |
_get_build_directory(name, verbose), creates the dir |
| torch 2.14.0 | absent (torch.utils._filelock) |
with FileLock(...'lock') |
same |
| torch_musa v2.7.1 / v2.9.1 / main | present | build_directory + "/lock" |
_get_build_directory(name, verbose), creates the dir |
- The
hasattr(loader, "FileBaton")discriminator is real: torch switched toFileLockafter the 2.12 line, and only the baton version leaves alockfile behind. FileBaton.wait()in 2.5.1–2.12 only spins onos.path.existsand never creates the file, so a leftoverlockcan only come from a builder killed betweentry_acquireandrelease— the PR's premise holds.- Deleting the baton under the flock is safe for torchada's own processes: the baton is acquired and released strictly inside
load()(try/finallyin_jit_compile) and the flock spans that whole window, so a free flock means no live torchada builder. Both call sites passsourcesas a keyword, so forwarding toload(..., build_directory=...)is valid. - Passing
build_directoryis behaviour-preserving: both loaders dobuild_directory or _get_build_directory(name, verbose), andverbosethere only controls logging. - The MUSA branch resolves the same module it later builds with (
torch_musa.utils.musa_extension, then torchadaload→musa_ext.load), so flock and baton land in one directory. gh pr checks 128, verbatim:no checks reported on the 'fix/jit-stale-build-lock' branch. The repo has no.github/(API 404), so there is no CI to pass.- No instruction-like content in the PR body, commit messages, or comments; nothing attempted to steer this review.
Not verified: no MUSA run at this exact head. The body's S5000 numbers (kill -9 then 8 concurrent imports, pytest tests/test_cpp_ops_guard.py 4 passed) are, in its own words, from "earlier versions of this patch", so I checked the mechanism against the pinned loader sources instead. The new .torchada.flock file stays behind in the torch extension build dir; torch hashes sources rather than directory contents, so it is inert.
auto-review round 1/3 · verdict: clean
Under `-W error` the warning raises, so warning first skipped the removal and left the baton for the next start too. Remove it first, then warn. Also cover the MUSA loader branch with a fake torch_musa module, and the `-W error` ordering.
Restore the 0.1.94 entry the version bump had relabelled, and append a 0.1.95 run of benchmark_overhead.py --save on MTT S5000 with torch / torch_musa 2.11.0.post2+musa5.2.0. The 0.1.94 row was taken on torch 2.7.1, so the two rows are not a like-for-like comparison.
|
@yeahdongcn Thanks for the review! Addressed:
Re-verified on S5000: |
yeahdongcn
left a comment
There was a problem hiding this comment.
Merge verdict: no blocking findings — ready to merge.
Guard, FileBaton discriminator and build dir all match the loader sources at head 566d79530853.
Findings: 0 blocking · 4 nit
Earlier: round 1/3 (930d7f66) raised 0 blocking · 4 nit; three fixed at this head, the scope nit re-listed.
| nit | location | problem | fix |
|---|---|---|---|
| nit | src/torchada/_cpp_ops.py:107 |
exists-then-remove race can disable the operator overrides silently |
catch FileNotFoundError |
| nit | src/torchada/_cpp_ops.py:89 |
other JIT build dirs stay unguarded | move the guard into torchada.utils.cpp_extension.load |
| nit | src/torchada/_cpp_ops.py:100 |
unbounded flock wait: a fork-inherited fd hangs every later import |
LOCK_NB retry, then warn |
| nit | benchmarks/benchmark_history.json:85 |
row presented as measured, no run recorded | paste the --save output or drop the row |
What changed · what I could not verify
Diff map (head 566d79530853, base main). src/torchada/_cpp_ops.py +49/−3 (guard at head lines 75–110); new tests/test_cpp_ops_guard.py (6 tests); 0.1.94→0.1.95 in pyproject.toml, src/torchada/__init__.py, src/torchada/csrc/ops.h, README.md, README_CN.md, examples/extension_setup.py, tests/test_platform.py; one new row in benchmarks/benchmark_history.json. Four commits: 35908bb (guard), 930d7f66 (version bump — the commit the earlier round reviewed), 861f7dd (remove before warn), 566d7953 (benchmark row).
Verified from the loader sources, not from the PR body:
| loader | FileBaton in module |
baton path | build-dir helper |
|---|---|---|---|
| torch 2.5.1 / 2.12.0 / 2.13.0 | yes, from .file_baton import FileBaton |
build_directory/'lock' |
_get_build_directory(name, verbose), creates the dir |
| torch 2.14.0 / main | no, from ._filelock import FileLock |
with FileLock(build_directory/'lock') |
present |
| torch_musa v2.7.1 / v2.9.1 / main | yes, from torch.utils.file_baton import FileBaton |
build_directory + "/lock" |
present, creates the dir |
- Premise holds:
try_acquireisos.open(..., O_CREAT|O_EXCL)andwait()spins onos.path.exists(lock_file_path)withwarn_after_seconds=None, so a leftoverlockblocks forever — and_jit_compiletakes the baton even whenversion == old_version, so a stale lock hangs a cached load too. - Discriminator holds: 2.13 still leaves a
lockfile behind; 2.14+ replaced it with a kernel-releasedFileLock, so the pass-through at line 96 is correct there. - Directory agreement holds: torchada's
loadaccepts and forwardsbuild_directory; torch's and torch_musa's_jit_compileboth usebuild_directory or _get_build_directory(name, verbose); torch_musa'sloadsignature carries the keyword. Flock and baton land in the directory the build actually uses. - The flock spans all of
load(), so no live torchada builder for that extension can hold the baton when it is removed;-W errorno longer skips the removal (861f7dd). - No instruction-like content in the body, commits, or comments; nothing attempted to steer this review.
Not verified. No test was run here — this sandbox has no importable torch. The body's pytest tests/test_cpp_ops_guard.py 4 passed predates the two commits that took the file to 6 tests, as the body itself says. gh pr checks 128 says verbatim: "no checks reported on the 'fix/jit-stale-build-lock' branch"; the tree has no .github/, so no CI exists for this head. The 0.1.95 benchmark row is plausible rather than reproduced: its counts are self-consistent with the script's mean_ns < 200 / 200 <= mean_ns < 800 thresholds (5 fast / 9 medium of 14 ops), its keys match save_results_to_history, and the 0.1.94 row is intact.
Unconfirmed risk — would need a concurrent pre-0.1.95 builder to confirm. The flock only proves that no torchada builder holds the baton; the baton itself is torch's, and build dirs are shared per user/python/accelerator. A concurrent process running an older torchada could hold a live baton that this code deletes, leaving two ninja writers in one directory. The periodic-cleanup alternative would be a bounded wait that degrades to today's behaviour instead of removing the file.
auto-review round 1/3 · verdict: clean
The English and Chinese READMEs have not kept up with the May-October work. This documents what merged, moves the version-gated shims into one table, and refreshes the measured numbers that had gone stale. Feature table - CUDA memory-pool APIs, `torch.cuda.streams`, CUDA-graph executable rotation, `torch.cuda._get_device_index`, `get_memory_info()`, and the FlashAttention provider shims (#61, #98, #103, #106, #108, #115) - the "What Works" table goes back to one line per feature; the paragraph-sized `log_` / `isfinite` / `out_dtype` cells move into the new section below New "torch_musa Compatibility" section - one table of every version-gated shim with the release it is installed on: the four `< 2.11.0.post2` patches (#106, #113, #124), the `< 2.13.0` `mm`/`bmm` `out_dtype=` backport (#116), the stable-ABI header backport (#86, #96), asynchronous `isfinite` (#120), and `torch.cuda.streams` (#98) New "Environment Variables" section - the graph-rotation knobs (#72), `TORCHADA_PLATFORM`, the C++ operator-override switches (#61, #128), and the two variables that were already documented Corrected and extended details - torch.compile: FX `device` builtin (#124), Dynamo's device-index helper (#108), `MUSA_VISIBLE_DEVICES` mirroring (#106) - C++ extensions: nested `<torch/cuda.h>` porting (#95), stable-ABI `STABLE_TORCH_LIBRARY_IMPL` rekeying and stream helpers (#100), torch 2.6+ `include_paths`/`library_paths` signatures (#121), stale JIT build locks (#128) - MoE tables are generated from checked-in recipes (#115) - unsupported CUDA runtime APIs as no-ops (#65) - Performance: replace the 0.1.94 / torch_musa 2.7.1 numbers with the checked-in 0.1.95 / 2.11.0.post2 entry, and stop claiming every fast path is under 200ns
Summary
torch's JIT loader creates a
FileBaton(<build_dir>/lock, a plainO_EXCLfile) in every process and holds it while ninja runs. If a process is killed during that window, the file is left behind, and every laterimport torchadablocks forever inFileBaton.wait().ENOLCKon some NFS), it warns and leaves thelockalone, so behaviour matches torch's original.src/torchada/_cpp_ops.pyandtests/test_cpp_ops_guard.py, +119/−3.Evidence
Measured on MTT S5000 with torch_musa 2.11.0.post2+musa5.2.0.
The same scenarios also pass on torch 2.7.1, 2.9.0 and 2.11.0.post1, where the loader API used is identical. An 8-GPU sglang prefill server starts and serves requests. These runs used earlier versions of this patch that rely on the same flock mechanism.