perf: add a reproducible benchmark pipeline#29
Open
t41372 wants to merge 19 commits into
Open
Conversation
…rovenance, trust-split completion - js/ts analyzer sources: emit whole function chunks (truncation cut closing braces; error-recovery parse trees under-reported analyze times ~5x) + validity tests (brace balance, python compile, tree-sitter parses without errors) - benchmark-compare: results carry each side's git identity (--measured-repo), micro degrades per-script under BENCH_COMPARE for harness imports and cold probes too, compatibility floor documented (sides >= 725f11d) - trust split completed: envspec.py (constructed-env contract) + hyperfine metric-ID minting + datasets.check_reusable (incl. writing-skit version stamp) moved under the coverage floor; suites/ keeps only discovery+spawn - budgets.toml regenerated canonically via render_budgets (byte round-trip test); closure/syscalls target rows scoped profile=full; tui.rss.n1000.peak_kib naming aligned with the rss suite's grammar - stale run.json/results.* unlinked at run start (a crashed run must not leave old provenance for summarize to merge with fresh partial data) - tui: peak-RSS skip is a single pre-spawn decision, parse failures crash; search span asserts rows both drop AND survive (never the filter-to-zero degenerate path), probe char single-sourced as datasets.SEARCH_PROBE_CHAR - sync tests: hyperfine pin <-> workflows, BENCH_CI_RUNNER <-> runs-on, budgets.toml canonical bytes; compare excludes pipeline.* self-timings; bounds render as plain numbers; docs updated to implementation truth
…import surface - design doc tree/section now name envspec.py as the environment contract's home (the round-1 move had updated only half the doc) - dropped the unimplemented pyperf-compare_to promise; metric-ID grammar named in code (pipeline.HEADLINE_METRICS) instead of a hand-copied catalog; compare inputs phrasing corrected (checkout ref:, never run: interpolation) - probe-char fossil test now uses SEARCH_PROBE_CHAR; the n=3 boundary of the matching-entry guarantee is pinned by parametrized test - check_reusable docstring honest about same-version branch switches + README note to delete .bench/datasets when switching such branches - import-surface test: benchmarks/ modules may import only runtime deps + pyperf (micro/) + textual (tui probe) + stdlib — a dev-only import would kill every A/B run at startup
Correctness (report top-15): - compare: per-unit noise floors (us floor was 2000µs — swallowed every realistic micro regression); sides' profile/platform/python mismatch now renders a loud not-directly-comparable warning instead of a clean-looking A/B - derive(): data-driven DERIVATIONS with strict both-or-neither pairs — a renamed producer metric fails loud instead of silently dropping a headline number - compare mode rides on SuitePlan.compare_mode (build_plan), not ambient BENCH_COMPARE env — local degrades identically to CI - budgets: max must be finite (inf/nan crashed check/--propose); profiles is now a list predicate and footprint.wheel_bytes is scoped [pr, full] (compare-profile results no longer render a spurious enforced FAIL) - results: NaN/Infinity rejected everywhere (json.loads parses bare literals; a NaN metric silently passed every bound) - merge: duplicate suite outputs rejected (raw/duration were silently overwritten); execute() asserts output.suite == plan.suite - startup/imports declare ns=(0,) — their empty-library need is plan-visible, not satisfied incidentally by sibling suites - __main__ boundary catches OSError (missing file printed a raw traceback); summarize_dir wraps run.json decode errors in PipelineError - pyperf/hyperfine parsers raise ParseError (not AttributeError/ValueError) on malformed elements; census probe re-raises str SystemExit codes as-is Structure/cleanup (below the report cap, verified CONFIRMED): - envspec.build_env: the unverified empty-library special case is gone — every dataset root passes the manifest guard, micro render/cold use the real n=0 set - probe char recorded in the manifest and enforced by check_reusable; the tui suite filters with the dataset's own recorded char - parsers.median/stddev delegate to statistics (p95 stays nearest-rank by design); envinfo uses skit.models.now_iso; dist_version's test-only probe seam dropped - hyperfine install: one composite action (.github/actions/install-hyperfine) replaces three drifting workflow copies; sync tests pin version+sha+URL to benchmarks/hyperfine.py and require every workflow to use the action - SuiteOutput.skip_all fills both suite fields from one argument (7 sites); generate/generate_runover share a _finalize manifest tail; footprint walks site-packages once (skit subtotal in the same pass) and tie-breaks the top-10 by name; tests use scoped_skit_dirs instead of five hand-rolled env blocks Skipped with rationale: default-groups release coupling (deliberate, documented ty-resolution choice), Manifest.kinds retention (diagnostic; now labeled), datasets --seed/--state-fraction flags (check_reusable already refuses non-default reuse), blanket subprocess timeouts (job-level timeout-minutes is the designed hang bound)
📝 WalkthroughWalkthroughThe PR adds a complete benchmark evaluation pipeline with deterministic datasets, measurement suites, typed results, performance budgets, A/B comparison reports, CI workflows, documentation, and hermetic contract tests. ChangesBenchmark pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
t41372
marked this pull request as ready for review
July 24, 2026 07:17
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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 `@benchmarks/__main__.py`:
- Around line 135-144: Update the exception tuple in the command-line error
handler of benchmarks/__main__.py to include OSError alongside the existing
error types. Preserve the current stderr message format and exit status so
missing or unreadable user-supplied paths produce the same clean benchmarks: …
failure response.
In `@benchmarks/hyperfine.py`:
- Around line 19-24: Update HYPERFINE_SHA256 and the corresponding
install-hyperfine checksum reference to
78c3c13b2909f291319760777591e138a8e32c0d8d575c327429188d67228a2a, keeping the
v1.20.0 asset URL unchanged.
In `@benchmarks/micro/bench_analyzers.py`:
- Around line 21-22: Replace the duplicated _EXT mapping and its language
iteration in the benchmark script with the shared sources.LANGS and
sources.EXTENSIONS registries used by the micro suite. Update filename
generation to derive each language’s expected extension from those registries,
and add a contract test that verifies the generated benchmark filenames match
the shared source definitions.
In `@benchmarks/suites/_env.py`:
- Around line 91-142: Add bounded timeouts to both subprocess.run calls in
run_hyperfine and diagnose_cases, and apply the same timeout policy to
subprocess.run calls in the sibling benchmark suite files. Ensure timeout
failures propagate as clear diagnostics rather than allowing a hung benchmark or
hyperfine process to block indefinitely.
In `@benchmarks/suites/footprint.py`:
- Around line 73-83: Add a short delay between failed uv pip install attempts in
the retry loop, applying it only when another attempt remains and preserving the
existing final RuntimeError behavior. Update the retry logic around the
subprocess.run call and _INSTALL_ATTEMPTS so retries use a brief backoff rather
than running immediately.
- Around line 67-79: Update both subprocess.run calls in _closure— the uv venv
creation and uv pip install steps—to pass cwd=ctx.workdir, ensuring both
commands use the external workdir’s configuration rather than the ambient
process directory.
In `@docs/design/benchmarks.md`:
- Line 56: Label the fenced tree block in the benchmarks documentation with the
text language identifier by changing the opening fence to specify text, while
preserving the existing directory listing content.
In `@pyproject.toml`:
- Around line 117-122: Update pyproject.toml at lines 117-122 to remove
benchmarks/__main__.py from the coverage omit list, and at lines 243-249 extend
mutmut’s source_paths to include benchmarks/ alongside src/skit/; no other
benchmark omit entries require changes.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 992e3cbf-b948-4759-8128-1a10bc57f1fb
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
.github/actions/install-hyperfine/action.yml.github/workflows/benchmark-compare.yml.github/workflows/benchmark-nightly.yml.github/workflows/benchmark.yml.gitignoreAGENTS.mdbenchmarks/README.mdbenchmarks/__init__.pybenchmarks/__main__.pybenchmarks/budgets.pybenchmarks/budgets.tomlbenchmarks/compare.pybenchmarks/datasets.pybenchmarks/envinfo.pybenchmarks/envspec.pybenchmarks/fixtures/__init__.pybenchmarks/fixtures/noop.jsbenchmarks/fixtures/noop.pybenchmarks/fixtures/noop.shbenchmarks/fixtures/sources.pybenchmarks/hyperfine.pybenchmarks/micro/bench_analyzers.pybenchmarks/micro/bench_launch.pybenchmarks/micro/bench_render.pybenchmarks/micro/bench_store.pybenchmarks/parsers.pybenchmarks/pipeline.pybenchmarks/results.pybenchmarks/suites/__init__.pybenchmarks/suites/_env.pybenchmarks/suites/_run.pybenchmarks/suites/footprint.pybenchmarks/suites/imports.pybenchmarks/suites/micro.pybenchmarks/suites/rss.pybenchmarks/suites/run_overhead.pybenchmarks/suites/scale.pybenchmarks/suites/startup.pybenchmarks/suites/syscalls.pybenchmarks/suites/tui.pybenchmarks/suites/tui_probe.pydocs/design/benchmarks.mdpyproject.tomltests/test_benchmarks_tooling.py
Comment on lines
+19
to
+24
| HYPERFINE_VERSION = "1.20.0" | ||
| HYPERFINE_SHA256 = "63ad53934062118f5b0be11785e0bb1603d4b91667d1921f2fd8df9a8712040a" | ||
| HYPERFINE_URL = ( | ||
| "https://github.com/sharkdp/hyperfine/releases/download/" | ||
| f"v{HYPERFINE_VERSION}/hyperfine-v{HYPERFINE_VERSION}-x86_64-unknown-linux-gnu.tar.gz" | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
curl -sL https://github.com/sharkdp/hyperfine/releases/download/v1.20.0/hyperfine-v1.20.0-x86_64-unknown-linux-gnu.tar.gz -o hf.tar.gz
sha256sum hf.tar.gzRepository: t41372/skit
Length of output: 196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "file:"
python3 - <<'PY'
from urllib.request import urlopen
url = "https://github.com/sharkdp/hyperfine/releases/download/v1.20.0/hyperfine-v1.20.0-x86_64-unknown-linux-gnu.tar.gz"
print(url)
with urlopen(url, timeout=30) as r:
data = r.read()
print("http_status", r.status, "content_length", len(data), "content_type", r.headers.get("content-type"))
PY
echo
grep -nE 'HYPERFINE_(VERSION|SHA256|URL)' benchmarks/hyperfine.py || trueRepository: t41372/skit
Length of output: 2862
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "grep HYPERFINE constants from benchmarks/hyperfine.py:"
grep -nE 'HYPERFINE_(VERSION|SHA256|URL)' benchmarks/hyperfine.py || true
echo
echo "SHA256 constants in repository:"
grep -RInE 'HYPERFINE_SHA256|63ad53934062118f5b0be11785e0bb1603d4b91667d1921f2fd8df9a8712040a' . || trueRepository: t41372/skit
Length of output: 842
🌐 Web query:
https://github.com/sharkdp/hyperfine/releases/download/v1.20.0/hyperfine-v1.20.0-x86_64-unknown-linux-gnu.tar.gz sha256
💡 Result:
The SHA256 checksum for the file hyperfine-v1.20.0-x86_64-unknown-linux-gnu.tar.gz is 642.5 KB (the download itself) with a corresponding sha256 value of 78c3c13b2909f291319760777591e138a8e32c0d8d575c327429188d67228a2a. Correction: Upon reviewing the official release data [1][2], the SHA256 checksum for the requested file hyperfine-v1.20.0-x86_64-unknown-linux-gnu.tar.gz is 78c3c13b2909f291319760777591e138a8e32c0d8d575c327429188d67228a2a.
Citations:
- 1: https://github.com/sharkdp/hyperfine/releases
- 2: https://github.com/sharkdp/hyperfine/releases/tag/v1.20.0
Correct the hyperfine asset checksum to match v1.20.0.
HYPERFINE_SHA256/install-hyperfine currently use 63ad53934062118f5b0be11785e0bb1603d4b91667d1921f2fd8df9a8712040a, but the published GitHub release asset hyperfine-v1.20.0-x86_64-unknown-linux-gnu.tar.gz has checksum 78c3c13b2909f291319760777591e138a8e32c0d8d575c327429188d67228a2a. Update both references so benchmark installs don’t fail at the install-time integrity gate.
🤖 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/hyperfine.py` around lines 19 - 24, Update HYPERFINE_SHA256 and
the corresponding install-hyperfine checksum reference to
78c3c13b2909f291319760777591e138a8e32c0d8d575c327429188d67228a2a, keeping the
v1.20.0 asset URL unchanged.
Comment on lines
+21
to
+22
| _LINES = (20, 200, 2000) | ||
| _EXT = {"python": "py", "shell": "sh", "js": "js", "ts": "ts"} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the shared source registry instead of duplicating _EXT.
benchmarks/suites/micro.py:95-103 materializes files from sources.LANGS and sources.EXTENSIONS, while this script maintains a second mapping. If either registry changes, the orchestrator can mark a language available and launch this script, but this loop may omit it or read the wrong filename, producing incomplete benchmark results without a corresponding skip.
Iterate over the shared language list and extension mapping, and add a contract test covering the generated filenames.
Proposed fix
+from benchmarks.fixtures import sources
+
_LINES = (20, 200, 2000)
-_EXT = {"python": "py", "shell": "sh", "js": "js", "ts": "ts"}
def main() -> None:
sources_dir = Path(os.environ["BENCH_SOURCES_DIR"])
runner = pyperf.Runner()
- for lang, ext in _EXT.items():
+ for lang in sources.LANGS:
+ ext = sources.EXTENSIONS[lang]
spec = spec_for(lang)Also applies to: 28-34
🤖 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/micro/bench_analyzers.py` around lines 21 - 22, Replace the
duplicated _EXT mapping and its language iteration in the benchmark script with
the shared sources.LANGS and sources.EXTENSIONS registries used by the micro
suite. Update filename generation to derive each language’s expected extension
from those registries, and add a contract test that verifies the generated
benchmark filenames match the shared source definitions.
Comment on lines
+91
to
+142
| def run_hyperfine( | ||
| ctx: RunCtx, | ||
| cases: list[Case], | ||
| *, | ||
| warmup: int, | ||
| min_runs: int, | ||
| env: Mapping[str, str], | ||
| export_name: str, | ||
| ) -> tuple[dict[str, Metric], dict[str, object]]: | ||
| """Spawn hyperfine on `cases`, parse its export, return per-case Metrics keyed | ||
| `<case>.median_ms` plus the raw times for the artifact.""" | ||
| if ctx.hyperfine is None: | ||
| raise RuntimeError("hyperfine not found") # callers skip-record before this | ||
| export = ctx.workdir / f"{export_name}.json" | ||
| argv = build_argv( | ||
| cases, | ||
| warmup=warmup, | ||
| min_runs=min_runs, | ||
| export_json=str(export), | ||
| hyperfine_bin=ctx.hyperfine, | ||
| ) | ||
| proc = subprocess.run( # noqa: S603 — fixed-shape argv built by hyperfine.build_argv | ||
| argv, | ||
| cwd=ctx.workdir, | ||
| env=dict(env), | ||
| check=False, | ||
| ) | ||
| if proc.returncode != 0: | ||
| # hyperfine swallows the failing command's stderr; an opaque exit code helps | ||
| # nobody. Re-run each case once, captured, and report what actually broke. | ||
| raise RuntimeError( | ||
| f"hyperfine batch {export_name!r} failed ({proc.returncode}); " | ||
| f"single-shot diagnosis: {diagnose_cases(ctx, cases, env)}" | ||
| ) | ||
| times_by_case = parse_export(export.read_text(encoding="utf-8")) | ||
| return metrics_from_export(times_by_case), {"times_s": times_by_case} | ||
|
|
||
|
|
||
| def diagnose_cases(ctx: RunCtx, cases: list[Case], env: Mapping[str, str]) -> str: | ||
| reports = [] | ||
| for case in cases: | ||
| probe = subprocess.run( # noqa: S603 — re-running the exact benchmarked argv | ||
| list(case.argv), | ||
| cwd=ctx.workdir, | ||
| env=dict(env), | ||
| capture_output=True, | ||
| text=True, | ||
| check=False, | ||
| ) | ||
| detail = "" if probe.returncode == 0 else f" stderr: {probe.stderr.strip()[-500:]!r}" | ||
| reports.append(f"[{case.name} rc={probe.returncode}{detail}]") | ||
| return " ".join(reports) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Consider adding a timeout to spawned subprocesses.
None of the subprocess.run calls here (or in the sibling suite files: footprint.py, imports.py, micro.py, rss.py, syscalls.py) pass timeout=. A hung benchmarked binary or a stuck hyperfine process would block the whole benchmark run indefinitely instead of failing fast with the exact diagnostic these functions are otherwise designed to produce.
♻️ Example fix
proc = subprocess.run( # noqa: S603 — fixed-shape argv built by hyperfine.build_argv
argv,
cwd=ctx.workdir,
env=dict(env),
+ timeout=600,
check=False,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def run_hyperfine( | |
| ctx: RunCtx, | |
| cases: list[Case], | |
| *, | |
| warmup: int, | |
| min_runs: int, | |
| env: Mapping[str, str], | |
| export_name: str, | |
| ) -> tuple[dict[str, Metric], dict[str, object]]: | |
| """Spawn hyperfine on `cases`, parse its export, return per-case Metrics keyed | |
| `<case>.median_ms` plus the raw times for the artifact.""" | |
| if ctx.hyperfine is None: | |
| raise RuntimeError("hyperfine not found") # callers skip-record before this | |
| export = ctx.workdir / f"{export_name}.json" | |
| argv = build_argv( | |
| cases, | |
| warmup=warmup, | |
| min_runs=min_runs, | |
| export_json=str(export), | |
| hyperfine_bin=ctx.hyperfine, | |
| ) | |
| proc = subprocess.run( # noqa: S603 — fixed-shape argv built by hyperfine.build_argv | |
| argv, | |
| cwd=ctx.workdir, | |
| env=dict(env), | |
| check=False, | |
| ) | |
| if proc.returncode != 0: | |
| # hyperfine swallows the failing command's stderr; an opaque exit code helps | |
| # nobody. Re-run each case once, captured, and report what actually broke. | |
| raise RuntimeError( | |
| f"hyperfine batch {export_name!r} failed ({proc.returncode}); " | |
| f"single-shot diagnosis: {diagnose_cases(ctx, cases, env)}" | |
| ) | |
| times_by_case = parse_export(export.read_text(encoding="utf-8")) | |
| return metrics_from_export(times_by_case), {"times_s": times_by_case} | |
| def diagnose_cases(ctx: RunCtx, cases: list[Case], env: Mapping[str, str]) -> str: | |
| reports = [] | |
| for case in cases: | |
| probe = subprocess.run( # noqa: S603 — re-running the exact benchmarked argv | |
| list(case.argv), | |
| cwd=ctx.workdir, | |
| env=dict(env), | |
| capture_output=True, | |
| text=True, | |
| check=False, | |
| ) | |
| detail = "" if probe.returncode == 0 else f" stderr: {probe.stderr.strip()[-500:]!r}" | |
| reports.append(f"[{case.name} rc={probe.returncode}{detail}]") | |
| return " ".join(reports) | |
| def run_hyperfine( | |
| ctx: RunCtx, | |
| cases: list[Case], | |
| *, | |
| warmup: int, | |
| min_runs: int, | |
| env: Mapping[str, str], | |
| export_name: str, | |
| ) -> tuple[dict[str, Metric], dict[str, object]]: | |
| """Spawn hyperfine on `cases`, parse its export, return per-case Metrics keyed | |
| `<case>.median_ms` plus the raw times for the artifact.""" | |
| if ctx.hyperfine is None: | |
| raise RuntimeError("hyperfine not found") # callers skip-record before this | |
| export = ctx.workdir / f"{export_name}.json" | |
| argv = build_argv( | |
| cases, | |
| warmup=warmup, | |
| min_runs=min_runs, | |
| export_json=str(export), | |
| hyperfine_bin=ctx.hyperfine, | |
| ) | |
| proc = subprocess.run( # noqa: S603 — fixed-shape argv built by hyperfine.build_argv | |
| argv, | |
| cwd=ctx.workdir, | |
| env=dict(env), | |
| timeout=600, | |
| check=False, | |
| ) | |
| if proc.returncode != 0: | |
| # hyperfine swallows the failing command's stderr; an opaque exit code helps | |
| # nobody. Re-run each case once, captured, and report what actually broke. | |
| raise RuntimeError( | |
| f"hyperfine batch {export_name!r} failed ({proc.returncode}); " | |
| f"single-shot diagnosis: {diagnose_cases(ctx, cases, env)}" | |
| ) | |
| times_by_case = parse_export(export.read_text(encoding="utf-8")) | |
| return metrics_from_export(times_by_case), {"times_s": times_by_case} | |
| def diagnose_cases(ctx: RunCtx, cases: list[Case], env: Mapping[str, str]) -> str: | |
| reports = [] | |
| for case in cases: | |
| probe = subprocess.run( # noqa: S603 — re-running the exact benchmarked argv | |
| list(case.argv), | |
| cwd=ctx.workdir, | |
| env=dict(env), | |
| capture_output=True, | |
| text=True, | |
| check=False, | |
| ) | |
| detail = "" if probe.returncode == 0 else f" stderr: {probe.stderr.strip()[-500:]!r}" | |
| reports.append(f"[{case.name} rc={probe.returncode}{detail}]") | |
| return " ".join(reports) |
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 111-116: Command coming from incoming request
Context: subprocess.run( # noqa: S603 — fixed-shape argv built by hyperfine.build_argv
argv,
cwd=ctx.workdir,
env=dict(env),
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 131-138: Command coming from incoming request
Context: subprocess.run( # noqa: S603 — re-running the exact benchmarked argv
list(case.argv),
cwd=ctx.workdir,
env=dict(env),
capture_output=True,
text=True,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🤖 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/suites/_env.py` around lines 91 - 142, Add bounded timeouts to
both subprocess.run calls in run_hyperfine and diagnose_cases, and apply the
same timeout policy to subprocess.run calls in the sibling benchmark suite
files. Ensure timeout failures propagate as clear diagnostics rather than
allowing a hung benchmark or hyperfine process to block indefinitely.
Comment on lines
+117
to
+122
| omit = [ | ||
| "benchmarks/suites/*", # subprocess orchestration around hyperfine/strace/uv/probes | ||
| "benchmarks/micro/*", # pyperf scripts re-exec'd as worker processes | ||
| "benchmarks/__main__.py", # thin argparse shim; all logic lives in covered modules | ||
| "benchmarks/fixtures/noop.py", # benchmark subject (a script the benchmarks run), not harness code | ||
| ] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A20 -B2 '^\[tool\.mutmut\]' pyproject.toml
fd -e py . benchmarks | sortRepository: t41372/skit
Length of output: 2479
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pyproject relevant sections =="
sed -n '100,130p;226,252p' pyproject.toml
echo "== benchmarks/__main__.py =="
sed -n '1,220p' benchmarks/__main__.py
echo "== benchmark tests/imports mentioning tests/test_benchmarks_tooling.py and benchmarks package =="
fd -e py . tests | rg '(^|/)test_benchmarks_tooling\.py$|benchmark|micro|suite' | sort
sed -n '1,220p' tests/test_benchmarks_tooling.pyRepository: t41372/skit
Length of output: 16346
Keep benchmark Python code under the mandatory quality gates.
benchmarks/__main__.py wires dispatch/exit-code logic and is omitted from coverage, while mutmut currently mutates only src/skit/ even though benchmarks/ is copied into workers.
pyproject.toml#L117-L122: removebenchmarks/__main__.pyfrom coverageomitso its dispatch paths participate in the 100% coverage floor.pyproject.toml#L228: extendsource_pathsto includebenchmarks/alongsidesrc/skit/;also_copyalone does not add mutation coverage for Python files.
📍 Affects 1 file
pyproject.toml#L117-L122(this comment)pyproject.toml#L243-L249
🤖 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 `@pyproject.toml` around lines 117 - 122, Update pyproject.toml at lines
117-122 to remove benchmarks/__main__.py from the coverage omit list, and at
lines 243-249 extend mutmut’s source_paths to include benchmarks/ alongside
src/skit/; no other benchmark omit entries require changes.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@benchmarks/envinfo.py`:
- Around line 179-183: Update collect_meta() so invoking uv does not raise
FileNotFoundError when the executable is unavailable: catch the missing-command
condition and record the uv version as "unknown". Preserve the existing version
capture when uv is installed, allowing execute() to produce run metadata and
results while suites continue skipping via ctx.uv being None.
In `@tests/test_benchmarks_tooling.py`:
- Around line 1143-1184: Extend test_run_and_summarize_commands to assert that
fake_execute receives the expected _REPO_ROOT value and the configured budgets
value, alongside the existing profile, out, and measured_root assertions. Use
the actual symbols passed by benchmark_cli.main rather than duplicating or
hardcoding equivalent values.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 312d0ef6-9812-4a32-95d7-779174c22573
📒 Files selected for processing (14)
benchmarks/README.mdbenchmarks/__main__.pybenchmarks/envinfo.pybenchmarks/micro/bench_analyzers.pybenchmarks/suites/_env.pybenchmarks/suites/footprint.pybenchmarks/suites/imports.pybenchmarks/suites/micro.pybenchmarks/suites/rss.pybenchmarks/suites/syscalls.pybenchmarks/suites/tui.pydocs/design/benchmarks.mdpyproject.tomltests/test_benchmarks_tooling.py
💤 Files with no reviewable changes (1)
- pyproject.toml
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
benchmarks/envspec.py (1)
69-74: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate that
manifest.jsonis a regular file.
.exists()also accepts a directory namedmanifest.json, allowing an invalid dataset root past the fail-fast check.Proposed fix
- if not (dataset_root / "manifest.json").exists(): + if not (dataset_root / "manifest.json").is_file():🤖 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/envspec.py` around lines 69 - 74, Update the manifest validation in the dataset-root setup to require that dataset_root / "manifest.json" is a regular file, not merely an existing filesystem entry. Preserve the existing RuntimeError and fail-fast behavior, and continue calling skit_dirs(dataset_root) only after this validation passes.
🤖 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.
Outside diff comments:
In `@benchmarks/envspec.py`:
- Around line 69-74: Update the manifest validation in the dataset-root setup to
require that dataset_root / "manifest.json" is a regular file, not merely an
existing filesystem entry. Preserve the existing RuntimeError and fail-fast
behavior, and continue calling skit_dirs(dataset_root) only after this
validation passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 833cfaec-7eab-4606-aea3-17742ea8d3d2
📒 Files selected for processing (2)
benchmarks/envspec.pytests/test_benchmarks_tooling.py
The pipeline design's implementation order ends by tightening the two import ratchets from the PR's own CI bench artifact — they shipped as deliberately generous bootstrap bounds (400) only to keep the interim runs green. That artifact measures 291 modules on the pinned python, so `check --propose` moves both to 321 and stamps each row's provenance from the run that produced it. Two README lines described only the pre-tightening state: `--propose` stamps `context` (so it is not hand-written), and the merge checklist's "tighten the bounds" step now happens pre-merge.
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
1 participant
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
Why
The project needs deterministic performance evidence that can distinguish product regressions from incompatible units, harness drift, and GitHub-hosted runner image changes.
This PR measures only — nothing under
src/skit/is touched, by design: the pipeline has to record the "before" that later optimization PRs will be judged against.Baseline
Generated by this PR's own
benchmarkCI job (artifactbenchmark-results-pr, run 30145102877) — not a local run.skit 0.4.1.dev0 · profile pr · 2026-07-25 · Linux 6.17.0-1020-azure · Intel Xeon Platinum 8573C ×4 · 15988 MiB · python 3.13.14 · uv 0.11.32 · textual 8.2.8 · pyperf 2.10.0 · runner
ubuntu-24.04image20260720.247.2startup.version.median_msstartup.version.over_python_msstartup.list_json.median_msscale.list_json.n100.median_msscale.list_json.n1000.median_msscale.list_json.per_entry_usrun_overhead.python.overhead_msrun_overhead.shell.overhead_mstui.first_idle.n100.median_mstui.first_idle.n1000.median_mstui.search.n1000.median_msrss.version.peak_kibrss.list_json.n1000.peak_kibimports.version.modulesimports.list_json.modulesfootprint.wheel_bytesmicro.store.list_entries.n1000.median_uspipeline.duration_s(harness, not a product metric)No skipped cases. 93 metrics total (headline set above); raw samples ship in
results.json.Budgets — enforced: 4 evaluated, 4 passed. Five
targetrows are violated, which is the expected state on day one: they are the optimization backlog, not a regression.startup.version.over_python_msscale.list_json.per_entry_usrss.list_json.n1000.peak_kibimports.version.has_typerimports.version.has_richThe two import-census ratchets ship tightened from this artifact (
check --propose):400→321(291 measured + 10% headroom), withcontextstamped from the run that produced it. No README performance claims and no "lightweight" wording land here — those are a later PR, generated from aresults.json, after the budgets are ratified.Validation
uv run ruff format --checkuv run ruff checkuv run ty checkuv run zizmor .github/workflows— no findingsuv run pytest --cov -q— 5165 passed, 31 skipped, 100% coverageMerge
Use Squash and merge after required checks pass. Post-merge one-time setup (create
gh-pages, enable Pages, dispatch the nightly once, pick alert thresholds after ~14 points) is the checklist inbenchmarks/README.md.Summary by CodeRabbit
.gitignoreto exclude benchmark artifacts.