Repository navigation
ROB-1441: add ability to mock datetime prompt - #555
Conversation
|
Warning Rate limit exceeded@nherment has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 58 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 (3)
""" WalkthroughThis change centralizes current date and time rendering in prompts by introducing a shared Jinja2 template snippet, updates all references from Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HolmesCLI
participant Prompts
participant Jinja2
User->>HolmesCLI: Run prompt (e.g., ask for current date/time)
HolmesCLI->>Prompts: Render prompt template
Prompts->>Jinja2: Include _current_date_time.jinja2
Jinja2-->>Prompts: Inject current UTC date/time and timestamp
Prompts-->>HolmesCLI: Completed prompt with date/time info
HolmesCLI-->>User: Output with current date/time context
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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 (
|
nherment
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docs/installation.md (1)
106-106: Consistent entry point update for Poetry source run.
The example now usesholmes_cli.py. Consider standardizing the Python invocation (e.g.,pythonvs.python3) across all docs to avoid confusion.tests/llm/utils/mock_utils.py (1)
48-49: Consider adding validation for the mocked_date field.The
mocked_datefield accepts any string, but the parsing logic intest_ask_holmes.pyexpects ISO format. Invalid formats could cause runtime errors during test execution.Consider adding a field validator to ensure the date format is valid:
from pydantic import BaseModel, TypeAdapter, field_validator +from datetime import datetime class HolmesTestCase(BaseModel): id: str folder: str mocked_date: Optional[str] = None tags: Optional[list[ALLOWED_EVAL_TAGS]] = None + + @field_validator('mocked_date') + @classmethod + def validate_mocked_date(cls, v): + if v is not None: + try: + datetime.fromisoformat(v.replace("Z", "+00:00")) + except ValueError as e: + raise ValueError(f"Invalid ISO datetime format: {e}") + return vtests/llm/test_ask_holmes.py (1)
44-50: Simplify conditional logic.The static analysis tool correctly identified unnecessary
elseafterreturn. The code can be simplified.if iterations: return [ add_tags_to_eval(experiment_name, test_case) for test_case in test_cases ] * iterations - else: - return [ - add_tags_to_eval(experiment_name, test_case) for test_case in test_cases - ] + return [ + add_tags_to_eval(experiment_name, test_case) for test_case in test_cases + ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.github/workflows/build-and-test.yaml(1 hunks).github/workflows/build-binaries-and-brew.yaml(1 hunks)Dockerfile(1 hunks)docs/api-keys.md(1 hunks)docs/installation.md(1 hunks)holmes/core/tool_calling_llm.py(1 hunks)holmes/plugins/prompts/_current_date_time.jinja2(1 hunks)holmes/plugins/prompts/generic_ask_conversation.jinja2(1 hunks)holmes/plugins/prompts/generic_ask_for_issue_conversation.jinja2(1 hunks)holmes/plugins/prompts/generic_investigation.jinja2(1 hunks)holmes/plugins/prompts/kubernetes_workload_ask.jinja2(1 hunks)holmes/plugins/prompts/kubernetes_workload_chat.jinja2(1 hunks)holmes/plugins/toolsets/datetime.py(1 hunks)holmes/plugins/toolsets/prometheus/prometheus_instructions.jinja2(0 hunks)tests/llm/fixtures/test_ask_holmes/30_basic_promql_graph_cluster_memory/get_current_time.txt(0 hunks)tests/llm/fixtures/test_ask_holmes/30_basic_promql_graph_cluster_memory/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/31_basic_promql_graph_pod_memory/get_current_time.txt(0 hunks)tests/llm/fixtures/test_ask_holmes/32_basic_promql_graph_pod_cpu/get_current_time.txt(0 hunks)tests/llm/fixtures/test_ask_holmes/33_http_latency_graph/get_current_time.txt(0 hunks)tests/llm/fixtures/test_ask_holmes/34_memory_graph/get_current_time.txt(0 hunks)tests/llm/fixtures/test_ask_holmes/43_current_datetime/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/43_current_datetime/toolsets.yaml(1 hunks)tests/llm/test_ask_holmes.py(4 hunks)tests/llm/utils/constants.py(1 hunks)tests/llm/utils/mock_utils.py(2 hunks)tests/llm/utils/tags.py(1 hunks)
💤 Files with no reviewable changes (6)
- holmes/plugins/toolsets/prometheus/prometheus_instructions.jinja2
- tests/llm/fixtures/test_ask_holmes/34_memory_graph/get_current_time.txt
- tests/llm/fixtures/test_ask_holmes/32_basic_promql_graph_pod_cpu/get_current_time.txt
- tests/llm/fixtures/test_ask_holmes/30_basic_promql_graph_cluster_memory/get_current_time.txt
- tests/llm/fixtures/test_ask_holmes/31_basic_promql_graph_pod_memory/get_current_time.txt
- tests/llm/fixtures/test_ask_holmes/33_http_latency_graph/get_current_time.txt
🧰 Additional context used
🪛 Pylint (3.3.7)
tests/llm/test_ask_holmes.py
[refactor] 43-50: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
🔇 Additional comments (22)
docs/api-keys.md (1)
139-139: Update CLI invocation script name.
The example has been correctly updated to referenceholmes_cli.pyinstead of the oldholmes.py. This aligns with the renamed entry point across build and documentation.holmes/core/tool_calling_llm.py (1)
647-647: Updated TODO to reflect new CLI entry point.
The comment now correctly references moving templating intoholmes_cli.py. No functional impact..github/workflows/build-and-test.yaml (1)
50-52: Switch PyInstaller entry script toholmes_cli.py.
The build step has been updated to useholmes_cli.pyand explicitly set the binary name toholmes, keeping it in sync with other CI workflows..github/workflows/build-binaries-and-brew.yaml (1)
71-72: Align PyInstaller build step with new CLI script.
The release build now invokes PyInstaller onholmes_cli.pyand names the artifactholmes, matching the test workflow and Dockerfile changes.Dockerfile (2)
139-139: Ensure new entrypoint script is present in the build context.Verify that
holmes_cli.pyexists at the repository root to avoid build failures during theCOPYstep.#!/bin/bash # Check for holmes_cli.py in the build context if [ ! -f holmes_cli.py ]; then echo "ERROR: holmes_cli.py not found" exit 1 fi
141-141: Validate ENTRYPOINT updates across CI workflows and docs.Confirm that all CI pipelines and documentation no longer reference the old
holmes.py:#!/bin/bash # Search for obsolete references to holmes.py rg -l "holmes.py" .github/workflows/ docs/ || echo "No holmes.py references found"holmes/plugins/prompts/_current_date_time.jinja2 (1)
1-1: Centralize date/time snippet
This new template cleanly encapsulates the current UTC date/time and timestamp logic for reuse across all prompts.holmes/plugins/prompts/generic_investigation.jinja2 (1)
6-6: Reuse_current_date_timesnippet
Replacing the inline date/time block with a Jinja2 include promotes consistency and DRY across prompt templates.tests/llm/utils/constants.py (2)
1-1: ImportLiteralfor new type alias
AddingLiteralenables defining a constrained set of allowed evaluation tags.
8-13: DefineALLOWED_EVAL_TAGSwithLiteral
This type alias enumerates valid evaluation categories ("logs","context_window","synthetic","datetime") for strong typing in tests.holmes/plugins/toolsets/datetime.py (1)
32-39: Disable datetime toolset by default
Changingenabledandis_defaulttoFalseensures the datetime toolset remains inactive unless explicitly enabled, aligning with the new test fixtures.tests/llm/fixtures/test_ask_holmes/43_current_datetime/toolsets.yaml (1)
1-5: Add config to disabledatetimetoolset for test
Explicitly disabling the datetime toolset here guarantees controlled evaluation of prompt-based datetime handling without invoking the actual tool.tests/llm/fixtures/test_ask_holmes/30_basic_promql_graph_cluster_memory/test_case.yaml (1)
6-6: LGTM: Test configuration enhancementThe addition of the
generate_mocks: Trueflag aligns with the PR objectives to enhance mock generation capabilities for datetime testing. This configuration flag will likely enable deterministic behavior during test evaluation.holmes/plugins/prompts/generic_ask_conversation.jinja2 (1)
6-6: LGTM: Centralized datetime injectionReplacing inline datetime variables with a reusable template include improves maintainability and enables consistent datetime mocking across all prompts.
Verify that the
_current_date_time.jinja2template provides equivalent functionality to the previous inline datetime variables:#!/bin/bash # Description: Verify the _current_date_time.jinja2 template exists and examine its content # Check if the template file exists fd "_current_date_time.jinja2" --type f # Examine the template content to ensure it provides datetime variables fd "_current_date_time.jinja2" --type f --exec cat {}holmes/plugins/prompts/generic_ask_for_issue_conversation.jinja2 (1)
5-5: LGTM: Consistent datetime centralizationThe replacement of inline datetime variables with the shared template include is consistent with the centralization strategy across all prompt templates.
holmes/plugins/prompts/kubernetes_workload_ask.jinja2 (1)
7-7: LGTM: Datetime centralization in workload templateThe datetime template inclusion follows the established pattern and is positioned appropriately within the prompt structure.
holmes/plugins/prompts/kubernetes_workload_chat.jinja2 (1)
5-5: LGTM: Completes datetime centralization effortThis change completes the consistent centralization of datetime injection across all prompt templates, enabling unified mocking and maintenance.
tests/llm/fixtures/test_ask_holmes/43_current_datetime/test_case.yaml (1)
1-13: Well-structured datetime test case.The test case properly defines a mocked datetime scenario with specific expected outputs. The ISO format for
mocked_dateis correct and the tags appropriately categorize this as a datetime and synthetic test.tests/llm/utils/tags.py (2)
6-13: Clean and focused tagging utility implementation.The
get_tagsfunction correctly converts tag strings to pytest marks usinggetattr, which is safe since tags are constrained byALLOWED_EVAL_TAGS.
16-19: Proper pytest parameterization with tags.The
add_tags_to_evalfunction correctly creates pytest parameters with marks and IDs, enabling tagged test execution.tests/llm/test_ask_holmes.py (2)
77-79: Good datetime mocking implementation.The mocking setup correctly:
- Patches the specific module where datetime is used
- Preserves normal datetime instantiation via
side_effect- Properly handles the
now()method return value
72-82: ```shell
#!/bin/bashList all Python files in the prompts package
find holmes/plugins/prompts -maxdepth 1 -type f -name "*.py" -print
Locate where ask_holmes is defined within the prompts package
rg -n "def ask_holmes" -g "holmes/plugins/prompts/*.py"
Inspect the file(s) containing ask_holmes for datetime import/usage
rg -n "import datetime" -g "holmes/plugins/prompts/.py"
rg -n "datetime." -g "holmes/plugins/prompts/.py"</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/llm/test_ask_holmes.py (2)
44-50: Refactor to remove unnecessary else clause.The test case generation logic correctly implements the tagging functionality, but the else clause is unnecessary since both branches return similar structures.
Apply this diff to simplify the logic:
iterations = int(os.environ.get("ITERATIONS", "0")) if iterations: return [ add_tags_to_eval(experiment_name, test_case) for test_case in test_cases ] * iterations - else: - return [ - add_tags_to_eval(experiment_name, test_case) for test_case in test_cases - ] + return [ + add_tags_to_eval(experiment_name, test_case) for test_case in test_cases + ]
203-203: Consider using logging instead of print for better debug control.The debug print statement is useful for diagnostics, but using the logging module would provide better control over output in different environments.
Consider this approach:
+ import logging + logger = logging.getLogger(__name__) + - print(f"PROMPT {messages[0].get('content')}") + logger.debug(f"PROMPT {messages[0].get('content')}")Or if you prefer to keep it as print for test visibility, consider adding a prefix:
- print(f"PROMPT {messages[0].get('content')}") + print(f"** PROMPT **\n{messages[0].get('content')}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
holmes/plugins/toolsets/__init__.py(0 hunks)holmes/plugins/toolsets/datetime.py(0 hunks)tests/llm/fixtures/test_ask_holmes/43_current_datetime_from_prompt/test_case.yaml(1 hunks)tests/llm/test_ask_holmes.py(5 hunks)
💤 Files with no reviewable changes (2)
- holmes/plugins/toolsets/init.py
- holmes/plugins/toolsets/datetime.py
✅ Files skipped from review due to trivial changes (1)
- tests/llm/fixtures/test_ask_holmes/43_current_datetime_from_prompt/test_case.yaml
🧰 Additional context used
🪛 Pylint (3.3.7)
tests/llm/test_ask_holmes.py
[refactor] 43-50: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: Pre-commit checks
- GitHub Check: build (3.12)
🔇 Additional comments (1)
tests/llm/test_ask_holmes.py (1)
5-6: LGTM! Imports are appropriate for the new functionality.The new imports support the datetime mocking and test tagging features being introduced.
Also applies to: 22-22
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/llm/test_ask_holmes.py (2)
43-50: Remove unnecessary else clause for cleaner code structure.The
elseclause after thereturnstatement is unnecessary and can be simplified by removing theelseand de-indenting the code.if iterations: return [ add_tags_to_eval(experiment_name, test_case) for test_case in test_cases ] * iterations - else: - return [ - add_tags_to_eval(experiment_name, test_case) for test_case in test_cases - ] + return [ + add_tags_to_eval(experiment_name, test_case) for test_case in test_cases + ]
208-208: Consider adding more descriptive debug information.The debug print statement provides useful information, but could be more descriptive about what it's showing.
- print(f"PROMPT {messages[0].get('content')}") + print(f"** PROMPT CONTENT **\n{messages[0].get('content')}")This maintains consistency with other debug output formatting in the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/llm/test_ask_holmes.py(5 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
tests/llm/test_ask_holmes.py
[refactor] 43-50: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🪛 GitHub Actions: Build and test HolmesGPT
tests/llm/test_ask_holmes.py
[error] 78-87: ruff-format: File was reformatted by the ruff formatter. Please run 'ruff --fix' or the equivalent to apply formatting changes.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (3.12)
2 participants
Change related to #552 that:
holmes/__init__pywithholmes.py