From f749d6ff6dc2d65fec2309778d4e40bcf2bd928c Mon Sep 17 00:00:00 2001 From: ev-br's Claude Code Agent Date: Wed, 28 Oct 2026 18:29:49 +0000 Subject: [PATCH 1/2] ENH: allow dirty trees in local comparisons This is a follow-up PR: - https://github.com/benchx-dev/benchx/pull/83 adds the design docs - this PR is branched off https://github.com/benchx-dev/benchx/pull/81, and needs a rebase once that one lands --- benchx/README.md | 23 +- benchx/benchx/bench.py | 4 +- benchx/benchx/cli.py | 10 +- benchx/benchx/compare.py | 61 ++++- benchx/benchx/runner.py | 5 +- benchx/benchx/session.py | 37 ++- benchx/benchx/snapshot.py | 4 + benchx/benchx/store.py | 9 +- benchx/tests/conftest.py | 6 + benchx/tests/test_benchmark_source.py | 9 +- benchx/tests/test_collectors.py | 6 +- benchx/tests/test_comparator.py | 130 +++++++++++ benchx/tests/test_e2e_numpy_revisions.py | 276 +++++++++++++++++++++++ benchx/tests/test_prototype.py | 72 +++++- benchx/tests/test_pyperf.py | 6 +- benchx/tests/test_session.py | 53 ++++- benchx/tests/test_sides.py | 2 +- benchx/tests/test_uc04.py | 3 +- docs/design/prototype-design.md | 36 ++- docs/design/prototype-scope.md | 5 +- 20 files changed, 691 insertions(+), 66 deletions(-) create mode 100644 benchx/tests/test_e2e_numpy_revisions.py diff --git a/benchx/README.md b/benchx/README.md index 0d8ddf7..cd98dbf 100644 --- a/benchx/README.md +++ b/benchx/README.md @@ -9,8 +9,8 @@ bx run [--out DIR] [--no-ingest] # runner + Google Benchmark adapter → result files → local store bx ingest # result files and work orders → the local store bx series [--workload … --quantity …] # list series -bx history # one series' points (a query) -bx head [-n 10] [--project P] [--workload W] [--json|--table] # the latest results, newest first +bx history # 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 @@ -159,7 +159,8 @@ 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. @@ -167,8 +168,9 @@ every result is pyperf. Results also record the installed project's version `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 @@ -180,10 +182,19 @@ every result is pyperf. Results also record the installed project's version ```console $ uv venv && uv pip install -e '.[test]' # editable: schemas are read from ../schemas -$ .venv/bin/python -m pytest tests # 344 tests, fake Google Benchmark binary, no compiler +$ .venv/bin/python -m pytest tests # 470 tests, fake Google Benchmark binary, no compiler $ ./demo.sh # real builds; needs cmake, a C++ compiler, network once ``` +`tests/test_e2e_numpy_revisions.py` is an end-to-end test that builds NumPy with +`spin` and compares two tags in one `bx compare`. It is skipped unless +`BENCHX_E2E_NUMPY` (a numpy git checkout) and `BENCHX_E2E_EXAMPLE` (the +`numpy-revisions` example directory) are set and `spin` is on the PATH; it also +needs the project-file entries of PR #65 and PR #71. `BENCHX_E2E_WORKTREES` +keeps the builds between runs, `BENCHX_E2E_REVISIONS` picks the two tags, +`BENCHX_E2E_SPIN_EXAMPLE` (the `numpy-spin` example directory) adds a single-run +scenario, and the test's docstring has the rest. + `demo.sh` prints every command before running it, as you would type it (`bx` for `.venv/bin/python -m benchx.cli`, `$WORK` for its scratch directory), so it doubles as a walkthrough. It prepares a git source with two diff --git a/benchx/benchx/bench.py b/benchx/benchx/bench.py index 94c4f9e..725e1e0 100644 --- a/benchx/benchx/bench.py +++ b/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), } diff --git a/benchx/benchx/cli.py b/benchx/benchx/cli.py index 02bdd5e..fd3d066 100644 --- a/benchx/benchx/cli.py +++ b/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: diff --git a/benchx/benchx/compare.py b/benchx/benchx/compare.py index cbcba90..1cab73c 100644 --- a/benchx/benchx/compare.py +++ b/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) diff --git a/benchx/benchx/runner.py b/benchx/benchx/runner.py index d235b77..7e49039 100644 --- a/benchx/benchx/runner.py +++ b/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") diff --git a/benchx/benchx/session.py b/benchx/benchx/session.py index e4f4737..24a21aa 100644 --- a/benchx/benchx/session.py +++ b/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") diff --git a/benchx/benchx/snapshot.py b/benchx/benchx/snapshot.py index 8168de9..76e14c8 100644 --- a/benchx/benchx/snapshot.py +++ b/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()] diff --git a/benchx/benchx/store.py b/benchx/benchx/store.py index b787223..ab22116 100644 --- a/benchx/benchx/store.py +++ b/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)}) diff --git a/benchx/tests/conftest.py b/benchx/tests/conftest.py index 32001ca..a52dcdf 100644 --- a/benchx/tests/conftest.py +++ b/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 diff --git a/benchx/tests/test_benchmark_source.py b/benchx/tests/test_benchmark_source.py index 98358f3..8f5d301 100644 --- a/benchx/tests/test_benchmark_source.py +++ b/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 diff --git a/benchx/tests/test_collectors.py b/benchx/tests/test_collectors.py index 23c33fe..45879a9 100644 --- a/benchx/tests/test_collectors.py +++ b/benchx/tests/test_collectors.py @@ -7,7 +7,7 @@ import pytest from benchx import collectors, compare, config, core, runner -from conftest import git +from conftest import git, warnings_of def spec(name="facts", script='print(\'{"a": 1}\')', **extra): @@ -184,7 +184,7 @@ def test_bench_runs_the_collector_and_records_its_facts(project): assert document["coordinates"]["environment"]["system"]["facts"]["s"] == 1 and "sha256" in document["coordinates"]["environment"]["system"]["facts"] assert document["provenance"]["info"]["how_built"] == {"k": 1} assert "collector-output" in {a["kind"] for a in document["provenance"]["artifacts"]} - assert "quality" not in document + assert warnings_of(document) == [] def test_a_mismatch_refuses_the_order_before_any_case(project, capsys, monkeypatch): @@ -199,7 +199,7 @@ def test_a_warn_mismatch_runs_and_marks_every_result(project, monkeypatch): configure(project, on_fail="warn") monkeypatch.setenv("K", "2") assert cli.main(["bench", "@x", *FAST, "--run-key", "c3"]) == cli.EXIT_OK - assert result_of("c3")["quality"]["warnings"] == ["collector-mismatch"] + assert warnings_of(result_of("c3")) == ["collector-mismatch"] def test_bx_run_refuses_an_order_with_collectors_unless_allowed(project, tmp, capsys): diff --git a/benchx/tests/test_comparator.py b/benchx/tests/test_comparator.py index 56c84c6..9bafbba 100644 --- a/benchx/tests/test_comparator.py +++ b/benchx/tests/test_comparator.py @@ -227,3 +227,133 @@ def test_environments_a_label_with_two_builds_is_three_sides(): docs[2]["coordinates"]["subject"]["configuration"] = {"hardening": "other"} doc = compare.compare(docs, run_key="run", profile="environments", baseline="plain", label="build") assert invariants(doc) == ["two-sides"] + + +# --- dirty trees and the benchmark file (UC-03 §10, schema §5.5) ------------------------------- + +OTHER_SUITE = {"uri": "https://example.org/suites.git", "type": "git"} + + +def dirty(docs, side, flag="subject_dirty"): + for d in docs: + if d["revision"]["key"] == side: + d["provenance"][flag] = "dirty" + return docs + + +def benchmark_trees(docs, tree_of, source=OTHER_SUITE): + """Record the benchmark of every result apart from the subject, as the file `tree_of(doc)`.""" + for d in docs: + d["provenance"]["benchmark_tree"] = tree_of(d) + d["benchmark"] = {"source": dict(source)} + return docs + + +def test_a_dirty_side_is_compared_and_named(): + doc = evaluate(dirty(run(), HEAD)) + assert invariants(doc) == [] and doc["local_only"] is True + assert doc["units"][0]["verdict"] == "regressed" + assert doc["units"][0]["caveats"] == ["contender: dirty-subject-tree"] # the clean baseline is not named + shown = compare.render(doc) + assert "[local-only]" in shown and "caveats: contender: dirty-subject-tree" in shown + + +def test_both_sides_dirty_are_each_named(): + doc = evaluate(dirty(dirty(run(), HEAD), BASE, "benchmark_dirty")) + assert invariants(doc) == [] and doc["local_only"] is True + assert doc["units"][0]["caveats"] == ["baseline: dirty-benchmark-tree", "contender: dirty-subject-tree"] + + +def test_clean_sides_are_not_local_only(): + doc = evaluate(run()) + assert doc["local_only"] is False and "caveats" not in doc["units"][0] + + +def test_a_tree_edited_during_the_run_is_a_third_side(): + """A side is a tree id, so a tree that changes mid-run leaves three sides and no verdict.""" + docs = run() + [d for d in docs if d["revision"]["key"] == HEAD][-1]["provenance"]["subject_tree"] = "e" * 40 + doc = evaluate(docs) + assert invariants(doc) == ["two-sides"] and doc["units"] == [] + + +def test_a_side_without_a_tree_id_cannot_be_named(): + """An unknown dirty state has no tree id (schema §4.1); the message is about the tree, not about labels.""" + docs = run() + for d in docs: + if d["revision"]["key"] == HEAD: + d["provenance"]["subject_dirty"] = "unknown" + del d["provenance"]["subject_tree"] + doc = evaluate(docs) + assert invariants(doc) == ["sides"] and doc["units"] == [] + detail = doc["failed_invariants"][0]["detail"] + assert "unknown" in detail and "--label" not in detail + + +def test_a_benchmark_file_that_is_one_file_passes_whether_clean_or_dirty(): + docs = benchmark_trees(run(), lambda d: "c" * 40) + assert invariants(evaluate(docs)) == [] + doc = evaluate(dirty(dirty(docs, BASE, "benchmark_dirty"), HEAD, "benchmark_dirty")) + assert invariants(doc) == [] and doc["local_only"] is True + + +def test_a_benchmark_file_that_differs_between_the_sides_fails(): + docs = benchmark_trees(run(), lambda d: ("c" if d["revision"]["key"] == BASE else "d") * 40) + doc = evaluate(docs) + assert invariants(doc) == ["one-benchmark-tree"] and doc["units"] == [] + assert "2 benchmark trees" in doc["failed_invariants"][0]["detail"] + + +def test_a_benchmark_file_that_changes_between_rounds_fails(): + docs = benchmark_trees(run(), lambda d: "c" * 40) + docs[-1]["provenance"]["benchmark_tree"] = "d" * 40 # the suite is edited before the last result + assert invariants(evaluate(docs)) == ["one-benchmark-tree"] + + +def test_a_benchmark_in_the_subjects_tree_differs_between_sides_by_design(): + docs = run() + for d in docs: # as the runner records an in-tree suite: the subject's tree and the subject's source + d["provenance"]["benchmark_tree"] = d["provenance"]["subject_tree"] + d["benchmark"] = {"source": d["source"]} + assert invariants(evaluate(docs)) == [] + + +def test_an_in_tree_record_that_names_another_source_is_malformed(): + docs = run() + for d in docs: + d["provenance"]["benchmark_tree"] = d["provenance"]["subject_tree"] + d["benchmark"] = {"source": dict(OTHER_SUITE)} + doc = evaluate(docs) + assert invariants(doc) == ["one-benchmark-tree"] + assert doc["failed_invariants"][0]["detail"].startswith("malformed record") + + +def test_one_side_in_tree_and_one_apart_fails(): + docs = run() + for d in docs: + if d["revision"]["key"] == BASE: + d["provenance"]["benchmark_tree"] = d["provenance"]["subject_tree"] + d["benchmark"] = {"source": d["source"]} + else: + d["provenance"]["benchmark_tree"] = "c" * 40 + d["benchmark"] = {"source": dict(OTHER_SUITE)} + # the benchmark's source URI differs too, so the identity match (`sides-agree`) fails beside it + assert "one-benchmark-tree" in invariants(evaluate(docs)) + + +def test_a_suite_in_another_checkout_of_the_same_repository_counts_as_apart(): + """Same canonical source URI as the subject, but its own file id: it is not in-tree.""" + docs = run() + for d in docs: + d["provenance"]["benchmark_tree"] = ("c" if d["revision"]["key"] == BASE else "d") * 40 + d["benchmark"] = {"source": dict(d["source"])} + assert invariants(evaluate(docs)) == ["one-benchmark-tree"] + same = benchmark_trees(run(), lambda d: "c" * 40, source={"uri": "https://example.org/demo"}) + assert invariants(evaluate(same)) == [] # and one file on both sides passes + + +def test_a_result_without_a_benchmark_tree_is_outside_the_rule(): + """A thin result has no benchmark_tree: nothing to compare.""" + docs = run() + assert all("benchmark_tree" not in d["provenance"] for d in docs) + assert invariants(evaluate(docs)) == [] diff --git a/benchx/tests/test_e2e_numpy_revisions.py b/benchx/tests/test_e2e_numpy_revisions.py new file mode 100644 index 0000000..a4f0c1a --- /dev/null +++ b/benchx/tests/test_e2e_numpy_revisions.py @@ -0,0 +1,276 @@ +"""End to end: two NumPy revisions built by `spin`, compared in one `bx compare` (UC-03). + +Opt-in. It builds NumPy with `spin` (a few minutes per revision the first time) and +needs network for the submodules, so it runs only when these are set: + + BENCHX_E2E_NUMPY a git checkout of numpy; the provider adds worktrees to it + and never builds in it + BENCHX_E2E_EXAMPLE the `numpy-revisions` example directory (benchx_user) + +and optionally: + + BENCHX_E2E_WORKTREES where the worktrees and builds go; set it to reuse them + between runs (default: a temporary directory, removed after) + BENCHX_E2E_REVISIONS two tags, "A,B" (default "v2.4.0,v2.4.3") + +Run it inside the example's environment (`environment.yml` there, plus pytest): `spin` +and a `python` with pyperf must be on the PATH. The code under test must have the project +file's `provider`, `collectors` and `compare` entries (PR #65 and PR #71); on a checkout +that lacks them, such as `main` before those merge, the test is skipped, as it is on a +machine without the variables. + +What it checks, with the clean trees of two tags: the comparison runs to a verdict with +no failed invariant and is not local-only, and the benchmark process of each side +imported the numpy built for that side, with and without a stale PYTHONPATH exported. +A probe suite records `numpy.__file__` on import, so the check does not depend on a +benchmark's numbers. + +With both builds in place it also checks, on a checkout made dirty by an untracked file (removed +afterwards), that a dirty `WORKSPACE` is compared and labeled local-only, with the dirty side named +before measuring and marked by `bx head`; and that a checkout git cannot hash (an unreadable +untracked file) is refused before anything runs. If `BENCHX_E2E_SPIN_EXAMPLE` names the `numpy-spin` +example directory, a single `bx bench` run on the dirty checkout is recorded as dirty and `bx head` +marks it, while the same run on the clean checkout is not marked. A benchmark file that is not committed +is compared and labeled local-only, not refused. +""" + +import json +import os +import shutil +import subprocess +import sys +import textwrap +from pathlib import Path + +import pytest + +import benchx +from conftest import git + +NUMPY = os.environ.get("BENCHX_E2E_NUMPY") +EXAMPLE = os.environ.get("BENCHX_E2E_EXAMPLE") +SPIN_EXAMPLE = os.environ.get("BENCHX_E2E_SPIN_EXAMPLE") +PERSISTENT_WORKTREES = os.environ.get("BENCHX_E2E_WORKTREES") +REVISIONS = os.environ.get("BENCHX_E2E_REVISIONS", "v2.4.0,v2.4.3").split(",") + +PACKAGE_ROOT = Path(benchx.__file__).resolve().parent.parent # the code under test, not an installed copy +PROJECT_FILE_ENTRIES = {"provider", "collectors", "compare"} + + +def missing_project_file_entries(): + schema = PACKAGE_ROOT.parent / "schemas" / "config" / "0.1.0" / "schema.json" + present = set(json.loads(schema.read_text())["properties"]) if schema.exists() else set() + return PROJECT_FILE_ENTRIES - present + + +pytestmark = [ + pytest.mark.skipif( + not (NUMPY and EXAMPLE and shutil.which("spin") and shutil.which("python")), + reason="end-to-end test: set BENCHX_E2E_NUMPY and BENCHX_E2E_EXAMPLE, and have spin and python on the PATH"), + pytest.mark.skipif( + bool(missing_project_file_entries()), + reason="the code under test lacks project-file entries (PR #65, PR #71): " + + ", ".join(sorted(missing_project_file_entries()))), +] + +PROBE = textwrap.dedent(''' + import os + + import numpy as np + + with open(os.environ["PROBE_OUT"], "a") as out: + out.write(f"{np.__version__} {np.__file__}\\n") + + + def bench_noop(): + return None +''') + + +def bx(args, cwd, env): + code = f"import sys; sys.path.insert(0, {str(PACKAGE_ROOT)!r}); from benchx.cli import main; sys.exit(main())" + return subprocess.run([sys.executable, "-c", code, *args], cwd=cwd, env=env, + capture_output=True, text=True, timeout=3600) + + +@pytest.fixture +def worktrees(tmp_path): + root = Path(PERSISTENT_WORKTREES) if PERSISTENT_WORKTREES else tmp_path / "worktrees" + root.mkdir(parents=True, exist_ok=True) + yield root + if not PERSISTENT_WORKTREES: # leave no registration behind in the numpy checkout + for tree in root.iterdir(): + if tree.is_dir(): + subprocess.run(["git", "-C", NUMPY, "worktree", "remove", "--force", str(tree)], capture_output=True) + subprocess.run(["git", "-C", NUMPY, "worktree", "prune"], capture_output=True) + + +@pytest.fixture +def project(tmp_path): + """A scratch copy of the example, in a git repository of its own so that the suite is + committed, with the probe suite added and the precision cut down.""" + project = tmp_path / "project" + shutil.copytree(EXAMPLE, project, ignore=shutil.ignore_patterns("results", "store.parquet", "__pycache__")) + (project / "bench_probe.py").write_text(PROBE) + config = json.loads((project / "benchx.conf.json").read_text()) + for level in config["adapter"]["precision"]["repetitions"]["levels"]: + level["n"] = 2 + config["compare"] = {"profile": "revisions", "rounds": 3} + (project / "benchx.conf.json").write_text(json.dumps(config, indent=1)) + git(project, "init", "-q") + git(project, "add", "-A") + git(project, "commit", "-qm", "scratch copy of the example") + return project + + +def run_compare(project, worktrees, run_key, *, numpy_dir=None, targets=None, extra=(), stale_pythonpath=None, + suite="bench_probe.py"): + """`bx compare` of two targets (by default `@numpy=A @numpy=B`) with the probe suite; returns the + finished process, the file the probe writes, and the environment it ran in.""" + probe_out = project / f"{run_key}.probe" + env = {k: v for k, v in os.environ.items() if k != "PYTHONPATH"} + env.update(NUMPY=str(numpy_dir or NUMPY), WORKTREES=str(worktrees), PROBE_OUT=str(probe_out)) + if stale_pythonpath: + env["PYTHONPATH"] = str(stale_pythonpath) + targets = targets or [f"@numpy={ref}" for ref in REVISIONS] + done = bx(["compare", *targets, "--suite", str(project / suite), "--run-key", run_key, "--json", + *extra], project, env) + return done, probe_out, env + + +def compare(project, worktrees, run_key, stale_pythonpath=None): + """`bx compare @numpy=A @numpy=B` with the probe suite; returns the comparison document and + the lines the probe wrote.""" + done, probe_out, _ = run_compare(project, worktrees, run_key, stale_pythonpath=stale_pythonpath) + assert done.returncode == 0, done.stderr[-3000:] + return json.loads(done.stdout), probe_out.read_text().splitlines() + + +def build_of(worktrees, ref): + (tree,) = [d for d in worktrees.iterdir() if d.name.startswith(f"{ref}-")] + return tree / "build-install" + + +def check(doc, probed, worktrees, *, local_only=False): + assert doc["failed_invariants"] == [] and doc["missing"] == [] + assert doc["local_only"] is local_only # two tags, clean trees, unless a checkout was made dirty + assert [u["rounds"] for u in doc["units"]] == [3] + expected = {ref.removeprefix("v"): build_of(worktrees, ref) for ref in REVISIONS} + seen = {} + for line in probed: + version, path = line.split(" ", 1) + seen.setdefault(version, set()).add(Path(path)) + assert set(seen) == set(expected) # every worker imported one of the two numpys, and both were used + for version, paths in seen.items(): + assert all(p.is_relative_to(expected[version]) for p in paths), (version, paths) + + +def test_each_side_imports_the_numpy_built_for_it(project, worktrees): + doc, probed = compare(project, worktrees, "e2e-clean") + check(doc, probed, worktrees) + + +def test_a_stale_pythonpath_does_not_change_what_a_side_imports(project, worktrees): + """The target's directory comes first on sys.path, so a PYTHONPATH naming one build, which + would make both sides measure that build if the target were the build root, does nothing.""" + doc, probed = compare(project, worktrees, "e2e-prepare") # makes sure both builds exist + check(doc, probed, worktrees) + first = next(build_of(worktrees, REVISIONS[0]).glob("**/site-packages")) + doc, probed = compare(project, worktrees, "e2e-stale", stale_pythonpath=first) + check(doc, probed, worktrees) + + +@pytest.fixture +def built(project, worktrees): + """Both builds exist (a first, clean comparison makes them); the second revision's checkout.""" + doc, probed = compare(project, worktrees, "e2e-built") + check(doc, probed, worktrees) + return build_of(worktrees, REVISIONS[1]).parent + + +def test_a_dirty_workspace_is_compared_and_labeled_local_only(project, worktrees, built): + """UC-03 §10: the second revision's checkout, with an uncommitted file, is the contender (`WORKSPACE`).""" + uncommitted = built / "benchx-e2e-uncommitted.txt" + uncommitted.write_text("an uncommitted file\n") + try: + done, probe_out, env = run_compare(project, worktrees, "e2e-dirty", numpy_dir=built, + targets=[f"@numpy={REVISIONS[0]}", "@numpy=WORKSPACE"]) + assert done.returncode == 0, done.stderr[-3000:] + doc, probed = json.loads(done.stdout), probe_out.read_text().splitlines() + head = bx(["head", "--table"], project, env) + finally: + uncommitted.unlink() + check(doc, probed, worktrees, local_only=True) + # Only the subject is dirty: the probe suite is a clean file in a repository of its own, recorded + # apart from the checkout, so no benchmark warning, and the clean tag is not named. + assert {c for u in doc["units"] for c in u["caveats"]} == {"contender: dirty-subject-tree"} + said = done.stderr + assert f"contender tree {built}" in said and "local-only" in said + assert said.index("local-only") < said.index("round 1/") # said before measuring + assert head.returncode == 0 and "dirty-subject-tree" in head.stdout + + +def test_an_uncommitted_benchmark_file_is_compared_and_labeled_local_only(project, worktrees, built): + """The suite is a file outside both checkouts; one that is not committed is `dirty`, not refused.""" + (project / "bench_uncommitted.py").write_text(PROBE) # untracked in the project's repository + done, probe_out, _ = run_compare(project, worktrees, "e2e-suite", suite="bench_uncommitted.py") + assert done.returncode == 0, done.stderr[-3000:] + doc, probed = json.loads(done.stdout), probe_out.read_text().splitlines() + check(doc, probed, worktrees, local_only=True) + assert {c for u in doc["units"] for c in u["caveats"]} == {"baseline: dirty-benchmark-tree", + "contender: dirty-benchmark-tree"} + assert [line.split()[0] for line in done.stderr.splitlines() if "benchmark file" in line] == ["baseline", "contender"] + + +@pytest.mark.skipif(os.geteuid() == 0, reason="root reads any file") +def test_a_workspace_git_cannot_hash_is_refused_before_anything_runs(project, worktrees, built): + """An unreadable untracked file leaves the checkout's dirty state unknown, and with no tree id.""" + unreadable = built / "benchx-e2e-unreadable.txt" + unreadable.write_text("x\n") + unreadable.chmod(0) + try: # --no-build: the provider only describes, so nothing is built either + done, _, _ = run_compare(project, worktrees, "e2e-unknown", numpy_dir=built, extra=["--no-build"], + targets=[f"@numpy={REVISIONS[0]}", "@numpy=WORKSPACE"]) + finally: + unreadable.chmod(0o644) + unreadable.unlink() + assert done.returncode == 2, done.stderr[-3000:] + assert "has no working-tree id" in done.stderr and "unknown" in done.stderr + assert not (project / "results" / "e2e-unknown").exists() # no order was written + + +@pytest.mark.skipif(not SPIN_EXAMPLE, reason="set BENCHX_E2E_SPIN_EXAMPLE to the numpy-spin example directory") +def test_numpy_spin_records_a_dirty_run_and_head_marks_it(tmp_path, built): + """UC-01 with `numpy-spin`'s own provider: a single run on a dirty checkout is recorded as dirty.""" + spin = tmp_path / "spin" + shutil.copytree(SPIN_EXAMPLE, spin, ignore=shutil.ignore_patterns("results", "store.parquet", "__pycache__")) + config = json.loads((spin / "benchx.conf.json").read_text()) + for level in config["adapter"]["precision"]["repetitions"]["levels"]: + level["n"] = 2 + (spin / "benchx.conf.json").write_text(json.dumps(config, indent=1)) + git(spin, "init", "-q") + git(spin, "add", "-A") + git(spin, "commit", "-qm", "scratch copy of the example") + build = built / "build-install" + env = dict(os.environ, NUMPY=str(built), BUILD=str(build), + PYTHONPATH=str(next(build.glob("**/site-packages")))) # the example's own "hack" + + def run(run_key): + done = bx(["bench", "@numpy", "--suite", str(spin / "bench_linalg.py"), "--run-key", run_key], spin, env) + assert done.returncode == 0, done.stderr[-3000:] + + run("spin-clean") + uncommitted = built / "benchx-e2e-uncommitted.txt" + uncommitted.write_text("an uncommitted file\n") + try: + run("spin-dirty") + finally: + uncommitted.unlink() + rows = bx(["head", "--table"], spin, env).stdout.splitlines() + dirty, clean = ([r for r in rows if key in r] for key in ("spin-dirty", "spin-clean")) + # Only the subject is dirty: the suite is a clean file in the example's own repository. + assert dirty and all(r.rstrip().endswith("success [dirty-subject-tree]") for r in dirty) + assert clean and not any("dirty-" in r for r in clean) + stored = [json.loads(line) for line in bx(["head", "--json"], spin, env).stdout.splitlines() if line.strip()] + flags = {d["provenance"]["run_key"]: d["provenance"]["subject_dirty"] for d in stored} + assert flags == {"spin-clean": "clean", "spin-dirty": "dirty"} diff --git a/benchx/tests/test_prototype.py b/benchx/tests/test_prototype.py index 227f4e0..8138cb3 100644 --- a/benchx/tests/test_prototype.py +++ b/benchx/tests/test_prototype.py @@ -1,13 +1,14 @@ """The success criteria of prototype-scope.md §3, on fake Google Benchmark output.""" import json +import os import re import time from pathlib import Path import pytest -from benchx import compare, core, identity, runner +from benchx import cli, compare, core, identity, runner from benchx.core.errors import StructureError from benchx.store import Store from conftest import NOISY, QUIET, SOURCE_URI, cases, git, make_build, order, run_rounds @@ -198,16 +199,22 @@ def test_ad_hoc_is_thin_and_a_dirty_run_is_accepted(repo, tmp): assert {s["project"] for s in store.series()} == {f"local/{docs[0]['coordinates']['environment']['identity']['runner']}"} -def test_revisions_refuses_a_dirty_side(repo, tmp): - """Criterion 7, UC-03 §10: a revisions comparison with a dirty side gets no verdicts.""" +def test_revisions_labels_a_dirty_side_local_only(repo, tmp): + """Criterion 7, UC-03 §10: a revisions comparison with a dirty side is compared and labeled + local-only, and the sides that are not clean are named.""" out = _dirty_run(repo, tmp, lambda base, head: [{"build": base, "source": repo["wt-base"]}, {"build": head, "source": repo["wt-head"]}], "dirty-rev") store = Store(tmp / "store.parquet") store.ingest_paths([out]) doc = compare.compare(store.documents("dirty-rev"), run_key="dirty-rev", profile="revisions", baseline=repo["base"]) - assert [f["invariant"] for f in doc["failed_invariants"]] == ["clean-tree"] - assert doc["units"] == [] + assert doc["failed_invariants"] == [] and doc["local_only"] is True + assert doc["units"] and core.check(doc, "comparison-document") == [] + caveats = {c for u in doc["units"] for c in u.get("caveats", [])} + assert {"contender: dirty-subject-tree", "contender: dirty-benchmark-tree"} <= caveats + assert not any(c.startswith("baseline:") for c in caveats) # the clean side is not named + shown = compare.render(doc) + assert "[local-only]" in shown and "contender: dirty-subject-tree" in shown def test_environments_labels_a_dirty_side_local_only(repo, tmp): @@ -224,6 +231,61 @@ def test_environments_labels_a_dirty_side_local_only(repo, tmp): assert doc["units"] +def test_a_dirty_result_carries_the_dirty_warnings_and_a_clean_one_does_not(repo, tmp): + """Schema §4.1: a quality warning for each flag that is `dirty`; none for a clean result or a condition.""" + out = _dirty_run(repo, tmp, lambda base, head: [{"build": base, "source": repo["wt-base"]}, + {"build": head, "source": repo["wt-head"]}], "warned") + seen = {} + for d in results(out): + assert core.check(d, "measurement-result") == [] + if d["measurement"].get("kind") == "condition": + assert "quality" not in d # the load-average sibling is a condition, not a case + continue + # A Google Benchmark suite is the subject's own: its record names the subject's source and tree. + assert d["provenance"]["benchmark_tree"] == d["provenance"]["subject_tree"] + assert d["benchmark"]["source"] == d["source"] + seen.setdefault(d["provenance"]["subject_dirty"], set()).add(tuple(d.get("quality", {}).get("warnings", []))) + assert seen == {"clean": {()}, "dirty": {("dirty-subject-tree", "dirty-benchmark-tree")}} + + +@pytest.mark.skipif(os.geteuid() == 0, reason="root reads any file") +def test_a_single_run_records_an_unknown_tree_without_a_tree_id(repo, tmp): + """UC-01 is unchanged: a tree git cannot hash is recorded as `unknown`, with no tree id (schema §4.1).""" + unreadable = repo["wt-head"] / "unreadable.txt" + unreadable.write_text("x\n") + unreadable.chmod(0) + try: + build = make_build(tmp / "b1", repo["wt-head"], cases()) + out = run_rounds(tmp, [{"build": build, "source": repo["wt-head"]}], rounds=1, run_key="unk", out=tmp / "unk") + finally: + unreadable.chmod(0o644) + docs = [d for d in results(out) if d["measurement"].get("kind") != "condition"] + assert docs + for d in docs: + assert core.check(d, "measurement-result") == [] + assert d["provenance"]["subject_dirty"] == d["provenance"]["benchmark_dirty"] == "unknown" + assert "subject_tree" not in d["provenance"] and "benchmark_tree" not in d["provenance"] + + +def test_head_and_history_show_the_dirty_warnings(repo, tmp, capsys): + """Q7: the listings mark a dirty result and leave a clean one unmarked.""" + out = _dirty_run(repo, tmp, lambda base, head: [{"build": base, "source": repo["wt-base"]}, + {"build": head, "source": repo["wt-head"]}], "listed") + store = Store(tmp / "store.parquet") + store.ingest_paths([out]) + marker = "[dirty-subject-tree, dirty-benchmark-tree]" + assert cli.main(["head", "--table"]) == cli.EXIT_OK + assert marker in capsys.readouterr().out + assert cli.main(["head", "-n", "50"]) == cli.EXIT_OK + rows = capsys.readouterr().out.splitlines()[1:] + assert any(marker in row for row in rows) and any(marker not in row for row in rows) + series = [x for x in store.series(workload=QUIET, quantity="wall-time") + if json.loads(x["estimator"]) == identity.MEDIAN] + assert cli.main(["history", series[0]["fingerprint"][:12]]) == cli.EXIT_OK + points = capsys.readouterr().out.splitlines()[1:] + assert any(marker in point for point in points) and any(marker not in point for point in points) + + def test_tracked_series_and_history(revisions_run, tmp): store = Store(tmp / "store.parquet") store.ingest_paths([revisions_run]) diff --git a/benchx/tests/test_pyperf.py b/benchx/tests/test_pyperf.py index 499b299..72d1cf1 100644 --- a/benchx/tests/test_pyperf.py +++ b/benchx/tests/test_pyperf.py @@ -20,7 +20,7 @@ from benchx import core, runner, snapshot from benchx.adapters import pyperf as adapter -from conftest import SOURCE_URI, order +from conftest import SOURCE_URI, order, warnings_of FIXTURES = Path(__file__).parent / "fixtures" PRECISION = {"repetitions": {"mode": "fixed", "levels": [{"unit": "process", "n": 2}, {"unit": "value", "n": 2}]}, @@ -101,7 +101,7 @@ def test_end_to_end_result(tmp, tree): assert doc["coordinates"]["environment"]["system"]["runtime"]["python"] kinds = {a["kind"] for a in doc["provenance"]["artifacts"]} assert {"native-output", "stdout", "stderr", "driver", "worker-environment"} <= kinds - assert "quality" not in doc # the workers had the environment the manager passed + assert warnings_of(doc) == [] # the workers had the environment the manager passed assert "summaries" not in measurement # pyperf's file stores none @@ -238,7 +238,7 @@ def test_environment_passes_through_to_the_workers(tmp, tree): artifact = next(a for a in doc["provenance"]["artifacts"] if a["kind"] == "worker-environment") captured = json.loads(Path(artifact["uri"].removeprefix("file://")).read_text()) assert captured["OPENBLAS_NUM_THREADS"] == "1" - assert "quality" not in doc + assert warnings_of(doc) == [] NAMES = snapshot.env_allowlist({}, adapter.ENV_ALLOWLIST) diff --git a/benchx/tests/test_session.py b/benchx/tests/test_session.py index baa2c3d..784c10b 100644 --- a/benchx/tests/test_session.py +++ b/benchx/tests/test_session.py @@ -1,6 +1,7 @@ """The session loop and the two modes of `bx compare` (prototype-design.md §1, §5).""" import json +import os import pytest @@ -48,13 +49,53 @@ def test_session_environments(repo, tmp): assert verdicts(doc) == {QUIET: "regressed", NOISY: "no-change-detected"} -def test_dirty_tree_refused_before_anything_runs(repo, tmp, builds): - (repo["wt-head"] / "bench.cpp").write_text("// uncommitted\n") +def compare_dirty(builds, tmp, **extra): + said = [] store = Store(tmp / "store.parquet") - with pytest.raises(session.SessionError, match="clean trees"): - session.compare_targets(builds["baseline"], builds["contender"], profile="revisions", - adapter="google-benchmark", suite="demo-bench", rounds=4, store=store, run_key="d1", out=tmp / "out") - assert not (tmp / "out").exists() and not store.path.exists() + doc = session.compare_targets(builds["baseline"], builds["contender"], profile="revisions", + adapter="google-benchmark", suite="demo-bench", rounds=4, store=store, + run_key="d1", out=tmp / "out", progress=said.append, **extra) + return doc, said + + +def test_a_dirty_side_is_compared_and_labeled_local_only(repo, tmp, builds): + """UC-03 §10: the comparison runs, says before measuring which side is not clean, and is local-only.""" + (repo["wt-head"] / "bench.cpp").write_text("// uncommitted\n") + doc, said = compare_dirty(builds, tmp) + assert doc["failed_invariants"] == [] and doc["local_only"] is True + assert verdicts(doc) + notes = [m for m in said if "local-only" in m] + assert len(notes) == 1 and notes[0].startswith(f"contender tree {repo['wt-head']} is dirty") + assert said.index(notes[0]) < said.index(next(m for m in said if m.startswith("round 1/4"))) # before measuring + caveats = {c for u in doc["units"] for c in u["caveats"]} + assert {"contender: dirty-subject-tree", "contender: dirty-benchmark-tree"} <= caveats + + +def test_both_dirty_sides_are_named(repo, tmp, builds): + (repo["wt-base"] / "bench.cpp").write_text("// uncommitted, base\n") + (repo["wt-head"] / "bench.cpp").write_text("// uncommitted, head\n") + doc, said = compare_dirty(builds, tmp) + assert doc["failed_invariants"] == [] and doc["local_only"] is True + assert [m.split()[0] for m in said if "local-only" in m] == ["baseline", "contender"] + + +def test_clean_sides_say_nothing_and_are_not_local_only(repo, tmp, builds): + doc, said = compare_dirty(builds, tmp) + assert doc["local_only"] is False and not [m for m in said if "local-only" in m] + + +@pytest.mark.skipif(os.geteuid() == 0, reason="root reads any file") +def test_a_side_with_no_tree_id_is_refused_before_anything_runs(repo, tmp, builds): + """An unreadable untracked file leaves git without a working-tree id: the dirty state is `unknown`.""" + unreadable = repo["wt-head"] / "unreadable.txt" + unreadable.write_text("x\n") + unreadable.chmod(0) + try: + with pytest.raises(session.SessionError, match=r"contender tree .* has no working-tree id.*unknown"): + compare_dirty(builds, tmp) + finally: + unreadable.chmod(0o644) + assert not (tmp / "out").exists() and not (tmp / "store.parquet").exists() @pytest.mark.parametrize("kwargs, message", [ diff --git a/benchx/tests/test_sides.py b/benchx/tests/test_sides.py index 11aecc4..1bff5fe 100644 --- a/benchx/tests/test_sides.py +++ b/benchx/tests/test_sides.py @@ -83,7 +83,7 @@ def test_a_working_tree_outside_git_is_identified_by_content(tmp_path, monkeypat provided(monkeypatch, description(root)) side = session._side("@plain", SUITE, "prepare", harness=harnesses.GOOGLE_BENCHMARK) assert side["identity"]["revision"].startswith("directory-") - assert side["identity"]["dirty"] == "dirty" # so a revisions comparison refuses it + assert side["identity"]["dirty"] == "dirty" # with a tree id, so a revisions comparison labels it local-only def test_a_missing_working_tree_says_so(tmp_path, monkeypatch): diff --git a/benchx/tests/test_uc04.py b/benchx/tests/test_uc04.py index 4c0fdd8..0d7fdfb 100644 --- a/benchx/tests/test_uc04.py +++ b/benchx/tests/test_uc04.py @@ -126,6 +126,7 @@ def test_a_failed_check_is_shown_in_the_comparison_and_changes_nothing_else(tree capsys.readouterr() code, out = compare(capsys) assert code == cli.EXIT_OK - assert "caveats: contender: collector-mismatch" in out + caveats = next(line for line in out.splitlines() if line.strip().startswith("caveats:")) + assert "contender: collector-mismatch" in caveats # beside any dirty-tree caveats the fixture tree earns assert "baseline: collector-mismatch" not in out assert "failed invariants" not in out and "bench_a" in out diff --git a/docs/design/prototype-design.md b/docs/design/prototype-design.md index e8c0774..1eb865a 100644 --- a/docs/design/prototype-design.md +++ b/docs/design/prototype-design.md @@ -256,10 +256,11 @@ criterion 1 holds by construction. `--json` emits this; the default output renders it as the reviewer table (regressed, improved, no change detected, and indeterminate sections, then -the missing, failed-invariant, and excluded listings with counts). In -the `environments` profile, a comparison in which either side is not `clean` -has `local_only: true` (schema §5.5); the `revisions` profile refuses it -instead (§5). +the missing, failed-invariant, and excluded listings with counts). In any +profile, a comparison in which either side is not `clean` has `local_only: +true` (schema §5.5), is headed `[local-only]`, and names the sides that are not +clean on the `caveats:` line; the `revisions` profile fails a side with no +working-tree id instead (§5). ## 3. Runner and adapter (`bx run`) @@ -331,7 +332,10 @@ landing where its §3.4 puts it: unless the harness's suite is a source file (pyperf) outside that checkout: then the order carries `benchmark`, and the result records the suite file's own source, revision (when under git) and blob id as `benchmark_tree` - (`runner-schema.md` §3.3). + (`runner-schema.md` §3.3). A result whose subject tree is `dirty` carries + the quality warning `dirty-subject-tree`, and one whose benchmark is `dirty` + `dirty-benchmark-tree` (schema §4.1); an `unknown` state has no tree id and + no warning of its own. - **Build configuration**, from a built-in plugin that reads `CMakeCache.txt` in the build directory: build type, compiler and version, flags, and the project's own options (cache entries prefixed with the @@ -456,6 +460,8 @@ columns, which is fine at this scale; a real deployment would index them. `bx head` shows the store's size and its latest results, newest first, with run key, workload, quantity, status, median, revision, project, and labels; +the status is followed by the result's quality warnings in brackets, such as +`[dirty-subject-tree]`, in both forms; with `--json` it prints the stored documents instead, and with `--table` the `bx bench` table (median, mean ± sd, range, n, status) of each measurand, led by revision, run key and project; condition measurements are left out, as in `bx bench`. @@ -466,7 +472,8 @@ order, since every ingest appends to the rewritten file. `bx series` and `bx history` are queries over the derived `series` and `series_point`. `bx history` lists a series' points in axis order, each with the ignored -facts that changed since the point before it. Without a revision graph +facts that changed since the point before it and the quality warnings of its +result, in brackets. Without a revision graph revision order is unavailable (schema §4.1), so an axis on the revision lists them by measurement start and says so. It evaluates nothing, since history mode is deferred. @@ -492,8 +499,11 @@ matched facts agree (`sides-agree`) and the invariants the profile names over the run: exactly two sides, which for `environments` differ in more than their label, equal attempt counts per side, and sides alternating by `procedure.slot`. Results the order did not ask for are not compared. A failed invariant is recorded and the affected units get no -verdict; under `revisions` a side that is not `clean` fails the `clean-tree` -invariant the same way. A variant present on one side only is listed as missing +verdict; under `revisions` the same happens to a side with no tree id (the +`sides` invariant), to a tree edited during the run (a third side: `two-sides`), +and to a benchmark recorded apart from the subject that is not one file on +both sides and in every round (`one-benchmark-tree`, schema §5.5). A side that +is not clean does not fail the run: it makes the comparison local-only. A variant present on one side only is listed as missing (criterion 4). Results with a status other than `success` are excluded and listed. @@ -530,8 +540,10 @@ compares them; it is the workbench's session loop (`system-decomposition.md` 1. checks that both build directories and the suite binary exist, and locates each side's source checkout, from `SOURCE_DIR` or the `CMAKE_HOME_DIRECTORY` the build records. Under `revisions` it also checks - that both trees are clean, and refuses before running anything, so a dirty - tree fails at once and not after R rounds; + that both trees have a working-tree id, and refuses before running + anything, so a tree whose state is unknown fails at once and not after R + rounds. A dirty side is not refused: the session says which sides are not + clean, before measuring, since the comparison will be local-only; 2. fixes a run key, from `--run-key` or a timestamped default; 3. for each of R rounds, for each side in order (baseline, then contender), writes one work order with the shared run key, `round`, and a running @@ -553,8 +565,8 @@ it has one. The description's declared text (`how_built`, `activation`, checked. If it stops partway, the results already delivered stay in the store, and a read-mode comparison of the run key reports missing results or a failed alternation invariant, as UC-03 §10 requires. -Allowing a local-only `revisions` comparison of a dirty tree is open -(`system-decomposition.md` §6 question 4). +A `revisions` comparison of a dirty tree is allowed and local-only +(`system-decomposition.md` §6 question 4, resolved). ## 6. Demonstration diff --git a/docs/design/prototype-scope.md b/docs/design/prototype-scope.md index 0cd89fa..14b9750 100644 --- a/docs/design/prototype-scope.md +++ b/docs/design/prototype-scope.md @@ -68,8 +68,9 @@ proto-workbench). `(producer, ingest_key)` of the results it consumed. 7. The ad hoc shape works end to end with no project, configuration file, or store path. A single run on a dirty tree is accepted and recorded as dirty. - A `revisions` comparison with a dirty side is refused (UC-03 §10), and an - `environments` comparison with one is labeled local-only (schema §5.5). + A comparison with a dirty side, under `revisions` or `environments`, is + labeled local-only and names the dirty side (UC-03 §10, schema §5.5); a + `revisions` side with no working-tree id is refused. 8. The same comparison computed from a throwaway store and from a persistent store yields the same document. From 3d25cc702b82c52a6f98d27dc0dbad3404a05a32 Mon Sep 17 00:00:00 2001 From: ev-br's Claude Code Agent Date: Thu, 29 Oct 2026 21:17:16 +0000 Subject: [PATCH 2/2] TST: ignore dirty-tree warnings in the setup_scope tests The two asv tests that came in with setup_scope assert the exact quality warnings of a result. With dirty trees allowed in local comparisons, a result also carries the dirty-* warnings the test earns by running in a tree with uncommitted files, so compare through warnings_of(), as the collector and pyperf tests do. Co-Authored-By: Claude Sonnet 5.5 --- benchx/tests/test_asv.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/benchx/tests/test_asv.py b/benchx/tests/test_asv.py index e338d6f..90c8827 100644 --- a/benchx/tests/test_asv.py +++ b/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):