ENH: allow dirty trees for local-only benchmarks, part 2

benchx/README.md

@@ -9,8 +9,8 @@ bx run <order.json> [--out DIR] [--no-ingest]
                                          # runner + Google Benchmark adapter → result files → local store
 bx ingest <dir|file>                     # result files and work orders → the local store
 bx series [--workload … --quantity …]    # list series
-bx history <series>                      # one series' points (a query)
-bx head [-n 10] [--project P] [--workload W] [--json|--table]  # the latest results, newest first
+bx history <series>                      # one series' points (a query); dirty ones are marked
+bx head [-n 10] [--project P] [--workload W] [--json|--table]  # the latest results, newest first; dirty ones are marked
 bx compare --run KEY --profile revisions|environments --baseline VALUE
            [--label NAME] [--results DIR] [--k 3] [--json]
                                          # read mode: compare results already measured
@@ -162,16 +162,18 @@ every result is pyperf. Results also record the installed project's version
   Every accepting ingest rewrites the file atomically.
 - **The session loop** (`benchx/session.py`, `bx compare` in measure mode) is
   the workbench's part: it checks both targets (and, for `revisions`, that both
-  trees are clean, before running anything), writes one order per side and
+  trees have a working-tree id, before running anything; it says which sides
+  are not clean), writes one order per side and
   round, alternates them over the rounds, delivers every result to the local
   store, and hands the run to the comparator. It builds nothing: both build
   directories must already exist.
 - **The comparator** (`bx compare --run`) runs in run mode for the `revisions` and
   `environments` profiles, each a projection built in code from the identity
   policy (`benchx/compare.py`): the axis tells the sides apart, everything the
   policy matches must agree, and it checks the profile's invariants (`revisions` also
-  requires clean trees, UC-03 §10; `environments` labels a dirty side
-  local-only), pairs the
+  requires one tree id per side and, for a benchmark recorded apart from the
+  subject, one benchmark file throughout, UC-03 §10; a dirty side makes any
+  profile's comparison local-only), pairs the
   sides by round, and applies `benchx/paired-relative/v1` (effect = mean
   relative paired difference, noise = its standard error, change when
   |effect| > *k* × noise). `environments` names its sides by the label

benchx/benchx/bench.py

@@ -14,6 +14,7 @@
 
 from . import core, snapshot
 from .errors import SessionError
+from .store import warning_codes
 
 
 def tree_of(module: Path, tree=None) -> Path:
@@ -86,8 +87,7 @@ def row_of(document: dict) -> dict:
         "min": min(values) if values else None,
         "max": max(values) if values else None,
         "loops": document.get("procedure", {}).get("inner_iterations"),
-        "warnings": [w.get("code", str(w)) if isinstance(w, dict) else str(w)
-                     for w in document.get("quality", {}).get("warnings", [])],
+        "warnings": warning_codes(document),
     }
 
 

benchx/benchx/cli.py

@@ -20,7 +20,7 @@
 from . import core, harnesses, orders, runner, session, snapshot
 from . import target as target_mod
 from .core.validation import Kind
-from .store import Store
+from .store import Store, warning_codes
 
 EXIT_OK = 0
 EXIT_INVALID = 1  # a document invalid, a result rejected, or a target without a description
@@ -287,7 +287,9 @@ def cmd_history(args):
     print("revision order unavailable without a revision graph; listed by measurement start")
     for p in points:
         changed = f"  changed: {', '.join(p['changed'])}" if p["changed"] else ""
-        print(f"{p['revision'][:12]}  {p['started_at']}  {p['value']:.6g} ({p['source']})  {p['ingest_key']}{changed}")
+        warned = f"  [{', '.join(p['warnings'])}]" if p["warnings"] else ""  # such as dirty-subject-tree
+        print(f"{p['revision'][:12]}  {p['started_at']}  {p['value']:.6g} ({p['source']})  {p['ingest_key']}"
+              f"{changed}{warned}")
     return EXIT_OK
 
 
