Skip to content

compass(spec): make four spec pins name the refusal they are about (#237) #263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jgong5 merged 1 commit into from
Sep 23, 2026
Merged

compass(spec): make four spec pins name the refusal they are about (#237) #263

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 91 additions & 9 deletions tests/compass/test_spec_verbs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -809,7 +809,15 @@ def test_raise_first_gives_the_spec_or_the_first_refusal():


def refused_by_condition():
"""One subject per condition in the check set, each earning its refusal."""
"""One subject per condition in the check set, each earning its refusal.

Each subject comes with the rule and a piece of the text only that
condition's refusal writes. A subject that is wrong in one respect is
usually wrong in another too -- a document missing an entry is refused for
the missing width as well as for the probe that could not fill it -- so a
rule alone can be earned by a neighbouring condition while the one named
here produces nothing.
"""
thin = without(TIER1, "device", "arch")
no_constant = without(
TIER1, "device", "runtime_constants", "persistent_forward_buffer_bytes"
Expand All @@ -833,24 +841,51 @@ def refused_by_condition():
)
return {
MISSING_ASKED: [
(validate(merge(fragments(tier1=thin)).document), Rule.SHAPE),
(
validate(merge(fragments(tier1=thin)).document),
Rule.SHAPE,
"`device.arch` is missing",
),
(
validate(merge(fragments(tier1=no_constant)).document),
Rule.NO_DEFAULTS,
"`device.runtime_constants.persistent_forward_buffer_bytes` is missing",
),
],
DERATES_ASKED: [
(validate(merge(fragments(tier1=no_derate)).document), Rule.DERATE)
(
validate(merge(fragments(tier1=no_derate)).document),
Rule.DERATE,
"`device.memory.derate` is missing, and its block states a spec peak",
)
],
WIDTHS_ASKED: [
(
validate(merged(), tp_widths=(16,)),
Rule.NO_DEFAULTS,
"was not measured at tensor-parallel width 16",
)
],
WIDTHS_ASKED: [(validate(merged(), tp_widths=(16,)), Rule.NO_DEFAULTS)],
PROBES_ASKED: [
(
validate(merge(fragments(tier1=no_probe)).document, tp_widths=(1,)),
Rule.NO_DEFAULTS,
(
"no probe here fills `allocator_retained_after_load_bytes` "
"at tensor-parallel width 1"
),
)
],
STACK_ASKED: [
(moved, Rule.PINNED_STACK, "this spec's constants were measured against")
],
TRANSFERS_ASKED: [
(
validate(transferred),
Rule.PINNED_STACK,
"measured against rocm '7.0.2', into a spec pinned to rocm '7.2.4'",
)
],
STACK_ASKED: [(moved, Rule.PINNED_STACK)],
TRANSFERS_ASKED: [(validate(transferred), Rule.PINNED_STACK)],
}


Expand All @@ -867,9 +902,14 @@ def test_the_check_set_names_every_condition_a_spec_can_be_refused_by():

@pytest.mark.parametrize("condition", CONDITIONS)
def test_each_condition_in_the_check_set_is_earned_by_a_spec(condition):
for checked, rule in refused_by_condition()[condition]:
for checked, rule, written in refused_by_condition()[condition]:
assert not checked.ok, condition
assert rule in {refusal.rule for refusal in checked.refusals}, condition
# The rule and the text on one refusal: the rule alone is shared by
# conditions, and the text alone could sit on a refusal of another rule.
assert any(
refusal.rule is rule and written in refusal.what
for refusal in checked.refusals
), (condition, [str(refusal) for refusal in checked.refusals])


def test_a_document_reaches_what_the_package_says_a_document_reaches():
Expand Down Expand Up @@ -1550,6 +1590,14 @@ def test_readings_no_card_could_produce_are_refused_though_their_difference_is_n
with pytest.raises(SpecRefusal) as refused:
non_torch_across_ranks(2, [odd, odd], 7.2e9)
assert refused.value.rule is Rule.DEVICE_WIDE
# These readings are two impossibilities, and the negative reserve would
# be refused by the same rule on its own. What is named is the free memory
# the card cannot hold, so a check that stopped asking that is seen here
# rather than covered by its neighbour.
assert "rank 0 reports 400000000000.0 bytes free of 288000000000.0" in (

@jgong5 jgong5 Sep 23, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review is agent-authored.

Non-blocking (principles 3 and 6): this assertion pins the order of impossible's checks, and the comment above it gives the wrong reason if it ever turns red.

My ruling: the order is an accident, not a contract. DeviceMemory.impossible is a chain of first-match checks. Its docstring ("What about these readings no single card could have produced") states no precedence between free > total and reserved < 0, and nothing else holds the order. I swapped those two branches with a line-count-preserving mutant:

tip 175739f87 merged tree
free > total and reserved < 0 branches swapped 0 failed / 1151 passed 1 failed: this test, :1597 ... in 'rank 0 reports -120000000000.0 bytes reserved, which is not a reading of a card (...)'

So this PR adds a new constraint on the order. The constraint cannot be avoided in this test. The test's premise is a plausible difference (non_torch = total - free - reserved > 0) beside free > total, and that forces reserved < 0. So this fixture always carries both impossibilities, and the only way it can witness the free > total branch is to fix which of the two is named. The branch itself is already witnessed on a fixture that reaches only that branch: test_a_rank_whose_readings_cannot_be_one_card_is_refused_and_named[overrides2-...] is red with the branch removed, both on the tip and on the merged tree.

I accept the order pin, because the body discloses it. The code comment does not, though. Someone who reorders the branches on purpose will read "a check that stopped asking that is seen here" and look for a check that was removed. A cheap fix is to say what is actually held. For example: "Both impossibilities are present, and which one impossible names first is fixed here on purpose; the branch on its own is held by the parametrised test above." The alternative is to drop this assertion and let the review record on #136 name [overrides2], which #237 asks for anyway.

refused.value.what
)
assert "take them again and find out" in refused.value.remedy


def test_a_reading_just_inside_the_limit_is_accepted_and_is_what_the_spec_takes():
Expand All @@ -1570,7 +1618,13 @@ def test_a_width_with_no_ranks_is_refused_rather_than_reduced(tp_width):
with pytest.raises(SpecRefusal) as refused:
non_torch_across_ranks(tp_width, [], 7.2e9)
assert refused.value.rule is Rule.RANK_AGREEMENT
assert str(tp_width) in refused.value.what
# The cross-rank reduction refuses a negative width by the same rule, for
# having no reading per rank, and names the width too. The text below is
# the width's own refusal, which is asked before any reading is counted.
assert (
f"tensor-parallel width {tp_width!r} has no ranks to read a card on"
in refused.value.what
)


def test_the_readings_are_reachable_by_name():
Expand Down Expand Up @@ -1661,6 +1715,34 @@ def test_a_spec_that_carries_the_hand_measured_entry_is_not_refused_for_it():
assert checked.ok


def test_the_probe_question_is_asked_of_the_tables_it_says_it_reads(monkeypatch):
# The condition registers `PROBE_TABLES` as what it reads, and the walk that
# produces its refusals has to be over the same tables. Walking every width
# table instead changes nothing today -- the reserve table is filled at
# every width, so asking about it never refuses -- which is why the walk is
# observed here by what it asks rather than by what it answers.
asked = []

def recording(constant, tp_width):
asked.append((constant, tp_width))
return probe_for(constant, tp_width)

monkeypatch.setattr(
importlib.import_module("atom.compass.spec.validate"), "probe_for", recording
)
checked = validate(merged().document, tp_widths=(16,))
# Width 16 is missing from every width table, so a walk over any of them
# would reach the probe; the width question says so once per table.
unmeasured = [
refusal
for refusal in checked.refusals
if "was not measured at tensor-parallel width 16" in refusal.what
]
assert len(unmeasured) == len(WIDTH_TABLES) > len(PROBE_TABLES)
assert asked, "the probe question asked nothing"

@jgong5 jgong5 Sep 23, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review is agent-authored.

Non-blocking (principles 3 and 8): this line is the only reason the test turns red when the width-1 hole in FILLED_BY is filled, and the PR body's reason for accepting that red does not hold.

My ruling on brittleness. The recorder observes a call to a module-level name, which makes this a white-box pin. I accept it. The property is about what the walk asks: _reach registers PROBE_TABLES as what PROBES reads. Under today's FILLED_BY, no refusal can show which tables were walked. That is F4 on the control: 0 failed / 1151 passed. So recording what is asked is the only observation available, and principle 3 does not ask for a more elaborate one.

The claim that fails. The body says that once the hole is filled, "no fixture can observe the walk". Once the hole is filled, PROBE_TABLES is (). A walk over WIDTH_TABLES still asks two questions, and the equality on the next line sees them. I measured this on the merged tree (tip 175739f87 + 00a4d1479, node 18), using line-count-preserving mutants and running tests/compass:

mutant result
this line changed to assert asked or not PROBE_TABLES, ... 0 failed / 1152 passed
the same change + _probes walks WIDTH_TABLES 1 failed: this test, :1743 [('driver_and...d_bytes', 16)] == [('allocator_...d_bytes', 16)]
the same change + hole filled ((SINGLE_CARD, MULTI_RANK)) this test passes. The 5 other reds are the tests that are about the hole.
the same change + hole filled + _probes walks WIDTH_TABLES this test fails at :1743: [('driver_and...d_bytes', 16)] == []
this line as written + hole filled this test fails at :1742: the probe question asked nothing

With assert asked or not PROBE_TABLES, the test keeps today's result and keeps catching the wrong walk after the hole is filled. It also stops going red for a change in another module that it has no quarrel with. The case the non-empty check exists for, a refactor that stops calling probe_for, only matters while PROBE_TABLES is non-empty. With PROBE_TABLES empty, "asked nothing" is the correct answer.

The cost of the current form is small. Filling the hole already reddens 4 tests at the tip, and this PR brings that to 6. Still, please either make the one-word change or correct the "still would not notice" paragraph of the body.

assert asked == [(path.rsplit(".", 1)[-1], 16) for path in PROBE_TABLES]


@pytest.mark.parametrize("tp_width", [0, -3])
def test_a_width_below_one_card_is_refused_rather_than_handed_a_probe(tp_width):
# Every width that was not exactly one fell to the multi-rank probe, zero
Expand Down