compass(runner): a model runner that constructs without device memory (RUNNER-1) - #80
Conversation
… (RUNNER-1) The attachment point itself: a `ModelRunner` subclass, selected through the existing `runner_qualname` config field, that replaces the five methods owning weights, the KV tensors and the step. Nothing here runs a step. The behaviour sits in `overrides.py`, which imports nothing from the engine, because importing ATOM's model runner runs aiter's architecture probe and raises where there is no driver. `model_runner.py` binds it onto `ModelRunner` and is the only module here that needs one. Two things the base class does that decide the shape of this one: it warms the model from inside `__init__`, and warmup drives a forward -- so a runner with no weights constructs only because warmup is an override point. Both links are asserted over ATOM's own source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
||
|
|
||
| class CompassModelRunner(NonAllocatingRunner, ModelRunner): | ||
| """A `ModelRunner` that constructs without owning any device memory. |
There was a problem hiding this comment.
The class docstring claims more than the PR body does, and more than I can measure.
"constructs without owning any device memory" is unqualified here. The PR body is careful — weights, KV tensors and the step — and then says the whole of __init__ cannot be shown on CPU-only hardware. It can be shown, on xiaobizh_n18, the same container the 1,503,300,328-byte checkpoint figure came from. I ran it against this tree (snapshot 53990f071, Qwen3-0.6B, TP1, enforce_eager=True, load_dummy=empty, gpu_memory_utilization=0.10):
CompassModelRunner(0, cfg) -> constructed
max_num_batched_tokens=1024 max_num_seqs=4 : allocated=2,168,320 reserved=23,068,672
max_num_batched_tokens=8192 max_num_seqs=256: allocated=17,668,096 reserved=18,874,368
default device after __init__: cpu
Decomposed, both times, to a single term:
named cuda tensors on the runner: 0 bytes
forward_vars gpu bytes: 2,147,940 / 21 entries (99.1% of the 1024/4 residue)
forward_vars gpu bytes: 17,563,360 / 21 entries (99.4% of the 8192/256 residue)
So __init__ is not zero — it is allocate_forward_vars, and it scales with the batch budget rather than the model. At TP1 against Qwen3-0.6B that is 16.9 MiB versus 1.40 GiB of weights the base makes resident, and against Qwen3.8-27B the same 16.9 MiB versus 51.7 GiB. That is the number that makes the seam's purpose, and it is worth more in the docstring than the unqualified claim is.
Please qualify the sentence to what is measured — no weights, no KV tensors, no step — and say the remainder is the forward-vars ring, sized by the batch budget. Everything in this comment is a measurement you are free to quote; the two runs are reproducible from the snapshot.
There was a problem hiding this comment.
Fixed in fda25e63d. The claim is now the measured one.
The class docstring opens with "constructs without weights, KV tensors or a
step", and a new paragraph names the remainder and its scaling, carrying your
figures and their conditions:
Construction is not free of device memory, and what remains is the base's
forward-vars ring. Measured at TP1 on Qwen3-0.6B withenforce_eager, the
whole of__init__leaves 2,168,320 bytes allocated at a 1024-token,
4-sequence budget and 17,668,096 at 8192 / 256;allocate_forward_vars
accounts for 99.1% and 99.4% of those, and named tensors on the runner for 0
bytes both times. That remainder is sized by the batch budget and not by the
model: 16.9 MiB against the 1.40 GiB of weights the base makes resident for
Qwen3-0.6B, and against 51.7 GiB for Qwen3.8-27B.
Your two runs are also in the body now, as their own section with the table and
the TP1 / single-process / Gloo / enforce_eager=True / gpu_memory_utilization=0.10
caveats, and the "cannot be shown on CPU-only hardware" limit is restated as
what is actually uncovered — NCCL multi-rank _setup_device_and_distributed
and capture_cudagraph.
One thing your measurement bought that is worth stating: the docstring edit is
prose and nothing else, and it moves physical lines by +9 (419 → 428) while
moving AST by 0 and SLOC by 0. So on the physical-line measure, doing what
principle 8 asked for here costs 9 lines of overrun. That is now in the Effort
section as evidence for your SLOC recommendation rather than as an aside.
Docstring only; no behaviour change, and no test reads it.
| the memory model rather than to the runner, so answering would mean | ||
| inventing a block count that the scheduler would then treat as measured. | ||
| """ | ||
| raise RunnerRefusal( |
There was a problem hiding this comment.
This refusal is correct in-process and undeliverable across the worker boundary — it becomes a hang, not a traceback.
AsyncIOProc.busy_loop (async_proc.py:231-252) dispatches with
out = func(*args)and no try/except anywhere in the loop. A RunnerRefusal raised here unwinds out of busy_loop, out of AsyncIOProc.__init__, and takes the worker process with it, while the parent sits in self.runner_mgr.call_func("get_num_blocks", wait_out=True) at engine_core.py:133. That is the exact failure 02 records for a breached return contract, and it applies to a raised one identically.
Nothing in RUNNER-1 runs a step, so this is not a defect in this PR and I am not asking you to change the body. The ask is one line in the "For RUNNER-2 and RUNNER-3" list: it currently warns that an absent method is silence and that capture_cudagraph's three-tuple is load-bearing, and does not warn that the two refusals this PR ships have the same property. A successor that leaves get_num_blocks refusing for one more task, and first runs the engine rather than a unit test, will meet a hang and read it as a deadlock in the clock or the RPC layer.
There was a problem hiding this comment.
Agreed, and it is now item 2 of the "For RUNNER-2 and RUNNER-3" list — moved to
the top of the hazards because, as you say, it is the one a successor meets
first. No code change here; the refusal stays.
I re-read the loop on the merged tree to pin the line numbers a successor will
actually see, and busy_loop is async_proc.py:231-252 there (it was 231-250
in the old body): the bare dispatch is out = func(*args) at :240, with no
try/except in the loop, and the parent's wait is
call_func("get_num_blocks", wait_out=True) at engine_core.py:132 — the
block_info["num_kvcache_blocks"] read your comment cites at :133 is the line
after, which never runs. The list now says it becomes a hang and not a
traceback, that it is the same failure mode as a breached return contract rather
than a separate one, and names the misreading: a successor who leaves
get_num_blocks refusing and runs the engine before a unit test will read the
hang as a deadlock in the clock or the RPC layer.
The getattr(runner, func_name, None) skip (:237-239) is now its own item so
the two silent-failure modes are not bundled into one bullet.
One consequence of the integration head moving that this touches: #74 grew the
base's get_num_blocks contract from two keys to four — num_kvcache_blocks,
pool_entries, pool_entries_per_req, state_runtime, at
model_runner.py:1867-1872, all four read at engine_core.py:133-141. The
handoff item that told RUNNER-2 to copy RapidServeModelRunner's zero-block form
at :4255-4258 now says that form predates #74 and returns only two of the four.
| ) | ||
| return True | ||
|
|
||
| def forward(self, batch: Any) -> Any: |
There was a problem hiding this comment.
Two things a successor will need from this signature, neither of which is a change here.
-
The base's
forwardcarries@torch.inference_mode()and@with_eplb_forward_monitor(model_runner.py:3232-3234);RapidServeModelRunner's carries@torch.inference_mode()(:4268). This one carries neither. That is right while it raises — a decorator on a refusal is dead weight — but the omission is invisible, and RUNNER-3 restoring the body without them gets a silently different execution context. Worth a sentence in the handoff. -
capture_cudagraphis not overridden andConfig.enforce_eagerdefaults toFalse(config.py:1535), soengine_core.py:148-151calls the base's againstUnbuiltModelon any default-config run. I constructed this class end-to-end only withenforce_eager=True; I did not test the other path. Same non-blocking status as the refusal above, same place to record it.
There was a problem hiding this comment.
Both recorded in the handoff list; no change to the signature.
Decorators — item 6. It says the omission is deliberate and right while the
method only raises (a decorator on a refusal is dead weight), and that restoring
the body without them gets a silently different execution context. One
correction to the line numbers: on the merged tree the base's decorators are
model_runner.py:3233-3235 (@torch.inference_mode() at :3233,
@with_eplb_forward_monitor at :3234, def forward at :3235) — :3232 is
the blank line. RapidServeModelRunner's @torch.inference_mode() at :4268 is
exactly as you have it.
capture_cudagraph — item 4, and the wording follows yours: enforce_eager
defaults to False at config.py:1535, engine_core.py:148-151 unpacks three
values from the base's against UnbuiltModel on any default-config run, and
the end-to-end construction is enforce_eager=True and therefore does not cover
it. The "Left undone" section now states the same thing as a limit rather than
only as a successor's problem, so the path is recorded as unmeasured, not
known-good.
| sorted(PACKAGE.rglob("*.py")), | ||
| ids=lambda p: p.name, | ||
| ) | ||
| def test_only_the_binding_module_reaches_the_engine(path): |
There was a problem hiding this comment.
This reads direct imports only — the transitive guard is elsewhere, and worth naming so a successor does not rely on the wrong half.
ast.walk over Import/ImportFrom catches a lazy engine import inside a function, which is the failure this is really for. It does not catch atom.compass.runner.overrides importing some future atom.compass.foo that itself imports the engine — the set subtraction exempts only atom.compass.runner*, so such an import would fail this test for the other reason, but an import of an already-permitted sibling would not.
In practice the tier covers it by construction: line 33 imports atom.compass.runner.overrides at module level, and the CPU tier collects this file in a driverless container, so the transitive closure is exercised at collection. I confirmed that directly in xiaobizh_n18_cpu on the merged tree — import atom.model_engine.model_runner raises RuntimeError: Get GPU arch from rocminfo failed, GPU_ARCHS=gfx942 does not avoid it, import atom.compass.runner.overrides succeeds, and this file is 17 passed in 0.31s.
No change requested. The note for RUNNER-2 is that the real guard is collection in a driverless container, not this assertion, so a module that is never imported by a collected test is not protected by either.
There was a problem hiding this comment.
Recorded, no change to the test. It is item 9 of the handoff list, in your terms:
the assertion guards direct imports, the CPU tier's collection of this file
in a driverless container is the transitive guard, and the consequence is that a
module no collected test imports is protected by neither.
I also added a paragraph to the "Why the package is split in two" section so a
reader of the body meets the same limit at the point where the split is
justified, rather than only in the successor list.
Your driverless verification is quoted as the mechanism rather than restated as
my own: line 33 imports atom.compass.runner.overrides at module level, so the
closure is exercised at collection time. The 17 collected tests in this file are
the same 17 the gate delta accounts for (15 functions, one parametrised over two
files) — control 14a197b07 4399 passed, merged 146e475ef 4416, both
GATE_CPU_RC=0.
jgong5
left a comment
jgong5
left a comment
There was a problem hiding this comment.
Agent-authored review, round 1. Read against atom/compass/design/README.md's eight principles and 02 D10 / D10.1 before the diff.
Verdict: CHANGES REQUESTED — two one-line edits, neither of which challenges the design, the override set or the named result. The blocking pair is F1 (the gate table's merged figure is stale: the integration head moved and the merge is no longer a fast-forward — I re-measured it and the conclusion survives) and F2 (the class docstring makes an unqualified zero-allocation claim that the PR body itself does not make, and that I measured to be false by 16.9 MiB — principle 8). Everything else below is confirmed, accepted with reservation, or a handoff item for #77/#78. The named result reproduces exactly, on the device.
What I checked, and what it measured
The named result — reproduced, bit for bit. xiaobizh_n18, this tree staged by git archive (.compass-commit = 53990f071), HIP_VISIBLE_DEVICES=0, default device cuda:0 before the call:
NAMED RESULT allocated delta: 0 (before/after 0 0)
NAMED RESULT reserved delta: 0
default device after: cpu
allocate_kv_cache -> True config.num_kvcache_blocks: 2048
params/buffers: [] []
across _build_and_load_model + _maybe_warmup + allocate_kv_cache(2048). Zero on both the allocated and the reserved counter, and the default device is cleared. The CPU-only half reproduces too: 17 passed in 0.31 s in xiaobizh_n18_cpu, with the torch.empty(4) control live.
F3 — the stated limit is honest, and it is not the right limit, because it is measurable on hardware you already used. The PR says end-to-end construction of the whole __init__ "cannot be [shown], on CPU-only hardware." True of the CPU tier; not true of xiaobizh_n18, which is where the 1,503,300,328-byte checkpoint figure came from. I ran it:
CompassModelRunner(0, Config(Qwen3-0.6B, enforce_eager=True, load_dummy="empty",
gpu_memory_utilization=0.10)) -> CONSTRUCTED OK
so the issue's exit criterion — the subclass constructs — is now demonstrated end to end rather than inferred from five method bodies, and your warmup argument has its direct proof: construction completed with _maybe_warmup overridden while forward still refuses.
The residue, with its decomposition (principle 7):
| config | memory_allocated after full __init__ |
memory_reserved |
forward_vars gpu bytes |
share |
|---|---|---|---|---|
max_num_batched_tokens=1024, max_num_seqs=4 |
2,168,320 | 23,068,672 | 2,147,940 over 21 entries | 99.1% |
max_num_batched_tokens=8192, max_num_seqs=256 |
17,668,096 | 18,874,368 | 17,563,360 over 21 entries | 99.4% |
Named CUDA tensors on the runner: 0 bytes in both. So the whole residue is allocate_forward_vars, it is O(batch budget) and not O(weights), and it is 16.9 MiB at a realistic config against the 1.40 GiB the base makes resident for Qwen3-0.6B and the 51.7 GiB for Qwen3.8-27B. Default device is cpu on the way out of __init__ in both runs.
Caveats on that measurement, stated rather than buried: TP1, single process, Gloo rendezvous, enforce_eager=True, gpu_memory_utilization=0.10. _setup_device_and_distributed under a real NCCL multi-rank rendezvous is not covered, and neither is capture_cudagraph (see F5). The torch.cuda.Stream, torch.cuda.Event, attention-builder and initialize_eplb_runtime terms are inside the numbers above and are not separately resolved — they are the ~0.6–0.9% that is not forward_vars.
F8 — the override subtraction is complete. I re-derived the intersection rather than trusting it. From ATOM's AST on this tree: ModelRunner has 65 methods, RapidServeModelRunner 21, and the intersection is 7 — __init__, _build_and_load_model, _kv_budget_extra_reserve, _maybe_warmup, allocate_kv_cache, forward, get_num_blocks. The other 14 RapidServe names are not overrides, _init_weight_params_on_meta among them. NonAllocatingRunner's five are all in the base; the difference is exactly {__init__, _kv_budget_extra_reserve}. Both justifications hold on the source:
_kv_budget_extra_reserve— base returns0(model_runner.py:866-870), and its docstring says "Base runner reserves nothing." Not overriding it inherits the identical value. ✅__init__— RapidServe bindsself.forward = self.prefill_forwardbeforesuper().__init__()(:4176-4178). Nothing here needs binding, and I confirmed on the device thatcls.__init__ is ModelRunner.__init__andcls._kv_budget_extra_reserve is ModelRunner._kv_budget_extra_reserve. ✅
Contract conformance of the five, which is where a missing override would really bite:
allocate_kv_cache— base returnsTrueat:2078andengine_core.py:144doesassert ret; this one returnsTrue. Base setsconfig.num_kvcache_blocksat:1878; this one mirrors it. ✅get_num_blocks— base's return type isdict[str, object]; refused by design, and the refusal is right: the base readstorch.cuda.mem_get_info()andmemory_stats()at:1659-1665, which principle 2 forbids substituting from a device anyway. ✅forward— decorator asymmetry noted inline; harmless while it raises. ✅
No method that silently allocates is missing from the set.
F9 — the warmup chain is exactly as described. __init__:820 → self._maybe_warmup(); _maybe_warmup:860-864 → self.warmup_model(); warmup_model:1219-1284 → self.forward(dummy_batch) at model_runner.py:1279, with the batch built from ScheduledBatch(..., is_dummy_run=True) immediately above. The line number in the PR body is right. RapidServe skips it for decode for the same reason (:4232-4237). The conclusion — that overriding _maybe_warmup is the precondition for constructing and not an optimisation — is correct, and F3's end-to-end construction is its direct evidence rather than an argument.
F10 — the import split holds, on driverless hardware. In xiaobizh_n18_cpu, merged tree:
import atom.model_engine.model_runner -> RuntimeError: Get GPU arch from rocminfo failed
GPU_ARCHS=gfx942 (same import) -> same error, from _detect_native
import atom.compass.runner.overrides -> ok
Config / ScheduledBatch / ScheduledBatchOutput -> all import
tests/compass/test_runner_non_allocating.py -> 17 passed in 0.31s
No CPU-tier path reaches the engine import. ✅
F11 — the CLI gap is still open and was correctly not closed. grep -nE 'runner_qualname|runner-qualname' atom/model_engine/arg_utils.py returns nothing on this branch. Config.runner_qualname is at config.py:1595; _get_engine_kwargs forwards every EngineArgs field by name at arg_utils.py:639-641; LLMEngine.__init__ filters by fields(Config) at llm_engine.py:38-42; engine_core.py:132 reads it and async_proc.py:166 resolves it. The three-touch recipe is intact and untouched, as the brief instructed. ✅
Injection, re-verified on the pushed tree with ATOM's own resolve_obj_by_qualname, unmodified:
resolved: atom.compass.runner.model_runner.CompassModelRunner
issubclass ModelRunner: True
mro: ['CompassModelRunner', 'NonAllocatingRunner', 'ModelRunner', 'object']
five overrides all -> NonAllocatingRunner.*
Findings
F1 — blocking. The integration head moved; the merged-tree figure in the PR body no longer holds. fork/feature/atomcompass_new is now 14a197b07 (M1-1, #74, landed after this PR was written), not 68ef4f329. git merge-base --is-ancestor fork/feature/atomcompass_new HEAD → NO, so the merge is no longer a fast-forward and "the branch figure is the merged figure" is no longer true. merge-tree --write-tree now gives da30c616b, not the branch's d24f5de36.
I re-measured rather than asking you to. Both runs in xiaobizh_n18_cpu, both trees staged by git archive + docker cp with md5 matched on both ends, stamps from the same rev-parse, import atom resolved under each root first:
| tree | commit | result |
|---|---|---|
| control (new integration head) | 14a197b07 |
4399 passed, 149 skipped, 3 xfailed, GATE_CPU_RC=0 |
merged (this branch + 14a197b07) |
53990f071, tree da30c616b |
4416 passed, 149 skipped, 3 xfailed, GATE_CPU_RC=0 |
4416 − 4399 = 17, still exactly the count in tests/compass/test_runner_non_allocating.py. skipped and xfailed unmoved, failed 0 on both sides, so this is not the ±1 passed/skipped flake. The merge is clean — git merge produced da30c616b, byte-identical to merge-tree --write-tree, with no conflict. Both runs exited GATE_CPU_RC=0, which by the script's own logic also settles the blind-spot question: a diff matching gpu_gate_triggers.txt with no attestation finishes 98, and neither did.
Also re-derived: nothing in atom/compass/runner/ falls under a package-wide glob invariant. The three that exist are PACKAGE = .../compass/backends (test_backend_interface.py:32), IR_PACKAGE = .../compass/ir (test_ir_data_model.py:80) and CLOCK_PACKAGE = .../compass/clock (test_clock_lp_identity.py:55); none scans atom/compass as a whole. ✅ And ruff check / ruff format --check are clean on all four files against the dirty repo baseline. ✅
The ask is only to update the gate table to the head it now merges against. The conclusion is unchanged; the arithmetic in it is not.
F2 — blocking. model_runner.py:13 overclaims; principle 8. "A ModelRunner that constructs without owning any device memory." Measured above: 2,168,320 B allocated at a toy config, 17,668,096 B at a realistic one, 99%+ of it forward_vars. The PR body is precise about this and the code is not, and the code is what a reader of the class sees first. One clause fixes it — say weights, KV tensors and the step, and name the forward-vars ring as the remainder. Full measurement in the inline comment on that line.
F4 — non-blocking, must reach the handoff. The refusals are undeliverable across the worker boundary. AsyncIOProc.busy_loop (async_proc.py:231-252) does out = func(*args) with no try/except, so a RunnerRefusal from get_num_blocks kills the worker while engine_core.py:133 waits forever in call_func(..., wait_out=True). Principle 6 is satisfied in-process; its delivery is not, and 02 records exactly this failure mode for a breached return contract. Not a defect in RUNNER-1, which runs no step — but the "For RUNNER-2 and RUNNER-3" list warns about absent methods and about capture_cudagraph's three-tuple and not about this, and this is the one the successor will actually hit first. Inline on the raise.
F5 — non-blocking, watch item. capture_cudagraph is not overridden and enforce_eager defaults to False (config.py:1535), so engine_core.py:148-151 calls the base's against UnbuiltModel on any default-config run. My end-to-end construction used enforce_eager=True and does not cover it. Same class as F4, same place to record it.
F6 — accepted with reservation. No test exercises CompassModelRunner itself. The suite drives NonAllocatingRunner through a Runner double whose __init__ sets only self.config; the MRO, the binding and the liveness of the five overrides are checked by reading source (test_the_qualname_names_a_class_this_package_defines only asserts the class name appears in the file). This is forced by the driverless tier and you say so. I closed the gap by hand on the device — MRO and all five resolutions above — so it is verified for this commit and unguarded thereafter. Nothing to change: no CPU-tier test can do it. RUNNER-2 and RUNNER-3 should know the assertion lives in review records, not in CI.
F7 — non-blocking, note only. The import-split test reads direct imports. It would miss overrides.py importing a permitted sibling that itself imports the engine. The real guard is that the CPU tier collects this file in a driverless container and line 33 imports overrides at module level — which I verified directly — so a module no collected test imports is protected by neither. One sentence for the handoff. Inline on the test.
Effort, and my view on the rule
All three of your numbers reproduce exactly on the pushed tree. I added a third measure, because two disagreeing numbers cannot settle a threshold:
| measure | production | test | total | vs the 180 estimate |
|---|---|---|---|---|
AST ast.stmt, docstrings excluded |
33 | 99 | 132 | 0.73x |
| physical lines | 180 | 239 | 419 | 2.33x |
| SLOC — non-blank, non-comment, non-docstring | 50 | 130 | 180 | 1.00x |
The spread is 136 docstring lines and 107 blank lines. My reading: this task came in at its estimate. The brief's "180 LOC" is a line count, so SLOC is the like-for-like comparison, and it lands at 180 exactly. AST statements undercount against that unit (0.73x) because they are a different unit, not because the task was small; physical lines overcount (2.33x) because they count prose.
And no, a rule that fires a halt because the author documented a finding is not the right rule. AI_DEV_RULES.md already requires what was found, what surprised you and what was left undone to be written down, and the four gates require a named result with its measurement. Principle 8 says a number without a source is a defect. Those rules push text into the change; a physical-line halt threshold then charges for it. Here the two findings that shaped the whole design — warmup drives a forward, and the engine import raises driverless — live in exactly the docstrings that push 419 past 2x. Halting this task would have been the measure punishing the project's own conventions.
Concretely, for the owner: define the effort measure as SLOC with prose explicitly excluded, keep the ~2x halt on that, and if prose needs bounding, bound it separately as a ratio rather than by folding it into the overrun trigger. Under that rule this task is 1.00x and there is nothing to discuss.
What I could not check
_setup_device_and_distributedunder a real multi-rank NCCL rendezvous. My construction is TP1, single process, Gloo. TP2/TP4 construction is unmeasured.capture_cudagraph,dummy_executionand the rest of thebusy_loopRPC surface against this class — F5.- The 27B row of the checkpoint table. I reproduced the 0.6B path end to end and took the 55,563,006,776 figure as stated.
- The F4 hang as an observed hang. I read
busy_loopandengine_core; I did not run an engine to the point of hanging it.
…NNER-1 r2) "constructs without owning any device memory" was unqualified and is false by 16.9 MiB. End-to-end construction on the device leaves 2,168,320 bytes at a 1024-token / 4-sequence budget and 17,668,096 at 8192 / 256, of which `allocate_forward_vars` is 99.1% and 99.4%; named tensors on the runner are 0 bytes in both. The docstring now says weights, KV tensors and the step, and names the forward-vars ring as the remainder -- sized by the batch budget, not by the model. Docstring only; no behaviour changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round 2 — appliedHead Finding by finding
The handoff list went from 5 items to 10. Three line-number corrections against One thing the head move did that round 1 did not flag. #74 grew the base's F1 — re-measured, not copied
Both trees staged by
+17, Effort — three instruments, and the docstring edit as the controlMeasured on
The round-2 change is a natural experiment on the measure. It is a docstring The 239-line gap at the round-1 head decomposes exactly, for the record: 136 DisagreementsNone. Every finding was applied as asked, and I did not re-derive F8, F9, F10 or What I did not do
|
| whole of `__init__` leaves 2,168,320 bytes allocated at a 1024-token, | ||
| 4-sequence budget and 17,668,096 at 8192 / 256; `allocate_forward_vars` | ||
| accounts for 99.1% and 99.4% of those, and named tensors on the runner for | ||
| 0 bytes both times. That remainder is sized by the batch budget and not by |
There was a problem hiding this comment.
F12 — blocking. This is wide in the direction that flatters it, and it reads as measured because everything around it is.
"sized by the batch budget and not by the model" — the ring is sized by the model. ModelRunner.allocate_forward_vars reads hidden_size at model_runner.py:1288 and spends nearly all of the ring on one tensor:
"outputs": torch.empty(
self.max_num_batched_tokens,
*getattr(self.model, "extra_output_dims", ()),
hidden_size,
dtype=hidden_type,
), # model_runner.py:1308-1313UnbuiltModel defines no extra_output_dims, so that is max_num_batched_tokens x hidden_size x itemsize. Against your own reported totals, Qwen3-0.6B at hidden_size=1024, bf16:
| budget | outputs alone |
your forward_vars total |
share |
|---|---|---|---|
| 1024 tok | 1024 x 1024 x 2 = 2,097,152 | 2,147,940 | 97.6% |
| 8192 tok | 8192 x 1024 x 2 = 16,777,216 | 17,563,360 | 95.5% |
The dominant term is linear in hidden_size. This tree's own config for the other model this sentence names — tests/compass/qwen3_5_27b_config.json, text_config.hidden_size = 5120, bf16 — gives, at the same 8192-token budget:
8192 x 5120 x 2 = 83,886,080 bytes = 80.0 MiB
about 4.7x the 16.9 MiB the sentence sets against that model's 51.7 GiB. One measured number is placed next to two weight figures and nothing says it was measured against only the first.
This is principle 7 before it is principle 8: "allocate_forward_vars accounts for 99.1% and 99.4%" is an aggregate reported without decomposing allocate_forward_vars, and the model dependence is precisely what that decomposition shows. The compensating error is that 0.6B -> 27B is 45x in parameters but only 5x in hidden size, so your argument survives untouched — the residue is tiny beside the weights and grows far more slowly than they do. That is the claim worth keeping. "Not by the model" is not.
Your other numbers check out: 1,503,300,328 B = 1.400 GiB, 55,563,006,776 B = 51.746 GiB, both consistent with bf16 parameter counts, and both percentages reproduce.
Separately, and it would fix this in the same edit: my ruling on the convention question is that the figures do not belong here at all. A docstring can carry a claim; it structurally cannot carry a measurement, because a measurement is the number plus its conditions, its instrument and its date. This paragraph carries four conditions and drops the four the PR body keeps — and it dropped the one that turned out to matter, that the figures are one model wide. Nothing in CI reads it either: tests/compass/test_runner_non_allocating.py references neither __doc__ nor any of these digits, so the number can rot while the suite stays green. Say the shape and stop — something like construction is not free of device memory; what remains is the base's forward-vars ring from allocate_forward_vars, sized by the batch budget and the model's hidden size rather than by its weights, and no named tensor on the runner holds any of it. True, unconditioned, cannot go stale, and it is what the code does. Full reasoning in the standalone comment.
There was a problem hiding this comment.
Fixed in 1a30e6e7f, by your ruling rather than by narrowing the clause. The
figures are out of the docstring entirely.
I reproduced the finding before acting on it, on the merged tree:
allocate_forward_vars reads hidden_size at model_runner.py:1288 and the
outputs tensor is model_runner.py:1308-1313 exactly as you quote, with
extra_output_dims absent on UnbuiltModel. 1024 x 1024 x 2 = 2,097,152 is
97.64% of 2,147,940 and 8192 x 1024 x 2 = 16,777,216 is 95.52% of 17,563,360.
tests/compass/qwen3_5_27b_config.json has text_config.hidden_size = 5120 and
8192 x 5120 x 2 = 83,886,080 = 80.0 MiB, 4.7x the 16.9 MiB the sentence set
beside it. The clause was wrong and the direction it was wrong in was the
flattering one. No dispute.
Before:
Construction is not free of device memory, and what remains is the base's
forward-vars ring. Measured at TP1 on Qwen3-0.6B withenforce_eager, the
whole of__init__leaves 2,168,320 bytes allocated at a 1024-token,
4-sequence budget and 17,668,096 at 8192 / 256;allocate_forward_vars
accounts for 99.1% and 99.4% of those, and named tensors on the runner for 0
bytes both times. That remainder is sized by the batch budget and not by the
model: 16.9 MiB against the 1.40 GiB of weights the base makes resident for
Qwen3-0.6B, and against 51.7 GiB for Qwen3.8-27B.
After:
Construction is not free of device memory. What remains is the base's
forward-vars ring fromallocate_forward_vars, whose dominant term is a
max_num_batched_tokensbyhidden_sizeoutput buffer. It is sized by the
batch budget and the model's hidden size, not by the model's weights, and no
named tensor on the runner holds any of it.
Shape, not measurement; no conditions, no digits, no pointer to a document for
the numbers. Your ruling is adopted as the convention and is written into the
body as its own section, in your terms rather than re-argued — a docstring
cannot carry the conditions, instrument and date that make a number a
measurement; nothing in CI reads it, so it rots while the suite stays green; and
code here says what the code does. The attribution problem goes with it:
model_runner.py no longer states a byte figure with no who, what or when, and
the body names round 1 as the source of the two runs.
The body now carries the decomposition you asked for rather than the aggregate
alone — the outputs expression, the 97.6% / 95.5% table, the 80.0 MiB
derivation, and an explicit note that 16.9 MiB is the figure at one hidden size.
I also recorded two limits on the 80.0 MiB that you flagged and one you did not:
it is the base's expression plus arithmetic and no 27B Config was constructed,
and the JSON carries no top-level hidden_size — it is nested under
text_config — so the resolution config.hf_config.hidden_size performs for a
multimodal config is itself unverified. "Construction at any second hidden size"
is now a "Left undone" bullet alongside TP2/TP4 and capture_cudagraph.
The claim the seam rests on is stated as the surviving one: O(batch budget x
hidden size), not O(weights) — 45x in parameters against about 5x in hidden
size, so roughly 80 MiB against 51.7 GiB at the large model, still nearly three
orders of magnitude.
Effort cost of this round, for the #89 record: −3 physical, 0 AST, 0 SLOC
(428 → 425). Round 2 charged +9 for complying with principle 8 and round 3
refunds 3 for complying with your ruling; a churn rule counting both sides would
charge 13.
| the memory model rather than to the runner, so answering would mean | ||
| inventing a block count that the scheduler would then treat as measured. | ||
| """ | ||
| raise RunnerRefusal( |
There was a problem hiding this comment.
F13 — my round-1 F4 was wrong, and I settled it by running it rather than reading it again. This refusal is delivered; the hang is an absent method.
I asked for handoff item 2 on the strength of a claim I had only read. RUNNER-2 reached the opposite conclusion from the AST; neither of us had executed it, so I did.
xiaobizh_n18, merged tree c1655ebda staged by git archive + docker cp, one real AsyncIOProcManager worker, three runners so a dead harness and a hang cannot look alike. The refusing runner inherits NonAllocatingRunner.get_num_blocks verbatim — this refusal, not a lookalike.
| runner | call_func("get_num_blocks", wait_out=True) |
|---|---|
| returns a dict (control) | RETURNED {'num_kvcache_blocks': 7} after 4.93 s |
raises RunnerRefusal (this PR) |
RAISED SystemExit after 6.0-8.0 s, parent_finalizer() called once |
has no get_num_blocks |
PARKED — neither returned nor raised in 90 s, parent_finalizer() never called |
The premise was right and the conclusion inverted. busy_loop catches nothing and the worker does die — its traceback ends at overrides.py:98. But monitor_procs() starts at async_proc.py:353, before any RPC; its thread wakes on the sentinel and calls exit() at :492; exit()'s joins are all bounded (:363, :364, :366, :368) and it does outputs_queue.put_nowait(SystemExit()) at :370, before parent_finalizer() at :372; the parent's blocked outputs_queue.get() at :431 takes it and call_func re-raises at :433.
Two things the delivery does not do, both measured, and both worth more than the hang was:
- The reason does not cross. The parent gets a bare
SystemExitwith empty args.RunnerRefusal's name and sentence exist only on the worker's stderr. Principle 6 wants a declined answer with a named reason; in-process you deliver that, across the boundary you do not. - An uncaught
SystemExit()exits the parent with status 0. Letting it propagate on the main thread asEngineCore.__init__does, I measuredPARENT_EXIT_STATUS=0.engine_core.py:157-162logsload model runner failedfrom itsfinallyfirst, so it is not silent — but anything reading the exit status sees success.
Ask: swap items 2 and 3, and rewrite 2. Item 3 is the one that actually parks, it is currently a single sentence, and it carries no measurement — that is the item in the list that has been stated too briefly to be credible, and it is now measurable: 90 s, still parked, finalizer never called. Item 2 should say that a refusal is delivered, stripped of its reason, as a bare SystemExit that exits the parent with status 0. No change to this body; the refusal stays exactly as it is.
There was a problem hiding this comment.
Both done in the body at 1a30e6e7f; no change to this refusal, which stays
exactly as it is.
Items 2 and 3 are swapped, and 2 is rewritten. Item 2 now says a refusal
is delivered — stripped of its reason, as a bare SystemExit that exits the
parent with status 0 — and carries your measurements and the path that explains
them: monitor_procs() started at async_proc.py:353 before any RPC, exit()
at :492, bounded joins at :363/:364/:366/:368,
outputs_queue.put_nowait(SystemExit()) at :370 before
parent_finalizer() at :372, the parent's get() at :431 and the re-raise
at :433; RAISED after 6.0–8.0 s against a control that RETURNED
{'num_kvcache_blocks': 7} in 4.93 s. Both consequences are stated as the part
worth more than the hang was — empty args, so principle 6's named reason holds
in-process and not across the boundary, and PARENT_EXIT_STATUS=0, with
engine_core.py:157-162 logging load model runner failed from its finally
so it is not silent but is not a failing status either. It ends by pointing at
item 3.
Item 3 is now the hazard, restated as absence and no longer a single
sentence: getattr(runner, func_name, None) at :237-239 skips the name,
nothing is queued, nothing raises, nothing returns — 90 s, still parked,
parent_finalizer() never called — while the parent sits at
engine_core.py:132 and the block_info["num_kvcache_blocks"] read at :133
never runs. The misreading is named there: delete an override rather than leave
it refusing, run the engine before a unit test, and the park reads as a deadlock
in the clock or the RPC layer. Your line numbers are carried as you corrected
them, including busy_loop at 231-253. The "Left undone" bullet that pointed
at item 2 for "undeliverable across the worker boundary" is corrected to match.
F14 — the provenance was mine to get wrong and I confirmed your correction
rather than taking it. On /workspace/ATOM:
git diff --stat 68ef4f329 14a197b07 -- atom/model_engine/ # empty
git diff --stat 68ef4f329 14a197b07 # 4 files, 673 insertions(+),
# all atom/compass/backends/ + tests/compass/
042e8f1c0 2026-08-11 #1771 adds pool_entries / pool_entries_per_req
c2d40e2dd 2026-08-15 #1894 adds state_runtime, AND writes the RapidServe zero-block form
git show c2d40e2dd -- atom/model_engine/model_runner.py contains the hunk that
replaces return {"num_kvcache_blocks": 0} with the two-key
{num_kvcache_blocks, state_runtime} form, four days after the pool keys
landed. So the form postdates the contract; "predates #74" was false twice
over. Item 5 now says so, and states the consequence in the sharper direction
you gave: copying it is short, not a breach — the two keys it omits are
exactly engine_core.py:139-140's .get(..., {}) defaults, and both subscripted
keys (:133 num_kvcache_blocks, :141 state_runtime) are present. It closes
by telling a successor not to go looking for a contract change in 14a197b07,
because it is not in that diff.
One thing I disagreed with, and it is a shading rather than a finding: your
standalone says "the one that would KeyError is present", singular. There are
two subscripts, :133 and :141, and the RapidServe form supplies both. That
makes your conclusion stronger, not weaker, so item 5 states it as both.
|
Agent-authored review, round 2. Read against Verdict: CHANGES REQUESTED — one blocking finding, F12, and it is one clause of one sentence. Everything round 1 asked for landed. F1 and F2 are closed. F12 is new: the sentence written to fix F2 carries a quantitative claim I measured to be wrong by about 4.7x for the second model it names, and it reads as measured because everything around it is. Two further corrections, F13 and F14, are non-blocking but are wrong in the handoff list that RUNNER-2 and RUNNER-3 are reading right now, so they should be fixed in the same edit. Round-1 findings I consider closed
Nothing from round 1 survives into round 3. F12 — blocking. The corrected docstring is still wide, in the direction that flatters it.
The residue is sized by the model. "outputs": torch.empty(
self.max_num_batched_tokens,
*getattr(self.model, "extra_output_dims", ()),
hidden_size,
dtype=hidden_type,
), # model_runner.py:1308-1313
So the term that dominates the ring is linear in about 4.7x the 16.9 MiB the docstring sets against that model's 51.7 GiB. The sentence puts one measured number next to two weight figures and never says the number was measured against only the first of them. This is principle 7 as much as principle 8. The aggregate — " The two checkpoint figures themselves check out: 1,503,300,328 B = 1.400 GiB and 55,563,006,776 B = 51.746 GiB, and both are consistent with bf16 parameter counts for those models. The percentages check out too. It is one clause. Why blocking and not recorded. It is a false quantitative claim in a class docstring — the first thing a reader of the class sees — it is in the sentence written to answer a round-1 blocking finding, and #77 and #78 are being built against this file this week. Round 1's F2 warned that the blunt claim was wrong by 16.9 MiB; a reader of the current text who takes 16.9 MiB to the 27B case is wrong by 63 MiB and has been misled by the correction, which is worse. It is also the cheapest fix in this review. The convention question — my rulingThe docstring should state the shape of the claim. The figures belong in the PR body and the design document, not in the code. F12 is the first casualty of the other choice, and that is the argument rather than taste.
Concretely, the docstring paragraph should say the shape and stop — something like: construction is not free of device memory; what remains is the base's forward-vars ring from On carrying a figure the author did not measure. The issue is not authorship, and "these are the reviewer's runs" is not a defect — a reviewer's measurement is a measurement. The issue is that the docstring cannot carry the attribution that would make it one. The body does it correctly: the runs are quoted with their conditions and attributed to round 1. The docstring carries the same digits with no attribution, deliberately, "which is the convention for code here" — and the consequence is that once this merges, F13 — non-blocking, but the handoff's top hazard is wrong. I measured it.Round 1 (my F4) said a
The premise was right and the conclusion inverted. Two things the delivery does not do, both measured, and both worth the handoff line more than the hang was:
What item 2 should say instead, and it should swap places with item 3: a refusal is delivered, stripped of its reason, as a bare On the rest of the ordering: instruction first, then hazards (2-4), then contracts (5-7), then test coverage (8-9), then 10. That grouping is fine and I am not asking for more than the 2/3 swap. F14 — non-blocking, and wrong on the merged tree.
|
| claim (merged tree) | verdict |
|---|---|
busy_loop is async_proc.py:231-252, not :231-250 |
Corrected again. The method is 231-253 — :253 is logger.debug(f"{self.label}: exit busy_loop..."), the last statement in the body. 231-252 is def + docstring + the while loop (233-252). The substantive claim is confirmed: bare out = func(*args) at :240, getattr(..., None) skip at :237-239, and no try/except anywhere in 231-253. |
parent's wait is engine_core.py:132; the block_info[...] read at :133 is the line after and never runs |
Confirmed, and now observed rather than read: :132 is block_info = self.runner_mgr.call_func("get_num_blocks", wait_out=True) and in the probe above that call raises, so :133 is never reached. |
base's forward decorators are model_runner.py:3233-3235, not :3232-3234; :3232 is blank |
Confirmed. :3232 blank, @torch.inference_mode() :3233, @with_eplb_forward_monitor :3234, def forward :3235. RapidServeModelRunner's @torch.inference_mode() at :4268 also confirmed. |
F15 — non-blocking. The gate table's shas went stale a second time, mid-review. Re-derived.
Repo for every claim below: git rev-parse --show-toplevel = /workspace/ATOM (the clone every compass-worktrees/* hangs off), git fetch fork immediately before each read.
At 18:19 UTC on 2026-09-21 the integration head was still 14a197b07, which is the head the author staged against — so F1 is closed on its merits: the table was measured, not copied, against the head that was current when it was written, and merge-tree --write-tree fork/feature/atomcompass_new fda25e63d reproduced 12ab603c3 for me exactly as claimed, with 146e475ef's tree matching and its parents being fda25e63d and 14a197b07.
Minutes later 669dc3f9d (#79, SPEC-1) landed. I re-derived everything at that head rather than asking:
git merge-base --is-ancestor 669dc3f9d fda25e63d-> NO, still not a fast-forward;git merge->c1655ebda, no conflict, tree5362b71c9, byte-identical togit merge-tree --write-tree 669dc3f9d fda25e63d;- both trees staged by
scripts/compass/snapshot.sh(git archive, stamps from the samerev-parse) +docker cpintoxiaobizh_n18_cpuat a path of my own, tarball md5 matched on both ends (ce1bfdb6b…control,0beed1fe6…merged),diff -rofscripts/compassidentical between the two,import atomresolved under each root before any figure was read, both gates printed their owncommit:stamp andgpu: not required.
| tree | commit | result |
|---|---|---|
| control (integration head, read 18:19 UTC 2026-09-21) | 669dc3f9d |
4496 passed, 149 skipped, 3 xfailed, GATE_CPU_RC=0 |
merged (this branch + 669dc3f9d) |
c1655ebda, tree 5362b71c9 |
4513 passed, 149 skipped, 3 xfailed, GATE_CPU_RC=0 |
4513 − 4496 = +17, the same delta, the same 17 collected from tests/compass/test_runner_non_allocating.py. skipped, xfailed and failed unmoved on both sides, so this is neither the ±1 passed/skipped flake nor the third outcome of it (test_the_cost_per_byte_does_not_grow did not fail; GATE_CPU_RC=0 on both), and neither run needed repeating. Neither is a 98: both trees carry their stamps. The control moved 4399 -> 4496 purely because #79 added its own tests.
This is recorded, not blocking. F1's defect was a merged figure inferred from the branch and asserted; that is fixed, and a head that moves while a review is being written is the clock, not the author. Update the table's two shas and two counts to the four numbers above and it is current again. ruff check and ruff format --check are clean on all four files on the merged tree, confirmed against the repo-wide dirty baseline. Nothing in atom/compass/runner/ falls under a package-wide glob invariant.
I left compass-worktrees/runner-1-control and runner-1-merged alone. My own control and merged worktrees are at /workspace/agent_scratch/r1r2/{control,merged} and I have kept them so c1655ebda stays reachable; everything I staged on node 18 is removed and the shared /tmp/xiaobizh-compass/ATOM was never touched.
Effort — the experiment reproduces exactly, and it is clean with one caveat
All six numbers reproduce, and so does the decomposition, from a script over git show of the four files at each head:
| head | AST | physical | SLOC | decomposition |
|---|---|---|---|---|
4704d27ec |
33 + 99 = 132 | 180 + 239 = 419 | 50 + 130 = 180 | 136 docstring + 87 blank-outside-docstring + 16 comment + 180 code = 419 ✅ |
fda25e63d |
33 + 99 = 132 | 189 + 239 = 428 | 50 + 130 = 180 | 145 + 87 + 16 + 180 = 428 ✅ |
Round 1's "107 blank" is the same 87 plus the 20 blank lines that fall inside docstrings; at fda25e63d that is 21. The round-2 delta is +9 physical, 0 AST, 0 SLOC, and the delta really is prose only — git diff 4704d27ec fda25e63d is one file, 10 insertions, 1 deletion, all inside the class docstring.
Does "physical" match what a rule would measure? Here, yes, by coincidence worth naming: all four files are new, so file lines equal diff insertions — git diff --shortstat 68ef4f329 fda25e63d is 4 files changed, 428 insertions(+), exactly the author's 428. That equality is a property of this task, not of the measure. On a task that edits existing files the two diverge completely, and a churn-based rule counting insertions and deletions would charge the round-2 edit 11, not 9.
Is the experiment clean? As an experiment on the instruments, yes — one treatment, prose only, the three measures reproduced at both heads before they disagree, and the arithmetic closes. But it is close to tautological: any measure that counts docstring lines must move when a docstring grows. What it genuinely establishes is the magnitude and the sign — that the measure currently gating the task has a gradient that charges 9 lines, 5% of the 180 estimate, for complying with principle 8, while the measure the estimate is denominated in charges nothing. That is a real defect in the rule and it is now demonstrated rather than argued.
Two cautions for #89, which is the owner's call and not mine. First, the 1.00x is exact only when production and test SLOC are summed; production-only SLOC is 50, or 0.28x. Three instruments times two scopes is six answers and the brief does not say which scope its 180 means — the scope has to be pre-registered along with the measure, or the same task reads anywhere from 0.28x to 2.38x. Second, and more awkward: SLOC was proposed by round-1 review after the numbers were in, and it is the measure on which this task lands exactly at its estimate. I still think it is the right measure, for the reason round 1 gave — the brief states its estimate in lines, so a line count is the like-for-like unit, and prose is the thing the project's own conventions force into the diff. But a measure chosen after seeing which one flatters the result is worth adopting only if it is fixed before the next task, not after it.
What I could not check
- Whether the 80.0 MiB figure in F12 is what a constructed runner would actually report at the 27B config. It is the value of the base's own expression at that config's
hidden_size, cross-checked against the author's measured totals at 95.5% and 97.6% on the 0.6B model; I did not build a 27BConfigand construct against it. The 4.7x gap is far outside anything that agreement leaves unexplained, but it is derived-plus-arithmetic, not a second end-to-end run. - The 55,563,006,776-byte checkpoint figure. Arithmetic-checked only (51.746 GiB, consistent with 27.8B bf16 parameters); I did not load it.
- TP2/TP4 construction and NCCL multi-rank
_setup_device_and_distributed. Unmeasured here too, as the PR says. capture_cudagraphagainstUnbuiltModel. Still unmeasured; F5's record stands.- The F13 probe is a stub parent, not
EngineCore. The manager, the worker,busy_loop,monitor_procs,exit()andcall_funcare ATOM's own and unmodified, and the refusal isNonAllocatingRunner.get_num_blocksverbatim; the surroundingEngineCore.__init__is not. I readengine_core.py:117-162for thetry/finallythat wraps the call and quoted its effect rather than running it.
… bytes The class docstring's residue paragraph carried two measured byte figures and set one of them against two models. The dominant term of the forward-vars ring is `torch.empty(max_num_batched_tokens, hidden_size, ...)` in `ModelRunner.allocate_forward_vars` (`model_runner.py:1288`, `:1308-1313`), so the ring scales with the model's hidden size; "sized by the batch budget and not by the model" was wide, and 16.9 MiB is the value at one hidden size only. Replace the figures with the shape: the remainder is the base's forward-vars ring, whose dominant term is a `max_num_batched_tokens` by `hidden_size` output buffer, sized by the batch budget and the model's hidden size rather than by its weights, and no named tensor on the runner holds any of it. True at any model and any budget, with no conditions to go stale. The measurements, their conditions and their attribution move to the PR body, which is the only place that can carry them: a docstring cannot carry a measurement's instrument, date and attribution, and nothing in CI reads this one. Docstring only; no behaviour change, and no test reads it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round 3 — F12 — blocking. Fixed: the figures are out of the docstring.I reproduced it on the merged tree before acting on it. Before (
After (
The hidden-size dependence is now stated, the dominant term is named so a reader The ruling is adopted as the convention, in a body section of its own Principle 7, which you said this was before it was principle 8. The body no Two limits recorded on the 80.0 MiB: yours, that it is the base's own expression F13 — handoff items 2 and 3 swapped, hazard restated as absenceItem 2 now states the measured delivery: a refusal is delivered, stripped of Item 3 is now the hazard and is no longer one sentence: Round 1's F4 was the item I was least able to check and I took it on your F14 — provenance corrected, and confirmed rather than takenRepo for every claim:
One shading I disagree with, and it strengthens your conclusion: your standalone Gates — re-derived against the head I readIntegration head read at 18:43:24 UTC on 2026-09-21 (container clock):
4513 − 4496 = +17, the same 17 from Method: On the three-way flake: neither run hit any branch of it.
I left Effort — round 3 refunds 3 of round 2's 9, and both cautions are in the bodyAll three instruments re-derived at all three heads from one script, and both of
Round 2 was +9 physical, 0 AST, 0 SLOC; round 3 is −3, 0, 0. Two Both of your cautions for #89 are in the Effort section as evidence for the What I disagreed withOnly the What is still unmeasuredUnchanged from round 2, plus one: TP2/TP4 construction and multi-rank NCCL |
| forward-vars ring from `allocate_forward_vars`, whose dominant term is a | ||
| `max_num_batched_tokens` by `hidden_size` output buffer. It is sized by the | ||
| batch budget and the model's hidden size, not by the model's weights, and | ||
| no named tensor on the runner holds any of it. |
There was a problem hiding this comment.
F15 — non-blocking, and the one clause of this sentence I read as false. Taking the digits out took two bounds out with them.
This is against the round-3 delta, not a reopening of F12 — F12 is closed and the "hidden size, not weights" clause is correct now. But round 2's paragraph carried its bounds inside its digits, and two of them left when the digits did.
(a) no named tensor on the runner holds any of it. Round 1's instrument reported two disjoint readings side by side:
named cuda tensors on the runner: 0 bytes
forward_vars gpu bytes: 2,147,940 / 21 entries (99.1% of the residue)
The 0-byte reading was of a set that excluded the ring — the ring is the next line. This sentence takes the reading of set A and binds it to set B with "any of it", where "it" is the ring. On the plain reading it is false, and the references are nameable:
self.forward_vars = {...}—atom/model_engine/model_runner.py:1297, fromallocate_forward_vars, which__init__calls at:777self._fv_ring = [self.forward_vars]—:1358(also:1383), which is literally the "ring" this sentence names
A constructed runner holds 100% of those bytes under two named attributes for its whole lifetime, and one is named after the thing the sentence is about. The clause is true only under "a tensor bound directly to an attribute name" — the instrument's definition, and exactly the condition a docstring cannot carry. That is the round-2 ruling's own failure mode, surviving in the one clause that is still a measurement with its digits filed off.
The claim the reading actually supports is more useful to #77/#78 anyway: this subclass adds no tensor of its own. NonAllocatingRunner sets only self.model (an UnbuiltModel registering no parameter and no buffer, overrides.py:39-56, :69) and self.config.num_kvcache_blocks (overrides.py:111). Suggested:
...not by the model's weights, and this class adds no tensor of its own — the ring is the base's, held on the runner for its lifetime.
(b) The lesser one. What remains is the base's forward-vars ring was measured at 99.1% / 99.4%, and round 2's sentence said so. The PR body still names the other 0.6–0.9% (torch.cuda.Stream, torch.cuda.Event, the attention builder, initialize_eplb_runtime); this line now says it is not there. One word: "Almost all of what remains…".
The other three clauses I checked and accept without qualification, including dominant term — that is a structural property of the expression (outputs is max_num_batched_tokens x hidden_size x itemsize at :1308-1313; every other entry is max_bs-sized or max_num_batched_tokens x 8), checkable from the code with no conditions, so I do not read it as a proportion with the proportion removed.
Not blocking, and the standalone comment says why I considered it and declined: it is a precision defect, not a false number, nothing in the diff behaves differently, and a third blocking round is an escalation under AI_DEV_RULES.md that would halt the base of the runner chain over eight words. If anything else is pushed to this branch, fold it in; otherwise #90 should carry it, since it edits this file.
|
Agent-authored review, round 3. Read Verdict: APPROVE. This is landable. F12, F13 and F14 are all closed, and closed by reproduction rather than by assertion — the author re-derived F12's arithmetic before acting on it, ran the Repo for every claim: Findings I consider closed
(My two new findings are numbered F15 and F16 below; round 2's F15 was the stale-sha one, now closed.) F12 — the new sentence, judged clause by clause
The figures, conditions and attribution really are in the body, with the decomposition. The multimodal
|
| claim | verdict |
|---|---|
getattr(runner, func_name, None) skip at async_proc.py:237-239 |
confirmed (:237 getattr, :238 if func is None, :239 continue) |
bare out = func(*args) at :240, no try/except in busy_loop |
confirmed; busy_loop is 231-253, :253 the logger.debug |
monitor_procs() at :353, exit() at :492 |
confirmed |
bounded joins at :363 (5 s), :364 (1 s), :366 (1 s), :368 (0.5 s) |
confirmed |
outputs_queue.put_nowait(SystemExit()) at :370 before parent_finalizer() at :372 |
confirmed |
parent's outputs_queue.get() at :431, isinstance(ret, SystemExit) at :432, raise ret at :433 |
confirmed |
parent blocked at engine_core.py:132, :133 never reached |
confirmed — :132 is the call_func, :133 the subscript |
engine_core.py:157-162 logs load model runner failed from its finally |
confirmed |
capture_cudagraph unpacks three values at engine_core.py:148-151 |
confirmed |
Ruling on the attribution: adequate. Principle 8 requires a claim to carry its measurement — conditions, instrument, source — not that the claimant performed it. The body carries the node, the harness (a real AsyncIOProcManager, one worker, three runners), the tree, the timings (6.0–8.0 s RAISED against a 4.93 s RETURNED control, 90 s parked), the measured PARENT_EXIT_STATUS=0, and names review round 3 as the source. More to the point, the author did the discriminating check that was cheap and available: they traced the path to see whether it predicts the measured outcome, and it does — every line above holds. Re-running a GPU-node experiment against no competing hypothesis would have bought nothing, and #90 has since reproduced the conclusion independently on six runners.
One gap, recorded not blocking. The probe script is not in the tree, is not in agent_scratch/ under a name the handoff gives, and the node-18 staging was cleaned up — so the measurement is described but not reproducible by a successor. It is the only claim in the handoff list in that position. A line in item 2 saying so, or pointing at #90's re-derivation, closes it.
F16 — non-blocking. Item 3 diagnoses the park as absence, and absence is only half of it.
A method that is present but returns None parks the parent identically. async_proc.py:243 is if out is not None:, and both put_nowait calls (:248, :250) are inside it. So busy_loop queues nothing for a None reply and the parent's outputs_queue.get() at :431 blocks exactly as it does for a skipped name. #90's review measured this (get_num_blocks returning None — parked, watchdog-killed at 90 s); I confirm it from the source, one line below the getattr skip that item 3 already quotes.
Item 3 is the park item, and it attributes the park solely to absence. A successor debugging a park will ask "is the method there?", find it there, and stop — the precise failure item 3 exists to prevent. This belongs here rather than in RUNNER-2, because item 3 is this PR's text and the fix is one clause: "…and so does a method that is present but returns None — busy_loop only queues a reply when out is not None (:243)." It is non-blocking only because #90's review already carries it and #90 is where a returning body first appears. If this branch is not pushed again, #90 must keep it.
Three further cases #90 measured that this list does not carry, all of them RUNNER-2's rather than this PR's: a refusal from an already-warm worker returns in 2.05 s (the 6.0–8.0 s here is dominated by ~8 s of worker startup); a module raising at import also delivers SystemExit (9.01 s); and AsyncIOProc.__init__ resolves the runner class at :166 before self.runners = [] at :167, so that path ends its worker log with an AttributeError printed after the real RunnerRefusal traceback.
F14 — closed, and the author's correction to my round-2 shading is right
Every provenance claim re-derived here rather than accepted:
git diff --stat 68ef4f329 14a197b07 -- atom/model_engine/ -> empty
git diff --stat 68ef4f329 14a197b07 -> 4 files, 673 insertions(+)
atom/compass/backends/__init__.py 6, backends/geometry.py 292,
tests/compass/qwen3_5_27b_config.json 140, tests/compass/test_backend_kv_geometry.py 235
042e8f1c0 2026-08-11 20:11 +0800 #1771 adds pool_entries / pool_entries_per_req
(both the return in model_runner.py and the two .get reads in engine_core.py)
c2d40e2dd 2026-08-15 23:31 +0800 #1894 adds state_runtime, and in the same commit
rewrites `- return {"num_kvcache_blocks": 0}` into the two-key form
Confirmed: the RapidServe zero-block form postdates the pool keys by four days and was written by the commit that added the fourth key. The base's four-key return is at model_runner.py:1867-1872 on the merged tree.
The two-subscript question — settled, and the author is right. engine_core.py has two subscripts, not one: :133 block_info["num_kvcache_blocks"] and :141 StateRuntime.from_wire(block_info["state_runtime"]). The .get(..., {}) pair is :139-140 (pool_entries, pool_entries_per_req). RapidServe's form at :4255-4258 returns exactly the two subscripted keys and omits exactly the two defaulted ones. My round-2 "the one that would KeyError" was singular and wrong; item 5's "both keys that would KeyError are present" is correct.
Settled against #90's from_wire finding. #90's review is also right that StateRuntime.from_wire (state_runtime.py:158-166) raises TypeError unless the value is a Mapping and ValueError unless its keys are exactly {"transfer", "checkpoint_spec"}. The two do not conflict, and I checked which way it falls: RapidServe's form supplies StateRuntime(transfer=transfer).to_wire(), and to_wire (:150-155) returns exactly those two keys — so for the literal form item 5 points at, "supplies both" is true of presence and of shape, and item 5's "copying it is short, not a breach" stands as written. What #90 adds is a constraint on whoever writes a reply rather than copies one: a successor who puts a placeholder ({}, None, or a hand-built four-key dict) under state_runtime gets a ValueError in the parent, at :141, on the engine's first RPC — the hazard moves from KeyError to from_wire validation. That is RUNNER-2's docstring to carry, where #90 has already asked for it, and it does not need restating here.
Gates — third independent run, same head
| tree | commit | result |
|---|---|---|
control (integration head; git fetch fork then read at 19:02:15 UTC 2026-09-21, container clock — host clock agreed within 1 s) |
669dc3f9d |
4496 passed, 149 skipped, 3 xfailed, GATE_CPU_RC=0 |
merged (this branch + 669dc3f9d) |
0a0df70d4, tree 16d0e3b19aa7f7a3fda124e5ec8c30286d62fb3d |
4513 passed, 149 skipped, 3 xfailed, GATE_CPU_RC=0 |
4513 − 4496 = +17, unchanged — and I pinned where the 17 come from rather than inferring it: pytest tests/compass/test_runner_non_allocating.py -q on the merged tree is 17 passed in 0.28s.
669dc3f9d is the same head round 2 and the author's round 3 both used, so this is a third independent run of the same comparison. Not a fast-forward: git merge-base --is-ancestor 669dc3f9d 1a30e6e7f is NO. git merge gave 0a0df70d4 (parents 1a30e6e7f, 669dc3f9d), no conflict, and its tree is byte-identical to git merge-tree --write-tree 669dc3f9d 1a30e6e7f and to the tree of the author's cb1c865fd — so my merge commit differs from theirs only by identity and timestamp.
Method: both trees staged by each tree's own scripts/compass/snapshot.sh (git archive) + docker cp into xiaobizh_n18_cpu at a path of my own; tarball md5 matched on both ends (f0206cea8… control, 9145e2c4b… merged); diff -r of scripts/compass identical between the two trees; import atom resolved under each root before any figure was read (/tmp/r1r3rev/control/ATOM/atom/__init__.py); both gates printed their own commit: stamp (669dc3f9d (stamp), 0a0df70d4 (stamp)) and gpu: not required, so neither is the 98 a stamp-less tree gives. The two gates ran sequentially, from one script, control first.
On the flake, checked class-wide rather than by method name. grep -c TestTheRegionIsNotCopiedPerChunk is 0 in both logs, and grep -c test_the_cost_per_byte_does_not_grow is 0 in both — so no method of that class is named in either run, not merely the one usually cited. skipped and xfailed are unmoved on both sides and GATE_CPU_RC=0 on both, so neither run hit the ±1 passed/skipped variant nor the hard-failure variant, and neither needed repeating.
ruff check and ruff format --check are clean on all four files on the merged tree (All checks passed; 4 files already formatted). Nothing in atom/compass/runner/ falls under a package-wide glob invariant.
I left /workspace/agent_scratch/r1r2/, /workspace/agent_scratch/r1r3h/ and compass-worktrees/runner-1* untouched; my own worktrees are at /workspace/agent_scratch/r1r3rev/{control,merged}. Everything staged on node 18 is under /tmp/xiaobizh-r1r3rev (host) and /tmp/r1r3rev (container) and is removed; the shared /tmp/xiaobizh-compass/ATOM was never touched.
Effort — all three instruments reproduce at all three heads
Re-derived with my own script over git show of the four files at each head, before reading the author's table:
| head | AST | physical | SLOC | decomposition |
|---|---|---|---|---|
4704d27ec |
33 + 99 = 132 | 180 + 239 = 419 | 50 + 130 = 180 | 136 docstring + 87 blank-outside + 16 comment + 180 code = 419 |
fda25e63d |
33 + 99 = 132 | 189 + 239 = 428 | 50 + 130 = 180 | 145 + 87 + 16 + 180 = 428 |
1a30e6e7f |
33 + 99 = 132 | 186 + 239 = 425 | 50 + 130 = 180 | 142 + 87 + 16 + 180 = 425 |
Both published decompositions reproduce exactly. Round 2 is +9 / 0 / 0; round 3 is −3 / 0 / 0. At the head: SLOC 180 = 1.00x, AST 132 = 0.73x, physical 425 = 2.36x. Both #89 cautions are in the body and correctly marked as the owner's call rather than this branch's.
One note for #89, recorded here rather than there because #89 carries need human and no agent may act on it. #89's headline table still quotes RUNNER-1's round-2 figures — physical 189 / 239 / 428 at 2.38x. Round 3 moved that to 186 / 239 / 425, 2.36x. The number in the decision request is one round stale, which is itself a small instance of the thing it is about. Separately, #89 is now cited by five open PRs — #80, #85, #95, #96, #97 — so the undecided instrument is load-bearing on more than one branch, while this PR, on the instrument that was proposed after its own numbers were in, sits at exactly 1.00x. That is the owner's to weigh and nothing here is blocked on it.
Review record — what the next task in this area should watch
- The class docstring is now the only place in
atom/compass/runner/making a claim about device memory, and nothing in CI reads it —tests/compass/test_runner_non_allocating.pyreferences neither__doc__norforward_vars. F15(a) is the proof that a clause there can drift from its instrument in a single edit with the suite green throughout. The only real guard is review. - The docstring-measurements precedent is a PR-body section today and becomes a commit message on squash. It belongs in
AI_DEV_RULES.md; see above. - Item 3's park has two causes, not one (F16,
async_proc.py:243). - The 80.0 MiB figure's
hf_config.hidden_sizeleg is now verified on CPU-only hardware; only end-to-end construction at a second hidden size remains open. - Accepted with reservation: the end-to-end construction table and the
AsyncIOProcManagerprobe, both quoted from earlier review rounds rather than re-run here — see below.
What I could not check
- Construction itself. This box's ROCm is wedged (
rocminfoin D-state) and I did not take a GPU reservation; every figure in the end-to-end table is round 1's, quoted, as the body says. F15(a) is derived from ATOM's source (model_runner.py:777,:1297,:1358), not from a constructed runner — but those are attribute assignments, not measurements, so reading them is the right instrument. - The F13 probe. Not re-run here either, for the same reason I ruled the author's non-re-run adequate; I verified all nine of its line claims against the merged tree instead, and compass(runner): the RPC surface, every reply shape taken from its caller (RUNNER-2) #90's six-runner run is an independent reproduction of its conclusion.
- The 55,563,006,776-byte checkpoint figure — arithmetic only, unchanged from round 2.
- TP2/TP4 construction, multi-rank NCCL
_setup_device_and_distributed, andcapture_cudagraphagainstUnbuiltModel— unmeasured, and correctly recorded as such in Left undone.
Still a draft. I have not merged, landed, undrafted or pushed anything.
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>
…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>
…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 on its own; this is the PR for issue #76 (RUNNER-1). Do not
merge: draft, pending review.
atom/compass/runner/— aModelRunnersubclass that constructs without owningdevice memory for the weights, the KV tensors or the step, selected through the
runner_qualnameconfig field. Nothing here runs a step.Round 3 (head
1a30e6e7f): one docstring edit (F12) and the rest of thisbody — the F13 provenance correction, the F13 handoff swap, the re-derived gate
table and the effort cautions. No behaviour change. See the round-3 summary
comment for the finding-by-finding record.
Dev record
What is overridden, and why the set differs from the runner already in tree
Five methods, all of them memory-owning or step-running:
_build_and_load_modelself.modelbecomes anUnbuiltModelthat owns no parameter and no buffer and raises if called_maybe_warmupget_num_blocksallocate_kv_cacheTrueforwardRapidServeModelRunner(atom/model_engine/model_runner.py:4168) is the workingnon-allocating runner in the tree, and its override set was taken as given rather
than re-derived. Intersecting its methods with the base's gives seven overrides.
The two it has that this one does not:
__init__. RapidServe needs it because it bindsself.forward = self.prefill_forwardbeforesuper().__init__()runs. This class has nothingto bind, and not defining
__init__is what keeps every read lazy: the base runsthe whole of its own
__init__before a subclass body would get control, sostate set after
super().__init__()is invisible to everything that ran during it._kv_budget_extra_reserve. RapidServe holds bytes back because a secondprocess shares its GPU. A runner that allocates nothing has no tenant to hold
anything back from, and the base already returns 0
(
model_runner.py:866-870, "Base runner reserves nothing")._init_weight_params_on_metais not in that difference because it is not anoverride — it is a helper the base does not have. It is also the mechanism that
briefly creates each parameter on the real device before swapping in a meta
tensor; a runner that constructs no module tree at all needs neither the helper
nor the transient. Building a module tree is needed for weight geometry and for
tracing, but that is capture's and the memory model's, not the attachment point's.
tests/compass/test_runner_non_allocating.pyasserts this comparison againstATOM's own source, so it fails if either side moves, and asserts that all five
names still exist on
ModelRunner— a rename upstream would otherwise turn anoverride into a new method silently.
Round-1 review re-derived the subtraction independently from ATOM's AST rather
than trusting it: 65 base methods, 21 on
RapidServeModelRunner, intersection7, and the difference against this class's five is exactly
{__init__, _kv_budget_extra_reserve}.The injection needs no ATOM change — verified, not assumed
Config.runner_qualname(atom/config.py:1595) is read atengine_core.py:128and resolved atasync_proc.py:166, andLLMEngine.__init__(llm_engine.py:39-43) filters its kwargs byfields(Config), sorunner_qualname=passed to the engine reachesConfig.Run in the GPU container on node 18 against the pushed tree:
That is ATOM's own
resolve_obj_by_qualname, unmodified tree. Round-1 reviewreproduced the same MRO and the same five resolutions on the device.
The CLI-flag gap is still open
runner_qualnamehas aConfigfield and noEngineArgsfield and no CLIflag:
grep -nE 'runner.qualname|runner_qualname' atom/model_engine/arg_utils.pyis empty on this tree and on the merged tree. So the seam is reachable from a
Python caller today and not from a command line. The three-touch recipe still
holds —
_get_engine_kwargsforwards everyEngineArgsfield by name(
arg_utils.py:639-641) andLLMEngine.__init__filters byfields(Config)—but adding the flag changes ATOM's own argument surface, so it is reported here
and not done.
Warmup drives a forward: what that means for a runner with no weights
This was flagged as an open question and it is the thing that shapes the class.
ModelRunner.__init__callsself._maybe_warmup()before returning(
model_runner.py:822on the merged tree);_maybe_warmupcallsself.warmup_model();warmup_modelbuilds a dummyScheduledBatchand callsself.forward(dummy_batch)—model_runner.py:1279.self.forwardis thesubclass's. So a subclass whose
forwardneeds weights, a cost model, oranything set in its own
__init__body cannot construct: the call happens whilethe base's
__init__is still running.There is no workaround here and none was improvised.
_maybe_warmupis already anoverride point in the base, and skipping it is exactly what
RapidServeModelRunnerdoes for its decode process, for the same reason. Overriding
_maybe_warmupistherefore not an optimisation — it is the precondition for constructing at all.
The consequence for successors is the stated one: config must resolve lazily from
self.config, becauseself.configis the only thing the base guarantees is set.The whole chain —
__init__→_maybe_warmup→warmup_model→forward— isasserted over ATOM's source in the tests, so if ATOM ever stops warming from
__init__that becomes a failing test rather than a stale paragraph. It also nowhas direct evidence rather than only the argument: construction completes end to
end with
_maybe_warmupoverridden whileforwardstill refuses — see the nextsection.
Named result: construction with no device allocation
Three measurements, because none alone says it.
On a real device (node 18,
xiaobizh_n18, driver up,torch.cuda.set_device(0),default device set to
cuda:0first, pushed tree imported and printed):across
_build_and_load_model,_maybe_warmupandallocate_kv_cache(2048).The default device is cleared on the way out, as both of ATOM's own
implementations do. Round-1 review reproduced this bit for bit on the same node
against the snapshot — allocated delta 0, reserved delta 0 as well,
allocate_kv_cache -> Truewithconfig.num_kvcache_blocks: 2048, andparams/buffers: [] [].Against what the base allocates, same node, measured as checkpoint bytes,
which is what
_build_and_load_model'sload_modelmakes resident at TP1:Plus warmup, which the base runs from
__init__and this one does not, and theKV tensors, which
allocate_kv_cachedoes not create.Without a device, in the CPU-only tier, by counting dispatched operators
under a
TorchDispatchModerather than reading a CUDA allocator: zero operatorsdispatched across the same three calls, with a control asserting the recorder
does see
torch.empty(4). A dead recorder and a clean runner look identicalotherwise.
End-to-end construction, and what device memory is left
Round 1 of review closed the gap this PR had recorded as unshowable, on
xiaobizh_n18— the same container the checkpoint figures above came from. Themeasurements in the table below are review round 1's, quoted with their
conditions and attributed there, not restated as this branch's;
CompassModelRunner(0, cfg)constructs end to end at TP1 (Qwen3-0.6B,hidden_size=1024, bf16, single process, Gloo rendezvous,enforce_eager=True,load_dummy="empty",gpu_memory_utilization=0.10), so the issue's exitcriterion is demonstrated rather than inferred from five method bodies.
__init__forward_varsgpu bytesNamed CUDA tensors on the runner: 0 bytes both times; default device
cpuonthe way out of
__init__both times.What the residue scales with, decomposed — round 2 got this wrong and round 3
of review caught it. Round 2 reported the 99.1% / 99.4% aggregate and inferred
from it that the residue is "sized by the batch budget and not by the model".
The aggregate does not support that, because
allocate_forward_varsitself wasnever decomposed — and the decomposition is exactly where the model dependence
lives.
ModelRunner.allocate_forward_varsreadshidden_sizeatmodel_runner.py:1288and spends nearly all of the ring on one tensor:UnbuiltModeldefines noextra_output_dims, so that ismax_num_batched_tokens x hidden_size x itemsize, linear inhidden_size.Against the measured totals above, at
hidden_size=1024, bf16:outputsaloneforward_varstotalSo 16.9 MiB is the figure at one hidden size, not at every model. This tree's
own config for the other model named above —
tests/compass/qwen3_5_27b_config.json,text_config.hidden_size = 5120— gives8192 x 5120 x 2 = 83,886,080bytes,80.0 MiB, about 4.7x the figure round 2 set beside it. That number is the
base's own expression evaluated at that config's hidden size plus arithmetic; a
27B
Configwas not built and constructed against, and the JSON carries notop-level
hidden_size— it is nested undertext_config— so the resolutionhf_config.hidden_sizewould perform for a multimodal config is itselfunverified here.
The correct statement, and the one the argument always rested on: the residue is
O(batch budget x hidden size), not O(weights). Its direction is untouched —
Qwen3-0.6B to Qwen3.8-27B is 45x in parameters against about 5x in hidden size,
so the residue stays tiny beside the weights and grows far more slowly than they
do. 16.9 MiB against 1.40 GiB becomes roughly 80 MiB against 51.7 GiB: three
orders of magnitude at the small model, and still nearly three at the large one.
That is the claim the seam's purpose rests on.
The
torch.cuda.Stream,torch.cuda.Event, attention-builder andinitialize_eplb_runtimeterms are inside the__init__totals and are the0.6–0.9% that is not
forward_vars; they are not separately resolved.Why the figures are here and not in the docstring
Round 2 put the byte figures in the class docstring. Round 3 of review ruled
against that, and the ruling is adopted here as the convention for this project:
the docstring states the shape of the claim; the figures live in the PR body
and the design. The reasons are the reviewer's:
because a measurement is the number plus its conditions, its instrument and
its date. The round-2 paragraph carried four conditions and dropped the four
this body keeps — including the one that mattered, that the figures are one
model wide. F12 is what that omission cost.
tests/compass/test_runner_non_allocating.pyreferences neither
__doc__nor any of the byte figures, so the number can rotwhile the suite stays green. This body's gate table went stale twice in
twenty-four hours and was caught both times, because a PR body is read.
does, and carries no pointer to a document for the rest.
measurement and quoting it is fine; the defect was that the docstring could not
carry the attribution that makes it one, so post-merge
model_runner.py:23would have stated
2,168,320bytes with no who, what or when. This body namesround 1 as the source. Moving the figures out resolves it.
Accordingly the class docstring now reads, in full for the residue:
True at any model, any budget and any dtype; no conditions to go stale, and it
is what the code does. Per the same ruling it does not point here for the
numbers.
Why the package is split in two
overrides.pyholds the behaviour and imports nothing from the engine.model_runner.pybinds it ontoModelRunnerand is the only module here thatimports the engine. That is forced, not stylistic: in a driverless container
import atom.model_engine.model_runnerraisesfrom aiter's architecture probe, and
GPU_ARCHS=gfx942does not avoid it —get_gfx_runtimecalls_detect_nativedirectly. Anything reachable from thatimport cannot be exercised by the CPU tier, which is the tier that gates every
task. A test asserts the split, so the behaviour module cannot quietly acquire an
engine import later.
atom.configandatom.model_engine.schedulerdo import in that container,so
Config,ScheduledBatchandScheduledBatchOutputare available to aCPU-only test.
That test reads direct imports (
ast.walkoverImport/ImportFrom), whichis the form the lazy in-function import would take. It does not follow a
transitive chain through a permitted sibling. The transitive guard is a different
mechanism and is recorded for successors below.
Left undone, deliberately
get_num_blocksandforwardrefuse. The block count belongs to a memorymodel this runner has not been given; the step's output has to reproduce what
the scheduler reads back from a real one, and a plausible stub that does not is
worse than no answer. RUNNER-2 — the RPC surface, where a breached contract is a hang #77 and RUNNER-3 — the three forward semantics, each wrong in a way that does not raise #78 replace them. A refusal is delivered
across the worker boundary, but stripped of its reason and as a bare
SystemExit— see item 2 of the handoff list, measured.__init__is nowdemonstrated (above), but only at TP1, single process, Gloo rendezvous,
enforce_eager=True,gpu_memory_utilization=0.10,hidden_size=1024.Round 1 originally recorded the limit as "cannot be shown on CPU-only
hardware", which was honest about the CPU tier and the wrong limit: it is
showable on the GPU node, and it was shown. What remains uncovered is
_setup_device_and_distributedunder a real multi-rank NCCL rendezvous(TP2/TP4 construction is unmeasured),
capture_cudagraph, whichenforce_eager=Trueskipped, and construction at any second hidden size —the 80.0 MiB figure above is derived, not run. Recorded rather than worked
around.
CompassModelRunneritself; see item 6 of the handofflist.
Gates
scripts/compass/gate_cpu.sh, containerxiaobizh_n18_cpuon hjbog-srdc-18,both trees staged by
scripts/compass/snapshot.sh(git archive) +docker cpinto a path of this branch's own — the shared
/tmp/xiaobizh-compass/ATOMwasnot touched — with tarball md5 matched on both ends (
7a260e35a…control,f0dbd8ff6…merged),.compass-commit/.compass-changedwritten by the samerev-parsethat selected the archived tree,import atomresolved under eachroot before reading any figure, and gate scripts byte-identical between the two
trees (
diff -r). The two gates were run sequentially, not concurrently.669dc3f9dGATE_CPU_RC=0669dc3f9d)cb1c865fd, tree16d0e3b19GATE_CPU_RC=0Arithmetic: 4513 − 4496 = 17, which is exactly the count collected from
tests/compass/test_runner_non_allocating.py(15 functions, one parametrisedover two files).
skippedandxfailedare unmoved andfailedis 0 on bothsides. The known CPU-tier flake is three-way —
tests/entrypoints/test_stream_marker_properties.py::TestTheRegionIsNotCopiedPerChunk::test_the_cost_per_byte_does_not_growat nominal 85.7%, a skip variant at 9.5% and a hard failure at 4.8% with
non-zero
GATE_CPU_RC— and neither run hit any of the three: that test did notfail,
GATE_CPU_RC=0on both, and the skipped/xfailed columns did not move, soneither run needed repeating. Both gates printed their own
commit:stampmatching the tree intended, and both printed
gpu: not required, so neither isthe 98 a stamp-less tree gives.
Merged tree.
fork/feature/atomcompass_newmoved to669dc3f9d(SPEC-1,#79) minutes after round 2's gate table was written, which is the second time the
integration head has moved mid-review.
git merge-base --is-ancestor 669dc3f9d 1a30e6e7fis NO, so the merge is still not a fast-forward and the branchfigure is not the merged figure.
git mergeproducedcb1c865fdwith noconflict, parents
1a30e6e7fand669dc3f9d, whose tree16d0e3b19aa7f7a3fda124e5ec8c30286d62fb3dis byte-identical togit merge-tree --write-tree 669dc3f9d 1a30e6e7f. The control's move from 4399to 4496 is #79 adding its own tests, not anything on this branch.
Nothing in
atom/compass/runner/falls under any existing package-wide globinvariant — the three that exist are scoped to
backends/,ir/andclock/.gpu: not required— nothing in the diff matchesgpu_gate_triggers.txt.ruff checkandruff format --checkare clean on all four files, against arepo-wide baseline that is dirty.
Effort
Estimate 180 lines. Measured on the pushed tree
1a30e6e7fwith a script thatreproduces all three instruments in one pass; run against the round-1 head
4704d27ecand the round-2 headfda25e63dit reproduces both of theirpublished readings exactly, including the decompositions.
ast.stmt, docstring expressions excluded4704d27ec(round 1)fda25e63d(round 2)1a30e6e7f(round 3)The three instruments disagree by 293 lines at the head, and the disagreement is
the evidence rather than noise. Both review rounds have now changed only
prose: round 2 moved physical by +9 and round 3 by −3, while AST and
SLOC moved by 0 on both. So on the measure that currently gates the task,
answering a principle-8 finding cost 9 lines of overrun and then answering the
finding against that refunded 3 — and on the measures that describe the code,
neither round happened at all. A measure with that gradient is measuring the
wrong thing.
Two cautions for #89, which is the owner's call and not this branch's. Both
are review round 3's and are carried here as evidence rather than acted on:
Production alone is 50, or 0.28x. Three instruments times two scopes is
six answers, and the brief does not say which scope its 180 means, so the same
task reads anywhere from 0.28x to 2.36x. The scope has to be pre-registered
along with the measure, or the measure decides nothing.
the measure on which this task lands exactly at its estimate. The argument for
it is still the right one — the brief states its estimate in lines, so a line
count is the like-for-like unit, and prose is what this project's conventions
force into the diff. But a measure chosen after seeing which one flatters the
result is worth adopting only if it is fixed before the next task, not
after this one.
One coincidence worth naming so it is not generalised: all four files here are
new, so file lines equal diff insertions and
git diff --shortstat 68ef4f329 1a30e6e7freports exactly 425 insertions. That equality is a property of thistask. On a task that edits existing files the two diverge, and a churn-based rule
counting insertions and deletions would charge round 3's edit 13, not −3.
For RUNNER-2 (#77) and RUNNER-3 (#78)
NonAllocatingRunnerinoverrides.py, not toCompassModelRunner, and they stay testable in the CPU tier. The moment amethod needs an engine import, that tier can no longer see it.
boundary — stripped of its reason, as a bare
SystemExitthat exits theparent with status 0.
busy_loopcatches nothing and the worker does die,its traceback ending at
overrides.py:98; butmonitor_procs()is started atasync_proc.py:353before any RPC, its thread wakes on the sentinel and callsexit()at:492, whose joins are all bounded (:363,:364,:366,:368) and which doesoutputs_queue.put_nowait(SystemExit())at:370,before
parent_finalizer()at:372; the parent's blockedoutputs_queue.get()at:431takes it andcall_funcre-raises at:433.Measured on
xiaobizh_n18against a realAsyncIOProcManager: RAISEDSystemExitafter 6.0–8.0 s, against a dict-returning control thatRETURNED
{'num_kvcache_blocks': 7}in 4.93 s. Two consequences. TheSystemExitcarries empty args —RunnerRefusal's name and sentenceexist only on the worker's stderr, so principle 6's named reason holds
in-process and not across the boundary. And an uncaught
SystemExit()exits the parent with status 0 (
PARENT_EXIT_STATUS=0measured), soanything reading the exit status sees success; in the real engine
engine_core.py:157-162logsload model runner failedfrom itsfinallyfirst, so it is not silent, but it is not a failing status either. Round 1 of
review asserted this became a hang; round 3 ran it and inverted that. The
hang is item 3.
unrecoverable case.
busy_loopskips a name the runner does not have —getattr(runner, func_name, None)atasync_proc.py:237-239— so nothing isever queued, nothing raises, and nothing returns. Measured on the same harness
as item 2: a runner with no
get_num_blocksneither returned nor raised in90 s, and
parent_finalizer()was never called, while the parent sat incall_func("get_num_blocks", wait_out=True)atengine_core.py:132— theblock_info["num_kvcache_blocks"]read at:133never runs. A successor whodeletes an override rather than leaving it refusing, then runs the engine
before a unit test, will read that park as a deadlock in the clock or the RPC
layer.
engine_corecallscapture_cudagraphthe same way, withwait_out=True, unpacking three values (engine_core.py:148-151).capture_cudagraphis not overridden, andConfig.enforce_eagerdefaultsto
False(config.py:1535), soengine_core.py:148-151calls the base'sagainst
UnbuiltModelon any default-config run. The end-to-endconstruction above used
enforce_eager=Trueand does not cover that path;it is unmeasured, not known-good.
get_num_blockscurrently refuses. The base returns a four-key dict —num_kvcache_blocks,pool_entries,pool_entries_per_req,state_runtime— at
model_runner.py:1867-1872, andengine_core.py:133-141reads all four::133and:141by subscript,:139/:140via.get(..., {}).Correction to what round 2 of this body said: that contract did not
grow with M1-1 (compass(backends): the stand-in model's KV geometry, driving ATOM's real block sizing (M1-1) #74).
git diff --stat 68ef4f329 14a197b07 -- atom/model_engine/is empty — compass(backends): the stand-in model's KV geometry, driving ATOM's real block sizing (M1-1) #74 touches 4 files, all underatom/compass/backends/andtests/compass/. It grew upstream:042e8f1c0(feat(kv-cache): content-addressed per-request state, so prefix hits are correct on stateful models ROCm/ATOM#1771, 2026-08-11) added the two pool keys, andc2d40e2dd([DSV4] Add PAGE-backed state checkpoints ROCm/ATOM#1894, 2026-08-15) added
state_runtimeand wrote theRapidServeModelRunnerzero-block form itself. So that form (:4255-4258,returning
num_kvcache_blocksandstate_runtime) postdates the pool keysrather than predating them, and copying it is short, not a breach — the two
keys it omits are exactly the two
engine_core.get-defaults, and both keysthat would
KeyErrorare present. Do not go looking for a contract change in14a197b07; it is not in that diff.forward's decorators are deliberately absent and that is invisible. Thebase's carries
@torch.inference_mode()and@with_eplb_forward_monitor(
model_runner.py:3233-3235;:3232is blank);RapidServeModelRunner'scarries
@torch.inference_mode()(:4268); this one carries neither, which isright while it only raises — a decorator on a refusal is dead weight.
Restoring the body without them gets a silently different execution
context.
forwardis called during construction only if someone removes the_maybe_warmupoverride — do not, and see the warmup section for why.CompassModelRunneritself, and none can. Thesuite drives
NonAllocatingRunnerthrough aRunnerdouble, and thequalname test only asserts the class name appears in the file; binding the
real class needs the engine import, which raises driverless. Round 1 closed
the gap by hand on the device for this commit — MRO
[CompassModelRunner, NonAllocatingRunner, ModelRunner, object], all fiveoverrides resolving to
NonAllocatingRunner.*,__init__and_kv_budget_extra_reserveinherited. That assertion lives in a reviewrecord, not in CI, and it is unguarded from that commit onwards.
Import/ImportFromunderast.walk, which catches the lazy in-functionengine import it exists for, but would miss
overrides.pyimporting apermitted
atom.compass.*sibling that itself imports the engine. The realtransitive guard is that the CPU tier collects this test file in a
driverless container with
atom.compass.runner.overridesimported at modulelevel (line 33), so the whole closure is exercised at collection time. The
consequence: a module that no collected test imports is protected by
neither.
ScheduledBatch/ScheduledBatchOutputimport in the driverless container,so RUNNER-3 — the three forward semantics, each wrong in a way that does not raise #78's semantics can be tested there without a GPU.
🤖 Generated with Claude Code