compass(tests): give the two unguarded own-package globs a non-empty guard - #203
Conversation
…guard `test_backend_interface.py` and `test_runner_non_allocating.py` each parametrise a package-wide `rglob` straight into the decorator, so a package that is renamed or moved collects zero cases. A parametrisation of nothing is a pass, and in `test_backend_interface.py` that is the whole file's verdict: measured, a moved root gives rc=0 with a `got empty parameter set` skip. `test_runner_non_allocating.py` reddens in the same state, but through a sibling that reads a path under the same root and raises `FileNotFoundError`. Its scan is equally inert; it is rescued by who its neighbour is, which is not a property of this test and disappears when that neighbour changes. Both now derive the module list through a helper and assert it non-empty, in the spelling `test_clock_lp_identity.py`, `test_spec_schema.py` and `test_ir_data_model.py` already use. Each guard also carries a control that points the derivation at a root that does not resolve, so the guard is pinned failing rather than only pinned alive. Tests only; no production file is touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
||
|
|
||
| def test_the_package_was_found(): | ||
| assert _backend_modules(), f"no modules under {PACKAGE}" |
There was a problem hiding this comment.
Reproduced, both directions, on my own staging. node 18, xiaobizh_n18_cpu, git archive + docker cp into /tmp/globguardgates, tarball md5 identical on both ends, __pycache__ purged, atom.__file__ printed and confirmed under the staged root before every count (/tmp/globguardwork/<tree>/atom/__init__.py in each block), each rc captured on its own line — never through a pipe (#191).
Renaming atom/compass/backends → cost_backends and updating every atom.compass.backends import, leaving the PACKAGE path constant stale as a real rename does:
control 92f1fdafe |
branch 09ff555e8 |
|
|---|---|---|
test_backend_interface.py |
PYTEST_RC=0 — 4 passed, 1 skipped, SKIPPED ... :91: got empty parameter set for (path) |
PYTEST_RC=1 — 1 failed, 5 passed, 1 skipped; test_the_package_was_found: AssertionError: no modules under /tmp/globguardwork/branch_be/atom/compass/backends, assert [], at :102 |
Baseline untouched: control 10 passed, branch 12 passed — the +2 are this guard and its control.
And the guard has not replaced a real assertion with a liveness check. atom/compass/backends/leaky.py importing atom.model_engine.model_runner, package in place: PYTEST_RC=1, 1 failed, 12 passed — test_the_package_imports_nothing_from_the_engine[leaky.py]: leaky.py:1 imports atom.model_engine.model_runner. Baseline is 12 passed and leaky.py adds one case, so all 12 survivors include this guard: it passes while the assertion it protects does its own job. A module added to an unrelated package (atom/compass/spec/unrelated_new_module.py) leaves both files at PYTEST_RC=0, 12 / 26, unchanged.
The control below is the part that matters, and it is not itself inert — which is worth naming, because nine tests on this board this month passed with their defect reinstated. It has two independent ways to go red and no way to pass vacuously:
- if
monkeypatch.setitem(globals(), ...)ever stopped reaching the derivation — a default argument, an inlinedrglob, a closure over the old value —_backend_modules()returns the real, non-empty list andassert not ...fails; - if the derivation widened past its own root (
PACKAGE.parent.rglob),sibling.pyone level out makes it non-empty and the same assert fails.
monkeypatch.setitem(globals(), ...) is also not a new spelling: tests/plugin/test_sglang_gdn_verify_batched_ssm.py:117 already uses it.
The guard itself is tests/compass/test_spec_schema.py:471-477 verbatim, comment line included. Principle 4, honoured — no second spelling for something the suite already has.
|
|
||
| The import scan walks the package, so a root that does not resolve yields | ||
| nothing and the parametrisation passes on zero cases. The non-empty case is | ||
| asserted on its own, because the neighbouring test that reads a path under the |
There was a problem hiding this comment.
This paragraph's claim is the load-bearing one in the PR, and it is now measured rather than argued. I checked it because "the loud site proves the class is handled" is exactly the reasoning that would have left test_backend_interface.py unfixed.
Same staging as my other comment: node 18 xiaobizh_n18_cpu, git archive + docker cp, md5 verified both ends, __pycache__ purged, atom.__file__ asserted under the staged root before each run, rcs captured separately.
Step 1 — reproduce the two cells. atom/compass/runner → sim_runner, imports updated, PACKAGE left stale:
control 92f1fdafe |
branch 09ff555e8 |
|
|---|---|---|
test_runner_non_allocating.py |
PYTEST_RC=1 — 1 failed, 19 passed, 1 skipped. The only failure is test_the_replaced_methods_are_the_ones_that_own_memory_or_run_a_step, FileNotFoundError: .../atom/compass/runner/overrides.py. The scan itself: SKIPPED ... :313: got empty parameter set for (path) |
PYTEST_RC=1 — 2 failed, 20 passed, 1 skipped: the sibling and test_the_package_was_found at :325, no modules under .../atom/compass/runner |
So on the control the scan is as inert here as it is in the other file. The file is red on someone else's behalf.
Step 2 — the part the PR asserts and did not measure: that the rescue disappears with no diff to this test. I let the neighbour follow the rename the way a real rename carries an import, deriving overrides.py from the imported module instead of from the stale PACKAGE constant:
def _overrides_path():
import atom.compass.sim_runner.overrides as _ov
return pathlib.Path(_ov.__file__)| result | |
|---|---|
control 92f1fdafe, runner renamed, neighbour follows |
PYTEST_RC=0 — 20 passed, 1 skipped, SKIPPED ... :318: got empty parameter set for (path) |
branch 09ff555e8, same edit |
PYTEST_RC=1 — test_the_package_was_found FAILED, 21 passed, 1 skipped |
The whole file goes green on the control with its scan collecting nothing, and the edit that silences it is one a rename would make for its own reasons. That is the argument, confirmed: the protection was an accident of who the sibling is, and only the guard is a statement about the scan. Principle 6 — the empty collection was the fallback, in both files equally.
| @pytest.mark.parametrize( | ||
| "path", | ||
| sorted(PACKAGE.rglob("*.py")), | ||
| _backend_modules(), |
There was a problem hiding this comment.
On the reverted ruff format rewrap — the revert is right, and for a stronger reason than the one given.
The PR reverts the rewrap on the ground that "the formatter baseline is not this PR's to move." Correct, but that phrasing concedes a cost the change does not actually carry. Two measurements:
1. ruff format is not this repository's formatter. .github/workflows/pre-checks.yaml runs two style jobs: psf/black@stable, and a ruff job whose command is ruff check . --output-format=rdjson --exit-zero --no-fix | reviewdog -filter-mode=diff_context. ruff check, not ruff format. The formatter of record is Black.
2. Black and ruff actively disagree on this exact construct, and Black agrees with the tree.
| tree | black --check |
|---|---|
| this branch, as committed | All done! 2 files would be left unchanged. |
| this branch with ruff's rewrap applied | 1 file would be reformatted — and the diff reverts it back to the form on this branch |
(black 26.5.1, ruff 0.16.7, in the jgong5_vllm container.) So taking the rewrap would have put test_backend_interface.py out of conformance with the formatter CI actually runs. The revert is not a concession; it is the only option that keeps the file clean under the gate that exists.
3. And the ruff format baseline is already dirty, identically on both sides, so the file is not a lone hold-out: on 92f1fdafe, ruff format --check tests/ reports 42 files would be reformatted, 168 already formatted, and tests/compass/ alone is 2 of 17. Both counts are unchanged by this branch.
Ruled: leave it reverted. The one thing I would change is the PR body sentence — "the formatter baseline is not this PR's to move" understates it. The accurate statement is that ruff format is not the baseline; Black is, and Black is green. That is principle 8 applied to the PR's own dev record: the reason given is a judgement, and there was a measurement available.
Review of #203 —
|
control 92f1fdafe |
branch 09ff555e8 |
|
|---|---|---|
test_backend_interface.py |
PYTEST_RC=0 — 4 passed, 1 skipped, SKIPPED ... :91: got empty parameter set for (path) |
PYTEST_RC=1 — 1 failed, 5 passed, 1 skipped; no modules under …/atom/compass/backends, assert [], at :102 |
test_runner_non_allocating.py |
PYTEST_RC=1 — 1 failed, 19 passed, 1 skipped; red only via FileNotFoundError: …/atom/compass/runner/overrides.py, own scan SKIPPED ... :313: got empty parameter set |
PYTEST_RC=1 — 2 failed, 20 passed, 1 skipped: the sibling and test_the_package_was_found at :325 |
Baselines untouched: control 10 / 24, branch 12 / 26. The +2 per file are the guard and its control.
The guard-passes-while-the-real-assertion-fires pairing holds at both sites. With the package in place and the invariant broken:
| reinstated defect | result | guard |
|---|---|---|
atom/compass/backends/leaky.py importing atom.model_engine.model_runner |
PYTEST_RC=1, 1 failed, 12 passed — test_the_package_imports_nothing_from_the_engine[leaky.py]: leaky.py:1 imports atom.model_engine.model_runner |
passed |
atom/compass/runner/leaky.py, same import |
PYTEST_RC=1, 1 failed, 26 passed — test_only_the_binding_module_reaches_the_engine[leaky.py] |
passed |
The arithmetic is what makes this conclusive: baseline is 12 / 26 passed, leaky.py adds exactly one parametrised case, so all 12 and all 26 survivors include the new guard. No real assertion was traded for a liveness check. #189's own direction 2 also holds: a module in an unrelated package (atom/compass/spec/unrelated_new_module.py) leaves both files PYTEST_RC=0, 12 / 26, unchanged.
And the control is not itself inert — the failure mode this board has now hit nine times. It has two ways to go red and no way to pass vacuously: if the monkeypatch.setitem(globals(), …) ever stopped reaching the derivation, _<pkg>_modules() returns the real non-empty list and assert not … fails; if the derivation widened past its own root, sibling.py one level out makes it non-empty and the same assert fails. (Correcting my own inline citation: the pre-existing use of that idiom is tests/plugin/test_sglang_gdn_verify_batched_ssm.py:**317**, not :117.)
2. "The loud site is not evidence the class is handled" — checked, and it is stronger than argued
The PR asserts that test_runner_non_allocating.py's redness is borrowed from a neighbour and would vanish with no diff to this test. I measured that rather than accepting it. On the control with runner renamed, I let the neighbour follow the rename the way a real rename carries an import — deriving overrides.py from the imported module instead of from the stale PACKAGE constant:
| result | |
|---|---|
control 92f1fdafe, renamed, neighbour follows |
PYTEST_RC=0 — 20 passed, 1 skipped, SKIPPED ... :318: got empty parameter set for (path) |
branch 09ff555e8, same edit |
PYTEST_RC=1 — test_the_package_was_found FAILED, 21 passed, 1 skipped |
The file goes fully green on the control with its scan collecting nothing, and the edit that silences it is one the rename would make for its own reasons. Both sites needed the guard; only one would ever have been caught. Principle 6 — the empty collection was the fallback, equally in both files.
3. The audit — denominator confirmed, all three instances reproduced, instance 3 is genuinely signal-free
I rebuilt the census from the AST rather than trusting the numbers, over tests/compass/*.py on 92f1fdafe: 17 modules, 50 @pytest.mark.parametrize calls of which 31 pass a literal display and 19 derive their argvalues, plus 11 assert-bearing for loops over a non-literal iterable inside a test body. 61 sites. Exact match, every sub-total.
The three filed in #202, reproduced:
| # | perturbation | measured |
|---|---|---|
1 test_cpu_gate_exclude.py:138 |
gpu_gate_triggers.txt renamed |
PYTEST_RC=0 — 66 passed → 36 passed, 1 skipped; got empty parameter set for (entry) at :138. 30 cases gone, file green |
2 test_spec_schema.py:194/:212 |
DEPLOYMENT_OWNED emptied |
PYTEST_RC=0 — 97 passed → 79 passed, 2 skipped; the skip at both lines |
3 test_ir_data_model.py:691, test_spec_schema.py:509 |
__all__ emptied in atom/compass/ir/__init__.py and atom/compass/spec/__init__.py |
PYTEST_RC=0, and 160 passed / 97 passed — byte-identical to baseline |
Instance 3 is confirmed to have no observable signal at all. Not a weaker signal — none. No skip line, no count change, no report line, identical summary string, identical rc. A for loop over an empty iterable inside a passing test is indistinguishable from the assertion having held, and the two parametrised shapes at least surrender a skip line and 30 or 18 passes. #202's ranking of this as "most invisible, least urgent" is right on both halves.
4. Finding — the "56 cleared" claim is falsified by one site, in the file the audit already filed against
Principle 8. The PR body states: "The remaining 56 sites each have something that states the non-empty case." I spot-checked six of them by emptying the source and measuring, and five hold. One does not.
| site | perturbation | result | cleared? |
|---|---|---|---|
test_cpu_gate_exclude.py:80 (_entries()) |
every entry removed, comments kept | PYTEST_RC=1, 2 failed |
yes — test_the_list_exists_and_is_not_empty |
test_runner_rpc_surface.py:226/:378/:624 |
RPC_SURFACE emptied |
PYTEST_RC=1, 3 failed |
yes — assert len(RPC_SURFACE) == 12 |
test_clock_lp_identity.py:447/:467 |
clock package renamed, imports follow |
PYTEST_RC=1, test_the_package_was_found at :444 |
yes — the landed guard, fires exactly as this PR's copies do |
test_ir_data_model.py:533 |
AMBIENT_READINGS emptied |
PYTEST_RC=1, 3 failed |
yes — a literal-valued sibling, test_an_ambient_reading_is_caught_however_it_is_capitalised[…] |
test_sync_inventory.py:96 (loop) |
sites + anchors emptied |
PYTEST_RC=1, 3 failed |
yes |
test_cpu_gate_exclude.py:122 (_paths(_section(MAN_BEGIN, MAN_END))) |
MANUAL section emptied, both markers kept | PYTEST_RC=0 — 64 passed, 1 skipped, got empty parameter set for (entry) at :122 |
no |
test_every_manual_entry_states_why collapses to zero cases, 2 assertions vanish, and the file reports green. It is the same class, in the same file, as #202's finding 1 — and _section() carries the same explicit return [] that #202 calls "principle 6 read backwards" at :138, twenty lines away. test_both_section_markers_are_present answers the marker-missing branch of that fallback; nothing answers the markers-present-and-section-empty branch, and test_the_list_exists_and_is_not_empty covers the whole list, not this sub-section. The realistic trigger is ordinary: regen_cpu_gate_exclude.sh preserves the MANUAL block, so the way it empties is a person removing the last hand-added entry — after which the next entry added without a stated reason is not caught, which is the thing this test exists to prevent.
An empty MANUAL section is arguably a legitimate end state. So is an empty DEPLOYMENT_OWNED, and #202 filed that one. Either way the clearing criterion was not applied consistently, and the blanket "56" sentence is wrong as written.
This is not a code defect in this PR and it does not block it. What it needs: the PR body sentence corrected to 55 cleared / 6 uncleared, and a fourth instance added to #202. I have posted the measurement there.
5. Gate, size, production zero
Gate 1, each tree's own scripts/compass/gate_cpu.sh (md5 ce05d5d5… identical both sides), .compass-commit / .compass-changed stamps written from the same rev-parse that produced the archive:
| tree | gate's own commit: |
result | GATE_CPU_RC |
|---|---|---|---|
| control | 92f1fdafe (stamp) |
4557 passed, 149 skipped, 3 xfailed | 0 |
| branch | 09ff555e8 (stamp) |
4561 passed, 149 skipped, 3 xfailed | 0 |
+4 passed, 0 failed, 149 skipped and 3 xfailed conserved. The stated numbers reproduce on both sides. Both ran 29 files excluded + tests/plugin. The TestTheRegionIsNotCopiedPerChunk three-way flake did not arise.
Size, counted myself after ruff format, statements = every ast.stmt with docstring Expr nodes included:
| file | statements | physical |
|---|---|---|
test_backend_interface.py |
45 → 54 (+9) | 102 → 130 (+28) |
test_runner_non_allocating.py |
134 → 143 (+9) | 326 → 355 (+29) |
| total | +18 | +57 |
Identical before and after ruff format — the formatter changes no statement count here. Counter calibrated against the landed reference: my script reports atom/compass/spec = 275 statements / 763 physical, matching the published 275 / … / 763. So +18 is the developer's number, independently obtained, and it is genuinely just under the 20-50 band. I accept the stated reason: the alternative to 18 was inventing a second spelling of a guard the suite already has, which principles 3 and 4 both forbid. Padding an estimate is not a reason to do it.
Production 0 / 0 / 0, with the reason rather than as a bare zero. git diff --name-status 92f1fdafe..09ff555e8 returns exactly M tests/compass/test_backend_interface.py and M tests/compass/test_runner_non_allocating.py. Nothing under atom/ or scripts/ is touched, because the defect is in what two tests assert, not in what they assert about. Principle 7 — the zero is decomposed, not asserted.
6. The formatter rewrap — ruled: leave it reverted
Detail in the inline comment. Short form: .github/workflows/pre-checks.yaml runs psf/black@stable plus ruff **check** (not ruff format), so Black is the formatter of record. black --check on this branch as committed: "2 files would be left unchanged." Apply ruff's rewrap and black --check reports "1 file would be reformatted", with a diff that reverts it back. The two formatters disagree on exactly this construct, and taking ruff's version would have put the file out of conformance with the gate that exists. Separately, the ruff format baseline for tests/ is already dirty and identically so on both sides — 42 of 210 files repo-wide, 2 of 17 in tests/compass/.
So the revert costs nothing and the developer's stated ground understates its own case. The only correction I would make is to the wording: not "the baseline is not mine to move" but "ruff format is not the baseline."
7. Rebase against #188 — verified
#188 (compass/runner-docstring-truth, 52439ae55) touches tests/compass/test_runner_non_allocating.py in exactly one hunk, @@ -168,6 +168,107 @@, anchored on test_calling_that_model_refuses_and_names_the_class_it_stands_for. #203's two hunks in that file are @@ -18,6 +18,12 @@ (module docstring) and @@ -310,9 +316,32 @@. Disjoint including context. A rebase either way applies cleanly.
8. Scope and the rest
Site #2 (_call_sites at test_runner_rpc_surface.py:117) is untouched, as #190 ruled — confirmed, the file is not in the diff. Gate 2: the four new tests read paths and parse text; no device, no import of anything that needs one — principle 2 intact. Filing the rest as #202 instead of widening is principle 5, and is what #189 asked for.
APPROVE. The two required corrections are to the record, not the branch: the "56" sentence in the PR body, and a fourth instance in #202.
Staging cleaned up: /tmp/globguardgates, /tmp/globguardwork, /tmp/globguardaudit removed from xiaobizh_n18_cpu; the shared /tmp/xiaobizh-compass/ATOM mount was never written to.
Pin re-verification of #203 — not a new review cycleThe existing All four pins bite. Zero inert, zero vacuous, zero naming the wrong defect. Three State, and the base determined two ways
MethodNode 18, Baselines reproduced: branch The table — every pin, its defect reinstated
Two further reinstatements from the PR body, re-measured rather than read: a Half-reverts — the partial, not only the total
Line-drift controlA null comment inserted at line 2 of both files, shifting every line below it: What the pins would not notice — measured, all three inherited
Depth, since this is the shape that bit #218 and #227. Both guarded globs are This is not a #203 defect. The same helper-plus- Verdict
Staging removed from |
…ons in test_cpu_gate_exclude.py (#210) Addresses **#202**, instances **1** (`test_cpu_gate_exclude.py:138`) and **4** (`:122`) — the two that live in `tests/compass/test_cpu_gate_exclude.py`. Instances 2 and 3 stay open on #202: they are in `test_spec_schema.py` and `test_ir_data_model.py`, which #183, #194 and #196 are editing. I said so on #202 before starting; no file other than `tests/compass/test_cpu_gate_exclude.py` is touched here. Both sites take a parametrisation's argvalues from a file rather than from a literal. A parametrisation of nothing is a pass, so each collects zero cases and the file reports green the moment what it reads goes away. Design principles this turns on: **6** (refuse rather than fall back — the empty collection *is* the fallback), **8** (every direction below is measured, including the one that says the pin costs something), **7** (the vanished-case counts are decomposed, because two of the four assertions that vanish move no count at all), and **3** for what was deliberately not added. ## The two guards are not the same assertion, and that is the finding `:122` was spot-checked and cleared by one review before another found it. The reason it cleared is the reason the two guards differ: **the MANUAL section is *allowed* to be empty, which is not the same as being *asserted* non-empty.** Applying one criterion to both sites is what produced the miss, so they get two. | site | what it derives from | what the guard asserts | why not the other one | |---|---|---|---| | `:138` | `scripts/compass/gpu_gate_triggers.txt` | `test_the_trigger_list_exists_and_is_not_empty` — the file **is a file**, and it **names at least one path** | There is no tree in which this file is legitimately absent or legitimately silent. It records what the CPU tier cannot see; empty, `gate_cpu.sh` answers `gpu: not required` for every diff. A refusal is the whole of the right answer. | | `:122` | the MANUAL block of `scripts/compass/cpu_gate_exclude.txt` | `test_the_manual_section_holds_the_entries_it_is_pinned_to_hold` — `len(...) == 1`, a **pinned count** | `assert entries` would be wrong here: a tree in which every driver failure is visible to the generator has no hand-added entries, and that is a legitimate end state. Zero stays reachable — it costs an edit to the pin, which is the declaration the file cannot make for itself. The count is what tells an emptiness somebody chose from one that arrived. | The generated section next door already says `assert entries` and is right to: its emptiness is not a legitimate end state either. The asymmetry inside this one file is the point. **The asymmetry is written down in the tree, not only in the behaviour.** Both halves of it are stated in `scripts/compass/README.md`, and the review surfaced the citations this body did not make: - **line 18**, `regen_gpu_gate_triggers.sh`: *"Writes the whole file, **header counts included**; nothing in it is maintained by hand."* A generated file that names nothing is not a state anybody chose, so absent and empty have no legitimate reading and a refusal is the whole of the right answer. - **line 17**, `regen_cpu_gate_exclude.sh`: *"Preserves the MANUAL section verbatim."* Hand-maintained by construction, so zero **is** a legitimate end state, and `assert entries` there would encode a claim about ATOM's suite that nothing supports. That is a stronger support than the behavioural argument above: the two files differ in who writes them, and the README says so. ### `:122` drops more than the two cases #202 counted `_manual_entries()` feeds three assertions, and only one of them is visible when it empties: | reader | what emptying does to it | visible? | |---|---|---| | `test_every_manual_entry_states_why` (`:122`) | 1 case → 0 | yes — one `got empty parameter set` skip | | `test_every_excluded_path_still_exists` | 29 cases → 28 | yes — the pass count drops | | `test_the_manual_section_is_sorted_and_unique` | `[] == sorted(set([]))` | **no** — passes, no count change, no skip line | | `test_no_path_appears_in_both_sections` | `gen & set()` is empty | **no** — same | So the measured `66 → 64 passed, 1 skipped` is the *visible* half. Two more assertions go vacuous with byte-identical output — #202's finding-3 shape, living inside its instance 4. **What the pin does and does not do for those four rows** — stated narrowly, because the table invites a stronger reading than its rows support. One pin over the shared derivation makes **every emptying of it announced**, which is the failure mode #202 named and the whole of what "covers all four" claims here. It does **not** make the two silent readers load-bearing. At the pinned count of `1`, `test_the_manual_section_is_sorted_and_unique` is still trivially true — a one-element list always equals `sorted(set(...))` — so that assertion cannot fail at any count below 2, before this PR or after it. Restored: non-emptiness. Not restored: signal in the two rows marked **no**. ## Reusing the spelling `test_the_trigger_list_exists_and_is_not_empty` is this file's **own** existing spelling — `test_the_list_exists_and_is_not_empty` at `:63`, `assert <path>.is_file()` then `assert <derivation>()` — pointed at the second file instead of the first, rather than #203's `test_the_package_was_found`, which is the same shape for a *package*. Both derivations are routed through a helper (`_trigger_entries`, `_manual_entries`) so the guard and the parametrisation read the same thing. **The `if TRIGGERS.is_file() else []` is kept, moved into the helper, and #202's item 1 says to drop it.** The reasoning for keeping it is this file's own, already written above `_section()`: a derivation that returns `[]` lets one dedicated test report the failure by name instead of every test in the module erroring at collection with a `FileNotFoundError` inside `_lines`. That expression was a defect because it stood where a refusal belonged *and no refusal existed*; with the refusal present it is a deferral to it, and the measurement below is what settles it — the branch now reports `test_the_trigger_list_exists_and_is_not_empty` with the resolved path in the message, which a collection-time traceback would not. ## Named result — both directions, per site Measured in `xiaobizh_n18_cpu` on node 18, staged by `git archive` + `docker cp` (tarball md5 verified identical on both ends), `__pycache__` purged, `atom.__file__` confirmed under the staged root before every run, each rc captured on its own line and never through a pipe. Baselines, `tests/compass/test_cpu_gate_exclude.py` alone: **control `92f1fdafe` — 66 passed, `PYTEST_RC=0`; this branch `82e465703` — 70 passed, `PYTEST_RC=0`** (+4 = two guards and their two controls). ### 1 — the guard fires when the collection silently empties | mutation | control `92f1fdafe` | this branch `82e465703` | |---|---|---| | `gpu_gate_triggers.txt` renamed | **`PYTEST_RC=0`** — 36 passed, 1 skipped; `got empty parameter set for (entry)` at `:138`. **30 cases vanish, green.** | **`PYTEST_RC=1`** — 1 failed, 39 passed, 1 skipped. `test_the_trigger_list_exists_and_is_not_empty`: `.../scripts/compass/gpu_gate_triggers.txt is missing; regenerate with scripts/compass/regen_gpu_gate_triggers.sh` | | MANUAL section emptied, both markers kept | **`PYTEST_RC=0`** — 64 passed, 1 skipped; the same skip at `:122`. **2 cases vanish, green.** | **`PYTEST_RC=1`** — 1 failed, 67 passed, 1 skipped. `test_the_manual_section_holds_the_entries_it_is_pinned_to_hold`: `the manual section holds [], not the 1 entry pinned here`, `assert 0 == 1` | Both control cells reproduce #202's measurements exactly. ### 2 — the test it guards still fails for its original reason when that reason is reinstated | reinstated defect | this branch | control | |---|---|---| | a trigger naming a module that does not exist — `atom/model_ops/this_module_was_renamed.py` appended to `gpu_gate_triggers.txt` | `PYTEST_RC=1`, 1 failed 70 passed — `test_every_trigger_path_still_exists[atom/model_ops/this_module_was_renamed.py]`. The guard **passed**. | — | | the reason comment stripped from the one MANUAL entry, count unchanged | `PYTEST_RC=1`, 1 failed 69 passed — `test_every_manual_entry_states_why[tests/test_lmcache_offload_disk_integration.py]`: `has no comment above it`. The pin **passed**. | `PYTEST_RC=1`, 1 failed 65 passed — same test, same message, so the assertion being preserved is the tree's, not one this PR invented | So neither real assertion was replaced by a liveness check: with the file present and the count right, the parametrised assertions still do their own job and both guards are silent. **It does not fail for a reason nobody intended.** A module added to an unrelated Compass package (`atom/compass/spec/unrelated_new_module.py`): `PYTEST_RC=0`, 70 passed, unchanged. **What the exact count costs, measured rather than argued.** A *second* MANUAL entry added with a stated reason (`tests/test_mla_index_cache.py`): `PYTEST_RC=1`, 1 failed 71 passed — the pin fires on the addition too, `assert 2 == 1`. That is the price of an exact count over a `>= 1` floor, and it is deliberate: the section exists precisely because its contents cannot be derived, so its size cannot be derived either, and every change to it is a hand-made assertion that is supposed to cost a second deliberate edit. `len(RPC_SURFACE) == 12` in `test_runner_rpc_surface.py` is the same trade, already taken in this suite. ### The control for each guard Each guard carries the pair #203 added: a per-site control that points the derivation at something that does not resolve, so the guard is pinned **failing**, not only pinned alive. `test_the_trigger_guard_finds_nothing_when_the_file_moves` points `TRIGGERS` at a path that does not exist with the real file one directory up; `test_the_manual_guard_finds_nothing_when_the_section_empties` points `EXCLUDE` at a list whose markers are both present, whose MANUAL block is empty — the exact shape `regen_cpu_gate_exclude.sh` leaves behind, since it copies the block verbatim — and which parks an entry *outside* the markers, so a derivation that read the whole file instead of its own section is caught there. ## Is either site's redness borrowed? Both. Differently. #203 asked this of its two sites and found one was being carried by a neighbour. Asked here: ### `:138` — entirely borrowed, and it evaporates when the rename finishes | tree | `GATE_CPU_RC` | result | |---|---|---| | control, `gpu_gate_triggers.txt` renamed, nothing else touched | **94** | `FATAL: cannot read .../gpu_gate_triggers.txt` — `gate_cpu.sh`'s own `[ -r "$TRIGGERS" ]`. Not a test. | | control, the same rename **followed through** into `gate_cpu.sh`, `gate_gpu.sh`, `regen_gpu_gate_triggers.sh` and `scripts/compass/README.md` | **0** | **4527 passed, 150 skipped, 3 xfailed** — the whole CPU gate green, 30 cases gone, one extra skip | | this branch, same followed-through rename | **1** | 1 failed, 4530 passed, 150 skipped, 3 xfailed — `test_the_trigger_list_exists_and_is_not_empty` | Following the rename through is what a real rename does: every reference that fails loudly gets fixed. After it, the only surviving `gpu_gate_triggers.txt` string under `tests/` is the test's own path constant — which does not fail, so nobody fixes it. The rescue was entirely the gate script's, and it lasts exactly as long as somebody's half-finished rename. ### `:122` — borrowed, conditional on *which* entry is removed, and it does not terminate Emptying the MANUAL section takes the gate from 29 exclusions to 28, so `tests/test_lmcache_offload_disk_integration.py` runs: | tree | pytest summary | `GATE_CPU_RC` | |---|---|---| | control, MANUAL emptied | **2 failed, 4555 passed**, 150 skipped, 3 xfailed in 44.24 s — the two lmcache disk tests | **never printed** | | this branch, MANUAL emptied | **3 failed, 4558 passed**, 150 skipped, 3 xfailed in 41.73 s — the pin plus the same two | **never printed** | `GATE_CPU_RC` is never printed because **the pytest process does not exit** after that file runs in the driverless container: it prints its summary and hangs. Three other tenants' pytest processes were sitting in exactly that state on the same container, `etime` 2d12h, one of them running that file alone. My two runs were bounded (`timeout -k 10 200`, wrapper `124`) and cleaned up. So where this site's redness exists, it arrives as a hang rather than a verdict. And it exists **only because today's single manual entry happens to name a test that really does need a driver**. The reason for removing a manual entry is usually the opposite: the test was fixed, or ATOM deleted it — in which case the gate excludes one file fewer, nothing runs that fails, nothing reddens, and the two cases plus two vacuous assertions go quietly. Neither site would reliably have been caught. ## Gates **Gate 1 — ATOM's suite unmodified.** Each tree's **own** `scripts/compass/gate_cpu.sh`, never overlaid; nothing under `scripts/compass/` is committed here, which is what keeps "one instrument" true on both sides. Staged by `git archive` + `docker cp` into `xiaobizh_n18_cpu`, both stamps written from the same `rev-parse`, tarball md5 matched on both ends. | tree | gate `commit:` | result | `GATE_CPU_RC` | |---|---|---|---| | control `feature/atomcompass_new` | `92f1fdafe` | **4557 passed**, 149 skipped, 3 xfailed | **0** | | this branch | `82e465703` | **4561 passed**, 149 skipped, 3 xfailed | **0** | Delta **+4 passed, 0 failed**, skips and xfails unchanged — the two guards and their two controls. The control reproduces the stated 4557. No failure on either side, so the `TestTheRegionIsNotCopiedPerChunk` three-way flake did not arise and there was nothing to re-run. **Gate 2 — new CPU-only tests.** Four, all in `tests/compass/`. They read two text files and monkeypatch two module constants; no device, no import of anything that reaches one. **Gate 3 — the named result.** Above: both directions, per site, with counts, plus the unintended-reason control and the pin's measured churn cost. ## Effort Counted after `ruff format`; statements = every `ast.stmt` node, docstring `Expr` nodes included. The counter is calibrated by reproducing #203's six numbers exactly (45→54 / 62→70 / 102→130 and 134→143 / 162→170 / 326→355). | file | statements | code | physical | |---|---|---|---| | `tests/compass/test_cpu_gate_exclude.py` | 53 → 75 (**+22**) | 66 → 97 (+31) | 149 → 247 (+98) | | **test total** | **+22** | +31 | +98 | | **production total** | **0** | 0 | 0 | **Production is 0, and not because nothing was looked at.** The defect is in what one test file asserts, not in what it asserts about: `gpu_gate_triggers.txt` and `cpu_gate_exclude.txt` are both correct as they stand, and `scripts/compass/` is this file's *subject* — committing into it is exactly what would stop the gate being one instrument on both sides. There was nowhere under `atom/` or `scripts/` for a correct fix to go. +22 statements sits in the 20-50 band without padding: 4 tests, 2 helpers, and the prose that says which assertion each guard is and why they are two different ones. Most of the +98 physical is that prose. **Formatter.** `black --check` is clean on this file at this head (`BLACK_RC=0`), and on the control. `ruff check` on the file is clean (`RUFF_RC=0`). `ruff format` is a **no-op** on this file on both sides — the counts above are identical before and after it — so nothing here is a formatting artefact, and no shape was decided by it: CI runs `psf/black@stable` as the formatter job and `ruff check .` as the ruff job. Tree-wide `ruff check .` reports **1003 errors on both sides**, delta 0 — the pre-existing baseline, untouched. ## Review closeout Reviewed at `82e465703` against `92f1fdafe` — **APPROVE**, nothing blocking, no new head. The review re-measured twenty-one claims from this body on node 18 and all twenty-one came back exact, including the six #203 calibration numbers and all four gate totals. Both rulings were upheld — the two-shape design call, and keeping the `if TRIGGERS.is_file() else []` against #202's item 1 (dropping it moves the failure to decorator-evaluation time, taking the whole module down on a `FileNotFoundError` inside `_lines` and reporting a less specific reason for a larger blast radius). Three findings came with it; all three are answered here, none changed the head. **Finding 1 — "covers all four" is true for emptiness, not for signal.** Accepted and restated above, in the paragraph following the four-row table. Pre-existing and not introduced here; adding a second manual entry to give the sort something to do would be worse than the vacuity. The review confirmed the vacuity directly by node id on the control rather than by arithmetic, and the `66 → 64 passed, 1 skipped` decomposes independently (`−1` parametrised manual case → empty-parameter-set skip, `−1` for `test_every_excluded_path_still_exists` 29 → 28). No change to the code. **Finding 3 — the two controls assert the derivation while their docstrings claim the guard.** Recorded rather than changed, and here is why the looser wording is the right one to keep. Each control asserts `not _trigger_entries()` / `not _manual_entries()`; each docstring says the guard "has to be shown failing, not just present". The inference from one to the other is one line long today, because both guards read the same helper the parametrisation reads — that shared routing is the *design* being pinned, not an incidental fact, and a control that pins the derivation pins exactly the thing the guard and the parametrisation have in common. The stronger spelling (`with pytest.raises(AssertionError): test_the_trigger_list_exists_and_is_not_empty()`) asserts the guard's own body a second time and would make the control restate the guard rather than state its input. End-to-end behaviour is measured separately and reported above: with the file renamed the branch gives `1 failed, 39 passed, 1 skipped` naming `test_the_trigger_list_exists_and_is_not_empty` and the resolved path, so the guards do fail. The coupling is already partial — that guard's *first* assertion reads `TRIGGERS.is_file()` directly — and the day either guard stops routing through its helper the controls would need re-pointing; that is the condition under which to revisit this, and it is named here so the next reader has it. #203 set this precedent. No change to the code. ### Left undone **The pinned count and `scripts/compass/README.md` are two statements of one fact that nothing joins.** README line 20 reads *"**29** excluded test files … **28 GENERATED** … + **1 MANUAL**"*. Add a manual entry today and `test_the_manual_section_holds_the_entries_it_is_pinned_to_hold` reddens loudly, by design — and README lines 20 and 21 go **silently wrong**. That is the same drift class this test file exists to stop, one directory over. It was unjoined before this PR, so it is not a regression introduced here, but this PR makes one half of the pair loud and leaves the other silent, which is the one cost of the exact count that the body did not price. **The shape a fix would take is already demonstrated in this tree**, one row down in the same README table, for `gpu_gate_known_failures.txt` (line 22): *"Its line count and `BASE_FAILED` are two statements of one fact; `gate_gpu.sh` refuses to run if they disagree."* The fix is that join — a check that reads the README's `29` / `28 GENERATED` / `1 MANUAL` / `30` counts and refuses when they disagree with the files they describe — not a fourth restatement of the number. **It is deliberately not fixed here, and nothing under `scripts/compass/` is touched.** That directory is held: its tree object is the instrument every open PR's gate delta is measured against, and editing it would make the gate a different instrument on the two sides of this delta — which `scripts/compass/README.md`'s own "gate a tree with its own `scripts/compass/`" section measures at four failures per side. Leaving it untouched is what keeps the **+4 passed, 0 failed** above legible. Filed separately as its own issue. ### Still open on #202 This PR covers instances **1** (`:138`) and **4** (`:122`) only. **#202 stays open**: instances **2** and **3** are in `tests/compass/test_spec_schema.py` (#183, #196, #212) and `tests/compass/test_ir_data_model.py` (#194), all four open and editing those files. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
) (#255) Closes #220 once reviewed. ## Round 2: head `48a7d8520`, restacked onto `05880556e` The branch was restacked onto the integration tip `05880556e` (`git rebase --onto 0588055 5a07d44`, no conflicts; `git diff 0588055...HEAD` lists only the test file). That puts the #203 guard spelling in the tree. The review-round fix is one commit on top, `48a7d8520`. Test file only; still **no production change**. **Blocking, `:128`: the parametrisation now states its non-empty case.** It uses the spelling from `test_runner_non_allocating.py` on the tip: a `_kv_modules()` helper, `test_the_package_was_found`, and the control `test_the_guard_finds_nothing_when_the_root_moves`. Measured on node 18, three kv test files, every edit line-count-preserving (184 -> 184, 149 -> 149): | tree | edit | result | |---|---|---| | `d205f36df` (old head) | none | 50 passed, rc 0 | | `d205f36df` (old head) | `:40` `"kv"` -> `"kv_moved"` | **46 passed, 1 skipped, rc 0**: `got empty parameter set` (the reviewer's figure) | | `48a7d8520` | none | 53 passed, rc 0 | | `48a7d8520` | null: trailing comment on `:40` | 53 passed, rc 0 | | `48a7d8520` | `:40` `"kv"` -> `"kv_moved"` | **2 failed**, 47 passed, 1 skipped, rc 1 | | `48a7d8520` | `_kv_modules` falls back to `PACKAGE.parent` when the root is missing | **1 failed**, 52 passed, rc 1 | | `48a7d8520` | module name reverted to `"atom.compass.kv." + path.stem` | **1 failed**, 52 passed, rc 1 | Red node ids, by row: ``` # rename tests/compass/test_kv_driverless_import.py::test_the_package_was_found tests/compass/test_kv_driverless_import.py::test_a_module_in_a_subpackage_keeps_its_subpackage # guard widened past its root tests/compass/test_kv_driverless_import.py::test_the_guard_finds_nothing_when_the_root_moves # stem-based module name tests/compass/test_kv_driverless_import.py::test_a_module_in_a_subpackage_keeps_its_subpackage ``` A first try at the control's witness, `PACKAGE.parent.rglob` unconditionally, gave a collection error (rc 2). The ids lambda refuses a path outside `PACKAGE`, so it was not a clean witness. The conditional fallback in the table is the one that isolates the control. **Non-blocking, taken:** - **`:148`**: `test_a_price_is_computed_without_one` also asserts `Path(seen["file"]).is_relative_to(REPO)`, so the child-side `atom` check survives an empty parametrisation. - **`rglob` plus `path.stem`**: `_module_name` builds the dotted name from `path.relative_to(REPO)`. Ids are the path relative to `PACKAGE`, so node ids are unchanged for the flat package. `test_a_module_in_a_subpackage_keeps_its_subpackage` pins it (row above). **Non-blocking, declined: `:42` stays `== ALLOWED`.** The follow-up below changes the docstrings to say numpy is the one third-party package the import loads. With equality this test is the measurement behind that sentence. With `<=` the sentence could go stale and nothing would say so. The cost is a loud, one-line red if numpy is ever removed. **Blind spot 1, corrected.** In round 1 I wrote that diffing `/proc/self/maps` "would need a named list again". The reviewer pointed out that it would not: each newly mapped `.so` could be required to sit under the stdlib or numpy's own directories, which is an allowlist by origin. It is still Linux-only and misses the subprocess case, so it stays out of this PR. **Follow-up, not in this PR (production code outside the file set).** The reviewer ruled that "no tensor library" is literally false, since the import loads numpy, while the property that matters holds. Recommended edit, docstring only, 0 statements: `atom/compass/kv/__init__.py:10` and `atom/compass/kv/transfer.py:39` should read "no device runtime; numpy is the one third-party package it loads". ### Gate delta against `05880556e` Node 18 (`xiaobizh_n18_cpu`). Each tree was staged by `git archive` plus both stamps into `/tmp/i220r2gates/<tree>/ATOM`, my own path and not the shared mount, and shipped by `docker exec -i ... cat`. The tarball md5 was `5dbc9740...` at both ends. Each tree used its own `scripts/compass`, with the same content hash on all three trees. `atom.__file__` was printed under each staged root before any count, and the gate's `commit:` stamp matched (`05880556e`, `48a7d8520`). Runs were serial, each under `timeout -k 10 5400`, with no pipe. | | control `05880556e` | branch `48a7d8520` | delta | |---|---|---|---| | passed | 4829 | 4838 | **+9** | | skipped / xfailed | 149 / 3 | 149 / 3 | 0 | | `GATE_CPU_RC` | 0 | 0 | | The control matches the established figure for `05880556e`. Decomposed by collected node id (`--collect-only` over `tests/compass` on both trees, then diffed): nothing is only in the control, and the only ids that exist just on the branch are the 9 tests in the new file: ``` tests/compass/test_kv_driverless_import.py::test_a_module_in_a_subpackage_keeps_its_subpackage tests/compass/test_kv_driverless_import.py::test_a_price_is_computed_without_one tests/compass/test_kv_driverless_import.py::test_importing_it_loads_no_device_runtime[__init__.py] tests/compass/test_kv_driverless_import.py::test_importing_it_loads_no_device_runtime[connector.py] tests/compass/test_kv_driverless_import.py::test_importing_it_loads_no_device_runtime[handoff.py] tests/compass/test_kv_driverless_import.py::test_importing_it_loads_no_device_runtime[transfer.py] tests/compass/test_kv_driverless_import.py::test_the_child_sees_a_package_reached_through_another tests/compass/test_kv_driverless_import.py::test_the_guard_finds_nothing_when_the_root_moves tests/compass/test_kv_driverless_import.py::test_the_package_was_found ``` No test failed on either side, and the skip count is equal, so the known flaky class `TestTheRegionIsNotCopiedPerChunk` did not move. Size: the test file is now **64** AST statements over 184 physical lines. It was 47 statements over 149 lines, so this round adds 17. Production is still 0. `ruff check` and `ruff format --check` both pass. --- *Round 1 body follows unchanged below, apart from blind spot 1 and the closing question, which the round-2 section above now covers. Its figures are for `d205f36df` against `5a07d4489`.* ## What this does `atom/compass/kv/` says in its docstring that it loads **no tensor library and no device runtime**, "so a price can still be computed where no driver exists". That was true, and no test checked it. This PR adds **`tests/compass/test_kv_driverless_import.py`**, which checks it. **No production change.** This rests on **principle 2**: simulated execution touches no GPU, and capability is configured, never read from a device runtime. It also rests on **principle 8**: the docstring made a claim with no measurement behind it. ### How it measures The claim is about what an import loads, and pytest has already loaded torch by the time any test runs. So each import runs **in a fresh interpreter**. The child imports the module, then reports what it added to `sys.modules` and the file each added module came from. - **The import is run, not scanned.** The pattern comes from `_import_time_imports` in `test_runner_non_allocating.py`, and I followed **#252's amendment** to it rather than the AST scan. #252 found that a source scan misses a driver reached through an import of an import. That matters here: most of what this package loads is in `atom.kv_transfer` and `atom.model_engine`, not in `atom/compass/kv/`. The #252 test runs its import in the pytest process, which works for "does the import raise". It does not work for "was torch loaded", because pytest has always loaded torch already. That is the only reason this test uses a subprocess. - **Parametrised over every module in `atom/compass/kv/`**, the same way #252 parametrises over `PACKAGE.rglob("*.py")`. - **Both sides of the boundary, and the wider form from #170.** `BOUNDARY` lists the modules the docstring names: the three `atom.compass.kv` modules, `atom.kv_transfer.disaggregation.{base,factory,types}`, and `atom.model_engine.sequence`. The test asserts that all of them **were loaded**. This is the positive assertion: if the import did nothing, the test fails instead of passing vacuously. - **A price is actually computed.** One test calls `TransferModel(...).release_at(...)` in the child, checks the result, and only then reads `sys.modules`. - **The instrument is checked on a known answer.** A control writes `probe_outer.py` (which imports `probe_inner`) into `tmp_path` and asserts that the child reports both. That shows the check sees a package reached **transitively**. ### Which modules count as "a tensor library or device runtime" **Everything outside this repository and the standard library, except numpy.** In the test: the third-party top-level packages added by the import must **equal** `ALLOWED = {"numpy"}`. - **Why an allowlist and not a named denylist.** A list like {torch, triton, aiter, hip, …} passes whichever runtime nobody wrote down. That is the same blindness as the sibling finding, where a scan filtered to `atom.*` never saw a bare `import aiter`. With an allowlist, any new third-party dependency makes the test fail by name, and someone has to decide about it. - **Why numpy is allowed.** `atom.model_engine.sequence` imports it, so it arrived with the wider claim from #170. It is a host-side array library and opens no device. Measured on `5a07d4489`: `import atom.compass.kv` in a fresh interpreter adds 144 modules, and the only top-level packages outside stdlib and `atom` are `['numpy']`. - **Why equality and not subset.** The test uses `== ALLOWED`, not `<= ALLOWED`. So the allowance disappears when numpy does, and numpy being detected proves installed packages are detected at all (see the instrument mutation below). - **How stdlib is identified.** Stdlib is decided by where a module was loaded from, not by `sys.stdlib_module_names`. My first version used the name list. It reported `__mp_main__` and `_sysconfigdata__x86_64-linux-gnu` as third-party, which is a false red on stdlib modules. The origin rule has a trap of its own, and I measured it on node 18. In `/opt/venv`, `platstdlib` is `/opt/venv/lib/python3.12`, and **`site-packages` sits inside it**. A plain "is under a stdlib dir" check would therefore classify torch as stdlib and pass vacuously. So a path that has a `site-packages` or `dist-packages` component never counts as stdlib. ## The named result Every mutation is **line-count-preserving**: it replaces a blank line in place (`connector.py:54`, `types.py:17`, `sequence.py:9`). Each tree was staged by `git archive` plus stamps and shipped with `docker cp`/`docker exec -i tar` into `xiaobizh_n18_cpu:/tmp/i220gates/<tree>/ATOM`. I checked tarball md5s on both ends. `atom.__file__` resolved under each staged root and was printed before any count, and the gate's own `commit:` stamp matches. Both sides use the tree's own `scripts/compass`, which is the same content hash (`ad0cd1bace97`) on every tree. ### Full `gate_cpu.sh`, before and after this PR | tree | control `5a07d4489` | branch `d205f36df` | |---|---|---| | unmutated | 4601 passed / 149 skipped / 3 xfailed, `GATE_CPU_RC=0` | **4607** passed / 149 / 3, `GATE_CPU_RC=0` (twice) | | `import torch` at `atom/compass/kv/connector.py:54` | **4601 passed, `GATE_CPU_RC=0`**: undetected | **5 failed, 4602 passed**, `GATE_CPU_RC=1` | | `import torch` at `atom/kv_transfer/disaggregation/types.py:17` | **4601 passed, `GATE_CPU_RC=0`**: undetected | **5 failed, 4602 passed**, `GATE_CPU_RC=1` | On both sides the five failing node ids are the same, and they are all in the new file: ``` tests/compass/test_kv_driverless_import.py::test_importing_it_loads_no_device_runtime[__init__.py] tests/compass/test_kv_driverless_import.py::test_importing_it_loads_no_device_runtime[connector.py] tests/compass/test_kv_driverless_import.py::test_importing_it_loads_no_device_runtime[handoff.py] tests/compass/test_kv_driverless_import.py::test_importing_it_loads_no_device_runtime[transfer.py] tests/compass/test_kv_driverless_import.py::test_a_price_is_computed_without_one ``` All five fail on the same assertion, `assert _third_party(seen["added"]) == ALLOWED`: `AssertionError: assert {'dill', 'num...g_extensions'} == {'numpy'}`, extra items `torch`, `torchgen`, `dill`, `tqdm`, `typing_extensions`. One run note. The first branch run of the `types.py` injection also showed **3 setup errors**, `EADDRINUSE` on port 29591, in `test_world_size_reports_logical`, `test_all_reduce_is_identity_on_a_lone_rank` and `test_all_gather_is_logical_width_with_absent_ranks_zeroed[0]`. I had run six gates at once, and those tests bind a fixed `torch.distributed` port. I re-ran that tree alone: 5 failed / 4602 passed, no errors. The table shows the solo run. The known flaky class `TestTheRegionIsNotCopiedPerChunk` did not show up in any run. ### Wider mutation set (kv test files only: `test_kv_driverless_import.py`, `test_kv_simulated_connector.py`, `test_kv_remote_prefill.py`) | mutation, line-count-preserving | control (44 tests) | branch (50 tests) | |---|---|---| | none | 44 passed | 50 passed | | **null**: `import json` at `connector.py:54` | 44 passed | **50 passed** | | `import torch` at `connector.py:54` | 44 passed | 5 failed, 45 passed | | `import torch` at `types.py:17` | 44 passed | 5 failed, 45 passed | | `import torch` at `atom/model_engine/sequence.py:9` (the wider claim from #170) | 44 passed | 5 failed, 45 passed | | `import triton` at `connector.py:54` (a different runtime) | not run | 5 failed, 45 passed: extra `triton` | | `import importlib; importlib.import_module("torch")` (no `import torch` for a scan to find) | not run | 5 failed, 45 passed | | `import aiter` at `connector.py:54` | not run | collection error, rc=2 (see below) | | **instrument**: in the test itself, `"site-packages"` → `"zzzz-packages"` (brings back the venv trap) | n/a | 5 failed, 45 passed: `assert set() == {'numpy'}` | About `import aiter`: on the CPU tier this was **already caught before this PR**. The existing kv test files import `atom.compass.kv` in-process, aiter's `rocminfo` probe raises, and collection errors. On a machine with a driver the import succeeds, and then the allowlist catches it as an extra `aiter`. I did not measure that on a GPU box. ## Gate delta against `5a07d4489` | | control `5a07d4489` | branch `d205f36df` | delta | |---|---|---|---| | passed | 4601 | 4607 | +6: the 6 new tests (1 control, 4 parametrised, 1 price) | | skipped / xfailed | 149 / 3 | 149 / 3 | 0 | | `GATE_CPU_RC` | 0 | 0 | | The control I ran matches the established figure for `5a07d4489` (4601/149/3, rc 0). | AST statements (after `black` 26.5.1) | statements | code lines | physical lines | |---|---|---|---| | production | **0** | 0 | 0 | | test: `tests/compass/test_kv_driverless_import.py` | **47** | 84 | 149 | Calibration: the same counter gives landed `atom/compass/spec/` = 275 / 495 / 763. It counts `ast.stmt` nodes; code lines are token lines excluding comments and docstrings. `black --check` and `ruff check` both exit 0 on the new file. ## What this pin still would not notice 1. **A device library loaded without a Python module**, for example `ctypes.CDLL("libamdhip64.so")` or a `subprocess` call to `rocminfo` at import. Neither adds anything to `sys.modules`. Reading `/proc/self/maps` would catch the first, as an allowlist by origin: every newly mapped `.so` would have to sit under the stdlib or numpy. I left it out because it is Linux-only and does not cover the subprocess case. 2. **Imports that happen at call time outside the pricing path.** The test prices through `TransferModel.release_at`. A connector method that runs `import torch` inside its body is not exercised in the child. 3. **Anything loaded before the child's snapshot**, for example a `sitecustomize` or `.pth` file that imports torch at interpreter start. That is a property of the environment, not the package, and the before/after difference hides it. 4. **A device runtime reached from inside `atom.*` without a new package**, for example an `atom` module that calls `rocminfo` at import and swallows the failure. `atom` is excluded from the third-party set by design. 5. **`try: import torch / except ImportError`** on a machine without torch. There the property holds trivially. On the CPU tier torch is installed, so the import succeeds and is caught. ## Round 1 question: the docstring says "no tensor library" (answered; see the follow-up in round 2) numpy is loaded, and it is an n-dimensional array library. This PR states the property as **no device runtime, with numpy the one allowed third-party package**, and the test docstring gives the reason. I have **not** edited the package docstrings in `atom/compass/kv/__init__.py` or `transfer.py`. The brief's file set is `tests/compass/` with no production change expected, so I am raising this instead of working around it. If the reviewer agrees, a follow-up could change "no tensor library" to "no device runtime; numpy is the only third-party package" so the sentence says exactly what the test checks. That is a docstring-only edit with 0 statements. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Closes #189 (items 1 and 3 of "What a fix should do"; item 2 is a process change, not a code change).
tests/compass/test_backend_interface.pyandtests/compass/test_runner_non_allocating.pyeach parametrised
sorted(PACKAGE.rglob("*.py"))straight into the decorator. A root thatdoes not resolve yields nothing, and a parametrisation of nothing is a pass, so the pin is
inert the moment the package it governs is renamed or moved. Both now derive the list
through a helper and assert it non-empty first.
Design principles this turns on: 6 (refuse rather than fall back — an empty collection
is the fallback here), 8 (every claim carries its measurement — both directions below
are measured, not argued), and 3 for what was deliberately not added.
Which is which, and why the loud one is not evidence the class is handled
Measured on node 18 in
xiaobizh_n18_cpu, by renaming the package in the tree(
atom/compass/backends→cost_backends,atom/compass/runner→sim_runner, everyatom.compass.<pkg>import updated with it) and running that file alone. Updating theimports is what a real rename does: they fail loudly, so they get fixed. The path constant
in the test does not fail, so it does not.
92f1fdafe09ff555e8test_backend_interface.py, backends renamedPYTEST_RC=0— 4 passed, 1 skipped,got empty parameter set for (path)at:91PYTEST_RC=1— 1 failed, 5 passed, 1 skipped;test_the_package_was_foundfails:no modules under .../atom/compass/backends,assert []test_runner_non_allocating.py, runner renamedPYTEST_RC=1— 1 failed, 19 passed, 1 skipped; the same empty-parameter-set skip at:313, red only throughtest_the_replaced_methods_are_the_ones_that_own_memory_or_run_a_stepraisingFileNotFoundErroron.../runner/overrides.pyPYTEST_RC=1— 2 failed, 20 passed, 1 skipped; the sibling andtest_the_package_was_foundtest_backend_interface.pyis the silent one: it reports green with its only pin assertingnothing.
test_runner_non_allocating.pyreddens — but not through its own scan, which isjust as inert (same skip, same zero cases). It reddens because a neighbour happens to read
a file under the same root.
And the neighbour's redness is itself an artefact of a half-finished rename. Measured on
review: let that sibling follow the rename through its import, which is what a real rename
does to a path it can resolve, and the control tree goes to
PYTEST_RC=0— 20 passed, 1skipped, fully green, with the scan still collecting nothing. The same edit on this branch
stays
PYTEST_RC=1, because the guard is a statement about the scan's own root. The rescuewas entirely borrowed.
So the two sites are not one silent and one caught. They are both silent; one of them was
being carried, for the length of time it took somebody to finish a rename. A protection you
get from who your sibling is, and lose without a diff to the test, is not the class being
handled. Both needed the guard; neither would reliably have been caught.
Named result — both directions, per site
Run on staging copies of this branch,
__pycache__purged,atom.__file__confirmed underthe staged root before each run.
1 — the guard fires when its package is renamed or moved. The two right-hand cells above.
Both new
test_the_package_was_foundcases fail with the package's resolved path in themessage, and the file that used to be
rc=0is nowrc=1.2 — the test it guards still fails for its original reason when that reason is reinstated.
A module that breaks the invariant, added to the package with the package in place:
atom/compass/backends/leaky.pyimportingatom.model_engine.model_runnerPYTEST_RC=1, 1 failed 12 passed —test_the_package_imports_nothing_from_the_engine[leaky.py]:leaky.py:1 imports atom.model_engine.model_runner. The guard passed.atom/compass/runner/leaky.pyimporting the samePYTEST_RC=1, 1 failed 26 passed —test_only_the_binding_module_reaches_the_engine[leaky.py]. The guard passed.So the guard has not replaced a real assertion with a liveness check: with the package
present the parametrised assertion still does its own job, and the guard is silent.
#189's own direction 2 — it does not fail for a reason nobody intended. A module added to
an unrelated Compass package (
atom/compass/spec/unrelated_new_module.py): both filesPYTEST_RC=0, 12 passed and 26 passed, unchanged from untouched.Baseline, untouched: 12 passed / 26 passed (control: 10 / 24 — the +2 each are the guard
and its control).
The control, and why the guard alone was not enough
Each guard carries
test_the_guard_finds_nothing_when_the_root_moves, which points thederivation at a root that does not resolve and asserts it comes back empty, with a module one
level out so a derivation that widened past its own root is caught there. Without it the
guard is only ever seen passing, which is the shape this board has now found eight tests in:
a pin that passes with its defect reinstated. The three landed guards
(
test_clock_lp_identity.py,test_spec_schema.py,test_ir_data_model.py) have no suchcontrol; these two do, and that is the only place this PR departs from the landed spelling.
The guard itself is that spelling verbatim — helper, then
assert _<pkg>_modules(), f"no modules under {PACKAGE}"— rather than a second spelling ofsomething the suite already has.
Site #2 is untouched
_call_sitesattest_runner_rpc_surface.py:117still reads all ofatom/. Narrowing itwould convert a reported exclusion into a silent one, which #190 ruled on. This PR does not
touch that file.
Gates
Gate 1 — ATOM's suite unmodified.
scripts/compass/gate_cpu.shfrom each tree's ownscripts/compass/(never overlaid, never committed to), staged bygit archive+docker cpinto
xiaobizh_n18_cpu, content digests verified identical on both ends, both stamps written.commit:GATE_CPU_RCfeature/atomcompass_new92f1fdafe09ff555e8Delta +4 passed, 0 failed, skips and xfails unchanged — the two guards and their two
controls. The control reproduces the stated 4557. No failure to triage, so the
TestTheRegionIsNotCopiedPerChunkthree-way flake did not arise on either side.Gate 2 — new CPU-only tests. Four, all in
tests/compass/; they read paths and parsetext, and touch no device.
Gate 3 — the named result. Above, both directions, per site, with counts.
Effort
Counted after
ruff format, statements = everyast.stmtnode with docstringExprnodesincluded.
tests/compass/test_backend_interface.pytests/compass/test_runner_non_allocating.pyProduction is zero because nothing under
atom/orscripts/is touched — this is a defectin what two tests assert, not in what they assert about. +18 is just under the 20-50 estimate:
reusing the landed guard spelling verbatim is what keeps it there, and padding it to reach a
band would have meant inventing a second spelling.
ruff formaton these two files also wants to rewrap a pre-existingassertintest_backend_interface.pythat the tree carries unwrapped. That rewrap is reverted here, andthe reason is not that a baseline is not mine to move — it is that
ruff formatis not thebaseline.
.github/workflows/pre-checks.yamlrunspsf/black@stableas the formatter joband
ruff check .(notruff format) as the ruff job. This branch as committed passesblack --check; with ruff's rewrap applied, Black wants to revert it, so taking ruff's versionwould have broken conformance with the gate that actually exists. The
ruff formatbaseline isseparately already dirty, and identically so on both sides — 42/210 files in
tests/, 2/17 intests/compass/. Runruff formathere to check a count, never to decide a shape.Audit of the rest of
tests/compass/17 test modules, 61 collection sites checked — 50
@pytest.mark.parametrizecalls (31with a literal display, 19 deriving their argvalues) plus 11 assert-bearing
forloops over anon-literal iterable inside a test body.
Found: the 2 filed here, plus 4 more, spanning 6 further sites in 3 files — filed as
#202, not folded in, per #189's instruction and because handing a finding to whatever
touches the file next is what created #186. All four are measured and all four report
PYTEST_RC=0with their defect in place:test_cpu_gate_exclude.py:138(gpu_gate_triggers.txtrenamed)test_spec_schema.py:194,:212(DEPLOYMENT_OWNEDemptied)test_ir_data_model.py:691,test_spec_schema.py:509(__all__emptied)test_cpu_gate_exclude.py:122(MANUAL section emptied, both markers kept)Instance 3 is not a weaker signal than the others; it is none. A parametrisation that
collects nothing at least prints
got empty parameter setand drops the pass count. Aforloop over an empty iterable inside a passing test prints nothing, moves no count, and returns
a byte-identical summary line.
Correction to this PR's first count, which said 56 sites cleared and three instances.
Instance 4 was found on review, by the same method, in a file this audit had already flagged
— twenty lines above its own finding 1, with the same
return []fallback. The manual sectionis legitimately allowed to be empty, which is what made it look cleared; that it is allowed
to be empty is not the same as its being asserted non-empty, and the two cases it drops go
with it. So the sweep was one short: 55 cleared, not 56, and #202 is four instances across
six sites. Reporting 56 as a cleared total, with an under-inspected member inside it, is the
aggregate-without-its-decomposition failure principle 7 names; the decomposition is above so
the next reader can check each row rather than the sum.
The 55 cleared each have something that states the non-empty case: a
test_the_package_was_found, a pinned length (assert len(RPC_SURFACE) == 12), a supersetassertion, a literal-valued sibling covering the same mechanism, or a literal display that can
only be empty if written empty.
🤖 Generated with Claude Code