compass(runner): the exit bullet points at the refusal comment instead of restating its losses - #402
Conversation
…d of restating its losses The package docstring's `exit` bullet said a hole at exit leaves "the graphs and five KV tensors it deletes" held. This runner allocates neither: `NonAllocatingRunner.allocate_kv_cache` records a block count and allocates nothing, and `capture_cudagraph` captures nothing, while ATOM creates `self.graphs` only inside its own `capture_cudagraph`. The comment on the `RPC_SURFACE` check in `model_runner` already says the opposite, and a test holds it against `ModelRunner.exit`'s body. The bullet was a second, unpinned copy of that loss list, and it had drifted. The bullet now names the dispatch site, says `ModelRunner.exit` is never reached, and points at that comment for what is lost. The sentence about the loop still breaking is unchanged. Closes #398 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| distributed environment is never destroyed and the graphs and five KV tensors | ||
| it deletes stay held. The worker still leaves its loop -- `busy_loop` breaks | ||
| - `exit` (`engine_core.py:260`) never reaches `ModelRunner.exit`; the comment | ||
| on the `RPC_SURFACE` check in `model_runner` says what that loses here. |
There was a problem hiding this comment.
Non-blocking. Filed as #405, not this PR's to fix.
The pointer is right, and the comment is the only place the losses should live. But "says what that loses here" hands the comment a completeness claim that it does not meet under one config.
- The comment's list (
model_runner.py:66-71) names nodel self.drafter(atom/model_engine/model_runner.py:1076-1077). - I measured that a drafter can exist on this runner at
exit.ModelRunner.__init__owns construction, and it callsbuild_drafterat:776, after the_build_and_load_modeloverride at:757. - The only speculative refusal in the runner is in
forward(overrides.py:517at37fba4df0), so a step is the only thing that reaches it. - With an eagle3 config at the default aux setting,
Drafter.load_model(UnbuiltModel)returns None andarm_aux_capturereturns None. So the runner constructs holding a drafter, and one shut down before its first step reachesexitwith it. - The pin (
test_what_the_comment_says_a_hole_at_exit_loses_is_what_exit_does) checks only comment ⊆exitbody, so it cannot see the omission.
The larger problem is in #405: the drafter is built on the device and its checkpoint is read. That breaks principle 2: "Simulated execution touches no GPU. No compute, no device allocation."
Rule: "A finding not fixed in the PR that found it gets an issue." (AI_DEV_RULES)
|
Review cycle 1: PR #402, head Agent-authored review. I read the Design principles of Verdict: APPROVE. There are no blocking findings. One finding is non-blocking: it is outside this PR's file set and is filed as #405, with an inline comment at 1. The new bullet, clause by clause
The
The full gate below includes them again. Design-doc references: none in 2. Point versus pinPointing is the right call, and it loses nothing a reader needs.
Principle 3: "Prioritise simplicity. Add only what is necessary, and nothing more." 3. The drafter question: measured, reachable, filed as #405I ran
So The bigger finding is that the drafter is built on the device and its checkpoint is read. That breaks principle 2: "Simulated execution touches no GPU. No compute, no device allocation." The MTP path dies with an unnamed The fix belongs in 4.
|
| # | Finding | Severity | Where |
|---|---|---|---|
| 1 | The refusal comment's loss list omits del self.drafter, which is reachable under eagle3. The underlying defect is that the drafter is built on the device before any refusal. |
non-blocking; filed as #405 | inline at atom/compass/runner/__init__.py:33 |
No blocking findings.
Closes #398
Decision: point at the comment, not pin
The brief left one choice: pin the bullet's loss list with about 10 test lines, or point the bullet at the refusal comment. I pointed. The comment's loss list is already pinned:
test_what_the_comment_says_a_hole_at_exit_loses_is_what_exit_doesholds it againstModelRunner.exit's body. The bullet was a second copy that nothing pinned, and it had drifted into claiming the opposite of the comment. Deleting the copy removes the drift at its source. Pinning would have added a second pin for the same facts, plus about 10 test lines to maintain. Under/ponytail full, deleting beats adding.The change: 3 docstring lines in
atom/compass/runner/__init__.pyBefore (
:32-34atb7cd11d48; the bullet runs to:36):After (
:32-34):The rest of the bullet is unchanged: "The worker still leaves its loop --
busy_loopbreaks on the dispatched name, [...] so the symptom is what shutdown failed to release, not a hang."Evidence: every claim the bullet makes, quoted at the tip
b7cd11d48The new bullet makes three claims, and each one is quoted below. It names no loss of its own: the losses are now named only in the comment.
exitis dispatched atengine_core.py:260atom/model_engine/engine_core.py:260:self.runner_mgr.call_func("exit")ModelRunner.exit"atom/model_engine/model_runner.py:1052:def exit(self):. The comment says the same thing: "ModelRunner.exitnever runs" (atom/compass/runner/model_runner.py:67).RPC_SURFACEcheck inmodel_runner"atom/compass/runner/model_runner.py:54-55:_UNANSWERED = unanswered_rpc_names(CompassModelRunner)/if _UNANSWERED:. The comment is at:56-85, inside thatif.unanswered_rpc_names(overrides.py:147-148) is "The dispatched namesgetattrwould answer with None for this runner", drawn fromRPC_SURFACE. Themodel_runnermodule bullet in the same docstring already says it is "where the composed class is checked againstoverrides.RPC_SURFACE".What the comment names (
model_runner.py:67-71), againstModelRunner.exit's body (atom/model_engine/model_runner.py:1052-1080)ModelRunner.exit:1058destroy_dist_env()self.modelis never dropped":1074-1075if hasattr(self, "model"):/del self.modeloverrides.py:248self.model = UnbuiltModel(model_class): the model exists, but no weight was builttorch.cuda.empty_cache()never runs":1078torch.cuda.empty_cache()hasattr-guarded and find nothing here, because this runner allocated none":1065-1073loop overkv_cache,kv_scale,index_cache,mamba_k_cache,mamba_v_cache, eachif hasattr(self, attr): delattr(self, attr)overrides.py:396-397allocate_kv_cache: "Record the block count and allocate nothing."What the old bullet claimed, and why it was false for this runner
ModelRunner.exit:1060-1061if not self.enforce_eager:/self.graphs = self.graph_pool = None. ATOM createsself.graphsonly inside its owncapture_cudagraph, at:3832.overrides.py:410-411capture_cudagraph: "Capture nothing". There is no graph for exit to release.:1065-1073overrides.py:396-397"allocate nothing". There is no tensor for exit to release.The #399 review measured the same thing with an AST walk (#399 (comment)).
ModelRunnerwrites the KV names and the graph attributes in only three methods:allocate_kv_cache,capture_cudagraphandexit.NonAllocatingRunnerreplaces the first two.Checks on the docstring's readers
Only
tests/compass/test_runner_rpc_surface.pyreadsPACKAGE_DOC(:48). Its readers are at:1118,:1129,:1152,:1170,:1174and:1214. I re-ran their text logic, with no torch import, on the tip docstring and on the head docstring. The results were identical:exit,model_runner,overrides,process_kvconnector_output,step_output. The edit keeps the- `exit`lead, and its new backticked names sit mid-line, where_bulletsdoes not read them as keys.CITATIONmatches, used by the citation check. Both sides give the same six sites, withengine_core.py:260the only one in theexitbullet. The pointer namesmodel_runnerwithout afile.py:NNN, so it adds no citation.Masked AST:
atom/compass/runner/__init__.py, with docstrings masked (1/1), is identical betweenb7cd11d48and the head. The diff is 3 docstring lines and nothing else.Gate 1: node 18,
xiaobizh_n18_cpugit merge-tree --write-tree b7cd11d48 cfb36f81f=4529ebe10d78, which equalscfb36f81f^{tree}. That is the tree staged withgit archiveplusdocker exec -i ... tar -xinto/tmp/i398gate/ATOM. The tarball md56483d50d7667...matched on both ends..compass-commitand.compass-changedwere written from the samerev-parse.scripts/compass/gate_cpu.sh, withtimeout -k 10 3000and no pipe. It printedcommit: cfb36f81f (stamp).atom.__file__=/tmp/i398gate/ATOM/atom/__init__.py.GATE_CPU_RC=0.b7cd11d48has tree869545f94c51, which is the tree compass(tests): say the refusal comment is what the exit test holds against ModelRunner.exit #399 gated at 5275 / 155 / 3, rc=0. Delta: 0. No test was added.EXIT=143) and re-ran alone, and the result above is from that re-run.Not covered
The comment names no graph release and no
del self.drafter(:1076-1077). The graph release finds nothing here (see above). For the drafter, I did not measure whether a speculative config builds one in__init__(model_runner.py:771-776) beforeoverrides.py:516refuses it. The comment is outside this PR's file set, so that question stays with the comment and its test.🤖 Generated with Claude Code