Repository navigation
compass(capture): hold the step-axis backstop, the normalisation row and both sentinel bindings - #359
Conversation
…and both sentinel bindings The width-1 refusal test now asserts argparse's exit status 2. Before, it asserted any non-zero status, which the backstop in _step_axis also satisfies, so with main's refusal removed only the message match failed. A new test calls _step_axis at a hint of 1 directly. main never lets that hint through, so until now nothing reached the backstop. The normalisation row's symbol-carrying operators are now an exact set, like gemm and attention. Before, the test only checked that one operator was present, so an operator declared into the family passed. The four operators were measured at TP1 and TP2. A new subprocess test installs the apply_simulated_tp sentinel and calls through both bindings. No capture reaches ATOM's call site, so before this nothing checked which names the sentinel covered. Closes #236 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| assert completed.returncode != 0 | ||
| # 2 is argparse's usage error. Without `main`'s refusal, `_step_axis` | ||
| # still refuses, but as an uncaught AssertionError, which exits 1. | ||
| assert completed.returncode == 2 |
There was a problem hiding this comment.
Non-blocking. Ruling on returncode == 2: keep it. It is sound, but it also holds the mechanism, not only the refusal.
Principle 6: "Refuse rather than fall back. A declined answer with a named reason is a result." Principle 8: "Every claim carries its measurement."
Exit status 2 is a documented stdlib contract, not an accident of the implementation: ArgumentParser.error "terminates the program with a status code of 2". The driver that uses it is main(), in this same file. So the assertion cannot drift under the test; only an edit to this file can move it. Here is what the entry point prints at --tp 1 --step-symbol --width 1, measured on node 18 (xiaobizh_n18_cpu):
| tree | rc | stderr | CAPTURE-RECORD |
time |
|---|---|---|---|---|
head (main refuses) |
2 | usage: … + test_capture_real_model.py: error: --width 1 traces no symbol: …, no Traceback |
0 | 2 s |
M2 (main's check → if False:; only the backstop refuses) |
1 | Traceback … AssertionError: the step axis came back as '1', not a symbol: … |
0 | 12 s |
R1 (parser.error( → sys.exit(; same message, same refusal) |
1 | --width 1 traces no symbol: …, no usage:, no Traceback |
0 | 2 s |
R1 is still a refusal that conforms to principle 6. It declines, names its reason, and prints no record. Even so, it turns this test red: 1 failed / 19 passed, test_the_capture_refuses_a_width_that_torch_would_specialise, :2633, assert 1 == 2. At the tip the same mutation gives 18 passed. So == 2 also pins how main refuses. It is not needed to tell the two refusals apart: "traces no symbol" already does that for M2.
That trade is acceptable, because the mechanism lives in the same file and is documented. If you want a second discriminator that does not depend on the mechanism, use assert "Traceback" not in completed.stderr. It reads 0 at head, 1 under M2 and 0 under R1: a clean refusal as against the backstop's uncaught raise. Either is fine. No change is required.
| " from atom.model_engine import model_runner\n" | ||
| " model_runner.apply_simulated_tp(None)\n" | ||
| " simulated_tp.apply_simulated_tp(None)\n" | ||
| "print('SENTINEL-CALLS', len(calls))\n" |
There was a problem hiding this comment.
Non-blocking: the child never says which atom it imported.
AI_DEV_RULES, setup rule 3: "Every command sets PYTHONPATH to its own worktree and verifies it with python -c "import atom; print(atom.__file__)" before trusting a result." Principle 8: "Every claim carries its measurement."
Measured, it is hermetic by construction. With -c, sys.path[0] is '', which is cwd, which is tree_root, and PYTHONPATH is tree_root as well. The same argv, cwd and env on node 18 printed CHILD_ATOM /tmp/pr359r1/head/ATOM/atom/__init__.py sys.path0= ''. The S1 and S2 mutants' tracebacks also name the staged copy: /tmp/pr359r1/mut/head-S1/ATOM/atom/distributed/simulated_tp.py, line 44.
Still, the capture tests in this file assert record["atom_package"].startswith(tree_root) (:2133), and this child asserts nothing about it. Two cheap changes would close that, and I'd make them only if you touch this again:
- add
import atomto the probe and printatom.__file__besideSENTINEL-CALLS; - assert that the printed path starts with
str(tree_root).
The rest checks out:
- Timeout: bounded, with
timeout=1800, the same as every subprocess in the file. - Failure mode: it fails rather than skips when the import fails. Removing
_declare_archfrom the probe (S5) gives 1 failed / 19 passed,:2242,assert 'SENTINEL-CALLS 2' in [], withRuntimeError: Get GPU arch from rocminfo failedin the message. - Cost: 9.10 s in
--durations, taking the file from 115 s to 124 s.
|
This review is agent-authored. Review, cycle 1: #359 (issue #236), head
|
| # | mutation | tip 6987b0716 |
head 12a2ad6d4 |
test (head) | line, assertion (head) |
|---|---|---|---|---|---|
| null | none | 18 passed | 20 passed | ||
| N0 | comment whitespace (# Both widths. + 2 spaces) |
20 passed | |||
| M2b | _step_axis: if not re.fullmatch(r"s\d+", str(axis)): → if False: |
18 passed (blind) | 1 failed / 19 passed | test_the_step_axis_refuses_a_hint_torch_specialises |
:2653 Failed: DID NOT RAISE <class 'AssertionError'> |
| M2 | main: if args.width < MIN_STEP_WIDTH: → if False: |
1 failed / 17 passed: same test, :2595, 'traces no symbol' in … (string half only) |
1 failed / 19 passed | test_the_capture_refuses_a_width_that_torch_would_specialise |
:2633 assert 1 == 2 (status half, before the string) |
| R1 | main: parser.error( → sys.exit( (same message) |
18 passed | 1 failed / 19 passed | same | :2633 assert 1 == 2. See the inline ruling. |
| M10s | "aten.split_with_sizes" declared into normalisation |
18 passed (blind) | 1 failed / 19 passed | test_the_symbol_reaches_the_work_that_decides_the_cost |
:2700 Extra items in the left set: 'aten.split_with_sizes.default' |
| M10 | "aten.as_strided" declared into normalisation |
1 failed / 17 passed: test_the_census_counts_the_symbols_the_probe_leaves, :2400, Extra items … 'normalisation' |
2 failed / 18 passed | the same probe test (:2437) and test_the_symbol_reaches_… |
:2700 'aten.as_strided.default' |
| M10v | "aten.view" declared into normalisation |
1 failed / 17 passed: test_the_census_counts_an_expression_as_a_symbol, :2433, KeyError: 'view' |
2 failed / 18 passed | that test (:2470) and test_the_symbol_reaches_… |
:2700 'aten.view.default' |
| S1 | model_runner.apply_simulated_tp = sentinel → rebinds itself |
18 passed (no such test) | 1 failed / 19 passed | test_a_call_through_either_binding_reaches_the_sentinel |
:2242. Child <string> line 11 → …/head-S1/ATOM/atom/distributed/simulated_tp.py, line 44, AttributeError: 'NoneType' object has no attribute 'tensor_parallel_size' |
| S2 | simulated_tp.apply_simulated_tp = sentinel → rebinds itself |
18 passed (no such test) | 1 failed / 19 passed | same | :2242, <string> line 12, same AttributeError |
| S3 | sentinel body → raise SystemExit |
18 passed | 1 failed / 19 passed | same | :2242 assert 'SENTINEL-CALLS 2' in [] |
| S4 | sentinel body → pass (records nothing) |
1 failed / 19 passed | same | :2242 assert 'SENTINEL-CALLS 2' in ['SENTINEL-CALLS 0'] |
|
| S5 | probe drops capture._declare_arch(tmpdir) |
1 failed / 19 passed (fails, does not skip) | same | :2242 in [], with RuntimeError: Get GPU arch from rocminfo failed |
Every row the PR claims reproduces, with the same counts, node ids and line numbers. I added four rows: R1, S4, S5 and M2 at the tip. I found no green mutant on the new code. AI_DEV_RULES gate 4 says "A check counts only once someone has seen it fire." All three #236 items have now been seen to fire.
2. The returncode == 2 ruling (principle 6): keep it, non-blocking
The full ruling and the measured table are inline on :2633.
Is it a stable contract? Yes. Exit status 2 is argparse's documented behaviour: ArgumentParser.error "terminates the program with a status code of 2". The driver that uses it is this file's own main.
What each refusal prints, measured:
- head: rc 2,
usage:pluserror: --width 1 traces no symbol…, no traceback, 2 s. - Backstop only (M2): rc 1, an
AssertionErrortraceback, 12 s. - Both: no
CAPTURE-RECORD.
The coupling is real but local. R1 swaps parser.error for sys.exit with the same message. That is still a principle-6 refusal, yet it reddens on assert 1 == 2. The alternative would be a message-based assertion on the argparse error ("usage:" or ": error:"), but it couples to argparse just as much. The mechanism-agnostic option is "Traceback" not in stderr, which reads 0, 1 and 0 across head, M2 and R1. It is optional.
3. The exact normalisation set: correct, and scoped to exactly what is measured
The fixture has one config. It is the vendored, sha256-checked qwen3_5_27b_config.json (the published Qwen3.8-27B). The assertion runs inside for tp in (1, 2) at the default width DECODE_SEQS = 2, and it covers no other model family and no other width. So no other supported model can reach this set.
What could move it. Only a change to ATOM's forward or to aiter's operator names can move it. That is the intended red, the same as for the gemm and attention rows beside it. An undeclared new norm operator would fail first in the unclassified bucket.
Measured on the head tree, at TP1 and TP2, and at widths 2 and 8 (the file's SECOND_WIDTH, which the assertion does not run), all four records give the identical row:
{_fused_qk_rmsnorm_group_quant_kernel: 129, pow.Tensor_Scalar: 48, mean.dim: 48, rsqrt.default: 48}.
The four operators are the whole declared normalisation family in OP_FAMILIES. So the set now says "every declared norm operator carries the symbol", and any extra declaration that carries it reddens (M10s, M10, M10v).
4. The sentinel subprocess test
- Hermetic? Yes, by construction, but not asserted in the child. That is non-blocking and inline on
:2229. MeasuredCHILD_ATOM /tmp/pr359r1/head/ATOM/atom/__init__.py, withsys.path[0] == '', which iscwd, which istree_root. The mutant tracebacks name the stagedsimulated_tp.py. - Bounded? Yes:
timeout=1800, as with every subprocess in the file. - Cost: 9.10 s. The file takes 115 s at the tip and 124 s at head, which is noise on a CPU tier of about 5,300 tests.
- Fails rather than skips? Yes. There is no skip path. S5 and the import failure both end in
assert 'SENTINEL-CALLS 2' in [], with the stderr tail attached. - Why not in-process? Measured: in
xiaobizh_n18_cpu, a bareimport atom.model_engine.model_runnerraisesRuntimeError: Get GPU arch from rocminfo failed. Installing_declare_archor_declare_cudain the pytest process would break the file's own rule that nothing mutatestorch.cudathere. So the subprocess is justified.
5. Truth checks (principle 8: "Every claim carries its measurement")
New and changed comments and docstrings. All five are true as measured:
- "Without
main's refusal,_step_axisstill refuses, but as an uncaught AssertionError, which exits 1". M2: rc 1,AssertionErrortraceback. - "Either refusal satisfies this, so it fails only with both gone". M2: 0
CAPTURE-RECORDlines. - "with the check disabled the call returns a plain
1". Measuredint '1'at hint 1, andSymInt 's26'at hint 2. - "raises on
None.tensor_parallel_size". S1/S2:AttributeErroratsimulated_tp.py:44. - The
_watch_simulated_tpdocstring, "leaves every test that runs a capture passing, and fails the one that calls both bindings directly". S3: 1 failed / 19 passed, and only the sentinel test.
Design-doc references. A grep over the whole file at head for design-doc references (D<n>, T<n>, P0.x, "principle N", "Gate N", doc numbers) returns nothing.
Ruff. ruff format --check and ruff check on the file both give rc 0.
#150 correction (#150 (comment)): accurate.
- The quote "Both halves, because the CLI is not the only entry point" is verbatim from round 2 (
5772255808). - "18 passed" for M2b and "
:2595, string only" for M2 both reproduce at the tip. - The fix description matches head:
DID NOT RAISEat 1 failed / 19 passed, andassert 1 == 2.
#236 correction (5804216613): accurate.
aten.as_stridedintonormalisationis red at the tip, intest_the_census_counts_the_symbols_the_probe_leaves:2400.PROBE_FAMILIESwas introduced by308922c5c(compass(tests): make the capture census discriminate, and assert what it measured (#238) #261). That landed after CAP-2: make the capture symbolic on ATOM's real decode forward #150's53057fc00.aten.split_with_sizesis 18 passed at the tip and red at head.
Effort: +65/−5 against the 15–40 estimate. That is under 2x, so it is not an escalation under the effort rule.
6. ponytail-review over the diff (+65/−5)
I found no delete:, stdlib:, native:, yagni: or shrink: finding. What I weighed:
- The 13-line probe string is the minimum a fresh interpreter needs: load, declare, install, call both bindings, print. Loading through
sys.pathwould save one line and costresolve()symmetry. Folding the one-linelines = …into the assert wraps to three lines under ruff's 88 columns. - The
MIN_STEP_WIDTHcontrol in the_step_axistest is a single self-check, which ponytail excludes by rule. - The exact set replaces a membership line in the same shape as the two rows above it.
Lean already. Ship.
7. Gate: the landing tree, merged and gated at each tip
The tip, re-read twice.
- At the start of this review the tip was
6987b0716: compass(spec): hold each refusal clause that names a machine, fragment, version or term #346 and compass(docs): drop the one time given for two runs at 08:51, and pin 19 of the flake table's 21 runs (#354) #356 above the PR's basebe86326cc. - Just before posting it was
d10cb834f787025cf668ba2962cb5e94ab01373b, which adds compass(spec): hold the probe tables, the reimport helper and the transfer stanzas against their defects #357. That commit touches onlytests/compass/test_spec_verbs.py.
None of the three tip commits touches this PR's file. merge-tree differs from 12a2ad6d4^{tree} (610e72814…) in both cases, so the PR's own gate does not stand for the landing tree. The first tree was gated once; after the tip moved to d10cb834f, the landing tree was gated once as well:
tree that lands at 6987b0716 |
tree that lands at d10cb834f (current) |
|
|---|---|---|
git merge-tree --write-tree <tip> 12a2ad6d4 |
bf26d601b90c9d23f049bfddd8d577a7b2f0c5ae |
0fb5c6749ad478b9957037d3bbe37dc98138dcab |
commit-tree stamp (no ref) |
f29521a63 |
8134adbea |
| passed | 5,265 | 5,265 |
| skipped | 155 | 155 |
| xfailed | 3 | 3 |
| failed | 0 | 0 |
GATE_CPU_RC |
0 PASSED | 0 PASSED |
| gpu | not required | not required |
How both were staged.
git archiveof the stamp commit was piped intoxiaobizh_n18_cpu, at a private path, with the md5 matched on both ends..compass-changed=tests/compass/test_capture_real_model.py.- The gate is the tree's own
scripts/compass/gate_cpu.sh, undertimeout -k 10 7200, unpiped, with one gate at a time and no other load of mine running. - The gate printed
atom: /tmp/pr359r1/merged{,2}/ATOM/atom/__init__.pyandcommit: <stamp>.
The count, decomposed (principle 7: "Never report an aggregate without its decomposition"). 5,265 is the PR's head figure of 5,263 plus 2. That +2 is #346's test_spec_verbs.py: --collect-only gives 153 at the PR's base, and 155 at 6987b0716, at d10cb834f and in both merged trees. #357 changes that file without changing its count. This file collects 18 at the tip and 20 in each merged tree. Skips and xfails are unchanged from the PR's control. No timing-class test failed.
What the next task here should watch
- The sentinel is now proven to cover both names. Still, as the PR says, no capture reaches ATOM's call site, because
_build_runneroverrides it. The source scan intest_apply_simulated_tp_is_called_only_where_the_capture_does_not_goremains the only guard against a second caller. returncode == 2ties the refusal test toparser.error. Ifmain's refusal is ever re-plumbed, change the status assertion in the same commit, or switch to theTraceback-free form.
Staging under /tmp/pr359r1/ in xiaobizh_n18_cpu is removed after this review, and the ControlMaster is closed.
Closes #236
What changed
One file,
tests/compass/test_capture_real_model.py: +65 / −5 test lines, 0 production lines. Head12a2ad6d4, based onbe86326cc._step_axisbackstop now has a test.test_the_step_axis_refuses_a_hint_torch_specialisescalls_step_axis(FakeTensorMode(shape_env=ShapeEnv()), 1)in process, and expects anAssertionErrormatchingnot a symbol.mainrefuses width 1 before the backstop is reached, so until now no test reached it. The same test calls_step_axisatMIN_STEP_WIDTH, which must return ans<n>symbol. That is the control.returncode == 2, argparse's usage-error status, where it used to assert!= 0. Withmain's refusal removed, the backstop exits 1, so the test now fails on the status as well as on the message. A comment in the test says why theRECORD_MARKERhalf cannot tell the two refusals apart: either refusal leaves the record out.normalisationrow is an exact set, likegemmandattention. The four operators were measured on node 18 at the tip, at both TP1 and TP2 with--step-symbol, and are the same at both:aiter._fused_qk_rmsnorm_group_quant_kernel.default(129),aten.mean.dim(48),aten.pow.Tensor_Scalar(48),aten.rsqrt.default(48).apply_simulated_tpsentinel now fires, on both names.test_a_call_through_either_binding_reaches_the_sentinelruns a fresh interpreter with_declare_cudaand_declare_archinstalled. It callsmodel_runner.apply_simulated_tp(None)andsimulated_tp.apply_simulated_tp(None), and requires the lineSENTINEL-CALLS 2. It takes about 8 s on node 18. If a binding is missing, or the sentinel calls through, the real function runs and raises onNone.tensor_parallel_size. It could not run in process: in the CPU container,import atom.model_engine.model_runnerfails on aiter'srocminfoprobe. One sentence in_watch_simulated_tp's docstring is updated to match.Named result: mutation table, node 18 (
xiaobizh_n18_cpu)Every mutation changes one line and keeps the line count. Each ran on its own copy of the staged tree (
git archiveof the commit), over the whole file. Tipbe86326ccis 18 tests and head12a2ad6d4is 20.be86326cc12a2ad6d4node 18's→node 18)_step_axis:if not re.fullmatch(r"s\d+", str(axis)):→if False:test_the_step_axis_refuses_a_hint_torch_specialises,:2653,Failed: DID NOT RAISE <class 'AssertionError'>, the raise halfmain:if args.width < MIN_STEP_WIDTH:→if False:test_the_capture_refuses_…,:2595,'traces no symbol' in …, the string half only:2633,assert 1 == 2on.returncode, the status half, before the string is read"aten.split_with_sizes"declared intonormalisationtest_the_symbol_reaches_the_work_that_decides_the_cost,:2700,Extra items in the left set: 'aten.split_with_sizes.default'"aten.as_strided"declared intonormalisation(the issue's mutation)test_the_census_counts_the_symbols_the_probe_leaves,:2400,Extra items … 'normalisation':2437) andtest_the_symbol_reaches_…(:2700,Extra items … 'aten.as_strided.default')"aten.view"declared intonormalisationtest_the_census_counts_an_expression_as_a_symbol,KeyError: 'view'test_the_symbol_reaches_…(:2700,'aten.view.default')model_runner.apply_simulated_tp = sentinel→ rebinds the originaltest_a_call_through_either_binding_…,:2242, traceback at<string>line 11 →simulated_tp.py:44 logical = config.tensor_parallel_sizesimulated_tp.apply_simulated_tp = sentinel→ rebinds the original<string>line 12raise SystemExitassert 'SENTINEL-CALLS 2' in []One correction to #236. The issue's own mutation,
aten.as_strideddeclared intonormalisation, is already red at the tip. It fails intest_the_census_counts_the_symbols_the_probe_leaves. #261 (#238) made that probe census an exact family set after #236 was measured at70f8cd4db.as_stridedcarries a symbol on the probe pass, so moving it moves a family. The blind spot is still real for an operator that carries the symbol only on the step-symbol pass.aten.split_with_sizes(80 ops) is green at the tip and red at the head, on the new exact set, and it is the row reported for item 2.aten.viewis caught at the tip too, but only by a test that builds a recorder by hand from that operator's name (KeyError: 'view').Gate 1: CPU tier, control measured on the same tip
scripts/compass/gate_cpu.shfrom each tree's own copy, onxiaobizh_n18_cpu. Each tree was staged withgit archiveplus.compass-commit/.compass-changedstamps, andatom.__file__resolved under each staged root. Runs were one at a time, unpiped, undertimeout -k 10 7200.be86326cc12a2ad6d4GATE_CPU_RCtests/compass/)The node-id delta is exactly the two new tests, from
--collect-onlyon the changed file:test_a_call_through_either_binding_reaches_the_sentinelandtest_the_step_axis_refuses_a_hint_torch_specialises. No timing-class test failed on either side.git merge-tree --write-tree be86326cc 12a2ad6d4=610e72814436ea42374c3185a406511faeb668b4=12a2ad6d4^{tree}.ruff format --checkandruff checkpass on the file.Review record
A correction is posted on #150 at #150 (comment): its finding 5 was closed on a basis that was only half fail-able.
Not done
gate_cpu.shreports it as not required, and the diff is test-only._build_runneroverrides that method.🤖 Generated with Claude Code