Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #935 +/- ##
==========================================
+ Coverage 86.54% 86.75% +0.21%
==========================================
Files 89 90 +1
Lines 26804 27110 +306
==========================================
+ Hits 23197 23519 +322
+ Misses 3607 3591 -16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a nightly benchmark workflow that discovers ChangesNightly benchmark workflow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@scripts/nightly_bench.py`:
- Around line 293-304: The qualification logic in nightly_bench.py is skipping
rows as “too-fast” when the standard or proof probe fails, which hides
timeout/error results from the report. Update the skip decision around the
std_probe/proof_probe flow and the later measured-data threshold so that rows
with probe errors or timeouts are still kept and reported, using the existing
std_qualifies, proof_qualifies, and skipped handling to distinguish real
failures from genuinely fast benchmarks.
- Around line 178-194: The timeout fallback in calibrate_timeout() is disabling
the per-run cap for hyperfine when `timeout` is missing or deemed too slow,
which allows a single hung repeat to block far too long. Update the timeout
handling so hyperfine() still enforces a strict per-run limit using the existing
probe()/subprocess backstop path, and only use TIMEOUT_PREFIX as an optional
wrapper rather than the sole enforcement mechanism. Make sure the logic around
calibrate_timeout(), probe(), and hyperfine() keeps the 2-minute per-run cap
active in all cases, including the no-`timeout` and slow-`timeout` branches.
- Line 524: The list comprehension in the configs construction uses the
ambiguous variable name l, which Ruff flags as E741. Update the comprehension in
the CONFIGS unpacking to use a descriptive name for the label field in the
configs mapping so the nightly_bench script stays lint-clean and the intent
remains clear.
- Around line 127-129: The requires_proofs() helper only matches direct children
of tests/proofs, so nested benchmark files are missed. Update requires_proofs()
in scripts/nightly_bench.py to detect any path under the tests/proofs tree by
checking the full ancestor path (for example via path.parts or path.parents)
instead of only path.parent.name, so files like tests/proofs/foo/bar.egg are
treated as proof-required.
🪄 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
Run ID: d8cd1420-b6bd-47d4-b37b-3a7e03fc56f0
📒 Files selected for processing (4)
.gitignoreCHANGELOG.mdMakefilescripts/nightly_bench.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: benchmark (ubuntu-latest, proof_testing_math)
🧰 Additional context used
📓 Path-based instructions (1)
CHANGELOG.md
📄 CodeRabbit inference engine (CLAUDE.md)
Update
CHANGELOG.mdwith a concise bullet when making major changes such as breaking changes or new features.
Files:
CHANGELOG.md
🪛 ast-grep (0.44.0)
scripts/nightly_bench.py
[error] 100-102: Avoid command injection
Context: subprocess.check_output(
["git", "-C", str(REPO_ROOT), *args], text=True, stderr=subprocess.DEVNULL
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-python)
[error] 154-159: Use of unsanitized data to create processes
Context: subprocess.run(
egglog_cmd(path, threads, proof),
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
timeout=RUN_TIMEOUT,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(os-system-unsanitized-data)
[error] 100-102: Command coming from incoming request
Context: subprocess.check_output(
["git", "-C", str(REPO_ROOT), *args], text=True, stderr=subprocess.DEVNULL
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 154-159: Command coming from incoming request
Context: subprocess.run(
egglog_cmd(path, threads, proof),
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
timeout=RUN_TIMEOUT,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 198-199: Command coming from incoming request
Context: subprocess.run(["timeout", str(RUN_TIMEOUT), "true"],
stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 217-228: Command coming from incoming request
Context: subprocess.run(
[
"hyperfine",
"--warmup", str(warmup),
"--runs", str(runs),
"--export-json", tmp_json,
shell_cmd,
],
capture_output=True,
text=True,
timeout=backstop,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 267-271: Command coming from incoming request
Context: subprocess.run(
["cargo", "build", "--release", "--bin", "egglog",
"--manifest-path", str(REPO_ROOT / "Cargo.toml")],
check=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[info] 527-527: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload, indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[warning] 231-231: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(tmp_json)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🪛 Ruff (0.15.18)
scripts/nightly_bench.py
[error] 101-101: subprocess call: check for execution of untrusted input
(S603)
[error] 102-102: Starting a process with a partial executable path
(S607)
[warning] 136-136: Boolean-typed positional argument in function definition
(FBT001)
[warning] 147-147: Boolean-typed positional argument in function definition
(FBT001)
[error] 155-155: subprocess call: check for execution of untrusted input
(S603)
[warning] 155-155: subprocess.run without explicit check argument
Add explicit check=False
(PLW1510)
[warning] 185-185: Using the global statement to update TIMEOUT_PREFIX is discouraged
(PLW0603)
[error] 199-199: subprocess call: check for execution of untrusted input
(S603)
[warning] 199-199: subprocess.run without explicit check argument
Add explicit check=False
(PLW1510)
[error] 199-199: Starting a process with a partial executable path
(S607)
[warning] 204-204: Boolean-typed positional argument in function definition
(FBT001)
[error] 218-218: subprocess call: check for execution of untrusted input
(S603)
[warning] 218-218: subprocess.run without explicit check argument
Add explicit check=False
(PLW1510)
[error] 219-225: Starting a process with a partial executable path
(S607)
[warning] 240-240: Boolean-typed positional argument in function definition
(FBT001)
[error] 268-268: subprocess call: check for execution of untrusted input
(S603)
[error] 269-270: Starting a process with a partial executable path
(S607)
[warning] 294-294: Boolean positional value in function call
(FBT003)
[warning] 295-295: Boolean positional value in function call
(FBT003)
[warning] 312-312: Boolean positional value in function call
(FBT003)
[warning] 313-314: Use elif instead of else then if, to reduce indentation
Convert to elif
(PLR5501)
[warning] 318-318: Boolean positional value in function call
(FBT003)
[warning] 330-330: Function definition does not bind loop variable cells
(B023)
[error] 524-524: Ambiguous variable name: l
(E741)
🔇 Additional comments (3)
Makefile (1)
1-1: LGTM!Also applies to: 10-15
CHANGELOG.md (1)
5-5: LGTM!.gitignore (1)
12-12: LGTM!Also applies to: 28-29
| def requires_proofs(path: Path) -> bool: | ||
| """Programs under tests/proofs/ only make sense with proofs enabled.""" | ||
| return path.parent.name == "proofs" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Detect all files under tests/proofs/, not only direct children.
Because discovery uses TEST_DIR.rglob("*.egg"), nested files like tests/proofs/foo/bar.egg would be treated as plain benchmarks and get non-proof cells instead of —.
Proposed fix
def requires_proofs(path: Path) -> bool:
"""Programs under tests/proofs/ only make sense with proofs enabled."""
- return path.parent.name == "proofs"
+ try:
+ path.relative_to(TEST_DIR / "proofs")
+ except ValueError:
+ return False
+ return True📝 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.
| def requires_proofs(path: Path) -> bool: | |
| """Programs under tests/proofs/ only make sense with proofs enabled.""" | |
| return path.parent.name == "proofs" | |
| def requires_proofs(path: Path) -> bool: | |
| """Programs under tests/proofs/ only make sense with proofs enabled.""" | |
| try: | |
| path.relative_to(TEST_DIR / "proofs") | |
| except ValueError: | |
| return False | |
| return True |
🤖 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 `@scripts/nightly_bench.py` around lines 127 - 129, The requires_proofs()
helper only matches direct children of tests/proofs, so nested benchmark files
are missed. Update requires_proofs() in scripts/nightly_bench.py to detect any
path under the tests/proofs tree by checking the full ancestor path (for example
via path.parts or path.parents) instead of only path.parent.name, so files like
tests/proofs/foo/bar.egg are treated as proof-required.
| def calibrate_timeout() -> None: | ||
| """Decide whether wrapping runs in `timeout` is cheap enough to use. | ||
|
|
||
| GNU coreutils `timeout` adds ~1ms; some reimplementations add ~100ms, which | ||
| would dominate fast benchmarks. We only use the wrapper when its overhead is | ||
| negligible; either way the per-run cap is also enforced by probe() and by a | ||
| subprocess-level backstop in hyperfine().""" | ||
| global TIMEOUT_PREFIX | ||
| if not shutil.which("timeout"): | ||
| log("Note: `timeout` not found; per-run cap enforced via probe only.") | ||
| return | ||
| best = min(_time_true() for _ in range(3)) | ||
| if best < 0.025: | ||
| TIMEOUT_PREFIX = ["timeout", str(RUN_TIMEOUT)] | ||
| else: | ||
| log(f"Note: `timeout` overhead is {best * 1000:.0f}ms; not wrapping runs " | ||
| "with it (per-run cap still enforced via probe and backstop).") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not disable the per-run timeout during hyperfine measurements.
When timeout is missing or considered too slow, the script falls back to only RUN_TIMEOUT * (warmup + runs) + 30; a single hung repeat can then stall one cell for many minutes instead of the promised 2-minute per-run cap.
Proposed fix
def calibrate_timeout() -> None:
@@
global TIMEOUT_PREFIX
- if not shutil.which("timeout"):
- log("Note: `timeout` not found; per-run cap enforced via probe only.")
- return
- best = min(_time_true() for _ in range(3))
- if best < 0.025:
- TIMEOUT_PREFIX = ["timeout", str(RUN_TIMEOUT)]
- else:
+ timeout_bin = shutil.which("timeout")
+ if not timeout_bin:
+ sys.exit("`timeout` not found; required to enforce the per-run cap.")
+ best = min(_time_true(timeout_bin) for _ in range(3))
+ TIMEOUT_PREFIX = [timeout_bin, str(RUN_TIMEOUT)]
+ if best >= 0.025:
log(f"Note: `timeout` overhead is {best * 1000:.0f}ms; not wrapping runs "
- "with it (per-run cap still enforced via probe and backstop).")
+ "would violate the per-run cap, so measurements may include wrapper overhead.")
-def _time_true() -> float:
+def _time_true(timeout_bin: str) -> float:
start = time.perf_counter()
- subprocess.run(["timeout", str(RUN_TIMEOUT), "true"],
+ subprocess.run([timeout_bin, str(RUN_TIMEOUT), "true"],
stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)Also applies to: 212-214
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 185-185: Using the global statement to update TIMEOUT_PREFIX is discouraged
(PLW0603)
🤖 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 `@scripts/nightly_bench.py` around lines 178 - 194, The timeout fallback in
calibrate_timeout() is disabling the per-run cap for hyperfine when `timeout` is
missing or deemed too slow, which allows a single hung repeat to block far too
long. Update the timeout handling so hyperfine() still enforces a strict per-run
limit using the existing probe()/subprocess backstop path, and only use
TIMEOUT_PREFIX as an optional wrapper rather than the sole enforcement
mechanism. Make sure the logic around calibrate_timeout(), probe(), and
hyperfine() keeps the 2-minute per-run cap active in all cases, including the
no-`timeout` and slow-`timeout` branches.
| # Qualification probes: the single-thread standard run and the proof run. | ||
| std_probe = None if req else probe(path, 1, False) | ||
| proof_probe = None if proof_excluded(path) else probe(path, 1, True) | ||
| proof_supported = proof_probe is not None and proof_probe[0] | ||
|
|
||
| std_qualifies = std_probe is not None and std_probe[0] and std_probe[1] >= MIN_BENCH_SECONDS | ||
| proof_qualifies = proof_supported and proof_probe[1] >= MIN_BENCH_SECONDS | ||
|
|
||
| if not (std_qualifies or proof_qualifies): | ||
| reason = "errored" if (req and not proof_supported) else "too-fast" | ||
| skipped.append({"name": name, "reason": reason}) | ||
| continue |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep timeout/error benchmarks in the report instead of skipping them as too-fast.
If the standard/proof qualification probe fails or times out, std_qualifies/proof_qualifies are false, and the later measured-data threshold can skip all-error rows as too-fast. That hides the exact failures the dashboard says it reports.
Proposed fix
std_qualifies = std_probe is not None and std_probe[0] and std_probe[1] >= MIN_BENCH_SECONDS
proof_qualifies = proof_supported and proof_probe[1] >= MIN_BENCH_SECONDS
+ std_failed = std_probe is not None and not std_probe[0]
+ proof_required_failed = req and proof_probe is not None and not proof_probe[0]
- if not (std_qualifies or proof_qualifies):
+ if not (std_qualifies or proof_qualifies or std_failed or proof_required_failed):
reason = "errored" if (req and not proof_supported) else "too-fast"
skipped.append({"name": name, "reason": reason})
continue
@@
measured = [c["mean"] for c in cells.values() if "mean" in c]
- if not measured or max(measured) < MIN_BENCH_SECONDS:
+ has_failure = any(c.get("status") in {"timeout", "error"} for c in cells.values())
+ if not has_failure and (not measured or max(measured) < MIN_BENCH_SECONDS):
skipped.append({"name": name, "reason": "too-fast"})
continueAlso applies to: 320-325
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 294-294: Boolean positional value in function call
(FBT003)
[warning] 295-295: Boolean positional value in function call
(FBT003)
🤖 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 `@scripts/nightly_bench.py` around lines 293 - 304, The qualification logic in
nightly_bench.py is skipping rows as “too-fast” when the standard or proof probe
fails, which hides timeout/error results from the report. Update the skip
decision around the std_probe/proof_probe flow and the later measured-data
threshold so that rows with probe errors or timeouts are still kept and
reported, using the existing std_qualifies, proof_qualifies, and skipped
handling to distinguish real failures from genuinely fast benchmarks.
| **meta, | ||
| "min_bench_seconds": MIN_BENCH_SECONDS, | ||
| "run_timeout_seconds": RUN_TIMEOUT, | ||
| "configs": [{"key": k, "label": l, "threads": t, "proof": p} for k, l, t, p in CONFIGS], |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rename the ambiguous comprehension variable.
Ruff flags l as E741 here; using descriptive names keeps this new script lint-clean.
Proposed fix
- "configs": [{"key": k, "label": l, "threads": t, "proof": p} for k, l, t, p in CONFIGS],
+ "configs": [
+ {"key": key, "label": label, "threads": threads, "proof": proof}
+ for key, label, threads, proof in CONFIGS
+ ],📝 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.
| "configs": [{"key": k, "label": l, "threads": t, "proof": p} for k, l, t, p in CONFIGS], | |
| "configs": [ | |
| {"key": key, "label": label, "threads": threads, "proof": proof} | |
| for key, label, threads, proof in CONFIGS | |
| ], |
🧰 Tools
🪛 Ruff (0.15.18)
[error] 524-524: Ambiguous variable name: l
(E741)
🤖 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 `@scripts/nightly_bench.py` at line 524, The list comprehension in the configs
construction uses the ambiguous variable name l, which Ruff flags as E741.
Update the comprehension in the CONFIGS unpacking to use a descriptive name for
the label field in the configs mapping so the nightly_bench script stays
lint-clean and the intent remains clear.
Source: Linters/SAST tools
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
@yihozhang and I discussed, I'm going to port this to use eval-live |
Replace the hand-rolled HTML/CSS/JS table in nightly_bench.py with eval-live (https://github.com/oflatt/eval-live), which provides in-browser filtering over the benchmark results. render_html() now flattens the per-benchmark cells into eval-live row objects (one row per benchmark, one column per configuration) and embeds eval_live.css()/js() plus initEvalLive() into a self-contained page. Skipped programs are surfaced as a second table. Measured cells stay numeric so the column filters offer numeric dropdowns; not-applicable cells render as an em-dash and timeout/error cells keep their status string. - Add scripts/requirements.txt pinning eval-live (git dependency). - `make nightly` now installs scripts/requirements.txt first, and nightly_bench.py fails fast if eval-live is missing before the sweep. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Render nightly benchmark report with eval-live
`make nightly` ran `pip install` directly, which fails on PEP 668 externally-managed systems (Debian/Python 3.12 on the nightly server) with "This environment is externally managed". Install eval-live into a local venv instead, and run nightly_bench.py with that venv's python so its `import eval_live` resolves. The venv is gitignored. Co-authored-by: oliver <oflatt@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
After the venv fix, the build failed with cargo 1.75.0 rejecting the
crate's 2024 edition ("feature `edition2024` is not stabilized"). The
nightly host's PATH puts a system cargo ahead of the rustup shim, so
rust-toolchain.toml (pinned to 1.91.0) is ignored. Prepend ~/.cargo/bin
to PATH at startup so the rustup shim — which reads rust-toolchain.toml
and installs the pinned toolchain on demand — wins; hyperfine lives there
too. Mirrors eggcc's nightly. Verified the shim resolves to 1.91.0 in-repo.
Co-authored-by: oliver <oflatt@gmail.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5 participants
A screenshot of the nightly below
I was not too careful about the nightly design, but just to get something in there for now, and we can further iterate.