Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
40 changes: 30 additions & 10 deletions atom/compass/spec/explain.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,18 @@
different actions. A term the schema does not know is asked for again by a real
path. A term the schema does know, with no field under it that this spec holds,
would come back empty however it was spelled, so it is refused as the absent
field it is -- the same refusal `MachineSpec.value` gives for that field, and
for a term naming several fields, for the first of them. A field the spec does
hold is never refused here, even when it holds nothing: a tokenizer table with
no entries explains to no rows, as `MachineSpec.value` answers it with none.
field it is -- the same refusal `MachineSpec.value` gives for that field.

A term naming several fields is refused when any required field under it is
absent, and the refusal names every one of them. Explaining from the fields
that are there would present part of a basis as the whole of it. An optional
field the document left out is not missing, so a block holding one still
explains from the rest.

A field the spec does hold is never refused here, even when it holds nothing:
a tokenizer table with no entries explains to no rows, as `MachineSpec.value`
answers it with none. It is still printed, as the empty value it holds, so a
reader can tell a field that contributed nothing from one that was left out.
"""

from collections.abc import Mapping
Expand All @@ -52,7 +60,7 @@
from .fields import BY_PATH, SCHEMA, Kind
from .machine import MachineSpec
from .merge import Merge, entry_label
from .rules import Rule, SpecRefusal, refuse_absent_field
from .rules import Rule, SpecRefusal, refuse_absent_field, refuse_absent_fields
from .tokenizers import ENTRY_FIELDS

#: Predicted quantities, and the spec fields each one is built out of.
Expand Down Expand Up @@ -114,11 +122,14 @@ class Basis:
term: str
digest: str
contributions: tuple[Contribution, ...]
#: Fields the spec holds that contribute no rows, printed so as not to
#: read as fields left out.
empty: tuple[Contribution, ...] = ()

def __str__(self) -> str:
return "\n".join(
[f"{self.term}, from spec {self.digest}"]
+ [f" {contribution}" for contribution in self.contributions]
+ [f" {row}" for row in self.contributions + self.empty]
)


Expand Down Expand Up @@ -200,10 +211,19 @@ def explain(
"ask again by the whole dotted path of a field or a block of fields, "
f"or by one of the quantities {sorted(QUANTITIES)}",
)
if not any(path in spec.values for path in paths):
refuse_absent_field(paths[0], BY_PATH[paths[0]].required)
absent = [path for path in paths if path not in spec.values]
missing = tuple(path for path in absent if BY_PATH[path].required)
if missing:
refuse_absent_fields(missing)
if len(absent) == len(paths):
refuse_absent_field(paths[0], False)
contributions: list[Contribution] = []
empty: list[Contribution] = []
for path in paths:
if path in spec.values:
contributions += _rows(spec, path, tp_width, origin)
return Basis(term, spec.digest(), tuple(contributions))
rows = _rows(spec, path, tp_width, origin)
contributions += rows
if not rows:
held = spec.values[path]
empty.append(_row(path, held, None, _supplied(origin, path)))
return Basis(term, spec.digest(), tuple(contributions), tuple(empty))
35 changes: 27 additions & 8 deletions atom/compass/spec/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,14 +141,7 @@ def refuse_absent_field(path: str, required: bool) -> NoReturn:
in the document.
"""
if required:
raise SpecRefusal(
Rule.TOTALITY,
f"`{path}` 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",
)
refuse_absent_fields((path,))
raise SpecRefusal(
Rule.TOTALITY,
f"`{path}` is declared by this schema as optional, and this spec states "
Expand All @@ -158,6 +151,32 @@ def refuse_absent_field(path: str, required: bool) -> NoReturn:
)


def refuse_absent_fields(paths: tuple[str, ...]) -> NoReturn:
"""Decline required fields this spec holds no value for, naming every one.

The required half of `refuse_absent_field`, which reads one path through
here, so a question over several fields and a question over one are
declined in the same words. Every missing field is named rather than the
first: each is a separate fragment to merge, and a refusal naming one of

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.

Nit, not blocking (principle 8: a claim carries its measurement). "each is a separate fragment to merge" is stated as fact, and it is not always true. A fragment is a partial document (merge.py module docstring), and one fragment can carry several fields. For example, the tokenizer probe writes host.cpu.cores_physical and host.cpu.cores_logical alongside its table. The reason to name every field does not need that claim. Each missing field needs some fragment to supply it, and a refusal that names only one field sends the reader back once per field. Suggest: "each needs a fragment that measures it, and a refusal naming one of them sends the reader back once per field."

The plural remedy, "merge the fragments that measure these fields", reads fine even when one fragment covers two of the fields. Only the docstring overstates this.

them sends the reader back once per field.
"""
one = len(paths) == 1
named = ", ".join(f"`{path}`" for path in paths)
are, them, measures = (
("is", "it", "fragment that measures this field")
if one
else ("are", "them", "fragments that measure these fields")
)
raise SpecRefusal(
Rule.TOTALITY,
f"{named} {are} declared by this schema, and this spec carries no value "
f"for {them}",
"a spec read with `MachineSpec.from_mapping` resolves every required "
f"field, so this one was assembled from parts; merge the {measures} "
f"before asking for {them}",
)


def refuse_unknown_key(path: str) -> NoReturn:
"""Decline a key the schema does not declare, saying why it is not one."""
owner = DEPLOYMENT_OWNED.get(path.rsplit(".", 1)[-1])
Expand Down
68 changes: 60 additions & 8 deletions tests/compass/test_spec_verbs.py
Original file line number Diff line number Diff line change
Expand Up @@ -1057,27 +1057,79 @@ def test_an_empty_tokenizer_table_explains_to_no_rows_rather_than_refusing():
assert spec.value("host.tokenizers") == ()
basis = explain(spec, "host.tokenizers")
assert basis.contributions == ()
assert str(basis) == f"host.tokenizers, from spec {spec.digest()}"
assert str(basis).splitlines() == [
f"host.tokenizers, from spec {spec.digest()}",
" host.tokenizers = ()",
]


def test_a_block_an_assembled_spec_holds_nothing_under_names_its_first_field():
# A spec built from parts can lack required fields too, and then the
# absent field is required, so the remedy is in whatever assembled it.
def test_an_empty_tokenizer_table_is_printed_as_empty_under_a_quantity():
# Three rows from the other fields and none from the table. Without its own
# line the table reads as a field the quantity does not use.
document = copy.deepcopy(merged().document)
document["host"]["tokenizers"] = []
basis = explain(MachineSpec.from_mapping(document), "admission")
assert len(basis.contributions) == 3
printed = str(basis).splitlines()
assert len(printed) == 5
assert printed[-1] == " host.tokenizers = ()"


def assembled_without(*paths):
"""The resolved spec assembled from parts, with the named fields left out."""
whole = resolved()
spec = MachineSpec(
values={p: v for p, v in whole.values.items() if not p.startswith("host.ipc.")},
return MachineSpec(
values={p: v for p, v in whole.values.items() if p not in paths},
tokenizers=whole.tokenizers,
)


def test_a_quantity_missing_one_field_is_refused_rather_than_explained_from_the_rest():
# The other six fields still explain to twelve rows, which read as a whole
# basis for a number that cannot be computed. The refusal is the accessor's.
spec = assembled_without("device.memory.capacity_bytes")
with pytest.raises(SpecRefusal) as refused:
explain(spec, "kv_blocks")
with pytest.raises(SpecRefusal) as accessed:
spec.value("device.memory.capacity_bytes")
assert refused.value.rule is Rule.TOTALITY
assert str(refused.value) == str(accessed.value)

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.

Pin gap, not blocking (principle 8). This assertion compares explain with spec.value. Both now build their text in the same refuse_absent_fields, so the comparison cannot fail on wording. It proves the two sides share a site. It does not prove the singular text is what it was.

I measured this on node 18 with a line-count-preserving mutation of rules.py L166 (195 → 195 lines), run over test_spec_verbs.py and test_spec_schema.py:

("is", "it", "fragment that measures this field") → ("is", "it", "fragment that measures that field"): 289 passed.

So the singular remedy is pinned by nothing. The PR body names two schema tests as holding the singular text. Those tests assert the what, and a what mutation does redden test_a_declared_field_a_spec_lacks_is_never_called_undeclared. They do not assert the remedy.

The text is byte-identical today. I checked all 39 fields × {required, optional} with the old rules.py from 77d203b86 against the new one: 78/78 identical, including refuse_absent_fields((p,)). So nothing is wrong now. The gap only means a later edit could change the wording silently. One line here closes it:

assert refused.value.remedy.endswith("merge the fragment that measures this field before asking for it")



def test_every_missing_field_under_a_quantity_is_named_and_not_only_the_first():
spec = assembled_without("host.admission_fixed_s", "host.ipc.shm_broadcast_s")
with pytest.raises(SpecRefusal) as refused:
explain(spec, "admission")
assert refused.value.rule is Rule.TOTALITY
assert refused.value.what.startswith(
"`host.admission_fixed_s`, `host.ipc.shm_broadcast_s` are declared by "
"this schema, and this spec carries no value for them"
)
assert "fragments that measure these fields" in refused.value.remedy


def test_a_block_an_assembled_spec_holds_nothing_under_names_every_field():
# A spec built from parts can lack required fields too, and then the
# absent field is required, so the remedy is in whatever assembled it.
spec = assembled_without("host.ipc.zmq_roundtrip_s", "host.ipc.shm_broadcast_s")
with pytest.raises(SpecRefusal) as refused:
explain(spec, "host.ipc")
assert refused.value.rule is Rule.TOTALITY
assert "`host.ipc.zmq_roundtrip_s` is declared by this schema" in (
assert "`host.ipc.zmq_roundtrip_s`, `host.ipc.shm_broadcast_s` are declared" in (
refused.value.what
)
assert "merge the fragment" in refused.value.remedy
assert "merge the fragments" in refused.value.remedy
assert "optional" not in refused.value.what


def test_a_block_explains_without_an_optional_field_the_document_left_out():
spec = resolved()
assert "provenance.notes" not in spec.values
paths = {row.path for row in explain(spec, "provenance").contributions}
assert "provenance.authored_by" in paths
assert "provenance.notes" not in paths


def test_the_basis_prints_its_spec_and_one_line_per_field():
combination = merged()
spec = MachineSpec.from_mapping(combination.document)
Expand Down