Repository navigation
Enable parallel test execution with pytest-xdist - #1621
Merged
Merged
Conversation
Add `-n auto` and `--dist loadgroup` to the default pytest addopts so all test runs (local and CI) automatically use parallel workers. Remove the shared `log_file` setting which would corrupt under concurrent writes from multiple xdist workers. pytest-xdist was already a dev dependency but was only used for LLM eval tests via explicit `-n 6` flags. This change makes parallelism the default for every pytest invocation. https://claude.ai/code/session_016bAG1p7sRsKkYvgoBr8fcz Signed-off-by: Claude <noreply@anthropic.com>
Contributor
|
✅ 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:53505fe0
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:53505fe0 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:53505fe0
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:53505fe0
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:53505fe0
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:53505fe0 me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:53505fe0
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:53505fe0Patch 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:53505fe0 \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:53505fe0Robusta 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:53505fe0 \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:53505fe0 |
Contributor
📂 Previous Runs📜 Run @ 46d8ec7 (#22280553116)✅ Results of HolmesGPT evalsAutomatically triggered by commit 46d8ec7 on branch Results of HolmesGPT evals
📜 Run @ c4b3e7c (#22280452201)✅ Results of HolmesGPT evalsAutomatically triggered by commit c4b3e7c on branch 📜 Run @ 861a8a8 (#22280393131)✅ Results of HolmesGPT evalsAutomatically triggered by commit 861a8a8 on branch Results of HolmesGPT evals
✅ Results of HolmesGPT evalsAutomatically triggered by commit e4089ec 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: |
Contributor
WalkthroughEnabled pytest-xdist parallelism and removed shared file logging in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant PR as "PR / push"
participant Check as "check-changes"
participant Test as "test job"
participant BuildBin as "build-binary job"
participant Gate as "build-and-test-gate"
PR->>Check: run check-changes
Check->>Test: trigger test job (if changes)
Check->>BuildBin: trigger binary build (if changes)
Test->>Gate: upload test results & status
BuildBin->>Gate: upload binary build status
Gate->>PR: final combined status (pass/fail)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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. |
Contributor
🔬 CLI Performance Benchmark🟡 Startup Time (no LLM)Measures
🟡 Full CLI with LLMMeasures
PR: |
- Split the single `build` job into `test` (3x Python matrix) and `build-binary` (single Python 3.11) that run in parallel - Add `actions/cache` for Poetry installation and pip/Poetry dependency caches, keyed on Python version + poetry.lock hash - PyInstaller binary now builds once instead of 3x (was redundantly building on every Python version in the matrix) - Tests no longer block on PyInstaller completion and vice versa - Gate job updated to require both `test` and `build-binary` jobs https://claude.ai/code/session_016bAG1p7sRsKkYvgoBr8fcz Signed-off-by: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pyproject.toml (1)
174-175: Consider capping worker count with--maxprocessesfor CI resource control
-n autowill spawn one worker per available CPU. On CI runners with 32+ cores this can result in significant memory pressure, especially if each worker loads a full application import graph. Adding--maxprocesses=<N>provides an upper bound without disabling parallelism.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 174 - 175, The pytest invocation uses "-n", "auto" (with "--dist", "loadgroup"), which can spawn too many workers on large CI machines; add a "--maxprocesses=<N>" flag alongside these options to cap the number of xdist workers (choose a sensible default like 8 or make it configurable via an environment variable) so parallelism is preserved but CI memory/CPU usage is bounded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyproject.toml`:
- Around line 184-185: The comment about debugging is misleading because when
pytest-xdist is enabled via addopts (e.g., "-n auto"), the "-s/--capture=no"
option does not work for parallel workers; update the note around
log_file/log_cli in pyproject.toml to remove the recommendation to use
"--capture=no" and instead state that only log_cli = true (already present) will
produce per-worker output under pytest-xdist, and mention that a shared log_file
is intentionally disabled for the same reason.
---
Nitpick comments:
In `@pyproject.toml`:
- Around line 174-175: The pytest invocation uses "-n", "auto" (with "--dist",
"loadgroup"), which can spawn too many workers on large CI machines; add a
"--maxprocesses=<N>" flag alongside these options to cap the number of xdist
workers (choose a sensible default like 8 or make it configurable via an
environment variable) so parallelism is preserved but CI memory/CPU usage is
bounded.
Python 3.12 removed the distutils module (PEP 632). Poetry 1.4.0's installer depends on it, so pip install setuptools before running the Poetry installer to provide the distutils compatibility shim. https://claude.ai/code/session_016bAG1p7sRsKkYvgoBr8fcz Signed-off-by: Claude <noreply@anthropic.com>
--capture=no does not work with xdist parallel workers. The comment now correctly points to log_cli which is already configured and safe under xdist. https://claude.ai/code/session_016bAG1p7sRsKkYvgoBr8fcz Signed-off-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build-and-test.yaml (1)
126-148:⚠️ Potential issue | 🟡 MinorHandle missing
test-results.jsonin both the upload and summary steps.If pytest crashes during collection (e.g., import error) the JSON file is never written. Two problems follow:
actions/upload-artifact@v4defaults toif-no-files-found: error, causing the upload step itself to fail and obscuring the real failure in CI output.- The inline Python script raises
FileNotFoundError, adding a second spurious failure for the same reason.🐛 Proposed fix
- name: Upload test results if: always() uses: actions/upload-artifact@v4 with: name: test-results-py${{ matrix.python-version }} path: test-results.json + if-no-files-found: warn - name: Show test summary if: always() run: | python3 -c " import json, sys, os + if not os.path.exists('test-results.json'): + print('test-results.json not found — pytest may have crashed during collection') + sys.exit(0) with open('test-results.json') as f: data = json.load(f)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-and-test.yaml around lines 126 - 148, Update the "Upload test results" step (actions/upload-artifact@v4) to avoid failing when test-results.json is missing by adding if-no-files-found: ignore, and update the "Show test summary" run step to check for the existence of 'test-results.json' before opening it (exit gracefully or print "no test results" if missing) so the inline Python won't raise FileNotFoundError; reference the step names "Upload test results" and "Show test summary" and the artifact/file name 'test-results.json' when making the changes.
🧹 Nitpick comments (4)
.github/workflows/build-and-test.yaml (4)
78-80: Consider addingfail-fast: falseto the matrix strategy.With the default
fail-fast: true, a failure on any single Python version (e.g., 3.10) immediately cancels the 3.11 and 3.12 jobs, hiding whether those versions are also broken.♻️ Suggested change
strategy: + fail-fast: false matrix: python-version: ["3.10", "3.11", "3.12"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-and-test.yaml around lines 78 - 80, Add fail-fast: false to the existing matrix strategy so failing one Python-version job doesn't cancel the others; update the strategy block that contains matrix and python-version (the strategy: matrix: python-version entries) to include fail-fast: false directly under strategy.
92-101: Cache paths don't include the installed Python packages —poetry installruns in full on every cache hit.With
poetry config virtualenvs.create false, packages are installed into theactions/setup-python-managed environment at/opt/hostedtoolcache/Python/<ver>/<arch>/lib/pythonX.Y/site-packages/, which is not among the cached paths. The cache only helps by:
- Skipping Poetry re-download (
~/.local)- Avoiding wheel/sdist re-downloads (
~/.cache/pip,~/.cache/pypoetry)For actual dependency installation caching, switch to
virtualenvs.create true(Poetry's default) and cache the virtualenv path:♻️ Suggested change
- - name: Cache Poetry installation and dependencies - uses: actions/cache@v4 - with: - path: | - ~/.local - ~/.cache/pip - ~/.cache/pypoetry - key: poetry-${{ matrix.python-version }}-${{ hashFiles('poetry.lock') }} - restore-keys: | - poetry-${{ matrix.python-version }}- + - name: Cache Poetry installation + uses: actions/cache@v4 + with: + path: ~/.local + key: poetry-bin-${{ runner.os }} + + - name: Cache Poetry virtualenv + uses: actions/cache@v4 + with: + path: ~/.cache/pypoetry/virtualenvs + key: poetry-venv-${{ matrix.python-version }}-${{ hashFiles('poetry.lock') }} + restore-keys: | + poetry-venv-${{ matrix.python-version }}-And remove
poetry config virtualenvs.create false(or set it totrueexplicitly).The same applies to the
build-binaryjob's cache block (lines 163–172).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-and-test.yaml around lines 92 - 101, The workflow's Poetry cache step ("Cache Poetry installation and dependencies") doesn't include the virtualenv where Poetry installs packages when `poetry config virtualenvs.create false`, so dependencies are reinstalled on every run; revert/remove `poetry config virtualenvs.create false` (or set it to `true`) and change the cache to include the created virtualenv directory (the Poetry virtualenv path under the runner, e.g. the virtualenv created by Poetry for the matrix Python version) so that installed packages are cached; apply the same change to the equivalent cache block in the `build-binary` job.
199-203: Inconsistent--hidden-import=vs--hiddenimportflag forms.Lines 199–201 use
--hidden-import=while lines 202–203 switch to--hiddenimport. Both are accepted by PyInstaller, but the inconsistency is confusing.♻️ Suggested change
--hidden-import=tiktoken_ext.openai_public \ --hidden-import=tiktoken_ext \ --hidden-import=backports \ - --hiddenimport litellm.llms.tokenizers \ - --hiddenimport litellm.litellm_core_utils.tokenizers \ + --hidden-import=litellm.llms.tokenizers \ + --hidden-import=litellm.litellm_core_utils.tokenizers \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-and-test.yaml around lines 199 - 203, The workflow uses two different PyInstaller flag forms (--hidden-import= and --hiddenimport) which is confusing; standardize them to a single form (prefer --hidden-import=) by replacing occurrences of the shorter form (e.g., the flags shown as "--hiddenimport litellm.llms.tokenizers" and "--hiddenimport litellm.litellm_core_utils.tokenizers") with the canonical "--hidden-import=litellm.llms.tokenizers" and "--hidden-import=litellm.litellm_core_utils.tokenizers" so all hidden-import flags are consistent.
184-184: Pin thepyinstallerversion for reproducible binary builds.
pip install --upgrade ... pyinstalleralways pulls the latest release. A new PyInstaller major version can change spec-file semantics or--add-dataglob support, silently breaking the build. The comment on line 182 already flags that this step must stay in sync with another workflow, making a version drift even harder to debug.♻️ Suggested change
- python -m pip install --upgrade pip setuptools pyinstaller + python -m pip install --upgrade pip setuptools pyinstaller==6.13.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-and-test.yaml at line 184, The workflow step that runs the install command "python -m pip install --upgrade pip setuptools pyinstaller" should pin the pyinstaller package to a specific tested version to ensure reproducible binary builds; update that command (or introduce a variable used by it) so pyinstaller is installed as a fixed version (e.g., pyinstaller==<chosen-version> or from a pinned constraint file) rather than pulling the latest release, and ensure the chosen version is documented or stored so it stays in sync with the related workflow referenced in the surrounding comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-and-test.yaml:
- Around line 103-108: The workflow installs Poetry pinned to 1.4.0 which lacks
official Python 3.12 support; update the installer invocation in the "Install
Poetry" step (the curl | python3 - --version ... line) to a Poetry release that
supports Python 3.12 (e.g., change --version 1.4.0 to --version 1.7.0 or a newer
stable like 2.3.2) and make the identical change in the build-binary job where
the same pin is used so both occurrences are consistent; keep the subsequent
poetry config virtualenvs.create false line unchanged.
---
Outside diff comments:
In @.github/workflows/build-and-test.yaml:
- Around line 126-148: Update the "Upload test results" step
(actions/upload-artifact@v4) to avoid failing when test-results.json is missing
by adding if-no-files-found: ignore, and update the "Show test summary" run step
to check for the existence of 'test-results.json' before opening it (exit
gracefully or print "no test results" if missing) so the inline Python won't
raise FileNotFoundError; reference the step names "Upload test results" and
"Show test summary" and the artifact/file name 'test-results.json' when making
the changes.
---
Nitpick comments:
In @.github/workflows/build-and-test.yaml:
- Around line 78-80: Add fail-fast: false to the existing matrix strategy so
failing one Python-version job doesn't cancel the others; update the strategy
block that contains matrix and python-version (the strategy: matrix:
python-version entries) to include fail-fast: false directly under strategy.
- Around line 92-101: The workflow's Poetry cache step ("Cache Poetry
installation and dependencies") doesn't include the virtualenv where Poetry
installs packages when `poetry config virtualenvs.create false`, so dependencies
are reinstalled on every run; revert/remove `poetry config virtualenvs.create
false` (or set it to `true`) and change the cache to include the created
virtualenv directory (the Poetry virtualenv path under the runner, e.g. the
virtualenv created by Poetry for the matrix Python version) so that installed
packages are cached; apply the same change to the equivalent cache block in the
`build-binary` job.
- Around line 199-203: The workflow uses two different PyInstaller flag forms
(--hidden-import= and --hiddenimport) which is confusing; standardize them to a
single form (prefer --hidden-import=) by replacing occurrences of the shorter
form (e.g., the flags shown as "--hiddenimport litellm.llms.tokenizers" and
"--hiddenimport litellm.litellm_core_utils.tokenizers") with the canonical
"--hidden-import=litellm.llms.tokenizers" and
"--hidden-import=litellm.litellm_core_utils.tokenizers" so all hidden-import
flags are consistent.
- Line 184: The workflow step that runs the install command "python -m pip
install --upgrade pip setuptools pyinstaller" should pin the pyinstaller package
to a specific tested version to ensure reproducible binary builds; update that
command (or introduce a variable used by it) so pyinstaller is installed as a
fixed version (e.g., pyinstaller==<chosen-version> or from a pinned constraint
file) rather than pulling the latest release, and ensure the chosen version is
documented or stored so it stays in sync with the related workflow referenced in
the surrounding comments.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/build-and-test.yaml (2)
92-101: Cache key collision betweentest(Python 3.11 matrix leg) andbuild-binaryjobs.Both jobs use the same key
poetry-3.11-${{ hashFiles('poetry.lock') }}but install different dependency sets (--with devvs. without). When they run concurrently, whichever saves the cache last will persist a potentially incomplete entry for the other job. Correctness is preserved becausepoetry installis idempotent, but cache hits may restore the wrong set and require follow-up installs.Consider namespacing the keys to differentiate the two purposes:
♻️ Proposed fix
# test job (lines 92-101) - key: poetry-${{ matrix.python-version }}-${{ hashFiles('poetry.lock') }} - restore-keys: | - poetry-${{ matrix.python-version }}- + key: poetry-test-${{ matrix.python-version }}-${{ hashFiles('poetry.lock') }} + restore-keys: | + poetry-test-${{ matrix.python-version }}- # build-binary job (lines 165-174) - key: poetry-3.11-${{ hashFiles('poetry.lock') }} - restore-keys: | - poetry-3.11- + key: poetry-binary-3.11-${{ hashFiles('poetry.lock') }} + restore-keys: | + poetry-binary-3.11-🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-and-test.yaml around lines 92 - 101, The cache key for the "Cache Poetry installation and dependencies" step collides across different jobs because it only uses matrix.python-version and poetry.lock; update the key to namespace by job purpose so test vs build-binary produce different caches (for example add ${{ github.job }} or a custom job-specific string to the key), and update restore-keys similarly so each job restores only its own cache; change the key expression referenced in the step (the key used now: poetry-${{ matrix.python-version }}-${{ hashFiles('poetry.lock') }}) to include the job identifier so caches do not clash.
202-207: Inconsistent PyInstaller flag style — combine spelling and syntax differences.Lines 202–204 use
--hidden-import=<value>(with hyphen, equals-sign format), while lines 205–206 use--hiddenimport <value>(no hyphen, space-separated). Both spellings are equivalent PyInstaller aliases, but the formatting differs in both the flag name and argument separator. Unifying to the--hidden-import=<value>style improves consistency.♻️ Proposed fix
- --hiddenimport litellm.llms.tokenizers \ - --hiddenimport litellm.litellm_core_utils.tokenizers \ + --hidden-import=litellm.llms.tokenizers \ + --hidden-import=litellm.litellm_core_utils.tokenizers \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-and-test.yaml around lines 202 - 207, The PyInstaller flags are inconsistently formatted: change the two occurrences of the alias using space-separated form ("--hiddenimport litellm.llms.tokenizers" and "--hiddenimport litellm.litellm_core_utils.tokenizers") to the consistent "--hidden-import=<value>" style used elsewhere (i.e., "--hidden-import=litellm.llms.tokenizers" and "--hidden-import=litellm.litellm_core_utils.tokenizers") so all hidden-import flags use the same hyphenated name and equals-sign argument form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/build-and-test.yaml:
- Around line 103-109: Update the Poetry installer version pin from 1.4.0 to a
release that officially supports Python 3.12 (use at least 1.7.0; e.g., 1.7.1)
by changing the installer invocation string "curl -sSL
https://install.python-poetry.org | python3 - --version 1.4.0" to use the newer
version, and apply the same change to the second occurrence of that installer
invocation (the one in the build-binary job) so both places install Poetry
>=1.7.0.
---
Nitpick comments:
In @.github/workflows/build-and-test.yaml:
- Around line 92-101: The cache key for the "Cache Poetry installation and
dependencies" step collides across different jobs because it only uses
matrix.python-version and poetry.lock; update the key to namespace by job
purpose so test vs build-binary produce different caches (for example add ${{
github.job }} or a custom job-specific string to the key), and update
restore-keys similarly so each job restores only its own cache; change the key
expression referenced in the step (the key used now: poetry-${{
matrix.python-version }}-${{ hashFiles('poetry.lock') }}) to include the job
identifier so caches do not clash.
- Around line 202-207: The PyInstaller flags are inconsistently formatted:
change the two occurrences of the alias using space-separated form
("--hiddenimport litellm.llms.tokenizers" and "--hiddenimport
litellm.litellm_core_utils.tokenizers") to the consistent
"--hidden-import=<value>" style used elsewhere (i.e.,
"--hidden-import=litellm.llms.tokenizers" and
"--hidden-import=litellm.litellm_core_utils.tokenizers") so all hidden-import
flags use the same hyphenated name and equals-sign argument form.
aantn
enabled auto-merge (squash)
February 22, 2026 16:22
arikalon1
approved these changes
Feb 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This change enables parallel test execution in the pytest configuration to improve test suite performance by leveraging multiple CPU cores.
Key Changes
-n autoflag to automatically detect and use all available CPU cores for running tests--dist loadgroupto distribute tests by load while respecting xdist_group marks, ensuring related tests run together--durations=10from "Show 5 slowest tests" to "Show 10 slowest tests" to match the actual configurationlog_fileconfiguration and related settings to prevent log file corruption that would occur when multiple parallel workers write to the same file simultaneouslylog_clior--capture=no) for debuggingImplementation Details
The removal of file logging is intentional and necessary for parallel test execution. When pytest-xdist spawns multiple worker processes, they would all attempt to write to the same log file, causing corruption. Users can still access test output through console logging (
log_cli) or by running tests with--capture=nofor debugging purposes.https://claude.ai/code/session_016bAG1p7sRsKkYvgoBr8fcz
Summary by CodeRabbit
Chores
Tests