compass(design): anchor model_runner.py citations to symbols, and test them - #299
Conversation
…t them Every design-doc citation into atom/model_engine/model_runner.py is now written `model_runner.py::Class.method`. It no longer carries a line number, so an edit that moves lines leaves it true. The only numbers left are the capture census rows. Those are the measured conversion lines, so they stay. tests/compass/test_model_runner_citations.py reads the design docs and the source by path. It fails, naming the doc and the citation, in three cases: - a cited symbol that the source does not define; - a line number, in any spelling, with no symbol before it in its paragraph or table row; - a line number outside that symbol's ast span. Closes #288 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| "prepare_sample` | 2564 |", "prepare_sample` | 2468 |" | ||
| ) | ||
| lo, hi = _spans(SOURCE)["ModelRunner.prepare_sample"] | ||
| assert check(docs, SOURCE) == [ |
There was a problem hiding this comment.
Blocking (principle 8; AI_DEV_RULES gate 4, "an inert pin blocks APPROVE"). This test pins only the lower half of the span check, and it pins only a single-number census cell.
I measured this on node 18 (xiaobizh_n18_cpu) with the head tree staged by git archive. Each edit preserves the line count:
| mutant | result |
|---|---|
T7: lo <= int(n) <= hi → lo <= int(n) (upper bound deleted) |
9 passed |
T1: _NUMBERS = r"\d+(?:-\d+)?" (comma lists no longer parsed) |
9 passed |
T1 matters because 5 of the 6 census numbers sit in multi-number cells (2468, 2479, 2481 and 510, 513). The PR body says "this test holds each number inside that symbol's span". Today no test fails if either half of that stops being true. The checker itself is fine: at the head, M2b (2481 → 2564 in the prepare_inputs row) goes red by name with 2564 is outside ModelRunner.prepare_inputs (2445-2546). The problem is that nothing pins it.
Suggested fix (+5 lines, measured). Move one number in each direction:
docs[CENSUS_DOC] = (
docs[CENSUS_DOC]
.replace("prepare_sample` | 2564 |", "prepare_sample` | 2468 |")
.replace("| 2468, 2479, 2481 |", "| 2468, 2479, 2564 |")
)
spans = _spans(SOURCE)
lo, hi = spans["ModelRunner.prepare_inputs"]
assert check(docs, SOURCE) == [
f"{CENSUS_DOC}: 2564 is outside ModelRunner.prepare_inputs ({lo}-{hi})",
f"{CENSUS_DOC}: 2468 is outside ModelRunner.prepare_sample (%d-%d)"
% spans["ModelRunner.prepare_sample"],
]With that change, the fixed file gives 9 passed. Each of the three mutants below then fails ::test_a_line_moved_into_another_function_fails_by_name (1 failed, 8 passed):
- upper bound deleted;
- lower bound deleted;
- number lists not parsed.
There was a problem hiding this comment.
Fixed in bf322e862. I took your version of the test.
- One census number now moves into a later function:
prepare_sample's2564becomes2468. - Another moves into an earlier one, and it comes from the comma-separated cell:
2481becomes2564in theprepare_inputsrow. - The test expects exactly those two failures, by name.
I ran each mutant against a copy of the file (local container, pytest --noconftest):
| mutant | result |
|---|---|
| none (null) | 16 passed |
| T7: upper bound deleted | 1 failed / 15 passed: ::test_a_line_moved_into_another_function_fails_by_name |
| lower bound deleted | 1 failed / 15 passed: the same node id |
| T1: comma lists not parsed | 1 failed / 15 passed: the same node id |
| names = {_last(name) for name in spans} | ||
| for doc, text in docs.items(): | ||
| owner = None | ||
| for segment in re.split(r"\n\s*\n|\n(?=\|)", text): |
There was a problem hiding this comment.
Non-blocking (principle 6: this is a fall-back, not a refusal). A Markdown bullet list is one segment here. So a symbol-less model_runner.py:N in a later bullet inherits an earlier bullet's symbol.
I measured it by appending and `model_runner.py:3300` to 01's "- the RPC boundary — engine_core.py:386-388" bullet. That bullet has no symbol of its own. The ModelRunner.forward symbol two bullets up has a span of 3262-3350, which contains 3300, so the result is 9 passed. This is a wrong green against the brief's rule that "a citation with no symbol is a named failure". The out-of-span variant (:1200) goes red, but it names the wrong symbol: 1200 is outside ModelRunner.forward.
Nothing in the docs triggers it today, because prose now carries no numbers. The fix is one token: split on bullets as well as on table rows, e.g. r"\n\s*\n|\n(?=\||\s*[-*] )". If you keep the current behaviour instead, state it in the limits.
There was a problem hiding this comment.
Fixed in bf322e862, with your split: \n\s*\n|\n(?=\||\s*[-*] ). A bullet is now its own segment. It is pinned by ::test_a_line_with_no_symbol_fails_by_name[later-bullet], whose citation is a bullet holding model_runner.py::ModelRunner.forward followed by a bullet holding model_runner.py:3300. Reverting the split fails exactly that id.
The table-row half of the split (T2) is now pinned too, by [later-row]. Removing it fails exactly that id.
| _FILE = r"[\w-]+\.(?:py|md|json|log|sh|yaml|txt)\b" | ||
| _TOKEN = re.compile( | ||
| rf"model_runner\.py::(?P<symbol>[\w.]+)(?:` \| (?P<cells>{_NUMBERS}) \|)?" | ||
| rf"|model_runner\.py`?(?::|,? lines? )(?P<direct>{_NUMBERS})" |
There was a problem hiding this comment.
Non-blocking (principle 8: states what is and is not caught). These natural "line N" spellings are not recognised. Each was appended to 07 in a paragraph with no symbol, and each gave 9 passed:
`model_runner.py` (line 1234)line 1234 of `model_runner.py``model_runner.py#L1234`
The brief said "any spelling", but no regex covers every spelling. I don't ask for more alternatives. Do add one sentence to the limits in the PR body saying that the recogniser is a fixed list, and naming these forms as out of scope.
There was a problem hiding this comment.
Taken as a stated limit, not code. The PR body's limits now say that the recogniser is a fixed list. They name `model_runner.py` (line N), line N of `model_runner.py` and `model_runner.py#LN` as not recognised. The citations in these docs use none of those spellings today.
| rf"model_runner\.py::(?P<symbol>[\w.]+)(?:` \| (?P<cells>{_NUMBERS}) \|)?" | ||
| rf"|model_runner\.py`?(?::|,? lines? )(?P<direct>{_NUMBERS})" | ||
| rf"|(?P<file>{_FILE})" | ||
| rf"|(?:`(?P<beside>(?!{_FILE})[\w.]+)(?:\(\))?` \()?(?:`|\(|# ):(?P<bare>{_NUMBERS})" |
There was a problem hiding this comment.
Non-blocking (principle 3, and gate 4's "a check counts only once someone has seen it fire"). Three pieces of this parser are pinned by no test. Each was deleted at the head with the line count preserved, and each gave 9 passed:
| mutant | what was deleted |
|---|---|
| T3 | (?:\(\))?, the `name()` (`:N`) form |
| T5 | _FILE narrowed to py, which drops the .md/.json/.log/.sh/.yaml/.txt owner resets |
| T6 | the \( and # bare prefixes, leaving only ` |
The same holds for the row split |\n(?=\|) on line 67 (T2: 9 passed). The (?!{_FILE}) lookahead is pinned: T4 fails [bare-colon].
The PR claims (:N and # :N are recognised, and credits the "revert gives 53 failures" run as evidence. That run was a one-off, not a test.
Either choice is fine:
- pin them: two more
parametrizerows, e.g.("`model_runner.py` (:1234)", ...)and("# :1234", ...)after a`model_runner.py`mention; - cut them: accept a narrower recogniser.
Don't leave them as unpinned claims.
There was a problem hiding this comment.
Pinned or cut in bf322e862. Each of the following fails exactly the named row when removed (local container, pytest --noconftest, one mutant at a time):
| piece | pin | mutant result |
|---|---|---|
T3 (?:\(\))? |
[bare-beside-call], i.e. `warmup_model()` (`:1234`) |
1 failed, that id |
T6 \( prefix |
[bare-paren], i.e. `model_runner.py` (:1234) |
1 failed, that id |
T6 # prefix |
[bare-comment], i.e. `model_runner.py` # :1234 |
1 failed, that id |
| T2 row split | [later-row] |
1 failed, that id |
| T5 owner resets | cut to `py | md, since nothing needed .json/.log/.sh/.yaml/.txt. Pinned by the new ::test_a_bare_line_owned_by_another_file_is_not_checked[engine_core.py]and[README.md]` |
| T4 lookahead | still pinned | 3 failed: [bare-backtick], plus both owned_by_another_file ids |
The null control (the real docs) still passes with the narrower _FILE.
| cudagraph_overhead = self._estimate_cudagraph_overhead() # :1695 | ||
| safety_margin = int(total * 0.02) # :1696 | ||
| budget = int(total * config.gpu_memory_utilization) # :1698 | ||
| # in _read_device_memory, which get_num_blocks calls first |
There was a problem hiding this comment.
Nit (principle 8). get_num_blocks does not call _read_device_memory first. Before that call it runs torch.set_default_device(self.device) and the head_dim back-fill. _read_device_memory is the first memory reading. Suggested wording: "which get_num_blocks calls before any budget arithmetic".
There was a problem hiding this comment.
Fixed in bf322e862. The header now reads # in _read_device_memory, which get_num_blocks calls before any budget arithmetic.
|
Review, cycle 1, of PR #299 (issue #288) at head Verdict: REQUEST_CHANGES at 1. Line-number decision (principle 3)Dropping line numbers from prose is right.
Keeping the census rows is justified. Those numbers are the data, not pointers. I checked all six against In-span drift is honest and acceptable. It is listed as limit 1, and exactness is left to the GPU-tier test, which is the only thing that can measure it. An optional follow-up, not required here: a CPU test could compare the 2. Wrong reds from the beside ruleAcceptable (principle 6). A red that names the token is a refusal with a reason, and the misattribution can only ever produce a red, never a green.
The wrong greens are the real risk, and they are non-blocking. Both are inline:
3. MutationsAll were run on node 18 in
The M2 to M6 reds come from more than one test, because every test compares against the full docs. The node id that answers each row is the null control. The dedicated in-memory tests are:
The fix for the blocking finding, probed. I rewrote the moved test to move one number in each direction (the code is in the inline comment). The fixed file gives 9 passed. Upper bound deleted, lower bound deleted and number lists removed each give 1 failed, 8 passed, all on 4. Spot-checks: 12 rewritten citations (principle 8)Each symbol exists, and each claim is true of it at the head.
One nit, inline on 5. ponytail-review, over the diffnet: -14 lines possible. The blocking fix adds 5, so the file would be about 139 lines, 1.74x the estimate. The size is not a concern: this is a one-shot parser with named failures, and 1.85x is under the 2x line. 6. No design-doc references in codeThe test is clean. A grep for Gate 1: the tree that will land
The delta is +9 passed, which matches the developer's count and the file's 9 node ids. No flaky class fired. Next actionThe developer rewrites |
The moved-citation test now moves one census number into a later function and another into an earlier one, including a number from a comma-separated cell. Removing the upper bound, the lower bound or the list parsing now fails it by name. A bullet is now its own segment, like a table row, so a symbol in one bullet no longer covers a line number in the next. Every other spelling the recogniser accepts is now pinned by a parametrize row: `(:N`, `# :N`, `name()` (`:N`), a later table row and a later bullet. A bare `:N` after another `.py` or `.md` file is ignored. The owner extensions nothing needed are gone (`.json`, `.log`, `.sh`, `.yaml`, `.txt`). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Developer round 2 on PR #299: head
Next: a delta review of 🤖 Generated with Claude Code |
|
Review, cycle 2, of PR #299 (issue #288) at head Verdict: APPROVE at
DeltaTwo files changed. The test goes +56/−49, and
The ponytail shrinks from cycle 1 were applied: the dict-driven parametrize, MutationsI re-ran the battery on node 18, in
Test file:
The developer's local claims reproduce on node 18. Every mutant fails the pin it was meant to fail, and no parser piece I can delete survives. One consequence of cutting ponytail-review (delta, and the file at 155 lines = 1.94x)net: -2 lines possible. Everything else earns its lines. Each Code hygiene. At the head, the test has no design-doc references: a grep for Gate 1: the tree that will land
The delta is +16, which equals the file's 16 node ids. No flaky class fired. Next actionLand |
|
Addendum to the cycle-2 review of PR #299 at head
|
1 participant
Closes #288. Base
feature/atomcompass_new, with no stacking. Headbf322e862(round 2; see "Round 2" at the end). The sections below record round 1 at1a6450cadunless they say otherwise.No blocking issues.
What changed
1. Every
model_runner.pycitation inatom/compass/design/*.mdnow names its symbol in one form:model_runner.py::Class.method.The head has 36
model_runner.py::citations across01,02,03,04,07,12,14and15. They replace the 53 line-number tokens the tip had; the only other edits are rewrapping. Examples:ModelRunner.get_num_blocks()(model_runner.py:1686-1899) becomesmodel_runner.py::ModelRunner.get_num_blocks.03's code block showing the device-memory arithmetic loses its# :1676column. The two functions are still named in the block's own# in ...headers.Mentions of the whole file are not citations into a function, so they stay as they are. These are
04"62torch.cuda.sites inmodel_runner.py" and the file-list mentions in16.2. New test:
tests/compass/test_model_runner_citations.py(155 lines at round 2, 16 node ids). It reads the design docs andatom/model_engine/model_runner.pyby path. Spans come fromast, running from thedefline toend_lineno. It fails, naming the doc and the citation, in three cases:model_runner.py::Xnames something the file does not define. This is the check that keeps symbol-only citations honest.model_runner.pyhas no::symbol before it in the same paragraph or table row. Every spelling in the brief is recognised:model_runner.py:N`.../model_runner.py`:N`model_runner.py` line N`:N`,(:Nor# :Nwhose nearest preceding file name in the doc ismodel_runner.py:Nright after a backticked name thatmodel_runner.pydefines (reviewer m4's spelling)Line-number decision (principle 3)
Prose citations carry no line numbers.
One exception: the capture census rows in
04. These areprepare_inputs2468/2479/2481,prepare_input_ids510/513 andprepare_sample2564.file:linekeys of the conversions thatEXPECTED_HOST_RESOLUTIONSintests/compass/test_capture_real_model.pyasserts.aiter_attention.pyandbackends.pyrows beside them keep their numbers too.model_runner.py::ModelRunner.prepare_inputs,::tokenIDProcessor.prepare_input_ids,::ModelRunner.prepare_sample), and this test holds each number inside that symbol's span.The block in
03headed "Line numbers aremodel_runner.pyat75a265a3f" lost its numbers, so its stamp line is gone too.Named result
Node ids, all in
tests/compass/test_model_runner_citations.py:::test_a_blank_line_above_a_cited_function_stays_greenprepare_inputsreally moved (+1) and the census still checks clean.::test_a_line_moved_into_another_function_fails_by_name['04_model_capture_and_cost_ir.md: 2468 is outside ModelRunner.prepare_sample (2548-2594)']:Nwith no symbol fails by name::test_a_line_with_no_symbol_fails_by_name[bare-colon],[bare-beside-name], plus[colon],[path-outside-backticks],[line-word]"<doc>: '<token>' names no symbol"::test_every_citation_names_a_symbol_and_stays_in_its_span[]::test_a_renamed_function_fails_by_name['04_model_capture_and_cost_ir.md: ModelRunner.prepare_sample is not defined']The same named result, on real files rather than in-memory copies (node 18,
xiaobizh_n18_cpu,pytest <file> -v). Every tree printed itsatom.__file__under its own root.def prepare_inputsinmodel_runner.py04censusprepare_sample2564changed to246804_model_capture_and_cost_ir.md: 2468 is outside ModelRunner.prepare_sample (2548-2594)See `model_runner.py:1234`.appended to0707_calibration_toolchain.md: 'model_runner.py:1234' names no symbol'<token>' names no symbol, including every# :Nin the03block and07's(:1233-1298)These trees were built from
f10c2df86, the same commit before the rebase.git diff f10c2df86 1a6450cad -- atom/compass/design tests/compass/test_model_runner_citations.py atom/model_engine/model_runner.pyis empty.#287 reviewer's mutations, re-run against this check (in-memory;
check()called directly):`warmup_model` (`:1234`)appended to07`model_runner.py` line 1234and`atom/model_engine/model_runner.py`:1234appended to07model_runner.py:1700, docs unchanged(:1191)placed aftermodel_runner.py::ModelRunner.warmup_model1191 is outside ModelRunner.warmup_model (1233-1298)(:1240)`engine_utility.py` (`:126`)placed after amodel_runner.py::citation:126is attributed toengine_utility.py, not tomodel_runner.py.Gate 1: ATOM's suite, unmodified, as a delta
This ran on node 18 in
xiaobizh_n18_cpu, using the tree's ownscripts/compass/gate_cpu.sh:git archiveplus stamps, piped throughdocker exec -i … tar -xinto a private/tmp/i288g2/{control,branch}/ATOM. Nothing was written to the shared mount.52815879…, branchd2f510ef….timeout -k 10 1500, run once and unpiped.atom.__file__GATE_CPU_RC6a83b56bc/tmp/i288g2/control/ATOM/atom/__init__.py6a83b56bc (stamp)1a6450cad/tmp/i288g2/branch/ATOM/atom/__init__.py1a6450cad (stamp)gate_cpu.sh --collect-onlyon both sides, thendiffof the sorted ids): 5303 ids on the control and 5312 on the branch. That is +9 and −0, and the +9 are exactly the nine node ids above. No flaky class moved.git merge-tree --write-tree 6a83b56bc 1a6450cad=43bc197255d68d4356cf88c992963890620db5e1, rc 0. This equals1a6450cad^{tree}.Gate 2
The test is CPU-only. It imports only
ast,re,pathlibandpytest, and it is in the CPU gate's collected set (above). It exercises two things this PR did not add: the existing design docs, andmodel_runner.py.Estimate against actual
The claim estimated about 80 test lines; the actual file is 148 lines, which is 1.85x. That is under the 2x escalation line, so this is not an escalation, but it is close.
idslist and the multi-line asserts accounts for about 15 of them.:Nbeside a named function (m4).What this does not check (limits)
EXPECTED_HOST_RESOLUTIONSintest_capture_real_model.py, which runs only in the GPU tier.:N. It applies when the backticked name also exists as a def name inmodel_runner.py(for exampleforward) and the:Nreally belongs to another file. The failure mode is a false red, never a false green. I checked all 22`name` (`:N`pairs in the docs today, and none collides.model_runner.py::are not citations and are not checked. An example is02's list ofRapidServeModelRunneroverrides. The class they belong to is cited and checked.scheduler.py:N,engine_core.py:Nand the rest have the same staleness problem. That is out of this issue's file set.`model_runner.py` (line N),line N of `model_runner.py`and`model_runner.py#LN`are not recognised. No design doc uses them today.:Nafter a file name is attributed to that file only when the file is.pyor.md. Other extensions (.log,.jsonand so on) do not reset the owner; the null control shows the docs do not need them.Surprises
atom/compass/memory,backends/geometry.pyand their tests). I rebased before the first push and re-ran gate 1 on the rebased trees. The numbers above are from that run.Round 2 (
bf322e862, on top of1a6450cad; no force-push)This round answers review cycle 1 (REQUEST_CHANGES at
1a6450cad).Blocking finding, fixed.
::test_a_line_moved_into_another_function_fails_by_namenow moves two census numbers and expects exactly two failures, by name:prepare_sample's2564becomes2468, moving into an earlier function;2468, 2479, 2481becomes2468, 2479, 2564, moving into a later function.Removing the upper bound, the lower bound or the comma-list parsing each fails that test alone (1 failed, 15 passed).
Non-blocking findings, all taken:
[later-bullet].[bare-paren],[bare-comment],[bare-beside-call]and[later-row]. The owner resets are cut to.pyand.md, and pinned by::test_a_bare_line_owned_by_another_file_is_not_checked[engine_core.py|README.md]._lastis inlined, the parametrize uses a dict, and the docstring is trimmed.03wording is fixed.Mutations (local container,
pytest --noconftest, one mutant per copy of the file). Every mutant fails exactly the pin named in the inline replies. The unmutated file gives 16 passed.Gate 1 on the tree that will land. The tip is
d96027e52, read again after #298 landed.git merge-tree --write-tree d96027e52 bf322e862=771238c5c1a3d56c143fe9a8dacde99738e30937, rc 0.The run was on node 18,
xiaobizh_n18_cpu:git archiveplus stamps into a private/tmp/i288g3;62766ad8…, merged40f085ba…;atom.__file__resolved under each root;gate_cpu.shran undertimeout -k 10 1500, once, unpiped;GATE_CPU_RCd96027e52d96027e52 (stamp)771238c5c771238c5c (stamp)Node-id delta: 5305 → 5321, +16 and −0. All 16 are in
tests/compass/test_model_runner_citations.py.Size. The test file is 155 lines, 1.94x the 80-line estimate. That is under the 2x escalation line; the round-2 additions (+7 net) are the pins the review asked for.
🤖 Generated with Claude Code