Weekly run of a fast benchmark - #1329
Conversation
Signed-off-by: Tomer Keshet <tomer@robusta.dev>
Signed-off-by: Tomer Keshet <tomer@robusta.dev>
Signed-off-by: Tomer Keshet <tomer@robusta.dev>
Signed-off-by: Tomer Keshet <tomer@robusta.dev>
|
WalkthroughAdds a two-tier benchmarking flow (fast/full) across CI, local runner, report generation, and docs: introduces a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Trigger as Trigger\n(schedule / workflow_dispatch)
participant Builder as Build Benchmark Command
participant Runner as Execute Benchmark Command
participant Storage as Artifact Store / Repo
participant PRSvc as PR Automation (git / gh)
Trigger->>Builder: inputs (benchmark_type / markers / models / iterations)
Builder->>Builder: construct `command` and set `benchmark_type`
Builder-->>Runner: export `command`, `benchmark_type`
Runner->>Runner: run constructed benchmark command (script)
Runner->>Storage: upload artifacts (fast/full results, eval_results.json)
Storage->>PRSvc: write timestamped history files
PRSvc->>PRSvc: commit branch, push, create/update PR (title/body include benchmark_type + command)
PRSvc-->>Trigger: PR opened/updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Docker image ready for
Use this tag to pull the image for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:56c4497
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:56c4497 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:56c4497
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:56c4497Patch Helm values in one line (choose the chart you use): HolmesGPT chart: helm upgrade --install holmesgpt ./helm/holmes \
--set registry=me-west1-docker.pkg.dev/robusta-development/development \
--set image=holmes-dev:56c4497Robusta wrapper chart: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.image=holmes-dev:56c4497 |
📂 Previous Runs📜 Run @ a974979 (#20740934476)✅ Results of HolmesGPT evalsAutomatically triggered by commit a974979 on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'weekly-benchmark' Status: Success - 10 test/model combinations loaded Experiments compared (30):
Comparison indicators:
📜 Run @ d0348c3 (#20721255719)✅ Results of HolmesGPT evalsAutomatically triggered by commit d0348c3 on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'weekly-benchmark' Status: Success - 11 test/model combinations loaded Experiments compared (30):
Comparison indicators:
|
| Status | Test case | Time | Turns | Tools | Cost |
|---|---|---|---|---|---|
| ✅ | 09_crashpod | 30.8s ↓15% | 5 | 12 | $0.1561 |
| ✅ | 101_loki_historical_logs_pod_deleted | 45.2s ↓32% | 7 | 15 | $0.1955 |
| ✅ | 111_pod_names_contain_service | 34.4s ↓18% | 6 | 14 | $0.1600 |
| ✅ | 12_job_crashing | 48.8s ±0% | 9 | 20 | $0.2374 |
| ✅ | 162_get_runbooks | 48.5s ±0% | 7 | 19 | $0.2347 |
| ✅ | 176_network_policy_blocking_traffic_no_runbooks | 36.9s ↓13% | 6 | 14 | $0.1741 |
| ✅ | 24_misconfigured_pvc | 33.0s ↓16% | 6 | 15 | $0.1623 |
| ✅ | 43_current_datetime_from_prompt | 3.4s ±0% | 1 | — | $0.0618 |
| ✅ | 61_exact_match_counting | 10.8s ±0% | 3 | 3 | $0.0859 |
| Total | 32.4s avg | 5.6 avg | 14.0 avg | $1.4678 |
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%.
Historical Comparison Details
Filter: excluding branch 'weekly-benchmark'
Status: Success - 11 test/model combinations loaded
Experiments compared (30):
- github-20719199615.1713.1 (branch:
codex/linear-mention-rob-47-holmesgpt-update-evals-to-accept-ad) - github-20718852788.1711.1 (branch:
master) - root-codex/linear-mention-rob-47-holmesgpt-update-evals-to-accept-ad-k=194-20260105_162821 (branch:
codex/linear-mention-rob-47-holmesgpt-update-evals-to-accept-ad) - ...and 27 more
Comparison indicators:
±0%— diff under 10% (within noise threshold)↑N%/↓N%— diff 10-25%↑N%/↓N%— diff over 25% (significant)
📜 Run @ 6f26b3d (#20718971370)
✅ Results of HolmesGPT evals
Automatically triggered by commit 6f26b3d on branch weekly-benchmark
Results of HolmesGPT evals
- ask_holmes: 9/9 test cases were successful, 0 regressions
| Status | Test case | Time | Turns | Tools | Cost |
|---|---|---|---|---|---|
| ✅ | 09_crashpod | 35.2s ±0% | 6 | 13 | $0.1112 |
| ✅ | 101_loki_historical_logs_pod_deleted | 68.5s ±0% | 11 | 24 | $0.2090 |
| ✅ | 111_pod_names_contain_service | 49.7s ↑20% | 9 | 20 | $0.1623 |
| ✅ | 12_job_crashing | 52.6s ±0% | 9 | 18 | $0.1640 |
| ✅ | 162_get_runbooks | 55.9s ±0% | 8 | 17 | $0.1884 |
| ✅ | 176_network_policy_blocking_traffic_no_runbooks | 42.0s ±0% | 7 | 14 | $0.1199 |
| ✅ | 24_misconfigured_pvc | 37.6s ±0% | 7 | 16 | $0.1232 |
| ✅ | 43_current_datetime_from_prompt | 3.4s ±0% | 1 | — | $0.0085 |
| ✅ | 61_exact_match_counting | 11.4s ±0% | 3 | 3 | $0.0326 |
| Total | 39.6s avg | 6.8 avg | 15.6 avg | $1.1191 |
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%.
Historical Comparison Details
Filter: excluding branch 'weekly-benchmark'
Status: Success - 11 test/model combinations loaded
Experiments compared (30):
- github-20718852788.1711.1 (branch:
master) - root-codex/linear-mention-rob-47-holmesgpt-update-evals-to-accept-ad-k=194-20260105_162821 (branch:
codex/linear-mention-rob-47-holmesgpt-update-evals-to-accept-ad) - avirobusta-master-k=09_crashpod-20260105_161221 (branch:
master) - ...and 27 more
Comparison indicators:
±0%— diff under 10% (within noise threshold)↑N%/↓N%— diff 10-25%↑N%/↓N%— diff over 25% (significant)
📜 Run @ 3175f13 (#20718124662)
✅ Results of HolmesGPT evals
Automatically triggered by commit 3175f13 on branch weekly-benchmark
Results of HolmesGPT evals
- ask_holmes: 9/9 test cases were successful, 0 regressions
| Status | Test case | Time | Turns | Tools | Cost |
|---|---|---|---|---|---|
| ✅ | 09_crashpod | 35.2s ±0% | 5 | 12 | $0.1576 |
| ✅ | 101_loki_historical_logs_pod_deleted | 53.8s ↓20% | 8 | 15 | $0.2013 |
| ✅ | 111_pod_names_contain_service | 48.0s ↑16% | 7 | 15 | $0.1784 |
| ✅ | 12_job_crashing | 70.3s ↑39% | 10 | 24 | $0.2665 |
| ✅ | 162_get_runbooks | 60.0s ↑14% | 8 | 15 | $0.2294 |
| ✅ | 176_network_policy_blocking_traffic_no_runbooks | 45.4s ±0% | 6 | 15 | $0.1890 |
| ✅ | 24_misconfigured_pvc | 43.3s ±0% | 7 | 19 | $0.1847 |
| ✅ | 43_current_datetime_from_prompt | 4.1s ↑18% | 1 | — | $0.0618 |
| ✅ | 61_exact_match_counting | 13.1s ↑11% | 3 | 3 | $0.0859 |
| Total | 41.5s avg | 6.1 avg | 14.8 avg | $1.5546 |
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%.
Historical Comparison Details
Filter: excluding branch 'weekly-benchmark'
Status: Success - 10 test/model combinations loaded
Experiments compared (30):
- root-codex/linear-mention-rob-47-holmesgpt-update-evals-to-accept-ad-k=194-20260105_162821 (branch:
codex/linear-mention-rob-47-holmesgpt-update-evals-to-accept-ad) - avirobusta-master-k=09_crashpod-20260105_161221 (branch:
master) - avirobusta-master-k=09_crashpod-20260105_160932 (branch:
master) - ...and 27 more
Comparison indicators:
±0%— diff under 10% (within noise threshold)↑N%/↓N%— diff 10-25%↑N%/↓N%— diff over 25% (significant)
✅ Results of HolmesGPT evals
Automatically triggered by commit 766775b on branch weekly-benchmark
Results of HolmesGPT evals
- ask_holmes: 9/9 test cases were successful, 0 regressions
| Status | Test case | Time | Turns | Tools | Cost |
|---|---|---|---|---|---|
| ✅ | 09_crashpod | 32.9s ±0% | 6 | 13 | $0.1639 |
| ✅ | 101_loki_historical_logs_pod_deleted | 55.8s ±0% | 10 | 16 | $0.2273 |
| ✅ | 111_pod_names_contain_service | 42.9s ±0% | 8 | 19 | $0.2007 |
| ✅ | 12_job_crashing | 54.9s ±0% | 10 | 23 | $0.2627 |
| ✅ | 162_get_runbooks | 42.5s ↓12% | 7 | 14 | $0.2044 |
| ✅ | 176_network_policy_blocking_traffic_no_runbooks | 38.9s ±0% | 6 | 15 | $0.1858 |
| ✅ | 24_misconfigured_pvc | 36.9s ±0% | 7 | 17 | $0.1797 |
| ✅ | 43_current_datetime_from_prompt | 3.0s ↓12% | 1 | — | $0.0618 |
| ✅ | 61_exact_match_counting | 10.6s ±0% | 3 | 3 | $0.0870 |
| Total | 35.4s avg | 6.4 avg | 15.0 avg | $1.5731 |
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%.
Historical Comparison Details
Filter: excluding branch 'weekly-benchmark'
Status: Success - 10 test/model combinations loaded
Experiments compared (30):
- github-20740586252.1755.1 (branch:
master) - avirobusta-master-k=09_crashpod-20260106_085231 (branch:
master) - avirobusta-master-k=09_crashpod-20260106_085003 (branch:
master) - ...and 27 more
Comparison indicators:
±0%— diff under 10% (within noise threshold)↑N%/↓N%— diff 10-25%↑N%/↓N%— diff over 25% (significant)
📖 Legend
| Icon | Meaning |
|---|---|
| ✅ | The test was successful |
| ➖ | The test was skipped |
| The test failed but is known to be flaky or known to fail | |
| 🚧 | The test had a setup failure (not a code regression) |
| 🔧 | The test failed due to mock data issues (not a code regression) |
| 🚫 | The test was throttled by API rate limits/overload |
| ❌ | The test failed and should be fixed before merging the PR |
🔄 Re-run evals manually
⚠️ Warning:/evalcomments always run using the workflow from master, not from this PR branch. If you modified the GitHub Action (e.g., added secrets or env vars), those changes won't take effect.To test workflow changes, use the GitHub CLI or Actions UI instead:
gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref weekly-benchmark -f markers=regression -f filter=
Option 1: Comment on this PR with /eval:
/eval
markers: regression
Or with more options (one per line):
/eval
model: gpt-4o
markers: regression
filter: 09_crashpod
iterations: 5
Run evals on a different branch (e.g., master) for comparison:
/eval
branch: master
markers: regression
| Option | Description |
|---|---|
model |
Model(s) to test (default: same as automatic runs) |
markers |
Pytest markers (no default - runs all tests!) |
filter |
Pytest -k filter (use /list to see valid eval names) |
iterations |
Number of runs, max 10 |
branch |
Run evals on a different branch (for cross-branch comparison) |
Quick re-run: Use /rerun to re-run the most recent /eval on this PR with the same parameters.
Option 2: Trigger via GitHub Actions UI → "Run workflow"
🏷️ Valid markers
benchmark, chain-of-causation, compaction, context_window, coralogix, counting, database, datadog, datetime, easy, elasticsearch, embeds, grafana-dashboard, hard, kafka, kubernetes, leaked-information, logs, loki, medium, metrics, network, newrelic, no-cicd, numerical, one-test, port-forward, prometheus, question-answer, regression, runbooks, slackbot, storage, toolset-limitation, traces, transparency
Commands: /eval · /rerun · /list
CLI: gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref weekly-benchmark -f markers=regression -f filter=
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @tests/generate_eval_report.py:
- Line 1347: Remove the in-function "import re" and instead add "import re" to
the module-level imports at the top of the file (alongside the other imports),
then delete the original import statement where it currently appears so there
are no duplicate imports.
🧹 Nitpick comments (3)
docs/development/evaluations/latest-results.md (1)
9-9: Improve link text for better accessibility and UX.The link text "click here" is not descriptive. Prefer a meaningful description of the link destination, such as "latest benchmark results" or "benchmark results history."
🔎 Suggested fix
-If you are not redirected automatically, [click here](../history/results_20260104_174301/). +If you are not redirected automatically, [view the latest benchmark results](../history/results_20260104_174301/).tests/generate_eval_report.py (1)
1349-1358: Unused variables from tuple unpacking.The variables
hour,minute, andsecondare extracted but never used. Use underscores to indicate they are intentionally ignored.Proposed fix
- year, month, day, hour, minute, second = history_match.groups() + year, month, day, _hour, _minute, _second = history_match.groups().github/workflows/eval-benchmarks.yaml (1)
118-161: PR title says "Weekly" even for manual runs.Line 146 hardcodes "Weekly Benchmark Results" but this workflow can also be triggered manually via
workflow_dispatch. Consider making the title dynamic.Suggested fix
- PR_TITLE="Weekly Benchmark Results $(date +%Y-%m-%d)" + if [ "${{ github.event_name }}" == "schedule" ]; then + PR_TITLE="Weekly Benchmark Results $(date +%Y-%m-%d)" + else + PR_TITLE="Benchmark Results $(date +%Y-%m-%d)" + fiAlso note: same-day runs will push to the same branch (
automated/benchmark-YYYYMMDD) with--force, which will overwrite previous results. This is acceptable for automation but worth documenting.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/eval-benchmarks.yamldocs/development/evaluations/.nav.ymldocs/development/evaluations/fast-benchmark-results.mddocs/development/evaluations/full-benchmark-results.mddocs/development/evaluations/history/index.mddocs/development/evaluations/history/results_20260104_174301.mddocs/development/evaluations/index.mddocs/development/evaluations/latest-results.mdrun_benchmarks_local.pytests/generate_eval_report.py
🧰 Additional context used
📓 Path-based instructions (3)
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Add blank line between header/bold text and a list in MkDocs documentation files, otherwise lists won't render properly
Files:
docs/development/evaluations/fast-benchmark-results.mddocs/development/evaluations/full-benchmark-results.mddocs/development/evaluations/history/index.mddocs/development/evaluations/history/results_20260104_174301.mddocs/development/evaluations/latest-results.mddocs/development/evaluations/index.md
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting with configuration in pyproject.toml
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks on Python files
Files:
tests/generate_eval_report.pyrun_benchmarks_local.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Tests should match source structure under tests/
Live execution is now enabled by default to ensure tests match real-world behavior
Files:
tests/generate_eval_report.py
🧠 Learnings (2)
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to tests/llm/**/*.yaml : Use real-world scenarios in eval tests (ML pipelines with checkpoint issues, database connection pools) not simulated scenarios
Applied to files:
docs/development/evaluations/fast-benchmark-results.mddocs/development/evaluations/full-benchmark-results.mddocs/development/evaluations/history/results_20260104_174301.mddocs/development/evaluations/index.md
📚 Learning: 2026-01-05T11:14:20.209Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.209Z
Learning: Add required environment variables to .github/workflows/eval-regression.yaml 'Run tests' step when adding evals for new cloud service integrations
Applied to files:
.github/workflows/eval-benchmarks.yaml
🪛 markdownlint-cli2 (0.18.1)
docs/development/evaluations/latest-results.md
9-9: Link text should be descriptive
(MD059, descriptive-link-text)
🪛 Ruff (0.14.10)
tests/generate_eval_report.py
1356-1356: Unpacked variable hour is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
1356-1356: Unpacked variable minute is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
1356-1356: Unpacked variable second is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
run_benchmarks_local.py
243-243: subprocess call: check for execution of untrusted input
(S603)
254-254: subprocess call: check for execution of untrusted input
(S603)
⏰ 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). (4)
- GitHub Check: build
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
🔇 Additional comments (23)
docs/development/evaluations/.nav.yml (1)
4-4: LGTM! Simplified navigation label aligns with new branding.The change from "Historical Results" to "History" is more concise and aligns with the new "Benchmark History" page title.
docs/development/evaluations/index.md (2)
9-18: LGTM! Clear benchmark types documentation.The new "Benchmark Types" section effectively distinguishes fast and full benchmarks with a well-formatted table and clear descriptions. The ⚡ emoji provides good visual distinction.
20-27: LGTM! Test categories section properly formatted.The updated test categories list is well-structured and follows MkDocs formatting guidelines with proper blank line spacing.
run_benchmarks_local.py (8)
17-21: LGTM! Clear benchmark type definitions.The
BENCHMARK_TYPESconstant provides a clean mapping between benchmark types and their corresponding pytest markers. This centralizes the configuration and makes it easy to maintain.
36-36: LGTM! Proper type hints and parameter handling.The new
benchmark_typeparameter is correctly typed asOptional[str]with an appropriate default value ofNone, and properly stored as an instance variable.Also applies to: 45-45
56-57: LGTM! Clear benchmark type display.The conditional display of the benchmark type helps users understand which benchmark configuration is running.
209-240: LGTM! Well-structured benchmark-type-aware report generation.The logic correctly determines output files based on benchmark type and builds appropriate commands. The fallback to
full-benchmark-results.mdfor custom markers is sensible.
285-286: LGTM! Consistent benchmark type handling in summary.The summary display correctly shows the benchmark type and determines the appropriate result file, maintaining consistency with the report generation logic.
Also applies to: 305-313
381-393: LGTM! Well-defined CLI arguments with validation.The new
--benchmark-typeargument uses proper validation withchoices, and both mutually exclusive arguments clearly document their relationship in help text.
438-469: LGTM! Robust validation and default behavior.The mutual exclusivity validation is correctly implemented, and the logic properly handles three scenarios: custom markers, explicit benchmark type, and default to fast-benchmark. The default behavior (fast-benchmark) aligns with the PR objectives.
257-274: Path construction is correct for MkDocs.MkDocs serves markdown files with the
.mdextension removed and a trailing slash. The code correctly generates relative paths like../history/results_20260104_174301/for redirect targets, which aligns with MkDocs' URL structure as confirmed in themkdocs.ymlnavigation entries.docs/development/evaluations/history/index.md (1)
1-10: LGTM! Clear benchmark history documentation.The updated history page provides clear distinction between fast and full benchmarks with consistent use of the ⚡ emoji and proper MkDocs formatting.
docs/development/evaluations/full-benchmark-results.md (2)
1-16: LGTM! Well-structured full benchmark results header.The header clearly identifies this as a full benchmark with appropriate metadata and a well-formatted info box distinguishing it from fast benchmarks.
17-71: LGTM! Well-formatted benchmark results tables.The results tables and detailed sections are properly formatted with appropriate links to Braintrust for deeper analysis. The auto-generation footer clearly indicates this file is maintained by CI.
docs/development/evaluations/fast-benchmark-results.md (1)
1-71: LGTM!The document is properly formatted with appropriate blank lines between sections and tables. The admonition block and table structures follow MkDocs conventions correctly.
docs/development/evaluations/history/results_20260104_174301.md (1)
1-121: LGTM!The generated results document is well-structured with proper blank lines between headers, sections, and tables. All MkDocs formatting conventions are followed correctly.
tests/generate_eval_report.py (4)
232-237: LGTM!The new
--benchmark-typeCLI argument is well-defined with appropriate choices and a sensible default ofNone.
1333-1343: LGTM!The benchmark type label and icon logic is clean and handles all three cases (fast, full, and unspecified) appropriately.
1359-1364: LGTM!The title generation correctly incorporates the benchmark icon for both history files and main result files.
1443-1461: LGTM!The info boxes use proper MkDocs admonition syntax and clearly document the markers, schedule, and purpose for each benchmark type.
.github/workflows/eval-benchmarks.yaml (3)
7-30: LGTM!The new
benchmark_typeinput with choices and the weekly schedule trigger are well-configured. Good use of mutual exclusivity documentation in the descriptions.
57-92: Potential quoting issue with custom markers.Line 71 embeds escaped quotes into the command string, which may not expand correctly when executed on line 105. Consider using single quotes or avoiding embedded quotes.
Additionally, when both
benchmark_type(default) and emptytest_markersare provided in workflow_dispatch, the condition on line 70 will be false (empty string), falling through to line 72 which adds the benchmark_type. This behavior appears intentional but should be verified.Suggested safer approach for custom markers
- if [ -n "$CUSTOM_MARKERS" ]; then - CMD="$CMD --markers \"$CUSTOM_MARKERS\"" + if [ -n "$CUSTOM_MARKERS" ]; then + CMD="$CMD --markers '$CUSTOM_MARKERS'"Or avoid quotes entirely if markers don't contain spaces:
- if [ -n "$CUSTOM_MARKERS" ]; then - CMD="$CMD --markers \"$CUSTOM_MARKERS\"" + if [ -n "$CUSTOM_MARKERS" ]; then + CMD="$CMD --markers $CUSTOM_MARKERS"
104-105: No changes needed.The script is executable (mode 100755) with a proper shebang (
#!/usr/bin/env python3), so direct command substitution works as intended. Since all inputs are controlled through the GitHub Actions workflow interface and arguments are properly escaped, the command execution is safe.
Signed-off-by: Tomer Keshet <tomer@robusta.dev>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/generate_eval_report.py (1)
1347-1347: Moveimport reto the top of the file.As previously noted, Python imports should be placed at the top of the file per coding guidelines.
🧹 Nitpick comments (4)
holmes/plugins/toolsets/__init__.py (1)
98-98: LGTM! Toolsets instantiated correctly.All three toolsets are properly instantiated with their default constructors.
Optional organizational suggestion: Consider grouping the OpenSearchQueryAssistToolset (line 98) together with the Elasticsearch toolsets (lines 107-108), since OpenSearch is an Elasticsearch fork. This would improve discoverability and maintainability.
🔎 Optional refactor to group related toolsets
DatadogGeneralToolset(), DatadogMetricsToolset(), DatadogTracesToolset(), - OpenSearchQueryAssistToolset(), CoralogixToolset(), RabbitMQToolset(), GitToolset(), BashExecutorToolset(), MongoDBAtlasToolset(), RunbookToolset(dal=dal, additional_search_paths=additional_search_paths), AzureSQLToolset(), ServiceNowTablesToolset(), ElasticsearchDataToolset(), ElasticsearchClusterToolset(), + OpenSearchQueryAssistToolset(), ]Also applies to: 107-108
holmes/plugins/toolsets/elasticsearch/elasticsearch.py (1)
74-108: Consider using try-except-else pattern for clearer intent.The success return at lines 80-83 could be moved to an
elseblock following the try-except pattern. This makes the happy path more explicit and is more idiomatic Python.🔎 Proposed refactor using else block
def _perform_health_check(self) -> Tuple[bool, str]: """Perform a health check by querying cluster health.""" try: response = self._make_request("GET", "_cluster/health", timeout=10) + except requests.exceptions.HTTPError as e: + if e.response.status_code == 401: + return ( + False, + "Elasticsearch authentication failed. Check your API key or credentials.", + ) + elif e.response.status_code == 403: + return ( + False, + "Elasticsearch access denied. Ensure your credentials have cluster access.", + ) + else: + return ( + False, + f"Elasticsearch API error: {e.response.status_code} - {e.response.text}", + ) + except requests.exceptions.ConnectionError: + return ( + False, + f"Failed to connect to Elasticsearch at {self.elasticsearch_config.url}", + ) + except requests.exceptions.Timeout: + return False, "Elasticsearch health check timed out" + except Exception as e: + return False, f"Elasticsearch health check failed: {str(e)}" + else: cluster_name = response.get("cluster_name", "unknown") status = response.get("status", "unknown") return ( True, f"Connected to Elasticsearch cluster '{cluster_name}' (status: {status})", ) - except requests.exceptions.HTTPError as e: - if e.response.status_code == 401: - return ( - False, - "Elasticsearch authentication failed. Check your API key or credentials.", - ) - elif e.response.status_code == 403: - return ( - False, - "Elasticsearch access denied. Ensure your credentials have cluster access.", - ) - else: - return ( - False, - f"Elasticsearch API error: {e.response.status_code} - {e.response.text}", - ) - except requests.exceptions.ConnectionError: - return ( - False, - f"Failed to connect to Elasticsearch at {self.elasticsearch_config.url}", - ) - except requests.exceptions.Timeout: - return False, "Elasticsearch health check timed out" - except Exception as e: - return False, f"Elasticsearch health check failed: {str(e)}"tests/generate_eval_report.py (1)
1354-1358: Prefix unused variables with underscore.The
hour,minute, andsecondvariables are unpacked but never used. Prefix them with underscores to indicate they are intentionally ignored.Proposed fix
if history_match: # History file - use date as title, add ⚡ for fast benchmarks - year, month, day, hour, minute, second = history_match.groups() + year, month, day, _hour, _minute, _second = history_match.groups() date_obj = datetime(int(year), int(month), int(day)).github/workflows/eval-benchmarks.yaml (1)
152-161: Existing PR body/title is not updated.When an existing PR is found, the workflow only logs a message but doesn't update the PR body with the new benchmark type and command information. Consider updating the PR with the latest run details.
Proposed fix
# Check if PR already exists EXISTING_PR=$(gh pr list --head "$BRANCH_NAME" --json number --jq '.[0].number') if [ -n "$EXISTING_PR" ]; then echo "Updating existing PR #$EXISTING_PR" + gh pr edit "$EXISTING_PR" \ + --title "$PR_TITLE" \ + --body "$PR_BODY" else gh pr create \ --title "$PR_TITLE" \ --body "$PR_BODY" \ --base master \ --head "$BRANCH_NAME" fi
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/eval-benchmarks.yamlconftest.pyholmes/plugins/toolsets/__init__.pyholmes/plugins/toolsets/elasticsearch/elasticsearch.pyholmes/plugins/toolsets/grafana/common.pytests/generate_eval_report.py
💤 Files with no reviewable changes (1)
- holmes/plugins/toolsets/grafana/common.py
✅ Files skipped from review due to trivial changes (1)
- conftest.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting with configuration in pyproject.toml
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks on Python files
Files:
holmes/plugins/toolsets/elasticsearch/elasticsearch.pyholmes/plugins/toolsets/__init__.pytests/generate_eval_report.py
holmes/plugins/toolsets/**/*.{py,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
holmes/plugins/toolsets/**/*.{py,yaml}: All toolsets MUST return detailed error messages from underlying APIs to enable LLM self-correction, including exact query/command executed, time ranges/parameters/filters used, and full API error response (status code and message)
For 'no data' responses in toolsets, specify what was searched and where
Never return unbounded data from APIs - always include filter parameters on tools that query collections
Files:
holmes/plugins/toolsets/elasticsearch/elasticsearch.pyholmes/plugins/toolsets/__init__.py
holmes/plugins/toolsets/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
holmes/plugins/toolsets/**/*.py: Use requests library for HTTP calls in Python toolsets, not specialized client libraries like opensearchpy
Implement simple Pydantic config class with validation for Python toolsets
Include health check in prerequisites_callable() method for Python toolsets
Each tool in Python toolsets should be a thin wrapper around a single API endpoint
Use JsonFilterMixin for client-side filtering when server-side filtering is not possible, adding max_depth and jq parameters
Maintain backwards compatibility in toolset config using Pydantic's extra='allow' when renaming config fields
Only define current field names in toolset config schema with extra='allow' to avoid polluting model_dump() output with deprecated fields
Use @model_validator(mode='after') in toolset config to map old field names to new names and log deprecation warnings
Bash toolset validates commands for safety
Files:
holmes/plugins/toolsets/elasticsearch/elasticsearch.pyholmes/plugins/toolsets/__init__.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Tests should match source structure under tests/
Live execution is now enabled by default to ensure tests match real-world behavior
Files:
tests/generate_eval_report.py
🧠 Learnings (14)
📚 Learning: 2026-01-05T11:14:20.209Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.209Z
Learning: Add required environment variables to .github/workflows/eval-regression.yaml 'Run tests' step when adding evals for new cloud service integrations
Applied to files:
.github/workflows/eval-benchmarks.yaml
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Use requests library for HTTP calls in Python toolsets, not specialized client libraries like opensearchpy
Applied to files:
holmes/plugins/toolsets/elasticsearch/elasticsearch.pyholmes/plugins/toolsets/__init__.py
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Implement simple Pydantic config class with validation for Python toolsets
Applied to files:
holmes/plugins/toolsets/elasticsearch/elasticsearch.pyholmes/plugins/toolsets/__init__.py
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Each tool in Python toolsets should be a thin wrapper around a single API endpoint
Applied to files:
holmes/plugins/toolsets/elasticsearch/elasticsearch.pyholmes/plugins/toolsets/__init__.py
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Only define current field names in toolset config schema with extra='allow' to avoid polluting model_dump() output with deprecated fields
Applied to files:
holmes/plugins/toolsets/__init__.py
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Maintain backwards compatibility in toolset config using Pydantic's extra='allow' when renaming config fields
Applied to files:
holmes/plugins/toolsets/__init__.py
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Include health check in prerequisites_callable() method for Python toolsets
Applied to files:
holmes/plugins/toolsets/__init__.py
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to holmes/plugins/toolsets/**/*.yaml : Toolsets should be YAML files in holmes/plugins/toolsets/{name}.yaml or {name}/ directory structure
Applied to files:
holmes/plugins/toolsets/__init__.py
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to holmes/plugins/toolsets/**/*.{py,yaml} : Never return unbounded data from APIs - always include filter parameters on tools that query collections
Applied to files:
holmes/plugins/toolsets/__init__.py
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to holmes/plugins/toolsets/**/*.{py,yaml} : For 'no data' responses in toolsets, specify what was searched and where
Applied to files:
holmes/plugins/toolsets/__init__.py
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to holmes/plugins/toolsets/**/*.{py,yaml} : All toolsets MUST return detailed error messages from underlying APIs to enable LLM self-correction, including exact query/command executed, time ranges/parameters/filters used, and full API error response (status code and message)
Applied to files:
holmes/plugins/toolsets/__init__.py
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to holmes/plugins/toolsets/**/kubernetes*.py : RBAC permissions are respected for Kubernetes access
Applied to files:
holmes/plugins/toolsets/__init__.py
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Bash toolset validates commands for safety
Applied to files:
holmes/plugins/toolsets/__init__.py
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to **/*.py : Always place Python imports at the top of the file, not inside functions or methods
Applied to files:
tests/generate_eval_report.py
🧬 Code graph analysis (2)
holmes/plugins/toolsets/elasticsearch/elasticsearch.py (3)
holmes/plugins/toolsets/utils.py (1)
toolset_name_for_one_liner(232-236)holmes/plugins/toolsets/json_filter_mixin.py (2)
JsonFilterMixin(51-134)extend_parameters(68-73)holmes/core/tools.py (1)
ToolParameter(141-147)
holmes/plugins/toolsets/__init__.py (2)
holmes/plugins/toolsets/elasticsearch/elasticsearch.py (2)
ElasticsearchClusterToolset(810-830)ElasticsearchDataToolset(789-807)holmes/plugins/toolsets/elasticsearch/opensearch_query_assist.py (1)
OpenSearchQueryAssistToolset(55-78)
🪛 Ruff (0.14.10)
holmes/plugins/toolsets/elasticsearch/elasticsearch.py
80-83: Consider moving this statement to an else block
(TRY300)
tests/generate_eval_report.py
1356-1356: Unpacked variable hour is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
1356-1356: Unpacked variable minute is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
1356-1356: Unpacked variable second is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ 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: run-benchmarks
- GitHub Check: llm_evals
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
🔇 Additional comments (12)
holmes/plugins/toolsets/__init__.py (1)
31-37: LGTM! Clean import consolidation.The imports are properly placed at the top of the file and follow alphabetical ordering. Consolidating these imports (removing duplication as noted in the summary) improves maintainability.
holmes/plugins/toolsets/elasticsearch/elasticsearch.py (2)
502-531: Excellent integration of JsonFilterMixin for the mappings tool.The
ElasticsearchMappingsclass now properly integratesJsonFilterMixin, enabling users to filter large mapping results usingmax_depthandjqparameters. The implementation correctly:
- Inherits from
JsonFilterMixin- Extends parameters using
JsonFilterMixin.extend_parameters- Applies filtering via
self.filter_resultThis follows the coding guidelines for using JsonFilterMixin for client-side filtering when needed.
682-777: Excellent integration of JsonFilterMixin for the list indices tool.The
ElasticsearchListIndicesclass now properly integratesJsonFilterMixin, providing both server-side filtering (via Elasticsearch's own parameters) and client-side filtering (viamax_depthandjq). The implementation is comprehensive and follows best practices.tests/generate_eval_report.py (4)
12-20: LGTM!Import path update looks correct.
232-237: LGTM!The new
--benchmark-typeargument is well-defined with clear choices and appropriate default.
1333-1343: LGTM!The benchmark type handling logic is clear. Using
getattris slightly defensive sincebenchmark_typeis defined inparse_args()with a default, but it doesn't cause any issues.
1443-1461: LGTM!The benchmark info boxes use MkDocs admonition syntax correctly and provide helpful context about each benchmark type.
.github/workflows/eval-benchmarks.yaml (5)
7-30: LGTM!The workflow inputs are well-structured with clear descriptions. The weekly schedule at 2 AM UTC on Sundays is appropriate for automated benchmarking.
94-106: LGTM!The evaluation step correctly sets up environment variables and runs the constructed command. Using
github.run_idfor the experiment ID ensures unique tracking.
107-116: LGTM!The artifact upload captures all relevant result files and uses
if: always()to ensure results are preserved even if tests fail.
127-129: Same-day runs will overwrite each other.The branch name uses only the date (
%Y%m%d), so multiple workflow runs on the same day will force-push to the same branch, potentially losing intermediate results. This is likely acceptable for weekly scheduled runs but could be surprising for manual dispatch.If preserving results from multiple same-day runs is important, consider adding a run number or timestamp:
BRANCH_NAME="automated/benchmark-$(date +%Y%m%d-%H%M%S)"
70-74: The current quoting strategy is correct and requires no changes.The escaped quotes
\"$CUSTOM_MARKERS\"on line 71 are necessary and work correctly. When this command is output to$GITHUB_OUTPUTand later executed via${{ steps.build-command.outputs.command }}, the quotes are preserved and allow pytest markers with spaces (e.g.,regression or benchmark,easy and kubernetes) to be passed as a single argument.Testing confirms:
- With quotes:
./run_benchmarks_local.py --markers "regression or benchmark"→ Correctly parsed by argparse- Without quotes:
./run_benchmarks_local.py --markers regression or benchmark→ Fails with "unrecognized arguments" errorThe proposed fixes in the review would introduce problems rather than solve them. The documentation in
running-evals.mdalso shows the expected usage with quotes:./run_benchmarks_local.py --markers "easy and kubernetes".
Signed-off-by: Tomer Keshet <tomer@robusta.dev>
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @.github/workflows/eval-benchmarks.yaml:
- Around line 118-162: When an existing PR is found the script only echoes
"Updating existing PR" and does not refresh its metadata; after detecting
EXISTING_PR populate PR_TITLE and PR_BODY as already computed and call the
GitHub CLI to update the PR (use gh pr edit with --title and --body and the PR
number stored in EXISTING_PR) so the title/body reflect the latest
BENCHMARK_TYPE and command outputs; ensure you reference EXISTING_PR when
calling gh pr edit and preserve the --base/--head behavior used when creating a
new PR.
🧹 Nitpick comments (1)
run_benchmarks_local.py (1)
262-279: Consider simplifying the redirect path construction.The current path construction works but is unnecessarily complex:
history_relative = f"../history/{history_output.name.replace('.md', '/')}".rstrip("/") + "/"🔎 Simpler alternative
- history_relative = ( - f"../history/{history_output.name.replace('.md', '/')}".rstrip("/") - + "/" - ) + history_relative = f"../history/{history_output.stem}/"Using
.stem(filename without extension) is clearer and achieves the same result.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/eval-benchmarks.yamlrun_benchmarks_local.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
Use Ruff for formatting and linting with configuration in pyproject.toml
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks on Python files
Files:
run_benchmarks_local.py
🧠 Learnings (1)
📚 Learning: 2026-01-05T11:14:20.209Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.209Z
Learning: Add required environment variables to .github/workflows/eval-regression.yaml 'Run tests' step when adding evals for new cloud service integrations
Applied to files:
.github/workflows/eval-benchmarks.yaml
🪛 Ruff (0.14.10)
run_benchmarks_local.py
248-248: subprocess call: check for execution of untrusted input
(S603)
259-259: subprocess call: check for execution of untrusted input
(S603)
⏰ 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: run-benchmarks
- GitHub Check: build (3.11)
- GitHub Check: llm_evals
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
🔇 Additional comments (13)
run_benchmarks_local.py (8)
17-21: LGTM! Clean benchmark type definitions.The constant provides a clear mapping between benchmark types and their pytest markers, improving maintainability.
36-36: LGTM! Proper parameter addition.The
benchmark_typeparameter is correctly typed, positioned, and stored.Also applies to: 45-45
56-57: LGTM! Clear status display.The conditional output provides useful feedback about which benchmark type is running.
209-249: LGTM! Well-structured report generation.The method correctly:
- Selects output files based on benchmark type
- Validates the report script exists before running
- Sets up PYTHONPATH for module imports
- Passes benchmark type to the report generator
The Ruff S603 warnings on subprocess calls are false positives—the script path is validated and arguments come from internal state.
290-313: LGTM! Consistent benchmark type handling.The summary correctly displays the benchmark type and selects the appropriate result file, mirroring the logic in
generate_report.
386-399: LGTM! Well-designed CLI argument.The
--benchmark-typeargument:
- Uses
choicesfor validation- Defaults to None, allowing
main()to apply conditional defaults- Clearly documents mutual exclusivity with
--markers
443-461: LGTM! Robust mutual exclusivity validation.The logic correctly:
- Validates that
--benchmark-typeand--markersaren't both provided- Defaults to
fast-benchmarkwhen neither is specified- Derives markers from
BENCHMARK_TYPESwhen using benchmark types
466-475: LGTM! Proper parameter passing.The
BenchmarkRunnerinstantiation correctly passes bothmarkersandbenchmark_typebased on the validated logic..github/workflows/eval-benchmarks.yaml (5)
7-26: LGTM! Well-structured workflow inputs.The inputs provide clear options:
benchmark_typeuses choice validation with appropriate defaults- Descriptions clearly document mutual exclusivity
- Empty string defaults enable proper conditional logic downstream
28-30: LGTM! Appropriate weekly schedule.Sunday at 2 AM UTC is a sensible time for automated weekly benchmark runs.
57-92: Verify command quoting with custom markers.The command building logic is well-structured, but line 71 uses escaped quotes:
CMD="$CMD --markers \"$CUSTOM_MARKERS\""When
CUSTOM_MARKERScontains spaces (e.g., "easy or medium"), the escaped quotes should preserve the argument correctly when executed on line 105. However, this quoting pattern can be fragile.Since workflow inputs are trusted and the script validates them, this is likely acceptable. You may want to test a manual workflow run with custom markers containing spaces to confirm the quoting works as expected.
If issues arise, consider building the command without embedded quotes and letting the script handle the parsing, or use array-based command building.
104-105: LGTM! Simplified execution.Directly running the built command reduces complexity and improves traceability of what was executed.
107-116: LGTM! Comprehensive artifact upload.Uploading both fast and full benchmark result files ensures artifacts are captured regardless of which benchmark type ran.
Signed-off-by: Tomer Keshet <tomer@robusta.dev>
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/eval-benchmarks.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-05T11:14:20.209Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.209Z
Learning: Add required environment variables to .github/workflows/eval-regression.yaml 'Run tests' step when adding evals for new cloud service integrations
Applied to files:
.github/workflows/eval-benchmarks.yaml
🔇 Additional comments (3)
.github/workflows/eval-benchmarks.yaml (3)
154-158: Great fix for updating existing PR metadata!The previous review comment correctly identified that existing PRs weren't being updated. Lines 156-158 now properly call
gh pr editto refresh the title and body when a PR already exists for the day's benchmark branch.
70-74: Script correctly enforces mutual exclusivity.The
run_benchmarks_local.pyscript validates mutual exclusivity at lines 443–447 by checking if both--markersand--benchmark-typeare provided and exiting with an error if so. The workflow's if/elif structure (lines 70–74) provides an additional layer by prioritizingCUSTOM_MARKERSwhen both environment variables are set, but the script's explicit validation confirms conflicting inputs are rejected. The comment on line 69 is accurate.
113-116: No action required. Theactions/upload-artifact@v4action handles missing files gracefully by default. When some paths in the list don't exist, it uploads the files that do exist and warns about the missing ones without failing the step (defaultif-no-files-found: warnbehavior). Since only one offast-benchmark-results.mdorfull-benchmark-results.mdwill exist depending on the benchmark type, the workflow will succeed and upload the available files as expected.
2 participants
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.