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
5 changes: 4 additions & 1 deletion docs/design/prototype-design.md
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,10 @@ landing where its §3.4 puts it:
neither locates a checkout the runner refuses the order, because a result
must state its revision (`runner.md` §2), and `runner-schema.md` §5.1 refuses
an order with no revision to record. The benchmark code is the subject's own
checkout, so the benchmark revision, dirty flag, and tree are the subject's
checkout, so the benchmark revision, dirty flag, and tree are the subject's,
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).
- **Build configuration**, from a built-in plugin that reads
`CMakeCache.txt` in the build directory: build type, compiler and version,
Expand Down
4 changes: 2 additions & 2 deletions docs/design/runner-schema.md
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ Where results are delivered (store URL, local file) is an invocation argument, n

### 3.3 Benchmark suite location (addendum)

§3's field table and §3.1 name only the subject's target. Nothing in the original draft says where the *benchmark* suite's own code lives, yet every non-thin result requires `benchmark.source` and `benchmark.revision` (`benchmark-result-schema.md` §4.1, §5.2) alongside `provenance.benchmark_dirty`/`benchmark_tree`. This is a gap in the original design, closed here: an optional `benchmark` object, `{source, revision}`, mirroring `sourceRef` and a bare revision key the way `target.kind: revision` does. When present, the runner records that revision the same way it records `target`'s, from the benchmark checkout it is pointed at; it does not fetch it. When absent, the benchmarks are assumed to live in the same checkout as `target`, and the runner reuses `target`'s resolved source, revision, dirty flag, and tree for `benchmark.*` and `provenance.benchmark_dirty`/`benchmark_tree` — the common case where a project's benchmarks live alongside its own code. A benchmark suite with no version control at all (an installed, unversioned directory) is not covered by this addendum and remains open.
§3's field table and §3.1 name only the subject's target, yet every non-thin result requires `benchmark.source` and `benchmark.revision` (`benchmark-result-schema.md` §4.1, §5.2) alongside `provenance.benchmark_dirty`/`benchmark_tree`. The order therefore says where the *benchmark* suite's own code lives with an optional `benchmark` object, `{source, revision}`, mirroring `sourceRef` and a bare revision key the way `target.kind: revision` does. When present, the runner records that revision the same way it records `target`'s, from the benchmark checkout it is pointed at; it does not fetch it. When absent, the benchmarks are assumed to live in the same checkout as `target`, and the runner reuses `target`'s resolved source, revision, dirty flag, and tree for `benchmark.*` and `provenance.benchmark_dirty`/`benchmark_tree` — the common case where a project's benchmarks live alongside its own code. A benchmark suite with no version control is covered for a single file: `benchmark.revision` is then absent, `benchmark.source` is the `file://` URI of the file's directory (`type: directory`), `provenance.benchmark_tree` is the file's git blob id (`git hash-object FILE`), and `benchmark_dirty` is `dirty`. The file, not its directory, is the unit hashed, so what it imports is not covered. A suite inside another git repository takes that repository's source and HEAD as its revision, with the file's own blob id as the tree and `clean` when the file equals HEAD's version. The prototype applies this to suites that are source files (pyperf); a suite that is a built binary (Google Benchmark) is the subject's own. The runner finds the suite's identity itself, whatever the order says, and refuses an order whose `benchmark` disagrees with it. A `file://` source names a place on one machine, so such a series does not follow the suite to another machine or folder (the prototype's identity keeps the benchmark's source URI).

## 4. Environment policy

Expand Down Expand Up @@ -203,7 +203,7 @@ Every `plan` entry ends as exactly one result, using the five statuses the resul

Reasons are drawn from a closed list of codes, optionally followed by a hyphen and a detail, so dashboards can group failures by prefix without parsing free text.