@@ -311,7 +313,9 @@ def cmd_head(args):
         median = f"{r['median']:.6g} {r['unit']}" if r["median"] is not None else "-"
         started = r["started_at"].strftime("%Y-%m-%d %H:%M:%S")
         labels = f"  {r['labels']}" if r["labels"] else ""
-        cells.append((f"{r['row']:>5}", started, r["run_key"], r["workload"], r["quantity"], r["status"], median,
+        codes = warning_codes(json.loads(r["document"]))  # such as dirty-subject-tree, as `--table` shows them
+        status = r["status"] + (f"  [{', '.join(codes)}]" if codes else "")
+        cells.append((f"{r['row']:>5}", started, r["run_key"], r["workload"], r["quantity"], status, median,
                       r["revision"][:10], f"{r['project']}{labels}"))
     widths = [max([len(cell[i]) for cell in cells], default=0) for i in range(len(cells[0]))] if cells else []
     for cell in cells:

benchx/benchx/compare.py

@@ -16,6 +16,7 @@
 from . import identity
 from .core import canonical, catalog
 from .projection import Projection, get
+from .snapshot import DIRTY_WARNINGS
 from .store import store_project
 
 METHOD = {"name": "benchx/paired-relative", "version": "v1", "effect_measure": "relative"}
@@ -24,7 +25,7 @@
     "revisions": Projection(identity.POLICY_DATA, ["/provenance/subject_tree"],
                             profile="benchx/profile/revisions/v1",
                             invariants=["one-run", "two-sides", "one-tree-per-side", "equal-attempts",
-                                        "alternating-slots", "clean-tree"]),
+                                        "alternating-slots", "one-benchmark-tree"]),
     # UC-04: one revision measured two ways. A side is what its subject
     # configuration, pinned components, and system are; the caller's label,
     # put in front of the axis per comparison, only names it.
