compass(spec): the refusal two readings earn, and a width table nobody entered - #146
Conversation
…y entered The two per-rank memory checks were asked rank by rank, so a pair of readings that earns both of them earned whichever one the rank listed first happened to fail. The two name different things to repair -- a reading to take again, against a card to take it on -- so a run could be sent after the neighbour when what was actually wrong is that a rank did not read a card, fix that, and be refused again. Each check is now asked of every rank before the next check is asked, which is what the module already said it did: the refusal is decided by the readings and not by the order they arrived in, and the rank it names is still the first that failed the check that fired. The width tables the probe question can speak for are derived from the probe table while the module is being imported, and the derivation subscripted it. A width-keyed constant written into the schema and not into the probe table -- the ordinary shape of a half-finished change, and the one the test holding the two lists together exists to catch -- therefore raised at import and took the whole package with it, including every caller with no interest in probes. The test failed at collection, as an import error naming the test session rather than the term nobody entered. The derivation now reads the table with a default of no probes: a table with no entry has no hole to report, so it is not a probe table, and the term is named where a caller asks about it, by the refusal this package gives everything else it does not know. The property the derivation exists for is unchanged and is now tested from both sides: give the width that has no probe a probe and the probe tables empty, the question has nothing left to ask about, and there is no second list to edit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Read first, in order: the eight principles in Range. Head Everything below was executed, on both sides, from worktrees of my own; nothing was read out of the body. Verdict: APPROVEBoth defects are real, both fixes do what they claim, and the stronger option on #144 is the right one. Four items are recorded below; one is a correction to the record and none asks for a code change. Numbered so the developer can answer them in a comment rather than a round 2. #144 — option 1, verified, and it holds for all four checksBoth rows of the table reproduce exactly, same two readings, run at the parent and at the head:
Then I went past the pair the issue names. Two loops fixes two checks against each other; the module has four, so I built rank sets that earn three and four of them at once and ran every permutation of each set, comparing the rule, the refusal text and the remedy with the rank index normalised out:
Five order-dependent sets at the parent, none at the head. The two aggregate checks were never order-dependent — The docstring's stated order now matches the code, measured rather than read: The one residue, and it is the acknowledged one. Two ranks that fail the same check differently — say I agree with taking option 1 over option 2, and the developer's argument for it is the right one: a docstring that stopped promising order-independence would have left two readings earning two remedies answered by arrival order, which is the instrument misdirecting a repair. One production statement for that is the correct trade. #145 — the defect was real; I reproduced the parent side before checking the fixThe demonstration was re-run from source, not monkeypatched: At and At and the guarding test fails on its own assertion, with the sentence the issue asked for: Finding 1 — the head-side cell of that table does not reproduce (record correction, no code change)The body's row reads
A width table added to the schema and to nothing else makes every fixture document incomplete, so the refusals are expected and they are the right ones — but they are 36, not 1. I tried the other plausible spelling of the same edit (appending to Nothing in the fix depends on it. The claim the issue asked to see — package imports, The single-source property survives
|
| reverted, alone | measured |
|---|---|
| the two loops back into one | test_which_refusal_a_pair_of_readings_earns_is_not_decided_by_rank_order fails on its assertion — assert 'which is not a reading of a card' in 'rank 0 had 85060000000.0 free against a cache budget of 92100000000.0 ...'; 1 failed, 140 passed |
.get(name, ()) back to the subscript |
test_a_width_table_no_probe_is_named_for_is_refused_and_not_an_import_error fails with KeyError: 'graph_replay_pool_bytes' raised out of validate.py:159 during the module import it performs; 1 failed, 140 passed |
Both figures are the body's, to the digit. Each fix is pinned by a test that fails without it.
Gate 1 — both sides re-measured on node 18
Both trees staged with their own scripts/compass/snapshot.sh, COMPASS_INTEGRATION_REF=fork/feature/atomcompass_new, tarball md5 verified on both ends, docker cp into xiaobizh_n18_cpu at a path of my own — nothing written into the shared mount, staging and worktrees removed afterwards. PYTHONPATH confirmed with import atom under each root before any count was read; each gate's own commit: stamp checked; gates run sequentially, never piped.
control dc2409574 |
branch 80a508dff |
delta | |
|---|---|---|---|
| passed | 4738 | 4741 | +3 |
| failed | 0 | 0 | 0 |
| skipped | 149 | 149 | 0 |
| xfailed | 3 | 3 | 0 |
GATE_CPU_RC |
0 | 0 | — |
| stamp | dc2409574 (stamp) |
80a508dff (stamp) |
— |
| gpu | not required | not required | — |
| wall | 40.18 s | 38.95 s |
Every reported number reproduces. The +3 decomposes by name, not by count: collected ids diffed between the two trees give 3 only on the branch, 0 only on the control, and the three are exactly
tests/compass/test_spec_verbs.py::test_a_probe_given_the_width_that_has_none_empties_the_probe_tables
tests/compass/test_spec_verbs.py::test_a_width_table_no_probe_is_named_for_is_refused_and_not_an_import_error
tests/compass/test_spec_verbs.py::test_which_refusal_a_pair_of_readings_earns_is_not_decided_by_rank_order
gpu: not required is earned rather than asserted: I read .compass-changed (11 files, the whole five-deep stack) against gpu_gate_triggers.txt and no changed path matches a trigger, so the GPU tier is not owed for this diff.
The CPU tier's flaky class, tests/entrypoints/test_stream_marker_properties.py::TestTheRegionIsNotCopiedPerChunk, did not fire on either side — zero failures, skips identical at 149, GATE_CPU_RC=0 both times — so nothing had to be discriminated against it and no re-run was needed.
ruff check and black --check on the three changed files: clean. Longest line at the head: memory.py 86, validate.py 87, test_spec_verbs.py 88. Nothing over 88.
Gate 2 — the three new tests
CPU-only, fixtures throughout, no device, beside the sections they belong to. reimported_validate is the right mechanism and its docstring says why: the derivation runs once, at import, so a test that wants to watch it run has to import the module. It does not register the second instance, so the one the rest of the suite holds is untouched — I confirmed that by running the full file after each of the new tests and getting the baseline counts back.
Finding 2 — the probe-table test pins one of the two widths the body reports (non-blocking, inline)
test_a_probe_given_the_width_that_has_none_empties_the_probe_tables asserts at tp_widths=(16,) only. The body reports both (1,) and (16,), and (1,) is the more interesting of the two — it is the ok=True case, where "asked and found nothing" and "could not ask" are hardest to tell apart. I measured (1,) myself and it holds, so this is coverage, not a defect.
Finding 3 — one sentence in the docstring is wider than the code (non-blocking, inline)
"each check is asked of every rank before the next check is asked" is exactly true of the first two checks and a little loose about the last two, which are asked of the ranks collectively rather than of every rank. The order claim it supports is correct; only the phrasing generalises past what the code does.
The local suite that never finished — investigated, and it is not this branch
The body says the whole tests/compass directory did not finish in the local gpu_docker container (>10 minutes, no output), so local iteration used only the two spec test files. That is real, it is environmental, and it predates this PR by days. What I found:
pytest tests/compasscollects in 3.66 s (785 tests) and runs cleanly until it reachestests/compass/test_capture_collectives.py::test_a_collective_custom_op_is_recorded_and_its_body_never_runs, at 11%. It then stops producing output. My run sat on that one test for the rest of its window.- The container's ROCm driver is wedged.
timeout 30 rocminfodid not return within 120 s — atimeoutcannot kill an uninterruptible process.psinside the container shows 176 processes in D state, 87 of themrocminfo, the oldest at 13 days, beside 3102 zombies. - It is not this branch and not this developer: another agent's run of the same test file on an unrelated worktree has been in D state for 13 h 19 m, and there are stuck
pytest tests/compass/...processes from four other worktrees, up to 10 days old. - On node 18's CPU container the same directory runs to completion in the gate — all 785 compass tests collect and pass there, inside a 39 s whole-suite run.
tests/compassis in neither exclusion list, so the difference is the machine: no device present, so torch never blocks on the wedged one.
Conclusion: a wedged ROCm driver in jgong5_vllm, not a hang in compass/spec-3c-refusal-order. The developer's account is accurate and the decision to measure on node 18 was the right one. Recorded here because it will cost the next agent the same hour: the local container needs a teardown.sh, and until then anything under tests/compass that touches torch device init will hang there with no output and no way to kill it.
Effort — re-measured, reported and not adjudicated
Against dc2409574, AST statements counted as distinct line numbers carrying an ast.stmt, physical counts excluding blanks. Neither issue named an envelope, and which instrument the rule means is an open owner decision; I am not suggesting anything be trimmed.
| claimed | measured | ||
|---|---|---|---|
| AST statements, production | +1 (51→52, 143→143) | +1 (memory.py 51→52, validate.py 143→143) |
✓ |
| AST statements, tests | +42 (704→746) | +42 (704→746) | ✓ |
| physical non-blank, production | +23 / −3 | +23 / −3 | ✓ |
| physical non-blank, tests | +98 / −1 | +98 / −1 | ✓ |
numstat |
— | 14/1, 9/2, 107/1 |
Every effort figure reproduces. One production statement for a defect that made the instrument name the wrong remedy is, for the record, cheap.
What the next task in this area should watch
- The guard between
FILLED_BYand the schema is a test, and it compares leaf names. With.getit is now the only thing standing between a mistyped table key and a probe question that silently has nothing to ask. Two width tables sharing a leaf name under different blocks would pass it. - The rank a refusal names is positional, and so is the reading it quotes. Two ranks failing the same check differently still produce the first one's sentence. Bounded and documented; worth a thought if a refusal ever has to be diffed between runs.
ABSOLUTE_LIMIT's endpoints (below 1.06/above 13.6against measured 1.0621 / 13.5723) are untouched here, correctly — deferred to whoever reads a second machine.ranks.py's bareValueErrorattp_width=0to a direct caller is still there and still SPEC-2's, outside this file set.- The local
gpu_dockercontainer needs a teardown before anyone triestests/compassthere again.
No blocking issues. Finding 1 is a correction to the PR body and asks for no code change; findings 2 and 3 are non-blocking and inline. Nothing to escalate, and no label applied.
| all, whether the cache that rank built was sized by its own budget, the ranks | ||
| against each other, and the one number that survives them. A contaminated run | ||
| can fail more than one, and the one raised is the first in that order. | ||
| can fail more than one, and **each check is asked of every rank before the next |
There was a problem hiding this comment.
Finding 3 (non-blocking): this sentence is a little wider than the code.
"each check is asked of every rank before the next check is asked" is exactly true of the first two checks — and that is the whole of the fix, so the claim it supports is right. It is loose about the last two: the spread and the absolute limit are asked of the ranks collectively, not of every rank, because across_ranks reduces with min/max before either fires.
I checked that this does not weaken the guarantee. Running every permutation of rank sets that earn three and four of the checks at once, the head gives one outcome per set where the parent gave two, and the aggregate pair was never order-dependent to begin with — the spread refusal names no rank at all. So the order the paragraph states is the order the code executes, verified end to end; only the phrase generalises past what the last two checks do.
No change asked for. Recorded so a reader who takes the sentence literally and goes looking for a per-rank loop behind checks three and four knows why there isn't one.
| ) | ||
| under_test = reimported_validate("validate_with_every_width_filled") | ||
| assert under_test.PROBE_TABLES == () | ||
| checked = under_test.validate(merged().document, tp_widths=(16,)) |
There was a problem hiding this comment.
Finding 2 (non-blocking): this pins one of the two widths the PR body reports.
The body states the property at both tp_widths=(1,) and tp_widths=(16,); the test asserts it at (16,) only. (1,) is the more interesting of the two — it is the ok=True case, where "asked the question and found nothing to say" and "could not ask the question" are hardest to tell apart, which is exactly the inversion this derivation exists to keep closed.
I measured it at the head rather than asking for it:
PROBE_TABLES = ()
validate(document, tp_widths=(1,)) -> ok=True PROBES in not_asked: [] in asked_in_part: []
validate(document, tp_widths=(16,)) -> ok=False PROBES in not_asked: [] in asked_in_part: []
Both hold, so this is coverage rather than a defect, and one extra validate(...) line would close it if you happen to be touching the file. Not worth a round 2 on its own.
| #: are held against each other by a test. | ||
| PROBE_TABLES = tuple( | ||
| path for path in WIDTH_TABLES if None in FILLED_BY[path.rsplit(".", 1)[-1]] | ||
| path for path in WIDTH_TABLES if None in FILLED_BY.get(path.rsplit(".", 1)[-1], ()) |
There was a problem hiding this comment.
Accepted, with one measurement for the record — the default swallows a mistyped key as well as a missing one, and the test is what catches it.
The review brief for this PR asked whether .get(name, ()) could weaken the derivation by answering a misspelt FILLED_BY key the same way it answers an absent one. It can, in the derivation itself: with the key mistyped against an otherwise-correct schema path, the head imports cleanly, the table quietly leaves PROBE_TABLES, and validate(document, tp_widths=(1,)) returns ok=True with the probe question silent — where the parent's subscript raised at import.
It does not go unnoticed in the package. Measured at this head, with the key mistyped:
probe_for("allocator_retained_after_load_bytes", 1)
-> REFUSED: `allocator_retained_after_load_bytes` is not one of the constants measured per tensor-parallel width
set(FILLED_BY) == {leaf of p for p in WIDTH_TABLES} -> False (the guard fires)
So both halves of what this comment promises are true: probe_for names the term at the call site, and test_the_probe_table_names_every_constant_the_schema_keys_by_width catches the mistype from either direction — an extra key as readily as a missing one. The comment is accurate as written and I am asking for no change.
Two things for whoever next touches this area, neither introduced here: the guard is a test rather than an instrument in the package, and it compares leaf names rather than full paths, so two width tables sharing a leaf under different blocks would pass it.
Pin re-verification of #146 — not a new review cycleThe existing APPROVE stands. Nothing below changes it. This is the reinstatement Principles referenced: 6 (refuse rather than fall back — what every pin here is Base, and whether #146 is staleNot stale. Determined from the repository and the API, not from the body:
So the head's parent is the base branch's current tip, and the merge-base is that How it was measuredBoth trees staged with their own Baselines reproduce the PR's own figures exactly:
The flaky class Line-drift control: a single null comment inserted above The tableCounts are
Score: 3 pins, 3 bite. Zero inert, zero vacuous, zero names-the-wrong-defect. Two Named failures and the assertions they fired on"It fails" is not a result, so here is each one. A1 — That is the identity assertion, not a bare "something was refused": the pin fires A2 — the ordering partial, and the reason to say the ordering is pinned. Swapping the This is the answer to the question the subject raises. The pin asserts which A3 — the rank a refusal names. 784 of 785 still pass: this pin is the only test in the suite holding the rank index in A4 — the narrowing order across all four checks. Seven tests go red, the pin among plus A5 — what pin A does not notice. Kill the B1 — (Line 151 in the reverted file; the review's B2 — the realistic wrong default. B3 — the one both new pins miss. Force the derivation to produce Neither of the two new tests is in that list. Both are blind to it, and for the shape C1 — the defect pin C exists for. Replace the comprehension with a literal tuple that Only this pin fires. A hard-coded second list passes every other test in the tree, so C2 — the inversion, and the sharpest single result here. Make Only this pin fires. The property "a run that asked and found nothing is not recorded Observation 1 — the probe pins cannot see an always-empty derivation (non-blocking, no code change)Measured at B3 above. Both new probe-table tests stay green when Observation 2 — one helper docstring sentence is held by nothing (non-blocking, no code change)
VerdictThree pins presented, three bite, each on a named assertion, each reproducing the Staging under |
1 participant
Two defects
#136's round-2 review found inatom/compass/spec/, filed as issues#144and#145. They cannot run concurrently -- both touch the same importgraph and the same test file -- so they land together.
Stacked on
compass/spec-3b-memory-refusalsatdc2409574, which is APPROVE atround 2 and unlanded. Base is that branch, not the integration branch.
Issue #145 -- a width table with no probe entry was an import-time
KeyErrorPROBE_TABLESis derived fromFILLED_BYwhilevalidateis being imported,and the derivation subscripted the table. A width-keyed constant written into
the schema and not into
FILLED_BY-- the ordinary shape of a half-finishedchange -- therefore raised while the package was being imported.
The fix is the one the review measured:
FILLED_BY.get(name, ()). A table withno entry has no
Nonein it, so it is not a probe table and contributesnothing; every table that has an entry derives exactly as before.
Measured, with
device.runtime_constants.graph_replay_pool_bytesadded toSCHEMAas aWIDTH_TABLEand to nothing else:dc2409574validateKeyError: 'graph_replay_pool_bytes'WIDTH_TABLEShas 3 entries,PROBE_TABLES1test_spec_verbs.pyalone: 36 failed, 105 passed)assert set(FILLED_BY) == {...},Extra items in the right set: 'graph_replay_pool_bytes'probe_for("graph_replay_pool_bytes", 1)Correction. The head-side cell of that row previously read "collects; 1
failed, 139 passed in the spec file". That figure is wrong and does not
reproduce: it was read off a
-k-filtered run of two tests (1 failed, 1 passed, 139 deselected) and reported as if it were the whole file. Themeasured figures are the ones above, and they were re-measured by the review.
The other 58 failures are the added field itself, not the derivation: the field
is required, so every fixture document in both spec files is now missing it and
is refused for that. The row's point is the one the counts still carry -- the
session collects and the failures are assertions, where before there was no
session at all. The failure that is about this change is the one the next row
quotes, in
test_the_probe_table_names_every_constant_the_schema_keys_by_width.The refusal text is the one the package gives everything else it does not know:
`graph_replay_pool_bytes` is not one of the constants measured per tensor-parallel width: [...].The single-source property still holds.
PROBE_TABLESis still derived fromFILLED_BYand is still what bothreached(PROBES, PROBE_TABLES)and_probesread. Patching
FILLED_BY["allocator_retained_after_load_bytes"]to(SINGLE_CARD, MULTI_RANK)and re-importing the module:The question empties with no second list to edit, and the run still counts it as
asked rather than as one it could not reach. That is now a test, so the property
is pinned rather than re-derived by hand.
Issue #144 -- the checks ran rank-major; option 1, the refusal is now the readings'
I took option 1: the refusal is a function of the readings, not of rank
order. The cost was one statement -- the per-rank loop is now two loops, the
first asking every rank whether its readings describe a card and the second
asking every rank whether free memory was binding. Option 2 would have cost a
sentence and left the instrument with the defect in it: two readings that earn
two different remedies would still be answered by whichever arrived first, and
the docstring would only have stopped promising otherwise. Nothing else in the
module reads rank order, so the traversal was not load-bearing for anything.
Measured, on the review's own case -- one rank that read numbers no card
produced, one whose cache was sized by what was free:
dc2409574[A, B]rank 0 had 85060000000.0 free against a cache budget of ...(free-binding)rank 1 reports 400000000000.0 bytes free of 288000000000.0, which is not a reading of a card[B, A]rank 0 reports 400000000000.0 bytes free ... not a reading of a cardrank 0 reports 400000000000.0 bytes free ... not a reading of a cardSame two readings, same refusal and same remedy either way. The rank the refusal
names is still positional -- it is the first rank that failed the check that
fired -- because which reading came in where is a question about the sequence
this caller passed and has no other answer. The docstring now says that, and the
"first in that order" sentence is true as written: each check is asked of every
rank before the next check is asked.
ranks.py's ownacross_ranksstill raises a bareValueErrorto a directcaller at
tp_width=0. That was ruled SPEC-2's and outside this file set; it isuntouched here and is not what the width refusal in
non_torch_across_rankscovers.
Gates
1. ATOM's suite, as a delta against the stated base. Both sides measured on
node 18 in
xiaobizh_n18_cpu, staged bygit archive+docker cp, each treegated with its own
scripts/compass/,COMPASS_INTEGRATION_REF=fork/feature/atomcompass_new,stamps written from the same
rev-parsethat produced each archive.dc240957480a508dffGATE_CPU_RCThe +3 is exactly the three tests this adds. Both sides report
gpu: not requiredfrom their own.compass-changedstamp -- none of the changed pathsis in
scripts/compass/gpu_gate_triggers.txt, so the GPU tier is not owed forthis diff. Skipped is identical on both sides, so the CPU tier's flaky class
(
tests/entrypoints/test_stream_marker_properties.py::TestTheRegionIsNotCopiedPerChunk,three-way pass/skip/fail) landed on the same outcome on both and did not need a
re-run.
2. New CPU-only tests in
tests/compass/. Three, all intests/compass/test_spec_verbs.py, beside the sections they belong to:test_which_refusal_a_pair_of_readings_earns_is_not_decided_by_rank_order,test_a_width_table_no_probe_is_named_for_is_refused_and_not_an_import_error,and
test_a_probe_given_the_width_that_has_none_empties_the_probe_tables. Thelast two import a second instance of
validateout of the same file under aname of its own, because the derivation under test runs once, at import, and a
test that wants to watch it run has to import the module rather than call
something in it. The instance the rest of the suite holds is untouched.
3. The named result. Both are in the tables above: the same two ranks in
either order producing the same refusal, beside a width table with no
FILLED_BYentry that is refused by name instead of taking the package down atimport.
4. Review. Dispatched by the lead.
Each fix is pinned by a reversion. Putting the old behaviour back, alone:
test_which_refusal_a_pair_of_readings_earns_is_not_decided_by_rank_orderfails on its assertion --assert 'which is not a reading of a card' in 'rank 0 had 85060000000.0 free against a cache budget of ...'; 1 failed, 140 passed.get(name, ())back to the subscripttest_a_width_table_no_probe_is_named_for_is_refused_and_not_an_import_errorfails,KeyError: 'graph_replay_pool_bytes'raised out ofvalidate.py:159during the module import it performs; 1 failed, 140 passedLint on the changed files:
ruff checkclean,black --checkclean, no lineover 88 characters in either module.
Effort
Neither issue names an envelope. What it cost, measured as a delta against
dc2409574, statements counted asast.stmtnodes:memory.py,validate.py)test_spec_verbs.py)The production side is one statement because both fixes are one expression and
one loop header; the rest of the 23 lines is the docstring paragraph that was
false and the comment saying why the loops are separate. Comments were not
trimmed to move either number.
What I could not do
tests/compassdirectory does not finish in the local container(it was still running at 10 minutes with no output). Everything above the gate
line was measured on the two spec test files; the gate is the whole suite on
node 18 and is the number that counts.
below 1.06/above 13.6,measured
1.0621/13.5723) is untouched -- it is a separate finding on aparagraph this change does not rewrite, and the review explicitly deferred it
to the touch that reads a second machine.
Staging trees and tarballs under
agent_scratch/compass_dev/spec3c_gateand/tmp/spec3cgateson node 18 are removed;compass-worktrees/spec-3bisuntouched.
🤖 Generated with Claude Code