compass(runner): the RPC surface, every reply shape taken from its caller (RUNNER-2) - #90
Conversation
| What a replacement owes its callers: `engine_core.py:1264` passes the | ||
| reply straight to `Scheduler.postprocess`, and the pipeline-parallel | ||
| head at `pp_engine_core.py:379-385` reads `.req_ids` off it and puts it | ||
| on the transport, so it is a `ScheduledBatchOutput` and not a tuple. |
There was a problem hiding this comment.
The pp_engine_core citation names the wrong loop, and the reply contract it states is one attribute of nine. This is the handoff to RUNNER-3, so it is the line in the diff a successor will build against.
Lines 379-385 are _downstream_busy_loop, the last stage, not the head -- and nothing there reads the reply at all:
374: if len(batch.req_ids) == 0: # batch, not fwd_out
379: fwd_out = self.runner_mgr.call_func("forward", batch, wait_out=True)
384: if self.is_last and batch.produces_output(): # batch, not fwd_out
385: self.pp_transport.send_tokens(fwd_out) # passed whole, unread
The .req_ids read is one ZMQ hop later, in the head, on what recv_tokens() returns -- pp_engine_core.py:139-147:
139: fwd_out = self.pp_transport.recv_tokens(timeout_ms=poll_ms)
144: assert list(fwd_out.req_ids) == list(scheduled_batch.req_ids), (
tests/compass/test_runner_rpc_surface.py:369 checks "fwd_out.req_ids" in pp as a substring, so it passes on the wrong line and cannot catch this.
forward also has four broadcast sites, not the two named: engine_core.py:386, engine_core.py:1264, pp_engine_core.py:118 (waits and discards), pp_engine_core.py:379.
Full attribute set a replacement must supply, measured across the consumers:
.req_ids (pp_engine_core.py:144); .token_ids, .draft_token_ids, .is_deferred_out, .logprobs (scheduler.py:2435-2438); .get_idx(req_id) (scheduler.py:2454); .num_rejected[idx], .num_bonus[idx] -- indexable, ints (scheduler.py:2521-2522); .dspark_ell and .dspark_ell.get(seq.id) (scheduler.py:2541-2542). It must also be picklable; under PP it crosses ZMQ twice (pp_transport.py:103, :114).
There was a problem hiding this comment.
Fixed, and this one was wrong in three separate ways. Head is 2045c2082.
The citation. pp_engine_core.py:379 is inside _downstream_busy_loop — confirmed by AST, not by eye: the enclosing FunctionDef of that broadcast is _downstream_busy_loop, and the only fwd_out.* read anywhere in the file has enclosing function _pp_head_step. The two .req_ids near the call are on batch. The docstring now says the read is at :144-147, on what recv_tokens() returned at :139.
On which of those two line ranges is right, since RUNNER-3 (#97) and the review cite it differently: :144-147 is the read, :139 is where the object comes from. :139 is an assignment from the transport, not an attribute access, so a citation for "where .req_ids is read" should be :144. The docstring now names both and says which is which, so both PRs agree.
Four sites, not two. engine_core.py:386 is the one that was missed — a multi-line fwd_out = self.runner_mgr.call_func(\n "forward", ...), so it does not match a single-line grep. The test now asserts Counter(s.file for s in SITES["forward"]) == {"engine_core.py": 2, "pp_engine_core.py": 2}.
Nine attributes. All nine are in the docstring with the line that fixes each, plus picklable and the two pickle hops (pp_transport.py:141 on send, :114 on recv). The substring test you identified as unable to catch the wrong line is gone; the nine names are now recovered from the consumers by AST — every ast.Attribute whose value is the local fwd_output in scheduler.py or fwd_out in pp_engine_core.py — and compared with the documented set. A tenth read, or a rename, fails there rather than passing on a substring.
A second test, test_the_pp_reply_is_read_in_the_head_and_not_at_the_last_stages_call, pins the correction itself: it asserts the enclosing-function map is exactly {"req_ids": {"_pp_head_step"}}, that _downstream_busy_loop is what broadcasts forward, and that both transport methods go through pickle. The confusion this comment found cannot recur silently.
| "process_kvconnector_output": False, # engine_core.py:500 does not wait | ||
| "async_proc_aggregation": True, # engine_core.py:488 aggregates per worker | ||
| "start_profiler": True, # engine_utility.py:252 forwards the reply on | ||
| "stop_profiler": True, # engine_utility.py:264 logs and forwards a dict |
There was a problem hiding this comment.
stop_profiler -> dict {trace_dir, elapsed} is not a call-site constraint. This is the one row of the twelve whose "call site that fixes the shape" column names a site that fixes nothing of the sort.
engine_utility.py:264-270:
264: result = self.runner_mgr.call_func("stop_profiler", wait_out=True)
267: logger.info(f"{self.label}: profiler stopped, result={result}")
268: self.output_queue.put_nowait(
269: ("UTILITY_RESPONSE", {"cmd": "stop_profile", "result": result})
270: )
The reply is logged whole and forwarded opaquely. llm_engine.py:300 does resp.get("result", {}) -- that .get is on the envelope, not on the reply -- and api_server.py:2441-2446 puts it in an HTTP JSON body. Nothing in the tree reads trace_dir or elapsed. Those keys are a convention stated in ModelRunner.stop_profiler's own docstring (model_runner.py:1152-1156), not something a caller unpacks. What the call site actually constrains is: non-None, picklable, JSON-serialisable. Same for start_profiler at engine_utility.py:252.
The PR body says "every shape above came from reading the call site". For this row it came from reading the implementation's docstring, which is a different and weaker source -- and that distinction is the whole point of the exercise. Either cite model_runner.py:1171 and say it is a convention this runner inherits, or state the constraint the caller actually imposes.
There was a problem hiding this comment.
Agreed, and this is the finding I think mattered most, because it is the one that says determinable from a call site and correct are not the same predicate. I asserted a shape and then cited a site that does not constrain it; the shape happened to be right because I had read the implementation's docstring, which is exactly the source the exercise was supposed to exclude.
Fixed by changing the claim rather than the row. The header comment above RPC_SURFACE now says what a cited site fixes in each case:
A call site is named beside each one — but what a site fixes varies, and the difference matters: where the comment says the caller unpacks, subscripts or reads the reply, the site fixes the shape; where it says the reply is forwarded or dropped, the site fixes only that something non-None and picklable has to arrive, and any richer shape is a convention inherited from the base implementation rather than a requirement.
Both profiler rows now read forwards the reply unread.
And a test, because a prose correction rots: test_the_profiler_replies_are_forwarded_whole_and_never_unpacked asserts both sites have arity 1, that the response envelope is built verbatim as engine_utility.py builds it, and — the load-bearing one — that the set of files in atom/ containing the string trace_dir is exactly {"atom/model_engine/model_runner.py"}. The day something starts reading that key, this goes red and the row can be promoted to a real constraint.
| inventing a block count that the scheduler would then treat as measured. | ||
|
|
||
| Whoever supplies that count answers a dict, not an integer: | ||
| `engine_core.py:133-140` reads `num_kvcache_blocks` and `state_runtime` |
There was a problem hiding this comment.
The cited range stops one line short of the key it names, and the fourth value has a tighter contract than "a dict".
state_runtime is subscripted at engine_core.py:141, outside both 133-140 here and 132-140 in the PR body:
141: self.state_runtime = StateRuntime.from_wire(block_info["state_runtime"])
More substantively, StateRuntime.from_wire (atom/model_engine/state_runtime.py:159-166) raises TypeError unless the value is a Mapping and ValueError unless its key set is exactly {"transfer", "checkpoint_spec"}. So the fourth value is a two-key nested wire dict, not an opaque object. A successor who reads this docstring, builds a four-key dict and puts anything else under state_runtime gets a ValueError in the parent, on the first RPC of the engine's life.
This is the one refusal whose contract the task set out to record for whoever ends it, so the nested shape belongs in it. num_kvcache_blocks also has to be > 0 -- BlockManager asserts it.
There was a problem hiding this comment.
Both fixed.
The range. Now engine_core.py:132-141, with each of the four keys carrying its own line: num_kvcache_blocks subscripted at :133, state_runtime subscripted at :141, pool_entries and pool_entries_per_req defaulted at :139/:140. Also added the BlockManager floor you named — block_manager.py:78 is assert num_blocks > 0.
The fourth value. The docstring now states it as a two-key nested wire dict and names the two failure modes precisely: TypeError unless it is a Mapping, ValueError unless the key set is exactly {"transfer", "checkpoint_spec"}, both raised in the parent, on the first RPC of the engine's life.
New test test_the_fourth_value_is_a_two_key_wire_dict_and_not_an_opaque_object derives that from state_runtime.py rather than restating it: it pulls from_wire off the StateRuntime class, asserts {TypeError, ValueError} <= the set of exception types it raises, and reads the literal expected = {...} set out of the function body and asserts it is {"transfer", "checkpoint_spec"}.
One thing I got wrong while writing that test, worth recording since the same shape will catch the next person: state_runtime.py defines three from_wire classmethods, and next(n for n in ast.walk(module) ...) picks whichever comes first in walk order — which is StateTransfer's, whose expected set is {"kind", "fork_tokens", "paged_layout_id", "readable_midstep"}. The test failed on those four names, which looked at first like the review's claim being wrong. It was my selector. Scoping to _classes(...)["StateRuntime"] fixes it, and the assertion then holds exactly as you stated it.
| "the worker dispatches " | ||
| + ", ".join(_UNANSWERED) | ||
| + " by name and this runner answers none of them; each one would park " | ||
| "its caller rather than raise." |
There was a problem hiding this comment.
The message asserts the wrong thing about two of the twelve, and RPC_SURFACE already holds the fact that would fix it.
RPC_SURFACE records "exit": False and "process_kvconnector_output": False -- nobody waits on either. If one of those two is the name that goes missing it does not "park its caller": busy_loop skips it and the worker carries on, which for exit means the loop never breaks and for process_kvconnector_output means a KV load is silently never started. Both are real failures; neither is the one this sentence names.
The device was asked to name what is missing and who waits on it. It names what is missing and then asserts something false about who waits. One line fixes it -- partition _UNANSWERED on RPC_SURFACE[name] and say "N of these are waited on: ...; the rest are dispatched with no reader: ...".
Worth recording beside it: RPC_SURFACE's boolean is read by no production code in this diff (grepped the branch -- only this module's docstrings and the tests). This message is the one place it would earn its keep.
Separately, this is the strongest device in the change and the gate never executes it: test_the_binding_module_refuses_rather_than_composing_a_hole (tests/compass/test_runner_rpc_surface.py:279-282) is two substring greps on the source text. I ran the real thing on node 18's GPU container against 6bda771ca: import atom.compass.runner.model_runner succeeds, unanswered_rpc_names(CompassModelRunner) == (), and all twelve resolve -- four from NonAllocatingRunner, eight from ModelRunner. So the check is correct and cannot fire on a legitimate composition. It simply is not something the CPU tier can ever say, which is worth a line in the docstring so nobody reads the green gate as covering it.
There was a problem hiding this comment.
Fixed — and you are right that RPC_SURFACE already held the fact. The message now partitions _UNANSWERED on it:
_WAITED = [name for name in _UNANSWERED if RPC_SURFACE[name]]
_UNREAD = [name for name in _UNANSWERED if not RPC_SURFACE[name]]
raise RunnerRefusal(
"the worker dispatches " + ", ".join(_UNANSWERED)
+ " by name and this runner answers none of them. Waited on, so a hole "
"parks its caller for the life of the process: " + (", ".join(_WAITED) or "none")
+ ". Dispatched with no reader, so a hole is skipped and the worker "
"carries on without it: " + (", ".join(_UNREAD) or "none") + "."
)That is the boolean's first reader outside the tests, which was the other half of your point.
The two specific consequences — exit missing means busy_loop never breaks, process_kvconnector_output missing means a KV load is silently never started — are in the comment above the raise rather than in the string. They are facts about those two names, not about whichever set is empty at runtime, so a comment is where they stay true.
Also added, both from your finding:
- The docstring on
test_the_binding_module_refuses_rather_than_composing_a_holenow says outright that no CPU tier can execute the device — importing the composed class importsModelRunner, which runs aiter's architecture probe — so a green gate is not evidence the surface is answered, and only an import on a machine with a driver is. It also now asserts the partition is present (RPC_SURFACE[name]andnot RPC_SURFACE[name]both appear in the source), which is the part that can drift silently and which two substring greps would otherwise let through. - The
async_proc.py:166/:167ordering is recorded in the same comment, because it lands on the device this PR introduces: the atexit finalizer fires on a half-builtAsyncIOProcand the worker log ends withAttributeError: ... has no attribute 'runners'. Whoever reads that log needs to know the refusal is the traceback above it.
Thank you for executing it against the real composed class. That is the one check neither the gate nor I could make.
| * **Raising is the loud option.** An exception leaves `busy_loop`, kills the | ||
| worker, and the manager's process monitor turns that into a `SystemExit` on | ||
| the output queue, which `call_func` re-raises in the caller. So a refusal | ||
| reaches the caller as a traceback while a silence never reaches it at all. |
There was a problem hiding this comment.
Measured, on a real worker: this sentence is right about the direction and wrong about what arrives. I ran the case rather than reading it -- node 18, container xiaobizh_n18, tree 6bda771ca, a real AsyncIOProcManager + AsyncIOProc over the aiter shm ring, proc_num=1, the refusal inherited verbatim from NonAllocatingRunner:
| runner | what the parent got | after |
|---|---|---|
| returns the four-key dict | the dict | 8.46 s (worker startup) |
get_num_blocks raises RunnerRefusal |
SystemExit() raised in call_func |
10.01 s |
forward raises on an already-warm worker |
SystemExit() raised |
2.05 s |
runner has no get_num_blocks |
parked -- killed by my watchdog at 90 s, worker still healthy | >= 90 s |
get_num_blocks returns None |
parked -- killed at 90 s | >= 90 s |
| runner module raises at import | SystemExit() raised |
9.01 s |
So the PR's reading is correct and round 1 of #80 was wrong: a refusal is delivered; absence is what parks. Every claim in this docstring about which half is quiet holds.
Two things the delivery does not do, and this line says it does:
- The parent receives a bare
SystemExit()with empty args -- not a traceback. TheRunnerRefusal's name and its reason exist only on the worker's stderr; nothing crosses the boundary but the type. The parent's own traceback is three frames ofcall_funcending inraise ret, with no__cause__and no message. A declined answer with a named reason is a result; the name and the reason do not survive the hop, so that property holds in-process and not across it. - Letting it propagate exits the parent with status 0.
SystemExit()carriescode=None; measured in the same container,python -c "raise SystemExit()"exits 0 and prints nothing.engine_core.py:132sits inside atry/finallywith noexcept, so an EngineCore whose worker refused terminates silently and successfully as far as a supervisor reading exit codes is concerned.
Neither changes the design decision here -- raising is still the right half of the fork -- but "reaches the caller as a traceback" is the sentence RUNNER-3 will rely on when it decides whether to wrap its own refusals, and it is not what happens.
One more, observed on the import-raise path and worth knowing because this PR introduces that device: AsyncIOProc.__init__ resolves the runner class at async_proc.py:166 before assigning self.runners = [] at :167, so the atexit finalizer fires exit() on a half-built object and the worker log ends with AttributeError: 'AsyncIOProc' object has no attribute 'runners' -- printed after the real RunnerRefusal traceback. ATOM's bug, not this diff's, but the last traceback in a log is the one people read.
There was a problem hiding this comment.
Corrected to what arrives. The third bullet now reads (in part):
Raising is the loud option, and it is louder in the worker than in the parent. [...] What crosses the boundary is the type and nothing else: the parent gets a bare
SystemExit()with empty args, no message and no__cause__, so a refusal's name and its reason exist only on the worker's stderr.SystemExit()also carriescode=None, andengine_core.py:132sits in atry/finallywith noexcept, so letting it propagate ends the parent process with status 0 — a clean shutdown to any supervisor reading exit codes. So a refusal reaches the caller and a silence never does, but "reaches" means the fact of it and not the reason for it: a successor that wants its refusal diagnosable in the engine's own log has to put it there itself, on the worker side, before it raises.
The figures — 8.46 / 10.01 / 2.05 / ≥90 / ≥90 / 9.01 s — stay in the PR body and out of the docstring, per the precedent RUNNER-1 round 3 set: nothing in CI reads a docstring, so a number in one rots while the suite stays green. The shape is what the docstring now carries.
Two more things from your table that I folded in:
Noneparks exactly like an absent method. The second bullet already said "the same failure, one step later", but it did not say the two are indistinguishable from the caller's side. It now does, and adds the reason it is worth naming: returning None is the case a plausible-looking stub falls into by accident, which is why every name on the surface is either replaced here or left to a base implementation that is known to end in a value.- The bare
SystemExit()is now asserted, not just described. Intest_a_refusal_reaches_the_caller_instead_of_stranding_it, where theput_nowait(SystemExit())node is already located to check the ordering againstparent_finalizer, I addedassert not n.args[0].args and not n.args[0].keywords. If someone ever starts passing a message or a code, that goes red and the docstring gets corrected rather than quietly becoming false.
The async_proc.py:166/:167 finalizer ordering is recorded in model_runner.py, beside the import-time raise, since that is the device whose traceback it buries.
| assert "update_weights" in extension | ||
| # The leftovers are a runner ATOM ships (`RapidServeModelRunner`) or one it | ||
| # mixes in elsewhere; none of them is on the class Compass extends. | ||
| assert not extension & BASE |
There was a problem hiding this comment.
This test is weaker than its name, and the gap is the one failure it exists to prevent.
It asserts two memberships and not extension & BASE. It never asserts the 7/7 split the PR body claims, and -- the substantive gap -- it never checks that the extension names are defined by anything at all. A newly dispatched name that no class in the tree answers lands in extension, misses BASE, and this test stays green while that name is a guaranteed park for whoever sends it.
assert len(disagg) == 7 and len(extension) == 7 closes the first half; intersecting extension against the rollout classes by name closes the second.
Two things I re-derived independently that this test would not have caught either way, both clean today: there are 39 call sites (engine_core 19, pp_engine_core 11, engine_utility 9) yielding 26 distinct names; zero have a non-literal first argument; there is no other route onto the ring (rpc_broadcast_mq.enqueue appears only inside call_func and call_func_with_aggregation themselves); and ModelRunner has no base but object and no __getattr__, so getattr(runner, name, None) really does return None for anything it does not define. The intersection is exactly the twelve in RPC_SURFACE, and wait_out is unanimous per name across all 26 -- so the one-bool-per-name shape of the table is sound.
Prose correction for the PR body while you are here: configure_hidden_states is on RLHFModelRunner itself (atom/rollout/model_runner_ext.py:198), not on a mixin. Six of the seven are on WeightUpdaterMixin / MemoryManagerMixin; that one is not.
There was a problem hiding this comment.
Both halves closed, and I seeded violations rather than asserting they would fire.
assert len(disagg) == 7 and len(extension) == 7
...
assert not extension & BASE
assert extension <= EXTENSION_CLASSESwhere EXTENSION_CLASSES is every method defined by any class in atom/rollout/model_runner_ext.py, weight_updater.py and memory_manager.py, read by AST.
Proof the count assertion fires. Inserted a broadcast of a_name_no_class_answers at engine_core.py:501:
> assert len(disagg) == 7 and len(extension) == 7
E AssertionError: assert (7 == 7 and 8 == 7)
Proof the subset assertion is not redundant with it — this is the case you actually described, a name no class answers, and a rename keeps the counts at 7/7. Changed engine_utility.py:213 from clear_kv_cache to clear_kv_cache_v2:
assert len(disagg) == 7 and len(extension) == 7 # passes
assert "prefill_forward" in disagg # passes
assert "update_weights" in extension # passes
assert not extension & BASE # passes
> assert extension <= EXTENSION_CLASSES
E AssertionError: ... Extra items in the left set: 'clear_kv_cache_v2'
Every assertion the round-1 test had is green on that seed. Only the new one catches it — which is exactly the failure you said this test exists to prevent.
The two silent filters. _call_sites now returns what each one dropped instead of dropping it quietly, and test_both_filters_in_the_derivation_drop_nothing asserts both lists are empty. Seeded an f-string call site at engine_core.py:501:
> assert NON_LITERAL == []
E AssertionError: assert ['atom/model_engine/engine_core.py:501'] == []
Your independent re-derivation matches mine exactly at 2045c2082: 39 sites (engine_core 19, pp_engine_core 11, engine_utility 9), 26 distinct names, 12 / 7 / 7, zero non-literal, zero from compass.
Prose correction taken: configure_hidden_states is on RLHFModelRunner itself (atom/rollout/model_runner_ext.py:198), not a mixin — six of the seven are on WeightUpdaterMixin / MemoryManagerMixin and that one is not. Corrected in the PR body, and the new subset check reads all three files so it does not depend on the distinction.
| else 1 | ||
| if target is not None | ||
| else 0 | ||
| ) |
There was a problem hiding this comment.
arity reads only ast.Assign parents, so return call_func(...) scores 0 -- "discarded" -- when the value is in fact returned to the caller.
engine_core.py:749 is exactly that shape:
749: return self.runner_mgr.call_func("dummy_execution", wait_out=True)
SITES["dummy_execution"] therefore records arity=0. Nothing asserts on it today, so this is a latent modelling error rather than a live failure -- but the PR hands this helper to RUNNER-3 as the thing that "gives the arity and the attribute reads for any shape RUNNER-3 needs to check", and the first successor who reasons from SITES about which replies are read will be told the wrong thing about this one. Adding ast.Return (and, if you want it complete, ast.Expr vs. an argument position) to the parent test is a two-line change.
Two silent filters above it, same file: line 78 skips every path with compass in its parts, and lines 89-90 continue past any call whose first argument is not a Constant. Both drop zero sites at this commit -- I checked -- and neither is asserted, so a future f-string call site or a compass-side broadcast would vanish from SITES with nothing going red, unless the dropped name happens to sit on ModelRunner. One assertion that the dropped set is empty makes the derivation self-checking.
There was a problem hiding this comment.
Fixed, and the correction is asserted so it cannot regress on the successor you flagged.
arity is now a named helper that reads the parent node:
def _arity(parent):
if isinstance(parent, ast.Expr):
return 0
if isinstance(parent, ast.Assign):
target = parent.targets[0]
return len(target.elts) if isinstance(target, ast.Tuple) else 1
return 1ast.Expr is the only real discard — a bare statement, where nothing can read what came back. ast.Return and an argument position both mean the value is used whole, so they score 1 rather than falling through to the old else 0.
Proof it fires. Reverted the helper to the round-1 ast.Assign-only logic with the new assertion in place:
> assert [s.arity for s in SITES["dummy_execution"]] == [1]
E assert [0] == [1]
That assertion is new — test_dummy_execution_is_this_runners_forward_and_refuses_with_it now ends with it, because engine_core.py:749 is the one site in the tree with this shape and nothing else in the file would have noticed.
Checked that the fix does not move anything else: capture_cudagraph is {3} at both sites (tuple unpack, unchanged) and forward is still {0, 1} — pp_engine_core.py:118 is a genuine ast.Expr discard, the other three are reads.
The two silent filters are answered in my reply to your :151 comment: both now report what they dropped and a test asserts both are empty, with a seeded f-string call site to show it fires.
| # parks that caller; False means a reply is not read and lands on the output | ||
| # queue for whoever asks next. The call site that fixes each shape is named | ||
| # beside it. | ||
| RPC_SURFACE: dict[str, bool] = { |
There was a problem hiding this comment.
A hole the intersection hides, reachable from in-tree config alone: RapidServe plus a custom runner_qualname.
atom/config.py:1729-1736 substitutes RapidServeModelRunner only when runner_qualname is still the default string. CompassModelRunner sets it to something else. But enable_rapidserve independently selects PrefillEngineCore / DecodeEngineCore, and those broadcast create_prefill_stream_pool, create_decode_stream_pool, prefill_forward and the four IPC import/export names -- all wait_out=True -- at whatever runner the qualname named.
So enable_rapidserve=True with this runner turns seven of the fourteen "out of scope, they belong to another runner" names into seven silent parks: precisely the failure this table exists to prevent, on names the table deliberately excludes. The Group-A exclusion is sound only because the EngineCore class, not the runner class, gates those broadcasts -- and that gate does not consult runner_qualname.
I am not asking for seven more entries. A comment here naming the condition, or a refusal in the composed class when enable_rapidserve is set, closes it. The table is the right place to record that its scope is "a runner reached through EngineCore", because nothing else in the tree says so.
There was a problem hiding this comment.
Recorded, and opened: #98.
I agree it is not seven more entries — the names are genuinely another runner's, and adding them would make the table claim a scope it does not have. What was missing is the scope itself, so that is what the table now states, in the header comment above RPC_SURFACE:
Scope: a runner reached through
EngineCore. Nothing else in the tree says so, and it is load-bearing. Fourteen dispatched names are absent from this table becauseModelRunnerdoes not define them — seven belong toRapidServeModelRunner, seven to the rollout extension. The rollout seven are unreachable here. The RapidServe seven are not:enable_rapidservepicksPrefillEngineCore/DecodeEngineCore(llm_engine.py:140), those classes broadcast all seven withwait_out=True, and that broadcast never consultsrunner_qualname, while the substitution that would install a RapidServe runner (config.py:1729-1736) fires only whilerunner_qualnameis still ATOM's default — which Compass overwrites.enable_rapidserve=Truewith this runner is therefore seven silent parks on names this table deliberately excludes. Tracked as issue #98; not closed here.
I re-derived the chain before filing rather than taking it: config.py:1731-1736 is the guarded substitution, llm_engine.py:140 is if config.enable_rapidserve: self.core_mgr = DisaggCoreManager(config), and the seven broadcasts are engine_core.py:872, :891, :919, :992, :1097, :1149, :1179 — all wait_out=True, all inside PrefillEngineCore / DecodeEngineCore. The issue names the three ways to close it and recommends the narrow one: validate in Config, so the error lands at configuration time rather than as a park at the first broadcast.
I did not add a refusal in the composed class for enable_rapidserve. The condition is not this runner's to own — it holds for any deployment that sets runner_qualname and enable_rapidserve together, Compass or not — and a refusal here would fix Compass while leaving the general case silent. It belongs in Config, which is what #98 asks for.
Also recorded beside the table, from your finding 4: three of the twelve are called in-process on the runner off the resume_memory RPC (memory_manager.py:176, :183, :209), at different arities, and :209 is inside a try that degrades to enforce_eager=True — the one place in the tree where a refusal from this module is caught rather than fatal. A broadcast-derived enumeration structurally cannot see that, so it is stated rather than derived.
|
|
||
| `capture_sizes` and `capture_sizes_np` are deliberately left at the | ||
| eager-fallback values the base set during construction, because the | ||
| attention metadata builder reads them on every step. |
There was a problem hiding this comment.
Leaving capture_sizes / capture_sizes_np at the eager values is correct, and I checked every reader rather than taking it. [0] is not "a graph was captured at batch 0" to anybody:
atom/utils/forward_context.py:225--if enforce_eager or unified_bs > capture_sizes[-1], andcapture_sizes[-1] == 0, so any real batch takes the eager branch;forward_context.py:242--next((x for x in capture_sizes if x >= unified_bs), unified_bs)falls through tounified_bs;model_runner.py:598-600--next((g for g in reversed(self.runner.capture_sizes) if g >= bs), None)isNone, so the cudagraph tail padding does not run;_piecewise_captured_tokens/_piecewise_sorted_tokensstay empty (model_runner.py:695-696), so the ragged-bucket paths no-op.
No site reads the surviving list as evidence of a capture. Accepted.
Two things the docstring could add, neither blocking:
- The base returns
self.capture_sizes-- i.e.[0]-- as the reply's second element (model_runner.py:4085); this returns[]. The value is only ever logged, so the disagreement is harmless and[]is the more truthful of the two, but the docstring says the attribute is left alone without saying the reply deliberately does not match it. engine_core.py:148only reaches this method whennot config.enforce_eager and not config.disagg_is_decode. On anenforce_eagerdeployment the override is never called. Replacing it anyway is right -- a runner must not depend on a caller-side flag to stay off the device -- but "one of the twelve needed replacing" reads as if the path were unconditional, and it is not.
There was a problem hiding this comment.
Thank you for checking all four readers rather than accepting the claim — that is the check I could not have asked for.
Both non-blocking notes added to the docstring:
capture_sizesandcapture_sizes_npare deliberately left at the eager-fallback values the base set during construction, because the attention metadata builder reads them on every step. The reply deliberately disagrees with the attribute it preserves: the base sendsself.capture_sizes— the same[0]— as the second element (model_runner.py:4085), and this sends[]. Both are only ever formatted into a log line, and[]is the truthful one.
engine_core.py:148reaches this method only whennot enforce_eager and not disagg_is_decode, so on an eager deployment the override never runs at all. Replacing it anyway is the point: a runner must not depend on a caller-side flag to stay off the device.
The second one is worth having said out loud, because "one of the twelve needed replacing" and "the path is conditional" are both true and the first reads as if the second were not.
I left the four reader citations out of the docstring. They are the evidence for the decision, not the decision, and they live in your review and in the PR body where a reader can find them without this file carrying four line numbers in another module that nothing here checks.
Review — RUNNER-2, the RPC surface (#90), round 1, agent-authoredVerdict: REQUEST CHANGES. The central claim is right, and I settled it by running it rather than reading it. The enumeration re-derives exactly. What holds this back is the named result itself: of the twelve rows, three state a shape from a site that does not constrain it, or state it short, and one of the three is the handoff RUNNER-3 will build Nine findings are inline. This comment carries the verdict, the measurements, and every finding including the ones that have no line to sit on. 1. The central claim, settled by observationNeither the RUNNER-1 round-1 review nor this PR had actually raised a
This PR's reading is correct and round 1 of #80 was wrong. A refusal is delivered; absence is the unrecoverable case. The 10 s on the first row is worker startup (aiter import ~8 s); the true delivery cost is the 2.05 s on the warm worker. All four AST-asserted steps hold, and the import-time raise is observed to work end to end. Two things the delivery does not do, both measured, both stated otherwise in
Neither changes the design decision — raising is still the right half of the fork — but this is the sentence RUNNER-3 will rely on when it decides whether to wrap its own refusals, so it should say what arrives. Observed on the import-raise path, ATOM's bug rather than this diff's but landing squarely on the device this PR introduces: 2. The twelve, and the twenty-six — both re-deriveDerived independently from source before reading the test. 39 call sites ( The fourteen leftovers account correctly (7 RapidServe-only, 7 on the rollout extension and on no class in 3. Shapes — three of twelve wrong or short (inline)
Everything else verified against its cited site and correct: 4. A surface the literal-only parse structurally cannot seeThree of the twelve are also called in-process on the runner itself, in 5. A hole the intersection hides (inline,
|
| tree | commit | passed | skipped | xfailed | rc |
|---|---|---|---|---|---|
| control = integration head | 669dc3f9d |
4496 | 149 | 3 | 0 |
merged = control + bb11c5e77 |
6bda771ca |
4561 | 149 | 3 | 0 |
merged − control = +65, decomposed and checked: test_runner_rpc_surface.py collects 48, test_runner_non_allocating.py collects 17, the two together run 65 passed. The author's +65 reproduces exactly at a different integration head, which is the strongest form this arithmetic can take.
The control question resolves too, and not by reading: the author's control (4704d27ec, 4397) sat two below their integration (14a197b07, 4399) because RUNNER-1 branched from 68ef4f329, before M1-1 landed — so the control carries RUNNER-1's tests and not M1-1's. At my head the comparison does not arise, because I measured the control at the integration head. scripts/compass/README.md's 4030 is now stale by 466 and was not used; note gate_cpu.sh:169 still prints that number in its own failure message.
gpu: not required on both runs, .compass-commit / .compass-changed stamps present so neither hit the exit-98 path. No flake in either run. ruff check and ruff format --check clean on all five touched files.
12. Effort — reproduced, with SLOC added
Measured against 4704d27ec, docstrings excluded from AST, SLOC = non-blank / non-comment / non-docstring:
| AST statements | SLOC | physical (net) | |
|---|---|---|---|
production (atom/compass/runner/) |
9 | 31 | +117 |
tests (tests/compass/) |
174 | 282 | +459 |
| total | 183 | 313 | +576 |
Against the 150-line estimate: AST 1.22x, SLOC 2.09x, physical 3.84x. Within the change, production runs 13.0x physical/AST and 3.4x SLOC/AST; tests 2.64x and 1.62x. The author's AST and physical figures reproduce exactly.
The instrument I would state the rule in is SLOC. AST charges one statement for the twelve-entry table, which is the densest object in the diff and the thing a reviewer has to check row by row against ATOM's source — so 1.22x understates precisely what this task was. Physical charges 117 lines for a production change that is 31 lines of code and 86 of prose and blanks, which makes a well-documented diff read as an overrun and quietly penalises writing the reasoning down. SLOC puts production at 31 and the whole at 313, and 2.09x is the ratio I would defend out loud. For #89 I would publish SLOC as the rule with AST beside it, because the two disagreeing by 1.7x is itself the signal that a diff is data- or prose-heavy — which is the thing a single number cannot say.
13. What a parent change would invalidate, and what I could not check
- This PR is stacked on a commit its parent has already left. Its merge-base is
4704d27ec;compass/runner-1-subclassis nowfda25e63d(round-2 docstring qualification,model_runner.pyonly, +10/−1). Nothing here depends on it, so no check of mine is invalidated — but the control figure in the PR body names a commit that is no longer the base, and a restack will move it again. exit, which this PR keeps, readsself.modelatmodel_runner.py:1051. That attribute exists only because RUNNER-1's_build_and_load_modelsetsself.model = UnbuiltModel(...). If round 2 of compass(runner): a model runner that constructs without device memory (RUNNER-1) #80 changes that override to leaveself.modelunset,exitstarts raising in the worker. Worth a note in RUNNER-1's review rather than a change here.- Not checked: multi-worker (TP>1) refusal delivery — I observed
proc_num=1only;AsyncIOProcManager.exit()joins each proc withtimeout=5before queueingSystemExit, so delivery latency scales with world size, and a refusal on rank != 0 has no primary output address of its own.async_proc_aggregationagainst a real registered connector — none exists in the tree yet. The GPU tier —gpu: not requiredon both runs, sogate_gpu.shwas not triggered. And nothing exercisedCompassModelRunnerinside a realEngineCore; that is RUNNER-3's.
What the next task in this area should watch
forward's contract is nine attributes and a pickle, not one attribute (finding 3). The parent gets a bare SystemExit with status 0, so if RUNNER-3 wants a refusal to be diagnosable in the engine's own log it has to put it there itself (finding 1). And dummy_execution will start answering the moment forward does — which is correct, and also means RUNNER-3 gets a second caller for free without a second test.
Accepted with reservation: the four non-stub decisions and the eager capture lists, each on evidence I re-derived rather than on the PR's account of them; the import-time raise as a device, with its message wrong and its test a substring grep. Checked and clean: the enumeration, the wait/no-wait table, nine of the twelve shapes, the exit reply oddity, the RapidServe defaulting, the gate arithmetic at a fresh head, the effort numbers, ruff.
…gaps Round-2 review response for the RPC surface (#90). The enumeration and the wait/no-wait table re-derived exactly under review; what changes here is three of the twelve stated shapes, the message the composed class raises, and two places where the derivation could shrink without going red. forward. Four sites broadcast it, not two: engine_core.py:386 and :1264, pp_engine_core.py:118 (waits and discards) and :379. The cited pp_engine_core.py:379-385 is _downstream_busy_loop, the last stage, and reads nothing off the reply -- the two nearby .req_ids reads are on `batch`, and fwd_out is handed whole to send_tokens. The .req_ids read is one ZMQ hop later, in the head, at :144-147, on what recv_tokens() returned at :139. And the contract is nine attributes plus picklable, not one: .req_ids, .token_ids, .draft_token_ids, .is_deferred_out, .logprobs, .get_idx(req_id), .num_rejected[idx], .num_bonus[idx], .dspark_ell.get(seq.id). The docstring now states all nine with the line that fixes each, since this is what a successor builds against. A new test recovers the nine from the consumers by AST rather than by substring, and pins the enclosing function of the .req_ids read so the last-stage/head confusion cannot recur. stop_profiler. {trace_dir, elapsed} was never a call-site constraint. It comes from ModelRunner.stop_profiler's own docstring; engine_utility.py:264 logs the reply whole and forwards it opaquely, llm_engine.py:300's .get("result", {}) is on the envelope, and "trace_dir" appears nowhere in the tree but the three lines that produce it. The header comment now says what a cited site actually fixes in each row -- shape where the caller unpacks or subscripts, non-None and picklable where it forwards -- and a test asserts the producer is the only file naming those keys. get_num_blocks. The cited range stopped one line before the key it named: state_runtime is subscripted at engine_core.py:141. And the fourth value is not "a dict" -- StateRuntime.from_wire (state_runtime.py:159-166) raises TypeError unless it is a Mapping and ValueError unless its keys are exactly {"transfer", "checkpoint_spec"}, in the parent, on the engine's first RPC. Both are now stated and both are now asserted, along with BlockManager's `assert num_blocks > 0`. The refusal docstring now says what arrives rather than only which direction it travels: the parent gets a bare SystemExit() with empty args, no message and no __cause__, so the refusal's name and reason stay on the worker's stderr; and SystemExit() carries code=None, so letting it propagate out of engine_core.py:132 -- a try/finally with no except -- exits the parent with status 0. A successor that wants its refusal readable in the engine's log has to log it worker-side before raising. The figures behind this stay in the PR body. A method that answers None is named as the same event as a missing one, since that is the case a plausible stub falls into by accident. The import-time refusal partitions _UNANSWERED on RPC_SURFACE instead of asserting every hole parks its caller. Two of the twelve are waited on by nobody: a missing `exit` means busy_loop never breaks, and a missing `process_kvconnector_output` means a KV load is silently never started. Both are real and neither is a park. This also gives RPC_SURFACE's boolean its first reader outside the tests. Two derivation gaps in the tests: - The leftover-14 test asserted two memberships and never checked the extension names are defined by anything. A newly dispatched name no class answers landed in `extension`, missed BASE, and stayed green while being a guaranteed park. It now asserts the 7/7 split and intersects `extension` against the rollout classes by name. - `arity` read only ast.Assign parents, so `return call_func(...)` scored 0, "discarded", when the value is the function's result -- engine_core.py:749, dummy_execution. The helper is handed to RUNNER-3 as the source of arity and attribute reads, so it now distinguishes ast.Expr (discarded) from ast.Return and argument positions (used whole), and dummy_execution's arity is asserted. Both silent filters in _call_sites -- the atom/compass skip and the non-literal first argument -- now report what they dropped, and a test asserts both are empty, so an f-string call site or a compass-side broadcast fails rather than vanishing. Scope recorded beside the table: RPC_SURFACE describes a runner reached through EngineCore. enable_rapidserve selects PrefillEngineCore/DecodeEngineCore independently of runner_qualname, and those broadcast seven wait_out=True names at whatever the qualname named, while config.py:1729-1736 substitutes the RapidServe runner only while runner_qualname is still ATOM's default. So enable_rapidserve with this runner is seven silent parks on names the table deliberately excludes. Tracked as #98; not closed here. Also recorded: three of the twelve are called in-process off the resume_memory RPC at other arities, where a refusal is caught rather than fatal, which a broadcast-derived enumeration cannot see. Smaller corrections: capture_cudagraph's reply deliberately disagrees with the attribute it preserves (base sends self.capture_sizes, i.e. [0]; this sends []), and engine_core.py:148 reaches it only when not enforce_eager and not disagg_is_decode -- replaced anyway, because a runner must not depend on a caller-side flag to stay off the device. The composed class's test says in its docstring that no CPU tier can execute that device, so a green gate is not evidence the surface is answered. Restacked onto RUNNER-1 at 1a30e6e. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bb11c5e to
2045c20
Compare
Round 2 — RUNNER-2, the RPC surface (#90)Head The three wrong shapes
The citation named There are four call sites, not two — And the contract is nine attributes plus picklable, now stated in full with the line that fixes each:
Plus picklable: under PP the last stage sends the object whole ( On
Fixed by correcting the claim rather than the row. The header comment above
The import-time raiseThe device is kept; the message is fixed. It now partitions The test for the device now also says in its own docstring that no CPU tier can execute it (importing the composed class imports The refusal docstringCorrected to what arrives, not just the direction: the parent gets a bare Per RUNNER-1 round 3, the figures stay here and out of the docstring — nothing in CI reads a docstring, so a number in one rots while the suite stays green. For the record, the reviewer's measurement on node 18 against a real
The bare The two test gaps, with seeded violationsEach fix is shown failing on a seeded breach rather than asserted to work. Gap 1 — the leftover-14 test. Now asserts Seed A, a name no class answers ( Seed B, the case that shows the subset check is not redundant with the count — a rename, which keeps the counts at 7/7 ( Every assertion the round-1 test had is green on seed B. Only the new one catches it. Gap 2 — The assertion is new; Both silent filters. The new hole — recorded and filedOpened #98: One thing I did differently from the suggestion: I did not add a refusal in the composed class when Also recorded beside the table, from finding 4: three of the twelve are called in-process off the Corrections to the record
GatesIntegration head read 2026-09-21 19:09:57 UTC (GitHub's own Both trees staged
Delta +69, decomposed and checked: Control at
Effort — three instrumentsMeasured against the new base
Against the 150-line estimate: AST 1.63x, SLOC 2.90x, physical 6.09x. The instrument reproduces the reviewer's round-1 figures exactly — 9 / 31 / +117 production, 174 / 282 / +459 tests, 183 / 313 / +576 total, 1.22x / 2.09x / 3.84x — which is the only reason I trust the round-2 row. For #89 I agree with the reviewer's recommendation and would carry all three, publishing SLOC as the rule with AST beside it. The AST/SLOC disagreement here is 1.78x (2.90 / 1.63), up from 1.71x in round 1. That gap is not noise, it is the measurement: AST charges one statement for a twelve-row table that a reviewer has to check row by row against ATOM's source, and physical charges +914 for a change that is 435 lines of code and the rest prose and blanks — which makes documenting the reasoning read as an overrun. Two numbers disagreeing by 1.7-1.8x is precisely the signal that a diff is data- or prose-heavy, and that is the thing one number cannot say. What I disagreed with, and the measurementNothing in the findings. Two notes. The only substantive divergence is the RapidServe refusal, above: filed as #98 against And one correction to my own reply, since it looked briefly like the review being wrong: Still not checkedUnchanged from round 1, and none of it is mine to close here: multi-worker (TP>1) refusal delivery; |
…ller (RUNNER-2) A worker resolves each RPC with `getattr(runner, name, None)` and forwards the result only `if out is not None`, so the two ways this surface breaks are both silent: a name the runner does not have is skipped and the caller blocks on an unbounded queue read, and a method that answers None does the same one step later. Raising is the loud path -- the worker dies, the manager's monitor turns that into a SystemExit on the output queue, and call_func re-raises it. So the twelve names are not a list anyone typed here. They are recovered from ATOM's source: every name broadcast through call_func / call_func_with_aggregation, intersected with the methods ModelRunner defines. That intersection is exactly twelve, and RPC_SURFACE records for each whether its caller waits. One of the twelve needed replacing. ATOM's capture_cudagraph zeroes device buffers, opens a graph memory pool and enters a capture context before it reaches the model, so a runner with no weights would have put bytes on a device before finding that out. It now answers the three values engine_core unpacks -- no seconds, no sizes, no pool bytes -- and leaves the eager capture_sizes alone. The rest of the surface keeps ATOM's own implementations, which need neither weights nor a device: replacing process_kvconnector_output or async_proc_aggregation with a stub would cut the connector out of the path it exists to drive, and dummy_execution refuses through this runner's own forward. The binding module now refuses to import a class that leaves any of the twelve unanswered, turning a caller that would wait forever into a worker that dies at construction with a traceback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ock form Two things a mid-build correction put on this surface, both now asserted rather than read. `busy_loop` dispatches under no `try`, so a refusal unwinds out of it, out of `AsyncIOProc.__init__` -- the process target -- and the worker exits. That alone would leave the caller parked on an unbounded queue read. What prevents it is on the manager side: a monitor thread started from `__init__`, before any RPC can be broadcast, waits on the process sentinels and calls `exit()`, which queues a `SystemExit` for the parked caller *before* it finalizes anything, and every wait on that path is bounded. The ordering is what makes the difference between a traceback and a deadlock, so the test asserts it from the AST instead of trusting a reading. `RapidServeModelRunner.get_num_blocks` answers two of the four keys the caller reads. It is not a breach -- the caller defaults the two pool-entry keys -- but it is the obvious model to copy for a runner answering without a device, and the shortfall is invisible at zero blocks and not invisible above them. Pinned, for whoever ends this runner's refusal. Also corrects the `busy_loop` line span cited in the module docstring (`231-252`, not `231-250`) and replaces an assertion whose first two terms were constant-truthy f-strings with the three equalities it meant to check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…gaps Round-2 review response for the RPC surface (#90). The enumeration and the wait/no-wait table re-derived exactly under review; what changes here is three of the twelve stated shapes, the message the composed class raises, and two places where the derivation could shrink without going red. forward. Four sites broadcast it, not two: engine_core.py:386 and :1264, pp_engine_core.py:118 (waits and discards) and :379. The cited pp_engine_core.py:379-385 is _downstream_busy_loop, the last stage, and reads nothing off the reply -- the two nearby .req_ids reads are on `batch`, and fwd_out is handed whole to send_tokens. The .req_ids read is one ZMQ hop later, in the head, at :144-147, on what recv_tokens() returned at :139. And the contract is nine attributes plus picklable, not one: .req_ids, .token_ids, .draft_token_ids, .is_deferred_out, .logprobs, .get_idx(req_id), .num_rejected[idx], .num_bonus[idx], .dspark_ell.get(seq.id). The docstring now states all nine with the line that fixes each, since this is what a successor builds against. A new test recovers the nine from the consumers by AST rather than by substring, and pins the enclosing function of the .req_ids read so the last-stage/head confusion cannot recur. stop_profiler. {trace_dir, elapsed} was never a call-site constraint. It comes from ModelRunner.stop_profiler's own docstring; engine_utility.py:264 logs the reply whole and forwards it opaquely, llm_engine.py:300's .get("result", {}) is on the envelope, and "trace_dir" appears nowhere in the tree but the three lines that produce it. The header comment now says what a cited site actually fixes in each row -- shape where the caller unpacks or subscripts, non-None and picklable where it forwards -- and a test asserts the producer is the only file naming those keys. get_num_blocks. The cited range stopped one line before the key it named: state_runtime is subscripted at engine_core.py:141. And the fourth value is not "a dict" -- StateRuntime.from_wire (state_runtime.py:159-166) raises TypeError unless it is a Mapping and ValueError unless its keys are exactly {"transfer", "checkpoint_spec"}, in the parent, on the engine's first RPC. Both are now stated and both are now asserted, along with BlockManager's `assert num_blocks > 0`. The refusal docstring now says what arrives rather than only which direction it travels: the parent gets a bare SystemExit() with empty args, no message and no __cause__, so the refusal's name and reason stay on the worker's stderr; and SystemExit() carries code=None, so letting it propagate out of engine_core.py:132 -- a try/finally with no except -- exits the parent with status 0. A successor that wants its refusal readable in the engine's log has to log it worker-side before raising. The figures behind this stay in the PR body. A method that answers None is named as the same event as a missing one, since that is the case a plausible stub falls into by accident. The import-time refusal partitions _UNANSWERED on RPC_SURFACE instead of asserting every hole parks its caller. Two of the twelve are waited on by nobody: a missing `exit` means busy_loop never breaks, and a missing `process_kvconnector_output` means a KV load is silently never started. Both are real and neither is a park. This also gives RPC_SURFACE's boolean its first reader outside the tests. Two derivation gaps in the tests: - The leftover-14 test asserted two memberships and never checked the extension names are defined by anything. A newly dispatched name no class answers landed in `extension`, missed BASE, and stayed green while being a guaranteed park. It now asserts the 7/7 split and intersects `extension` against the rollout classes by name. - `arity` read only ast.Assign parents, so `return call_func(...)` scored 0, "discarded", when the value is the function's result -- engine_core.py:749, dummy_execution. The helper is handed to RUNNER-3 as the source of arity and attribute reads, so it now distinguishes ast.Expr (discarded) from ast.Return and argument positions (used whole), and dummy_execution's arity is asserted. Both silent filters in _call_sites -- the atom/compass skip and the non-literal first argument -- now report what they dropped, and a test asserts both are empty, so an f-string call site or a compass-side broadcast fails rather than vanishing. Scope recorded beside the table: RPC_SURFACE describes a runner reached through EngineCore. enable_rapidserve selects PrefillEngineCore/DecodeEngineCore independently of runner_qualname, and those broadcast seven wait_out=True names at whatever the qualname named, while config.py:1729-1736 substitutes the RapidServe runner only while runner_qualname is still ATOM's default. So enable_rapidserve with this runner is seven silent parks on names the table deliberately excludes. Tracked as #98; not closed here. Also recorded: three of the twelve are called in-process off the resume_memory RPC at other arities, where a refusal is caught rather than fatal, which a broadcast-derived enumeration cannot see. Smaller corrections: capture_cudagraph's reply deliberately disagrees with the attribute it preserves (base sends self.capture_sizes, i.e. [0]; this sends []), and engine_core.py:148 reaches it only when not enforce_eager and not disagg_is_decode -- replaced anyway, because a runner must not depend on a caller-side flag to stay off the device. The composed class's test says in its docstring that no CPU tier can execute that device, so a green gate is not evidence the surface is answered. Restacked onto RUNNER-1 at 1a30e6e. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
RUNNER-1's round-3 review left this here because this is where a returning body first appears on the surface, and attributing the park solely to absence sends whoever is debugging the hang to check whether the method is there, find that it is, and stop. It is one line below the skip, not a separate mechanism: async_proc.py:243 is `if out is not None:` and both of busy_loop's put_nowait calls -- the primary output queue at :248 and the KV queue at :250 -- are inside it, while the getattr skip is at :237-239. From the caller's side an answer of None and a method that was never defined are one event. The test for it was one substring. It now asserts the structure: exactly one `out is not None` guard in busy_loop, and the set of put_nowait line numbers inside that guard equal to the set in the whole loop. Moving either put out of the guard fails it, which a substring check cannot see. Restacked onto the integration head at 1b473e5, which carries RUNNER-1 (#80) squashed. No longer stacked on anything. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2045c20 to
4ce635c
Compare
Round 2, addendum — restacked onto the landed integration headThe round-2 comment above was written against Restack
One more finding, from RUNNER-1's round 3, and it is mineRUNNER-1's handoff attributed the park solely to absence. That is half the mechanism, and the missing half belongs here because this is where a returning body first appears on the surface.
The module docstring now says that, and the test that covered it was one substring ( The substring version passes on that seed. That makes four seeded violations in this round, one per fix.
|
| tree | commit | passed | skipped | xfailed | rc |
|---|---|---|---|---|---|
| control = integration head | 1b473e5af |
4518 | 149 | 3 | 0 |
| merged = control + this branch | 4ce635c8b |
4570 | 149 | 3 | 0 |
Delta +52 — this task's own tests only, as expected. test_runner_rpc_surface.py collects 52, and 4518 + 52 = 4570. RUNNER-1's test_runner_non_allocating.py (17) is now inside the control rather than in the delta, which is exactly what landing #80 does to this arithmetic.
The three control readings are consistent: 4496 at 354965883, 4518 at 1b473e5af = 4496 + 17 (RUNNER-1) + 5 (#82). And the same branch measured +69 = 52 + 17 against 354965883 before #80 landed, which is the same 52 with RUNNER-1's 17 still on my side of the line.
commit: named the right sha on both runs and gpu: not required (.compass-changed stamp) printed on both, so neither hit the exit-98 path. No flake: TestTheRegionIsNotCopiedPerChunk passed in both and the skip counts are identical at 149/149. ruff check / ruff format --check clean on all five touched files.
Effort, re-measured against 1b473e5af
| AST statements | SLOC | physical (net) | |
|---|---|---|---|
production (atom/compass/runner/) |
11 | 37 | +226 |
tests (tests/compass/) |
238 | 423 | +728 |
| total | 249 | 460 | +954 |
AST 1.66x, SLOC 3.07x, physical 6.36x against the 150-line estimate. The AST/SLOC disagreement is now 1.85x (was 1.71x in round 1), which strengthens rather than weakens the recommendation for #89: publish SLOC as the rule with AST beside it, because two instruments disagreeing by 1.7–1.9x is itself the signal that a diff is data- or prose-heavy.
The instrument reproduces the reviewer's round-1 figures exactly — 9 / 31 / +117 production, 174 / 282 / +459 tests, 183 / 313 / +576 total — which is the only reason to trust these rows.
Unchanged from the round-2 comment
The three shape corrections, the partitioned import-time message, both derivation-gap closures with their seeded violations, #98 for the RapidServe hole, and the provenance correction (#74 touches zero lines under atom/model_engine/; the keys came from 042e8f1c0, and c2d40e2dd wrote the short RapidServe form after the four-key contract existed — both verified by git show --numstat).
| "process_kvconnector_output": False, # engine_core.py:500 does not wait | ||
| "async_proc_aggregation": True, # engine_core.py:488, the one bounded wait | ||
| "start_profiler": True, # engine_utility.py:252 forwards the reply unread | ||
| "stop_profiler": True, # engine_utility.py:264 forwards the reply unread |
There was a problem hiding this comment.
The replacement states two of the three legs round 1 named — the JSON one is gone.
Withdrawing {trace_dir, elapsed} is right, and the shape-vs-forwarding distinction in the header is the correct repair. I checked the claim that holds it up: trace_dir really does appear in exactly one file under atom/ — atom/model_engine/model_runner.py, at :1152 (its own docstring), :1156 and :1171. Nothing reads it.
But the header now says a forwarding site fixes only "non-None and picklable", and for these two rows there is one more constraint, imposed by a real in-tree caller. The reply is forwarded opaquely three hops and then serialised:
engine_utility.py:264 result = call_func("stop_profiler", wait_out=True)
engine_utility.py:269 ("UTILITY_RESPONSE", {"cmd": "stop_profile", "result": result})
llm_engine.py:300 return [resp.get("result", {}) for resp in responses] # the envelope
api_server.py:2441-2443 traces = engine.stop_profile()
return {"status": ..., "message": ..., "traces": traces}
atom/entrypoints/openai/api_server.py:2435-2443 is a FastAPI route, so the reply ends up inside a JSON response body. Round 1's own sentence was "non-None, picklable, JSON-serialisable"; two of those three survived the fix. ATOM's own {trace_dir: str, elapsed: float} satisfies it, so nothing is broken today — but this row's whole purpose is to tell a successor what a replacement owes, and a picklable reply that is not JSON-encodable turns /stop_profile into a 500 rather than a park.
One clause in the header, or one more assertion in test_the_profiler_replies_are_forwarded_whole_and_never_unpacked — the route already forms a grep as stable as the envelope string that test pins. Non-blocking.
To be explicit about the loop-stop rule: this is not round 1's finding surviving. Round 1's was "the cited site does not constrain the shape", and that is closed. This is the replacement being one leg short.
| for p in (REPO / "atom").rglob("*.py") | ||
| if "trace_dir" in p.read_text() | ||
| } | ||
| assert producers == {"atom/model_engine/model_runner.py"} |
There was a problem hiding this comment.
This is a package-wide glob, and the red it can produce is one no branch's gate can see.
The assertion is the right idea — the day something starts reading that key, this goes red and the row can be promoted to a real constraint. The scope is what I would narrow.
producers is built by reading every .py under atom/, atom/compass/ included, and compared for set equality. So a future Compass module that happens to name a local trace_dir — a capture or calibration module is the obvious candidate — fails this test without falsifying anything it claims. Worse, it fails it on the integration branch while both contributing PRs are green on their own gates, because neither branch contains both halves. That is the failure mode where two green PRs make a red merge, and per-branch gating structurally cannot see it.
I measured the exposure rather than guessing at it: zero collisions today, across all 41 fork/compass/* branches on the fork. (fork/compass/integrate-cc-memory matched a call_func grep, but those are def call_func / def call_func_with_aggregation definitions in atom/compass/replay/local_proc.py, which _call_sites cannot pick up since it matches only ast.Call on an ast.Attribute. No live collision there either.)
So this is a latent coupling, not a live break. Excluding atom/compass/ from producers keeps the tripwire's whole meaning — it exists to catch a consumer of the profiler reply appearing in ATOM, and a Compass-side string is not one — while removing the cross-branch edge:
if "trace_dir" in p.read_text() and "compass" not in p.partsThe sibling assert FROM_COMPASS == [] in test_both_filters_in_the_derivation_drop_nothing has exactly the same shape, but there the coupling is the point and the docstring says so, so I am not asking for that one to change. Non-blocking.
Review — RUNNER-2, the RPC surface (#90), round 2, agent-authoredVerdict: APPROVE. This is landable. Head No round-1 finding survives. All nine inline threads are closed on evidence I re-derived rather than on the replies. Three new findings, all non-blocking, two of them inline. The loop-stop rule is not engaged: finding A is not a survival of round 1's Everything here was measured in the container or on node 18; nothing is quoted from the PR body. 1. The three corrected shapes — re-derived, and none of the three over-corrects
No tenth, and none in Four sites, with the parent node that decides whether the reply is read:
The enclosing-function test does what it claims, and I made it fire. So the exact last-stage/head confusion round 1 found cannot recur silently. One limit worth stating rather than raising: it pins the function, not the line, so a move within
2. The seeded violations — I ran five, each reproduces the quoted failure exactlyRun in
Seed 2 is the one that mattered and it behaves exactly as claimed. The rename reached line 223, which means Seed 4: I confirmed the substring version passes on the same seed. After dedenting the KV put, 3. The
|
| variant | picked | outcome |
|---|---|---|
| unmodified | from_wire at :158 |
assertion passes |
decoy nested class Inner with its own from_wire, placed first in StateRuntime's body |
still :158-equivalent, the direct method |
assertion passes — ast.walk is breadth-first, so a depth-1 method always beats a nested one |
duplicate top-level class StateRuntime later in the file |
the decoy | assertion FAILS, loudly |
StateRuntime.from_wire renamed away |
nothing | StopIteration, loudly |
| renamed away and a nested decoy present | the decoy | assertion FAILS, loudly |
Every wrong-pick path is loud, because the assertion pins the literal expected set and that set is exactly what differs between the three candidates. The defect the test exists to prevent is closed. No change asked.
One correction to the record, in finding C: the file has two from_wire classmethods, not three.
4. The twelve-name partition — right for all twelve
wait_out is keyword-only with default False (async_proc.py:425), and call_func_with_aggregation (:436) takes no wait_out at all and always waits with timeout=10.0. So a site with no wait_out kwarg does not wait. Exactly six sites have no kwarg, and they are exactly the two unwaited names: process_kvconnector_output at engine_core.py:378, :500 and pp_engine_core.py:113, :232, :369; and exit at engine_core.py:260. Every other site passes the literal True — zero sites pass a variable or a non-literal, so the derivation cannot be fooled by an unevaluable keyword. waits is unanimous per name across all 26 dispatched names.
{exit, process_kvconnector_output} is therefore the correct unwaited partition, and RPC_SURFACE's booleans match site-by-site for all twelve. The message at model_runner.py:66-75 is the first production read of that boolean, which is where round 1 said it belonged, and the two consequences that are facts about the names rather than about the runtime set sit in the comment above rather than in the string — the right split.
5. The RapidServe hole — declining to guard is right, on measurement
I re-derived the chain: config.py:1730-1736 substitutes RapidServeModelRunner only while runner_qualname is ATOM's default string; llm_engine.py:140 selects DisaggCoreManager from enable_rapidserve alone; the seven broadcasts are PrefillEngineCore :872, :891, :919, :992 and DecodeEngineCore :1097, :1149, :1179, and I confirmed all seven carry the literal wait_out=True. Issue #98 is open and its body states the mechanism accurately.
Declining is right, for three reasons I can measure rather than assert:
- The condition is not this runner's. It holds for any
runner_qualnamethat is not ATOM's default — nothing in the broadcast path consults the qualname at all. - This task's boundary is that no ATOM file is modified, and the correct fix is an ATOM file. A Compass-only guard would close Compass and leave the general case silent, which is the worse of the two half-fixes.
- A Compass-side refusal would be a worse diagnostic than the hang for the reader most likely to see it. There is no clean place for it: the class deliberately has no
__init__, andenable_rapidserveis only visible viaself.configafter the base__init__has run, so the guard would have to fire inside the worker. This PR's own measured delivery says a worker-side raise arrives at the parent as a bareSystemExit()withcode=None, andengine_core.py:132is atry/finallywith noexcept— so the guard would convert a hang into a silent parent exit with status 0. TheConfigvalidation RapidServe engine cores broadcast seven waited RPCs at whatever runner_qualname named #98 asks for lands in the parent, at configuration time, with a message, before any process is forked. That is strictly better, and it is what the author chose.
The minimum that was owed here is present: the scope — "a runner reached through EngineCore" — is recorded beside RPC_SURFACE (overrides.py:94-105), because nothing else in the tree says it. The in-process surface a broadcast-derived enumeration structurally cannot see is recorded too, and I checked its three citations: rollout/memory_manager.py:176 subscripts one key of get_num_blocks, :183 discards allocate_kv_cache, :209 calls capture_cudagraph unremarked inside a try that degrades to enforce_eager=True.
The hole stays open until #98 lands. Nobody should read this PR's green surface as covering it, and the table now says so.
6. Gates — re-derived at the head I read
Integration head read 2026-09-21 19:33:07 UTC (GitHub Date header) as 1b473e5af, re-read at 19:38:50 UTC, unchanged. Both trees staged with each tree's own scripts/compass/snapshot.sh (git archive, stamps written), scp → docker cp → extract under /tmp/rev90r2gate/, md5 verified host → node → container (1c15abb3… control, ef92e91b… merged, identical at all three hops), run from each tree's own gate_cpu.sh, sequentially, in container xiaobizh_n18_cpu. Output redirected to a file, never piped, so each gate's own exit code was read. /tmp/xiaobizh-compass/ATOM was confirmed present and left untouched; my staging area was removed from both the node and the container afterwards.
| tree | commit | passed | skipped | xfailed | rc |
|---|---|---|---|---|---|
| control = integration head | 1b473e5af |
4518 | 149 | 3 | 0 |
| merged = the PR head | 4ce635c8b |
4570 | 149 | 3 | 0 |
Delta +52, and it decomposes: tests/compass/test_runner_rpc_surface.py collects and passes 52 (run separately in the local GPU container: 52 passed), so 4518 + 52 = 4570. The author's figures reproduce exactly, including the +52 and the consistency check that RUNNER-1's 17 are now inside the control. Since the branch is now based directly on 1b473e5af, "merged" and "the PR head" are the same tree, which removes the merge step as a source of divergence.
commit: named the right sha on each (1b473e5af (stamp), 4ce635c8b (stamp)), gpu: not required (.compass-changed stamp) on both with the five changed paths listed, so neither hit the exit-98 path. GATE_CPU_RC=0 printed exactly once per run. No flake: skip counts identical at 149/149 and TestTheRegionIsNotCopiedPerChunk does not appear in either log — the class-wide flake did not fire in either direction, so no red needed checking against it.
ruff check and ruff format --check clean on all five touched files (All checks passed!, 5 files already formatted).
7. Effort — reproduced independently, and the overrun is the review's cost
My own instrument, written before reading the author's: AST = ast.stmt nodes excluding docstring Exprs, SLOC = non-blank / non-comment / non-docstring physical lines via tokenize, physical = git diff --numstat net, all as HEAD-minus-base per file.
against 1b473e5af |
AST | SLOC | physical |
|---|---|---|---|
production (atom/compass/runner/) |
11 | 37 | +226 |
tests (tests/compass/) |
238 | 423 | +728 |
| total | 249 | 460 | +954 |
| vs 150 | 1.66x | 3.07x | 6.36x |
Every figure reproduces the author's exactly. So does round 1 on the same instrument against 4704d27ec: 9 / 31 / +117 production, 174 / 282 / +459 tests, 183 / 313 / +576 total, 1.22x / 2.09x / 3.84x. Two independent implementations agreeing to the unit on six numbers is as much as this instrument can be asked for.
The ruling: 3.07x is a genuine overrun on the instrument, and it is the review's cost, not a mis-cut task. I measured that rather than asserting it. Production grew +6 SLOC between rounds (31 → 37). The test file grew +141 SLOC, and per changed unit every line of it is a round-1 finding:
| unit | Δ SLOC | round-1 finding it answers |
|---|---|---|
test_forward_..._nine_attributes (replacing ..._an_object_not_a_tuple) |
+34 − 10 | forward: nine attributes, AST recovery replacing a substring |
test_the_pp_reply_is_read_in_the_head_... |
+31 | forward: the wrong-loop citation |
test_a_reply_is_forwarded_only_when_it_is_not_none |
+25 | the None park, substring → structure |
test_the_fourth_value_is_a_two_key_wire_dict... |
+20 | get_num_blocks: the nested wire dict |
test_the_profiler_replies_are_forwarded_whole... |
+11 | stop_profiler: the shape that is not a constraint |
_arity +7, test_dummy_execution... +1 |
+8 | arity read only ast.Assign |
EXTENSION_CLASSES / ROLLOUT +11, test_the_dispatched_names_outside... +2 |
+13 | the leftover-14 test checked nothing defined them |
_call_sites +2, test_both_filters_... +3 |
+5 | the two silent filters |
test_the_binding_module_refuses... +2, test_a_refusal_reaches_the_caller... +1, imports/module +4 |
+7 | the partition; the bare SystemExit |
| +141 | = the whole of the round-2 test growth |
So the number grew, and it should have. AI_DEV_RULES treats a >2x overrun as a halt because the usual cause is a mis-cut task; here the cause is named and it is not that. The production change is 0.25x the estimate. What the estimate never covered was a test side that derives its subject from ATOM's source instead of restating it — and that derivation is the entire value of this task, since the enumeration is the deliverable.
For #89, my measurement and my vote. SLOC as the rule with AST beside it, which makes six reviewers. My reason is the one this round supplies and round 1 could not: the two instruments disagree by 1.85x here (3.07 / 1.66) against 1.71x in round 1, and the gap grew in the round where the added work was almost entirely assertions over ATOM's AST. AST charges one statement for a twelve-row table and one for a nine-name set literal, which is precisely the work a reviewer re-derives row by row; physical charges +954 for 460 lines of code, which prices a documented diff as an overrun and taxes writing the reasoning down. I would add one thing to #89 that neither number carries: publish the estimate's own scope. A 150-line estimate against a 37-SLOC production change and a 423-SLOC test suite is not 3.07x over — it is an estimate that measured a different thing, and no choice of instrument fixes that.
Findings
A. stop_profiler / start_profiler: the replacement states two of the three legs round 1 named. (inline, overrides.py:127, non-blocking) The header now says a forwarding site fixes "non-None and picklable". The reply also reaches atom/entrypoints/openai/api_server.py:2439-2443, where it is returned as {"traces": traces} from a FastAPI route — so the callers impose non-None, picklable and JSON-encodable. Round 1's own sentence was "non-None, picklable, JSON-serialisable". One clause, or one assertion on the route.
B. The trace_dir tripwire is a package-wide glob, and the failure it can cause is one no branch can see. (inline, tests/compass/test_runner_rpc_surface.py:660, non-blocking) producers is built by reading every .py under atom/, including atom/compass/, and compared for equality. A Compass module with a local trace_dir would fail it without falsifying its claim, and it would fail on the integration branch while both contributing PRs are green. Measured: zero collisions today across all 41 fork/compass/* branches. Excluding atom/compass/ from producers keeps the tripwire's meaning — it exists to catch a consumer of the profiler reply appearing — and removes the coupling. The sibling assert FROM_COMPASS == [] has the same shape but is deliberate and documented as such, so I am not asking for it to change.
C. Record correction: state_runtime.py defines two from_wire classmethods, not three. (standalone, non-blocking) StateTransfer.from_wire (:88) and StateRuntime.from_wire (:158). The third, PagedStateCheckpointSpec.from_wire, lives in atom/model_engine/page_unit_checkpoint.py and is only called from state_runtime.py:171. The substance of the story is unaffected and I verified it: an unscoped next(n for n in ast.walk(module) ...) does select line 88, StateTransfer's, whose expected is {"kind", "fork_tokens", "paged_layout_id", "readable_midstep"} — exactly the four names that briefly looked like the review being wrong.
Round-1 findings: all nine closed
| # | finding | closed by, as I verified it |
|---|---|---|
| 1 | forward cited the wrong loop; two sites not four; one attribute of nine |
all three re-derived independently; the enclosing-function test seeded and fired |
| 2 | stop_profiler's cited site fixes nothing of the sort |
claim corrected rather than the row; trace_dir producer set verified — see A for the residue |
| 3 | get_num_blocks range short by one; fourth value tighter than "a dict" |
:132-141 with a line per key, TypeError/ValueError in the parent, assert num_blocks > 0, all confirmed at source |
| 4 | the import-raise asserted something false about two of the twelve | partitioned on RPC_SURFACE[name]; partition verified correct for all twelve from wait_out's default |
| 5 | the refusal docstring described a delivery that does not arrive | bare SystemExit, code=None, status 0 and the atexit AttributeError all recorded, and the empty-args case asserted |
| 6 | the leftover-14 test asserted nothing defined the names | 7/7 and extension <= EXTENSION_CLASSES; both seeds run, the rename slips the count and is caught only by the subset |
| 7 | arity read only ast.Assign |
_arity helper; seed reproduces [0] == [1]; capture_cudagraph still {3} and forward still {0, 1} |
| 8 | the RapidServe hole | scope recorded beside the table, chain re-derived, #98 open — declining to guard ruled correct above |
| 9 | the eager capture lists | both notes in the docstring; no reader treats the surviving [0] as a capture |
Accepted with reservation: nothing. Checked and clean: the enumeration (39 sites — engine_core 19, pp_engine_core 11, engine_utility 9 — 26 names, 12 / 7 / 7, zero non-literal, zero from compass), the wait/no-wait table, all twelve shapes, the four non-stubs, the exit reply oddity, the gate arithmetic at a fresh head, the effort figures on an instrument I wrote myself, ruff.
What I could not check
Unchanged from round 1, and none of it is this PR's to close: multi-worker (TP>1) refusal delivery; async_proc_aggregation against a real registered connector, which does not exist in the tree; the GPU tier, not triggered on either run; and the composed CompassModelRunner inside a real EngineCore, which is #97's. I did not re-execute the six-runner refusal experiment — round 1 settled it, and nothing in this delta moves it. I also did not verify that a FastAPI route would actually 500 on a non-JSON-encodable reply; finding A rests on reading the route, not on driving it.
What the next task in this area should watch
forward's contract is nine attributes and a pickle, and the nine are now recovered from the consumers by AST, so RUNNER-3 should extend that assertion rather than restate the list. A refusal arrives as a bare SystemExit() and exits the parent with status 0, so anything that needs to be diagnosable in the engine's log must be logged worker-side first. dummy_execution answers the moment forward does. And the RapidServe hole is open until #98 lands — a green surface here says nothing about enable_rapidserve=True.
Agent-authored review, round 2. Not merged, not landed, not undrafted, nothing pushed.
…duler (RUNNER-3) `forward` reported a refusal; it now reports what a step produced. Three rules decide that reply, and each is wrong in a way nothing raises on: - the tokens belong to the previous output-producing batch, not this one; - the lag is counted in output-producing steps, so a run of pure middle chunks does not advance it; - a batch that samples nothing names its requests and reports no tokens. The rules live in `step_output`, which imports nothing from the engine and so runs where there is no driver; `overrides.forward` builds ATOM's reply object from them, taking the one engine import at call time. `forward` keeps `torch.inference_mode` and not `with_eplb_forward_monitor`: the monitor commits an expert-load window from what a real forward routed, and a predicted step routes nothing. That reasoning stands on its own; what keeps a successor from inheriting a silently different execution context is the new assertion pinning all three decorator lists against ATOM's source, not the one other runner that happens to agree. A speculative config is refused rather than reported. The reply drafts nothing, so `scheduler.py:2579` never fills `spec_token_ids` and `:2521-2522` reads zeros out of correctly sized arrays: a well-formed description of a run with speculation off, offered as a prediction of a run with it on. The shapes are right and the semantics are not modelled, so the configuration is named. The tests drive the real `Scheduler`, `BlockManager` and `Sequence` in the loop the engine runs them in -- including `compute_detailed_aggregates`, which sits between `schedule()` and `forward` at `engine_core.py:385` -- over a four-request prefill streak, and carry the two wrong implementations as controls. A third control records the one of the three that the scheduler cannot tell apart at all. The reported token id now has a driven control too. Both of the scheduler's stop checks are gated on `not seq.ignore_eos` (`scheduler.py:2630`, `:2633`), so a run that sets `ignore_eos=True` says nothing about which id is safe to report; `_drive` takes it as a parameter, and reporting the end-of-text id or a configured stop id ends every request on its first decoded token. `test_only_the_binding_module_reaches_the_engine` reads import-time scope rather than every import node or the top level alone: it walks and prunes at `def`/`lambda`, so `forward`'s call-time import no longer counts while an import nested in a module-scope `try:`/`except ImportError:`, in a module-scope `if`, or in a class body still does. Six samples check the predicate against what the interpreter runs when each is imported. The `try:` form is why the extent matters: a driverless collection would not fail on it, because the `except` swallows the failure. `forward` cannot answer None -- the one contract on this surface whose breach is a hang rather than a traceback, since `async_proc.py:243` queues nothing for a None reply. Pinned as one return with a value, that return last in the body, and every reply of the driven run. Restacked onto the integration head 83ef2a0, which carries RUNNER-2 (#90) squashed on top of RUNNER-1 (#80). No longer stacked on anything. The `forward` docstring keeps RUNNER-2's account of the nine reads a replacement owes its callers, which the restack merged with this method's own paragraphs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tened (#200) Closes #193. `atom/compass/runner/__init__.py`'s package docstring said, of the `RPC_SURFACE` check: > the worker skips a name the runner lacks without raising, so a hole in that > surface is a caller that waits forever rather than an error The first half is true. The second is false for both entries the table records as unwaited, **re-verified at this branch's base (`92f1fdafe`) before anything was changed**: | name | broadcast at | `wait_out` | what a hole actually costs | |---|---|---|---| | `exit` | `engine_core.py:260` | default `False` (`async_proc.py:425`) | nobody waits. `busy_loop`'s `if func_name == "exit": break` sits beside the per-runner loop, not inside it, so the loop still breaks. What is lost is `ModelRunner.exit` (`model_runner.py:1038`): `destroy_dist_env()` never runs, and the graphs and the five KV tensors it `delattr`s stay held | | `process_kvconnector_output` | `engine_core.py:378`, `engine_core.py:500`, `pp_engine_core.py:113`, `pp_engine_core.py:232`, `pp_engine_core.py:369` | default `False` at all five | nobody waits. `connector.start_load_kv` (`model_runner.py:3343-3348`) is never reached and an async KV load is silently never started | The other ten entries are `wait_out=True` (or the one aggregating form), and for those the sentence was right. So the docstring named the one failure mode that cannot occur at 2 of 12, and sent a reader debugging a leak or a missing KV load to look for a park that does not exist. **The issue estimated two call sites for `process_kvconnector_output`; there are five.** The first draft of this change cited the two the issue named, and the new site test failed on it. The prose now cites all five. ## The correction The docstring now partitions the surface per entry the way `model_runner.py`'s composed-class refusal already does, rather than softening the claim to something true of neither half: a hole in a waited name parks its caller for the life of the process; a hole in an unwaited one parks nobody and loses the work the name stood for, named for each. It defers to `overrides`' reply-contract docstring for the mechanism — including that a present method returning `None` is the same event to a caller as an absent one (`async_proc.py:243`) — rather than repeating it. Principle 8; principle 3 for not restating `overrides`. ## The audit of the rest of that docstring **22 claims checked, 19 true, 3 false.** The other two false ones are fixed in the same commit because they are the same docstring: 1. *"Two modules, split by what each is allowed to import"* — the package has held **three** since `step_output` landed in #97, one PR after this docstring was written in #90. Nothing went red, because a count in prose has nothing to disagree with. Replaced by a bullet per module, pinned against the directory. 2. *"`overrides` ... imports nothing from the engine"* — `overrides.forward` imports `ScheduledBatchOutput` from `atom.model_engine.scheduler` at call time (`overrides.py:366`), deliberately and with its own comment saying why. `overrides`' own docstring already says "at module scope"; this one had dropped the qualifier. The qualifier is restored and the call-time import is named. The 19 true ones include the aiter/`rocminfo` chain (`model_runner.py:19` imports `aiter`; `aiter/jit/utils/chip_info.py:24-37` shells out and raises), `model_runner` being the only module here that needs a driver (already pinned by `test_only_the_binding_module_reaches_the_engine`), the package being empty at import time, and the three `COMPASS_RUNNER_QUALNAME` claims (already pinned in `test_runner_non_allocating.py`). **What was not audited:** only this one docstring. `overrides.py`'s module docstring and `RPC_SURFACE`'s comment block, `step_output.py`, and `model_runner.py` were read as evidence but not audited claim by claim — except for one thing found while reading, filed separately below rather than folded in. ## Pinning it — both directions Three tests in `tests/compass/test_runner_rpc_surface.py`, which already derives the dispatch surface from ATOM's source. Measured on node 18, branch tree: | mutation | result | |---|---| | reinstate the un-partitioned docstring (the base's `__init__.py`) | **3 failed** — all three new tests | | `"exit": False` → `True` in `RPC_SURFACE` | **5 failed** — 2 new + 3 landed | | a fourth module the docstring does not name | **1 failed** — the module test | | drop `pp_engine_core.py:369` from the prose | **1 failed** — the site test | | unmodified | 55 passed | So the tests fail when the defect is reinstated *and* when the table stops matching the prose from the other side. ## Gates `scripts/compass/gate_cpu.sh`, the branch's own copy, in `xiaobizh_n18_cpu`, staged by `git archive` + `docker cp` with both stamps, `atom.__file__` verified under each root, each rc captured separately (never piped — #191): | tree | stamp | result | `GATE_CPU_RC` | |---|---|---|---| | control `92f1fdafe` | `commit: 92f1fda (stamp)` | 4557 passed, 149 skipped, 3 xfailed | **0** | | branch `812cea70b` | `commit: 812cea7 (stamp)` | 4560 passed, 149 skipped, 3 xfailed | **0** | **Delta +3 passed, +0 skipped, +0 failed** — exactly the three new tests. Skips are identical on both sides, so the `TestTheRegionIsNotCopiedPerChunk` flake did not fire on either. `black --check` clean on both changed files; `ruff check` clean on both. ## Size Counter calibrated against landed `atom/compass/spec/` = **275 stmts / 495 code / 763 physical**, run after `ruff format`. | | stmts | code | physical | |---|---|---|---| | production (`__init__.py`, prose only) | 3 → 3 (docstring `Expr` + 2 assigns) | 2 → 2 | 30 → 56 | | test (added lines) | **23** | 19 | 73 | ## Filed separately, not folded in `atom/compass/runner/model_runner.py:52-53` — the partitioned refusal comment this correction was told to be consistent with — says an unwaited hole "for `exit` means the loop never breaks". That is false for the same reason the sentence here was: the break is a sibling of the per-runner loop. The landed test `test_the_two_names_no_caller_waits_for_and_what_replying_costs` already asserts the opposite in its own docstring. Both came from #90. It is outside this issue's file set, so it has an issue of its own rather than a hand-off: #199. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…ng it a hang (#207) (#211) Closes #207. ## What the sentence claimed, and why both halves are false at this head `atom/compass/design/02_model_runner_and_cost_backend.md` closed its return-contract section with: > **Return contracts are load-bearing across a process boundary.** `engine_core` calls > `capture_cudagraph` with `wait_out=True` and unpacks three values; a stub that returned > `None` killed the worker on an unpacking error while the parent waited forever. > **Across a process boundary a breached contract becomes a hang, not a traceback.** Verified at `92f1fdafe` before changing anything. **"a hang" is false.** A breach that raises is the loud path, not the silent one. `AsyncIOProcManager.monitor_procs` (`async_proc.py:475`) starts a monitor thread from `__init__` (`:353`) that waits on the process sentinels and calls `exit()`, which puts a `SystemExit()` on the output queue (`:370`) *before* `parent_finalizer()` (`:372`); `call_func` re-raises it (`:432-433`). `tests/compass/test_runner_rpc_surface.py::test_a_refusal_reaches_the_caller_instead_of_stranding_it` already asserts that whole chain, including the ordering. It is also false for two of the twelve names the section had just enumerated. `RPC_SURFACE` records `exit: False` and `process_kvconnector_output: False` — no caller reads either reply (`engine_core.py:260`, `:378`, `:500`), so a breach at either is not a hang under any reading. **The anecdote is false too, and in a way the correction had to fix.** The unpack is in the **parent**, at `engine_core.py:149`, not in the worker. `busy_loop` binds the reply to a single name (`out = func(*args)`, `async_proc.py:240`), so there is no unpacking in the worker for a `None` to fail at — and a `None` never reaches the parent's unpack either, because `if out is not None:` (`:243`) declines to queue it. A `None` reply therefore leaves the worker healthy and parks the parent; it does not do both halves of what the sentence described. ## Older and independent, not a fifth instance `git blame` puts the sentence at `cddda00b50`, **2026-09-19 00:13:59 +0000**, the design import. The commit the four `exit`-claim code instances trace to is `83ef2a094a` (#90), **2026-09-21 19:42:57 +0000** — 2 days 19 h 29 m later, and three calendar days by the dates each commit displays. `cddda00b50` is an ancestor of `83ef2a094a`, and #90's file set is five files under `atom/compass/runner/` and `tests/compass/`, with no `design/`. So this is an independent instance of the same shape, not propagation, and it stands alone. ## The replacement, and whether it needed a consequence The sibling history here is a sentence of this kind carrying a *different* wrong consequence at three consecutive heads, each rewrite reaching for a new one; the accepted fix was a full stop. Asked of this one: **no, it did not need a consequence.** The replacement states the partition and stops: * a reply of the wrong **shape** arrives and raises where it is unpacked, in-process; * a reply that never **arrives** queues nothing — an absent name is skipped, a `None` is declined — and `call_func`'s `outputs_queue.get()` takes no timeout either way; * that silence parks somebody **only where somebody is waiting**: ten of the twelve, and not `exit` or `process_kvconnector_output`. **The diagnosis that motivated this was half right, and is not precedent.** It was recorded as *"the old sentence's error was the universal quantifier, not the wording of the outcome"*. The quantifier half holds. The other half does not: the **outcome word was independently false**. Restrict the old sentence to the ten names that do have a waiting caller and it is *still* wrong for the branch its own first clause is about — a reply of the wrong **shape** raises at `engine_core.py:149`, in the parent, **with a traceback**, which is the precise thing "not a traceback" denies. So the stated diagnosis would have licensed a narrower edit — keep "becomes a hang, not a traceback", scope it to ten names — that is still false. The replacement does not do that, and the reason it does not is not the reason the diagnosis gave: it re-derives the paragraph **by breach mode** (wrong shape vs. never arrives) rather than by narrowing the quantifier, so the outcome word is replaced along with the scope. The right statement of the defect is *"the paragraph conflated two breach modes"*. The fix is right; the diagnosis as written did not name why. **The scope under which "ten of the twelve" is true.** The count holds for *a runner reached through `EngineCore`* — the scope `RPC_SURFACE` declares for itself in `atom/compass/runner/overrides.py` and that this paragraph does not restate. On the RapidServe path `enable_rapidserve` picks `PrefillEngineCore`/`DecodeEngineCore`, which broadcast seven further names with `wait_out=True` that the table deliberately excludes (issue #98); there the number is neither ten nor twelve. The paragraph points a reader at `overrides.py` for the table, and the scope note sits directly above that table, so the fact is one hop away — but it is not stated in the paragraph, and it is stated here. The raising case and the table of which names wait already exist at length in `overrides.py`'s reply-contract docstring. The paragraph points at that module rather than restating it, so nothing here duplicates it, and nothing here contradicts it. This turns on principle **8** (every claim carries its measurement — the paragraph now cites the site and the arity it depends on) and principle **6** (the partition refuses one story for all twelve rather than falling back on the loudest one). ## Named result — both directions, with counts The guard is in a **new** file, `tests/compass/test_design_rpc_reply_contract.py`. `tests/compass/test_runner_rpc_surface.py` is **not touched** — PR #200 and PR #208 both already edit it at the same insertion point. Nothing in the guard writes an answer down. `audit(text, surface)` parses the paragraph's own numbers out of its prose and compares them against facts derived from source: `RPC_SURFACE` for the waited/unwaited partition, an AST walk of `engine_core.py` for the unpack's line and arity, an AST walk of `async_proc.py` for the untimed queue read. Both the text and the surface are arguments, so every direction runs through the same `audit` the passing test runs. **Fires** (one side moved alone): | drift | complaints | |---|---| | the whole paragraph reverted to the version this replaces | **4** | | `dummy_execution` loses its waiting caller, prose not told | **2** | | the cited unpack line stops being the unpack line | **1** | | `flush_pp_send` dropped from the enumeration | **1** | The full revert's four: ``` how many there are: the paragraph says None, the source says 12 how many are waited on: the paragraph says None, the source says 10 which are not waited on: the paragraph says [], the source says ['exit', 'process_kvconnector_output'] the line it cites for the capture_cudagraph unpack: the paragraph says None, the source says 149 ``` **Silent** (the control a drift guard characteristically lacks): | case | complaints | |---|---| | the real document against the real surface | **0** | | `dummy_execution` re-partitioned in the document **and** in the surface together | **0** | Two further tests pin the claims that carry no number: that `busy_loop` binds the reply to a single `ast.Name` (so the worker cannot break on an unpack), that `engine_core.py` constructs the manager (so it is the parent), and that `call_func`'s `outputs_queue.get()` has no arguments. One thing the derivation found: `capture_cudagraph` is unpacked at **two** sites — `engine_core.py:149` in `EngineCore` and `:1109` in `DecodeEngineCore.__init__`, on the RapidServe path that `RPC_SURFACE` puts outside its own scope. The guard scopes the cited site to `EngineCore` and checks the **arity across every site**, so "three values" does not depend on which one the paragraph names. ## What this guard does not cover **The guard is blind to the defect it was built for.** It compares *numbers*; the paragraph's headline defect was an **un-numbered assertion**. Re-inserting the old rule — *"Across a process boundary a breached contract becomes a hang, not a traceback."* — verbatim into the corrected paragraph, leaving every number intact, is a document the guard accepts: `audit` returns **0 complaints** and all seven tests pass. Measured here, not inferred. So state the coverage plainly: **the pin covers the numbered copies — the enumeration, the twelve, the ten, the unwaited pair, the cited line, the arity — and it does not cover the sentence.** A reader should not infer from "this paragraph is now guarded" that the rule cannot come back. **Left undone**, with what would close it: * *The un-numbered rule can return.* Closing it needs a claim-shaped assertion rather than a number-shaped one — the cheapest honest form is a negative pin, asserting that the defect's own wording (the `REVERTED` literal's final sentence, or the phrase "becomes a hang, not a traceback") is **absent** from the paragraph. That is a different instrument from `audit` and is deliberately not folded into it here; a number-parsing guard is the right instrument for the numbers, and the paragraph's other un-numbered claims (single-name binding, untimed `get`, parent-side construction) are pinned separately by the two AST tests. * *`REVERTED` is the one hand-transcribed artefact* in a file whose argument is that it transcribes nothing. It is byte-identical to `git show 92f1fda:atom/compass/design/02_model_runner_and_cost_backend.md` today — checked, not assumed — and nothing holds it there. The failure mode is benign in direction: transcription drift weakens the *firing* case (it would reinstate a paraphrase instead of the defect) and cannot fake a pass of the silent one, because the reverted-document assertion is only that `audit` is non-empty. Cheapest close: assert `REVERTED not in DOCUMENT`, which also catches a future edit that makes the two converge. * *`stated()` reads `waiters` and `total` positionally* — `counted[0]` and `counted[1]` from the order the spelled-out numbers appear in the wait sentence. Correct at this head ("ten of the twelve names above ..." → `waiters: 10, total: 12`). A meaning-preserving reword that reversed the order — "of the twelve names above, ten have a caller that waits" — swaps them: measured here, `stated` then reports `waiters=12, total=10` and `audit` emits **two complaints that both misdescribe what changed**. The `NUMBER` map also holds `one`, so the ordinary English word "one" appearing inside that sentence shifts both indices. The failure is a *wrong* complaint, not a missing one. Closing it needs the two numbers keyed to their roles rather than to their order. ## Gates **Gate 1 — ATOM's suite unmodified, as a delta against a control measured here.** `scripts/compass/gate_cpu.sh` from each tree's own copy, staged by `git archive` + `docker cp` into `xiaobizh_n18_cpu` with both stamps written, tarball md5 verified on both ends, `atom.__file__` asserted under each root before any count was read. No file under `scripts/compass/` is committed. | | stamp | result | `GATE_CPU_RC` | |---|---|---|---| | control | `92f1fdafe` | 4557 passed, 149 skipped, 3 xfailed | 0 | | branch | `74b1d6d02` | **4564** passed, 149 skipped, 3 xfailed | 0 | Delta **+7 passed, 0 failed, skips identical** — exactly the seven tests this PR adds. The control reproduces the 4557 this branch point has reproduced on every independent run. The CPU tier's three-way flake (`test_stream_marker_properties.py::TestTheRegionIsNotCopiedPerChunk`) did not appear on either side. **Gate 2 — new CPU-only tests**, seven, in `tests/compass/`. No GPU, no import of `model_runner.py`; the derivation is an AST walk. **Gate 3 — the named result**, both directions, counted above. **Gate 4** — reviewer agent. **Lint.** This tree's formatter is **black** — `.github/workflows/pre-checks.yaml` runs `psf/black@stable`, and its ruff job runs `ruff check`, not `ruff format`. | command, on `tests/compass/ atom/compass/` | `92f1fdafe` | `74b1d6d02` | |---|---|---| | `black --check` | 43 files unchanged, rc=0 | **44** files unchanged, rc=0 | | `ruff check` | `All checks passed!`, rc=0 | `All checks passed!`, rc=0 | The +1 is this PR's new file. Both re-measured at both refs from `git archive` stagings, black 26.5.1, ruff 0.16.7. An earlier revision of this description claimed `ruff format --check` reported the new file already formatted. **That claim was false and has been withdrawn**: `ruff format --check tests/compass/test_design_rpc_reply_contract.py` prints `1 file would be reformatted`, rc=1, disagreeing with black at three multi-line `assert` statements (`:108-110`, `:191-193`, `:201-203`) over where the failure message goes. That is the known black-vs-ruff-format split, not a defect in the file: `ruff format --check` already wants **7** landed files reformatted at `92f1fdafe` and **8** here, the difference being this one. `ruff format` is not run by this tree and its verdict gates nothing; the sentence was an unverified claim written down as fact, which is exactly what principle **8** — the subject of this PR — forbids. ## Effort Counted with `ast.stmt` nodes including docstring `Expr` nodes, after formatting, calibrated against landed `atom/compass/spec/` = **275 / 495 / 763** statements/code/physical, which the counter reproduces exactly. | | statements | code | physical | |---|---|---|---| | production | **0** | 0 | 13 changed lines | | test | **70** | 148 | 248 | Production is **0 not because nothing changed but because the production change is prose**: the file set's non-test half is a Markdown design document, which carries no AST. The 13 changed lines replace a four-line paragraph with an eleven-line one. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…he docstring (#393) tests/compass/test_runner_rpc_surface.py said the comment, the package docstring and the string assertion "were added together". #80 (1b473e5) wrote the package docstring. #90 (83ef2a0) amended it by one sentence (+4/-1), about overrides.RPC_SURFACE and the caller that waits forever, and added the refusal comment and the exact-string assertion. The sentence now says "the package docstring's sentence about the surface". The AST is identical with docstrings masked. Gate (node 18, CPU tier, combined with #392 on f89b149): 5275 passed, 155 skipped, 3 xfailed, GATE_CPU_RC=0. Closes #384 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 participant
Closes nothing yet — draft. Task record: issue #77. No longer stacked on
anything: RUNNER-1 (#80) landed as
1b473e5af, and this PR is now baseddirectly on
feature/atomcompass_newat that commit. Head4ce635c8b.RUNNER-3 (#97) stacks on this branch.
Round 2 (
4ce635c8b): three of the twelve stated shapes were wrong orshort and are corrected —
forward,stop_profiler,get_num_blocks; theimport-time refusal now partitions on
RPC_SURFACEinstead of claiming everyhole parks a caller; two derivation gaps in the tests are closed with seeded
violations; a
Nonereply is named as its own park rather than folded intoabsence; and a hole the intersection hides is filed as #98. Details in the
round-2 comment on this PR and in the nine inline threads.
The sentence this was designed around
Reading
async_proc.pyturned that into a two-sided rule with a third case thebrief did not name, and all three are now pinned by tests:
getattr(runner, name, None)returns None,busy_loopcontinues, the worker stays healthy — and await_out=Truecaller blocks onself.outputs_queue.get(), which takes no timeout. Unbounded wait, no error anywhere.Noneasync_proc.py:243isif out is not None:, and both of the loop'sput_nowaitcalls (:248,:250) are inside it — one line below thegetattrskip at:237-239. So this is one line further down the same path, not a second mechanism. Indistinguishable from the row above, from the caller's side — which is why naming only absence is worse than saying nothing: it sends whoever is debugging the hang to check whether the method is there, find that it is, and stop.busy_loop, the worker process dies,AsyncIOProcManager.monitor_procsnotices the sentinel and callsexit(), which puts aSystemExiton the output queue, andcall_funcre-raises it in the caller. Loud, and recoverable.So on this surface a refusal is the safe failure and a silence is the fatal
one. That inverted the design: the two methods that refuse (
get_num_blocks,forward) are fine as they stand, and the work was to make sure nothing on thesurface can go quiet.
What the third row delivers, measured rather than read (reviewer, node 18,
container
xiaobizh_n18, a realAsyncIOProcManager+AsyncIOProcover theaiter shared-memory ring,
proc_num=1, watchdog at 90 s):get_num_blocksraisesRunnerRefusalSystemExit()raised incall_funcforwardraises on an already-warm workerSystemExit()raisedget_num_blocksget_num_blocksreturnsNoneSystemExit()raisedTwo properties the delivery does not have, and the docstring now says so:
the parent receives a bare
SystemExit()with empty args — no message, no__cause__— so the refusal's name and reason exist only on the worker'sstderr; and
SystemExit()carriescode=None, so letting it propagate out ofengine_core.py:132(atry/finallywith noexcept) exits the parent withstatus 0, a clean shutdown to any supervisor reading exit codes. Raising is
still the right half of the fork, but a successor that wants its refusal
diagnosable in the engine's own log has to log it worker-side first.
ATOM's own code already states half of the rule —
ModelRunner.freeze_gc_heap'sdocstring says "The count is returned because
busy_loopreplies onlyif out is not None— an RPC target returning None hangs itswait_out=Truecaller."Named result: the twelve, with what each call site actually fixes
A cited site does not constrain the same thing in every row, and conflating the
two is the defect round 1 shipped. Where the caller unpacks, subscripts or
reads the reply the site fixes the shape; where it forwards or discards
it, the site fixes only non-
Noneand picklable, and any richer shape is aconvention inherited from the base implementation.
get_num_blocksengine_core.py:132-141—block_info["num_kvcache_blocks"](:133),.get("pool_entries", {})(:139),.get("pool_entries_per_req", {})(:140),StateRuntime.from_wire(block_info["state_runtime"])(:141)state_runtimeis itself a two-key wire dict —from_wireraisesTypeErrorunless it is aMappingandValueErrorunless its keys are exactly{"transfer", "checkpoint_spec"}. The block count must be> 0(block_manager.py:78).allocate_kv_cacheengine_core.py:142-145—assert retcapture_cudagraphengine_core.py:149-151,:1109-1111—cap_cost, bs, pool_bytes = …forwardengine_core.py:386,:1264;pp_engine_core.py:118(waits and discards),:379. Reads atscheduler.py:2435-2542andpp_engine_core.py:144dummy_executionengine_core.py:749—return self.runner_mgr.call_func(...), returned to its own callerNoneexitengine_core.py:260— reply never readfreeze_gc_heapengine_core.py:203-206— value discarded inside atry/except ExceptionNone. The one waited name whose raise is caught; itsNonestill hangs.process_kvconnector_outputengine_core.py:378,:500;pp_engine_core.py:113,:232,:369None, or the reply sits on the primary queue for whoever asks nextasync_proc_aggregationengine_core.py:488,pp_engine_core.py:252,:406viacall_func_with_aggregationKVConnectorOutput, one per workerstart_profilerengine_utility.py:252— forwarded as("UTILITY_RESPONSE", {"result": result}), unreadNone, picklablestop_profilerengine_utility.py:264— logged whole, then forwarded, unreadNone, picklable.{trace_dir, elapsed}is a convention fromModelRunner.stop_profiler's own docstring, not a caller's requirement —trace_dirappears nowhere in the tree but the three lines that produce it, andllm_engine.py:300's.get("result", {})is on the envelope, not the reply.flush_pp_sendpp_engine_core.py:81,:125,:362,:395— value discarded, the wait is the pointNoneforward's nine, the line that fixes each, since this is what RUNNER-3builds against:
.req_idspp_engine_core.py:144.token_idsscheduler.py:2435.draft_token_idsscheduler.py:2436.is_deferred_outscheduler.py:2437.logprobsscheduler.py:2438Noneordict[int, float].get_idx(req_id)scheduler.py:2454None.num_rejected[idx]scheduler.py:2521int()-able.num_bonus[idx]scheduler.py:2522int()-able.dspark_ellscheduler.py:2541-2542None, or a mapping answering.get(seq.id)Plus picklable: under PP the last stage sends it whole (
pp_engine_core.py:385)and the head reads it back (
:139), each hop throughpickleatatom/distributed/pp_transport.py:141and:114. The.req_idsread is at:144-147, in_pp_head_step, on whatrecv_tokens()returned at:139—not at the
:379broadcast, which is inside_downstream_busy_loop, thelast stage, and reads nothing off the reply. Round 1 cited the wrong loop; a
test now pins the enclosing function so it cannot recur.
async_proc_aggregationis the one whose breach is not an unbounded hang —the aggregating form reads each worker's queue with
timeout=10.0and returnsNoneon expiry. On that bounded path a missed put does not merely cost 10 s:it permanently offsets that worker's KV queue, so every later poll reads a
stale-by-one output.
How the twelve were enumerated — not from a typed list
busy_loophas no dispatch table: it dispatches whatever name arrives on theshared-memory ring. So the table is the set of names ATOM puts on that ring.
tests/compass/test_runner_rpc_surface.py::_call_siteswalks every.pyunderatom/, collects eachcall_func/call_func_with_aggregationwith a literalfirst argument, and records the file, line,
wait_out, whether it is theaggregating form, and the unpack arity taken from the parent node —
ast.Expris a discard,ast.Assigngives the unpack width, andast.Returnor an argument position means the value is used whole.
That yields 39 call sites (engine_core 19, pp_engine_core 11, engine_utility
9) and 26 distinct names. Intersecting with the methods
ModelRunnerdefinesgives exactly the twelve — the test asserts the equality and the count.
Both filters in that walk now report what they dropped, and a test asserts
both lists are empty: broadcasts from
atom/compassand broadcasts whose firstargument is not a literal. Both are empty at this commit, so an f-string call
site or a compass-side broadcast fails the suite rather than vanishing from the
derivation.
The other 14 are accounted for by a second test so the intersection cannot
silently shrink: 7 belong to
RapidServeModelRunneronly (prefill_forward,create_prefill_stream_pool,create_decode_stream_pool, and the four IPCimport/export names), and 7 belong to the RLHF rollout extension —
update_weights,update_weights_from_ipc,update_weights_from_shmonWeightUpdaterMixin;release_memory,resume_memory,clear_kv_cacheonMemoryManagerMixin; andconfigure_hidden_statesonRLHFModelRunneritself (
atom/rollout/model_runner_ext.py:198), not on a mixin. That test nowasserts the 7/7 split and intersects the extension names against those three
files, so a dispatched name that no class in the tree answers fails rather than
passing as a residue.
A hole the intersection hides — filed as #98.
config.py:1729-1736substitutes
RapidServeModelRunneronly whilerunner_qualnameis still ATOM'sdefault; Compass overwrites it. But
enable_rapidserveindependently selectsPrefillEngineCore/DecodeEngineCore(llm_engine.py:140), and thosebroadcast the RapidServe seven — all
wait_out=True— at whatever the qualnamenamed. So
enable_rapidserve=Truewith this runner is seven silent parks onnames this table deliberately excludes. The Group-A exclusion is sound only
because the EngineCore class, not the runner class, gates those broadcasts.
The table now records that its scope is "a runner reached through
EngineCore",because nothing else in the tree says so. Not closed here: the condition holds
for any deployment that sets both, so it belongs in
Config, which is what #98asks for.
One surface a literal-only parse structurally cannot see, also recorded
beside the table: three of the twelve are called in-process on the runner
itself, reached over the
resume_memoryRPC —memory_manager.py:176subscripts one key of
get_num_blocks,:183discardsallocate_kv_cache, and:209callscapture_cudagraphunremarked inside atrythat degrades toenforce_eager=True. Different arities, and the one place in the tree where arefusal from this module is caught rather than fatal.
What was done about the
getattrsilenceRPC_SURFACEinatom/compass/runner/overrides.pynames all twelve withwhether the caller waits, and the call site beside each. It is data the tests
check against ATOM's source, not documentation.
unanswered_rpc_names(runner)returns the namesgetattrwould answerwith
None.atom/compass/runner/model_runner.pyraises at import time if thecomposed
CompassModelRunnerleaves any of them unanswered — and the messagepartitions the missing names on
RPC_SURFACE: waited names park theircaller for the life of the process, unwaited ones do not park anyone at all
(
busy_loopskips the name, so a missingexitmeans the loop never breaksand a missing
process_kvconnector_outputmeans a KV load is silently neverstarted). That is
RPC_SURFACE's boolean's first reader outside the tests.a rename upstream moves the derived set and fails.
The reviewer executed (3) against the real composed class on a GPU container,
which the CPU tier cannot: the import succeeds,
unanswered_rpc_names(CompassModelRunner) == (),and all twelve resolve — four from
NonAllocatingRunner, eight fromModelRunner. The test for it is two substring greps plus an assertion that thepartition is present; its docstring now says outright that no CPU tier can
execute the device, so a green gate is not read as covering it.
Worth knowing for anyone reading a worker log when it does fire:
AsyncIOProc.__init__resolves the runner class atasync_proc.py:166beforeassigning
self.runners = []at:167, so the atexit finalizer runs on ahalf-built object and the log ends with
AttributeError: 'AsyncIOProc' object has no attribute 'runners'— printedafter the real refusal. ATOM's bug, not this diff's, but the last traceback
in a log is the one people read. Recorded beside the raise.
What actually changed, and why only one method needed a new body
The composed class is
CompassModelRunner(NonAllocatingRunner, ModelRunner), sogetattralready finds ATOM's implementation for anything the mixin does notdefine. The question for each of the twelve was therefore not "is it present"
but "does ATOM's own body work for a runner with no weights and no device".
One does not:
capture_cudagraph. It zeroesforward_vars["kv_indptr"].gpu,opens a CUDA graph memory pool and enters
graph_capture()before it evertouches the model — so inheriting it would put bytes on a device before
discovering there is nothing to trace, and only then fail. It is replaced with a
body that captures nothing and says so in the three values the caller unpacks:
(0.0, [], 0). It deliberately does not touchcapture_sizes/capture_sizes_np, which the base sets to the eager-fallback[0]duringconstruction and the attention metadata builder reads every step. Two things
the round-2 docstring adds: the reply deliberately disagrees with the
attribute it preserves (the base sends
self.capture_sizes, i.e.[0], atmodel_runner.py:4085; this sends[], and both are only ever logged), andengine_core.py:148reaches this method only whennot enforce_eager and not disagg_is_decode, so on an eager deployment the override never runs — replacedanyway, because a runner must not depend on a caller-side flag to stay off the
device.
The reviewer checked every reader of the surviving
[0]and confirmed no sitereads it as evidence of a capture:
forward_context.py:225,:242,model_runner.py:598-600, and the empty_piecewise_*lists at:695-696.The rest keep ATOM's implementations, and that is a decision, not an
omission. Replacing them would have been worse in three concrete ways:
process_kvconnector_outputandasync_proc_aggregationare how a connectorregistered in ATOM's factory is driven at all. A no-op stub would cut the
simulated KV connector out of the one path it exists on. ATOM's
process_kvconnector_outputhas noreturnstatement at all, so it isNoneby construction and there is no stale-reply hazard.freeze_gc_heaphas a real CPU effect and returns an int on every path,including with
ATOM_GC_FREEZEoff.dummy_executionbuildsSequence/ScheduledBatchon CPU only and reachesself.forwardbefore anything device-side — the same discriminator thatcondemned
capture_cudagraph, applied and answered the other way. It alreadyrefuses through this runner's own
forward, and it will start working themoment RUNNER-3 lands one.
exit,start_profiler,stop_profilerandflush_pp_sendallocate nothingand read only state the base sets in
__init__(_pp_pending_sendatmodel_runner.py:715).exitreadsself.model, which exists only becauseRUNNER-1's
_build_and_load_modelsets it — a dependency on #80 worth naming.The line drawn, stated once: a value is refused when a consumer will treat
it as a measurement of the simulated system (
get_num_blocks' block count goesto the BlockManager;
forward's output goes topostprocess). It is answeredwhen it only tells the caller the RPC completed, or describes what this runner
actually did.
The oddity worth recording:
exitreplies although nobody waitsOf the twelve, two are dispatched without
wait_out.process_kvconnector_outputcorrectly returns nothing.
exitreturnsTrue, whichbusy_loopputs on theprimary output queue with no reader — normally the setup for the next waiting
caller reading a stale reply. It is harmless only because
busy_loopbreaks outof the loop on that name (the put at
async_proc.py:248does precede the breakat
:251-252), so there is no next caller. Pinned by a test.Gates
CPU tier, node 18, container
xiaobizh_n18_cpu. Both trees staged asgit archivesnapshots viascripts/compass/snapshot.sh(which writes the.compass-commit/.compass-changedstamps, without whichgate_cpu.shexits98), md5-verified host → node → container, extracted under
/tmp/xiaobizh-r2r2/,and run from each tree's own copy of
gate_cpu.sh, sequentially./tmp/xiaobizh-compass/ATOMwas not touched, and the staging area was removedafterwards.
Integration head read 2026-09-21 19:18:21 UTC (GitHub's own
Dateheader)as
1b473e5af, which is RUNNER-1 (#80) squashed onto a tree that also nowcarries #82, #83 and #88. It moved three times during this round:
669dc3f9dat the round-1 review,
354965883after #83/#88,1b473e5afafter #80.1b473e5af4ce635c8bDelta +52, which is this task's own tests only:
test_runner_rpc_surface.pycollects 52, and 4518 + 52 = 4570. RUNNER-1'stest_runner_non_allocating.py(17) is now in the control rather than in thedelta, which is what landing #80 was expected to do. Round 1 was +65 = 48 + 17
against a control that predated #80; at
354965883, before #80 landed, thesame branch measured +69 = 52 + 17.
gpu: not required (.compass-changed stamp)printed on both — all five changedpaths checked against
gpu_gate_triggers.txt, no match — andcommit:namedthe right sha on each, so neither hit the exit-98 path. No flake in either run:
tests/entrypoints/test_stream_marker_properties.py::TestTheRegionIsNotCopiedPerChunkpassed in both and the skip counts are identical at 149/149. Four gate runs
across this round, all
GATE_CPU_RC=0printed exactly once.ruff checkandruff format --checkclean on all five touched files — a package-widetests/compass/format glob reports three other files, all froma8ebf8c47,which predates this branch's base.
Effort — three instruments
Estimate 150 lines, a single number. Measured against the integration head
1b473e5af; docstrings excluded from AST; SLOC = non-blank, non-comment,non-docstring.
atom/compass/runner/)tests/compass/)Against the estimate: AST 1.66x, SLOC 3.07x, physical 6.36x. (Round
1, against
4704d27ec: 183 / 313 / +576 → 1.22x / 2.09x / 3.84x. Theinstrument reproduces the reviewer's round-1 figures exactly — 9 / 31 / +117
production and 174 / 282 / +459 tests — which is the only reason to trust this
row.)
For #89, publish SLOC as the rule with AST beside it. AST charges one
statement for a twelve-row table that a reviewer has to check row by row against
ATOM's source, so 1.66x understates exactly what this task was. Physical charges
+954 for a change that is 460 lines of code and the rest prose and blanks, which
makes a documented diff read as an overrun and quietly penalises writing the
reasoning down. The two disagree by 1.85x here and 1.71x in round 1 — and
that disagreement is the data/prose-heavy signal, which is the thing a single
number cannot say.
Provenance of the four-key
get_num_blockscontractCorrected, because it would otherwise be copied. The four keys came from
042e8f1c0(ROCm#1771, 2026-08-11).c2d40e2dd(ROCm#1894, 2026-08-15) addedstate_runtimeand rewrotereturn {"num_kvcache_blocks": 0}into thecurrent two-key RapidServe form — so that form postdates the four-key
contract and is a deliberate short form, not a stale predecessor. M1-1
(
14a197b07, #74) touches zero lines underatom/model_engine/and did notgrow the contract, contrary to what this PR was told mid-build.
The short form is not a breach — the caller defaults both pool-entry keys — but
the default is not semantically neutral:
block_manager.py:116-121turns themissing
pool_entriesintonum_state_slots = 0, a decision input at:162(
enabled = enable_prefix_caching and num_state_slots > 0) and in thepermanent-unschedulable predicate at
scheduler.py:1364-1376. For a statelessmodel 0 is correct; for a per-request-state model it is a silent "no slots ever
existed" rather than a crash.
Handoff to RUNNER-3 (#97)
forwardstill refuses. Its contract is nine attributes and a pickle, notone object read by attribute — the table above, all nine with the line that
fixes each, and a test that recovers the nine from the consumers by AST rather
than by substring, so a tenth read or a rename fails.
.req_idsread ispp_engine_core.py:144-147, in_pp_head_step, on whatrecv_tokens()returned at:139— not at the:379broadcast.SystemExit(), message-less, andexits the parent with status 0. If RUNNER-3 wants a refusal diagnosable in the
engine's log it has to put it there itself, worker-side.
dummy_executionneeds no work: it callsself.forward, so it answers as soonas
forwarddoes, and RUNNER-3 gets a second caller free without a second test.RPC_SURFACEsaysforwardis waited on, so returningNonefrom it is thehang, not a test failure — and it is the same hang as not defining it, one
line further down
busy_loop. Bothput_nowaitcalls sit insideif out is not None:(async_proc.py:243, puts at:248and:250), whicha test now asserts structurally rather than as a substring.
tests/compass/test_runner_rpc_surface.pygives arity and attribute reads forany shape RUNNER-3 needs.
_aritywas wrong in round 1 — it read onlyast.Assignparents, soreturn call_func(...)scored 0, "discarded". Fixedand asserted on
dummy_execution, which is the one site in the tree with thatshape.
Not done / not in scope
get_num_blocksstill refuses — the block count belongs to the memory model.Its four-key contract, including the nested two-key
state_runtimewire dict,is in the docstring and the tests for whoever supplies it.
Config,because it holds for any deployment that sets
enable_rapidservewith anon-default
runner_qualname, not only for this runner.test_runner_non_allocating.py'sOVERRIDDENset gainscapture_cudagraph, and the RapidServe-difference testnow asserts the difference is three methods rather than two.
async_proc_aggregationagainst a real registered connector, which does not exist in the tree; the GPU
tier, not triggered; and
CompassModelRunnerinside a realEngineCore,which is compass(runner): the three step semantics, driven through ATOM's scheduler (RUNNER-3) #97's.
🤖 Generated with Claude Code