Improvements to evals framework - #702
Conversation
|
Warning Rate limit exceeded@aantn has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis update introduces a new top-level Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Pytest
participant conftest.py
participant ReportingModules
participant TestModules
User->>Pytest: Run pytest with CLI flags (e.g., --generate-mocks)
Pytest->>conftest.py: Parse CLI options, configure logging
Pytest->>TestModules: Execute tests (setup/teardown via fixtures)
TestModules->>ReportingModules: Update test properties/results
Pytest->>ReportingModules: On terminal summary, call show_llm_summary_report
ReportingModules->>ReportingModules: Aggregate results, generate reports
ReportingModules->>User: Output summary (terminal, GitHub, mock report)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 4
🧹 Nitpick comments (12)
conftest.py (1)
35-69: Consider removing or documenting commented code.The large block of commented code for pytest-xdist worker-specific logging should either be removed if no longer needed or documented with a clear explanation of why it's preserved for future use.
- # Configure worker-specific log files for xdist compatibility - # worker_id = getattr(config, "workerinput", {}).get("workerid", "master") - # if worker_id != "master": - # # Set worker-specific log file to avoid conflicts - # config.option.log_file = f"tests-{worker_id}.log" - - # Determine worker id - # Also see: https://pytest-xdist.readthedocs.io/en/latest/how-to.html#creating-one-log-file-for-each-worker - # worker_id = os.environ.get("PYTEST_XDIST_WORKER", default="gw0") - - # # Create logs folder - # logs_folder = os.environ.get("LOGS_FOLDER", default="logs_folder") - # os.makedirs(logs_folder, exist_ok=True) - - # # Create file handler to output logs into corresponding worker file - # file_handler = logging.FileHandler(f"{logs_folder}/logs_worker_{worker_id}.log", mode="w") - # file_handler.setFormatter( - # logging.Formatter( - # fmt="{asctime} {levelname}:{name}:{lineno}:{message}", - # style="{", - # ) - # ) - # # Create stream handler to output logs on console - # # This is a workaround for a known limitation: - # # https://pytest-xdist.readthedocs.io/en/latest/known-limitations.html - # console_handler = logging.StreamHandler(sys.stderr) # pytest only prints error logs - # console_handler.setFormatter( - # logging.Formatter( - # # Include worker id in log messages, \r is needed to separate lines in console - # fmt="\r{asctime} " + worker_id + ":{levelname}:{name}:{lineno}:{message}", - # style="{", - # ) - # ) - # # Configure logging - # logging.basicConfig(level=logging.INFO, force=True, handlers=[console_handler, file_handler]) + # TODO: Worker-specific logging for pytest-xdist can be implemented here if needed + # See: https://pytest-xdist.readthedocs.io/en/latest/how-to.html#creating-one-log-file-for-each-workertests/llm/utils/test_results.py (3)
22-32: Consider edge cases in test ID extraction.The current logic assumes test cases follow the pattern
test_name[number_description]and extracts the number prefix. However, if the test case doesn't contain underscores or has a different format, it might not work as expected.Consider adding more robust parsing:
@property def test_id(self) -> str: """Extract test ID from pytest nodeid. Example: 'test_ask_holmes[01_how_many_pods]' -> '01' """ if "[" in self.nodeid and "]" in self.nodeid: test_case = self.nodeid.split("[")[1].split("]")[0] - # Extract number from start of test case name - return test_case.split("_")[0] if "_" in test_case else test_case + # Extract number from start of test case name + parts = test_case.split("_") + if parts and parts[0].isdigit(): + return parts[0] + return test_case return "unknown"
66-72: Simplify the regression check logic.The static analysis tool correctly identifies that this can be simplified by returning the negated condition directly.
@property def is_regression(self) -> bool: if self.passed or self.is_mock_failure: return False # Known failure (expected to fail) - if self.actual_score == 0 and self.expected_score == 0: - return False - return True + return not (self.actual_score == 0 and self.expected_score == 0)
54-63: Consider the TODO comment about mock failures.There's a TODO comment suggesting that
mock_failuresshould potentially affect thepassedproperty. This might impact the overall test reporting logic.Should mock failures be considered as non-passing tests? The current logic treats them separately, but the TODO suggests they might need to be integrated into the pass/fail determination. Would you like me to help clarify this logic or open an issue to track this decision?
tests/llm/utils/property_manager.py (1)
36-43: Fix unused loop variable.The static analysis tool correctly identifies that
prop_valueis not used in the loop body.- for i, (prop_key, prop_value) in enumerate(request.node.user_properties): + for i, (prop_key, _prop_value) in enumerate(request.node.user_properties): if prop_key == key: request.node.user_properties[i] = (key, value) returntests/llm/utils/test_helpers.py (1)
52-61: Consider edge case in correctness evaluation printing.The function assumes
correctness_eval.metadataexists and contains a "rationale" key. Consider adding defensive checks.def print_correctness_evaluation(correctness_eval: Any) -> None: """Print correctness evaluation results.""" print("\n⚖️ CORRECTNESS EVALUATION:") print(f" Score: {correctness_eval.score}") print(" Rationale: ") - rationale = correctness_eval.metadata.get("rationale", "") + rationale = getattr(correctness_eval, 'metadata', {}).get("rationale", "") for line in rationale.split("\n"): if line.strip(): print(f" {line}")tests/llm/reporting/terminal_reporter.py (1)
110-157: Consider reliability and cost implications of LLM analysis.The LLM-powered failure analysis is innovative, but there are several considerations:
- Network dependency: Analysis will fail if the LLM API is unavailable
- Cost implications: Running GPT-4o analysis for every failed test could be expensive
- Rate limiting: Multiple concurrent requests might hit API limits
Consider adding configuration options to control when analysis runs:
# Add environment variable check import os def _get_llm_analysis(result: TestResult) -> str: if not os.getenv("ENABLE_LLM_ANALYSIS", "false").lower() == "true": return "LLM analysis disabled (set ENABLE_LLM_ANALYSIS=true to enable)" # Existing implementation...Would you like me to help implement caching for analysis results or add configuration options to control when LLM analysis is performed?
tests/llm/utils/commands.py (1)
17-20: Consider using Optional[int] for exit_code parameterThe
exit_codeparameter defaults toNonebut is typed asint. This could lead to type checking issues.- exit_code: int = None, + exit_code: Optional[int] = None,tests/llm/reporting/github_reporter.py (2)
18-19: Use context manager for file operationsWhile the current implementation works, using a context manager is more Pythonic and ensures proper file closure even if an exception occurs during writing.
- with open("evals_report.txt", "w", encoding="utf-8") as file: - file.write(markdown) + Path("evals_report.txt").write_text(markdown, encoding="utf-8")Note: You'll need to import
Pathfrompathlibat the top of the file:from pathlib import Path
32-40: Consider using a more structured approach for tracking test countsThe current variable initialization with multiple assignments on single lines can be hard to read and maintain. Consider using a dictionary or dataclass to organize these counts.
- ask_holmes_total = ask_holmes_passed = ask_holmes_regressions = ( - ask_holmes_mock_failures - ) = 0 - investigate_total = investigate_passed = investigate_regressions = ( - investigate_mock_failures - ) = 0 - workload_health_total = workload_health_passed = workload_health_regressions = ( - workload_health_mock_failures - ) = 0 + test_counts = { + "ask": {"total": 0, "passed": 0, "regressions": 0, "mock_failures": 0}, + "investigate": {"total": 0, "passed": 0, "regressions": 0, "mock_failures": 0}, + "workload_health": {"total": 0, "passed": 0, "regressions": 0, "mock_failures": 0}, + }Then update the counting logic to use the dictionary structure. This would make the code more maintainable and easier to extend.
tests/llm/utils/mock_toolset.py (1)
723-734: Consider using contextlib.suppress for cleaner exception handlingThe nested try-except blocks could be simplified, though the current implementation with fallback behavior might be intentional for robustness.
def _safe_print(terminalreporter, message: str = "") -> None: """Safely print to terminal reporter to avoid I/O errors""" - try: - terminalreporter.write_line(message) - except Exception: - # If write_line fails, try direct write - try: - terminalreporter._tw.write(message + "\n") - except Exception: - # Last resort - ignore if all writing fails - pass + with contextlib.suppress(Exception): + terminalreporter.write_line(message) + return + + # If write_line failed, try direct write as fallback + with contextlib.suppress(Exception): + terminalreporter._tw.write(message + "\n")Note: You'll need to import
contextlibat the top of the file.tests/llm/conftest.py (1)
323-323: Remove personal commentThe comment "NATAN - this link is correct" appears to be a personal note that should be removed.
- # NATAN - this link is correct braintrust_url = get_braintrust_url(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
conftest.py(1 hunks)pyproject.toml(2 hunks)tests/llm/conftest.py(7 hunks)tests/llm/reporting/__init__.py(1 hunks)tests/llm/reporting/github_reporter.py(1 hunks)tests/llm/reporting/property_manager.py(1 hunks)tests/llm/reporting/terminal_reporter.py(1 hunks)tests/llm/test_ask_holmes.py(10 hunks)tests/llm/test_investigate.py(3 hunks)tests/llm/test_workload_health.py(3 hunks)tests/llm/utils/braintrust.py(2 hunks)tests/llm/utils/commands.py(2 hunks)tests/llm/utils/langfuse.py(0 hunks)tests/llm/utils/mock_toolset.py(7 hunks)tests/llm/utils/property_manager.py(1 hunks)tests/llm/utils/setup_cleanup.py(1 hunks)tests/llm/utils/system.py(1 hunks)tests/llm/utils/test_case_utils.py(2 hunks)tests/llm/utils/test_helpers.py(1 hunks)tests/llm/utils/test_mock_toolset.py(1 hunks)tests/llm/utils/test_results.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/llm/utils/langfuse.py
🧰 Additional context used
🧠 Learnings (2)
tests/llm/test_ask_holmes.py (2)
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
Learnt from: Sheeproid
PR: #586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
tests/llm/utils/mock_toolset.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
🧬 Code Graph Analysis (5)
tests/llm/utils/test_mock_toolset.py (1)
tests/llm/utils/mock_toolset.py (1)
clear_mocks_for_test(265-293)
tests/llm/test_workload_health.py (1)
tests/llm/utils/property_manager.py (2)
set_initial_properties(7-33)update_test_results(46-56)
tests/llm/test_investigate.py (2)
tests/llm/utils/test_case_utils.py (2)
InvestigateTestCase(64-68)MockHelper(78-155)tests/llm/utils/property_manager.py (2)
set_initial_properties(7-33)update_test_results(46-56)
tests/llm/utils/property_manager.py (1)
tests/llm/utils/test_case_utils.py (2)
Evaluation(27-29)HolmesTestCase(43-56)
tests/llm/utils/braintrust.py (1)
tests/llm/utils/test_results.py (2)
test_id(23-32)test_name(35-48)
🪛 Ruff (0.12.2)
tests/llm/utils/test_results.py
70-72: Return the negated condition directly
Inline condition
(SIM103)
tests/llm/utils/property_manager.py
38-38: Loop control variable prop_value not used within loop body
Rename unused prop_value to _prop_value
(B007)
tests/llm/utils/mock_toolset.py
729-733: Use contextlib.suppress(Exception) instead of try-except-pass
(SIM105)
🔇 Additional comments (33)
tests/llm/reporting/__init__.py (1)
1-1: LGTM - Clean package initialization.The docstring clearly describes the purpose of this new reporting package namespace.
tests/llm/utils/test_mock_toolset.py (1)
206-206: LGTM - Consistent with method rename.The test correctly uses the renamed
clear_mocks_for_test()method, which better clarifies that it clears mocks for a single test case folder rather than all mocks globally.tests/llm/utils/test_case_utils.py (1)
99-99: LGTM - Appropriate logging level adjustment.Changing test case loading messages from
infotodebuglevel reduces log noise during normal test runs while maintaining visibility when debug logging is enabled. This aligns well with the broader test infrastructure improvements.Also applies to: 145-145, 147-149, 153-153
tests/llm/utils/system.py (1)
10-35: LGTM - Robust Git worktree support.The enhanced
get_active_branch_name()function properly handles both standard Git directories and Git worktree setups. The implementation correctly:
- Detects when
.gitis a file (worktree case) vs directory- Parses the
gitdir:format to find the actual Git directory- Falls back gracefully with proper error handling
- Uses modern pathlib for better path operations
This will improve branch detection reliability across different Git configurations.
tests/llm/test_investigate.py (3)
24-25: LGTM - Clean integration of property management utilities.The new imports bring in the standardized property management utilities that replace manual property handling throughout the test.
113-114: LGTM - Early property initialization.Calling
set_initial_properties()early ensures test metadata is available even if the test fails before completion, improving debugging and reporting capabilities.
214-215: LGTM - Consolidated result handling.The
update_test_results()call replaces the previous manual appending of multiple user properties, centralizing test result data management and improving maintainability.tests/llm/test_workload_health.py (3)
26-26: Good integration with new property management utilities.The import of property management utilities aligns well with the framework refactoring goals to centralize and standardize test metadata handling.
105-106: Excellent defensive programming approach.Setting initial properties early ensures test metadata is available even if the test fails prematurely, which improves debugging and reporting reliability.
178-179: Clean consolidation of property updates.Replacing manual property appends with the
update_test_resultsutility function reduces code duplication and improves maintainability across the test suite.conftest.py (2)
5-30: Well-designed CLI options support the framework improvements.The new pytest options (
--generate-mocks,--regenerate-all-mocks,--skip-setup,--skip-cleanup) directly support the PR objectives of enabling faster iteration on evals by avoiding repeated initialization or cleanup steps.
71-84: Good logging noise reduction.Suppressing verbose logs from LiteLLM, httpx, and related libraries will significantly improve test output readability and focus attention on relevant test information.
pyproject.toml (3)
50-50: Good addition for session-scoped fixture support.The
pytest-shared-session-scopedependency directly supports the framework improvements for coordinated setup and cleanup operations mentioned in the PR objectives.
107-107: Improved test performance visibility.Increasing the number of slowest tests reported from 5 to 10 will provide better insights into test performance, which is valuable when optimizing the evals framework.
110-120: Comprehensive logging configuration enhances debugging.The detailed pytest logging configuration provides good visibility into test execution while maintaining reasonable log levels (INFO instead of DEBUG) to avoid excessive noise.
tests/llm/utils/braintrust.py (1)
178-213: Well-designed URL generation utility.The
get_braintrust_urlfunction provides a clean, focused approach to Braintrust integration by generating URLs for test linking rather than fetching experiment data. The implementation correctly handles optional parameters, validates API key availability, and provides clear documentation.tests/llm/reporting/property_manager.py (3)
6-21: Clean encapsulation of property management.The
TestPropertyManagerclass provides a well-structured approach to managing test metadata, with proper defensive programming (checking foruser_propertiesattribute) and clear method organization.
22-41: Excellent standardization of test result properties.The
add_test_resultmethod consolidates common test metadata patterns into a single, well-documented interface. This will significantly improve consistency across the test suite.
65-75: Creative pytest fixture registration approach.While the
pytest_plugin()function approach for fixture registration is less common than usingconftest.py, it's a valid pattern that keeps the fixture close to its implementation and maintains good encapsulation.tests/llm/utils/property_manager.py (1)
14-18: Good handling of different correctness evaluation types.The code properly handles both
Evaluationobjects and direct values for the correctness score, which provides good flexibility for different test configurations.tests/llm/utils/test_helpers.py (2)
18-19: Good backward compatibility approach.The backward compatibility alias ensures existing code continues to work while providing a more descriptive function name for new usage.
63-77: Robust error handling in span logging.The function properly handles cases where tool results might not have a
dataattribute and falls back to string representation. Good defensive programming.tests/llm/utils/setup_cleanup.py (3)
44-49: Good resource management with ThreadPoolExecutor.The code properly limits the number of workers to avoid overwhelming the system while ensuring efficient parallel execution.
80-85: Excellent error visibility with warnings.Using
warnings.warn()ensures that timeout and error information is visible in pytest output, which is crucial for debugging test infrastructure issues.
136-156: Efficient deduplication logic.The function properly handles deduplication of test cases based on ID while filtering for cases that need setup. The logic is clear and efficient.
tests/llm/reporting/terminal_reporter.py (2)
126-146: Well-structured prompt engineering.The prompt provides comprehensive context including test details, expected vs actual output, tools called, and error messages. The categorization system helps users understand different types of failures.
17-33: Good table design with fixed widths.The table columns are appropriately sized for terminal display, and the use of Rich styling makes the output readable and professional.
tests/llm/utils/commands.py (2)
61-133: Well-structured command execution with comprehensive error handlingThe refactored
_run_commandsfunction effectively consolidates the command execution logic with proper error handling for different failure scenarios. The use ofCommandResultto encapsulate outcomes provides a clean interface for callers.
135-148: Clean refactoring of setup/cleanup functionsThe simplified
before_testandafter_testfunctions effectively delegate to the shared_run_commandsimplementation, reducing code duplication while maintaining a clear interface.tests/llm/test_ask_holmes.py (2)
85-86: Good practice: Early property initializationSetting initial properties early in the test ensures that metadata is available even if the test fails during setup. This improves observability and debugging capabilities.
166-167: Excellent integration with centralized infrastructureThe use of
log_tool_calls_to_spanshelper function provides consistent tool call logging across all tests, improving traceability in Braintrust.tests/llm/utils/mock_toolset.py (1)
76-135: Well-implemented session-level mock clearingThe
clear_all_mocksfunction provides a robust mechanism for clearing mock files across all test cases. The error handling and progress reporting are well done.tests/llm/conftest.py (1)
67-152: Excellent implementation of shared test infrastructure coordinationThe
shared_test_infrastructurefixture effectively usespytest_shared_session_scopeto coordinate setup and cleanup across parallel test workers. The implementation properly handles skip options and ensures operations are performed only once even with multiple workers.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/llm/conftest.py (1)
202-202: Duplicate comment: Fix typo in fixture nameThis was already identified in previous reviews.
🧹 Nitpick comments (5)
tests/llm/utils/reporting/terminal_reporter.py (1)
40-53: Consider using dataclass or factory method for TestResult constructionThe TestResult construction is verbose with many parameters. Consider creating a factory method or using keyword arguments more systematically to improve readability.
- test_result = TestResult( - nodeid=result.get("nodeid", ""), - expected=result["expected"], - actual=result["actual"], - pass_fail=pass_fail, - tools_called=result["tools_called"], - logs="", # We don't have logs in this context - test_type=result["test_type"], - error_message=None, - execution_time=result.get("execution_time"), - expected_correctness_score=result["expected_correctness_score"], - actual_correctness_score=result["actual_correctness_score"], - mock_data_failure=result.get("mock_data_failure", False), - ) + test_result = TestResult.from_result_dict(result, pass_fail=pass_fail)tests/llm/utils/reporting/github_reporter.py (1)
32-40: Simplify variable initialization with tuple unpackingThe variable initialization can be simplified and made more readable.
- ask_holmes_total = ask_holmes_passed = ask_holmes_regressions = ( - ask_holmes_mock_failures - ) = 0 - investigate_total = investigate_passed = investigate_regressions = ( - investigate_mock_failures - ) = 0 - workload_health_total = workload_health_passed = workload_health_regressions = ( - workload_health_mock_failures - ) = 0 + # Initialize counters for each test type + counters = { + "ask": {"total": 0, "passed": 0, "regressions": 0, "mock_failures": 0}, + "investigate": {"total": 0, "passed": 0, "regressions": 0, "mock_failures": 0}, + "workload_health": {"total": 0, "passed": 0, "regressions": 0, "mock_failures": 0}, + }tests/llm/utils/setup_cleanup.py (1)
78-83: Potential race condition in remaining cases calculationThe calculation of remaining cases could be inaccurate due to race conditions when multiple threads complete simultaneously.
- remaining_cases = ( - len(test_cases) - - successful_test_cases - - failed_test_cases - - timed_out_test_cases - ) + completed_cases = successful_test_cases + failed_test_cases + timed_out_test_cases + remaining_cases = len(test_cases) - completed_cases - 1 # -1 for current casetests/llm/utils/commands.py (1)
11-36: Consider using dataclass for CommandResultThe CommandResult class would benefit from using a dataclass to reduce boilerplate and improve type hints.
+from dataclasses import dataclass + -class CommandResult: - def __init__( - self, - command: str, - test_case_id: str, - success: bool, - exit_code: int = None, - elapsed_time: float = 0, - error_type: str = None, - error_details: str = None, - ): - self.command = command - self.test_case_id = test_case_id - self.success = success - self.exit_code = exit_code - self.elapsed_time = elapsed_time - self.error_type = error_type # 'timeout', 'failure', or None - self.error_details = error_details +@dataclass +class CommandResult: + command: str + test_case_id: str + success: bool + exit_code: Optional[int] = None + elapsed_time: float = 0 + error_type: Optional[str] = None # 'timeout', 'failure', or None + error_details: Optional[str] = Nonetests/llm/conftest.py (1)
130-147: Potential performance issue with test case reconstructionThe cleanup logic iterates through all session items to reconstruct test cases, which could be inefficient for large test suites.
- # Reconstruct test cases from IDs - from tests.llm.utils.test_case_utils import HolmesTestCase # type: ignore[attr-defined] # type: ignore[attr-defined] - - cleanup_test_cases = [] - - for item in request.session.items: - if ( - item.get_closest_marker("llm") - and hasattr(item, "callspec") - and "test_case" in item.callspec.params - ): - test_case = item.callspec.params["test_case"] - if ( - isinstance(test_case, HolmesTestCase) - and test_case.id in test_case_ids - and test_case not in cleanup_test_cases - ): - cleanup_test_cases.append(test_case) + # Store full test case objects in setup phase instead of just IDs + # This avoids expensive reconstruction during cleanup
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
CLAUDE.md(1 hunks)docs/development/evals/index.md(3 hunks)docs/development/evals/writing.md(1 hunks)pyproject.toml(2 hunks)tests/llm/conftest.py(6 hunks)tests/llm/utils/commands.py(2 hunks)tests/llm/utils/reporting/__init__.py(1 hunks)tests/llm/utils/reporting/github_reporter.py(1 hunks)tests/llm/utils/reporting/terminal_reporter.py(1 hunks)tests/llm/utils/setup_cleanup.py(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- tests/llm/utils/reporting/init.py
- docs/development/evals/index.md
- docs/development/evals/writing.md
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/llm/utils/setup_cleanup.py (3)
tests/llm/utils/commands.py (2)
run_commands(61-132)exit_info(31-35)tests/llm/utils/test_case_utils.py (1)
HolmesTestCase(43-56)tests/llm/utils/test_helpers.py (1)
truncate_output(7-15)
⏰ 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 (12)
tests/llm/utils/reporting/terminal_reporter.py (3)
12-15: LGTM: Good early return patternThe function correctly handles the empty results case with an early return, avoiding unnecessary processing.
148-157: Robust error handling for LLM API callsGood practice to catch all exceptions and return a meaningful error message instead of letting the API failure crash the reporting.
119-119: litellm dependency is already declared in pyproject.tomlThe
litellm = "1.74.7"entry appears on line 43 ofpyproject.toml, so the test-only import inside_get_llm_analysisis covered by your project’s dependencies. No further action required.tests/llm/utils/setup_cleanup.py (3)
37-71: Excellent concurrent execution designThe function properly uses ThreadPoolExecutor with appropriate worker limits and handles both setup and cleanup operations cleanly. The future-to-test-case mapping is well-implemented.
95-107: Good timeout handling with comprehensive loggingThe timeout handling is well-implemented with proper logging, error formatting, and warning emission to ensure visibility in pytest output.
156-176: Efficient deduplication logicThe function properly extracts unique test cases using a set for deduplication while maintaining the original order. Good use of early filtering based on required conditions.
tests/llm/utils/commands.py (2)
76-84: Good command processing logicThe command processing correctly handles empty lines and accumulates output properly. The timing measurement is accurate.
93-106: Comprehensive error handling for CalledProcessErrorThe error handling properly captures all relevant information including exit codes, stdout, stderr, and timing. The error details formatting is well-structured.
tests/llm/conftest.py (4)
44-63: Robust mock configuration handlingThe fixture properly handles CLI options with safe defaults and correctly determines the mock mode based on environment and options.
68-84: Good early exit handling for shared fixtureThe fixture correctly handles collect-only mode and mock mode by yielding twice as required by pytest-shared-session-scope, even when skipping setup.
333-347: Good xdist worker detectionThe function properly detects xdist worker processes to avoid duplicate output. The worker ID detection logic is robust.
358-364: Excellent modularization of reportingThe reporting has been properly modularized with clear separation of concerns between GitHub output, console output, and mock operations reporting.
…dev/holmesgpt into session-scoped-setup-v3
…dev/holmesgpt into session-scoped-setup-v3
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/llm/conftest.py (1)
335-367: Verify debug print statements have been removedBased on previous review comments, debug print statements were flagged for removal throughout this file. Please confirm that all debug print statements have been cleaned up before merging, as they shouldn't be present in production code.
🧹 Nitpick comments (2)
holmes/core/tracing.py (1)
130-130: Consider investigating the necessity of type ignore commentsThe
# type: ignorecomments on these return statements suggest typing issues. Consider whether these can be resolved through:
- Better type annotations for the Braintrust spans
- Refining the Union return type
- Using more specific type hints
If the comments are necessary due to third-party library limitations, consider adding brief explanatory comments.
Also applies to: 135-135
tests/llm/conftest.py (1)
35-41: Consider minor optimization for is_llm_test functionThe function works correctly but could be slightly optimized:
def is_llm_test(nodeid: str) -> bool: """Check if a test nodeid is for an LLM test.""" - return any( - [ - "test_ask_holmes" in nodeid, - "test_investigate" in nodeid, - "test_workload_health" in nodeid, - ] - ) + return any( + test_type in nodeid + for test_type in ["test_ask_holmes", "test_investigate", "test_workload_health"] + )Using a generator expression avoids creating an intermediate list.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
holmes/core/tool_calling_llm.py(3 hunks)holmes/core/tracing.py(2 hunks)tests/llm/conftest.py(6 hunks)tests/llm/test_ask_holmes.py(8 hunks)tests/llm/test_investigate.py(4 hunks)tests/llm/test_workload_health.py(5 hunks)tests/llm/utils/setup_cleanup.py(1 hunks)tests/llm/utils/test_helpers.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/llm/test_investigate.py
- tests/llm/utils/test_helpers.py
- tests/llm/utils/setup_cleanup.py
- tests/llm/test_ask_holmes.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
holmes/core/tool_calling_llm.py (7)
holmes/core/tracing.py (2)
DummySpan(38-54)start_span(41-42)holmes/core/tools.py (2)
get_parameterized_one_liner(168-169)get_parameterized_one_liner(195-202)holmes/plugins/toolsets/logging_utils/logging_api.py (1)
get_parameterized_one_liner(105-132)holmes/plugins/toolsets/internet/internet.py (1)
get_parameterized_one_liner(216-218)holmes/plugins/toolsets/opensearch/opensearch.py (4)
get_parameterized_one_liner(104-105)get_parameterized_one_liner(134-135)get_parameterized_one_liner(162-163)get_parameterized_one_liner(183-184)holmes/plugins/toolsets/prometheus/prometheus.py (4)
get_parameterized_one_liner(355-356)get_parameterized_one_liner(458-459)get_parameterized_one_liner(567-570)get_parameterized_one_liner(713-719)holmes/plugins/toolsets/robusta/robusta.py (3)
get_parameterized_one_liner(76-77)get_parameterized_one_liner(140-141)get_parameterized_one_liner(198-199)
tests/llm/test_workload_health.py (2)
holmes/core/tracing.py (1)
SpanType(28-35)tests/llm/utils/property_manager.py (2)
set_initial_properties(7-33)update_test_results(46-56)
🪛 Ruff (0.12.2)
tests/llm/test_workload_health.py
138-139: 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 (17)
holmes/core/tracing.py (3)
35-35: LGTM: Clean addition of EVAL span typeThe new
EVAL = "eval"enum member follows the existing pattern and provides appropriate categorization for evaluation contexts.
41-41: LGTM: Simplified dummy span method signatureRemoving the explicit type annotation for
span_typein the dummy implementation makes sense since this no-op method ignores all parameters anyway.
125-125: LGTM: Simplified span type handlingUsing
span_type.valuedirectly instead ofgetattr(SpanTypeAttribute, span_type.name)is cleaner and more straightforward.holmes/core/tool_calling_llm.py (3)
42-42: LGTM: Import cleanup aligns with span type refactorRemoving the unused
SpanTypeimport is appropriate since the code now uses string literals for span types.
425-425: LGTM: Consistent span type refactorUsing the string literal
"tool"instead ofSpanType.TOOLaligns with the broader tracing refactor and maintains the same functionality.
454-455: LGTM: Enhanced tool call tracingThe new metadata fields improve observability:
"description"provides human-readable tool call summaries viaget_parameterized_one_liner"structured_tool_result"captures the complete tool response for detailed analysisThese enhancements will be valuable for debugging and monitoring tool execution.
tests/llm/test_workload_health.py (5)
9-9: LGTM: Appropriate import for span typingThe
SpanTypeimport is correctly used later in the test for proper span categorization.
27-27: LGTM: Property manager integrationThe imports support the refactor to centralize test metadata handling through the property manager utilities.
Also applies to: 29-29
106-107: LGTM: Early property initializationSetting initial properties early ensures test metadata is available even if the test fails during execution, improving error reporting and debugging.
139-139: LGTM: Appropriate span type categorizationUsing
SpanType.LLMfor the "Holmes Run" span correctly categorizes this as an LLM operation.
179-180: LGTM: Consolidated test result updatesUsing
update_test_resultsconsolidates property updates into a single function call, improving consistency and maintainability across test files.tests/llm/conftest.py (6)
5-26: LGTM: Comprehensive import refactor supports modularityThe updated imports reflect the successful extraction of functionality into dedicated utility modules:
pytest_shared_session_scopefor coordinating setup/cleanup across workers- Dedicated reporting modules for GitHub and terminal output
- Centralized mock management utilities
- Test result handling utilities
This modularization improves maintainability and code organization.
47-62: LGTM: Well-structured mock configuration fixtureThe fixture correctly handles option retrieval with safe defaults and implements clear logic for determining mock mode based on environment variables and CLI options. The hierarchical decision making (regenerate-all implies generate, environment overrides) is intuitive.
68-152: LGTM: Sophisticated infrastructure coordination fixtureThis fixture effectively solves the complex problem of coordinating setup/cleanup across pytest-xdist workers. Key strengths:
- Proper handling of different execution modes (collect-only, mock, live)
- Respects CLI flags for skipping setup/cleanup phases
- Uses pytest_shared_session_scope correctly with the two-yield pattern
- Comprehensive logic for first-worker setup and last-worker cleanup
- Clear logging for debugging coordination issues
This addresses a real challenge in distributed test execution and follows the established patterns for the shared session scope library.
201-201: LGTM: Typo fix addressedThe fixture name has been corrected from "llm_availablity_check" to "llm_availability_check", addressing the previous review comment.
328-332: LGTM: Excellent UX improvement with clickable linksThe ANSI escape code implementation for clickable terminal links is a thoughtful enhancement. The escape sequence format
\033]8;;URL\033\\TEXT\033]8;;\033\\is correctly implemented and will provide better user experience in modern terminals that support it.
335-367: LGTM: Clean refactor with proper xdist handlingThe refactor successfully:
- Delegates reporting responsibilities to dedicated modules (separation of concerns)
- Handles xdist worker coordination to prevent duplicate output
- Simplifies the main function by removing complex inline logic
This design is much more maintainable and testable than having all reporting logic inline.
2 participants
Many improvements to evals framework, primarily:
Note: