evals: add support for model-comparison and cost calculation - #894
Conversation
WalkthroughThreads model metadata through tests and reporting, replaces test_id/test_name extraction with test_case_name/clean_test_case_id, captures LLM cost/token metrics, centralizes error handling and Braintrust logging, updates reporters for model-aware aggregation/comparison, and adds CLI/logging hooks for cost output. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Pytest
participant Test as "Parametrized Test"
participant Tracer
participant LLM
participant Props as "property_manager"
participant Brain as "log_to_braintrust"
participant Reporter as "Reporter/Aggregator"
Pytest->>Test: instantiate per model (get_models)
Test->>Tracer: start_experiment(additional_metadata={model})
Test->>LLM: call DefaultLLM(model) / tool calls
LLM-->>Test: response + cost/tokens
Test->>Props: update_test_results(..., result)
Props-->>Test: user_properties updated (model, clean_test_case_id, cost/tokens, error fields)
Test->>Tracer: span.log(metadata/tags incl. model, holmes_duration)
Test->>Brain: log_to_braintrust(eval_span, test_case, model, result/scores/error)
Pytest->>Reporter: collect results (test_case_name, model, times, status, cost)
Reporter->>Reporter: aggregate by test_case_name & model -> Avg/P90, costs, pass%
Reporter-->>Console/GitHub: render tables and markdown
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/llm/utils/reporting/github_reporter.py (1)
119-136: Add model column to the markdown table for multi-model clarityWith multi-model runs, identical test_case_name rows are ambiguous. Include the model explicitly so readers can compare models at a glance.
Apply:
- markdown += "\n\n| Test suite | Test case | Status |\n" - markdown += "| --- | --- | --- |\n" + markdown += "\n\n| Test suite | Test case | Model | Status |\n" + markdown += "| --- | --- | --- | --- |\n" @@ - for result in sorted_results: + for result in sorted_results: test_suite = result["test_type"] - test_case_name = result["test_case_name"] + test_case_name = result["test_case_name"] + model = result.get("model", "Unknown") @@ - if braintrust_url: - test_case_name = f"[{test_case_name}]({braintrust_url})" + if braintrust_url: + test_case_name = f"[{test_case_name}]({braintrust_url})" @@ - markdown += f"| {test_suite} | {test_case_name} | {status.markdown_symbol} |\n" + markdown += f"| {test_suite} | {test_case_name} | {model} | {status.markdown_symbol} |\n"This aligns the reporter with the PR’s goal of direct model comparison in a single run.
tests/llm/test_ask_holmes.py (1)
256-260: Fix type inconsistency: tools_called should be a list, not a stringDownstream code expects a List[str] (see TestResult.tools_called). Passing "None" as a string can break consumers.
- if result.tool_calls: - tools_called = [tc.description for tc in result.tool_calls] - else: - tools_called = "None" + if result.tool_calls: + tools_called = [tc.description for tc in result.tool_calls] + else: + tools_called = []
🧹 Nitpick comments (14)
tests/llm/utils/reporting/github_reporter.py (1)
90-117: Nit: “test cases” vs “executions” terminologyYour summary lines count each model run, not unique test cases. Consider rewording to “…/… executions were successful” to avoid confusion when multiple models multiply totals.
tests/llm/conftest.py (2)
601-606: Optional: stabilize ordering by including model as a tertiary keyThis makes row order deterministic when multiple models share the same test_case_name.
- key=lambda r: ( - r["test_type"], - r["test_case_name"], - ), + key=lambda r: (r["test_type"], r["test_case_name"], r.get("model", "")),
111-116: Follow project guideline: move imports to module topImports inside functions (clear_all_mocks, HolmesTestCase, run_all_test_commands/Operation) violate the repo rule “ALWAYS place Python imports at the top of the file.” Move them to the file header; if import cost is a concern, guard usage rather than placement.
Also applies to: 171-176, 190-196
tests/llm/test_investigate.py (2)
75-79: Sanitize MODELS env var valuesTrim whitespace and ignore empty entries to avoid accidental model names like " gpt-4o" or [""].
-def get_models(): - """Get list of models to test from MODELS env var.""" - models_str = os.environ.get("MODELS", "gpt-4o") - return models_str.split(",") +def get_models(): + """Get list of models to test from MODELS env var.""" + models_str = os.environ.get("MODELS", "gpt-4o") + return [m.strip() for m in models_str.split(",") if m.strip()]
122-124: Nit: avoid shadowing built-in name ‘input’You assign input = test_case.investigate_request. Consider a more specific name (e.g., investigate_input or request_input) to avoid overshadowing the Python built-in.
tests/llm/test_ask_holmes.py (1)
57-61: Sanitize MODELS env var valuesMirror the same robustness in all multi-model tests.
-def get_models(): - """Get list of models to test from MODELS env var.""" - models_str = os.environ.get("MODELS", "gpt-4o") - return models_str.split(",") +def get_models(): + """Get list of models to test from MODELS env var.""" + models_str = os.environ.get("MODELS", "gpt-4o") + return [m.strip() for m in models_str.split(",") if m.strip()]tests/llm/test_workload_health.py (1)
114-114: Avoid shadowing built-in name 'input'Using input as a variable shadows the built-in, hurting readability and tooling.
- input = test_case.workload_health_request + request_payload = test_case.workload_health_request @@ - result = workload_health_check(request=input) + result = workload_health_check(request=request_payload) @@ - eval_span.log( - input=input, + eval_span.log( + input=request_payload, output=output or "", expected=str(expected), dataset_record_id=test_case.id, scores=scores, metadata={"model": model}, tags=tags, )Also applies to: 131-136, 169-176
tests/llm/utils/reporting/terminal_reporter.py (7)
13-31: P90 calculation: prefer nearest-rank with ceiling to avoid underestimationUsing int(len*0.9) floors the index and underestimates P90 for small samples. Use ceil-based nearest rank.
- sorted_times = sorted(times) - p90_index = int(len(sorted_times) * 0.9) - # Handle edge case for small sample sizes - if p90_index >= len(sorted_times): - p90_index = len(sorted_times) - 1 - return sorted_times[p90_index] + import math + sorted_times = sorted(times) + # Nearest-rank method: ceil(0.9 * N) - 1, clamped + p90_index = max(0, min(len(sorted_times) - 1, math.ceil(0.9 * len(sorted_times)) - 1)) + return sorted_times[p90_index]
33-45: Define “valid runs” to optionally exclude mock-data failuresRight now you exclude only setup failures and skips. Consider excluding mock_data_failure as infra issues so pass% reflects “real” runs.
- setup_failures = sum(1 for r in results if r.get("is_setup_failure", False)) - skipped = sum(1 for r in results if r.get("status") == "skipped") - return len(results) - setup_failures - skipped + setup_failures = sum(1 for r in results if r.get("is_setup_failure", False)) + skipped = sum(1 for r in results if r.get("status") == "skipped") + mock_failures = sum(1 for r in results if r.get("mock_data_failure", False)) + return len(results) - setup_failures - skipped - mock_failuresIf you intentionally keep mock failures in the denominator, add a short comment explaining why.
62-83: Ruff SIM102: simplify nested conditionFlatten the nested if to satisfy Ruff and improve readability.
- elif setup_failures > 0: - # Only show setup indicator if it's partial (not all runs failed setup) - if runs is None or setup_failures < runs: - indicators = " 🔧" + elif setup_failures > 0 and (runs is None or setup_failures < runs): + # Only show setup indicator if it's partial (not all runs failed setup) + indicators = " 🔧"
119-125: Dead code and missed reuse for time groupingtest_time_groups is built but never used. Also, grouping by nodeid prevents aggregating across models by test_case_name.
Replace with precomputed times_by_test_case for reuse:
- # Group results by test name to calculate P90 - test_time_groups = defaultdict(list) - for result in sorted_results: - test_key = result.get("nodeid", "") - if result.get("execution_time"): - test_time_groups[test_key].append(result.get("execution_time")) + # Precompute times by test_case_name for Avg/P90 lookup (across models) + times_by_test_case = defaultdict(list) + for result in sorted_results: + if result.get("execution_time") and result.get("test_case_name"): + times_by_test_case[result["test_case_name"]].append(result["execution_time"])Then, use times_by_test_case in the time rendering (see below).
174-194: Avoid O(n^2) scan per row when computing Avg/P90You rescan sorted_results for each row. Use the precomputed times_by_test_case.
- exec_time = result.get("execution_time") - if exec_time: - # Get the test case name to look up all times for this test - test_case_name = result["test_case_name"] - # Find all execution times for this specific test case (across all models) - test_times = [] - for r in sorted_results: - if r["test_case_name"] == test_case_name and r.get("execution_time"): - test_times.append(r["execution_time"]) - - # Calculate average time for multiple runs - if len(test_times) > 1: - avg_time = sum(test_times) / len(test_times) - p90 = _calculate_p90(test_times) - time_str = f"Avg: {avg_time:.1f}s\nP90: {p90:.1f}s" - else: - time_str = f"{exec_time:.1f}s" + exec_time = result.get("execution_time") + if exec_time: + test_times = times_by_test_case.get(result["test_case_name"], []) + if len(test_times) > 1: + avg_time = sum(test_times) / len(test_times) + p90 = _calculate_p90(test_times) + time_str = f"Avg: {avg_time:.1f}s\nP90: {p90:.1f}s" + else: + time_str = f"{exec_time:.1f}s" else: time_str = "N/A"
642-645: Totals pass% should use valid runs (exclude setup/skip and, optionally, mock failures)For consistency with per-test rows (which use valid_runs), compute totals with the same denominator.
- total_actual_runs = total_runs - total_pass_pct = _calculate_pass_percentage(total_pass, total_actual_runs) + # Derive totals' valid runs: runs - setup - skipped (- mock failures if excluded above) + total_skipped = sum(1 for results in test_groups.values() for r in results if r.get("status") == "skipped") + total_valid_runs = total_runs - total_setup_fail - total_skipped + total_pass_pct = _calculate_pass_percentage(total_pass, total_valid_runs)If you decide to exclude mock failures in _calculate_valid_runs, subtract them here as well for symmetry.
85-112: Remove unused helper_parse_test_nameA ripgrep search for
\b_parse_test_name\s*\(only matches its own definition in tests/llm/utils/reporting/terminal_reporter.py, confirming there are no call sites. Since test case names are now sourced fromresult["test_case_name"]in tests/llm/utils/test_results.py, this function is vestigial and may drift out of sync. Removing it will simplify maintenance.• File tests/llm/utils/reporting/terminal_reporter.py, lines 85–112: delete the entire
_parse_test_namedefinition.
📜 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 (7)
tests/llm/conftest.py(2 hunks)tests/llm/test_ask_holmes.py(10 hunks)tests/llm/test_investigate.py(6 hunks)tests/llm/test_workload_health.py(6 hunks)tests/llm/utils/reporting/github_reporter.py(1 hunks)tests/llm/utils/reporting/terminal_reporter.py(11 hunks)tests/llm/utils/test_results.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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 (project is type-checked with mypy)
Use Ruff for formatting and linting (configured in pyproject.toml)
Files:
tests/llm/utils/reporting/github_reporter.pytests/llm/conftest.pytests/llm/utils/test_results.pytests/llm/test_investigate.pytests/llm/test_ask_holmes.pytests/llm/test_workload_health.pytests/llm/utils/reporting/terminal_reporter.py
{tests/**/*.py,pyproject.toml}
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are defined in pyproject.toml; never introduce undefined markers in tests
Files:
tests/llm/utils/reporting/github_reporter.pytests/llm/conftest.pytests/llm/utils/test_results.pytests/llm/test_investigate.pytests/llm/test_ask_holmes.pytests/llm/test_workload_health.pytests/llm/utils/reporting/terminal_reporter.py
🧬 Code graph analysis (6)
tests/llm/utils/reporting/github_reporter.py (2)
tests/llm/utils/test_results.py (3)
test_case_name(24-40)TestStatus(43-118)markdown_symbol(80-92)tests/llm/utils/braintrust.py (1)
get_braintrust_url(172-206)
tests/llm/conftest.py (1)
tests/llm/utils/test_results.py (2)
TestResult(8-40)test_case_name(24-40)
tests/llm/test_investigate.py (3)
tests/llm/test_ask_holmes.py (1)
get_models(57-60)tests/llm/test_workload_health.py (1)
get_models(66-69)holmes/core/tracing.py (1)
SpanType(90-98)
tests/llm/test_ask_holmes.py (4)
tests/llm/test_investigate.py (1)
get_models(75-78)tests/llm/test_workload_health.py (1)
get_models(66-69)holmes/core/tracing.py (6)
start_experiment(123-125)start_experiment(148-177)start_trace(127-129)start_trace(179-209)SpanType(90-98)log(107-108)holmes/core/llm.py (1)
DefaultLLM(61-268)
tests/llm/test_workload_health.py (2)
holmes/core/tracing.py (6)
start_experiment(123-125)start_experiment(148-177)start_trace(127-129)start_trace(179-209)SpanType(90-98)log(107-108)tests/llm/conftest.py (1)
mock_generation_config(51-79)
tests/llm/utils/reporting/terminal_reporter.py (1)
tests/llm/utils/test_results.py (3)
passed(56-59)test_case_name(24-40)TestStatus(43-118)
🪛 Ruff (0.12.2)
tests/llm/utils/reporting/terminal_reporter.py
78-80: Use a single if statement instead of nested if statements
(SIM102)
⏰ 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.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
🔇 Additional comments (9)
tests/llm/conftest.py (2)
479-479: LGTM on explicit None for clean_test_case_id in skipped testsClear, intentional signal that fallback parsing should be used.
557-559: LGTM on threading model and clean_test_case_id via user_propertiesThis enables consistent cross-file reporting and model-aware analytics.
tests/llm/test_investigate.py (2)
134-135: LGTM on span naming with model contextIncluding the model in the eval span name materially improves trace disambiguation.
199-201: LGTM on tagging and metadata with modelConsistent propagation of model into tags/metadata will help downstream analytics.
Also applies to: 208-210
tests/llm/test_ask_holmes.py (1)
64-68: LGTM on multi-model threading and trace tagging
- Parametrization over model and propagation into user_properties are correct.
- Span naming, tags, and metadata now carry model context consistently.
- ask_holmes signature and call sites updated appropriately.
Also applies to: 77-81, 123-124, 150-151, 160-161, 172-174, 239-242, 249-251
tests/llm/test_workload_health.py (3)
117-119: Model-scoped trace naming looks goodIncluding the model in the span name makes multi-model runs easy to distinguish in Braintrust and logs.
73-75: Add Parametrize IDs for ReadabilityAdd
idsto themodelparametrization so each model appears by name in pytest outputs:- @pytest.mark.parametrize("model", get_models()) + @pytest.mark.parametrize("model", get_models(), ids=lambda m: m)• Location: tests/llm/test_workload_health.py (around line 73)
• Confirmed: the custom"llm"marker is declared under[tool.pytest.ini_options]in pyproject.toml.
164-176: Confirmed:Span.logsupports arbitrary keyword arguments (includingtags)
The Braintrust-backedSpan.logmethod is defined asdef log(self, **event: Any), allowing any named fields to be passed through (e.g.tags) and batched/uploaded accordingly. The no-opDummySpan.log(self, *args, **kwargs)likewise accepts and ignores extra kwargs without error. No downstream changes are required; your use of thetagskwarg will be forwarded as intended.tests/llm/utils/reporting/terminal_reporter.py (1)
245-246: LLM prompt switch to test_case_name is correctUsing result.test_case_name keeps analysis aligned with the new naming flow.
aantn
changed the title
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
tests/llm/conftest.py (1)
594-607: Normalize fallback test_case_name to remove any model suffix in parametrized nodeidsWhen clean_test_case_id is missing (e.g., skipped/early-fail), the fallback name may include a trailing “-” (and sometimes provider/model with a slash), making names inconsistent vs. normal runs.
Apply this diff right after assigning temp_result.test_case_name:
- # Add extracted test case name to the result dict - result["test_case_name"] = temp_result.test_case_name + # Add extracted test case name to the result dict + result["test_case_name"] = temp_result.test_case_name + # Normalize to remove any trailing model suffix (e.g., "-gpt-4o" or "-anthropic/claude-3-5-sonnet") + # Keep original base name that precedes the last hyphen-delimited suffix. + # This keeps grouping identical across skipped and non-skipped runs. + if "-" in result["test_case_name"]: + # Split once on the first hyphen only if we detect common model-delimiter patterns + # Safer: strip the last hyphen and everything after it + import re + result["test_case_name"] = re.sub(r"-[^-]+(?:/[^-]+)*$", "", result["test_case_name"])tests/llm/test_investigate.py (1)
114-116: Use positional args for tracer.start_experiment to support both Braintrust and no-op tracersCalling with additional_metadata=... breaks DummyTracer which expects metadata as the second positional arg.
Apply this diff:
- metadata = {"model": model} - tracer.start_experiment(additional_metadata=metadata) + metadata = {"model": model} + tracer.start_experiment(None, metadata)tests/llm/test_ask_holmes.py (1)
118-121: Use positional args for tracer.start_experiment to support both tracer implementationsSame compatibility concern as in investigate: switch to positional call.
Apply this diff:
- metadata = {"model": model} - tracer.start_experiment(additional_metadata=metadata) + metadata = {"model": model} + tracer.start_experiment(None, metadata)
🧹 Nitpick comments (16)
holmes/utils/console/logging.py (1)
44-51: Make cost logger emissions reliable across verbosity levels and add return type
- Add a return type to satisfy mypy.
- Ensure the "holmes.costs" logger propagates to the RichHandler (it does by default, but setting it explicitly avoids surprises if project-wide logging config changes).
- Consider tightening types to avoid the current type: ignore on the call site by allowing Optional[List[bool]] in cli_flags_to_verbosity (shown below).
Apply this diff here:
-def init_logging(verbose_flags: Optional[List[bool]] = None, log_costs: bool = False): +def init_logging( + verbose_flags: Optional[List[bool]] = None, log_costs: bool = False +) -> Console: @@ # Setup cost logger if requested if log_costs: cost_logger = logging.getLogger("holmes.costs") cost_logger.setLevel(logging.DEBUG) + cost_logger.propagate = TrueAnd outside this hunk (to drop the type: ignore at call sites), consider updating the helper signature:
# Outside this hunk def cli_flags_to_verbosity(verbose_flags: Optional[List[bool]]) -> Verbosity: ...holmes/core/tool_calling_llm.py (1)
707-728: Harden cost extraction in streaming path as wellMirror the same dict/object detection and lazy logging format here to avoid dropping usage fields with non-dict usage objects and to avoid f-string overhead when the logger is disabled.
Apply this diff:
- try: - cost_value = ( - full_response._hidden_params.get("response_cost", 0) - if hasattr(full_response, "_hidden_params") - else 0 - ) - # Ensure cost is a float - cost = float(cost_value) if cost_value is not None else 0.0 - usage = getattr(full_response, "usage", {}) - if usage: - cost_logger.debug( - f"LLM iteration cost: ${cost:.6f} | Tokens: {usage.get('prompt_tokens', 0)} prompt + {usage.get('completion_tokens', 0)} completion = {usage.get('total_tokens', 0)} total" - ) - elif cost > 0: - cost_logger.debug( - f"LLM iteration cost: ${cost:.6f} | Token usage not available" - ) + try: + cost_value = ( + full_response._hidden_params.get("response_cost", 0) + if hasattr(full_response, "_hidden_params") + else 0 + ) + cost = float(cost_value) if cost_value is not None else 0.0 + usage = getattr(full_response, "usage", None) + if usage: + if isinstance(usage, dict): + p = usage.get("prompt_tokens", 0); c = usage.get("completion_tokens", 0); t = usage.get("total_tokens", 0) + else: + p = getattr(usage, "prompt_tokens", 0); c = getattr(usage, "completion_tokens", 0); t = getattr(usage, "total_tokens", 0) + cost_logger.debug("LLM iteration cost: $%.6f | Tokens: %d prompt + %d completion = %d total", cost, p, c, t) + elif cost > 0: + cost_logger.debug("LLM iteration cost: $%.6f | Token usage not available", cost)docs/development/evals/index.md (1)
89-94: Fix markdown lint: use asterisk list style and keep blank line after intromarkdownlint (MD004) expects asterisk-style bullets in this repo. Also keep the blank line between the paragraph and the list (already present).
Apply this diff:
-When running multi-model benchmarks: -- Results will show a **Model Comparison Table** with side-by-side performance metrics -- Each model's pass rate, execution times, and P90 percentiles are displayed -- Tests are parameterized by model, so you'll see separate results for each model/test combination -- Use `CLASSIFIER_MODEL` to ensure consistent scoring across all models +When running multi-model benchmarks: +* Results will show a **Model Comparison Table** with side-by-side performance metrics +* Each model's pass rate, execution times, and P90 percentiles are displayed +* Tests are parameterized by model, so you'll see separate results for each model/test combination +* Use `CLASSIFIER_MODEL` to ensure consistent scoring across all modelsholmes/main.py (2)
107-111: New CLI flag (--log-costs) is well-scopedGood, opt-in and self-explanatory. Consider propagating to investigate subcommands later if cost visibility is also useful there.
228-228: Drop unnecessary type ignore on init_logging callinit_logging now accepts (Optional[List[bool]], bool) and returns Console. The type: ignore can be removed.
Apply this diff:
- console = init_logging(verbose, log_costs) # type: ignore + console = init_logging(verbose, log_costs)tests/llm/conftest.py (1)
538-553: Good UX on error display; add minor guardrails
- The compact error rendering for “Test not executed”/“Unknown” is a nice touch.
- Consider also truncating extremely long values in user_props["actual"] to prevent overly wide tables in edge cases (optional).
Also applies to: 559-559, 573-591
tests/llm/test_investigate.py (2)
76-80: Type-hint and sanitize MODELS parsingReturn type is missing and values may contain spaces. Strip items and drop empties for robustness.
Apply this diff:
-def get_models(): - """Get list of models to test from MODELS env var.""" - models_str = os.environ.get("MODELS", "gpt-4o") - return models_str.split(",") +def get_models() -> list[str]: + """Get list of models to test from MODELS env var (comma-separated).""" + models_str = os.environ.get("MODELS", "gpt-4o") + return [m.strip() for m in models_str.split(",") if m.strip()]
133-140: Flatten nested with statements where possible (Ruff SIM117)You can combine patch.dict and start_trace to reduce indentation. Note eval_span-dependent spans must remain nested.
Apply this diff:
- try: - with patch.dict( - os.environ, {"HOLMES_STRUCTURED_OUTPUT_CONVERSION_FEATURE_FLAG": "False"} - ): - with tracer.start_trace( - name=f"{test_case.id}[{model}]", span_type=SpanType.EVAL - ) as eval_span: + try: + with patch.dict( + os.environ, {"HOLMES_STRUCTURED_OUTPUT_CONVERSION_FEATURE_FLAG": "False"} + ), tracer.start_trace(name=f"{test_case.id}[{model}]", span_type=SpanType.EVAL) as eval_span:tests/llm/test_ask_holmes.py (2)
58-62: Type-hint and sanitize MODELS parsingMirror the investigate test helper: add return type, strip whitespace, and drop empties.
Apply this diff:
-def get_models(): - """Get list of models to test from MODELS env var.""" - models_str = os.environ.get("MODELS", "gpt-4o") - return models_str.split(",") +def get_models() -> list[str]: + """Get list of models to test from MODELS env var (comma-separated).""" + models_str = os.environ.get("MODELS", "gpt-4o") + return [m.strip() for m in models_str.split(",") if m.strip()]
260-263: Normalize tools_called to a list for consistencyPassing "None" as a string works but is semantically odd. Prefer an empty list to represent “no tools”.
Apply this diff:
- if result.tool_calls: - tools_called = [tc.description for tc in result.tool_calls] - else: - tools_called = "None" + tools_called = [tc.description for tc in result.tool_calls] if result.tool_calls else []tests/llm/utils/reporting/terminal_reporter.py (6)
226-231: Inline cost formatting and trim branchesUse a compact expression; improves readability and satisfies Ruff’s SIM108.
Apply this diff:
- cost = result.get("cost", 0) - if cost > 0: - cost_str = f"${cost:.4f}" - else: - cost_str = "—" + cost = result.get("cost", 0.0) + cost_str = f"${cost:.4f}" if cost > 0 else "—"
166-172: Remove unused grouping (dead code) or use ittest_time_groups is populated but never used.
Apply this diff to remove:
- # Group results by test name to calculate P90 - test_time_groups = defaultdict(list) - for result in sorted_results: - test_key = result.get("nodeid", "") - if result.get("execution_time"): - test_time_groups[test_key].append(result.get("execution_time"))
86-92: Percentile calculation: use statistics.quantiles for correctness on small samplesYour manual p90 index can under/over-shoot for small N. The standard library can compute quantiles with an inclusive method.
Apply this diff:
- sorted_times = sorted(times) - p90_index = int(len(sorted_times) * 0.9) - # Handle edge case for small sample sizes - if p90_index >= len(sorted_times): - p90_index = len(sorted_times) - 1 - return sorted_times[p90_index] + import statistics + try: + return statistics.quantiles(times, n=10, method="inclusive")[8] + except Exception: + # Fallback: simple max for very small samples (<=1) + return max(times) if times else 0
449-459: Ruff SIM118: iterate dicts directly, not .keys()Minor cleanup to satisfy Ruff.
Apply this diff:
- for model in times_dict.keys(): + for model in times_dict: @@ - for model in costs_dict.keys(): + for model in costs_dict:Also applies to: 486-501
1146-1163: Total pass percentage should use valid runs, not all runsFor consistency with per-test rows, compute totals as passed / (runs - skipped - setup_fail).
Apply this diff:
- total_actual_runs = total_runs - total_pass_pct = _calculate_pass_percentage(total_pass, total_actual_runs) + total_valid_runs = total_runs - total_skip - total_setup_fail + total_pass_pct = _calculate_pass_percentage(total_pass, total_valid_runs)
1205-1211: Iteration note is known-wrong; either fix or removeThe TODO acknowledges incorrect iteration counts. Consider removing this note until iteration calculation can distinguish per-model runs vs. true iterations.
Proposed quick fix (remove block):
- max_iterations = max(len(results) for results in test_groups.values()) - if max_iterations > 1: - # TODO this is wrong - in case you run with 1 Iterations it says '2' instead of '1' - console.print( - f"[dim]Note: Tests were run with {max_iterations} iterations[/dim]" - ) + # (Iteration note removed until accurate calculation is implemented)
📜 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 (10)
docs/development/evals/index.md(2 hunks)holmes/core/tool_calling_llm.py(8 hunks)holmes/main.py(3 hunks)holmes/utils/console/logging.py(1 hunks)tests/llm/conftest.py(3 hunks)tests/llm/test_ask_holmes.py(13 hunks)tests/llm/test_investigate.py(7 hunks)tests/llm/utils/braintrust.py(2 hunks)tests/llm/utils/property_manager.py(2 hunks)tests/llm/utils/reporting/terminal_reporter.py(13 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 (project is type-checked with mypy)
Use Ruff for formatting and linting (configured in pyproject.toml)
Files:
holmes/utils/console/logging.pyholmes/main.pytests/llm/utils/property_manager.pyholmes/core/tool_calling_llm.pytests/llm/utils/braintrust.pytests/llm/test_investigate.pytests/llm/conftest.pytests/llm/test_ask_holmes.pytests/llm/utils/reporting/terminal_reporter.py
{tests/**/*.py,pyproject.toml}
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are defined in pyproject.toml; never introduce undefined markers in tests
Files:
tests/llm/utils/property_manager.pytests/llm/utils/braintrust.pytests/llm/test_investigate.pytests/llm/conftest.pytests/llm/test_ask_holmes.pytests/llm/utils/reporting/terminal_reporter.py
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
In MkDocs docs, always add a blank line between a header/bold text and a following list
Files:
docs/development/evals/index.md
🧬 Code graph analysis (6)
holmes/main.py (1)
holmes/utils/console/logging.py (1)
init_logging(44-103)
tests/llm/utils/braintrust.py (2)
tests/llm/utils/test_case_utils.py (3)
HolmesTestCase(44-69)AskHolmesTestCase(72-85)InvestigateTestCase(88-93)tests/llm/conftest.py (1)
mock_generation_config(51-79)
tests/llm/test_investigate.py (4)
tests/llm/utils/braintrust.py (1)
log_to_braintrust(173-305)tests/llm/utils/commands.py (1)
set_test_env_vars(152-180)holmes/core/investigation.py (1)
investigate_issues(23-75)tests/llm/utils/property_manager.py (2)
update_mock_error(111-114)update_test_results(52-108)
tests/llm/conftest.py (1)
tests/llm/utils/test_results.py (2)
TestResult(8-40)test_case_name(24-40)
tests/llm/test_ask_holmes.py (6)
tests/llm/utils/braintrust.py (1)
log_to_braintrust(173-305)holmes/core/tracing.py (6)
start_experiment(123-125)start_experiment(148-177)start_trace(127-129)start_trace(179-209)SpanType(90-98)log(107-108)holmes/core/tool_calling_llm.py (1)
LLMResult(192-209)tests/llm/conftest.py (1)
mock_generation_config(51-79)tests/llm/utils/property_manager.py (1)
update_test_results(52-108)holmes/core/llm.py (1)
DefaultLLM(61-268)
tests/llm/utils/reporting/terminal_reporter.py (1)
tests/llm/utils/test_results.py (3)
TestStatus(43-118)passed(56-59)test_case_name(24-40)
🪛 markdownlint-cli2 (0.17.2)
docs/development/evals/index.md
90-90: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
91-91: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
92-92: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
93-93: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
🪛 Ruff (0.12.2)
tests/llm/utils/braintrust.py
260-261: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
tests/llm/test_investigate.py
134-139: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
tests/llm/utils/reporting/terminal_reporter.py
125-127: Use a single if statement instead of nested if statements
(SIM102)
228-231: Use ternary operator cost_str = f"${cost:.4f}" if cost > 0 else "—" instead of if-else-block
Replace if-else-block with cost_str = f"${cost:.4f}" if cost > 0 else "—"
(SIM108)
449-449: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
486-486: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
⏰ 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). (3)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (8)
holmes/core/tool_calling_llm.py (1)
46-48: Cost logger configuration verifiedConfirmed via code search that only
holmes/utils/console/logging.pyexplicitly configures the"holmes.costs"logger, and no other module invokesbasicConfigoraddHandleron it:
holmes/utils/console/logging.py (lines 49–50):
Setscost_logger = logging.getLogger("holmes.costs")and appliescost_logger.setLevel(logging.DEBUG)only when--log-costsis enabled.No other occurrences of
logging.basicConfig(oraddHandler(targeting"holmes.costs"in the codebase.The cost logger will remain silent by default and only emit output when requested. Approving the changes.
docs/development/evals/index.md (4)
67-87: Clear new section — good examples and contextThe “Multi-Model Benchmarking” section reads well and aligns with the new MODELS flow. Nice job including both single-run and multi-iteration examples.
178-194: Recommended workflow doc is solidGood emphasis on MODELS and the explicit list of comparison outputs. No changes needed.
196-212: Alternative flows read wellExamples for separate runs are clear, including the classifier note for Anthropic. LGTM.
214-225: Braintrust integration section is succinct and helpfulNice to have the environment variable snippet and a sample command. Consider linking to any internal doc on Braintrust dashboards if available.
holmes/main.py (1)
184-185: ask(): plumbed log_costs option — LGTMWires through cleanly.
tests/llm/test_investigate.py (1)
256-259: LGTM: results/threading + cost propagationPassing the full result into update_test_results enables downstream cost/token reporting and aligns with the reporter changes.
tests/llm/test_ask_holmes.py (1)
330-331: LGTM: model-threaded LLM initialization and duration logging
- DefaultLLM(model, tracer=tracer) makes the model explicit and traceable per run.
- Logging holmes_duration on the eval span matches the new reporting.
Also applies to: 376-378
arikalon1
left a comment
arikalon1
left a comment
There was a problem hiding this comment.
nice work
left some minor comments
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
holmes/core/tracing.py (3)
234-235: Don't hardcode org in trace URL; use BRAINTRUST_ORGHardcoding
robustadevbreaks links for users running with a differentBRAINTRUST_ORG.- return f"https://www.braintrust.dev/app/robustadev/p/{self.project}/experiments/{experiment_name}" + return f"https://www.braintrust.dev/app/{BRAINTRUST_ORG}/p/{self.project}/experiments/{experiment_name}"
241-249: Move import out of method to comply with project stylePer repo guidelines, imports must be at the top of the file. Move
ChatCompletionWrapperimport to the module-level and remove the in-function import.- from braintrust.oai import ChatCompletionWrapper + # ChatCompletionWrapper is imported at module import time (see top-of-file).
17-33: Top-level import for ChatCompletionWrapper; drop unused SpanTypeAttributeThis aligns with the "imports at top" rule and fixes a likely Ruff F401 on the unused
SpanTypeAttribute.try: - import braintrust - from braintrust import Span, SpanTypeAttribute + import braintrust + from braintrust import Span + from braintrust.oai import ChatCompletionWrapper @@ - if TYPE_CHECKING: - from braintrust import Span, SpanTypeAttribute - else: - Span = Any - SpanTypeAttribute = Any + if TYPE_CHECKING: + from braintrust import Span + from braintrust.oai import ChatCompletionWrapper + else: + Span = Any + ChatCompletionWrapper = Any
🧹 Nitpick comments (5)
holmes/core/tracing.py (5)
155-158: Fix docstring parameter nameDocstring still refers to
metadatabut the parameter isadditional_metadata.- Args: - experiment_name: Name for the experiment, auto-generated if None - metadata: Metadata to attach to experiment + Args: + experiment_name: Name for the experiment, auto-generated if None + additional_metadata: Extra metadata to attach to the experiment (merged with machine state tags)
85-88: Make NoopSpan detection less brittleString-matching the repr of a type is fragile. Consider checking the class name case-insensitively; keep the None check.
-def _is_noop_span(span) -> bool: - """Check if a span is a Braintrust NoopSpan (inactive span).""" - return span is None or str(type(span)).endswith("_NoopSpan'>") +def _is_noop_span(span: Any) -> bool: + """Check if a span is a Braintrust NoopSpan (inactive span).""" + if span is None: + return True + cls_name = getattr(span, "__class__", type(None)).__name__ + return "noopspan" in cls_name.lower()
271-273: Docstring nit: return type description mentions DummySpan instead of DummyTracerThe factory returns a tracer, not a span, when disabled.
- Returns: - Tracer instance if tracing enabled, DummySpan if disabled + Returns: + Tracer instance if tracing enabled, DummyTracer if disabled
127-137: Optional: add type hints to DummyTracer methods for consistency with mypyTo align with “type hints are required,” consider annotating these as well. Keeps the DummyTracer API consistent with BraintrustTracer.
- def start_trace(self, name: str, span_type=None): + def start_trace(self, name: str, span_type: Optional[Any] = None) -> DummySpan: @@ - def get_trace_url(self): + def get_trace_url(self) -> Optional[str]: @@ - def wrap_llm(self, llm_module): + def wrap_llm(self, llm_module: Any) -> Any:
79-83: Optional: add return type to get_experiment_name and readable_timestampMinor typing polish; helps callers and satisfies stricter mypy configs.
-def readable_timestamp(): +def readable_timestamp() -> str: @@ -def get_experiment_name(): +def get_experiment_name() -> str:
📜 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 (2)
holmes/core/tracing.py(1 hunks)tests/llm/test_workload_health.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/llm/test_workload_health.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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 (project is type-checked with mypy)
Use Ruff for formatting and linting (configured in pyproject.toml)
Files:
holmes/core/tracing.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). (1)
- GitHub Check: llm_evals
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/llm/utils/reporting/terminal_reporter.py (1)
1188-1192: Fix “skipped entirely” computationCurrent formula divides by a max iteration count and is marked as wrong. Count fully-skipped test groups directly.
- if total_skip > 0: - console.print( - f"[dim]Note: {total_skip // max(len(test_groups.get(name, [])) for name in test_groups)} tests were skipped entirely[/dim]" - ) + fully_skipped_tests = sum(1 for results in test_groups.values() if all(r.get("status") == "skipped" for r in results)) + if fully_skipped_tests > 0: + console.print(f"[dim]Note: {fully_skipped_tests} test(s) were skipped entirely[/dim]")
♻️ Duplicate comments (5)
tests/llm/utils/test_case_utils.py (1)
18-22: Preserve backward compatibility with MODEL and trim whitespacePrior behavior used MODEL (singular). Support both env vars and robustly split/trim to avoid empty entries. Also add a return type.
-def get_models(): - """Get list of models to test from MODELS env var.""" - models_str = os.environ.get("MODELS", "gpt-4o") - return models_str.split(",") +from typing import List + +def get_models() -> List[str]: + """Get list of models from environment. + + Backward compatible with legacy MODEL. Both MODEL and MODELS may contain comma-separated values. + Precedence: MODEL > MODELS > default. + """ + raw = os.environ.get("MODEL") or os.environ.get("MODELS") or "gpt-4o" + return [m.strip() for m in raw.split(",") if m.strip()]tests/llm/test_ask_holmes.py (1)
70-72: Use positional args for start_experiment to support both tracersAs noted in a previous review, Braintrust and no-op tracers don’t share the same keyword name. Using positional args keeps both working.
- metadata = {"model": model} - tracer.start_experiment(additional_metadata=metadata) + metadata = {"model": model} + tracer.start_experiment(None, metadata)tests/llm/utils/property_manager.py (1)
145-169: Cost/tokens: log via named logger, avoid duplicates, record tokens even when cost == 0
- Use a dedicated logger (holmes.costs) for CLI piping.
- Persist properties with update_property to prevent duplicates.
- Tokens are useful even when cost is 0; persist when attributes exist.
- Keep the informative log line.
- # Check for cost tracking in LLMResult (from ask_holmes tests) - if hasattr(result, "total_cost") and result.total_cost > 0: + # Check for cost/tokens tracking in LLMResult (from ask_holmes tests) + if hasattr(result, "total_cost"): test_case_id = None model = None # Extract test_case_id and model from user_properties for key, value in request.node.user_properties: if key == "clean_test_case_id": test_case_id = value elif key == "model": model = value - if test_case_id and model: - logging.info( - f"Test {test_case_id} with {model} - Total cost: ${result.total_cost:.6f}, Total tokens: {result.total_tokens if hasattr(result, 'total_tokens') else 'N/A'}" - ) - - request.node.user_properties.append(("cost", result.total_cost)) - if hasattr(result, "total_tokens"): - request.node.user_properties.append(("total_tokens", result.total_tokens)) - if hasattr(result, "prompt_tokens"): - request.node.user_properties.append(("prompt_tokens", result.prompt_tokens)) - if hasattr(result, "completion_tokens"): - request.node.user_properties.append( - ("completion_tokens", result.completion_tokens) - ) + if test_case_id and model: + logging.getLogger("holmes.costs").info( + "Test %s with %s - Total cost: $%.6f, Total tokens: %s", + test_case_id, + model, + float(getattr(result, "total_cost", 0.0)), + getattr(result, "total_tokens", "N/A"), + ) + + # Always persist cost/tokens when attributes exist (even if cost == 0) + update_property(request, "cost", float(getattr(result, "total_cost", 0.0))) + if hasattr(result, "total_tokens"): + update_property(request, "total_tokens", int(getattr(result, "total_tokens", 0))) + if hasattr(result, "prompt_tokens"): + update_property(request, "prompt_tokens", int(getattr(result, "prompt_tokens", 0))) + if hasattr(result, "completion_tokens"): + update_property(request, "completion_tokens", int(getattr(result, "completion_tokens", 0)))tests/llm/test_investigate.py (1)
100-103: Fix: make start_experiment call signature work with both real and no-op tracers.Calling with keyword additional_metadata breaks when a no-op tracer is used. Pass by position.
Apply:
- metadata = {"model": model} - tracer.start_experiment(additional_metadata=metadata) + metadata = {"model": model} + tracer.start_experiment(None, metadata)#!/bin/bash # Verify all start_experiment call sites and tracer signatures rg -nP --type=py 'start_experiment\s*\(' -C2 rg -nP --type=py 'def\s+start_experiment\s*\(' holmes -C3tests/llm/test_workload_health.py (1)
88-90: Fix: make start_experiment call signature compatible across tracer implementations.Use positional args to avoid TypeError with the no-op tracer.
Apply:
- metadata = {"model": model} - tracer.start_experiment(additional_metadata=metadata) + metadata = {"model": model} + tracer.start_experiment(None, metadata)
🧹 Nitpick comments (25)
tests/llm/utils/setup_cleanup.py (2)
32-41: Good internalization of truncation logic; consider minor resiliency tweaksThe helper looks solid and typed. Two optional improvements:
- Guard against extremely long inputs by short-circuiting on empty strings and normalizing trailing newline behavior.
- Consider pluralization for label when remaining == 1.
Apply this minimal tweak:
def _truncate_output(data: str, max_lines: int = 10, label: str = "lines") -> str: - """Truncate output to max_lines for readability.""" + """Truncate output to max_lines for readability.""" + if not data: + return "" lines = data.split("\n") if len(lines) > max_lines: preview_lines = lines[:max_lines] remaining = len(lines) - max_lines - preview_lines.append(f"... [TRUNCATED: {remaining} more {label} not shown]") + unit = label if remaining != 1 else label.rstrip("s") + preview_lines.append(f"... [TRUNCATED: {remaining} more {unit} not shown]") return "\n".join(preview_lines) return data
43-46: Use the new truncation in warnings to prevent megabyte-sized logsYou’ve implemented
_truncate_outputand wiredformat_error_output, but error details in warnings still use full text (the truncated logging lines are commented out below). Recommend truncatingresult.error_detailsinside the warning messages to avoid huge outputs in CI.Apply this focused change where warnings are emitted:
- warnings.warn( - f"{operation.value} timeout for test {test_case.id}: Command '{result.command}' timed out after {result.elapsed_time:.2f}s. Output: {result.error_details}", + warnings.warn( + f"{operation.value} timeout for test {test_case.id}: Command '{result.command}' timed out after {result.elapsed_time:.2f}s. Output: {format_error_output(result.error_details)}", UserWarning, stacklevel=2, )- warnings.warn( - f"{operation.value} failed for test {test_case.id}: Command '{result.command}' failed with {result.exit_info} in {result.elapsed_time:.2f}s. Output: {result.error_details}", + warnings.warn( + f"{operation.value} failed for test {test_case.id}: Command '{result.command}' failed with {result.exit_info} in {result.elapsed_time:.2f}s. Output: {format_error_output(result.error_details)}", UserWarning, stacklevel=2, )tests/llm/utils/reporting/terminal_reporter.py (6)
166-172: Remove unused local aggregation to avoid confusion
test_time_groupsis built but never used. Drop it to keep handle_console_output minimal.- # Group results by test name to calculate P90 - test_time_groups = defaultdict(list) - for result in sorted_results: - test_key = result.get("nodeid", "") - if result.get("execution_time"): - test_time_groups[test_key].append(result.get("execution_time")) + # (Removed unused per-test P90 aggregation; summary handles P90 later)
227-231: Ruff SIM108: Inline the trivial branch with a ternaryMicro-cleanup; reduces four lines to one.
- if cost > 0: - cost_str = f"${cost:.4f}" - else: - cost_str = "—" + cost_str = f"${cost:.4f}" if cost and cost > 0 else "—"
123-129: Ruff SIM102: Flatten nested condition in failure indicatorsSame behavior with simpler logic.
- elif setup_failures > 0: - # Only show setup indicator if it's partial (not all runs failed setup) - if runs is None or setup_failures < runs: - indicators = " 🔧" + elif setup_failures > 0 and (runs is None or setup_failures < runs): + # Only show setup indicator if it's partial (not all runs failed setup) + indicators = " 🔧"
449-460: Ruff SIM118: Iterate dict directly instead of .keys()No behavior change.
- for model in times_dict.keys(): + for model in times_dict:
486-503: Ruff SIM118: Iterate dict directly instead of .keys()Same nit for costs.
- for model in costs_dict.keys(): + for model in costs_dict:
132-159: Dead code: _parse_test_name isn’t used anymoreAll call sites consume result["test_case_name"]. Safe to remove to reduce surface area.
- def _parse_test_name(nodeid: str, remove_iteration: bool = True) -> str: - ... - return nodeid.split("::")[-1] if "::" in nodeid else nodeid + # Removed unused parser; test_case_name is precomputed upstream.tests/llm/utils/test_case_utils.py (1)
110-130: Setup failure handling: good; add light typing for mypy and consistencyLogic is correct and aligns with downstream reporting. Recommend typing request and shared_test_infrastructure to avoid mypy noise and make intent explicit.
-from typing import Any, List, Literal, Optional, TypeVar, Union, cast +from typing import Any, List, Literal, Optional, TypeVar, Union, cast, Mapping +import pytest @@ -def check_and_skip_test( - test_case: HolmesTestCase, request=None, shared_test_infrastructure=None -) -> None: +def check_and_skip_test( + test_case: HolmesTestCase, + request: Any | None = None, + shared_test_infrastructure: Mapping[str, Any] | None = None, +) -> None:tests/llm/test_ask_holmes.py (1)
50-56: Parametrization by model looks good; consider readable IDsOptional: label parametrized tests with model names for clearer nodeids.
-@pytest.mark.parametrize("model", get_models()) +@pytest.mark.parametrize("model", get_models(), ids=lambda m: f"model={m}")tests/llm/utils/property_manager.py (6)
1-2: Augment imports for typing and contextlib; keep imports top-levelTo align with project guidelines and upcoming refactors below.
-import logging -from typing import List, Any, Union, Optional, Dict +import logging +from contextlib import suppress +from typing import List, Any, Union, Optional, Dict, Mapping, Sequence
6-13: Annotate request type for mypy and docsSignature is otherwise fine; add a conservative type for request.
-def set_initial_properties(request, test_case: HolmesTestCase, model: str) -> None: +def set_initial_properties(request: Any, test_case: HolmesTestCase, model: str) -> None:
64-73: Strengthen types for update_test_results signatureUse Mapping/Sequence for inputs; clarify return type; keeps compatibility.
-def update_test_results( - request, - output: str, - tools_called: Union[List[str], str], - scores: Optional[Dict[str, Any]] = None, - result: Any = None, - test_case: Any = None, - eval_span: Any = None, - caplog: Any = None, -) -> Dict[str, Any]: +def update_test_results( + request: Any, + output: str, + tools_called: Union[Sequence[str], str], + scores: Optional[Mapping[str, Any]] = None, + result: Any = None, + test_case: Any = None, + eval_span: Any = None, + caplog: Any = None, +) -> Dict[str, Any]:
100-107: Ruff SIM102: collapse nested hasattr chainPure readability tweak.
- if hasattr(test_case, "evaluation") and hasattr( - test_case.evaluation, "correctness" - ): + if hasattr(test_case, "evaluation") and hasattr(test_case.evaluation, "correctness"):
173-177: Avoid duplicate mock flags; reuse update_propertyKeeps a single truthy flag even on multiple invocations.
def update_mock_error(request, error: Exception) -> None: """Update properties when a mock error occurs.""" update_property(request, "actual", f"Mock data error: {str(error)}") - request.node.user_properties.append(("mock_data_failure", True)) + update_property(request, "mock_data_failure", True)
203-214: Prefer contextlib.suppress over try/except passSame behavior, more idiomatic; imports already added above.
- if eval_span is not None and test_case is not None and model is not None: - try: - log_to_braintrust( - eval_span=eval_span, - test_case=test_case, - model=model, - result=result, - error=error, - mock_generation_config=mock_generation_config, - ) - except Exception: - pass # Don't fail the test due to logging issues + if eval_span is not None and test_case is not None and model is not None: + with suppress(Exception): + log_to_braintrust( + eval_span=eval_span, + test_case=test_case, + model=model, + result=result, + error=error, + mock_generation_config=mock_generation_config, + )tests/llm/test_investigate.py (3)
120-126: Ruff SIM117: combine context managers.Minor readability win; fewer indentation levels.
Apply:
- with patch.dict( - os.environ, {"HOLMES_STRUCTURED_OUTPUT_CONVERSION_FEATURE_FLAG": "False"} - ): - with tracer.start_trace( - name=f"{test_case.id}[{model}]", span_type=SpanType.EVAL - ) as eval_span: + with patch.dict( + os.environ, {"HOLMES_STRUCTURED_OUTPUT_CONVERSION_FEATURE_FLAG": "False"} + ), tracer.start_trace( + name=f"{test_case.id}[{model}]", span_type=SpanType.EVAL + ) as eval_span:
201-204: Harden tool_calls extraction.Tool call items may not always have tool_name; fall back to description/str and guard None.
Apply:
- tools_called = [t.tool_name for t in result.tool_calls] + tools_called = [ + getattr(t, "tool_name", getattr(t, "description", str(t))) + for t in (result.tool_calls or []) + ]
111-111: Nit: avoid shadowing built-in name 'input'.This local is also unused; remove it.
Apply:
- input = test_case.investigate_requesttests/llm/test_workload_health.py (6)
118-126: Ruff SIM117: merge nested context managers.Compact the patch + span contexts into a single with.
Apply:
- try: - with patch.multiple("server", dal=mock_dal, config=config): - # Note: Currently workload_health_check does not trace llm calls and the run includes the startup time of the tools - with eval_span.start_span("Holmes Run", type=SpanType.TASK.value): - start_time = time.time() - result = workload_health_check(request=input) - holmes_duration = time.time() - start_time - eval_span.log(metadata={"Holmes Duration": holmes_duration}) + try: + with patch.multiple("server", dal=mock_dal, config=config), \ + eval_span.start_span("Holmes Run", type=SpanType.TASK.value): + start_time = time.time() + result = workload_health_check(request=input) + holmes_duration = time.time() - start_time + eval_span.log(metadata={"holmes_duration": holmes_duration})
125-125: Standardize metadata key to snake_case for parity with investigate tests.Use holmes_duration consistently to simplify downstream aggregation.
Apply:
- eval_span.log(metadata={"Holmes Duration": holmes_duration}) + eval_span.log(metadata={"holmes_duration": holmes_duration})
101-101: Nit: avoid shadowing built-in 'input'; rename to workload_request.Reduces cognitive overhead and potential static analysis noise.
Apply:
- input = test_case.workload_health_request + workload_request = test_case.workload_health_request @@ - result = workload_health_check(request=input) + result = workload_health_check(request=workload_request) @@ - input=input, + input=workload_request,Also applies to: 123-123, 159-159
168-168: Harden tool_calls extraction.Be tolerant to items without tool_name and to None/empty lists.
Apply:
- tools_called = [t.tool_name for t in result.tool_calls] + tools_called = [ + getattr(t, "tool_name", getattr(t, "description", str(t))) + for t in (result.tool_calls or []) + ]
152-166: Optional: centralize Braintrust logging for this test type as well.Currently, log_to_braintrust doesn’t handle HealthCheckTestCase, so you’re logging manually here. Consider extending the helper to support this type for uniformity, or keep manual logging but align field names with investigate tests.
If you’d like, I can draft an update to tests/llm/utils/braintrust.py to support HealthCheckTestCase (input/expected extraction) and then refactor this block to:
from tests.llm.utils.braintrust import log_to_braintrust log_to_braintrust( eval_span=eval_span, test_case=test_case, model=model, result=result, scores=scores, mock_generation_config=mock_generation_config, )
129-151: Optional: reduce noisy print statements in tests.These prints can overwhelm CI logs. Prefer eval_span.log, logging, or gating under a DEBUG flag.
📜 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 (8)
tests/llm/test_ask_holmes.py(7 hunks)tests/llm/test_investigate.py(4 hunks)tests/llm/test_workload_health.py(4 hunks)tests/llm/utils/property_manager.py(4 hunks)tests/llm/utils/reporting/terminal_reporter.py(13 hunks)tests/llm/utils/setup_cleanup.py(1 hunks)tests/llm/utils/test_case_utils.py(2 hunks)tests/llm/utils/test_helpers.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/llm/utils/test_helpers.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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 (project is type-checked with mypy)
Use Ruff for formatting and linting (configured in pyproject.toml)
Files:
tests/llm/utils/setup_cleanup.pytests/llm/utils/test_case_utils.pytests/llm/utils/reporting/terminal_reporter.pytests/llm/utils/property_manager.pytests/llm/test_ask_holmes.pytests/llm/test_investigate.pytests/llm/test_workload_health.py
{tests/**/*.py,pyproject.toml}
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are defined in pyproject.toml; never introduce undefined markers in tests
Files:
tests/llm/utils/setup_cleanup.pytests/llm/utils/test_case_utils.pytests/llm/utils/reporting/terminal_reporter.pytests/llm/utils/property_manager.pytests/llm/test_ask_holmes.pytests/llm/test_investigate.pytests/llm/test_workload_health.py
🧬 Code graph analysis (6)
tests/llm/utils/test_case_utils.py (1)
tests/llm/conftest.py (1)
shared_test_infrastructure(85-198)
tests/llm/utils/reporting/terminal_reporter.py (1)
tests/llm/utils/test_results.py (3)
TestStatus(43-118)passed(56-59)test_case_name(24-40)
tests/llm/utils/property_manager.py (4)
tests/llm/utils/test_case_utils.py (2)
Evaluation(34-36)HolmesTestCase(50-75)tests/llm/utils/classifiers.py (2)
evaluate_correctness(63-173)evaluate_sections(176-257)tests/llm/conftest.py (1)
mock_generation_config(51-79)tests/llm/utils/braintrust.py (1)
log_to_braintrust(173-305)
tests/llm/test_ask_holmes.py (6)
tests/llm/utils/test_case_utils.py (3)
get_models(18-21)AskHolmesTestCase(78-91)check_and_skip_test(110-129)tests/llm/utils/property_manager.py (3)
set_initial_properties(6-51)update_test_results(64-170)handle_test_error(179-237)holmes/core/tracing.py (6)
SpanType(90-98)start_experiment(123-125)start_experiment(148-177)start_trace(127-129)start_trace(179-209)log(107-108)tests/llm/utils/braintrust.py (1)
log_to_braintrust(173-305)holmes/core/tool_calling_llm.py (1)
LLMResult(192-209)holmes/core/llm.py (1)
DefaultLLM(61-268)
tests/llm/test_investigate.py (5)
tests/llm/utils/test_case_utils.py (3)
get_models(18-21)InvestigateTestCase(94-99)check_and_skip_test(110-129)tests/llm/utils/property_manager.py (3)
set_initial_properties(6-51)update_test_results(64-170)handle_test_error(179-237)tests/llm/utils/braintrust.py (1)
log_to_braintrust(173-305)holmes/core/tracing.py (3)
TracingFactory(260-293)create_tracer(264-293)log(107-108)tests/llm/utils/commands.py (1)
set_test_env_vars(152-180)
tests/llm/test_workload_health.py (4)
tests/llm/utils/test_case_utils.py (2)
get_models(18-21)check_and_skip_test(110-129)tests/llm/utils/property_manager.py (3)
set_initial_properties(6-51)update_test_results(64-170)handle_test_error(179-237)holmes/core/tracing.py (7)
start_experiment(123-125)start_experiment(148-177)start_trace(127-129)start_trace(179-209)SpanType(90-98)start_span(104-105)log(107-108)server.py (1)
workload_health_check(186-246)
🪛 Ruff (0.12.2)
tests/llm/utils/reporting/terminal_reporter.py
125-127: Use a single if statement instead of nested if statements
(SIM102)
228-231: Use ternary operator cost_str = f"${cost:.4f}" if cost > 0 else "—" instead of if-else-block
Replace if-else-block with cost_str = f"${cost:.4f}" if cost > 0 else "—"
(SIM108)
449-449: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
486-486: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
tests/llm/utils/property_manager.py
102-105: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
204-214: Use contextlib.suppress(Exception) instead of try-except-pass
(SIM105)
tests/llm/test_investigate.py
120-125: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
tests/llm/test_workload_health.py
119-121: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
⏰ 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). (3)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (7)
tests/llm/utils/reporting/terminal_reporter.py (1)
26-72: Result counters are clean and reuse TestStatus correctlyThe shape and branching cover all categories you render downstream, with a defensive error for unknown types. Looks good.
tests/llm/test_ask_holmes.py (1)
148-156: Braintrust logging on success path is correctly parameterizedLogging includes model and scores; consistent with error path via handle_test_error. LGTM.
tests/llm/test_investigate.py (3)
26-31: LGTM on import changes; complies with project guidelines.Top-level imports look correct and follow the “imports at top” rule. No action needed.
37-37: Good addition: shared Braintrust logging helper is imported.Centralizing logging via a single helper helps consistency and reduces duplication.
82-97: get_models Splitting Logic VerifiedConfirmed that
get_models()is defined to read theMODELSenvironment variable (defaulting to"gpt-4o") and returns a clean list viasplit(","). It’s parametrizing themodelargument correctly across all tests.•
get_models()definition found in tests/llm/utils/test_case_utils.py:18–21
• Usage of@pytest.mark.parametrize("model", get_models())in:
– tests/llm/test_workload_health.py:71
– tests/llm/test_investigate.py:82
– tests/llm/test_ask_holmes.py:51No further action needed—env var parsing is handled cleanly.
tests/llm/test_workload_health.py (2)
23-29: LGTM on import updates and adherence to top-level import rule.Imports are correctly placed; good alignment with coding guidelines.
70-86: Model parameterization + early property/skip setup look good.The flow mirrors investigate tests and keeps behavior consistent.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
docs/development/evals/index.md (4)
67-87: Quote MODEL values in examples for shell robustness.Quoting avoids accidental word-splitting and is friendlier across shells. The section content is solid otherwise.
-RUN_LIVE=true MODEL=gpt-4o,anthropic/claude-3-5-sonnet-20241022,gpt-4o-mini \ +RUN_LIVE=true MODEL="gpt-4o,anthropic/claude-3-5-sonnet-20241022,gpt-4o-mini" \ @@ -RUN_LIVE=true ITERATIONS=10 \ - MODEL=gpt-4o,anthropic/claude-3-5-sonnet-20241022 \ +RUN_LIVE=true ITERATIONS=10 \ + MODEL="gpt-4o,anthropic/claude-3-5-sonnet-20241022" \ @@ -RUN_LIVE=true MODEL=gpt-4o,gpt-4o-mini \ +RUN_LIVE=true MODEL="gpt-4o,gpt-4o-mini" \
89-94: Fix list style to satisfy markdownlint MD004 (asterisks required).The linter expects asterisks, not hyphens. Replace dashes with asterisks in this list.
-When running multi-model benchmarks: -- Results will show a **Model Comparison Table** with side-by-side performance metrics -- Each model's pass rate, execution times, and P90 percentiles are displayed -- Tests are parameterized by model, so you'll see separate results for each model/test combination -- Use `CLASSIFIER_MODEL` to ensure consistent scoring across all models +When running multi-model benchmarks: +* Results will show a **Model Comparison Table** with side-by-side performance metrics +* Each model's pass rate, execution times, and P90 percentiles are displayed +* Tests are parameterized by model, so you'll see separate results for each model/test combination +* Use `CLASSIFIER_MODEL` to ensure consistent scoring across all models
196-212: Unify invocation style: use poetry-run for consistency with earlier examples.Most examples use
poetry run pytest; mirror that here for consistency.-RUN_LIVE=true ITERATIONS=10 EXPERIMENT_ID=baseline_gpt4o MODEL=gpt-4o pytest -n 10 tests/llm/ +RUN_LIVE=true ITERATIONS=10 EXPERIMENT_ID=baseline_gpt4o MODEL=gpt-4o poetry run pytest -n 10 tests/llm/ @@ -RUN_LIVE=true ITERATIONS=10 EXPERIMENT_ID=claude35 MODEL=anthropic/claude-3-5-sonnet-20241022 CLASSIFIER_MODEL=gpt-4o pytest -n 10 tests/llm/ +RUN_LIVE=true ITERATIONS=10 EXPERIMENT_ID=claude35 MODEL=anthropic/claude-3-5-sonnet-20241022 CLASSIFIER_MODEL=gpt-4o poetry run pytest -n 10 tests/llm/ @@ -RUN_LIVE=true ITERATIONS=10 EXPERIMENT_ID=gpt4o_mini MODEL=gpt-4o-mini pytest -n 10 tests/llm/ +RUN_LIVE=true ITERATIONS=10 EXPERIMENT_ID=gpt4o_mini MODEL=gpt-4o-mini poetry run pytest -n 10 tests/llm/
178-194: Document the--log-costsflag for cost comparisonsThe holmes CLI supports a
--log-costsoption to enable LLM cost reporting. Update the docs immediately after the multi-model testing example to surface this flag:• File:
docs/development/evals/index.md
Insert after the existing bash snippet (lines 178–194).+# Note: Cost comparisons require enabling cost logging in the CLI. +# Use the `--log-costs` flag to include cost data in your reports, e.g.: +# poetry run holmes <command> --log-costsThis aligns with the implementation in
holmes/main.py(opt_log_costs →--log-costs) andinit_logging(..., log_costs)inholmes/utils/console/logging.pywhich hooks up the cost logger when enabled.
📜 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 (2)
docs/development/evals/index.md(2 hunks)tests/llm/utils/test_case_utils.py(2 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 (project is type-checked with mypy)
Use Ruff for formatting and linting (configured in pyproject.toml)
Files:
tests/llm/utils/test_case_utils.py
{tests/**/*.py,pyproject.toml}
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are defined in pyproject.toml; never introduce undefined markers in tests
Files:
tests/llm/utils/test_case_utils.py
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
In MkDocs docs, always add a blank line between a header/bold text and a following list
Files:
docs/development/evals/index.md
🧬 Code graph analysis (1)
tests/llm/utils/test_case_utils.py (1)
tests/llm/conftest.py (1)
shared_test_infrastructure(85-198)
🪛 markdownlint-cli2 (0.17.2)
docs/development/evals/index.md
90-90: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
91-91: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
92-92: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
93-93: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
⏰ 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). (3)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (1)
docs/development/evals/index.md (1)
214-225: Braintrust section reads well.The flow and env var guidance are clear and consistent with the rest of the doc.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
holmes/core/tool_calling_llm.py (2)
195-195: Prefer default_factory for tool_calls to eliminate None checks and align with Pydantic best practicesThis avoids repetitive None guards and matches how you already handle instructions with Field(default_factory=list).
class LLMResult(BaseModel): - tool_calls: Optional[List[dict]] = None + tool_calls: List[dict] = Field(default_factory=list) @@ - previous_tool_calls=result.tool_calls or [], + previous_tool_calls=result.tool_calls, @@ - if result.tool_calls is None: - result.tool_calls = [] - result.tool_calls.append(tool_call_result.as_tool_result_response()) + result.tool_calls.append(tool_call_result.as_tool_result_response())Optional: consider messages: List[dict] = Field(default_factory=list) as well, since you always assign a list, but that would be a broader change.
Also applies to: 428-428, 437-440
284-284: Using message_tokens instead of total_tokens fixes the prior accumulator clobbering riskThis addresses the earlier “total_tokens reused” bug.
🧹 Nitpick comments (7)
holmes/core/tool_calling_llm.py (7)
46-48: Attach a NullHandler to the library logger to prevent “No handler could be found” warningsSince this is a library-style logger, add a NullHandler so importing projects without logging config won't see warnings. Keep propagation to root for configured apps.
# Create a named logger for cost tracking cost_logger = logging.getLogger("holmes.costs") +cost_logger.addHandler(logging.NullHandler())
289-294: Minor: align pre-check with reserved output tokens to reduce unnecessary truncationYou later reserve min(maximum_output_token, MAX_OUTPUT_TOKEN_RESERVATION). Consider mirroring that here to avoid truncating when only the unreserved delta pushes you over.
For example:
- Precompute reserved = min(maximum_output_token, MAX_OUTPUT_TOKEN_RESERVATION) for the early check, just like inside truncate_messages_to_fit_context.
309-338: Harden usage extraction (dict or object) and switch to parameterized logging to avoid silent drops and extra formatting workSome providers return usage as an object, not a dict; the current code will hit the except and skip accumulation. Also, use logger formatting args to avoid string formatting cost when debug is disabled.
- try: - cost_value = ( - full_response._hidden_params.get("response_cost", 0) - if hasattr(full_response, "_hidden_params") - else 0 - ) - # Ensure cost is a float - cost = float(cost_value) if cost_value is not None else 0.0 - usage = getattr(full_response, "usage", {}) - if usage: - prompt_toks = usage.get("prompt_tokens", 0) - completion_toks = usage.get("completion_tokens", 0) - total_toks = usage.get("total_tokens", 0) - cost_logger.debug( - f"LLM call cost: ${cost:.6f} | Tokens: {prompt_toks} prompt + {completion_toks} completion = {total_toks} total" - ) - # Accumulate costs - result.total_cost += cost - result.prompt_tokens += prompt_toks - result.completion_tokens += completion_toks - result.total_tokens += total_toks - elif cost > 0: - cost_logger.debug( - f"LLM call cost: ${cost:.6f} | Token usage not available" - ) - result.total_cost += cost - except Exception as e: - logging.debug(f"Could not extract cost information: {e}") + try: + cost_value = ( + full_response._hidden_params.get("response_cost", 0) + if hasattr(full_response, "_hidden_params") + else 0 + ) + cost = float(cost_value) if cost_value is not None else 0.0 + usage = getattr(full_response, "usage", None) + if usage: + if isinstance(usage, dict): + prompt_toks = usage.get("prompt_tokens", 0) + completion_toks = usage.get("completion_tokens", 0) + total_toks = usage.get("total_tokens", 0) + else: + prompt_toks = getattr(usage, "prompt_tokens", 0) + completion_toks = getattr(usage, "completion_tokens", 0) + total_toks = getattr(usage, "total_tokens", 0) + cost_logger.debug( + "LLM call cost: $%.6f | Tokens: %d prompt + %d completion = %d total", + cost, prompt_toks, completion_toks, total_toks + ) + # Accumulate costs + result.total_cost += cost + result.prompt_tokens += prompt_toks + result.completion_tokens += completion_toks + result.total_tokens += total_toks + elif cost > 0: + cost_logger.debug( + "LLM call cost: $%.6f | Token usage not available", + cost + ) + result.total_cost += cost + except Exception as e: + logging.debug("Could not extract cost information: %s", e)
392-407: Include post-processing token usage in totals; return usage along with costPost-processing cost is now added, but token totals remain underreported. Return a usage map and add to the accumulator at the call site.
@@ - post_processed_response, post_processing_cost = ( + post_processed_response, post_processing_cost, pp_usage = ( self._post_processing_call( prompt=user_prompt, investigation=raw_response, user_prompt=post_process_prompt, ) ) - result.total_cost += post_processing_cost + result.total_cost += post_processing_cost + # Accumulate usage from post-processing if available + result.prompt_tokens += pp_usage.get("prompt", 0) + result.completion_tokens += pp_usage.get("completion", 0) + result.total_tokens += pp_usage.get("total", 0) @@ - ) -> tuple[Optional[str], float]: + ) -> tuple[Optional[str], float, dict[str, int]]: @@ - # Log cost information for post-processing - post_processing_cost = 0.0 + # Log cost information for post-processing + post_processing_cost = 0.0 + usage_map: dict[str, int] = {"prompt": 0, "completion": 0, "total": 0} try: - cost_value = ( + cost_value = ( full_response._hidden_params.get("response_cost", 0) if hasattr(full_response, "_hidden_params") else 0 ) - # Ensure cost is a float - post_processing_cost = ( - float(cost_value) if cost_value is not None else 0.0 - ) - if post_processing_cost > 0: - cost_logger.debug( - f"Post-processing LLM cost: ${post_processing_cost:.6f}" - ) - except Exception: - pass # Silent fail for cost logging - - return full_response.choices[0].message.content, post_processing_cost # type: ignore + # Ensure cost is a float + post_processing_cost = float(cost_value) if cost_value is not None else 0.0 + usage = getattr(full_response, "usage", None) + if usage: + if isinstance(usage, dict): + usage_map["prompt"] = usage.get("prompt_tokens", 0) + usage_map["completion"] = usage.get("completion_tokens", 0) + usage_map["total"] = usage.get("total_tokens", 0) + else: + usage_map["prompt"] = getattr(usage, "prompt_tokens", 0) + usage_map["completion"] = getattr(usage, "completion_tokens", 0) + usage_map["total"] = getattr(usage, "total_tokens", 0) + if post_processing_cost > 0: + cost_logger.debug("Post-processing LLM cost: $%.6f", post_processing_cost) + except Exception: + pass # Silent fail for cost logging + + return full_response.choices[0].message.content, post_processing_cost, usage_map # type: ignore except Exception: logging.exception("Failed to run post processing", exc_info=True) - return investigation, 0.0 + return investigation, 0.0, {"prompt": 0, "completion": 0, "total": 0}Also applies to: 584-585, 604-627
409-412: Return fields look good; consider dropping ‘prompt’ to avoid duplicationYou already return messages; storing prompt=json.dumps(messages) duplicates the same content and grows memory/log size. Optional clean-up if not relied upon elsewhere.
676-676: Remove unnecessary type: ignorecount_tokens_for_message returns int; the suppression can be dropped.
- message_tokens = self.llm.count_tokens_for_message(messages) # type: ignore + message_tokens = self.llm.count_tokens_for_message(messages)
700-720: Stream path: support dict/object usage and parameterized logging; optionally expose/accumulate totals if desiredMirror the non-streaming improvements so logs don’t silently omit token info on providers that return usage objects.
- try: - cost_value = ( - full_response._hidden_params.get("response_cost", 0) - if hasattr(full_response, "_hidden_params") - else 0 - ) - # Ensure cost is a float - cost = float(cost_value) if cost_value is not None else 0.0 - usage = getattr(full_response, "usage", {}) - if usage: - cost_logger.debug( - f"LLM iteration cost: ${cost:.6f} | Tokens: {usage.get('prompt_tokens', 0)} prompt + {usage.get('completion_tokens', 0)} completion = {usage.get('total_tokens', 0)} total" - ) - elif cost > 0: - cost_logger.debug( - f"LLM iteration cost: ${cost:.6f} | Token usage not available" - ) - except Exception as e: - logging.debug(f"Could not extract cost information: {e}") + try: + cost_value = ( + full_response._hidden_params.get("response_cost", 0) + if hasattr(full_response, "_hidden_params") + else 0 + ) + cost = float(cost_value) if cost_value is not None else 0.0 + usage = getattr(full_response, "usage", None) + if usage: + if isinstance(usage, dict): + prompt_toks = usage.get("prompt_tokens", 0) + completion_toks = usage.get("completion_tokens", 0) + total_toks = usage.get("total_tokens", 0) + else: + prompt_toks = getattr(usage, "prompt_tokens", 0) + completion_toks = getattr(usage, "completion_tokens", 0) + total_toks = getattr(usage, "total_tokens", 0) + cost_logger.debug( + "LLM iteration cost: $%.6f | Tokens: %d prompt + %d completion = %d total", + cost, prompt_toks, completion_toks, total_toks + ) + elif cost > 0: + cost_logger.debug( + "LLM iteration cost: $%.6f | Token usage not available", + cost + ) + except Exception as e: + logging.debug("Could not extract cost information: %s", e)Question: do you want streaming to accumulate totals across iterations and surface them to the caller (e.g., via a final StreamMessage)? If so, I can sketch a minimal accumulator object passed through the generator lifecycle.
📜 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 (1)
holmes/core/tool_calling_llm.py(14 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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 (project is type-checked with mypy)
Use Ruff for formatting and linting (configured in pyproject.toml)
Files:
holmes/core/tool_calling_llm.py
🧬 Code graph analysis (1)
holmes/core/tool_calling_llm.py (1)
holmes/core/llm.py (6)
count_tokens_for_message(44-45)count_tokens_for_message(174-200)get_context_window_size(36-37)get_context_window_size(157-171)get_maximum_output_token(40-41)get_maximum_output_token(254-268)
⏰ 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). (3)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (3)
holmes/core/tool_calling_llm.py (3)
13-13: Pydantic Field import is correctly addedImporting Field enables safe defaults for mutable fields below. Good move.
266-268: Result accumulator initialization looks goodInitializing a single LLMResult and threading it through the loop keeps aggregation consistent.
681-686: LGTM on streaming truncation pre-checkSame logic as non-streaming path; consistent behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/llm/utils/braintrust.py (1)
70-97: Fix NameError: undefinedinputand stray literal 'f' in log messages
input=inputwill raise NameError; compute a concreteinput_dataper test type.- The strings "Uploading f{...}" and "Updating dataset item f{...}" include an extra literal 'f'.
Apply this diff:
- logging.info(f"Uploading f{len(test_cases)} test cases to braintrust") + logging.info(f"Uploading {len(test_cases)} test cases to braintrust") @@ - logging.info(f"Updating dataset item f{test_case.id}") + logging.info(f"Updating dataset item {test_case.id}") @@ - self.dataset.update( + # Derive input payload for Braintrust dataset rows + input_data = ( + getattr(test_case, "user_prompt", None) + or str(getattr(test_case, "investigate_request", "")) + ) + self.dataset.update( id=test_case.id, - input=input, + input=input_data, expected=test_case.expected_output, metadata={"test_case": test_case.model_dump()}, tags=[], ) @@ - logging.info(f"Creating dataset item f{test_case.id}") + logging.info(f"Creating dataset item {test_case.id}") self.dataset.insert( id=test_case.id, - input=input, + input=input_data, expected=test_case.expected_output, metadata={"test_case": test_case.model_dump()}, tags=[], )tests/llm/utils/reporting/terminal_reporter.py (1)
1189-1192: Fix incorrect count of “tests were skipped entirely”Dividing run-count by max iterations misreports when iteration counts vary. Track a counter of fully-skipped tests instead.
@@ - if total_skip > 0: - console.print( - f"[dim]Note: {total_skip // max(len(test_groups.get(name, [])) for name in test_groups)} tests were skipped entirely[/dim]" - ) + skipped_tests_count = sum( + 1 for results in test_groups.values() if all(r.get("status") == "skipped" for r in results) + ) + if skipped_tests_count > 0: + console.print(f"[dim]Note: {skipped_tests_count} test(s) were skipped entirely[/dim]")tests/llm/test_ask_holmes.py (1)
75-121: Move scoring and Braintrust logging inside the eval span
update_test_resultsmay create spans viaparent_span=eval_span; running it after the context exits risks lost logs. Log and score while the span is active.@@ - try: - with tracer.start_trace( - name=f"{test_case.id}[{model}]", span_type=SpanType.EVAL - ) as eval_span: + try: + with tracer.start_trace( + name=f"{test_case.id}[{model}]", span_type=SpanType.EVAL + ) as eval_span: @@ - with set_test_env_vars(test_case): - result = ask_holmes( + with set_test_env_vars(test_case): + result = ask_holmes( test_case=test_case, model=model, tracer=tracer, eval_span=eval_span, mock_generation_config=mock_generation_config, request=request, ) @@ - with set_test_env_vars(test_case): - result = ask_holmes( + with set_test_env_vars(test_case): + result = ask_holmes( test_case=test_case, model=model, tracer=tracer, eval_span=eval_span, mock_generation_config=mock_generation_config, request=request, ) + + # Score and log while eval_span is alive + assert result is not None, "ask_holmes() returned no result" + output = result.result + scores = update_test_results( + request=request, + output=output, + tools_called=[ + (tc.get("tool_name") or tc.get("description") or "unknown") + for tc in (result.tool_calls or []) + ], + scores=None, # Let it calculate + result=result, + test_case=test_case, + eval_span=eval_span, + caplog=caplog, + ) + log_to_braintrust( + eval_span=eval_span, + test_case=test_case, + model=model, + result=result, + scores=scores, + mock_generation_config=mock_generation_config, + )
♻️ Duplicate comments (6)
tests/llm/utils/braintrust.py (1)
259-264: Handle dict/object tool_calls robustly and include cost/token metadataCurrent code assumes
tool_callsentries are dicts with "tool_name". In practice they can be dicts or objects, and "description" may be present. Also, cost/token fields onresultare useful Braintrust metadata.Apply this diff:
- # Add tool usage metrics if available - if result and getattr(result, "tool_calls", None): - metadata["tool_call_count"] = len(result.tool_calls) - metadata["tools_used"] = list({tc["tool_name"] for tc in result.tool_calls}) - # Note: holmes_duration is logged separately directly to eval_span in ask_holmes() + # Add tool usage metrics if available (handles dicts/objects) + if result and getattr(result, "tool_calls", None): + metadata["tool_call_count"] = len(result.tool_calls) + tools: set[str] = set() + for tc in result.tool_calls: + if isinstance(tc, dict): + tools.add(tc.get("tool_name") or tc.get("description") or "unknown") + else: + tools.add(getattr(tc, "tool_name", None) or getattr(tc, "description", "unknown")) + metadata["tools_used"] = sorted(tools) + + # Optional: include cost/tokens if present on result + for field in ("total_cost", "total_tokens", "prompt_tokens", "completion_tokens"): + val = getattr(result, field, None) + if val is not None: + metadata[field] = val + # Note: holmes_duration is logged separately directly to eval_span in ask_holmes()tests/llm/test_ask_holmes.py (1)
70-72: Use positional args to support both tracer implementationsSome tracers accept
metadata, othersadditional_metadata. Passing positionally avoids incompatibilities.- metadata = {"model": model} - tracer.start_experiment(additional_metadata=metadata) + metadata = {"model": model} + tracer.start_experiment(None, metadata)tests/llm/test_investigate.py (1)
101-103: Use positional args to support both tracer implementationsFor compatibility with both no-op and Braintrust tracers:
- config.model = model - metadata = {"model": model} - tracer.start_experiment(additional_metadata=metadata) + config.model = model + metadata = {"model": model} + tracer.start_experiment(None, metadata)tests/llm/utils/property_manager.py (3)
63-72: Strengthen typings forupdate_test_resultsparametersUse
Sequence[str]andMapping[str, float]to satisfy mypy and reflect intent.-def update_test_results( - request, - output: str, - tools_called: Union[List[str], str], - scores: Optional[Dict[str, Any]] = None, +def update_test_results( + request: Any, + output: str, + tools_called: Union[Sequence[str], str], + scores: Optional[Mapping[str, float]] = None, result: Any = None, test_case: Any = None, eval_span: Any = None, caplog: Any = None, -) -> Dict[str, Any]: +) -> Dict[str, Any]:
189-205: Move import to module top or usecontextlib.suppressfor silent failures
- Follow import-at-top guideline.
- Prefer
contextlib.suppress(Exception)to emptyexcept.Apply this diff:
+from contextlib import suppress +from tests.llm.utils.braintrust import log_to_braintrust # safe import; no circular refs @@ - # Import here to avoid circular dependency - from tests.llm.utils.braintrust import log_to_braintrust - # Log error to Braintrust span if available if eval_span is not None and test_case is not None and model is not None: - try: - log_to_braintrust( - eval_span=eval_span, - test_case=test_case, - model=model, - result=result, - error=error, - mock_generation_config=mock_generation_config, - ) - except Exception: - pass # Don't fail the test due to logging issues + with suppress(Exception): + log_to_braintrust( + eval_span=eval_span, + test_case=test_case, + model=model, + result=result, + error=error, + mock_generation_config=mock_generation_config, + )
147-158: Avoid duplicate user_properties; useupdate_propertyand always record tokensEnsure single source of truth and persist tokens even when cost is 0.
- if hasattr(result, "total_cost"): - request.node.user_properties.append(("cost", result.total_cost)) + if hasattr(result, "total_cost"): + update_property(request, "cost", float(getattr(result, "total_cost", 0.0))) # Always record tokens if present - if hasattr(result, "total_tokens"): - request.node.user_properties.append(("total_tokens", result.total_tokens)) - if hasattr(result, "prompt_tokens"): - request.node.user_properties.append(("prompt_tokens", result.prompt_tokens)) - if hasattr(result, "completion_tokens"): - request.node.user_properties.append( - ("completion_tokens", result.completion_tokens) - ) + if hasattr(result, "total_tokens"): + update_property(request, "total_tokens", int(getattr(result, "total_tokens", 0))) + if hasattr(result, "prompt_tokens"): + update_property(request, "prompt_tokens", int(getattr(result, "prompt_tokens", 0))) + if hasattr(result, "completion_tokens"): + update_property(request, "completion_tokens", int(getattr(result, "completion_tokens", 0)))
🧹 Nitpick comments (14)
tests/llm/utils/braintrust.py (2)
173-181: Type the span parameter and align with import policy
- Add a precise type for
eval_span(Span | DummySpan) to satisfy mypy.- Per project guidelines, move imports to the top-level; avoid importing inside functions.
Apply this diff:
-from typing import Any, List, Optional, Union +from typing import Any, List, Optional, Union +from tests.llm.utils.test_case_utils import AskHolmesTestCase, InvestigateTestCase # type: ignore[attr-defined] @@ -def log_to_braintrust( - eval_span, +def log_to_braintrust( + eval_span: Span | DummySpan, test_case: HolmesTestCase, model: str, @@ -) -> None: +) -> None: @@ - from tests.llm.utils.test_case_utils import AskHolmesTestCase, InvestigateTestCase + # imports moved to module top per guidelines
306-317: Fix docstring parameters for get_braintrust_urlThe docstring mentions
test_suite,test_id,test_namewhich are not parameters anymore. Update to reflect the actual signature for accuracy.- """Generate Braintrust URL for a test. - - Args: - test_suite: Either "ask_holmes" or "investigate" - test_id: Test ID like "01" - test_name: Test name like "how_many_pods" - span_id: Optional span ID for direct linking - root_span_id: Optional root span ID for direct linking - """ + """Generate a Braintrust URL for the current experiment and optional span anchors. + + Args: + span_id: Optional span ID for deep-linking directly to a record + root_span_id: Optional root span ID for scoping the view + """tests/llm/utils/reporting/terminal_reporter.py (6)
166-172: Remove unused grouping for P90 calculation
test_time_groupsis built but never used. Drop the block to reduce noise.- # Group results by test name to calculate P90 - test_time_groups = defaultdict(list) - for result in sorted_results: - test_key = result.get("nodeid", "") - if result.get("execution_time"): - test_time_groups[test_key].append(result.get("execution_time"))
226-231: Use a compact ternary and distinguish 0 cost from missing costShow "$0.0000" when cost is zero and "—" only when cost is absent.
- cost = result.get("cost", 0) - if cost > 0: - cost_str = f"${cost:.4f}" - else: - cost_str = "—" + cost = result.get("cost") + cost_str = f"${cost:.4f}" if cost is not None else "—"
449-461: Nit: iterate dict directly instead of.keys()Ruff SIM118:
for model in times_dictis preferred.- for model in times_dict.keys(): + for model in times_dict:
486-503: Nit: iterate dict directly instead of.keys()Ruff SIM118:
for model in costs_dictis preferred.- for model in costs_dict.keys(): + for model in costs_dict:
132-159: Remove or use_parse_test_nameThis helper is not referenced anymore. Consider deleting it or integrating it where needed.
464-472: Type hint accuracy for costs
_format_colored_coststreats missing/None differently than 0; update the hint toDict[str, Optional[float]]for correctness. No behavior change required.tests/llm/test_ask_holmes.py (1)
138-141: Make tools list extraction resilient to shape differencesTool entries can be dicts with "tool_name" or "description". Use a safe fallback to avoid KeyError.
- tools_called=[tc["description"] for tc in result.tool_calls] - if result.tool_calls - else [], + tools_called=[ + (tc.get("tool_name") or tc.get("description") or "unknown") + for tc in (result.tool_calls or []) + ],tests/llm/test_investigate.py (2)
111-114: Remove unused variable
input = test_case.investigate_requestis unused.- input = test_case.investigate_request
122-127: Combine nested context managers (Ruff SIM117)Condense the nested
withblocks for readability.- with patch.dict( - os.environ, {"HOLMES_STRUCTURED_OUTPUT_CONVERSION_FEATURE_FLAG": "False"} - ): - with tracer.start_trace( - name=f"{test_case.id}[{model}]", span_type=SpanType.EVAL - ) as eval_span: + with patch.dict( + os.environ, {"HOLMES_STRUCTURED_OUTPUT_CONVERSION_FEATURE_FLAG": "False"} + ), tracer.start_trace(name=f"{test_case.id}[{model}]", span_type=SpanType.EVAL) as eval_span:tests/llm/utils/property_manager.py (3)
5-12: Type therequestparameterPer project rules, add a type. If you prefer not to import pytest types here,
Anyis acceptable.-def set_initial_properties(request, test_case: HolmesTestCase, model: str) -> None: +def set_initial_properties(request: Any, test_case: HolmesTestCase, model: str) -> None:
101-106: Collapse nestedhasattrchecks (Ruff SIM102)Slight simplification; no behavior change.
- if hasattr(test_case, "evaluation") and hasattr( - test_case.evaluation, "correctness" - ): - if isinstance(test_case.evaluation.correctness, Evaluation): - evaluation_type = test_case.evaluation.correctness.type + if ( + hasattr(test_case, "evaluation") + and hasattr(test_case.evaluation, "correctness") + and isinstance(test_case.evaluation.correctness, Evaluation) + ): + evaluation_type = test_case.evaluation.correctness.type
163-167: Avoid duplicate mock flagsAlso use
update_propertyhere.def update_mock_error(request, error: Exception) -> None: """Update properties when a mock error occurs.""" update_property(request, "actual", f"Mock data error: {str(error)}") - request.node.user_properties.append(("mock_data_failure", True)) + update_property(request, "mock_data_failure", True)
📜 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 (5)
tests/llm/test_ask_holmes.py(7 hunks)tests/llm/test_investigate.py(3 hunks)tests/llm/utils/braintrust.py(2 hunks)tests/llm/utils/property_manager.py(4 hunks)tests/llm/utils/reporting/terminal_reporter.py(14 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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 (project is type-checked with mypy)
Use Ruff for formatting and linting (configured in pyproject.toml)
Files:
tests/llm/test_ask_holmes.pytests/llm/utils/braintrust.pytests/llm/utils/property_manager.pytests/llm/utils/reporting/terminal_reporter.pytests/llm/test_investigate.py
{tests/**/*.py,pyproject.toml}
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are defined in pyproject.toml; never introduce undefined markers in tests
Files:
tests/llm/test_ask_holmes.pytests/llm/utils/braintrust.pytests/llm/utils/property_manager.pytests/llm/utils/reporting/terminal_reporter.pytests/llm/test_investigate.py
🧬 Code graph analysis (5)
tests/llm/test_ask_holmes.py (6)
tests/llm/utils/test_case_utils.py (3)
get_models(18-21)AskHolmesTestCase(78-91)check_and_skip_test(110-129)tests/llm/utils/property_manager.py (3)
set_initial_properties(5-50)update_test_results(63-160)handle_test_error(169-227)holmes/core/tracing.py (6)
SpanType(90-98)start_experiment(123-125)start_experiment(148-177)start_trace(127-129)start_trace(179-209)log(107-108)tests/llm/utils/braintrust.py (1)
log_to_braintrust(173-299)tests/llm/utils/mock_toolset.py (1)
MockGenerationConfig(77-81)holmes/core/llm.py (1)
DefaultLLM(61-268)
tests/llm/utils/braintrust.py (3)
tests/llm/utils/test_case_utils.py (3)
HolmesTestCase(50-75)AskHolmesTestCase(78-91)InvestigateTestCase(94-99)tests/llm/conftest.py (1)
mock_generation_config(51-79)holmes/core/tracing.py (1)
log(107-108)
tests/llm/utils/property_manager.py (3)
tests/llm/utils/test_case_utils.py (2)
Evaluation(34-36)HolmesTestCase(50-75)tests/llm/utils/classifiers.py (2)
evaluate_correctness(63-173)evaluate_sections(176-257)tests/llm/utils/braintrust.py (1)
log_to_braintrust(173-299)
tests/llm/utils/reporting/terminal_reporter.py (1)
tests/llm/utils/test_results.py (3)
TestStatus(43-118)passed(56-59)test_case_name(24-40)
tests/llm/test_investigate.py (4)
tests/llm/utils/test_case_utils.py (3)
get_models(18-21)InvestigateTestCase(94-99)check_and_skip_test(110-129)tests/llm/utils/property_manager.py (3)
set_initial_properties(5-50)update_test_results(63-160)handle_test_error(169-227)tests/llm/utils/braintrust.py (1)
log_to_braintrust(173-299)holmes/core/investigation.py (1)
investigate_issues(23-75)
🪛 Ruff (0.12.2)
tests/llm/utils/property_manager.py
101-104: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
194-204: Use contextlib.suppress(Exception) instead of try-except-pass
(SIM105)
tests/llm/utils/reporting/terminal_reporter.py
125-127: Use a single if statement instead of nested if statements
(SIM102)
228-231: Use ternary operator cost_str = f"${cost:.4f}" if cost > 0 else "—" instead of if-else-block
Replace if-else-block with cost_str = f"${cost:.4f}" if cost > 0 else "—"
(SIM108)
449-449: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
486-486: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
tests/llm/test_investigate.py
122-127: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
⏰ 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). (3)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: Pre-commit checks
🔇 Additional comments (1)
tests/llm/utils/reporting/terminal_reporter.py (1)
1135-1152: Totals Pass% now aligns with per-test semantics — nice fixUsing VALID_RUNS for totals keeps consistency with row calculations. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/llm/utils/braintrust.py (1)
81-97: Bug: dataset update/insert passes built-ininputinstead of the test input.
input=inputrefers to the Python built-in instead of the test case input, which will upload a function object to Braintrust. Use the test case’s actual input (Ask: user_prompt; Investigate: investigate_request), coerced to a string when needed.Apply this diff:
- self.dataset.update( - id=test_case.id, - input=input, - expected=test_case.expected_output, - metadata={"test_case": test_case.model_dump()}, - tags=[], - ) + # Resolve test input per test type + input_data = ( + test_case.user_prompt + if hasattr(test_case, "user_prompt") + else str(getattr(test_case, "investigate_request", "")) + ) + self.dataset.update( + id=test_case.id, + input=input_data, + expected=test_case.expected_output, + metadata={"test_case": test_case.model_dump()}, + tags=[], + ) @@ - self.dataset.insert( - id=test_case.id, - input=input, - expected=test_case.expected_output, - metadata={"test_case": test_case.model_dump()}, - tags=[], - ) + input_data = ( + test_case.user_prompt + if hasattr(test_case, "user_prompt") + else str(getattr(test_case, "investigate_request", "")) + ) + self.dataset.insert( + id=test_case.id, + input=input_data, + expected=test_case.expected_output, + metadata={"test_case": test_case.model_dump()}, + tags=[], + )tests/llm/test_ask_holmes.py (2)
1-16: Missing import:jsonis used in ask_holmes but not imported.
json.dumps(...)will raise NameError. Add a top-level import.Apply this diff:
import os import time +import json from typing import Optional
76-121: Log and scoring occur after the eval_span context exits—move inside the span.You call update_test_results(parent_span=eval_span) and log_to_braintrust with eval_span after the context manager ends, risking no-op logs. Compute scores, update properties, and log while the span is active.
Apply this diff to compute and log within the eval span (and remove the duplicated block below):
with tracer.start_trace( name=f"{test_case.id}[{model}]", span_type=SpanType.EVAL ) as eval_span: @@ with set_test_env_vars(test_case): result = ask_holmes( test_case=test_case, model=model, tracer=tracer, eval_span=eval_span, mock_generation_config=mock_generation_config, request=request, ) else: with set_test_env_vars(test_case): result = ask_holmes( test_case=test_case, model=model, tracer=tracer, eval_span=eval_span, mock_generation_config=mock_generation_config, request=request, ) + + # Compute scores and log while eval_span is active + output = result.result if result and result.result else "" + tools_called = ( + [tc.description for tc in (result.tool_calls or [])] + if result + else [] + ) + scores = update_test_results( + request=request, + output=output, + tools_called=tools_called, + scores=None, # Let it calculate + result=result, + test_case=test_case, + eval_span=eval_span, + caplog=caplog, + ) + log_to_braintrust( + eval_span=eval_span, + test_case=test_case, + model=model, + result=result, + scores=scores, + mock_generation_config=mock_generation_config, + )And delete the now-redundant block below (lines 133–156):
- output = result.result - - scores = update_test_results( - request=request, - output=output, - tools_called=[tc.description for tc in result.tool_calls] - if result.tool_calls - else [], - scores=None, # Let it calculate - result=result, - test_case=test_case, - eval_span=eval_span, - caplog=caplog, - ) - - if eval_span: - log_to_braintrust( - eval_span=eval_span, - test_case=test_case, - model=model, - result=result, - scores=scores, - mock_generation_config=mock_generation_config, - )
♻️ Duplicate comments (3)
tests/llm/utils/braintrust.py (1)
259-264: Handle dict/object tool_calls and include cost/token metadata.
tc.tool_nameassumes objects; elsewhere tool calls can be dicts. Also, adding cost/tokens to metadata improves Braintrust comparison.Apply this diff:
- if result and getattr(result, "tool_calls", None): - metadata["tool_call_count"] = len(result.tool_calls) - metadata["tools_used"] = list({tc.tool_name for tc in result.tool_calls}) - # Note: holmes_duration is logged separately directly to eval_span in ask_holmes() + if result and getattr(result, "tool_calls", None): + metadata["tool_call_count"] = len(result.tool_calls) + tools: set[str] = set() + for tc in result.tool_calls: + if isinstance(tc, dict): + tools.add(tc.get("tool_name") or tc.get("description") or "unknown") + else: + tools.add( + getattr(tc, "tool_name", None) + or getattr(tc, "description", "unknown") + ) + metadata["tools_used"] = sorted(tools) + # Optional: include cost/tokens if present + for field in ("total_cost", "total_tokens", "prompt_tokens", "completion_tokens"): + val = getattr(result, field, None) + if val is not None: + metadata[field] = val + # Note: holmes_duration is logged separately directly to eval_span in ask_holmes()holmes/core/tool_calling_llm.py (2)
233-235: Remove unusedsizefield from ToolCallResult.
sizeis unused throughout the repo and adds confusion.Apply this diff:
- size: Optional[int] = ( - None # TODO: currently unused - remove it? need to verify this doesn't break clients - )
111-130: Accumulate costs robustly across dict/objectusage.Same shape issue; also keep the log consistent.
Apply this diff:
- usage = getattr(full_response, "usage", {}) + usage = getattr(full_response, "usage", None) @@ - if usage: - prompt_toks = usage.get("prompt_tokens", 0) - completion_toks = usage.get("completion_tokens", 0) - total_toks = usage.get("total_tokens", 0) + if usage: + if isinstance(usage, dict): + prompt_toks = usage.get("prompt_tokens", 0) + completion_toks = usage.get("completion_tokens", 0) + total_toks = usage.get("total_tokens", 0) + else: + prompt_toks = getattr(usage, "prompt_tokens", 0) + completion_toks = getattr(usage, "completion_tokens", 0) + total_toks = getattr(usage, "total_tokens", 0) cost_logger.debug( f"{log_prefix} cost: ${cost:.6f} | Tokens: {prompt_toks} prompt + {completion_toks} completion = {total_toks} total" )
🧹 Nitpick comments (5)
tests/llm/utils/braintrust.py (2)
70-91: Fix f-string typos in log messages.Strings like "Uploading f{len(test_cases)}" and "Updating dataset item f{test_case.id}" will literally render the letter "f". Remove the stray "f" from inside the braces.
Apply this diff:
- logging.info(f"Uploading f{len(test_cases)} test cases to braintrust") + logging.info(f"Uploading {len(test_cases)} test cases to braintrust") @@ - logging.info(f"Updating dataset item f{test_case.id}") + logging.info(f"Updating dataset item {test_case.id}") @@ - for test_case in test_cases: - logging.info(f"Creating dataset item f{test_case.id}") + for test_case in test_cases: + logging.info(f"Creating dataset item {test_case.id}")
302-336: Move urllib import to module top per project guidelines.
from urllib.parse import quoteshould be at the top; keep function body import-free.Apply this diff:
- from urllib.parse import quote - experiment_name = get_experiment_name()And at the file top (near other imports):
from braintrust import Dataset, Experiment, ReadonlyExperiment, Span import logging +from urllib.parse import quote import ostests/llm/test_ask_holmes.py (1)
139-145: Be robust to tool_calls shape.If a future change switches tool_calls to dicts, accessing
.descriptionwill fail. Prefer a safer extraction.Apply this diff:
- tools_called=[tc.description for tc in result.tool_calls] - if result.tool_calls - else [], + tools_called=[ + getattr(tc, "description", None) + or getattr(tc, "tool_name", None) + or str(tc) + ] + if result and result.tool_calls + else [],tests/llm/test_investigate.py (1)
122-147: Combine with-statements where practical (Ruff SIM117).Minor readability win: merge the first two nested with statements.
Example:
- with patch.dict( - os.environ, {"HOLMES_STRUCTURED_OUTPUT_CONVERSION_FEATURE_FLAG": "False"} - ): - with tracer.start_trace( - name=f"{test_case.id}[{model}]", span_type=SpanType.EVAL - ) as eval_span: + with patch.dict( + os.environ, {"HOLMES_STRUCTURED_OUTPUT_CONVERSION_FEATURE_FLAG": "False"} + ), tracer.start_trace( + name=f"{test_case.id}[{model}]", span_type=SpanType.EVAL + ) as eval_span:Keep the inner eval_span.start_span blocks as-is for clear sub-tasks.
holmes/core/tool_calling_llm.py (1)
447-461: Also track token usage during post-processing.You add post-processing cost to totals but not the token counts, leading to under-reported token metrics.
Apply this diff:
- post_processed_response, post_processing_cost = ( + post_processed_response, post_processing_cost, pp_usage = ( self._post_processing_call( prompt=user_prompt, investigation=raw_response, user_prompt=post_process_prompt, ) ) result.total_cost += post_processing_cost + # Accumulate tokens from post-processing if available + if pp_usage: + result.prompt_tokens += pp_usage.get("prompt", 0) + result.completion_tokens += pp_usage.get("completion", 0) + result.total_tokens += pp_usage.get("total", 0)And update
_post_processing_callto return usage:- ) -> tuple[Optional[str], float]: + ) -> tuple[Optional[str], float, dict[str, int]]: @@ - full_response = self.llm.completion(messages=messages, temperature=0) + full_response = self.llm.completion(messages=messages, temperature=0) @@ - post_processing_cost = _extract_cost_from_response(full_response) - if post_processing_cost > 0: + post_processing_cost = _extract_cost_from_response(full_response) + # Extract usage if present + usage = getattr(full_response, "usage", None) + if isinstance(usage, dict): + prompt_toks = usage.get("prompt_tokens", 0) + completion_toks = usage.get("completion_tokens", 0) + total_toks = usage.get("total_tokens", 0) + else: + prompt_toks = getattr(usage, "prompt_tokens", 0) if usage else 0 + completion_toks = getattr(usage, "completion_tokens", 0) if usage else 0 + total_toks = getattr(usage, "total_tokens", 0) if usage else 0 + if post_processing_cost > 0: cost_logger.debug( f"Post-processing LLM cost: ${post_processing_cost:.6f}" ) - - return full_response.choices[0].message.content, post_processing_cost # type: ignore + return full_response.choices[0].message.content, post_processing_cost, { # type: ignore + "prompt": prompt_toks, + "completion": completion_toks, + "total": total_toks, + } @@ - return investigation, 0.0 + return investigation, 0.0, {"prompt": 0, "completion": 0, "total": 0}
📜 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 (4)
holmes/core/tool_calling_llm.py(18 hunks)tests/llm/test_ask_holmes.py(7 hunks)tests/llm/test_investigate.py(3 hunks)tests/llm/utils/braintrust.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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 (project is type-checked with mypy)
Use Ruff for formatting and linting (configured in pyproject.toml)
Files:
tests/llm/utils/braintrust.pytests/llm/test_ask_holmes.pyholmes/core/tool_calling_llm.pytests/llm/test_investigate.py
{tests/**/*.py,pyproject.toml}
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are defined in pyproject.toml; never introduce undefined markers in tests
Files:
tests/llm/utils/braintrust.pytests/llm/test_ask_holmes.pytests/llm/test_investigate.py
🧬 Code graph analysis (4)
tests/llm/utils/braintrust.py (3)
tests/llm/utils/test_case_utils.py (3)
HolmesTestCase(50-75)AskHolmesTestCase(78-91)InvestigateTestCase(94-99)tests/llm/conftest.py (1)
mock_generation_config(51-79)holmes/core/tracing.py (1)
log(107-108)
tests/llm/test_ask_holmes.py (6)
tests/llm/utils/test_case_utils.py (3)
get_models(18-21)AskHolmesTestCase(78-91)check_and_skip_test(110-129)tests/llm/utils/property_manager.py (3)
set_initial_properties(5-50)update_test_results(63-160)handle_test_error(169-227)holmes/core/tracing.py (6)
SpanType(90-98)start_experiment(123-125)start_experiment(148-177)start_trace(127-129)start_trace(179-209)log(107-108)tests/llm/utils/braintrust.py (1)
log_to_braintrust(173-299)holmes/core/tool_calling_llm.py (1)
LLMResult(276-288)holmes/core/llm.py (1)
DefaultLLM(61-268)
holmes/core/tool_calling_llm.py (2)
holmes/core/llm.py (6)
count_tokens_for_message(44-45)count_tokens_for_message(174-200)get_context_window_size(36-37)get_context_window_size(157-171)get_maximum_output_token(40-41)get_maximum_output_token(254-268)holmes/core/safeguards.py (1)
prevent_overly_repeated_tool_call(78-120)
tests/llm/test_investigate.py (5)
tests/llm/utils/test_case_utils.py (3)
get_models(18-21)InvestigateTestCase(94-99)check_and_skip_test(110-129)tests/llm/utils/property_manager.py (3)
set_initial_properties(5-50)update_test_results(63-160)handle_test_error(169-227)tests/llm/utils/braintrust.py (1)
log_to_braintrust(173-299)holmes/core/tracing.py (5)
TracingFactory(260-293)create_tracer(264-293)SpanType(90-98)start_span(104-105)log(107-108)holmes/core/investigation.py (1)
investigate_issues(23-75)
🪛 Ruff (0.12.2)
tests/llm/test_investigate.py
122-127: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
⏰ 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). (3)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: Pre-commit checks
🔇 Additional comments (2)
tests/llm/utils/braintrust.py (1)
222-249: Use duck-typing for Ask-vs-Investigate branching and keep imports at top.Replace isinstance checks with attribute checks. Also, prefer "system_prompt" naming clarity when deriving the prompt.
Apply this diff:
- if isinstance(test_case, AskHolmesTestCase): + if hasattr(test_case, "user_prompt"): if ( result and hasattr(result, "messages") and result.messages and len(result.messages) > 0 ): prompt = result.messages[0]["content"] elif result and hasattr(result, "prompt"): prompt = result.prompt @@ - if isinstance(test_case, AskHolmesTestCase): + if hasattr(test_case, "user_prompt"): metadata["test_type"] = ( test_case.test_type or os.environ.get("ASK_HOLMES_TEST_TYPE", "cli").lower() ) @@ - if isinstance(test_case, AskHolmesTestCase): + if hasattr(test_case, "user_prompt"): input_data = test_case.user_prompt expected = ( test_case.expected_output if isinstance(test_case.expected_output, str) else str(test_case.expected_output) ) - elif isinstance(test_case, InvestigateTestCase): + elif hasattr(test_case, "investigate_request"): input_data = str(test_case.investigate_request) expected = str(test_case.expected_output) else: input_data = "" expected = ""Likely an incorrect or invalid review comment.
tests/llm/test_investigate.py (1)
121-191: Good fix: scoring and Braintrust logging moved inside the active span.Keeping evaluation and logging within the eval_span context avoids no-op logs and aligns with tracing best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/core/tool_calling_llm.py (1)
164-166: Fix negative allocation bug; use tokens for tool messages; tighten typesWhen message_size_without_tools ∈ (max_context_size - maximum_output_token, max_context_size - reserved_for_output_tokens], available_space becomes negative, producing negative allocations and brittle slicing. Also, we’re still using character counts despite having a token counter.
- Ensure available_space ≥ 0 and guard the zero-space case.
- Compute needed_space by tokens (fallback to len(content)).
- Make allocation non-negative.
- Tighten type hints for messages and count_tokens_fn (Callable[[list[dict]], int]).
- Logging should reflect “~tokens” (approximate) when mixed units are possible.
-from typing import Dict, List, Optional, Type, Union +from typing import Dict, List, Optional, Type, Union, Callable @@ -def truncate_messages_to_fit_context( - messages: list, max_context_size: int, maximum_output_token: int, count_tokens_fn -) -> list: +def truncate_messages_to_fit_context( + messages: list[dict], + max_context_size: int, + maximum_output_token: int, + count_tokens_fn: Callable[[list[dict]], int], +) -> list[dict]: @@ - available_space = ( - max_context_size - message_size_without_tools - maximum_output_token - ) - remaining_space = available_space - tool_call_messages.sort(key=lambda x: len(x["content"])) + available_space = max( + 0, max_context_size - message_size_without_tools - maximum_output_token + ) + if available_space == 0: + # No space left for tool messages; strip their content entirely. + for msg in tool_call_messages: + msg["content"] = "" + msg.pop("token_count", None) + return messages + + remaining_space = available_space + # Prefer token sizing; fallback to char length + for msg in tool_call_messages: + if "token_count" not in msg or not isinstance(msg["token_count"], int): + msg["token_count"] = count_tokens_fn([msg]) + tool_call_messages.sort( + key=lambda x: x.get("token_count", len(x.get("content", ""))) + ) @@ - max_allocation = remaining_space // remaining_tools - needed_space = len(msg["content"]) - allocated_space = min(needed_space, max_allocation) + max_allocation = max(0, remaining_space // max(1, remaining_tools)) + needed_space = msg.get("token_count", len(msg.get("content", ""))) + allocated_space = max(0, min(needed_space, max_allocation)) @@ - truncation_notice = "\n\n[TRUNCATED]" + truncation_notice = "\n\n[TRUNCATED]" @@ - if allocated_space > len(truncation_notice): + if allocated_space > len(truncation_notice): msg["content"] = ( msg["content"][: allocated_space - len(truncation_notice)] + truncation_notice ) logging.info( - f"Truncating tool message '{msg['name']}' from {needed_space} to {allocated_space-len(truncation_notice)} tokens" + f"Truncating tool message '{msg.get('name','<unnamed>')}' from ~{needed_space} to ~{max(0, allocated_space - len(truncation_notice))} tokens" ) else: - msg["content"] = truncation_notice[:allocated_space] + msg["content"] = truncation_notice[:allocated_space] logging.info( - f"Truncating tool message '{msg['name']}' from {needed_space} to {allocated_space} tokens" + f"Truncating tool message '{msg.get('name','<unnamed>')}' from ~{needed_space} to ~{allocated_space} tokens" ) - msg.pop("token_count", None) # Remove token_count if present + msg.pop("token_count", None) # Force recount after truncation @@ - return messages + return messagesAlso applies to: 181-186, 188-235
♻️ Duplicate comments (4)
holmes/core/tool_calling_llm.py (4)
242-244: Remove unused size field from ToolCallResultThis field is unused and was previously flagged; dropping it simplifies the schema and avoids misleading clients.
class ToolCallResult(BaseModel): @@ - size: Optional[int] = ( - None # TODO: currently unused - remove it? need to verify this doesn't break clients - )If you’re worried about external consumers, we can deprecate first via model_config = {"ser_json_timedelta": "..."} or by documenting the removal in the changelog.
80-104: Handle usage as dict or object to avoid lost logging and incomplete metricsusage may be a mapping or an object depending on the client. Current code assumes dict and will drop logging silently on AttributeError (caught by try/except). This also affects totals downstream.
- usage = getattr(full_response, "usage", {}) - - if usage: - prompt_toks = usage.get("prompt_tokens", 0) - completion_toks = usage.get("completion_tokens", 0) - total_toks = usage.get("total_tokens", 0) - cost_logger.debug( - f"{log_prefix} cost: ${cost:.6f} | Tokens: {prompt_toks} prompt + {completion_toks} completion = {total_toks} total" - ) + usage = getattr(full_response, "usage", None) + + if usage: + if isinstance(usage, dict): + prompt_toks = usage.get("prompt_tokens", 0) + completion_toks = usage.get("completion_tokens", 0) + total_toks = usage.get("total_tokens", 0) + else: + prompt_toks = getattr(usage, "prompt_tokens", 0) + completion_toks = getattr(usage, "completion_tokens", 0) + total_toks = getattr(usage, "total_tokens", 0) + cost_logger.debug( + "%s cost: $%.6f | Tokens: %d prompt + %d completion = %d total", + log_prefix, cost, prompt_toks, completion_toks, total_toks + ) elif cost > 0: - cost_logger.debug( - f"{log_prefix} cost: ${cost:.6f} | Token usage not available" - ) + cost_logger.debug("%s cost: $%.6f | Token usage not available", log_prefix, cost)
106-139: Accumulate tokens regardless of usage shape; prefer logger args over f-stringsSame shape issue as above, and we should ensure totals are updated even when usage is an object. Also avoid f-strings in debug for performance and consistency.
- cost = _extract_cost_from_response(full_response) - usage = getattr(full_response, "usage", {}) + cost = _extract_cost_from_response(full_response) + usage = getattr(full_response, "usage", None) @@ - if usage: - prompt_toks = usage.get("prompt_tokens", 0) - completion_toks = usage.get("completion_tokens", 0) - total_toks = usage.get("total_tokens", 0) - cost_logger.debug( - f"{log_prefix} cost: ${cost:.6f} | Tokens: {prompt_toks} prompt + {completion_toks} completion = {total_toks} total" - ) + if usage: + if isinstance(usage, dict): + prompt_toks = usage.get("prompt_tokens", 0) + completion_toks = usage.get("completion_tokens", 0) + total_toks = usage.get("total_tokens", 0) + else: + prompt_toks = getattr(usage, "prompt_tokens", 0) + completion_toks = getattr(usage, "completion_tokens", 0) + total_toks = getattr(usage, "total_tokens", 0) + cost_logger.debug( + "%s cost: $%.6f | Tokens: %d prompt + %d completion = %d total", + log_prefix, cost, prompt_toks, completion_toks, total_toks + ) # Accumulate costs and tokens costs.total_cost += cost costs.prompt_tokens += prompt_toks costs.completion_tokens += completion_toks costs.total_tokens += total_toks elif cost > 0: - cost_logger.debug( - f"{log_prefix} cost: ${cost:.6f} | Token usage not available" - ) + cost_logger.debug("%s cost: $%.6f | Token usage not available", log_prefix, cost) costs.total_cost += cost
285-294: Reconcile tool_calls typing with actual values to remove type: ignore and satisfy mypycall() builds tool_calls as list[dict] (from as_tool_result_response()), but LLMResult.tool_calls expects List[ToolCallResult]. You’re compensating with type: ignore at return sites. Prefer aligning the type to the actual payload shape or, alternatively, accumulate both typed and dict forms.
Option A (minimal, matches current usage): change LLMResult.tool_calls to List[dict].
-class LLMResult(LLMCosts): - tool_calls: Optional[List[ToolCallResult]] = None +class LLMResult(LLMCosts): + tool_calls: Optional[List[dict]] = NoneThen, remove the type: ignore on the return sites (see lines 466–479 comments below).
Option B (more type-safe): keep LLMResult.tool_calls as List[ToolCallResult], but in call() maintain two lists:
- prev_tool_calls: list[dict] for duplicate-prevention and messages.
- tool_call_results: list[ToolCallResult] for returning in LLMResult.
I can draft the Option B refactor if you prefer stronger typing.
🧹 Nitpick comments (5)
holmes/core/tool_calling_llm.py (5)
46-48: Harden the logger with a NullHandler to avoid "No handler found" warningsAdding a NullHandler prevents warnings when this module is imported in apps that don't configure logging.
cost_logger = logging.getLogger("holmes.costs") +if not cost_logger.handlers: + cost_logger.addHandler(logging.NullHandler())
59-78: Relying on _hidden_params.response_cost is brittle across providersUsing a private attribute risks silent zeros when providers or SDKs change their internals. Consider a fallback path that derives cost from usage tokens and model pricing when response_cost is missing.
- Minimal: also check for a public attribute (e.g., getattr(full_response, "response_cost", None)).
- Robust: when cost is absent but usage exists, compute cost from token counts using your model price table (litellm.model_cost) and the active model name.
Would you like a patch that computes a fallback cost from usage with litellm?
353-356: Naming: distinguish prev_tool_calls (dicts) from result_tool_calls (models)To reduce confusion and avoid type: ignore later, consider:
- prev_tool_calls: list[dict] (used for safeguards and message assembly)
- result_tool_calls: list[ToolCallResult] (returned on LLMResult)
This aligns runtime needs with types and keeps mypy happy.
453-461: Include token totals for post-processing to avoid undercountingYou add post-processing cost but not tokens. For consistency with other paths and accurate totals, also add prompt/completion/total tokens from the post-processing call.
Option A (return usage counts):
- ) -> tuple[Optional[str], float]: + ) -> tuple[Optional[str], float, dict[str, int]]: @@ - # Extract and log cost information for post-processing - post_processing_cost = _extract_cost_from_response(full_response) + # Extract and log cost information for post-processing + post_processing_cost = _extract_cost_from_response(full_response) + usage = getattr(full_response, "usage", None) + if usage and not isinstance(usage, dict): + usage = { + "prompt_tokens": getattr(usage, "prompt_tokens", 0), + "completion_tokens": getattr(usage, "completion_tokens", 0), + "total_tokens": getattr(usage, "total_tokens", 0), + } @@ - return full_response.choices[0].message.content, post_processing_cost # type: ignore + return full_response.choices[0].message.content, post_processing_cost, (usage or {"prompt_tokens":0,"completion_tokens":0,"total_tokens":0}) # type: ignore @@ - return investigation, 0.0 + return investigation, 0.0, {"prompt_tokens":0,"completion_tokens":0,"total_tokens":0}And at the call site:
- post_processed_response, post_processing_cost = ( + post_processed_response, post_processing_cost, pp_usage = ( self._post_processing_call( @@ - costs.total_cost += post_processing_cost + costs.total_cost += post_processing_cost + costs.prompt_tokens += pp_usage.get("prompt_tokens", 0) + costs.completion_tokens += pp_usage.get("completion_tokens", 0) + costs.total_tokens += pp_usage.get("total_tokens", 0)Option B (mutate accumulator inside _post_processing_call): pass costs into the method and call _extract_and_update_costs there. Slightly cleaner surface; I can draft this if preferred.
Also applies to: 669-676, 679-680
466-471: Remove type: ignore after aligning LLMResult.tool_calls typingOnce LLMResult.tool_calls is List[dict] (Option A) or you return typed ToolCallResult objects (Option B), these suppressions can be dropped.
- tool_calls=tool_calls, # type: ignore # Pydantic converts dicts to ToolCallResult + tool_calls=tool_calls, @@ - tool_calls=tool_calls, # type: ignore # Pydantic converts dicts to ToolCallResult + tool_calls=tool_calls,Also applies to: 474-479
📜 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 (1)
holmes/core/tool_calling_llm.py(12 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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 (project is type-checked with mypy)
Use Ruff for formatting and linting (configured in pyproject.toml)
Files:
holmes/core/tool_calling_llm.py
🧬 Code graph analysis (1)
holmes/core/tool_calling_llm.py (2)
holmes/core/llm.py (6)
count_tokens_for_message(44-45)count_tokens_for_message(174-200)get_context_window_size(36-37)get_context_window_size(157-171)get_maximum_output_token(40-41)get_maximum_output_token(254-268)examples/custom_llm.py (3)
count_tokens_for_message(19-20)get_context_window_size(13-14)get_maximum_output_token(16-17)
⏰ 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). (3)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (6)
holmes/core/tool_calling_llm.py (6)
13-13: Import placement and usage look goodImporting Field from pydantic at top-of-file complies with project guidelines.
50-57: LLMCosts model is a clean abstractionGood separation of concerns for accumulating cost and token usage.
372-383: Context fit check looks goodToken counting and truncation hook are used correctly prior to the LLM call.
397-399: Good: centralized cost accumulation per LLM callUsing _extract_and_update_costs here ensures totals are in sync with logs (post-fix for usage shape).
729-735: Streaming path: context checks mirrored correctlyToken-limit handling mirrors the non-streaming path. No issues spotted.
753-755: Good: per-iteration cost logging without affecting totalsLogging cost info here is useful; after the usage-shape fix, this will be robust across clients.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/core/tool_calling_llm.py (1)
261-269: Use ToolCallResult objects fortool_callsinstead of raw dictsThe root issue is that we serialize each
ToolCallResultto a dict before constructing theLLMResult, yet the model’s field remainsOptional[List[ToolCallResult]]. Downstream consumers (e.g.get_tool_usage_summary, the Slack plugin, console renderers) expect realToolCallResultinstances with.description,.result, and helper methods like.get_stringified_data(), not plain dicts.• In holmes/core/tool_calling_llm.py, update both code paths where tool calls are accumulated (lines ~481–483 and ~807–809) to append the model object rather than its dict representation.
• Keep the field declarationtool_calls: Optional[List[ToolCallResult]] = Noneunchanged so Pydantic continues to validate and serialize nested models.
• This preserves all methods and attributes onToolCallResultand prevents attribute‐access errors inget_tool_usage_summaryand other consumers.Proposed diff (non-streaming path around line 481):
for future in futures: tool_call_result: ToolCallResult = future.result() - tool_calls.append(tool_call_result.as_tool_result_response()) + tool_calls.append(tool_call_result) messages.append(tool_call_result.as_tool_call_message())And in the streaming path (around line 807):
for future in futures: tool_call_result: ToolCallResult = future.result() - tool_calls.append(tool_call_result.as_tool_result_response()) + tool_calls.append(tool_call_result) messages.append(tool_call_result.as_tool_call_message())With these changes,
LLMResult.tool_callsremains a list of fullToolCallResultobjects; all existing consumers andget_tool_usage_summarywill continue to work without modification.
♻️ Duplicate comments (2)
holmes/core/tool_calling_llm.py (2)
220-221: Remove unusedsizefield from ToolCallResultThis field is still unused and was previously flagged. Removing it reduces confusion and schema churn.
class ToolCallResult(BaseModel): @@ - size: Optional[int] = NoneIf you keep it for backward compatibility, document when it’s set and by whom.
80-117: Handle both dict and object shapes forusageand use parameterized loggingAs previously suggested, usage may be an object; calling .get can break. Also switch f-strings to parameterized logging.
-def _process_cost_info( - full_response, costs: Optional[LLMCosts] = None, log_prefix: str = "LLM call" -) -> None: +def _process_cost_info( + full_response: Any, costs: Optional[LLMCosts] = None, log_prefix: str = "LLM call" +) -> None: @@ - usage = getattr(full_response, "usage", {}) - - if usage: - prompt_toks = usage.get("prompt_tokens", 0) - completion_toks = usage.get("completion_tokens", 0) - total_toks = usage.get("total_tokens", 0) - cost_logger.debug( - f"{log_prefix} cost: ${cost:.6f} | Tokens: {prompt_toks} prompt + {completion_toks} completion = {total_toks} total" - ) + usage = getattr(full_response, "usage", None) + + if usage: + if isinstance(usage, dict): + prompt_toks = usage.get("prompt_tokens", 0) + completion_toks = usage.get("completion_tokens", 0) + total_toks = usage.get("total_tokens", 0) + else: + prompt_toks = getattr(usage, "prompt_tokens", 0) + completion_toks = getattr(usage, "completion_tokens", 0) + total_toks = getattr(usage, "total_tokens", 0) + cost_logger.debug( + "%s cost: $%.6f | Tokens: %d prompt + %d completion = %d total", + log_prefix, cost, prompt_toks, completion_toks, total_toks + ) @@ - elif cost > 0: - cost_logger.debug( - f"{log_prefix} cost: ${cost:.6f} | Token usage not available" - ) + elif cost > 0: + cost_logger.debug("%s cost: $%.6f | Token usage not available", log_prefix, cost) @@ - except Exception as e: - logging.debug(f"Could not extract cost information: {e}") + except Exception: + cost_logger.debug("Could not extract cost information", exc_info=True)
🧹 Nitpick comments (4)
holmes/core/tool_calling_llm.py (4)
46-48: Prefer a hierarchical module logger and child for costsMinor consistency improvement: create a module logger and a costs child to keep log hierarchy tidy and allow selective filtering.
+logger = logging.getLogger(__name__) -# Create a named logger for cost tracking -cost_logger = logging.getLogger("holmes.costs") +# Create a child logger for cost tracking +cost_logger = logger.getChild("costs")
59-77: Harden cost extraction and add type hints
- Use a type for full_response to satisfy mypy.
- Check both a public attribute and the private _hidden_params safely.
- Log with parameterized style and include exc_info for debugging.
-from typing import Dict, List, Optional, Type, Union +from typing import Any, Dict, List, Optional, Type, Union @@ -def _extract_cost_from_response(full_response) -> float: +def _extract_cost_from_response(full_response: Any) -> float: @@ - try: - cost_value = ( - full_response._hidden_params.get("response_cost", 0) - if hasattr(full_response, "_hidden_params") - else 0 - ) - # Ensure cost is a float - return float(cost_value) if cost_value is not None else 0.0 - except Exception: - return 0.0 + try: + cost_value = 0 + # Prefer a direct attribute if present (some clients may expose it) + if hasattr(full_response, "response_cost"): + cost_value = getattr(full_response, "response_cost", 0) + elif hasattr(full_response, "_hidden_params"): + hidden = getattr(full_response, "_hidden_params", {}) or {} + if isinstance(hidden, dict): + cost_value = hidden.get("response_cost", 0) + return float(cost_value or 0) + except Exception: + cost_logger.debug("Could not extract response cost", exc_info=True) + return 0.0Note: relying on private attributes is brittle across providers. If there’s a stable surface we can key on, prefer that. Happy to adjust to match your client(s).
Would you like me to align this to the exact client(s) you use (OpenAI/Azure Bedrock/litellm), so we don’t depend on private fields?
248-258: Add return type hints for dict-returning helpersThese helpers return dicts; add explicit return types for mypy compliance.
- def as_tool_call_message(self): + def as_tool_call_message(self) -> dict: @@ - def as_tool_result_response(self): + def as_tool_result_response(self) -> dict: @@ - def as_streaming_tool_result_response(self): + def as_streaming_tool_result_response(self) -> dict:Also applies to: 236-247, 222-235
363-364: Avoid logging full message payloads at debug in productionThe debug log includes the entire messages payload and tools, which may expose sensitive data. Consider gating behind a verbose flag or redacting.
- logging.debug(f"sending messages={messages}\n\ntools={tools}") + logging.debug("sending messages payload and tools (redacted)") + # Or: logging.debug("sending messages=%s | tools=%s", redact(messages), redact(tools))If you prefer, I can add a redact() helper that strips tool params and long contents.
📜 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 (1)
holmes/core/tool_calling_llm.py(11 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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 (project is type-checked with mypy)
Use Ruff for formatting and linting (configured in pyproject.toml)
Files:
holmes/core/tool_calling_llm.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). (3)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: Pre-commit checks
🔇 Additional comments (3)
holmes/core/tool_calling_llm.py (3)
13-13: Importing Field for default_factory is correctBrings Pydantic’s Field into scope to avoid mutable defaults elsewhere. No issues.
50-57: LLMCosts model looks goodClear fields and defaults. Works well with model_dump() for spreading into LLMResult.
731-734: Good: cost logging for streaming iterationsStreaming path logs per-iteration cost without accumulating, which matches the function’s contract. No action needed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
tests/llm/test_workload_health.py (1)
23-29: Importedget_models: ensure it trims/validates and uses a consistent env var nameThis file depends on
tests/llm/utils/test_case_utils.get_models. Per past feedback, that function lacks type hints and does not trim/filter empty entries. Also, there’s inconsistency across comments (“MODELS”) vs. implementation (“MODEL”). Standardize the env var (recommendMODELS) and return a typed, cleaned list.Run to confirm current signature and env var usage:
#!/bin/bash set -euo pipefail echo "get_models() definition:" rg -n -C2 $'^def\\s+get_models\\s*\\(' tests/llm/utils/test_case_utils.py echo echo "References to MODEL(S) env var in test utils:" rg -nP 'os\\.environ\\.get\\(["\\\']MODEL' tests/llm/utils/test_case_utils.py -n -C2 || true rg -nP 'os\\.environ\\.get\\(["\\\']MODELS' tests/llm/utils/test_case_utils.py -n -C2 || truetests/llm/test_investigate.py (3)
158-190: Good: scoring and Braintrust logging now happen inside the active eval_span.Addresses the earlier bug about logging to a closed span and ensures parent_span is valid for evaluation.
203-204: Make tools_called extraction robust to variant tool call shapes.Some implementations expose description or str(t) instead of tool_name.
- tools_called = [t.tool_name for t in result.tool_calls] if result.tool_calls else [] + tools_called = [ + getattr(t, "tool_name", getattr(t, "description", str(t))) + for t in (result.tool_calls or []) + ]
100-103: Fix tracer.start_experiment to support both Braintrust and Dummy tracers.Using the keyword additional_metadata breaks when a no-op tracer is returned (it expects metadata as the second positional argument). Use positional args.
- metadata = {"model": model} - tracer.start_experiment(additional_metadata=metadata) + metadata = {"model": model} + # Use positional args: (experiment_name=None, metadata) + tracer.start_experiment(None, metadata)
🧹 Nitpick comments (16)
tests/llm/test_workload_health.py (9)
1-1: Remove blanket type ignore; declaremodelonMockConfigto satisfy mypyA file-level
# type: ignoresuppresses all type checking and violates the project guideline that tests are type-checked. Also, assigningconfig.model = modelwithout declaring the attribute will trigger mypy errors once the blanket ignore is removed.
- Drop the blanket ignore.
- Add a typed
modelattribute toMockConfig.Apply:
-# type: ignore @@ class MockConfig(Config): + # Declared so mypy recognizes dynamic assignment in tests + model: Optional[str] = NoneAlso applies to: 92-93
101-103: Avoid shadowing built-ininputUsing
inputas a variable name shadows the built-in and hurts readability and tooling. Rename to something likewh_request.- input = test_case.workload_health_request + wh_request = test_case.workload_health_request @@ - result = workload_health_check(request=input) + result = workload_health_check(request=wh_request) @@ - input=input, + input=wh_request,Also applies to: 121-124, 159-159
104-107: Definescoresonce to satisfy type-checkers and avoid shadowingInitialize
scoresbefore the eval block and remove the inner reassignment. This avoids potential “may be unbound” warnings once the file-level ignore is removed.- result = None + result = None + scores: dict[str, float] = {} @@ - scores = {} - scores["correctness"] = correctness_eval.score + scores["correctness"] = correctness_eval.scoreAlso applies to: 149-151
118-126: Combine context managers and use a monotonic clock
- Ruff SIM117: prefer a single with-statement for multiple contexts.
- Use
time.perf_counter()for duration measurement.- with patch.multiple("server", dal=mock_dal, config=config): - # Note: Currently workload_health_check does not trace llm calls and the run includes the startup time of the tools - with eval_span.start_span("Holmes Run", type=SpanType.TASK.value): - start_time = time.time() - result = workload_health_check(request=input) - holmes_duration = time.time() - start_time + with patch.multiple("server", dal=mock_dal, config=config), \ + eval_span.start_span("Holmes Run", type=SpanType.TASK.value): + # Note: Currently workload_health_check does not trace llm calls and the run includes the startup time of the tools + start_time = time.perf_counter() + result = workload_health_check(request=wh_request) + holmes_duration = time.perf_counter() - start_time
127-128: Strengthen assertion messageInclude test id and model to make failures easier to triage across multi-model runs.
- assert result, "No result returned by workload_health_check()" + assert result, f"[{test_case.id}][{model}] workload_health_check() returned no result"
129-137: Chatty prints: consider logging or gating for noise reductionRepeated prints add noise when running large model matrices. Consider:
- using
caplogto emit at INFO/DEBUG, or- gating prints behind an env/flag (e.g., only when
-sorHOLMES_VERBOSE=1).
153-166: Log the joined expected string instead ofstr(expected)You already compute
debug_expected(joined list). Log that for cleaner Braintrust records.- expected=str(expected), + expected=debug_expected,
190-195: Threshold assertion reads well; minor enhancement optionalAs an optional improvement, include the actual score and expected threshold in the assertion message for quicker diagnosis in CI.
71-75: Add stable IDs to parametrized tests;llmmarker is already defined
- The custom pytest marker
llmis present under[tool.pytest.ini_options]in pyproject.toml (line 96–104), so no changes are needed for marker registration.- To improve test-node names when running multiple models, add
idsto themodelparametrization:-@pytest.mark.parametrize("model", get_models()) +@pytest.mark.parametrize("model", get_models(), ids=lambda m: f"model={m}")Optionally, you can apply the same pattern to
test_casefor consistency:@pytest.mark.parametrize( "test_case", get_workload_health_test_cases(), ids=lambda tc: f"case={tc.name}" )tests/llm/test_investigate.py (7)
85-96: Normalize model strings to avoid env-var whitespace pitfalls.MODEL="gpt-4o, gpt-4o-mini" will pass a value with a leading space for the second model. Strip it once at the start.
def test_investigate( model: str, test_case: InvestigateTestCase, caplog, request, mock_generation_config, shared_test_infrastructure, # type: ignore ): - # Set initial properties early so they're available even if test fails + # Normalize model (handles comma-separated env var with spaces) + model = model.strip() + # Set initial properties early so they're available even if test fails set_initial_properties(request, test_case, model)
111-116: Remove unused variable that shadows built-in input().input is unused and shadows Python’s built-in. Drop it.
- input = test_case.investigate_request - expected = test_case.expected_output + expected = test_case.expected_output result = None output = None scores = {}
117-120: Avoid mutating the shared test_case.investigate_request across parameterizations.Deep-copy before modifying sections so runs across different models don’t bleed state.
- investigate_request = test_case.investigate_request + investigate_request = deepcopy(test_case.investigate_request) if not investigate_request.sections: investigate_request.sections = DEFAULT_SECTIONSAdd this import at the top of the file:
+from copy import deepcopyIf test_case objects are re-instantiated per param, this is harmless; if reused, this prevents hidden coupling. Please confirm the lifecycle.
121-129: Combine nested with statements (Ruff SIM117).Flatten patch.dict + start_trace into a single with for clarity and to satisfy the linter.
- with patch.dict( - os.environ, {"HOLMES_STRUCTURED_OUTPUT_CONVERSION_FEATURE_FLAG": "False"} - ): - with tracer.start_trace( - name=f"{test_case.id}[{model}]", span_type=SpanType.EVAL - ) as eval_span: + with patch.dict( + os.environ, {"HOLMES_STRUCTURED_OUTPUT_CONVERSION_FEATURE_FLAG": "False"} + ), tracer.start_trace( + name=f"{test_case.id}[{model}]", span_type=SpanType.EVAL + ) as eval_span:
147-156: Use time.perf_counter() for duration measurements.Higher resolution and monotonic; better for timing.
- start_time = time.time() + start_time = time.perf_counter() result = investigate_issues( investigate_request=investigate_request, config=config, dal=mock_dal, trace_span=holmes_span, ) - holmes_duration = time.time() - start_time + holmes_duration = time.perf_counter() - start_time
163-169: Honor test-specific evaluation type instead of hardcoding "strict".Use the evaluation type from the test case when provided to avoid false negatives/positives.
- correctness_eval = evaluate_correctness( + # Resolve evaluation type from test case when available + evaluation_type = "strict" + if hasattr(test_case, "evaluation") and hasattr( + test_case.evaluation, "correctness" + ) and hasattr(test_case.evaluation.correctness, "type"): + evaluation_type = test_case.evaluation.correctness.type + + correctness_eval = evaluate_correctness( output=output, expected_elements=expected, parent_span=eval_span, caplog=caplog, - evaluation_type="strict", + evaluation_type=evaluation_type, )
1-1: Avoid file-wide type checking disable.The file-level “# type: ignore” defeats mypy on tests. Prefer fixing specific spots or using narrow ignores (e.g., type: ignore[arg-type]) where needed.
📜 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 (2)
tests/llm/test_investigate.py(3 hunks)tests/llm/test_workload_health.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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 (project is type-checked with mypy)
Use Ruff for formatting and linting (configured in pyproject.toml)
Files:
tests/llm/test_investigate.pytests/llm/test_workload_health.py
{tests/**/*.py,pyproject.toml}
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are defined in pyproject.toml; never introduce undefined markers in tests
Files:
tests/llm/test_investigate.pytests/llm/test_workload_health.py
🧬 Code graph analysis (2)
tests/llm/test_investigate.py (6)
tests/llm/utils/test_case_utils.py (3)
get_models(18-21)InvestigateTestCase(94-99)check_and_skip_test(110-129)tests/llm/utils/property_manager.py (3)
set_initial_properties(5-50)update_test_results(63-160)handle_test_error(169-227)tests/llm/utils/braintrust.py (1)
log_to_braintrust(173-299)holmes/core/tracing.py (5)
TracingFactory(260-293)create_tracer(264-293)SpanType(90-98)start_span(104-105)log(107-108)tests/llm/utils/commands.py (1)
set_test_env_vars(152-180)holmes/core/investigation.py (1)
investigate_issues(23-75)
tests/llm/test_workload_health.py (5)
tests/llm/utils/test_case_utils.py (2)
get_models(18-21)check_and_skip_test(110-129)tests/llm/utils/property_manager.py (3)
set_initial_properties(5-50)update_test_results(63-160)handle_test_error(169-227)tests/llm/conftest.py (2)
mock_generation_config(51-79)shared_test_infrastructure(85-198)holmes/core/tracing.py (7)
start_experiment(123-125)start_experiment(148-177)start_trace(127-129)start_trace(179-209)SpanType(90-98)start_span(104-105)log(107-108)server.py (1)
workload_health_check(186-246)
🪛 Ruff (0.12.2)
tests/llm/test_investigate.py
122-127: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
tests/llm/test_workload_health.py
119-121: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
⏰ 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). (3)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: Pre-commit checks
🔇 Additional comments (11)
tests/llm/test_workload_health.py (6)
82-86: Early property capture and skip checks are in the right placeCapturing properties before heavy setup and performing skip/setup-failure checks up-front improves debuggability and speeds up collection. LGTM.
139-151: Correctness evaluation flow looks goodYou compute correctness once here and pass
scorestoupdate_test_results, avoiding double evaluation. LGTM.
168-170: LGTM: Safe extraction of tool namesHandles
Nonegracefully and normalizes to a list of tool names for reporting.
175-177: Good:update_test_resultsreceivesresultto record cost/tokensThis aligns with the PR’s goal to expose cost info in reports.
178-188: Centralized error handling integration is solidPassing
model,result, andmock_generation_configenables richer logging and consistent reporting on failure paths. LGTM.
88-90: Verifiedadditional_metadataparameter in start_experiment signature
Thestart_experimentmethod in both the no-opDummyTracer(line 123) and the real tracer implementation (line 148) explicitly accepts anadditional_metadataargument. The test’s use oftracer.start_experiment(additional_metadata=metadata)aligns perfectly with the API.tests/llm/test_investigate.py (5)
26-32: Top-level imports for property management look good.Brings in set_initial_properties, update_test_results, and handle_test_error at module scope, complying with the “imports at top” guideline.
37-38: Good: centralized Braintrust logging via log_to_braintrust.Keeps reporting concerns out of the test body and reduces duplication.
81-86: Model parameterization wired correctly.Parameterizing by model and adding model: str to the test signature is consistent with multi-model comparison.
93-97: Pre-flight properties + skip checks are in the right place.Properties are set before potential early exits, and skip/setup checks use the updated signature. Good.
81-83: Pytest markerllmis declared – Thepyproject.tomlunder[tool.pytest.ini_options]includesmarkers = [ "llm: Evaluate LLM behaviour (prompt, tools, etc.)", … ]so the
@pytest.mark.llmusage intests/llm/test_investigate.pyis already covered. No changes needed.
2 participants
This PR includes 3 main changes: