Conversation
📂 Previous Runs📜 Run @ 46fb701 (#22390682808)✅ Results of HolmesGPT evalsAutomatically triggered by commit 46fb701 on branch Results of HolmesGPT evals
📜 Run @ b49d93f (#22390629266)✅ Results of HolmesGPT evalsAutomatically triggered by commit b49d93f on branch Results of HolmesGPT evals
📜 Run @ 11e989b (#22390350645)✅ Results of HolmesGPT evalsAutomatically triggered by commit 11e989b on branch Results of HolmesGPT evals
📜 Run @ 6e3feb9 (#22105928410)✅ Results of HolmesGPT evalsAutomatically triggered by commit 6e3feb9 on branch Results of HolmesGPT evals
📜 Run @ 2c269f9 (#21819218454)✅ Results of HolmesGPT evalsAutomatically triggered by commit 2c269f9 on branch Results of HolmesGPT evals
✅ Results of HolmesGPT evalsAutomatically triggered by commit 1cdde30 on branch Results of HolmesGPT evals
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" Option 3: Add PR labels to include extra evals in automatic regression runs:
Examples: 🏷️ Valid tags
Commands: CLI: |
|
✅ Docker images ready for
Use these tags to pull the images for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:27f69856
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:27f69856 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:27f69856
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:27f69856
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:27f69856
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:27f69856 me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:27f69856
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:27f69856Patch 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:27f69856 \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:27f69856Robusta 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:27f69856 \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:27f69856 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSplit Poetry installation out from combined steps across CI workflows and a composite setup action; switched installs to read version from Changes
Sequence Diagram(s)(omitted — CI/workflow configuration changes do not introduce a new multi-component runtime control flow requiring visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/build-binaries-and-brew.yaml (2)
19-24:⚠️ Potential issue | 🟠 MajorOutdated action versions in the build job.
actions/checkout@v2(line 19) andactions/setup-python@v2(line 22) are flagged by actionlint as too old. Other workflows in this PR use@v4/@v5. Update these for consistency.
148-174:⚠️ Potential issue | 🟠 MajorOutdated actions and deprecated
set-outputcommands in downstream jobs.Static analysis flags several issues in the
mac-hash,linux-hash, andupdate-formulajobs:
actions/checkout@v2on lines 149, 167, 183 — update to@v4.::set-outputon lines 156 and 174 is deprecated — useecho "{name}={value}" >> $GITHUB_OUTPUTinstead.Proposed fix for set-output (lines 156, 174)
- run: echo "::set-output name=MAC_BUILD_HASH::$(sha256sum holmes-macos-latest-${{ github.ref_name }}.zip | awk '{print $1}')" + run: echo "MAC_BUILD_HASH=$(sha256sum holmes-macos-latest-${{ github.ref_name }}.zip | awk '{print $1}')" >> $GITHUB_OUTPUT- run: echo "::set-output name=LINUX_BUILD_HASH::$(sha256sum holmes-ubuntu-22.04-${{ github.ref_name }}.zip | awk '{print $1}')" + run: echo "LINUX_BUILD_HASH=$(sha256sum holmes-ubuntu-22.04-${{ github.ref_name }}.zip | awk '{print $1}')" >> $GITHUB_OUTPUT
🤖 Fix all issues with AI agents
In @.github/workflows/build-and-test.yaml:
- Around line 22-27: Update the GitHub Actions step versions: replace the uses
reference for the checkout action (actions/checkout@v2) with the newer major
(actions/checkout@v4) and replace the setup-python action
(actions/setup-python@v2) with the newer major (actions/setup-python@v5) so they
match other workflows; keep the existing step names and the
matrix.python-version input unchanged (the steps referencing actions/checkout
and actions/setup-python should be edited to use the `@v4` and `@v5` tags
respectively).
In @.github/workflows/cli-performance.yaml:
- Around line 74-95: The workflow fails because asdf reads .tool-versions which
isn't present on master; modify the steps around "Checkout benchmark script from
PR" / "Copy benchmark script" so that the PR's .tool-versions is also retrieved
and placed at the repository root before the "Install asdf and Poetry"
step—either add .tool-versions to the sparse-checkout in the checkout step or
add a new copy step to copy pr-scripts/.tool-versions to ./ (root) so that the
asdf-vm/actions/install@v4 step can find it.
In `@CONTRIBUTING.md`:
- Around line 10-20: Update the Python requirement text that currently states
"Python `3.11`" in CONTRIBUTING.md to match the CI and setup action: either
change it to the recommended version `3.12` (used by setup-holmes-env) or
replace the single-version line with a note that multiple versions are supported
(e.g., 3.10–3.12) to align with build-and-test.yaml; ensure the updated wording
explicitly mentions compatibility with the setup-holmes-env action and the
build-and-test.yaml matrix so readers know which versions are supported.
🧹 Nitpick comments (5)
.github/actions/setup-holmes-env/action.yml (1)
9-12: Poetry version default duplicates.tool-versions— risk of drift.The default
2.3.2here and the value in.tool-versionsmust be kept in sync manually. If one is updated without the other, workflows using this composite action directly vs. those relying on.tool-versionswill diverge. Consider reading the version from.tool-versionsat runtime or adding a comment warning maintainers to update both locations..github/workflows/publish-pypi.yaml (1)
19-23: Relies on.tool-versionsfile (unlike the composite action which inlines the version).This is fine since checkout happens before asdf install, but note the inconsistency:
setup-holmes-env/action.ymlpassestool_versionsinline while this workflow reads from the file. Both approaches work, but maintainers should be aware that updating Poetry version requires changes in multiple places (.tool-versions, the composite action default, andbuild-binaries-and-brew.yamlWindows step)..github/workflows/build-and-test.yaml (1)
58-61:poetry runwithvirtualenvs.create false— works but is redundant.Since line 37 disables virtualenv creation,
poetry run pytestjust delegates to the system Python where packages are already installed. Usingpytestdirectly would be equivalent and clearer. This is minor and optional..github/workflows/build-binaries-and-brew.yaml (1)
37-43: Hardcoded Poetry version on Windows — third location to maintain.
pip install poetry==2.3.2is now a third place where the Poetry version is specified (alongside.tool-versionsand thesetup-holmes-envaction default). If any one is updated without the others, Windows builds will use a different Poetry version.Consider extracting this into a workflow-level
envvariable or reading it from.tool-versionsat runtime:Example: parse from .tool-versions
- name: Install dependencies (Windows) if: matrix.os == 'windows-latest' run: | python -m pip install --upgrade pip setuptools pyinstaller - pip install poetry==2.3.2 + POETRY_VERSION=$(grep '^poetry ' .tool-versions | awk '{print $2}') + pip install poetry==$POETRY_VERSION poetry config virtualenvs.create false poetry install --no-rootNote: This would need PowerShell syntax since it's Windows, or add
shell: bashto use Git Bash..github/workflows/cli-performance.yaml (1)
128-212: Consider extracting the inline Python comparison script into a committed file.The inline Python heredoc spanning ~75 lines is functional but hard to test, lint, and maintain. Since you already commit
scripts/cli_performance_benchmark.py, a companionscripts/cli_performance_compare.pywould allow local testing, linting, and reuse.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.github/workflows/build-binaries-and-brew.yaml:
- Around line 148-156: Update the deprecated actions/checkout@v2 step to use
actions/checkout@v4 and replace the deprecated ::set-output usage in the
calc-hash step (id: calc-hash) by writing the MAC_BUILD_HASH to the
$GITHUB_OUTPUT file (e.g., echo "MAC_BUILD_HASH=$(sha256sum ... | awk '{print
$1}')" >> $GITHUB_OUTPUT) instead of using ::set-output; ensure the Download
MacOS artifact step name remains unchanged and the filename interpolation
holmes-macos-latest-${{ github.ref_name }}.zip is preserved.
- Around line 182-201: The commit step uses a shell variable TAG_NAME that was
defined only inside the "Update holmesgpt.rb formula" run block and therefore is
empty in the separate "Commit and push changes" step; also actions/checkout@v2
is deprecated. Fix by either merging the two run blocks so TAG_NAME (and
MAC_BUILD_HASH/LINUX_BUILD_HASH) remain in scope for the git commit, or replace
TAG_NAME in the commit command with the expression ${{ github.ref_name }}; and
update the checkout action to a supported version (e.g., actions/checkout@v4).
Update references in the "Update holmesgpt.rb formula" and "Commit and push
changes" steps accordingly.
- Around line 166-174: The linux-hash job uses deprecated patterns: replace the
checkout step's uses: actions/checkout@v2 with actions/checkout@v4, and stop
using the deprecated ::set-output in the Calculate hash step (id=calc-hash);
instead compute the SHA256 and write the output into GITHUB_OUTPUT (e.g. echo
"LINUX_BUILD_HASH=$(sha256sum holmes-ubuntu-22.04-${{ github.ref_name }}.zip |
awk '{print $1}')" >> $GITHUB_OUTPUT). Keep the existing Download Linux artifact
step (actions/download-artifact@v4) as-is and ensure the Calculate hash step
uses the calc-hash id so downstream steps can reference the job output.
In @.github/workflows/cli-performance.yaml:
- Around line 128-212: The script assumes pr_startup/master_startup are non-null
and calls format_benchmark_table(pr_startup, master_startup) which will crash if
load_json returned None; update the logic around load_json,
format_benchmark_table, report_lines and status computation to guard those
values: check pr_startup and master_startup before calling
format_benchmark_table (similar to the existing pr_llm/master_llm guard), append
a clear "Skipped - startup benchmark missing" block to report_lines when either
is missing, set startup_diff to 0 or a neutral sentinel in that case and compute
status accordingly, and when writing the final summary line only reference
git_sha/iterations if pr_startup/master_startup are present (otherwise include a
safe placeholder) so the script never accesses dict keys on None and exits
cleanly.
🧹 Nitpick comments (3)
.github/workflows/build-and-test.yaml (1)
37-38: Inconsistent virtualenv configuration across workflows.This workflow uses
virtualenvs.create false, whilecli-performance.yamlusesvirtualenvs.create true+virtualenvs.in-project true. Consider standardizing the approach across workflows to reduce maintenance burden and confusion. If there's a reason for the difference (e.g., benchmarking needs isolation), a brief comment in the workflow would help..github/workflows/cli-performance.yaml (1)
245-251: Regression check doesn't handle the potentialerrorstatus.If the compare step outputs
status=error(e.g., due to missing data or a script failure), this check would pass silently since it only tests for"regression". Consider also failing on unexpected statuses..github/workflows/build-binaries-and-brew.yaml (1)
37-43: Poetry version on Windows is duplicated and can drift from.tool-versions.The Windows step hardcodes
poetry==2.3.2while Unix/Mac gets the version from.tool-versions. If the version is updated in.tool-versions, this line can easily be missed. Consider extracting it into a workflow-levelenvvariable or reading it from.tool-versionsat runtime.Example: read from `.tool-versions`
- name: Install dependencies (Windows) if: matrix.os == 'windows-latest' run: | python -m pip install --upgrade pip setuptools pyinstaller - pip install poetry==2.3.2 + $poetryVersion = (Select-String -Path .tool-versions -Pattern '^poetry\s+(.+)$').Matches.Groups[1].Value + pip install "poetry==$poetryVersion" poetry config virtualenvs.create false poetry install --no-root
|
CLI Performance Benchmark is expected to fail because it checks out master branch which does include .tool-versions |
🔬 CLI Performance Benchmark🟡 Startup Time (no LLM)Measures
🟡 Full CLI with LLMMeasures
PR: |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.github/workflows/build-and-deploy-docs.yaml:
- Around line 6-7: Remove the temporary pull_request workflow trigger by
deleting the pull_request key (and the accompanying TODO comment "#
TODO(mainred): to be deleted") from the workflow YAML so the workflow no longer
runs on PRs; ensure only the intended triggers remain in the file and validate
the YAML after removal.
- Around line 29-36: Uncomment the deployment step so docs are pushed on merges:
restore the previously commented run line containing "poetry run mkdocs
gh-deploy --force" and remove the comment markers so the job executes; also fix
the TODO comment text by replacing the full-width parenthesis with ASCII "(" and
correct "uncoment" to "uncomment" (the modified lines reference the commented
run step with "poetry run mkdocs gh-deploy --force" and the TODO comment above
it).
In @.github/workflows/build-binaries-and-brew.yaml:
- Around line 6-7: Remove the temporary pull_request trigger (the
"pull_request:" key and the TODO(mainred) comment) from the workflow so the job
only runs on the intended release event; locate and delete the "pull_request:"
entry in the workflow file (and the associated comment TODO(mainred)) leaving
the release: [created] trigger as the sole trigger.
- Around line 28-43: The workflow currently runs the "Install asdf and Poetry"
step using asdf-vm/actions/install@v4 on all OSes and then uses a hardcoded
Poetry path in the Windows "Install dependencies" step; add a conditional to the
asdf step (e.g., if: matrix.os != 'windows-latest') so asdf is not executed on
Windows, and update the Windows branch of the "Install dependencies" step to
install Poetry via a Windows-compatible method (pipx or the official Windows
installer) instead of referencing
C:\Users\runneradmin\AppData\Roaming\Python\Scripts\poetry; ensure the step
names "Install asdf and Poetry" and both "Install dependencies" branches reflect
these conditionals and the alternative Poetry install so the Windows job no
longer depends on asdf or the hardcoded path.
🧹 Nitpick comments (1)
.github/workflows/cli-performance.yaml (1)
31-34: Inconsistent Poetry virtualenv configuration across workflows.These jobs use
virtualenvs.create true+virtualenvs.in-project true, while the sharedsetup-holmes-envaction (used by other workflows) setsvirtualenvs.create false. This means:
- Here: Poetry creates a
.venvinside the project directory.- Other workflows: Poetry installs directly into the system Python.
This inconsistency can lead to different dependency resolution behavior and subtle bugs. If there's a specific reason for in-project virtualenvs here (e.g., benchmark isolation), add a comment explaining it. Otherwise, align with the shared action's approach.
Also applies to: 92-95
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/build-binaries-and-brew.yaml:
- Line 121: The release workflow has had the release upload, hash calculation,
and Homebrew formula update jobs removed (replaced by a TODO); restore the
original jobs (e.g., the release upload job, hash calculation job, and
update_homebrew/Homebrew formula job) or block merging until this TODO is
resolved: reintroduce the job definitions and their artifacts/outputs, ensure
they still run on the release trigger (release: [created]), wire the
dependencies between them (upload -> calc_hashes -> update_homebrew), and
include any necessary secrets/permissions and artifact upload/download steps so
the release pipeline behaves as before.
- Around line 88-104: In the pyinstaller invocation block (the "pyinstaller
holmes_cli.py" command), fix the indentation of the "ls dist" line so it uses
the same 8-space indent as the rest of the command block, and add the missing
flag --hidden-import=backports to the pyinstaller flags (alongside existing
--hidden-import entries like --hidden-import=tiktoken_ext.openai_public and
--hidden-import=tiktoken_ext) to keep this workflow in sync with the other
workflow.
🧹 Nitpick comments (2)
.github/actions/setup-holmes-env/action.yml (1)
31-37: Add error handling for Poetry version extraction.If
.tool-versionsis missing or doesn't contain apoetryline,POETRY_VERSIONwill be empty and the installer will be invoked with--version "", likely producing a confusing failure. This is especially relevant since this is a reusable composite action that could be called from contexts where.tool-versionsmight not be present (e.g., if the checkout step failed or the file was excluded via sparse-checkout).Proposed fix
- name: Install Poetry shell: bash run: | POETRY_VERSION=$(grep '^poetry ' .tool-versions | awk '{print $2}') + if [ -z "$POETRY_VERSION" ]; then + echo "::error::Could not determine Poetry version from .tool-versions" + exit 1 + fi echo "Installing Poetry version: $POETRY_VERSION" curl -sSL https://install.python-poetry.org | python3 - --version "$POETRY_VERSION" echo "$HOME/.local/bin" >> $GITHUB_PATH.github/workflows/build-and-test.yaml (1)
28-33: Same missing guard onPOETRY_VERSIONextraction as the composite action.If the
grepdoesn't match, the install will proceed with an empty--versionargument. Consider adding the same guard suggested forsetup-holmes-env/action.yml. Also, the duplicated Poetry install snippet across multiple workflows is a good candidate for consolidation — this workflow could potentially use thesetup-holmes-envcomposite action instead of inlining these steps.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/actions/setup-holmes-env/action.yml:
- Around line 26-30: In the "Install Python dependencies" workflow step remove
the debug artifact by deleting the stray `ls ../../..` command from the run
block so the step only runs the intended Python install commands (the block
under the step name "Install Python dependencies" that currently contains `ls
../../..` and `python -m pip install --upgrade pip setuptools pyinstaller`
should be left with only the python install line).
🧹 Nitpick comments (2)
.github/workflows/build-and-deploy-docs.yaml (2)
19-21: Python version is hardcoded — consider reading from.tool-versionsfor consistency.Other workflows in this PR are moving toward asdf-managed versions. The Python version
3.11is hardcoded here while the broader initiative is to centralize tool versions in.tool-versions. If Python is later added to that file, this will drift silently.Low priority since
.tool-versionscurrently only tracks Poetry.
29-30: Step name "Install Poetry" is accurate for current state but will mislead if.tool-versionsgrows.Currently,
.tool-versionscontains onlypoetry 2.3.2, so the step name is correct. However, if.tool-versionsis extended later (e.g., with Python or other tools),asdf-vm/actions/install@v4will install all listed tools, making the step name misleading. Consider either:
- Renaming the step to reflect what it actually does (e.g., "Install tools via asdf"), or
- Using the
tool_versionsinput to be explicit about what gets installed, overriding.tool-versionsentirely for that job.Note: The action does not support selective tool filtering—the
tool_versionsinput requires specifying the complete list of tools to install.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.github/workflows/build-binaries-and-brew.yaml:
- Line 21: Update the GitHub Actions runner label in the workflow by replacing
the deprecated os: macos-13 entry with a supported runner (e.g., os: macos-14 or
os: macos-15); if you need Intel builds use os: macos-15-intel or choose os:
macos-latest. Locate the os: macos-13 line in the build-binaries-and-brew.yaml
workflow and change it to the appropriate supported label so jobs targeting
macOS no longer fail.
In @.github/workflows/cli-performance.yaml:
- Around line 74-76: Restore the master checkout in the benchmark workflow:
re-enable the commented `ref: master` under the `benchmark-master` job so it
actually checks out the master branch (instead of the PR), and ensure the
workflow copies `.tool-versions` from the PR checkout into the master checkout
before running benchmarks; specifically modify the `benchmark-master` job to
perform a checkout of master (use `ref: master`) and then add a step to copy
`.tool-versions` from the PR workspace into the master workspace prior to
running the benchmark commands.
In @.github/workflows/publish-pypi.yaml:
- Line 6: Remove the temporary pull_request trigger from the workflow by
deleting the top-level "pull_request:" key so the publish job no longer runs on
every PR; ensure the workflow only includes the intended triggers (e.g., "push:"
or "workflow_dispatch:") and matches the patterns used in other publish
workflows to avoid accidental PR-triggered publishes.
- Around line 30-41: Restore the two commented steps "Update package version"
and "Publish to PyPI": re-enable the step that runs the two sed commands
updating __version__ in holmes/__init__.py and version in pyproject.toml and the
step that runs `poetry publish --build -u __token__ -p ${{ secrets.PYPI_TOKEN
}}`; while restoring, fix the YAML indentation under each `run: |` so the sed
lines and publish command are indented two spaces (not one) and validate the
workflow parses, and optionally gate the merge by adding a check or comment to
ensure PYPI_TOKEN secret exists before merging.
🧹 Nitpick comments (2)
.github/workflows/build-and-deploy-docs.yaml (1)
29-34: Inconsistent Poetry installation method compared to other workflows.This workflow uses
asdf-vm/actions/install@v4whilebuild-and-test.yaml,publish-pypi.yaml,cli-performance.yaml, andbuild-binaries-and-brew.yamlall use a script-basedcurl+grep .tool-versionsapproach. Pick one method and use it consistently across all workflows to reduce maintenance burden..github/workflows/cli-performance.yaml (1)
34-38: Virtualenv configuration differs from other workflows.This workflow uses
virtualenvs.create true+virtualenvs.in-project true, while other workflows (docs, build-and-test, publish-pypi, setup action) usevirtualenvs.create false. The inconsistency may be intentional for benchmark isolation, but if not, it adds confusion.
ab83a9a to
6ee3c7e
Compare
Signed-off-by: Qingchuan Hao <qingchuan.hao@microsoft.com>
mainred
changed the title
|
@moshemorad could you please take a look? Not sure you are working on the same task. |
Signed-off-by: Qingchuan Hao <qingchuan.hao@microsoft.com>
|
@moshemorad could you please take a look? Thanks. I just resolved several code conflicts. If it's not planned, I can close this PR. |
|
cc @arikalon1 |
|
Close for not plannned |
2 participants
This PR introduces asdf and asdf-vm/actions/install github action to manage the tool versions synced in both local dev env and github action env.
Besides the PR triggered check-ins, I have also triggered Publish to PyPI and Build and Release, but please forget the build error caused by the wrong merge request file name