compass(spec): explain refuses an unknown term as ADDRESSING and an absent one as TOTALITY - #264
Conversation
…bsent one as TOTALITY explain(spec, term) raised one SHAPE refusal whenever the basis came back empty. That folded two cases together, and neither of them is a document to correct: - A term that is not a field, a block or a quantity is now refused under ADDRESSING, and the remedy says to ask again by a real dotted path. - A term the schema declares, with nothing under it that this spec holds, is now refused through refuse_absent_field, the same TOTALITY refusal MachineSpec.value gives for that field. An optional field the document left out is told to be written in the document; a required one missing from a spec built from parts is told to merge the fragment that measures it. Neither is told to ask again, because any spelling finds the same absence. The site leaves ELSEWHERE in the SHAPE partition, and a new test places the explain sites under ADDRESSING and TOTALITY. Closes #259 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| "name a field or a block of fields by its dotted path, or one of " | ||
| f"the quantities {sorted(QUANTITIES)}", | ||
| ) | ||
| refuse_absent_field(paths[0], BY_PATH[paths[0]].required) |
There was a problem hiding this comment.
Blocking (principle 6): this refusal fires on a spec that does carry the field, and every clause of its text is then false.
not contributions is not the same test as "no path under the term is in spec.values". A field can be present and still produce zero rows: _rows returns [] for host.tokenizers when the table has no entries. An empty tokenizer list passes check (it is a Sequence) and table(), so MachineSpec.from_mapping reads it.
Measured on node 18 (xiaobizh_n18_cpu), on the head's explain.py, with the spec built by MachineSpec.from_mapping(written("host.tokenizers", [])) from tests/compass/test_spec_schema.py:
read ok; host.tokenizers in values: True ()
value(): ()
[host.tokenizers] TOTALITY: a spec resolves every field the document was required to state: `host.tokenizers` is declared by this schema, and this spec carries no value for it. a spec read with `MachineSpec.from_mapping` resolves every required field, so this one was assembled from parts; merge the fragment that measures this field before asking for it
So:
- the spec was read with
from_mapping, not assembled from parts; - it carries a value, and
spec.value("host.tokenizers")returns(); - the remedy ("merge the fragment") cannot be acted on.
That also breaks the PR's central claim that explain(spec, p) and spec.value(p) cannot disagree about one path. Here one answers and the other refuses.
Before this PR, the site gave the generic SHAPE text for this case. The PR made the message specific, and that specific message is untrue here.
Asked for:
- Refuse as absent only when nothing under the term is in
spec.values, and name a field that really is absent. For example, testif not any(path in spec.values for path in paths). Every path is then absent, sopaths[0]is honest. - Decide the empty-table case explicitly: either an empty
Basis, which says truthfully that no tokenizer was measured, or a refusal under the rule that owns tokenizers. - Add one test that drives
explainon a read spec whose tokenizer list is empty.
There was a problem hiding this comment.
Fixed in 47e40e05d. The guard now reads if not any(path in spec.values for path in paths) and runs before the rows are built. So refuse_absent_field fires only when every path under the term is absent, and the field it names really is absent.
The empty-table case is decided explicitly: an empty Basis, not a refusal. host.tokenizers: [] is held, and value() answers (). explain now answers with zero rows (host.tokenizers, from spec <digest>), which says truthfully that no tokenizer was measured. A refusal under the tokenizer rule would be a new contract for a spec that from_mapping accepts, so I left it out.
The new test tests/compass/test_spec_verbs.py::test_an_empty_tokenizer_table_explains_to_no_rows_rather_than_refusing drives this on a read spec. It asserts value() == (), contributions == () and the printed header. It is pinned by M5, which restores refuse-on-zero-rows in one line: if not any(_rows(...) for p in paths if p in spec.values). M5 reddens that test and nothing else.
| @@ -183,15 +190,18 @@ def explain( | |||
| for field in SCHEMA | |||
| if field.path == term or field.path.startswith(f"{term}.") | |||
There was a problem hiding this comment.
Non-blocking (principle 7): naming only the first field is deterministic and true, but it hides how many others are missing.
Deterministic: yes. A block's paths follow SCHEMA order. A quantity's paths follow the order of its own QUANTITIES tuple, not schema order. The PR body says "in schema order", which is correct for blocks only. Please fix that sentence.
Honest: the named field really is absent, since contributions is empty (subject to the tokenizer case on L206). Every field in the schema except provenance.fragments and provenance.notes is required. So on a spec that was read, this line only ever names one of those two single fields, and nothing is hidden there.
What the caller does not learn: on an assembled spec, every field under the term is absent, but the refusal names one. For kv_blocks, the seven fields come from more than one fragment. The caller merges the fragment for field 1 and asks again. explain then returns a partial basis without refusing: this is the "left undone" item. I measured it on node 18 with device.memory.capacity_bytes dropped from a resolved spec: kv_blocks returns 12 rows and no refusal. So the other N−1 absences are never shown, first because only field 1 is named and then because the partial basis is silent.
Keeping refuse_absent_field byte-identical to value() is the right trade here (principle 3). I would not widen the message in this PR. The follow-up for the partial-basis case should also decide whether a refusal lists every absent field under the term.
There was a problem hiding this comment.
Agreed, and the PR body is corrected. A block's paths follow SCHEMA order, and a quantity's follow its own QUANTITIES tuple. The code is unchanged here: the message stays byte-identical to value()'s. Whether a refusal should name every absent field under a term goes to the partial-basis follow-up, alongside the silent partial kv_blocks basis you measured.
Review, cycle 1: REQUEST_CHANGES at head
|
| variant | mutation | result | failing node ids (tests/compass/…) |
|---|---|---|---|
| N1 | none | 284 passed | none |
| N2 | docstring word, L38 | 284 passed | none |
| M1 | L195 ADDRESSING→TOTALITY; L206 → inline SpecRefusal(Rule.ADDRESSING, <optional text>) |
4 failed | test_spec_schema.py::test_explain_declines_an_unknown_term_and_an_absent_one_at_two_sites, test_spec_verbs.py::test_a_term_the_schema_does_not_know_is_asked_for_again_by_path (TOTALITY is ADDRESSING), test_spec_verbs.py::test_an_optional_field_the_document_left_out_is_refused_as_absent (ADDRESSING is TOTALITY), test_spec_verbs.py::test_a_block_an_assembled_spec_holds_nothing_under_names_its_first_field |
| M1a | only L195 swapped | 2 failed | the placement test; test_a_term_the_schema_does_not_know_is_asked_for_again_by_path |
| M1b | only the absent half: rules.py L145/L153 TOTALITY→ADDRESSING, texts kept |
7 failed | the placement test, both new absent-case verbs tests, and four #183 tests (test_a_declared_field_a_spec_lacks_is_never_called_undeclared, test_the_three_refusals_are_not_interchangeable, test_an_optional_field_the_document_omits_is_refused_as_optional, test_the_other_accessors_decline_on_a_fragment_rather_than_raise) |
| M2 | L198 remedy → the optional remedy; L206 → inline SpecRefusal(Rule.TOTALITY, <optional what>, "ask again by…") |
4 failed | the placement test (('explain.py', 206) == ('rules.py', 152)), plus the three verbs tests on their remedy assertions: 'ask again by the whole dotted path' in …, 'write it in the document' in … and 'merge the fragment' in … |
| M3f | L206 .required→False |
1 failed | test_spec_verbs.py::test_a_block_an_assembled_spec_holds_nothing_under_names_its_first_field ('merge the fragment' in …) |
| M3t | L206 .required→True |
2 failed | the placement test (('rules.py', 144) == ('rules.py', 152)), test_spec_verbs.py::test_an_optional_field_the_document_left_out_is_refused_as_absent ('is declared by this schema as optional' in …) |
| M4 | L195 ADDRESSING→SHAPE |
3 failed | test_spec_schema.py::test_every_site_that_declines_a_document_is_driven_here, the placement test, test_a_term_the_schema_does_not_know_is_asked_for_again_by_path |
This matches the developer's M1, M2 and M3. M3t adds that the optional branch is pinned too.
Gate 1: the tree that will land
- The integration tip, re-read at the start of this review, is
815a0867981a07156a3bfffcc162246e4844bc2c, unmoved. git merge-tree --write-tree 815a0867981a07156a3bfffcc162246e4844bc2c da6f12036146e610d9ef8989d7360ba1f3713c96gavebc801abaef5303382eb78e78b6e214a33027ac3d, rc=0. That equals the head tree, so it agrees with the developer.scripts/compass/gate_cpu.shwas run once on that tree, on node 18 (xiaobizh_n18_cpu):- it used the tree's own scripts;
- stamps:
.compass-commit= the head,.compass-changed= the three changed files; atom.__file__ = /tmp/pr264rev/n1/ATOM/atom/__init__.py;- bounded by
timeout -k 10 1500, unpiped.
- The gate printed
commit: da6f12036 (stamp)and then 5059 passed, 149 skipped, 3 xfailed,GATE_CPU_RC=0, in 175 s. The load average on the box was about 96. - This agrees with the developer's branch run: +3 against the control's 5056.
- The junit XML confirms that the four new node ids are present and the removed one is absent. No
test_stream_marker_properties.pyflake fired. - Staging was removed afterwards.
For the next task in this area
explain's "no rows" is not "no field"._rowscan return nothing for a field that is present, and that is the case to test whenever this refusal is touched again.- The follow-up issue (partial basis, and naming every absent field) is recommended above. I have not filed it; that is the coordinator's call.
No other blocking issues.
… term is held The absent-field refusal fired whenever a term produced no rows. A field that is present can produce none: a tokenizer table read with no entries. There explain called a spec that was read "assembled from parts" and disagreed with MachineSpec.value, which answers (). The refusal now fires only when no path under the term is in the spec, so the field it names really is absent. A held field with nothing in it explains to an empty basis, and a test drives that on a read spec. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Developer round 1: response to REQUEST_CHANGES at
|
| variant | result | failing node ids |
|---|---|---|
| tip | 281 passed | none |
| N1, N2 (null controls) | 285 passed each | none |
| M1 | 4 failed | the placement test and the three case tests, each on its rule is assertion |
| M1a | 2 failed | the placement test; test_a_term_the_schema_does_not_know_is_asked_for_again_by_path |
| M1b | 7 failed | the placement test, both absent-case verbs tests, and the four #183 tests |
| M2 | 4 failed | the placement test (site moved), and the three remedy assertions |
| M3f | 1 failed | test_a_block_an_assembled_spec_holds_nothing_under_names_its_first_field |
| M3t | 2 failed | the placement test; test_an_optional_field_the_document_left_out_is_refused_as_absent |
| M4 | 3 failed | test_every_site_that_declines_a_document_is_driven_here, the placement test, test_a_term_the_schema_does_not_know_is_asked_for_again_by_path |
| M5 (refuse on zero rows again) | 1 failed | test_an_empty_tokenizer_table_explains_to_no_rows_rather_than_refusing |
Full node ids and the assertion each failed on are in the PR body.
Gate 1 (node 18, stamped, the tree's own scripts, atom.__file__ under each staged root, one run at a time, unpiped):
- The tip
175739f87gave 5107 / 149 / 3,GATE_CPU_RC=0. - The merged tree
2a1435be8402fa2e8f6255df38d7aa0f93f1b326gave 5111 / 149 / 3,GATE_CPU_RC=0. - Node ids: 1 removed (
test_a_term_this_spec_carries_nothing_for_is_refused), 5 added, 0 outcome changes. No timing flake fired. ruffandblackrc=0.
Lines vs estimate:
- production +19/−8 against ~40;
- test +48/−3 against ~60.
No other blocking issues known. Ready for review cycle 2.
Review, cycle 2: APPROVE at head
|
| variant | mutation (L203 unless stated) | result | failing node ids |
|---|---|---|---|
| N1 | none | 285 passed | none |
| N2 | docstring word, L38 | 285 passed | none |
| M5 | if not any(_rows(spec, path, tp_width, origin) for path in paths if path in spec.values): (refuse on zero rows again) |
1 failed | tests/compass/test_spec_verbs.py::test_an_empty_tokenizer_table_explains_to_no_rows_rather_than_refusing, raising the cycle-1 false TOTALITY text |
| M6 | if False: (guard removed) |
3 failed | tests/compass/test_spec_schema.py::test_explain_declines_an_unknown_term_and_an_absent_one_at_two_sites, tests/compass/test_spec_verbs.py::test_an_optional_field_the_document_left_out_is_refused_as_absent, tests/compass/test_spec_verbs.py::test_a_block_an_assembled_spec_holds_nothing_under_names_its_first_field |
M5 matches the developer's result: that one test fails and nothing else does. M6 is my addition. It shows the new guard is pinned in the other direction too: without it, an absent term would come back as an empty basis.
Gate 1: the tree that will land
- The tip was re-read before and after the gate:
175739f87fc53bf24190315641f00b4a07535400, unmoved. git merge-tree --write-tree 175739f87 47e40e05d1819c85ce9ee1a1bed9f6aa6575bbc2gave2a1435be8402fa2e8f6255df38d7aa0f93f1b326, rc=0. This is the same as the developer's.- One run of
scripts/compass/gate_cpu.shwith these settings:- the tree's own scripts;
- stamps written;
atom.__file__ = /tmp/pr264rev2/n1/ATOM/atom/__init__.py;- bounded by
timeout -k 10 1500; - unpiped.
- The gate printed
commit: 47e40e05d (stamp)and then 5111 passed, 149 skipped, 3 xfailed,GATE_CPU_RC=0, in 391 s, at a load average of about 150. - This agrees with the developer's 5111, which is +4 over the tip's 5107.
- The junit XML shows
test_an_empty_tokenizer_table_explains_to_no_rows_rather_than_refusingpresent andtest_a_term_this_spec_carries_nothing_for_is_refusedabsent. - No
test_stream_marker_properties.pyflake fired. - Staging was removed afterwards.
Both inline threads from cycle 1 are addressed by 47e40e05d.
Closes #259
What changed
explain(spec, term)raised oneSpecRefusal(Rule.SHAPE, …)whenever the basis came back empty. Neither case it covered is a document to correct, so neither isSHAPE, and its remedy (a re-ask) is wrong for one of them. It is now split:explain(spec, "device.clock_ceiling"))SHAPE: the document has the shape the schema declares: this spec carries nothing named 'device.clock_ceiling'. name a field or a block of fields by its dotted path, or one of the quantities ['admission', 'collective', 'kv_blocks', 'kv_transfer']ADDRESSING: a field is asked for by the path of the field itself: `device.clock_ceiling` is not a field, a block of fields or a quantity this schema knows. ask again by the whole dotted path of a field or a block of fields, or by one of the quantities ['admission', 'collective', 'kv_blocks', 'kv_transfer']explain(spec, "provenance.notes"))SHAPEmessage, with'provenance.notes'TOTALITY: a spec resolves every field the document was required to state: `provenance.notes` is declared by this schema as optional, and this spec states no value for it. write it in the document if a reader needs it; a field the document leaves out is left out here rather than inventedMachineSpec(values, tokenizers)constructor (explain(assembled, "host.ipc"))SHAPEmessageTOTALITY: …: `host.ipc.zmq_roundtrip_s` is declared by this schema, and this spec carries no value for it. a spec read with `MachineSpec.from_mapping` resolves every required field, so this one was assembled from parts; merge the fragment that measures this field before asking for ithost.tokenizers: []on a read specSHAPEmessageBasis(host.tokenizers, from spec <digest>), matchingspec.value("host.tokenizers") == ()The change is in
atom/compass/spec/explain.py:pathsmeans the schema does not know the term, so that check runs first and raisesADDRESSING.spec.values, it callsrefuse_absent_field(paths[0], BY_PATH[paths[0]].required).Decisions the brief did not cover
refuse_absent_field; there is no second TOTALITY site. It is the refusalMachineSpec.valuealready gives, which is how compass(spec): name which of the two ways a path failed to resolve #183 splitvalue(). So for a declared field,explain(spec, p)andspec.value(p)give the same answer: byte-identical text when absent (asserted), and an answer from both when present, including the empty tokenizer table (asserted). They do disagree on an undeclared key that is a deployment knob:explain(spec, "block_size")isADDRESSING, whilespec.value("block_size")isSEPARATION("remove it; EngineArgs.block_size carries it").explainputs nothing in a document, so "remove it" would be the wrong remedy there. The one new site is the ADDRESSING one inexplain.py. The TOTALITY site is the existing optional/required pair inrules.py, and the placement test pins explain's refusal to it.MachineSpec(values, tokenizers)can lack a required field.test_kv_simulated_connector.pybuilds one that way.validate's own stand-in spec never leavesvalidate, so it does not reachexplain. The field's declaration decides which of the two messages applies. A message hard-wired to say "optional" would have been false for this case.host.tokenizers: []reads throughfrom_mapping, andvalue()returns(). Refusing it as absent would claim the spec was assembled and ask for a fragment, which is false on every count.explainreturns an empty basis instead: it truthfully says that no tokenizer was measured. The guard checks presence, not the row count, so the fieldrefuse_absent_fieldnames is always really absent.SCHEMAorder; a quantity's follow its ownQUANTITIEStuple. On a spec that was read, onlyprovenance.fragmentsandprovenance.notescan be absent, so nothing is hidden there. On an assembled spec, the other absent fields under the term go unnamed. That belongs to the follow-up below.Partition
ELSEWHERE.test_every_site_that_declines_a_document_is_driven_herestill drives onlySHAPEsites, and now guarantees that neither new case can drift back toSHAPE(M4 below).test_explain_declines_an_unknown_term_and_an_absent_one_at_two_sites. It places the unknown-term site inexplain.pyunderADDRESSING, and the absent-term site at the accessor's own site underTOTALITY. Neither may be underSHAPE.Named result: each case has its own test with its own rule and remedy, and each test bites
The runs were on node 18 (
xiaobizh_n18_cpu), on the merged tree2a1435be8(tip175739f87+ head47e40e05d), overtests/compass/test_spec_schema.pyandtests/compass/test_spec_verbs.py. Every tree was staged bygit archive+docker exec -i tar(md5 matched on both ends), andatom.__file__resolved under each staged root. Every mutation keeps the line counts:explain.py209,rules.py176.tests/compass/…), and the assertion each failed on175739f87test_spec_schema.py::test_explain_declines_an_unknown_term_and_an_absent_one_at_two_sites(unknown in _sites_naming("ADDRESSING"));test_spec_verbs.py::test_a_term_the_schema_does_not_know_is_asked_for_again_by_path(rule is Rule.ADDRESSING);test_spec_verbs.py::test_an_optional_field_the_document_left_out_is_refused_as_absentandtest_spec_verbs.py::test_a_block_an_assembled_spec_holds_nothing_under_names_its_first_field(rule is Rule.TOTALITY)TOTALITYonlytest_a_term_the_schema_does_not_know_is_asked_for_again_by_pathrules.pyabsent-field rule →ADDRESSING(both branches)absent in _sites_naming("TOTALITY")); both absent-case verbs tests; and four #183 tests:test_a_declared_field_a_spec_lacks_is_never_called_undeclared,test_the_three_refusals_are_not_interchangeable,test_an_optional_field_the_document_omits_is_refused_as_optional,test_the_other_accessors_decline_on_a_fragment_rather_than_raise"ask again by the whole dotted path","write it in the document"and"merge the fragment".required→Falsetest_a_block_an_assembled_spec_holds_nothing_under_names_its_first_field("merge the fragment").required→Truerules.py:144≠:152);test_an_optional_field_the_document_left_out_is_refused_as_absent("as optional")SHAPEtest_spec_schema.py::test_every_site_that_declines_a_document_is_driven_here, the placement test,test_a_term_the_schema_does_not_know_is_asked_for_again_by_pathif not any(_rows(...) for p in paths if p in spec.values)test_spec_verbs.py::test_an_empty_tokenizer_table_explains_to_no_rows_rather_than_refusing(raisesSpecRefusal)Gates
Gate 1 (
scripts/compass/gate_cpu.sh, node 18, one run at a time,timeout -k 10 1500, output not piped):GATE_CPU_RC175739f872a1435be8402fa2e8f6255df38d7aa0f93f1b326=git merge-tree --write-tree 175739f87 47e40e05d(rc=0)tests/compass/test_spec_verbs.py::test_a_term_this_spec_carries_nothing_for_is_refusedtests/compass/test_spec_schema.py::test_explain_declines_an_unknown_term_and_an_absent_one_at_two_sites,tests/compass/test_spec_verbs.py::test_a_term_the_schema_does_not_know_is_asked_for_again_by_path,tests/compass/test_spec_verbs.py::test_an_optional_field_the_document_left_out_is_refused_as_absent,tests/compass/test_spec_verbs.py::test_a_block_an_assembled_spec_holds_nothing_under_names_its_first_field,tests/compass/test_spec_verbs.py::test_an_empty_tokenizer_table_explains_to_no_rows_rather_than_refusingtest_stream_marker_properties.pyfired.scripts/compass, with.compass-commitand.compass-changedwritten. The printedcommit:stamps were175739f87and47e40e05d. The staging was removed afterwards.da6f12036against tip815a08679, gave 5056 → 5059, RC 0/0.Gate 2: four verbs tests and one placement test, CPU-only.
ruff checkandblack --checkgive rc=0 on all three files at the merged tree.Lines (non-blank, against the tip):
explain.py: 11 lines of code (1 of them the changed import) and 8 lines of docstringtest_spec_verbs.py+41/−2,test_spec_schema.py+7/−1What surprised me
_rowsreturns nothing for a tokenizer table that is present but empty. The first head refused that case with a message whose every clause was false. The reviewer caught it, and M5 now pins it.provenance: every other block has at least one required field.blackin the CPU container uses a line length above 88.Left undone (out of this brief)
device.memory.capacity_bytesdropped,kv_blocksreturns 12 rows and no refusal. The same follow-up should decide whether a refusal lists every absent field under the term. The coordinator is filing it.🤖 Generated with Claude Code