@@ -109,8 +110,14 @@ def compare(documents, *, run_key, profile, baseline, label=None, k=3.0, min_rou
     failed = doc["failed_invariants"]
     projected = [projection.apply(d, store_project(d)) for d in documents]
     if any(p.axis[0] is None for p in projected):
-        failed.append({"invariant": "sides", "detail": f"a result has no {varying}: give every bx bench call "
-                                                       "--label NAME=VALUE, and name that label here"})
+        if profile == "revisions":
+            # UC-03 §10: a side with no working-tree id (its dirty state is `unknown`) cannot be named.
+            detail = (f"a result has no {varying}: its tree's dirty state is unknown, so the side cannot be told "
+                      "apart from the other or checked for a change during the run. Make every file in the "
+                      "tree readable, or git-ignore it")
+        else:
+            detail = f"a result has no {varying}: give every bx bench call --label NAME=VALUE, and name that label here"
+        failed.append({"invariant": "sides", "detail": detail})
         return doc
 
     # A side is a distinct axis value, named by its first part (a tree, a label).
@@ -149,12 +156,8 @@ def compare(documents, *, run_key, profile, baseline, label=None, k=3.0, min_rou
         if any(len(t) != 1 or None in t for t in trees.values()) or trees[base] == trees[contender]:
             failed.append({"invariant": "one-tree-per-side", "detail": "each side must have one tree id, and "
                                                                       "the two must differ"})
-    if "clean-tree" in check:
-        # UC-03 §10: a dirty or unknown tree cannot be validated (schema §5.5).
-        unclean = sorted({str(names[s]) for d, s in zip(documents, sides)
-                          if any(d["provenance"].get(f) != "clean" for f in ("subject_dirty", "benchmark_dirty"))})
-        if unclean:
-            failed.append({"invariant": "clean-tree", "detail": f"sides with a dirty or unknown tree: {unclean}"})
+    if "one-benchmark-tree" in check:
+        failed.extend(_benchmark_file(documents, names, sides))
     if "alternating-slots" in check:
         slots = {}
         for d, s in zip(documents, sides):
@@ -219,6 +222,42 @@ def compare(documents, *, run_key, profile, baseline, label=None, k=3.0, min_rou
     return doc
 
 
+def _benchmark_file(documents, names, sides) -> list[dict]:
+    """The `one-benchmark-tree` invariant (schema §5.5, UC-03 §10). A benchmark recorded apart from
+    the subject has its own `benchmark_tree`, which differs from `subject_tree`; it must be one file
+    on both sides and in every round. A benchmark in the subject's checkout has `benchmark_tree`
+    equal to `subject_tree` and differs between the sides as the subject does, so it is not
+    checked. A result without a `benchmark_tree` (a thin result) is outside the rule.
+
+    An in-tree record must also name the subject's source as its benchmark's: one that does not is
+    malformed. The reverse is not required, since a suite in another checkout of the same
+    repository has the subject's source URI but its own file id."""
+    found, malformed = [], []
+    for d, side in zip(documents, sides):
+        provenance = d["provenance"]
+        tree = provenance.get("benchmark_tree")
+        if tree is None:
+            continue
+        separate = tree != provenance.get("subject_tree")
+        found.append((side, tree, separate))
+        recorded = d.get("benchmark")
+        if recorded is not None and not separate and _c(recorded.get("source")) != _c(d["source"]):
+            malformed.append(str(names[side]))
+    failed = []
+    if malformed:
+        failed.append({"invariant": "one-benchmark-tree",
+                       "detail": "malformed record: the benchmark is in the subject's tree but names another "
+                                 f"source (sides {sorted(set(malformed))})"})
+    if any(separate for _, _, separate in found):
+        trees = {tree for _, tree, _ in found}
+        if len(trees) != 1 or not all(separate for _, _, separate in found):
+            failed.append({"invariant": "one-benchmark-tree",
+                           "detail": f"a benchmark recorded apart from the subject must be one file on both sides "
+                                     f"and in every round: {len(trees)} benchmark trees "
+                                     f"({', '.join(sorted(t[:12] for t in trees))})"})
+    return failed
+
+
 def _workload(by_side) -> dict:
     """The unit's workload variant, as the comparison document names it."""
     workload = next(d for docs in by_side.values() for d in docs)["coordinates"]["workload"]
@@ -263,6 +302,10 @@ def _caveats(by_side, base, contender) -> list[str]:
         for d in by_side[side]:
             for w in d.get("quality", {}).get("warnings", []):
                 found.add(f"{role}: {w.get('code', w) if isinstance(w, dict) else w}")
+            # A side that is not clean is named whether or not its producer warned (UC-03 §10).
+            for flag, code in DIRTY_WARNINGS:
+                if d["provenance"].get(flag) == "dirty":
+                    found.add(f"{role}: {code}")
     return sorted(found)
 
 

benchx/benchx/runner.py

@@ -303,7 +303,10 @@ def run(order_path, out_dir, allow_collectors=False) -> dict:
                 "procedure": procedure,
                 "provenance": provenance,
             }
-            warnings = [] if condition else ([setup_warning] if setup_warning else []) + collected.warnings + case_warnings
+            # A result says so when its subject or benchmark tree is dirty (schema §4.1).
+            dirty = [code for flag, code in snapshot.DIRTY_WARNINGS if provenance[flag] == "dirty"]
+            warnings = [] if condition else (dirty + ([setup_warning] if setup_warning else [])
+                                             + collected.warnings + case_warnings)
             if warnings:
                 document["quality"] = {"warnings": warnings}
             errors = core.check(document, "measurement-result")

benchx/benchx/session.py

@@ -97,18 +97,24 @@ def _side(spec: str, suite: str, operation: str, harness, environment_variables=
         identity = snapshot.git_identity(source) or snapshot.directory_identity(source)
         if not identity:
             raise SessionError(f"cannot identify the working tree {source}")
+    found = _found_benchmark(harness, path, suite, identity["path"])
     return {"kind": kind, "path": path, "source": identity["path"], "identity": identity, "uri": uri,
-            "benchmark": _benchmark(harness, path, suite, identity["path"]),
+            "benchmark": _benchmark(found),
+            # As the runner records it: a benchmark in the subject's tree is dirty as the subject is.
+            "benchmark_dirty": found["dirty"] if found else identity["dirty"],
             "text": target_mod.text(description), "labels": dict((description or {}).get("labels", {}))}
 
 
-def _benchmark(harness, path: Path, suite: str, subject_source: str) -> dict | None:
-    """The order's `benchmark` entry when the suite's code is not in the subject's
-    checkout (see snapshot.benchmark_identity), else None. A suite that is a built
-    binary is the subject's own."""
+def _found_benchmark(harness, path: Path, suite: str, subject_source: str) -> dict | None:
+    """The identity of the suite's code when it is not in the subject's checkout (see
+    snapshot.benchmark_identity), else None. A suite that is a built binary is the subject's own."""
     if not adapters.get(harness.name).SUITE_IS_SOURCE:
         return None
-    found = snapshot.benchmark_identity(path / suite, subject_source)
+    return snapshot.benchmark_identity(path / suite, subject_source)
+
+
+def _benchmark(found: dict | None) -> dict | None:
+    """The order's `benchmark` entry for a suite found outside the subject's checkout."""
     if found is None:
         return None
     return {"source": {"uri": found["uri"], "type": found["type"]},
@@ -241,11 +247,22 @@ def compare_targets(baseline: str, contender: str, *, profile: str, suite: str,
     sides = [_side(spec, suite, operation, harness, environment_variables, provider,
                    timeout=orders.ORDER_TIMEOUT_S) for spec in (baseline, contender)]
     if profile == "revisions":
-        # UC-03 §10: refuse before any measurement, not after R rounds.
+        # UC-03 §10: a dirty side is compared and labeled local-only. A side with no working-tree id
+        # (its dirty state is `unknown`) cannot be named, so it is refused before any measurement,
+        # not after R rounds.
+        for name, side in zip(("baseline", "contender"), sides):
+            if side["identity"]["dirty"] == "unknown":  # schema §4.1: no tree id exactly when unknown
+                raise SessionError(f"{name} tree {side['source']} has no working-tree id (its dirty state is "
+                                   f"{side['identity']['dirty']}): the sides cannot be told apart, or checked for "
+                                   "a change during the run. Make every file in the tree readable, or git-ignore "
+                                   "it (UC-03 §10)")
+    if progress:  # TODO: log these at logging.WARNING once benchx has logging (plan, Q13)
         for name, side in zip(("baseline", "contender"), sides):
-            if side["identity"]["dirty"] != "clean":
-                raise SessionError(f"{name} tree {side['source']} is {side['identity']['dirty']}; "
-                                   "a revisions comparison needs clean trees (UC-03 §10)")
+            for what, state in (("tree", side["identity"]["dirty"]),
+                                ("benchmark file", side["benchmark_dirty"] if side["benchmark"] else "clean")):
+                if state != "clean":
+                    where = f" {side['source']}" if what == "tree" else ""
+                    progress(f"{name} {what}{where} is {state}: the comparison will be local-only")
     source_uri = _source_uri(sides, source_uri)
     if len({json.dumps((side.get("benchmark") or {}).get("source")) for side in sides}) != 1:
         raise SessionError("the sides' suites come from different sources; give --suite as one absolute path")

benchx/benchx/snapshot.py

@@ -28,6 +28,10 @@
 )
 
 
+# The quality warning a result carries for each dirty flag that is `dirty` (schema §4.1).
+DIRTY_WARNINGS = (("subject_dirty", "dirty-subject-tree"), ("benchmark_dirty", "dirty-benchmark-tree"))
+
+
 def env_allowlist(env: dict, adapter_names=()) -> tuple:
     """Core default, then the project's names from `env`, then the adapter's."""
     extra = [n.strip() for n in env.get("BENCHX_ENV_ALLOWLIST", "").split(",") if n.strip()]

benchx/benchx/store.py

@@ -283,6 +283,12 @@ def _rejection(name: str, error: DocumentError) -> dict:
 }
 
 
+def warning_codes(document: dict) -> list[str]:
+    """A result's quality warnings by code: a warning is a token, or an object that has a `code`."""
+    return [w.get("code", str(w)) if isinstance(w, dict) else str(w)
+            for w in document.get("quality", {}).get("warnings", [])]
+
+
 def _add_entities(entities: dict, doc: dict, project: str, projection: Projection) -> None:
     def put(name, row):
         entities.setdefault(name, {}).setdefault(_ENTITY_KEYS[name](row), row)
@@ -313,4 +319,5 @@ def put(name, row):
                              "producer": doc["producer"]["name"], "ingest_key": doc["ingest_key"],
                              "attempt_key": doc["attempt_key"], "revision": doc["revision"]["key"],
                              "axis": list(point["axis"]), "value": point["value"], "source": point["source"],
-                             "started_at": doc["provenance"]["started_at"], "ignored": point["ignored"]})
+                             "started_at": doc["provenance"]["started_at"], "ignored": point["ignored"],
+                             "warnings": warning_codes(doc)})