**Flagged issue (found while fixing the token-regex bug below).** The result schema's `measurement.reason` pattern is `^[a-z0-9]+([.-][a-z0-9]+)*$` (`schemas/measurement-result/0.1.0/schema.json`) — lowercase alphanumeric segments joined only by `.` or `-`. Two things above violated it and are corrected in this revision: the codes used `_` (now `-`, matching `harness-adapter.md`'s own `harness.no-output`-style reasons), and `policy_unsatisfied:<check>` used `:` as a code/detail separator, which the pattern permits nowhere — no delimiter was ever reserved for it. The fix joins code and detail with `-` instead (`policy-unsatisfied-<check>`), which validates, but weakens the original intent: §7's "closed list of codes, optionally followed by a detail, so dashboards can group failures without parsing free text" assumed an unambiguous boundary between the fixed code and the open check name. Joined by the same character multi-word codes already use, that boundary survives only for a human who knows the closed code list, not for an automated grouping-by-prefix reader. `<check>` must itself be hyphen-safe for the composed reason to validate, which is also why `gpu.clocks_locked` in §4.1 is corrected to `gpu.clocks-locked`. A structured alternative — keeping `policy-unsatisfied` as the whole `reason` and carrying `<check>` in a separate field (a new `constraint`-adjacent field, or `provenance.info`) instead of concatenating it — would preserve the separation properly; that's a result-schema change and is left open rather than decided here.
**Reason format.** The result schema's `measurement.reason` pattern is `^[a-z0-9]+([.-][a-z0-9]+)*$` (`schemas/measurement-result/0.1.0/schema.json`): lowercase alphanumeric segments joined only by `.` or `-`. Codes therefore join words with `-` (matching `harness-adapter.md`'s `harness.no-output`-style reasons), and a code and its detail are joined with `-` too (`policy-unsatisfied-<check>`), since the pattern permits no other separator, such as `:`. `<check>` must itself be hyphen-safe for the composed reason to validate, which is why the check is named `gpu.clocks-locked` in §4.1. The cost is that the boundary between the fixed code and the open check name is not unambiguous: joined by the same character multi-word codes use, it is visible only to a human who knows the closed code list, not to an automated grouping-by-prefix reader. A structured alternative — keeping `policy-unsatisfied` as the whole `reason` and carrying `<check>` in a separate field (a new `constraint`-adjacent field, or `provenance.info`) instead of concatenating it — would preserve the separation properly; that's a result-schema change and is left open.

## 7. Versioning and hashing

Expand Down
1 change: 1 addition & 0 deletions prototype/benchx/adapters/asv.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
NAME = "asv"
CONTEXT_KEY = "asv"
WORKLOAD_PARAMETERS = False
SUITE_IS_SOURCE = True # the suite is Python source, as pyperf's
PRODUCER = {"name": "benchx/asv-adapter", "version": "0.2.0", "mapping_version": "asv-on-pyperf/v2"}
QUANTITIES = pyperf.QUANTITIES
ENV_ALLOWLIST = pyperf.ENV_ALLOWLIST
Expand Down
2 changes: 2 additions & 0 deletions prototype/benchx/adapters/gbench.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
from .base import Unsupported, run_bounded

NAME = "google-benchmark"
# The suite is a binary built from the subject's source, so its benchmarks are the subject's own.
SUITE_IS_SOURCE = False
CONTEXT_KEY = "google_benchmark"
WORKLOAD_PARAMETERS = False
PRODUCER = {"name": "benchx/gbench-adapter", "version": "0.1.0", "mapping_version": "gbench-to-benchx/v1"}
Expand Down
2 changes: 2 additions & 0 deletions prototype/benchx/adapters/pyperf.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
from .base import Unsupported, run_bounded

NAME = "pyperf"
# The suite is a Python source file, which may live outside the subject's checkout.
SUITE_IS_SOURCE = True

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note, we would also need this for asv after rebase.

CONTEXT_KEY = "pyperf"
WORKLOAD_PARAMETERS = False
PRODUCER = {"name": "benchx/pyperf-adapter", "version": "0.1.0", "mapping_version": "pyperf-to-benchx/v1"}
Expand Down
4 changes: 3 additions & 1 deletion prototype/benchx/orders.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def default_run_key(prefix: str) -> str:

def make_order(*, suite: dict, target: dict, precision: dict, run_key: str, quantity: str = "wall-time",
case_s: float = CASE_TIMEOUT_S, order_s: float = ORDER_TIMEOUT_S, project=None, labels=None,
environment_variables=None, round_=None, slot=None) -> dict:
environment_variables=None, round_=None, slot=None, benchmark=None) -> dict:
"""One resolved work order. Optional fields that are None or empty are left out.

Raises OrderError when the result is not a valid work order.
Expand Down Expand Up @@ -57,6 +57,8 @@ def make_order(*, suite: dict, target: dict, precision: dict, run_key: str, quan
order["labels"] = dict(labels)
if project:
order["project"] = project
if benchmark:
order["benchmark"] = benchmark
errors = core.check(order, "work-order")
if errors:
raise OrderError("; ".join(str(error) for error in errors))
Expand Down
37 changes: 30 additions & 7 deletions prototype/benchx/runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,6 @@ def check(order: dict) -> Iterator[Refusal]:
yield Refusal(f"no adapter for {suite['adapter']!r}")
if order["target"]["kind"] not in ("build", "working_tree"):
yield Refusal(f"only build and working_tree targets are supported; {order['target']['kind']!r} is deferred")
if "benchmark" in order:
yield Refusal("only benchmarks in the subject's own checkout are supported")
if "include" in suite or "exclude" in suite:
yield Refusal("only a single suites[].filter is supported, not include/exclude")
if adapter is not None:
Expand All @@ -108,6 +106,18 @@ def check(order: dict) -> Iterator[Refusal]:
yield from e.problems


def _benchmark_mismatches(stated: dict | None, found: dict | None):
"""Refusals when the order's `benchmark` is not what the suite file turns out to be."""
if stated is None:
return
if found is None:
yield Refusal("the order names a benchmark source, but the suite is in the subject's own checkout")
elif (stated["source"]["uri"], stated.get("revision")) != (found["uri"], found.get("revision")):
yield Refusal(f"the order names benchmark {stated['source']['uri']}"
f"{'@' + stated['revision'] if 'revision' in stated else ''}, "
f"but the suite is in {found['uri']}{'@' + found['revision'] if 'revision' in found else ''}")


def run(order_path, out_dir) -> dict:
"""Execute one order; write results and the order into out_dir."""
inspection = core.inspect(Path(order_path), "work-order")
Expand Down Expand Up @@ -139,6 +149,14 @@ def run(order_path, out_dir) -> dict:
if source is None:
# runner.md §2: a result that cannot state its revision is a defect.
refusals.append(Refusal("cannot locate the checkout the target was built from; set target.source_dir"))
benchmark = None
if source is not None and getattr(adapter, "SUITE_IS_SOURCE", False):
# The suite's own code, when it is not in the subject's checkout (runner-schema.md §3.3):
# found as it is, whatever the order says; an order that says otherwise is refused.
benchmark = snapshot.benchmark_identity(target_path / order["suites"][0]["suite"], source["path"])
refusals += _benchmark_mismatches(order.get("benchmark"), benchmark)
elif "benchmark" in order:
refusals.append(Refusal(f"{adapter.NAME} suites are the subject's own; the order names a benchmark source"))
if refusals:
raise Refused(refusals)

Expand Down Expand Up @@ -228,13 +246,15 @@ def run(order_path, out_dir) -> dict:
"started_at": _timestamp(started),
"ended_at": _timestamp(started + duration),
"subject_dirty": source["dirty"],
"benchmark_dirty": source["dirty"],
"benchmark_dirty": benchmark["dirty"] if benchmark else source["dirty"],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think right now compare.py matches for clean trees, so I think if we have an unversioned benchmark file we can't run it against revisions. I think it might just be a missing update from #83? Either way, happy to do that in follow-up too if it's tricky!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed it's gh-83. It touches multiple places, so I'd prefer to get to it once in all adapters in gh-83 to keep the scope manageable.

