Parallelize tests #221
Parallelize tests #221
Conversation
Mainly because the test runs for long. I will have to look into what an appropriate number is, but I suspect 500 will still find most of the failures that 5000 samples would.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideCI test workflow is consolidated into a single matrix-based job that runs pytest in parallel across API markers and mutation markers, Hypothesis test data generation is reduced for a slow test, and pytest-xdist is added as a development dependency to support parallel execution. Flow diagram for matrix-based parallel pytest executionflowchart TD
A[Start CI workflow] --> B[Checkout code]
B --> C[Set up Python environment]
C --> D[Install dev dependencies including pytest-xdist]
D --> E[Define matrix with markers]
E --> F{Matrix entry}
F -->|api| G[Run pytest -m api with xdist]
F -->|mutation| H[Run pytest -m mutation with xdist]
G --> I[Reduce Hypothesis settings for slow test]
H --> I
I --> J[Collect and upload test results]
J --> K[Finish CI workflow]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@PGijsbers has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughConsolidates CI test jobs into a single matrix-driven GitHub Actions job, adds pytest-xdist to dev deps for parallel test runs, and reduces a Hypothesis max_examples setting from 5000 to 500 in one slow test. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The test matrix currently encodes marker expressions directly as strings (e.g.
"not php_api","mut") and concatenates them in thepytest -margument; consider using boolean matrix axes (likephp_api: [true,false],mutations: [true,false]) and constructing the marker expression in the step to avoid fragile string handling and make the intent clearer. - Because
docker composeis always started with both thepythonandphpprofiles, even when running only non-php_apitests, you might be incurring unnecessary startup cost; consider varying the profiles based on the matrix entry so that only the needed services are started for each test subset. - The inline comment
# This number needs to be better motivatedonmax_examples=500suggests a TODO; it would be helpful either to document the rationale for this lower value now (e.g. time vs. coverage tradeoff) or to replace it with a more specific action item so the intent is clear to future maintainers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The test matrix currently encodes marker expressions directly as strings (e.g. `"not php_api"`, `"mut"`) and concatenates them in the `pytest -m` argument; consider using boolean matrix axes (like `php_api: [true,false]`, `mutations: [true,false]`) and constructing the marker expression in the step to avoid fragile string handling and make the intent clearer.
- Because `docker compose` is always started with both the `python` and `php` profiles, even when running only non-`php_api` tests, you might be incurring unnecessary startup cost; consider varying the profiles based on the matrix entry so that only the needed services are started for each test subset.
- The inline comment `# This number needs to be better motivated` on `max_examples=500` suggests a TODO; it would be helpful either to document the rationale for this lower value now (e.g. time vs. coverage tradeoff) or to replace it with a more specific action item so the intent is clear to future maintainers.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #221 +/- ##
=======================================
Coverage ? 54.32%
=======================================
Files ? 32
Lines ? 1132
Branches ? 100
=======================================
Hits ? 615
Misses ? 516
Partials ? 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/routers/openml/datasets_list_datasets_test.py (1)
226-231: Consider documenting the rationale for the 10x reduction in test examples.The reduction from 5000 to 500 examples is significant (90% reduction in coverage). While this aligns with the PR's goal of faster test execution through parallelization, the inline comment suggests this value needs better justification.
Consider:
- Documenting why 500 examples provides sufficient coverage for this property-based test
- Whether the parallelization gains justify the reduced test thoroughness
- If 500 is too aggressive, a middle ground like 1000-2000 might balance speed and coverage
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/tests.yml(2 hunks)pyproject.toml(1 hunks)tests/routers/openml/datasets_list_datasets_test.py(1 hunks)
⏰ 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: Sourcery review
- GitHub Check: tests (php_api, not mut)
- GitHub Check: tests (not php_api, not mut)
- GitHub Check: tests (not php_api, mut)
- GitHub Check: tests (php_api, mut)
🔇 Additional comments (3)
pyproject.toml (1)
34-34: LGTM!The addition of pytest-xdist correctly enables parallel test execution with the
-n autoflag used in the workflow..github/workflows/tests.yml (2)
15-19: LGTM! Matrix strategy correctly partitions test execution.The matrix strategy creates 4 parallel jobs that partition tests by markers, enabling faster test execution through parallelization.
29-29: Verify that all tests are appropriately marked for matrix filtering.The parallel execution strategy looks correct, using
-n autofor parallelization and marker filtering to partition tests across matrix jobs. However, ensure that:
- All relevant tests are marked with either
php_apior the absence is intentional- All mutation tests are marked with
mut- Tests without markers will run only in the "not php_api and not mut" job
Run the following script to verify marker coverage:
#!/bin/bash # Description: Verify test marker coverage for matrix filtering echo "=== Tests marked with 'php_api' ===" rg -n --type=py -C2 '@pytest\.mark\.php_api' tests/ echo -e "\n=== Tests marked with 'mut' ===" rg -n --type=py -C2 '@pytest\.mark\.mut' tests/ echo -e "\n=== All test functions (for comparison) ===" ast-grep --pattern 'def test_$_($$$):'
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/tests.yml (1)
40-45: Coverage file not accessible to Codecov action.The
coverage xmlcommand generates the file inside the Docker container, but the Codecov action runs on the host and won't find it. You need to copy the coverage file from the container to the host filesystem.🔎 Proposed fix
- name: Produce coverage report - run: docker exec openml-python-rest-api coverage xml + run: | + docker exec openml-python-rest-api coverage xml + docker cp openml-python-rest-api:/app/coverage.xml coverage.xml - name: Upload results to Codecov uses: codecov/codecov-action@v4 with: token: ${{ secrets.CODECOV_TOKEN }} + files: ./coverage.xmlNote: Adjust
/app/coverage.xmlto the actual path inside the container where the coverage file is generated.
🧹 Nitpick comments (1)
.github/workflows/tests.yml (1)
15-19: Consider addingfail-fast: falseto the matrix strategy.With the default
fail-fast: true, if one matrix combination fails, the other running jobs are cancelled. For test jobs, it's often useful to see all failures across the matrix.🔎 Suggested change
strategy: + fail-fast: false matrix: php_api: [true, false] mutations: [true, false]
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/tests.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Sourcery review
- GitHub Check: tests (true, false)
- GitHub Check: tests (true, true)
🔇 Additional comments (2)
.github/workflows/tests.yml (2)
28-34: LGTM!The conditional profile inclusion and error handling for the docker compose command are well-implemented.
36-39: LGTM!The marker construction logic correctly maps matrix values to pytest markers, and
-n autoenables parallel test execution via pytest-xdist.
Summary by Sourcery
Parallelize test execution in CI and adjust test configuration for improved performance.
Build:
CI:
Tests:
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.