compass(spec): make four spec pins name the refusal they are about (#237) - #263
Conversation
) Four tests in the spec package passed with the defect they were written around put back, because each accepted a refusal a neighbouring check also produces, or could not see the path at all: - the no-ranks width test now asserts the width's own refusal text; at a negative width the cross-rank length check used to satisfy it with the same rule and the width in its message - the per-condition "earned by a spec" helper now carries, per subject, a piece of text only that condition's refusal writes, asserted on the same refusal as the rule; the probe subject's rule was also earned by the missing-width refusal - the impossible-readings test now asserts it was refused for free memory beyond the card, not for the negative reserve it also carries - a new test records what the probe question asks, so a walk over every width table rather than the ones the question registers is observable while the extra table is one that never refuses Tests only; no production file changes. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| if "was not measured at tensor-parallel width 16" in refusal.what | ||
| ] | ||
| assert len(unmeasured) == len(WIDTH_TABLES) > len(PROBE_TABLES) | ||
| assert asked, "the probe question asked nothing" |
There was a problem hiding this comment.
This review is agent-authored.
Non-blocking (principles 3 and 8): this line is the only reason the test turns red when the width-1 hole in FILLED_BY is filled, and the PR body's reason for accepting that red does not hold.
My ruling on brittleness. The recorder observes a call to a module-level name, which makes this a white-box pin. I accept it. The property is about what the walk asks: _reach registers PROBE_TABLES as what PROBES reads. Under today's FILLED_BY, no refusal can show which tables were walked. That is F4 on the control: 0 failed / 1151 passed. So recording what is asked is the only observation available, and principle 3 does not ask for a more elaborate one.
The claim that fails. The body says that once the hole is filled, "no fixture can observe the walk". Once the hole is filled, PROBE_TABLES is (). A walk over WIDTH_TABLES still asks two questions, and the equality on the next line sees them. I measured this on the merged tree (tip 175739f87 + 00a4d1479, node 18), using line-count-preserving mutants and running tests/compass:
| mutant | result |
|---|---|
this line changed to assert asked or not PROBE_TABLES, ... |
0 failed / 1152 passed |
the same change + _probes walks WIDTH_TABLES |
1 failed: this test, :1743 [('driver_and...d_bytes', 16)] == [('allocator_...d_bytes', 16)] |
the same change + hole filled ((SINGLE_CARD, MULTI_RANK)) |
this test passes. The 5 other reds are the tests that are about the hole. |
the same change + hole filled + _probes walks WIDTH_TABLES |
this test fails at :1743: [('driver_and...d_bytes', 16)] == [] |
| this line as written + hole filled | this test fails at :1742: the probe question asked nothing |
With assert asked or not PROBE_TABLES, the test keeps today's result and keeps catching the wrong walk after the hole is filled. It also stops going red for a change in another module that it has no quarrel with. The case the non-empty check exists for, a refactor that stops calling probe_for, only matters while PROBE_TABLES is non-empty. With PROBE_TABLES empty, "asked nothing" is the correct answer.
The cost of the current form is small. Filling the hole already reddens 4 tests at the tip, and this PR brings that to 6. Still, please either make the one-word change or correct the "still would not notice" paragraph of the body.
| # be refused by the same rule on its own. What is named is the free memory | ||
| # the card cannot hold, so a check that stopped asking that is seen here | ||
| # rather than covered by its neighbour. | ||
| assert "rank 0 reports 400000000000.0 bytes free of 288000000000.0" in ( |
There was a problem hiding this comment.
This review is agent-authored.
Non-blocking (principles 3 and 6): this assertion pins the order of impossible's checks, and the comment above it gives the wrong reason if it ever turns red.
My ruling: the order is an accident, not a contract. DeviceMemory.impossible is a chain of first-match checks. Its docstring ("What about these readings no single card could have produced") states no precedence between free > total and reserved < 0, and nothing else holds the order. I swapped those two branches with a line-count-preserving mutant:
tip 175739f87 |
merged tree | |
|---|---|---|
free > total and reserved < 0 branches swapped |
0 failed / 1151 passed | 1 failed: this test, :1597 ... in 'rank 0 reports -120000000000.0 bytes reserved, which is not a reading of a card (...)' |
So this PR adds a new constraint on the order. The constraint cannot be avoided in this test. The test's premise is a plausible difference (non_torch = total - free - reserved > 0) beside free > total, and that forces reserved < 0. So this fixture always carries both impossibilities, and the only way it can witness the free > total branch is to fix which of the two is named. The branch itself is already witnessed on a fixture that reaches only that branch: test_a_rank_whose_readings_cannot_be_one_card_is_refused_and_named[overrides2-...] is red with the branch removed, both on the tip and on the merged tree.
I accept the order pin, because the body discloses it. The code comment does not, though. Someone who reorders the branches on purpose will read "a check that stopped asking that is seen here" and look for a check that was removed. A cheap fix is to say what is actually held. For example: "Both impossibilities are present, and which one impossible names first is fixed here on purpose; the branch on its own is held by the parametrised test above." The alternative is to drop this assertion and let the review record on #136 name [overrides2], which #237 asks for anyway.
|
This review is agent-authored. It is the first review of PR #263, at head Verdict: APPROVE at Each of the four pins turns red by name with its defect back in, and fails on the assertion that names that defect. The merged tree gates at the tip's count +1, with the delta decomposed by node id. The diff has no design-doc references. There are two non-blocking inline findings ( 1. Each defect reinstated (principle 8)Method. Node 18, The two trees:
These match the developer's table node for node. The only differences are larger absolute counts at the newer tip, and the same deltas: 0→1, 1→2, 2→4 and 1→2. In every run, every failing node is in 2. Brittleness rulings (principles 3 and 8)
3. The "still would not notice" list
4. Importers of
|
| tip | tip + 00a4d1479 |
|
|---|---|---|
175739f87 |
5107 (brief's figure) | merge tree 61f6f9ba2 (def75749d): 5108 passed, 149 skipped, 3 xfailed, GATE_CPU_RC=0 |
77d203b86 (current) |
5111 passed, 149 skipped, 3 xfailed, GATE_CPU_RC=0 |
merge tree baad3fd25 (43f856a41): 5112 passed, 149 skipped, 3 xfailed, GATE_CPU_RC=0 |
At the current tip, the delta by node id from both sides' --junitxml is:
- only on the branch:
tests/compass/test_spec_verbs.py::test_the_probe_question_is_asked_of_the_tables_it_says_it_reads, passed; - only on the control: none;
- outcome changed: none.
gpu: not required on both sides. merge-tree is clean against both tips, and no test name in the merged file is duplicated.
Not in scope, untouched
- F3 (compass(spec): nothing tests that _walk raises the first refusal, and two reached() sites are dead #243,
need human). - The review-record corrections on SPEC-3b - the absolute memory refusal, and the probe table hole that stays a hole #136.
"Refs #237" is right. I have not touched #243, #136 or #224.
Refs #237: the four inert pins. It does not close the issue, because F3 and the review-record corrections are not done here (see the end).
No blocking issues. Tests only: one file,
tests/compass/test_spec_verbs.py. No production file changes.atom/compass/spec/machine.pyandrules.pyare not touched.What changed
Each of the four pins passed with its defect reinstated because it accepted a refusal a neighbouring check also produces, or could not observe the path at all. Each now asserts which refusal fired: its rule and a piece of its
whatonly that refusal writes (never theRuledefinition string, which nothing holds, see #249).test_a_width_with_no_ranks_is_refused_rather_than_reduced"tensor-parallel width {w!r} has no ranks to read a card on"inwhat. The cross-rank length check inranks.pyalso refuses-2withRule.RANK_AGREEMENTand puts-2in its message, which satisfied both old assertions.refused_by_condition()/test_each_condition_in_the_check_set_is_earned_by_a_spec(checked, rule, text), and the test asserts rule and text on the same refusal. The probe subject'sNO_DEFAULTSwas also earned by the missing-width refusal.test_readings_no_card_could_produce_are_refused_though_their_difference_is_not"rank 0 reports 400000000000.0 bytes free of 288000000000.0"inwhatand the remedy's"take them again and find out". That reading also has a negative reserve, the next branch down, which answered when thefree > totalbranch was removed.test_the_probe_question_is_asked_of_the_tables_it_says_it_readsvalidate'sprobe_forin a recorder and asserts the probe question asks exactly[(name, 16) for PROBE_TABLES], non-empty. Today a walk over every width table adds only calls that return a probe, so no refusal can show it; what is asked can. The fixture's precondition (width 16 missing from every width table, so any walk would reach the probe) is asserted, not assumed.Named result: each pin red by name with its defect back in (principle 8)
Instrument:
python -m pytest tests/compass -q -p no:randomly -p no:cacheprovider, node 18xiaobizh_n18_cpu, one staged tree per side, mutants serial, each line-count preserving and restored with a byte-equality check after the run. Control = integration tipe1da5404e(my tests absent, i.e. my fix reverted); branch = head00a4d1479. Counts arefailed / passed. Line numbers are at the head.e1da5404e00a4d1479memory.pyreworded, same length in lines_probes:for path in PROBE_TABLES:→WIDTH_TABLES:test_the_probe_question_is_asked_of_the_tables_it_says_it_reads,:1743assert asked == [...]:[('driver_and...d_bytes', 16)] == [('allocator_...d_bytes', 16)](the extra reserve-table call)memory.py:if tp_width < 1:→< -99999:[0]only, byValueError)test_a_width_with_no_ranks_is_refused_rather_than_reduced[-2],:1624:assert 'tensor-parallel width -2 has no ranks to read a card on' in 'non_torchhas 0 reading(s) for the -2 ranks of this group'.[0]still fails withValueError: max() iterable argument is emptyvalidate:refusals += _probes(...)→+= []test_each_condition_in_the_check_set_is_earned_by_a_spec[whether a probe fills the widths this deployment uses and nobody measured],:909(theany(rule is … and text in what)assertion). Its message lists the one refusal present, the missing-width one. Also the new F4 test,:1742the probe question asked nothing. The two nodes that already bit still do.impossible:free > total→free > 9.9e99 * 1.0test_readings_no_card_could_produce_are_refused_though_their_difference_is_not,:1597:assert 'rank 0 reports 400000000000.0 bytes free of 288000000000.0' in 'rank 0 reports -120000000000.0 bytes reserved, which is not a reading of a card (...)'reserved < 0branch disabled[overrides3-…]node on both. The new F1 assertion does not demand the negative-reserve branch.Every added assertion was seen to fail, and the control column is the "fix reverted" run: with my tests absent, F4 is fully inert and F2/F6/F1 fail only on nodes other than the one each finding names.
The other per-condition texts (run at
addd347dc)spec/and the test file are byte-identical betweenaddd347dcande1da5404e(git diff --statempty on those paths)._widthsoutput removed[STACK]parametrisation already bit on its rule_transfersoutput removed[TRANSFERS]parametrisation already bit on its ruleSo for WIDTHS, STACK and TRANSFERS the new per-condition text adds no discrimination against these removals. It is there so the helper has one shape, and so a subject that also earns a neighbour's rule is caught. For MISSING and DERATES I ran no producer-removal mutant: the survey and
_missingare exercised by dozens of tests, so such a mutant tells you nothing about this parametrisation.Three runs, same pattern. The core battery was run at three tips, because the tip moved twice during the work (#163, #256, #183, #196, then #208, #172, #164; #183 and #196 touch
spec/). The mutated lines are unchanged across all of them. At each tip F4 went 0→1, F2 1→2, F6 2→4 and F1 1→2, on the same nodes:51854571d→0dce57806addd347dc→de5f0c47ae1da5404e→00a4d1479Instrument is sound at this head (checked, not assumed)
The brief says the two
test_spec_*files are the only importers ofatom.compass.spec. At this head there are four:tests/compass/test_kv_simulated_connector.py:41andtests/compass/test_memory_readings.py:50(landed in #164) also importMachineSpec, SpecRefusal. Under every mutant above, neither file had a failing node, and both are intests/compass, sopytest tests/compassis still a complete denominator for anyspec/mutation. (grep -rlE "compass\.spec|from \.+spec|compass import .*spec" --include=*.py atom tests scripts, excluding the package itself.)Gate (principle 7)
scripts/compass/gate_cpu.shfrom each tree's ownscripts/compass, staged bygit archive+docker cpintoxiaobizh_n18_cpuat/tmp/issue237gates-xbz/, tarball md5 matched on all three hops,.compass-commit/.compass-changedwritten from the samerev-parse,atom.__file__asserted under the staged root before any count,timeout -k 10 2400, not piped, one gate at a time. Nothing was written into the shared mount.e1da5404e(tip)00a4d1479commit:stamp printed by the gatee1da5404e (stamp)00a4d1479 (stamp)atom:/tmp/issue237gates-xbz/control/ATOM/atom/__init__.py/tmp/issue237gates-xbz/branch/ATOM/atom/__init__.pyGATE_CPU_RCgpu:Delta +1 passed, 0 skipped, 0 xfailed, 0 failed, compared by node id from each side's
--junitxml:tests/compass/test_spec_verbs.py::test_the_probe_question_is_asked_of_the_tables_it_says_it_reads, passedThe edited tests keep their node ids, so they do not show up in the delta. The flaky class
TestTheRegionIsNotCopiedPerChunkpassed on both sides.The tip moved twice while I worked, so the same pair was gated at each tip, always with the same single-node +1 and
GATE_CPU_RC=0on both sides:51854571d(the brief's figure)0dce57806: 4839 / 149 / 3addd347dc(after #163, #256, #183, #196)de5f0c47a: 4923 / 149 / 3e1da5404e(after #208, #172, #164)00a4d1479: 5045 / 149 / 3The base moved again after the PR was opened (#212, #260). #212 touches
spec/machine.py,spec/rules.pyandtest_spec_schema.py. The only behaviour it changes is the version refusal's rule,SHAPE→VERSION, and none of the texts or rules asserted here involve it. I did not rebase. Instead I gated the merge result against the new tip, building the merge commit withgit merge-tree+commit-tree, so no worktree was touched:815a08679(tip)00a4d1479(8c232cd9b)GATE_CPU_RC=0GATE_CPU_RC=0+ test_the_probe_question_is_asked_of_the_tables_it_says_it_readsonly; nothing removed, no outcome changedAnother agent's
gate_cpu.shwas running in the same container during this pair (both runs took about 3m10s, not 2m35s). The flaky timing class passed on both sides anyway.Size
AST statements / code lines / physical lines. The counter reproduces
spec/= 275 / 495 / 763 at669dc3f9d.e1da5404e00a4d1479atom/compass/spec/tests/compass/test_spec_verbs.pyWhat these assertions still would not notice
validate's module-levelprobe_for. A refactor that stops calling it by that name (inlineFILLED_BYlookup, say) records nothing and the test fails loudly (asked nothing), so the gap does not open silently. What it does not check is a walk that filtersWIDTH_TABLESinline to the tables with a hole: same calls, same result, so there is no defect for it to catch. When the width-1 hole inFILLED_BYis filled,PROBE_TABLESbecomes empty and this test goes red onassert asked. That red is correct, because at that point no fixture can observe the walk, but it will need rewriting then. The per-width skip (if width in resolved[path]) is not exercised here; the audit's m28 measured it held by 9 other nodes.free > totalandreserved < 0, the first one is named. If someone reorders those branches on purpose, this test goes red. The parametrised sibling still holds each branch on a fixture that reaches only that branch.whatis pinned, not the remedy. A guard that refuses under another rule is caught by the existingrule is RANK_AGREEMENT.test_a_width_a_probe_would_fill_is_not_reported_as_a_hole_in_the_probes(a negative needle) is unchanged, but the new F4 test now asserts that the same width-16 fixture was asked of the probe and got an answer, which separates "asked and found nothing" from "never asked" there.test_a_spec_that_carries_the_hand_measured_entry_is_not_refused_for_itasserts onlyok. Its claim is an absence: with the entry present, the walk skips it before any probe is consulted, so from the outside "asked and skipped" looks exactly like "never asked". I did not add an assertion that pretends to hold it.Not in scope
memory.pycan be rewritten false with the suite unchanged. That belongs to compass(spec): nothing tests that _walk raises the first refusal, and two reached() sites are dead #243 (need human, the owner is ruling on whether docstring claims can be held at all). I did not attempt it.Rulemember's definition sentence is held by nothing (compass(spec): runtime_constant's second width-table read can revert to a bare KeyError unnoticed #249). None of the new assertions reads it.machine.pyandrules.pyare untouched, so this does not conflict with the compass(spec): the version refusal asks for a reader, not an edit #212 restack (compass(spec): name which of the two ways a path failed to resolve #183 and compass(spec): one field has one value, whichever way it is spelled #196 have landed and are under this head).Principles: 6 (a refusal names its reason, so a test of a refusal should check that reason, not just that some refusal fired), 7 (the gate delta is decomposed by node id), 8 (each claimed pin carries the mutation that shows it).
🤖 Generated with Claude Code