"runner": RUNNER,
**({} if condition else {"artifacts": artifacts}),
"info": info,
}
if "tree" in source:
provenance["subject_tree"] = provenance["benchmark_tree"] = source["tree"]
if benchmark:
provenance["benchmark_tree"] = benchmark["tree"]
if "labels" in order:
provenance["labels"] = order["labels"]
workload = {"name": workload_name}
Expand All @@ -252,10 +272,13 @@ def run(order_path, out_dir) -> dict:
**({"project": order["project"]} if "project" in order else {}),
"source": order["target"]["source"],
"revision": {"key": source["revision"]},
"benchmark": {"source": order["target"]["source"],
# An unversioned tree is identified by its hash in provenance, not by a revision.
**({} if source["revision"].startswith("directory-")
else {"revision": {"key": source["revision"]}})},
"benchmark": ({"source": {"uri": benchmark["uri"], "type": benchmark["type"]},
**({"revision": {"key": benchmark["revision"]}} if "revision" in benchmark else {})}
if benchmark else
{"source": order["target"]["source"],
# An unversioned tree is identified by its hash in provenance, not by a revision.
**({} if source["revision"].startswith("directory-")
else {"revision": {"key": source["revision"]}})}),
"coordinates": {
"workload": workload,
"subject": subject,
Expand Down
19 changes: 18 additions & 1 deletion prototype/benchx/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,23 @@ def _side(spec: str, suite: str, operation: str, harness=None, environment_varia
if not identity:
raise SessionError(f"cannot identify the working tree {source}")
return {"kind": kind, "path": path, "source": identity["path"], "identity": identity, "uri": uri,
"benchmark": _benchmark(harness, path, suite, identity["path"]),
"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."""
if not adapters.get(harness.name).SUITE_IS_SOURCE:
return None
found = snapshot.benchmark_identity(path / suite, subject_source)
if found is None:
return None
return {"source": {"uri": found["uri"], "type": found["type"]},
**({"revision": found["revision"]} if "revision" in found else {})}


def _order(side: dict, *, harness, suite, filter, quantity, precision, environment_variables, project, run_key,
source_uri, round_=None, slot=None, labels=None, case_s=orders.CASE_TIMEOUT_S,
order_s=orders.ORDER_TIMEOUT_S) -> dict:
Expand All @@ -113,7 +127,8 @@ def _order(side: dict, *, harness, suite, filter, quantity, precision, environme
return orders.make_order(suite=harness.suite_entry(suite, filter), target=target, precision=precision,
quantity=quantity, run_key=run_key, project=project,
labels={**side["labels"], **(labels or {})}, case_s=case_s, order_s=order_s,
environment_variables=environment_variables, round_=round_, slot=slot)
environment_variables=environment_variables, round_=round_, slot=slot,
benchmark=side.get("benchmark"))
except orders.OrderError as e:
raise SessionError(f"work order{'' if round_ is None else f' for round {round_}'}: {e}") from None

Expand Down Expand Up @@ -215,6 +230,8 @@ def compare_targets(baseline: str, contender: str, *, profile: str, suite: str,
raise SessionError(f"{name} tree {side['source']} is {side['identity']['dirty']}; "
"a revisions comparison needs clean trees (UC-03 §10)")
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")

run_key = run_key or orders.default_run_key("compare")
out = Path(out) if out else Path("results") / run_key.replace("/", "_")
Expand Down
33 changes: 33 additions & 0 deletions prototype/benchx/snapshot.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,39 @@ def source_uri(source_dir) -> str:
return Path(source_dir, common).resolve().parent.as_uri() if common else Path(source_dir).resolve().as_uri()


def blob_id(path) -> str | None:
"""The git blob id of a file: what `git hash-object FILE` prints."""
try:
content = Path(path).read_bytes()
except OSError:
return None
return hashlib.sha1(b"blob %d\0" % len(content) + content).hexdigest()


def benchmark_identity(suite_file, subject_source) -> dict | None:
"""Where the suite's own code lives, when that is not the subject's checkout:
None when the file is inside `subject_source`.

The file is the unit, not its directory (runner-schema.md §3.3). Its tree id
is its blob id. In a git checkout the source is that repository, with its
HEAD as the revision, and the file is `clean` when it equals HEAD's version
and `dirty` otherwise, including when HEAD has none. Outside git the source
is the file's directory as a `file://` URI, with no revision, and the file is
`dirty`: nothing says it was ever committed.
"""
suite = Path(suite_file).resolve()
if suite.is_relative_to(Path(subject_source).resolve()):
return None
tree = blob_id(suite)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My Claude noticed this could be too strict about line-endings (on Windows for example where the git default is CRLF). It suggests we use git hash-object instead of the blob, but I haven't looked too much into it, maybe we can look at it down the line!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh let's punt on Windows for the moment? There are multiple places which assume POSIX, let's turn windows testing and make it run in one go separately.

top = _run(["git", "rev-parse", "--show-toplevel"], cwd=suite.parent)
head = top and _run(["git", "rev-parse", "HEAD"], cwd=top)
if head:
committed = _run(["git", "rev-parse", f"HEAD:{suite.relative_to(Path(top).resolve()).as_posix()}"], cwd=top)
return {"uri": source_uri(top), "type": "git", "revision": head, "tree": tree,
"dirty": "clean" if committed == tree else "dirty"}
return {"uri": suite.parent.as_uri(), "type": "directory", "tree": tree, "dirty": "dirty"}


_SKIPPED_DIRS = {".git", ".benchx", "__pycache__"}


Expand Down
Loading
Loading