feat(bench): aelf bench all reproducibility harness (#437) - #465
Merged
Conversation
Reviewer's GuideImplements the v2.0 reproducibility harness for Sequence diagram for aelf bench all reproducibility harnesssequenceDiagram
actor User
participant CLI as aelf_cli__cmd_bench
participant Dispatcher as benchmarks_run_main_all
participant Adapter as benchmark_adapters
participant FS as Filesystem
User->>CLI: run "aelf bench all --canonical --out PATH"
CLI->>Dispatcher: main_all(out_path, canonical=true, adapters=None, smoke=false)
Dispatcher->>Dispatcher: select invocations = CANONICAL_INVOCATIONS
Dispatcher->>Dispatcher: _validate_canonical_cut(invocations)
loop for each AdapterInvocation
Dispatcher->>Adapter: subprocess python -m benchmarks.<name>_adapter --output tmp.json
Adapter-->>Dispatcher: exit code, stdout/stderr
Dispatcher->>FS: read tmp.json
FS-->>Dispatcher: adapter JSON
Dispatcher->>Dispatcher: run_invocation() → InvocationResult
end
Dispatcher->>Dispatcher: _merge(results)
Dispatcher->>Dispatcher: _headline_cut_for(invocations)
Dispatcher->>Dispatcher: build_report(results, label="v2.0.0 canonical")
Dispatcher->>FS: write merged JSON to PATH
Dispatcher-->>CLI: return exit code (0/1/2)
CLI-->>User: print summary, exit
Class diagram for benchmarks.run dispatcher (v2.0 reproducibility harness)classDiagram
class AdapterInvocation {
+str adapter
+str sub_key
+str module
+tuple~str~ args
+str label
}
class InvocationResult {
+AdapterInvocation invocation
+str status
+float elapsed_sec
+dict~str, Any~ output
+str error_message
}
class DispatcherConfig {
<<interface>>
+CANONICAL_INVOCATIONS : tuple~AdapterInvocation~
+SMOKE_INVOCATIONS : tuple~AdapterInvocation~
}
class BenchRunner {
+int main_all(out_path, canonical, adapters, smoke, runner, tmp_root)
+InvocationResult run_invocation(inv, runner, tmp_root)
+dict~str, dict~str, Any~~ build_report(results, label, invocations_used)
+dict~str, dict~str, Any~~ _merge(results)
+dict~str, Any~ _headline_cut_for(invocations)
+void _validate_canonical_cut(invocations_used)
}
class EnvironmentIntrospection {
+str _utc_now_iso()
+str _git_commit()
+str _aelfrice_version()
}
class SubprocessRunner {
+CompletedProcess _default_runner(cmd, out_path)
}
AdapterInvocation <.. DispatcherConfig : used in
AdapterInvocation <.. BenchRunner : used in
InvocationResult <.. BenchRunner : used in
SubprocessRunner <.. BenchRunner : used in
EnvironmentIntrospection <.. BenchRunner : used in
DispatcherConfig <|.. BenchRunner
SubprocessRunner <|.. BenchRunner
EnvironmentIntrospection <|.. BenchRunner
Class diagram for benchmarks.tolerance band classifierclassDiagram
class Verdict {
<<enum>>
PASS
WARN
FAIL
}
class BandCheck {
+tuple~str~ path
+float canonical
+float observed
+float lower
+float upper
+str band_kind
+Verdict verdict
+str note
}
class BandConfig {
<<interface>>
+dict~str, float~ DEFAULT_RELATIVE_BANDS
+float FALLBACK_RELATIVE_BAND
+float ABSOLUTE_FLOOR
}
class BandComputer {
+tuple~float, float, str~ compute_band(metric_name, canonical, overrides, floor)
+Verdict classify(canonical, observed, lower, upper)
+list~tuple~tuple~str~, float~~ _walk_leaves(obj, path)
+list~BandCheck~ check_report(canonical, observed, metric_overrides, floor)
+tuple~Verdict, dict~str, int~~ summarize(checks)
+dict~str, Any~ load_report(path)
}
BandCheck --> Verdict : uses
BandComputer --> BandCheck : produces
BandConfig <|.. BandComputer
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR implements the v2.0 reproducibility harness for benchmarks, comprising a nightly canonical runner workflow, dispatcher ( Changesv2.0 Reproducibility Harness
Sequence DiagramsequenceDiagram
participant GHA as GitHub Actions<br/>(Nightly Cron)
participant Disp as Benchmark<br/>Dispatcher
participant Adap as Benchmark<br/>Adapters
participant Tol as Tolerance<br/>Classifier
participant Res as Results<br/>Branch
GHA->>Disp: aelf bench all --canonical<br/>--out daily.json
Disp->>Disp: Select CANONICAL_INVOCATIONS
loop For each adapter invocation
Disp->>Adap: python -m adapter<br/>--output temp.json
Adap->>Adap: Run benchmark
Adap->>Disp: Write JSON results
Disp->>Disp: Capture status &<br/>parse output
end
Disp->>Disp: Merge outputs into<br/>schema-v2 report
Disp->>GHA: Write daily.json
GHA->>Tol: Load canonical +<br/>cron report
Tol->>Tol: Walk metric trees<br/>& compute bands
Tol->>Tol: Classify each leaf<br/>(pass/warn/fail)
Tol->>Tol: Summarize verdicts
Tol->>GHA: Report regression?
alt Regression detected
GHA->>GHA: Fail workflow
else No regression
GHA->>Res: Commit & push<br/>daily JSON
Res->>Res: Update branch
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| from pathlib import Path | ||
| from benchmarks import tolerance | ||
| cano = tolerance.load_report(Path('benchmarks/results/v2.0.0.json')) | ||
| obs = tolerance.load_report(Path('${{ steps.bench.outputs.out }}')) |
| import subprocess | ||
| import sys | ||
| import time | ||
| from dataclasses import dataclass, field |
|
|
||
| import json | ||
| import subprocess | ||
| from dataclasses import dataclass |
| from __future__ import annotations | ||
|
|
||
| import json | ||
| from pathlib import Path |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, 4 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- In
benchmarks/run.py::main_all, theno adapters matchederror message is built afterinvocationshas already been filtered, so theavailable:list will always be empty; capture the full set of adapters before filtering so the message can enumerate valid options. - In
.github/workflows/bench-canonical.yml, the conditionif: hashFiles(steps.bench.outputs.out) != ''is invalid becausehashFilesonly accepts string literals; use a direct check on the output (e.g.if: steps.bench.outputs.out != '') or a static path instead. - The type annotation for
_headline_cut_for(dict[str, list[str] | dict[str, Any]]) does not match the actual structure being returned (lists of dicts), which may confuse static type checkers; tighten the annotation to reflectlist[dict[str, Any]].
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `benchmarks/run.py::main_all`, the `no adapters matched` error message is built after `invocations` has already been filtered, so the `available:` list will always be empty; capture the full set of adapters before filtering so the message can enumerate valid options.
- In `.github/workflows/bench-canonical.yml`, the condition `if: hashFiles(steps.bench.outputs.out) != ''` is invalid because `hashFiles` only accepts string literals; use a direct check on the output (e.g. `if: steps.bench.outputs.out != ''`) or a static path instead.
- The type annotation for `_headline_cut_for` (`dict[str, list[str] | dict[str, Any]]`) does not match the actual structure being returned (lists of dicts), which may confuse static type checkers; tighten the annotation to reflect `list[dict[str, Any]]`.
## Individual Comments
### Comment 1
<location path="benchmarks/run.py" line_range="280-284" />
<code_context>
+ "error", 2 if any was "skipped_data_missing" and none was "error").
+ """
+ invocations = SMOKE_INVOCATIONS if smoke else CANONICAL_INVOCATIONS
+ if adapters is not None:
+ invocations = tuple(i for i in invocations if i.adapter in adapters)
+ if not invocations:
+ raise SystemExit(
+ f"no adapters matched filter {adapters!r}; "
+ "available: " + ", ".join(sorted({i.adapter for i in invocations}))
+ )
</code_context>
<issue_to_address>
**issue (bug_risk):** Filtering adapters mutates the `invocations` set, breaking the error message and making adapter lists misleading.
In `main_all`, `invocations` is overwritten with the filtered tuple and then reused for the "available" list in the error path. When no adapters match, `invocations` is empty, so the message shows no available adapters and loses the original registry context. Keep the original tuple (e.g. `all_invocations = SMOKE_INVOCATIONS if smoke else CANONICAL_INVOCATIONS`), derive the filtered tuple from that, and use `all_invocations` when building the "available" list. You could also normalize the `adapters` filter (e.g. `.lower()`) to avoid case-sensitivity issues.
</issue_to_address>
### Comment 2
<location path="benchmarks/tolerance.py" line_range="122-126" />
<code_context>
+ metadata like `_status`, `_elapsed_sec`).
+ """
+ leaves: list[tuple[tuple[str, ...], float]] = []
+ if isinstance(obj, dict):
+ for k, v in obj.items():
+ if isinstance(k, str) and k.startswith("_"):
+ continue
+ leaves.extend(_walk_leaves(v, (*path, str(k))))
+ elif isinstance(obj, (int, float)) and not isinstance(obj, bool):
+ leaves.append((path, float(obj)))
</code_context>
<issue_to_address>
**issue (bug_risk):** Skipping all keys starting with '_' means single-invocation adapter results (stored under "_") will never be band-checked.
`_walk_leaves` skips all dict keys starting with `_`, but `build_report`/`_merge` stores single‑invocation adapter results under the `_` key (e.g. `results['locomo']['_'] = {...}`). As a result, you never traverse into that subtree, so band checks are never applied to single‑invocation adapters (only multi‑invocation ones with non‑`_` keys are covered).
To fix this, consider narrowing the skip condition to specific metadata keys (e.g. `_status`, `_elapsed_sec`) or only skipping non‑dict/non‑metric values, so that the `_` bucket for actual metrics is still traversed.
</issue_to_address>
### Comment 3
<location path="tests/test_bench_dispatcher.py" line_range="181-190" />
<code_context>
+ assert "big" in entry["args"]
+
+
+def test_runner_crash_recorded_as_error(tmp_path):
+ """If the runner raises, status=error and message captured."""
+ def crashing_runner(cmd, out_path):
+ raise RuntimeError("boom")
+ out = tmp_path / "crash.json"
+ rc = bench_run.main_all(
+ out_path=out, canonical=False, smoke=True,
+ runner=crashing_runner, tmp_root=tmp_path / "tmp",
+ )
+ assert rc == 1
+ data = json.loads(out.read_text())
+ err = data["results"]["amabench"]["_"]
+ assert err["status"] == "error"
+ assert "boom" in err["error_message"]
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for the case where an adapter exits 0 but fails to write its output file
There’s a code path in `run_invocation` that sets status=`error` when the adapter exits with code 0 but the output file is missing. Current tests don’t cover this. Please add a test using a stub runner that returns `returncode=0` without writing `out_path`, and assert that this path is taken and that `main_all` returns 1.
</issue_to_address>
### Comment 4
<location path="tests/test_bench_dispatcher.py" line_range="122-131" />
<code_context>
+ ) == 11
+
+
+def test_skipped_data_missing_propagates(tmp_path):
+ """Adapter exit-code 2 → status=skipped_data_missing, overall rc=2."""
+ out = tmp_path / "skip.json"
+ rc = bench_run.main_all(
+ out_path=out, canonical=False, smoke=True,
+ runner=_make_runner(per_adapter_returncode={"amabench": 2}),
+ tmp_root=tmp_path / "tmp",
+ )
+ assert rc == 2
+ data = json.loads(out.read_text())
+ assert data["results"]["amabench"]["_"]["status"] == "skipped_data_missing"
+ assert "data missing" in data["results"]["amabench"]["_"]["error_message"]
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider a test for malformed JSON output from the adapter
There’s a specific branch handling `json.JSONDecodeError` when reading the adapter’s `--output` file that isn’t exercised by tests. Please add a case where the stub runner writes invalid JSON (e.g., a plain string or truncated JSON) with `returncode=0`, and assert that the status is `"error"`, the error_message indicates a parse failure, and `main_all` returns 1 to cover this path.
Suggested implementation:
```python
assert sum(
len([k for k in v if k != "_"]) or (1 if "_" in v else 0)
for v in data["results"].values()
) == 11
def test_malformed_adapter_output_sets_error(tmp_path):
"""Malformed adapter --output → status=error, overall rc=1."""
out = tmp_path / "malformed.json"
rc = bench_run.main_all(
out_path=out,
canonical=False,
smoke=True,
runner=_make_runner(
per_adapter_returncode={"amabench": 0},
per_adapter_output={"amabench": "{not-json"},
),
tmp_root=tmp_path / "tmp",
)
assert rc == 1
data = json.loads(out.read_text())
result = data["results"]["amabench"]["_"]
assert result["status"] == "error"
assert (
"parse" in result["error_message"].lower()
or "json" in result["error_message"].lower()
or "decode" in result["error_message"].lower()
)
The dispatcher subprocesses each adapter, so all tests stub the runner
```
To make this test pass, `tests/test_bench_dispatcher.py` (or the helper module that defines `_make_runner`) will also need:
1. Updating `_make_runner` to accept an optional `per_adapter_output` mapping, e.g. `def _make_runner(*, per_adapter_returncode=None, per_adapter_output=None, ...)`.
2. In the returned stub runner, when invoked for adapter `name` (e.g. `"amabench"`), detect the `--output` argument in the `cmd` list, and:
- If `per_adapter_output` contains an entry for that adapter, write its string value directly to the resolved `--output` path (without JSON validation), so `" {not-json"` triggers a `json.JSONDecodeError` in the dispatcher.
- Preserve existing behavior for any other adapters or when `per_adapter_output` is `None`.
3. Ensure the stub runner still returns a `subprocess.CompletedProcess` (or equivalent) with `returncode` taken from `per_adapter_returncode[name]` (defaulting to 0) so that this test exercises the branch where the adapter exits successfully but its output cannot be parsed.
</issue_to_address>
### Comment 5
<location path="benchmarks/run.py" line_range="187" />
<code_context>
return subprocess.run(cmd, capture_output=True, text=True, check=False)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+122
to
+126
| if isinstance(obj, dict): | ||
| for k, v in obj.items(): | ||
| if isinstance(k, str) and k.startswith("_"): | ||
| continue | ||
| leaves.extend(_walk_leaves(v, (*path, str(k)))) |
There was a problem hiding this comment.
issue (bug_risk): Skipping all keys starting with '' means single-invocation adapter results (stored under "") will never be band-checked.
_walk_leaves skips all dict keys starting with _, but build_report/_merge stores single‑invocation adapter results under the _ key (e.g. results['locomo']['_'] = {...}). As a result, you never traverse into that subtree, so band checks are never applied to single‑invocation adapters (only multi‑invocation ones with non‑_ keys are covered).
To fix this, consider narrowing the skip condition to specific metadata keys (e.g. _status, _elapsed_sec) or only skipping non‑dict/non‑metric values, so that the _ bucket for actual metrics is still traversed.
Comment on lines
+181
to
+190
| def test_runner_crash_recorded_as_error(tmp_path): | ||
| """If the runner raises, status=error and message captured.""" | ||
| def crashing_runner(cmd, out_path): | ||
| raise RuntimeError("boom") | ||
| out = tmp_path / "crash.json" | ||
| rc = bench_run.main_all( | ||
| out_path=out, canonical=False, smoke=True, | ||
| runner=crashing_runner, tmp_root=tmp_path / "tmp", | ||
| ) | ||
| assert rc == 1 |
There was a problem hiding this comment.
suggestion (testing): Add a test for the case where an adapter exits 0 but fails to write its output file
There’s a code path in run_invocation that sets status=error when the adapter exits with code 0 but the output file is missing. Current tests don’t cover this. Please add a test using a stub runner that returns returncode=0 without writing out_path, and assert that this path is taken and that main_all returns 1.
Comment on lines
+122
to
+131
| def test_skipped_data_missing_propagates(tmp_path): | ||
| """Adapter exit-code 2 → status=skipped_data_missing, overall rc=2.""" | ||
| out = tmp_path / "skip.json" | ||
| rc = bench_run.main_all( | ||
| out_path=out, canonical=False, smoke=True, | ||
| runner=_make_runner(per_adapter_returncode={"amabench": 2}), | ||
| tmp_root=tmp_path / "tmp", | ||
| ) | ||
| assert rc == 2 | ||
| data = json.loads(out.read_text()) |
There was a problem hiding this comment.
suggestion (testing): Consider a test for malformed JSON output from the adapter
There’s a specific branch handling json.JSONDecodeError when reading the adapter’s --output file that isn’t exercised by tests. Please add a case where the stub runner writes invalid JSON (e.g., a plain string or truncated JSON) with returncode=0, and assert that the status is "error", the error_message indicates a parse failure, and main_all returns 1 to cover this path.
Suggested implementation:
assert sum(
len([k for k in v if k != "_"]) or (1 if "_" in v else 0)
for v in data["results"].values()
) == 11
def test_malformed_adapter_output_sets_error(tmp_path):
"""Malformed adapter --output → status=error, overall rc=1."""
out = tmp_path / "malformed.json"
rc = bench_run.main_all(
out_path=out,
canonical=False,
smoke=True,
runner=_make_runner(
per_adapter_returncode={"amabench": 0},
per_adapter_output={"amabench": "{not-json"},
),
tmp_root=tmp_path / "tmp",
)
assert rc == 1
data = json.loads(out.read_text())
result = data["results"]["amabench"]["_"]
assert result["status"] == "error"
assert (
"parse" in result["error_message"].lower()
or "json" in result["error_message"].lower()
or "decode" in result["error_message"].lower()
)
The dispatcher subprocesses each adapter, so all tests stub the runnerTo make this test pass, tests/test_bench_dispatcher.py (or the helper module that defines _make_runner) will also need:
- Updating
_make_runnerto accept an optionalper_adapter_outputmapping, e.g.def _make_runner(*, per_adapter_returncode=None, per_adapter_output=None, ...). - In the returned stub runner, when invoked for adapter
name(e.g."amabench"), detect the--outputargument in thecmdlist, and:- If
per_adapter_outputcontains an entry for that adapter, write its string value directly to the resolved--outputpath (without JSON validation), so" {not-json"triggers ajson.JSONDecodeErrorin the dispatcher. - Preserve existing behavior for any other adapters or when
per_adapter_outputisNone.
- If
- Ensure the stub runner still returns a
subprocess.CompletedProcess(or equivalent) withreturncodetaken fromper_adapter_returncode[name](defaulting to 0) so that this test exercises the branch where the adapter exits successfully but its output cannot be parsed.
|
|
||
|
|
||
| def _default_runner(cmd: list[str], out_path: Path) -> subprocess.CompletedProcess[str]: | ||
| return subprocess.run(cmd, capture_output=True, text=True, check=False) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/bench-canonical.yml:
- Around line 91-109: The canonical report is an uncalibrated skeleton so the
current script can falsely pass; after loading the canonical report (cano =
tolerance.load_report(...)) add a check for the calibration marker (e.g.,
inspect cano['_calibration_pass_required'] or the emptied 'results' tree) and if
calibration is required or results are empty exit non-zero (fail fast) with a
clear message before calling tolerance.check_report/summarize; update the inline
CI python block that calls tolerance.load_report, check_report, and summarize to
perform this calibration check and abort early when the canonical is not yet
calibrated.
- Around line 75-89: The "Run aelf bench all --canonical" step (id: bench) must
not allow hard failures to mark the workflow green — remove or set
continue-on-error: false for the bench step so the `uv run aelf bench all
--canonical --out "${out}"` command fails the job on error; ensure downstream
band-check still runs as a separate step that can inspect the bench outputs but
does not mask the bench step failure.
In `@benchmarks/run.py`:
- Around line 186-187: The _default_runner currently calls subprocess.run
without a timeout, allowing a hung adapter to block the harness; modify the
_default_runner function to include a timeout argument when calling
subprocess.run (e.g., subprocess.run(cmd, capture_output=True, text=True,
check=False, timeout=300)) or alternatively add an optional timeout parameter to
_default_runner (def _default_runner(cmd: list[str], out_path: Path, timeout:
int = 300)) and pass it through to subprocess.run so stuck processes are
terminated after the configured timeout.
- Around line 282-286: The error message builds "available:" from the
already-empty filtered variable invocations; change the raise SystemExit in the
adapter-filter branch so it computes available adapters from the original
unfiltered source (the list/iterator used before filtering) instead of
invocations, and include that computed set (e.g., the unique adapter names) in
the processLogger/raise message—update the block around the raise SystemExit to
reference that original collection when building ", ".join(sorted({...})) so the
message shows actual available adapters.
- Around line 143-144: The current out_path uses tmp_root /
f"{inv.adapter}_{inv.sub_key or 'all'}.json" which can collide across concurrent
runs; change out_path generation in benchmarks/run.py to produce a unique
per-invocation filename (for example by appending a uuid4, timestamp+pid, or
using tempfile to create a unique temp filename) so that inv.adapter,
inv.sub_key and inv.module still appear in the name but each invocation gets its
own file; update the cmd to use that new unique out_path variable.
In `@docs/v2_reproducibility_harness.md`:
- Around line 225-231: The runbook's Step 6 references pushing to the wrong
branch name; update the documented branch name to match the workflow used in the
PR by replacing "benchmark-results" with "bench-canonical-results" in the
Reproducibility section and any related entries (Step 6, README badge,
docs/COMMANDS.md mention), and ensure the
`.github/workflows/bench-canonical.yml` workflow branch target and the runbook
text are consistent so operators are pointed to the actual
`bench-canonical-results` branch.
In `@README.md`:
- Around line 137-139: The README overstates automation for the "Bench
Canonical" badge (the README badge block referencing the nightly cron and the
bench-canonical-results branch); update the README.md badge text to indicate the
badge is pending/manual (not live/auto-updating) and clarify that the workflow
in this PR does not rewrite the README badge block; mention "Bench Canonical",
"bench-canonical-results" and the README badge block so the change is easy to
find and ensure the docs/v2_reproducibility_harness.md link remains correct.
In `@src/aelfrice/cli.py`:
- Around line 4301-4320: The new optional flags (bench_out, bench_canonical,
bench_adapters, bench_smoke) are never parsed because p_bench already defines a
positional argument named rest with nargs=argparse.REMAINDER before them;
argparse will consume everything after the positional into rest. Fix by moving
the p_bench.add_argument(..., dest="rest", nargs=argparse.REMAINDER) declaration
so it comes after the optional flags, or change that positional to nargs="*" (or
otherwise optional) and adjust downstream logic to accept an empty rest; ensure
you update references to rest parsing code accordingly so flags like bench_out,
bench_canonical, bench_adapters, and bench_smoke are recognized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a4e56680-2a49-4093-9872-9d76a141ef13
📒 Files selected for processing (11)
.github/workflows/bench-canonical.ymlREADME.mdbenchmarks/results/v2.0.0.jsonbenchmarks/run.pybenchmarks/tolerance.pydocs/COMMANDS.mddocs/v2_reproducibility_harness.mdsrc/aelfrice/cli.pytests/test_bench_dispatcher.pytests/test_bench_tolerance.pytests/test_benchmarks_dir.py
Comment on lines
+75
to
+89
| - name: Run aelf bench all --canonical | ||
| id: bench | ||
| run: | | ||
| set -euo pipefail | ||
| today=$(date -u +%Y-%m-%d) | ||
| out=".bench-results-branch/v2.0.0-cron-${today}.json" | ||
| # `--canonical` so the dispatcher refuses if the cut doesn't | ||
| # match CANONICAL_INVOCATIONS. The merged JSON's label still | ||
| # reads `v2.0.0 cron <ts>` (canonical-vs-cron is by filename, | ||
| # not by --canonical flag inside the run). | ||
| uv run aelf bench all --canonical --out "${out}" | ||
| echo "out=${out}" >> "$GITHUB_OUTPUT" | ||
| # Continue on band-check failure so we still commit the cron | ||
| # entry; `Band-check` step below sets the actual job status. | ||
| continue-on-error: true |
There was a problem hiding this comment.
Don't let benchmark execution failures go green.
Because this step has continue-on-error: true and the final gate only checks steps.bandcheck.outcome, a hard failure in uv run aelf bench all ... will skip the band-check and still leave the workflow successful. That turns adapter crashes, parser failures, and missing-data failures into false-green nightlies.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/bench-canonical.yml around lines 75 - 89, The "Run aelf
bench all --canonical" step (id: bench) must not allow hard failures to mark the
workflow green — remove or set continue-on-error: false for the bench step so
the `uv run aelf bench all --canonical --out "${out}"` command fails the job on
error; ensure downstream band-check still runs as a separate step that can
inspect the bench outputs but does not mask the bench step failure.
Comment on lines
+143
to
+144
| out_path = tmp_root / f"{inv.adapter}_{inv.sub_key or 'all'}.json" | ||
| cmd = [sys.executable, "-m", inv.module, *inv.args, "--output", str(out_path)] |
There was a problem hiding this comment.
Use unique per-invocation temp output files to avoid cross-run collisions.
{adapter}_{sub_key}.json under a shared tmp dir can be overwritten by concurrent bench all runs, producing mixed or flaky results.
Suggested fix
+import tempfile
...
- out_path = tmp_root / f"{inv.adapter}_{inv.sub_key or 'all'}.json"
+ safe_sub = inv.sub_key or "all"
+ fd, tmp_name = tempfile.mkstemp(
+ prefix=f"{inv.adapter}_{safe_sub}_",
+ suffix=".json",
+ dir=str(tmp_root),
+ )
+ os.close(fd)
+ out_path = Path(tmp_name)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/run.py` around lines 143 - 144, The current out_path uses tmp_root
/ f"{inv.adapter}_{inv.sub_key or 'all'}.json" which can collide across
concurrent runs; change out_path generation in benchmarks/run.py to produce a
unique per-invocation filename (for example by appending a uuid4, timestamp+pid,
or using tempfile to create a unique temp filename) so that inv.adapter,
inv.sub_key and inv.module still appear in the name but each invocation gets its
own file; update the cmd to use that new unique out_path variable.
Comment on lines
+186
to
+187
| def _default_runner(cmd: list[str], out_path: Path) -> subprocess.CompletedProcess[str]: | ||
| return subprocess.run(cmd, capture_output=True, text=True, check=False) |
There was a problem hiding this comment.
Add a subprocess timeout to prevent a hung adapter from blocking the harness.
A single stuck adapter process currently blocks the whole run indefinitely.
Suggested fix
-def _default_runner(cmd: list[str], out_path: Path) -> subprocess.CompletedProcess[str]:
- return subprocess.run(cmd, capture_output=True, text=True, check=False)
+def _default_runner(cmd: list[str], out_path: Path) -> subprocess.CompletedProcess[str]:
+ return subprocess.run(
+ cmd,
+ capture_output=True,
+ text=True,
+ check=False,
+ timeout=60 * 30, # 30 min per adapter, adjust as needed
+ )🧰 Tools
🪛 Ruff (0.15.12)
[error] 187-187: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/run.py` around lines 186 - 187, The _default_runner currently
calls subprocess.run without a timeout, allowing a hung adapter to block the
harness; modify the _default_runner function to include a timeout argument when
calling subprocess.run (e.g., subprocess.run(cmd, capture_output=True,
text=True, check=False, timeout=300)) or alternatively add an optional timeout
parameter to _default_runner (def _default_runner(cmd: list[str], out_path:
Path, timeout: int = 300)) and pass it through to subprocess.run so stuck
processes are terminated after the configured timeout.
robotrocketscience
added a commit
that referenced
this pull request
May 7, 2026
Two bugs caught by the first local smoke run after PR #465 went up: 1. `from benchmarks import run` failed at runtime — `benchmarks/` is the top-level academic-suite directory and per pyproject.toml is dev-only / not packaged. `aelf bench all` is therefore reachable only from a source checkout. Added a cwd-presence check (`benchmarks/run.py` must exist), pushes cwd onto sys.path before importing, and emits a clear pointer ("git clone … && uv sync && aelf bench all") when run from an installed-only context. 2. `rest = nargs=argparse.REMAINDER` on the bench subparser was dead code (`args.rest` is read nowhere) but it consumed every downstream optional flag, including the new --out / --canonical / --adapters / --smoke flags. Dropped it; new flags now parse correctly. Smoke verified end-to-end after fix: uv run aelf bench all --smoke --out /tmp/bench-smoke.json → mab/Conflict_Resolution: ok (49.5s), amabench: ok (6.4s) → schema-v2 JSON written, real metrics inside. All 46 dispatcher/tolerance/inert-target tests still green.
|
This PR is now behind Auto-rebase was removed because the bot has no signing key; rebasing as the bot strips author signatures and the |
robotrocketscience
force-pushed
the
feat/issue-437-bench-all-harness
branch
from
May 8, 2026 04:13
92e094a to
8ccb9f3
Compare
robotrocketscience
added a commit
that referenced
this pull request
May 8, 2026
Two bugs caught by the first local smoke run after PR #465 went up: 1. `from benchmarks import run` failed at runtime — `benchmarks/` is the top-level academic-suite directory and per pyproject.toml is dev-only / not packaged. `aelf bench all` is therefore reachable only from a source checkout. Added a cwd-presence check (`benchmarks/run.py` must exist), pushes cwd onto sys.path before importing, and emits a clear pointer ("git clone … && uv sync && aelf bench all") when run from an installed-only context. 2. `rest = nargs=argparse.REMAINDER` on the bench subparser was dead code (`args.rest` is read nowhere) but it consumed every downstream optional flag, including the new --out / --canonical / --adapters / --smoke flags. Dropped it; new flags now parse correctly. Smoke verified end-to-end after fix: uv run aelf bench all --smoke --out /tmp/bench-smoke.json → mab/Conflict_Resolution: ok (49.5s), amabench: ok (6.4s) → schema-v2 JSON written, real metrics inside. All 46 dispatcher/tolerance/inert-target tests still green.
robotrocketscience
added a commit
that referenced
this pull request
May 8, 2026
The error message at `benchmarks.run.main_all` built the `available:` list from `invocations` after the filter applied, producing an empty list whenever the filter matched nothing — exactly the case the message exists to help with. Capture the available-adapters set BEFORE filtering so the user sees the real list of valid adapter names when their --adapters flag fails to match. Refs Sourcery review on #465.
robotrocketscience
added a commit
that referenced
this pull request
May 8, 2026
…es (#437) `hashFiles()` only accepts string literals as arguments — passing `steps.bench.outputs.out` (an expression) is silently invalid and returns an empty hash, so the band-check step's `if: hashFiles(steps.bench.outputs.out) != ''` condition was always-false and the band-check was skipped on every cron run. Gate on `steps.bench.outputs.out != ''` instead — when the bench step succeeded (under continue-on-error), it sets the `out` output; when it errored, the output is unset. Refs Sourcery review on #465.
Capture the 2026-05-06 ratification of all eight design asks in the spec memo. Seven resolved against the spec's recommendations; #2 (what "all" means) overridden — operator picked full benchmarks instead of the recommended sized headline cut. Adds a Ratification section at the bottom with the resolved table, the superseded headline-cut numbers (LongMemEval full vs oracle subset, StructMemEval --bench big vs small), the multi-hour cron-runtime implication, and the 8-step implementation order this PR follows.
Subprocess-per-adapter dispatcher that fans out the canonical headline cut (full per the 2026-05-06 ratification), parses each adapter's --output JSON, and merges into one schema-v2 file. Registry-driven: CANONICAL_INVOCATIONS lists every (adapter, sub_key, module, args) tuple. MAB and StructMemEval are multi-invocation (4 splits / 4 tasks); the rest are single-invocation. SMOKE_INVOCATIONS is a separate ≤2-minute registry the PR-CI tier consumes. --canonical flag asserts the active invocation set matches CANONICAL_INVOCATIONS verbatim; mismatched cut → SystemExit so a partial run cannot overwrite v2.0.0.json. Adapter exit-code contract: 0 ok / 2 skipped_data_missing / non-zero error. The runner is injectable so unit tests can exercise the merge / schema / cut-mismatch paths without spawning real benchmarks. Reachable as `python -m benchmarks.run` immediately; cli.py wiring for `aelf bench all` lands in the next commit.
`target == "all"` short-circuits to benchmarks.run.main_all() with the new --out / --canonical / --adapters / --smoke flags. Removes "all" from _BENCH_INERT_TARGETS so the inert path no longer shadows. Single-name targets (mab, locomo, longmemeval, structmemeval, amabench) stay inert in cli.py — the dispatcher is the only v1 wrapper. Direct adapter invocation (`python -m benchmarks.<name>_adapter`) remains the path until a sub-subcommand wrapper lands. Docstring updated to point at the 2026-05-06 ratification.
Relative-with-floor band policy ratified 2026-05-06. Each canonical metric value gets a [lower, upper] band: - Relative: ±X% of canonical, X picked from per-metric defaults (F1 ±7%, exact-match ±10%, latency ±25%) or per-metric overrides in the canonical JSON. - Absolute floor: ±0.02 (2pp on a 0..1 metric) when relative would be tighter — prevents tiny-value flapping. - Soft warn: drift inside band but >50% of band half-width emits warn (workflow notice) instead of pass. Walks the canonical results tree leaf-by-leaf so per-adapter shape (MAB by-split, StructMemEval by-task, etc.) needs no special-casing. Missing leaves in the observed report are FAIL not silent. Extra leaves are ignored — canonical is source of truth for what's checked. `load_report` enforces schema_version=2 on read.
27 tests total, all green. Dispatcher (10 tests): - registry counts (4 MAB + 1 LoCoMo + 1 LongMemEval + 4 StructMemEval + 1 AMA-Bench = 11 canonical, 2 smoke) - end-to-end smoke run produces schema-v2 JSON - --canonical with partial --adapters refuses - --canonical with full set accepts - adapter exit-code 2 → status=skipped_data_missing, overall rc=2 - error overrides skip in overall rc - unknown adapter filter raises - headline_cut declaration recorded (LongMemEval full / StructMemEval --bench big — captures the override from spec recommendation) - runner-crash exception → status=error Tolerance (17 tests): - relative band for f1 / exact_match / latency - absolute floor activates for tiny canonical values - per-metric override takes precedence - pass/warn/fail classification at boundaries - nested-leaf walk over adapter shapes - missing leaves → FAIL, extras → ignored - underscore-prefixed metadata keys skipped - summarize: FAIL dominates WARN dominates PASS - schema_version=2 enforcement on read Both modules use injectable runners / pure-data inputs so tests run in <1s without spawning real benchmark subprocesses.
Daily cron at 05:00 UTC (staggered after replay-soak's 04:00) runs `aelf bench all --canonical` and writes the merged JSON to a dedicated `bench-canonical-results` branch — same dedicated-branch pattern replay-soak adopted in #461 to sidestep the main-branch ruleset (`required_signatures`, no direct push). No long-lived signing key required. Band-check step compares the cron entry against `benchmarks/results/v2.0.0.json` via benchmarks.tolerance and fails the job on any band-busting regression. WARN-tier drift (inside band, >50% of half-width) commits and passes — the workflow notice surfaces without paging. timeout-minutes: 360 (6h) for the full-cut runtime; tunable down once real cron entries land. continue-on-error on the bench step so a crashing adapter still commits the partial JSON for diffability. PR smoke tier deferred — pytest matrix already exercises the dispatcher and tolerance modules with stubs, which catches harness regressions in the same way the spec's PR smoke job would. A real PR smoke against tiny adapter inputs needs offline fixtures (`tests/fixtures/bench_smoke/`) which is substantial scope; tracked as a follow-up.
…l` (#437) README: - Reproducibility badge between OSSInsight and the lede. Wrapped in `bench-canonical-badge:start/end` markers so the cron can rewrite just the badge line without touching surrounding text. Initial state is `pending first canonical run` (lightgrey) until the operator runs the calibration pass. - New `## Reproducibility` section before `## Roadmap` documenting `aelf bench all --canonical` as the ship-gate command and pointing at docs/v2_reproducibility_harness.md. COMMANDS: - New row under Diagnostics for `bench all --out PATH ...`. Documents --canonical / --adapters / --smoke and the 0/1/2 exit-code contract. - Existing `bench [--top-k N]` row (synthetic harness) stays as-is.
…anonical (#437) Bug spotted while writing the skeleton v2.0.0.json: the canonical schema declares a `metric_overrides` field, but check_report ignored it unless the caller explicitly passed one. Cron-time band-checking would have used DEFAULT_RELATIVE_BANDS for everything, silently ignoring per-metric overrides operators wrote into the canonical. check_report now falls back to canonical["metric_overrides"] when the caller doesn't pass one. Explicit caller-passed overrides still take precedence (used by tests). +2 tests pin the fallback behavior.
Schema-v2 skeleton with the canonical headline_cut declaration matching
benchmarks.run.CANONICAL_INVOCATIONS exactly: 4 MAB splits, 1 LoCoMo,
1 LongMemEval (full per the override), 4 StructMemEval tasks
(--bench big), 1 AMA-Bench. results: {} until the operator runs the
calibration pass.
`_calibration_pass_required: true` flags the file as not-yet-canonical;
`_calibration_protocol` describes the procedure (3+ runs, observed
range × 1.5 as per-metric override band, full data dirs and judge
keys present). The cron's band-check step against this skeleton is
a no-op (no leaves to check, summarize → PASS) until results is
populated, which is the intended behavior — harness lands first,
canonical numbers land in a follow-up commit by the operator.
…ation (#437) Removes "all" from the parametrized inert-target list (it now dispatches via benchmarks.run.main_all) and adds a new test_aelf_bench_all_requires_out test pinning the --out-required exit-2 contract. The other inert targets (mab, locomo, longmemeval, structmemeval, amabench) still emit the v1.0 pointer-to-README message — only `all` moved.
10 atomic commits total (this is #11). Lands the harness, dispatcher, band-checker, nightly cron, docs, and skeleton canonical JSON. The calibration pass that fills v2.0.0.json with real numbers is the explicit operator action this gate hands off. Verification: - pytest: 2624 passed, 41 skipped (no regressions vs main). - New tests: 29 unit (10 dispatcher / 19 tolerance, all stub-driven, <1s wall) + 2 inert-list/integration adjustments. - Discretion grep against entire branch diff vs github/main: clean. - benchmarks.run loads + canonical_invocations matches the spec (4 MAB + 1 LoCoMo + 1 LongMemEval + 4 StructMemEval + 1 AMA = 11). - benchmarks.tolerance compute_band/classify/check_report exercised end-to-end via tests including override-from-canonical fallback. - Workflow YAML parses; uses the same dedicated-branch pattern replay-soak adopted in #461 (sidesteps main ruleset). Blockers (require user decision before next phase): - [user] Calibration pass: run `aelf bench all --canonical --out benchmarks/results/v2.0.0.json` ≥3 times against full data dirs (HF caches populated, /tmp/LoCoMo + /tmp/StructMemEval present, ANTHROPIC_API_KEY for judge metrics if in scope), take observed range × 1.5 as the per-metric override band, replace the skeleton with the result. Until this lands, the cron's band-check is a no-op (no leaves to check → PASS by default). - [user] PR-smoke fixtures (offline): not landed in this PR. Spec recommended `tests/fixtures/bench_smoke/` pinned fixtures so a 2-min PR smoke job can exercise real adapter shapes without HF download. This needs licensed-content review (LoCoMo turns, MAB QA) and is scope for a follow-up. Pytest unit tests cover dispatcher regressions in the meantime via stubs. - [user] README badge cron-rewrite path: badge ships with placeholder state ("pending first canonical run"). The cron's `bench-canonical-results` branch commit is wired, but the README in-place rewrite (between `bench-canonical-badge:start/end` markers) is not. Wire it in a follow-up after first canonical run, or wire it now if you want — small sed in the workflow. Open questions: - adapter exit-code 2 contract (skipped_data_missing): dispatcher honors it but no adapter currently emits it. Spec called for it. Can land per-adapter as separate small PRs once data-dir plumbing is decided. Rollback: - `git revert 2a11f22..HEAD` (or revert just the `feat(cli)` commit b5564d1 to disable `aelf bench all` while keeping the modules available via `python -m benchmarks.run`).
Two bugs caught by the first local smoke run after PR #465 went up: 1. `from benchmarks import run` failed at runtime — `benchmarks/` is the top-level academic-suite directory and per pyproject.toml is dev-only / not packaged. `aelf bench all` is therefore reachable only from a source checkout. Added a cwd-presence check (`benchmarks/run.py` must exist), pushes cwd onto sys.path before importing, and emits a clear pointer ("git clone … && uv sync && aelf bench all") when run from an installed-only context. 2. `rest = nargs=argparse.REMAINDER` on the bench subparser was dead code (`args.rest` is read nowhere) but it consumed every downstream optional flag, including the new --out / --canonical / --adapters / --smoke flags. Dropped it; new flags now parse correctly. Smoke verified end-to-end after fix: uv run aelf bench all --smoke --out /tmp/bench-smoke.json → mab/Conflict_Resolution: ok (49.5s), amabench: ok (6.4s) → schema-v2 JSON written, real metrics inside. All 46 dispatcher/tolerance/inert-target tests still green.
First-canonical-run finding: raw merged JSON was 37MB, dominated by per_question lists (6,667 rows across 6 adapters at full cut). The band-check doesn't read per-row scores; canonical contract is summary metrics that change rarely. Stripping per_question shrinks v2.0.0.json from 37MB → 6.5KB without information loss for the gate. `_strip_detail()` walks the output recursively; field set is configurable via `_DETAIL_FIELDS_TO_STRIP` (currently just `per_question`; add to the set if a future adapter emits another per-row list). +1 test pinning the behavior with a 2000-row payload. Per-row detail still exists in the in-memory output and the adapter's --output write path; only the merged dispatcher JSON is stripped.
… it (#437) Same calibration finding as the per_question strip: dispatcher-level metadata fields (status, elapsed_sec, error_message) were exposed as peers of the adapter's `output` block. tolerance.check_report walks all numeric leaves under `results`; elapsed_sec varies every run and would FAIL every band-check by design. Renamed to `_status` / `_elapsed_sec` / `_error_message` so the existing underscore-prefix skip in `_walk_leaves` keeps them out of the metric walk. Tests updated; same 47 pass.
…bers (#437) Replaces the skeleton with the 2026-05-07 canonical pass. 6 of 11 invocations green (MAB ×4, LongMemEval, AMA-Bench); 5 errored due to missing /tmp/ data dirs (LoCoMo + StructMemEval ×4). Total wall ~33 min on the local checkout. Headline numbers in this canonical: - MAB f1: 0.0001 (Test_Time_Learning) → 0.1811 (Long_Range_Understanding) - MAB substring_em: 0.0234 → 0.7025 (Conflict_Resolution wins big) - LongMemEval: avg_beliefs/q = 49.15, avg_latency_ms = 7.8, n=500 - AMA-Bench: 208 episodes, 2496 QA (no aggregate F1 — adapter-side aggregation gap; counts only) Per-metric override bands not calibrated yet (3+ runs × 1.5 spread is a follow-up). Defaults from `benchmarks.tolerance` apply. Spec memo updated with § First calibration pass: per-invocation status table, three dispatcher-level fixes that landed alongside, and the open follow-ups (adapter exit-code-2 contract, AMA aggregation, LongMemEval LLM-judge pass, dispatcher-side path scrubbing). Local-run absolute paths in error messages sanitized by hand: $HOME / $TMPDIR substitution before commit. CI cron writes from /home/runner/... so this isn't a recurring concern; flagged as a follow-up if an operator re-runs locally.
…#437) First canonical pass landed real numbers for 6 of 11 invocations (MAB ×4, LongMemEval, AMA-Bench). LoCoMo and StructMemEval ×4 errored on missing /tmp/ data dirs and stay at status="error" until operator populates the data and re-runs those adapters. Badge color: yellow (partial), not green (full). Cron will flip it on the next pass that brings all 11 to ok.
The error message at `benchmarks.run.main_all` built the `available:` list from `invocations` after the filter applied, producing an empty list whenever the filter matched nothing — exactly the case the message exists to help with. Capture the available-adapters set BEFORE filtering so the user sees the real list of valid adapter names when their --adapters flag fails to match. Refs Sourcery review on #465.
…es (#437) `hashFiles()` only accepts string literals as arguments — passing `steps.bench.outputs.out` (an expression) is silently invalid and returns an empty hash, so the band-check step's `if: hashFiles(steps.bench.outputs.out) != ''` condition was always-false and the band-check was skipped on every cron run. Gate on `steps.bench.outputs.out != ''` instead — when the bench step succeeded (under continue-on-error), it sets the `out` output; when it errored, the output is unset. Refs Sourcery review on #465.
robotrocketscience
force-pushed
the
feat/issue-437-bench-all-harness
branch
from
May 8, 2026 04:16
8ccb9f3 to
a279f7a
Compare
This was referenced May 8, 2026
Closed
robotrocketscience
added a commit
that referenced
this pull request
May 8, 2026
Per the 3-state exit-code contract ratified at PR #465 gate commit 60e1b68 and resurfaced by the StructMemEval calibration session in issue #473: adapters that find no data (data dir missing, dataset file absent, filters matching zero rows) now sys.exit(2) instead of print + plain `return`. The dispatcher (`benchmarks/run.py::run_invocation`) already classifies exit-code 2 as `_status: skipped_data_missing` per the 2026-05-06 contract; this commit closes the producer half so the canonical JSON distinguishes "data not installed" from "adapter crashed." Adapters touched (uniform pattern: try/except FileNotFoundError around the load, plus the existing post-load empty-list guard, both routing to sys.stderr + sys.exit(2)): - structmemeval_adapter.py — surfaced the bug via issue #473's /tmp/StructMemEval-empty trace. - locomo_adapter.py — load_locomo() reads a JSON file directly. - longmemeval_adapter.py — load_from_file() OR HuggingFace fallback. - mab_adapter.py — load_mab_split() can FileNotFoundError on absent HuggingFace cache. - mab_entity_index_adapter.py — same load_mab_split() path. - amabench_adapter.py — HuggingFace load_dataset() typically raises on absent data; if it returns zero rows after filtering, also exit 2. The dispatcher's existing fallback at run.py:167-172 (`adapter exited 0 but did not write $TMPDIR/T/<adapter>.json`) becomes a strict "adapter bug" signal rather than masking missing-data situations.
9 tasks
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Lands the v2.0 reproducibility harness for #437.
aelf bench all --canonical --out PATHnow subprocess-dispatches every academic-suite adapter (MAB, LoCoMo, LongMemEval, StructMemEval, AMA-Bench) at the canonical headline cut and merges the per-adapter outputs into one schema-v2 JSON. Tolerance-band classifier ships alongside; nightly cron is wired to a dedicatedbench-canonical-resultsbranch (same pattern #461 adopted for replay-soak — sidesteps themainruleset).The 8 design asks in
docs/v2_reproducibility_harness.mdwere ratified inline before implementation:aelf benchsubcommandschema_version: 2status: skipped_data_missing(exit 2)The override on #2 is the only deviation from the spec memo's recommendation. Implications captured in
docs/v2_reproducibility_harness.md§ Ratification — full LongMemEval (not oracle), StructMemEval--bench big(not small), multi-hour cron runtime instead of tens-of-minutes.What landed
benchmarks/run.py(340 LOC) — registry-driven dispatcher (11 canonical invocations, 2 smoke). Subprocess-per-adapter; injectable runner for tests.--canonicalcut-mismatch refusal.benchmarks/tolerance.py(217 LOC) — relative-with-floor band classifier. Readsmetric_overridesfrom canonical JSON; explicit caller-passed overrides take precedence. Pass/warn/fail classification with FAIL-dominates summary.src/aelfrice/cli.py—aelf bench allwired with--out,--canonical,--adapters,--smokeflags.allremoved from_BENCH_INERT_TARGETS..github/workflows/bench-canonical.yml— daily 05:00 UTC cron, hardened-runner, pinned actions, dedicated-branch push, in-pipeline band-check.tests/test_bench_dispatcher.py+tests/test_bench_tolerance.py— 29 stub-driven unit tests, all <1s wall-clock.tests/test_benchmarks_dir.py— adjusted[all]parametrization (no longer inert) + newtest_aelf_bench_all_requires_out.benchmarks/results/v2.0.0.json— schema-v2 skeleton with the canonical headline_cut,_calibration_pass_required: true. Real numbers land via the operator's calibration pass.README.md— reproducibility badge between OSSInsight and the lede (markered for cron rewrite). New## Reproducibilitysection before Roadmap.docs/COMMANDS.md— new row under Diagnostics.bench [--top-k N]row (synthetic) preserved.Test plan
uv run pytest tests/— 2624 passed, 41 skipped (no regressions vs main).python -c "from benchmarks import run, tolerance"imports clean.github/main: clean.aelf bench all --canonical --out benchmarks/results/v2.0.0.jsonagainst full data dirs (operator action; see Blockers).bench-canonical-resultsbranch (lands organically once cron fires).Blockers / follow-ups
aelf bench all --canonical --out benchmarks/results/v2.0.0.json≥3 times against full HuggingFace caches + populated/tmp/LoCoMo+/tmp/StructMemEval+ (optional)ANTHROPIC_API_KEYfor judge metrics. Take observed range × 1.5 as per-metric override band. Until this lands, the cron's band-check is a no-op (no leaves in canonical → summarize → PASS).tests/fixtures/bench_smoke/pinned LoCoMo turns + MAB QA so a 2-min PR smoke can exercise real adapter shapes. Not in this PR; license-attribution review needed. Pytest unit tests cover dispatcher regressions in the meantime via stubs.bench-canonical-results; the in-place README rewrite (betweenbench-canonical-badge:start/endmarkers) is not wired. Trivial sed in the workflow once you decide if this is wanted.skipped_data_missing) — dispatcher honors it; no adapter currently emits it. Land per-adapter as small follow-ups once/tmpdata-dir plumbing is decided.Provenance
benchmarks/results/v2.0.0.jsonis canonical,uv sync && aelf bench all#437docs/v2_reproducibility_harness.md(PR docs(v2_reproducibility_harness): spec memo for #437 #454, ratified in this PR's first commit)Summary by Sourcery
Introduce a v2.0 reproducibility harness for
aelf bench all, wiring a dispatcher, tolerance-band classifier, CLI flags, nightly cron workflow, and documentation to produce and validate canonical benchmark results.New Features:
aelf bench alldispatcher that runs all academic-suite benchmarks and merges results into a schema-v2 JSON report.--out,--canonical,--adapters, and--smokeflags on theaelf benchCLI for reproducibility runs.benchmarks/results/v2.0.0.json.Enhancements:
bench allharness in the commands reference.CI:
Bench Canonicalnightly GitHub Actions workflow that runs the canonical harness, band-checks against the canonical JSON, and pushes results to a dedicated branch.Tests:
aelf bench allbehaviour.Summary by CodeRabbit
New Features
aelf bench allcommand to run reproducible benchmark suites with--canonical,--smoke, and--adaptersfiltering optionsDocumentation
bench allcommand and tolerance-based validation policy