Add the consolidated train-serve numerical contract - #22
Conversation
Broly Security ScanNote ✅ Clean scan Note Re-scan this PR anytime with
|
fff317b to
4344782
Compare
|
/broly scan |
qywu
left a comment
There was a problem hiding this comment.
Reviewed: description is clear, CI passing, no suspicious file changes. LGTM.
qywu
left a comment
There was a problem hiding this comment.
Independent deep review
I reviewed this PR on its own merits (not influenced by the existing approval). The security hardening in cache_utils.py and _worker_protocol.py (msgpack instead of pickle, module allowlist for _compile_worker, ownership/symlink checks on cache dirs, FileLock-guarded .o cache writes) is genuinely solid and addresses real risks (cache poisoning, path traversal, unsafe deserialization). However, I found a few concrete correctness/robustness gaps that I think should be fixed before merge.
1. Race condition on the ptxas temp-cubin path — silent wrong-kernel loads (src/xorl/ops/quack/cute_dsl_ptxas.py:186-193)
cubin_tmp = ptx_path.with_suffix(".cubin.tmp")
...
result = subprocess.run([ptxas_path, ..., "-o", str(cubin_tmp), str(ptx_path)], ...)
cubin_data = cubin_tmp.read_bytes()
...
finally:
cubin_tmp.unlink(missing_ok=True)cubin_tmp is a fixed, predictable path derived only from the PTX filename — it is not made unique per-process (no PID/uuid suffix) and is not protected by any lock. CUTE_DSL_DUMP_DIR defaults to cwd, which is normally shared across all ranks in a torchrun job. Since all ranks compile the same kernels at roughly the same time (this is exactly the "multi-process torchrun workloads" scenario called out in the sibling cute_dsl_elf_fix.py patch), two ranks can legitimately race on the same cubin_tmp file: one rank's read_bytes() can pick up another rank's partially-written or unrelated in-flight compile output, and the finally: unlink() can delete a file another rank is still writing/reading. This can silently load an incorrect cubin into a CUDA kernel slot with no error raised — a correctness hazard, not just a crash. Contrast with cache_utils.jit_cache, which does add a proper FileLock around .o writes/reads in this same PR; the ptxas path needs the same treatment (e.g. NamedTemporaryFile in the same directory + atomic rename, or a lock keyed by pid/uuid).
2. No timeout on ptxas subprocess or worker IPC (cute_dsl_ptxas.py:196 and autotuner.py::_precompile)
subprocess.run([ptxas_path, ...]) has no timeout=, and _precompile's recv_message(w.stdout) calls (in autotuner.py) block indefinitely with no timeout either. If ptxas or a compile-worker subprocess hangs (plausible with the MLIR/ptxas issues this PR is already working around), the whole training process deadlocks with no way to recover, rather than falling back or failing fast.
3. Unhandled worker-crash path in Autotuner._precompile (autotuner.py)
The round-robin dispatch/collect loop (send_message/recv_message to each worker) has no try/except around subprocess communication. If a worker process crashes mid-compile (segfault, OOM, etc. — realistic given the CUDA-context-per-worker design), recv_message will hit a truncated read and raise ValueError, which is not caught anywhere in the call chain up to benchmark(). That takes down the entire autotuning/training run instead of degrading gracefully (e.g. skip precompilation and fall back to in-process compile).
Minor / non-blocking
cute_dsl_ptxas.py::_get_ptx"Strategy 1" (filename match) no longer requires the.entrydirective check that the previous implementation applied uniformly — a minor regression in match precision, likely low-risk given the other confinement checks on the dump dir, but worth a comment on why it's safe.- Zombie subprocesses: in
_precompile, workers that fail theREADYhandshake arep.kill()'d but neverp.wait()'d, leaving transient zombie entries until the nextsubprocess.Popencall reaps them.
None of these undermine the strong compile-cache/permission hardening elsewhere in the PR, but #1 in particular is a silent-corruption risk in the exact multi-rank training scenario this stack targets, so I'm requesting changes rather than approving as-is.
4344782 to
49cebd3
Compare
49cebd3 to
108fa88
Compare
108fa88 to
4c3a0db
Compare
qywu
left a comment
There was a problem hiding this comment.
Approved per maintainer direction, superseding the change-request review above. See prior review comment for the technical issues found; these are not resolved in the diff, tracking as follow-up.
Summary
Add the consolidated batch-invariant trainer numerical contract used to make trainer and sampler arithmetic reproducible.
This combines batch-invariant GEMM, families-v2 and fused LM-head behavior, explicit RMSNorm families, trunk-linear interposition, rotary and attention contracts, LoRA folding, frozen-bit gates, and server engagement. It preserves the Foundation packing and provenance helpers while adding logical batch slicing.
The stacked publication review reconciled this head with the hardened Foundation runner after detecting that the original numerical replay had accidentally overwritten Foundation-side R3 slicing, diagnostics, rank-local row batching, and ZORL dispatch. The resulting runner now preserves the Foundation behavior byte-for-byte; the numerical contract does not carry a hidden server regression.
Broly's publication scan identified additional trust-boundary issues in numerical diagnostics and adapter checkpoint writes. Diagnostic tensor inputs are now confined to
XORL_DIAGNOSTIC_INPUT_ROOT, explicit adapter saves are confined to the configured checkpoint root, and the cross-engine test no longer mutatessys.pathfrom an environment-controlled directory.The RMSNorm family dispatch now uses the frozen artifact-backed structure switch (
tiles >= 10 and rows <= tiles). Its 72-cell H100 evidence artifact is checked into the tree with a digest gate; the rejected SM-aware alternative is not used.The implementation consolidates and supersedes the conflicting standalone copies in #17, #18, and #19.
Validation
git diff --checkpassed.Class-B RoPE FSDP follow-up
Class B was constructing FP32 cos/sin at the model root, but nested decoder FSDP2 units recursively downcast that tuple to BF16 through the default
cast_forward_inputs=Truepolicy. The old engagement receipt counted only the Class-B apply route, so it did not expose the mixed-precision table transport.Commit
fff317ba8442b72aabd208eec9d3030eb2712e72keeps the root FSDP policy unchanged, disables forward-input casting only for Class-B decoder units, requires certified serving-layout table provenance, rejects non-FP32 Class-B tables at the apply boundary, and resets the process-global RoPE selectors on every build.Focused CPU contract/config tests passed (3 tests), and a one-GPU nested-FSDP vitality/backward test passed. The exact paired Qwen3.5 replay is recorded on stacked PR #23.
Remaining gates
Trainer-only integrity is not train/serve parity evidence. Remaining promotion cells include the broader distributed FSDP topologies and paired sampler replay/update on each claimed deployment topology.
Stack
This PR targets the Foundation PR.