diff --git a/atom/compass/spec/memory.py b/atom/compass/spec/memory.py index e56743e78a..85430fdef8 100644 --- a/atom/compass/spec/memory.py +++ b/atom/compass/spec/memory.py @@ -102,7 +102,16 @@ Four checks, then, narrowing: whether one rank's readings describe a card at all, whether the cache that rank built was sized by its own budget, the ranks against each other, and the one number that survives them. A contaminated run -can fail more than one, and the one raised is the first in that order. +can fail more than one, and **each check is asked of every rank before the next +check is asked**, so the one raised is the first in that order. Asking them the +other way round -- both questions of rank 0, then both of rank 1 -- would make +the refusal a function of the order the ranks were listed in: two readings, one +that is no card and one whose cache a neighbour sized, earn a different refusal +depending on which of them is rank 0. The two name different things to repair, +so a run sent after the wrong one can fix what it was told to fix and see the +same refusal again. The rank a refusal names is still the first that failed the +check that fired, which is a question about which reading came in where and has +no other answer. One difference from the engine's own arithmetic is deliberate: the engine floors this term at zero and this does not. A floor turns an impossible reading into a @@ -204,6 +213,10 @@ def non_torch_across_ranks( "out plausible whether or not the readings themselves can be " "true together", ) + # Every rank is asked the first question before any is asked the second, so + # which of the two a run is refused by is decided by the readings and not by + # the position a failing rank happened to arrive in. + for rank, reading in enumerate(readings): if reading.free_was_binding: raise SpecRefusal( Rule.DEVICE_WIDE, diff --git a/atom/compass/spec/validate.py b/atom/compass/spec/validate.py index cdd085d1b0..264898aa10 100644 --- a/atom/compass/spec/validate.py +++ b/atom/compass/spec/validate.py @@ -147,9 +147,16 @@ #: about -- and a question registered as reading a field it cannot speak for #: reports a run that could not ask it and a run that asked and found nothing #: as the same record. Derived from `FILLED_BY`, so a probe given the width -#: that has none empties this with no second list to remember. +#: that has none empties this with no second list to remember. A table no +#: probe is named for at all contributes nothing here rather than failing the +#: derivation: this runs while the module is being imported, so a subscript +#: would answer a table nobody has entered yet by denying every caller of the +#: package, including the ones with no interest in probes, and the reader would +#: be told which import failed rather than which term is missing. The naming is +#: left to `probe_for`, which refuses such a term by name, and the two lists +#: are held against each other by a test. PROBE_TABLES = tuple( - path for path in WIDTH_TABLES if None in FILLED_BY[path.rsplit(".", 1)[-1]] + path for path in WIDTH_TABLES if None in FILLED_BY.get(path.rsplit(".", 1)[-1], ()) ) STACK_PINS = tuple(f"{PINNED_BLOCK}.{component}" for component in PINNED) diff --git a/tests/compass/test_spec_verbs.py b/tests/compass/test_spec_verbs.py index 40233bd841..0d52a064bb 100644 --- a/tests/compass/test_spec_verbs.py +++ b/tests/compass/test_spec_verbs.py @@ -28,6 +28,7 @@ """ import copy +import importlib.util import pytest @@ -48,7 +49,9 @@ tokenizer_fragment, validate, ) -from atom.compass.spec.fields import BY_PATH, Kind +from atom.compass.spec import fields as schema_module +from atom.compass.spec import probes as probes_module +from atom.compass.spec.fields import BY_PATH, Field, Kind from atom.compass.spec.probes import FILLED_BY, cpu_counts from atom.compass.spec.tokenizers import ENTRY_FIELDS from atom.compass.spec.validate import ( @@ -1408,6 +1411,40 @@ def test_a_rank_whose_free_memory_was_binding_is_refused_and_named(): assert "92100000000.0" in refused.value.what +def test_which_refusal_a_pair_of_readings_earns_is_not_decided_by_rank_order(): + # One rank read numbers no card produced; the other built its cache out of + # the gap a neighbour left. Each fails a different one of the two per-rank + # checks, so the pair earns both -- and the two name different things to + # repair: a reading to take again, against a card to take it on. Every rank + # is asked the first question before any is asked the second, so the pair + # earns the same refusal whichever of them arrived first. Asked rank by + # rank instead, whichever was listed first would decide the remedy, and a + # run sent after the wrong one repairs it and is refused again. + binding = reading(200.0e9) + impossible = DeviceMemory( + free_bytes=400.0e9, + total_bytes=CAPACITY, + reserved_bytes=-120.0e9, + kv_budget_bytes=1.0, + ) + assert binding.free_was_binding and not binding.impossible + assert impossible.impossible and not impossible.free_was_binding + for ranks, first_to_fail in ( + ([binding, impossible], "rank 1"), + ([impossible, binding], "rank 0"), + ): + with pytest.raises(SpecRefusal) as refused: + non_torch_across_ranks(2, ranks, 7.2e9) + assert refused.value.rule is Rule.DEVICE_WIDE + assert "which is not a reading of a card" in refused.value.what + assert "set the cache size" not in refused.value.what + assert "take them again and find out" in refused.value.remedy + # Which check fired is the readings'; the rank it names is still the + # first that failed that check, which is a question about where in the + # sequence the reading came in and has no other answer. + assert first_to_fail in refused.value.what + + def test_a_reading_the_engine_would_floor_at_zero_is_refused_here(): # The engine floors this term at zero. A probe does not: a floor turns an # impossible reading into a plausible one. @@ -1625,6 +1662,75 @@ def test_the_probe_question_reads_only_the_tables_a_probe_can_fall_short_on(): } +def reimported_validate(name): + """`validate` imported again, the way a session imports it the first time. + + The probe tables are derived once, while the module is being imported, so a + test about that derivation has to import the module rather than call + something in it. This loads a second instance out of the same file under a + name of its own, and does not register it, so the instance the rest of the + suite is holding is the one it started with. + """ + location = importlib.util.find_spec("atom.compass.spec.validate").origin + loaded = importlib.util.spec_from_file_location( + f"atom.compass.spec.{name}", location + ) + module = importlib.util.module_from_spec(loaded) + loaded.loader.exec_module(module) + return module + + +def test_a_width_table_no_probe_is_named_for_is_refused_and_not_an_import_error( + monkeypatch, +): + # A width-keyed constant written into the schema and not into the probe + # table is the ordinary shape of a half-finished change, and the two lists + # are held together by a test for exactly that reason. The derivation runs + # while the module is being imported, so a table it could not answer for + # would deny every caller of the package instead -- and that test would + # fail at collection, as an import error naming the test session rather + # than the term nobody entered. + added = Field("device.runtime_constants.graph_replay_pool_bytes", Kind.WIDTH_TABLE) + monkeypatch.setattr(schema_module, "SCHEMA", schema_module.SCHEMA + (added,)) + under_test = reimported_validate("validate_with_a_table_no_probe_is_named_for") + assert added.path in under_test.WIDTH_TABLES + # A table with no entry has no hole to report, so it is not a probe table. + assert added.path not in under_test.PROBE_TABLES + # The term is named where a caller asks about it, by the refusal this + # package exists to give. + with pytest.raises(SpecRefusal) as refused: + probe_for("graph_replay_pool_bytes", 1) + assert refused.value.rule is Rule.NO_DEFAULTS + assert "`graph_replay_pool_bytes` is not one of the constants" in refused.value.what + # And what the test holding the two lists together now sees is its own + # comparison, failing on the term that is missing from the table. + assert set(FILLED_BY) != { + path.rsplit(".", 1)[-1] for path in under_test.WIDTH_TABLES + } + + +def test_a_probe_given_the_width_that_has_none_empties_the_probe_tables(monkeypatch): + # The property the derivation exists for, and the one a second list would + # lose: give the hole a probe and the question has nothing left to ask + # about, with nothing else to edit. The run still counts it as asked -- it + # is not a question this run could not reach -- and it finds nothing to say. + monkeypatch.setitem( + FILLED_BY, + "allocator_retained_after_load_bytes", + (probes_module.SINGLE_CARD, probes_module.MULTI_RANK), + ) + under_test = reimported_validate("validate_with_every_width_filled") + assert under_test.PROBE_TABLES == () + checked = under_test.validate(merged().document, tp_widths=(16,)) + assert not checked.ok + assert not any( + condition.startswith(PROBES_ASKED) for condition in checked.not_asked + ) + assert not any( + condition.startswith(PROBES_ASKED) for condition in checked.asked_in_part + ) + + def test_the_probe_question_says_it_could_not_be_asked_when_its_table_is_gone(): # The other width table still resolves, so the width question was asked of # part of what it reads. The probe question was not asked at all, and the