benchx/tests/conftest.py

@@ -136,6 +136,12 @@ def tmp(tmp_path):
     return tmp_path
 
 
+def warnings_of(document) -> list:
+    """A result's quality warnings other than the dirty-tree ones, which a test earns by running in a
+    tree with uncommitted files (schema §4.1). For tests of what the collectors and the workers warn."""
+    return [w for w in document.get("quality", {}).get("warnings", []) if not str(w).startswith("dirty-")]
+
+
 def copy_tree(src: Path, dst: Path) -> Path:
     shutil.copytree(src, dst)
     return dst

benchx/tests/test_asv.py

@@ -12,8 +12,9 @@
 from benchx import bench, cli, core, harnesses
 from benchx.adapters import asv, asv_runtime
 from benchx.store import Store
+from conftest import warnings_of
 
-SUITE = textwrap.dedent('''
+SUITE =textwrap.dedent('''
     class Sort:
         params = [[10, 100], ["a", "b"]]
         param_names = ["n", "key"]
@@ -269,7 +270,7 @@ def test_setup_means_one_call_per_loop(suite):
     assert protocol["asv"]["setup_scope"] == "call"
     assert document["procedure"]["inner_iterations"] == 1
     # A sum of 100 ints is far below a value's 10 ms: pyperf would have batched it.
-    assert document["quality"]["warnings"] == [asv.UNBATCHED]
+    assert warnings_of(document) == [asv.UNBATCHED]
 
 
 def test_setup_and_teardown_run_between_calls(suite, tmp):
@@ -329,7 +330,7 @@ def test_sample_scope_batches_calls(suite):
     assert protocol["calibration"] == {"mode": "adaptive", "minimum_sample_seconds": 0.01}
     assert protocol["asv"]["setup_scope"] == "sample"
     assert document["procedure"]["inner_iterations"] > 1
-    assert "quality" not in document
+    assert warnings_of(document) == []
 
 
 def test_sample_scope_shares_setup_within_a_value(suite, monkeypatch):

benchx/tests/test_benchmark_source.py

@@ -8,7 +8,7 @@
 
 pytest.importorskip("pyperf")
 
-from benchx import cli, core, runner, snapshot
+from benchx import adapters, cli, core, runner, snapshot
 from benchx.store import Store
 from conftest import git
 
@@ -67,6 +67,13 @@ def result(run_key):
     return document
 
 
+@pytest.mark.parametrize("adapter, apart", [("pyperf", True), ("asv", True), ("google-benchmark", False)])
+def test_which_adapters_record_a_suite_apart_from_the_subject(adapter, apart):
+    """A suite that is Python source can live outside the subject's tree, and is then recorded as its own
+    benchmark; a built binary is the subject's (schema §5.5, `one-benchmark-tree`)."""
+    assert adapters.get(adapter).SUITE_IS_SOURCE is apart
+
+
 def test_a_suite_inside_the_subject_is_the_subjects_own(subject):
     assert snapshot.benchmark_identity(subject / "lib.py", subject) is None