Add per-eval timeout to evals + small bug in eval results table - #910
Conversation
WalkthroughAdds optional per-test setup timeout support by introducing setup_timeout to HolmesTestCase, propagating it through run_commands to _invoke_command, and applying it to subprocess.run. Updates timeout messaging. Adjusts terminal reporter to compute average cost per test run (rows) instead of per unique test. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as run_commands
participant Invoker as _invoke_command
participant SubProc as subprocess.run
Note over Runner: Determine per-test timeout<br/>(test_case.setup_timeout or env default)
Runner->>Invoker: _invoke_command(cmd, cwd, timeout)
Invoker->>SubProc: run(cmd, cwd, timeout=timeout)
SubProc-->>Invoker: Completed process / Timeout
alt Success
Invoker-->>Runner: stdout
else Timeout/Error
Invoker-->>Runner: raise / TIMEOUT_ERROR (mentions env and setup_timeout)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/llm/utils/commands.py (2)
113-120: Prefer explicit None-check; avoid truthiness filter for setup_timeoutUsing a truthiness check will ignore a legitimate override if someone (incorrectly) sets 0, and could pass through negative integers if upstream validation is missing. After enforcing
gt=0in HolmesTestCase, use an explicit None-check here for clarity.Apply this diff:
- # Use per-test timeout if specified, otherwise use default - timeout = ( - test_case.setup_timeout - if hasattr(test_case, "setup_timeout") and test_case.setup_timeout - else None - ) + # Use per-test timeout if provided; otherwise use default at invocation + timeout = test_case.setup_timeout if test_case.setup_timeout is not None else None
143-143: Include working directory in timeout error details for faster triageThe message already shows the elapsed timeout and remediation. Adding the cwd helps correlate failures to the correct test folder, especially when multiple setups run in parallel.
Apply this diff:
- error_details = f"TIMEOUT after {e.timeout}s\n\nYou can increase timeout with environment variable EVAL_SETUP_TIMEOUT=<seconds> or by setting 'setup_timeout' in test_case.yaml\n\nScript that timed out:\n$ {_truncate_script(script)}" + error_details = ( + f"TIMEOUT after {e.timeout}s (cwd: {test_case.folder})\n\n" + "You can increase timeout with environment variable EVAL_SETUP_TIMEOUT=<seconds> " + "or by setting 'setup_timeout' in test_case.yaml\n\n" + f"Script that timed out:\n$ {_truncate_script(script)}" + )tests/llm/utils/reporting/terminal_reporter.py (1)
913-918: Average cost denominator should exclude non-executed runs; update label accordinglyUsing all rows (including skipped/setup-failed) underestimates the “average per test” since those runs typically have zero cost. Compute the denominator from executed runs (VALID_RUNS) or, even stricter, from runs that actually incurred cost. Also, the label should reflect “per run” to match the new semantics.
Apply this diff:
- # Count total number of test runs (not unique test cases) - total_test_runs = len(sorted_results) - avg_cost_per_test = total_cost / total_test_runs if total_test_runs else 0 + # Average over executed runs (exclude skipped/setup-failed) + valid_runs = count_results(sorted_results, ResultType.VALID_RUNS) + # Alternatively, to average only over runs that incurred cost, use: + # valid_runs = sum(1 for r in sorted_results if r.get("cost", 0) > 0) + avg_cost_per_test = total_cost / valid_runs if valid_runs else 0 - console.print( - f"[cyan]Total evaluation cost: ${total_cost:.4f}, Average per test: ${avg_cost_per_test:.6f}[/cyan]" - ) + console.print( + f"[cyan]Total evaluation cost: ${total_cost:.4f}, Average per run: ${avg_cost_per_test:.6f}[/cyan]" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
tests/llm/utils/commands.py(4 hunks)tests/llm/utils/reporting/terminal_reporter.py(1 hunks)tests/llm/utils/test_case_utils.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Type hints are required (mypy is configured in pyproject.toml)
Files:
tests/llm/utils/reporting/terminal_reporter.pytests/llm/utils/commands.pytests/llm/utils/test_case_utils.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Do not use invalid pytest markers; only use markers/tags declared in pyproject.toml
Files:
tests/llm/utils/reporting/terminal_reporter.pytests/llm/utils/commands.pytests/llm/utils/test_case_utils.py
tests/llm/**
📄 CodeRabbit inference engine (CLAUDE.md)
Resource and file naming in evals should be neutral and must not hint at the problem (avoid names like broken-pod or crashloop-app)
Files:
tests/llm/utils/reporting/terminal_reporter.pytests/llm/utils/commands.pytests/llm/utils/test_case_utils.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
🔇 Additional comments (1)
tests/llm/utils/commands.py (1)
57-61: LGTM: Per-call timeout plumbed correctly into subprocess.runThe optional timeout parameter with a safe fallback to EVAL_SETUP_TIMEOUT is clear, logged, and correctly passed to subprocess.run.
Also applies to: 69-69
2 participants
No description provided.