Run CI tests with max processes - #7254
Conversation
Sorry, something went wrong.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 3ba46c64-cd6c-4fcb-b689-a20a7f18ce76 📥 CommitsReviewing files that changed from the base of the PR and between 04274b4 and 954b0be. 📒 Files selected for processing (1)
📝 Walkthrough WalkthroughReplaces a large platform-independent test allowlist with a smaller FLAKY_MP_TESTS, adds a step to detect CPU cores, runs the main CPython test suite in parallel using detected cores while excluding flaky MP tests, and runs flaky multiprocessing tests separately at -j 1. Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow as GitHub Actions Workflow
participant Runner as CI Runner
participant CoreDetect as Core Detection Step
participant RustPy as rustpython -m test
Workflow->>Runner: start job
Runner->>CoreDetect: execute core detection (os.process_cpu_count)
CoreDetect-->>Workflow: outputs.cores
Workflow->>RustPy: run CPython tests (-j ${{steps.cores.outputs.cores}}, -x FLAKY_MP_TESTS, matrix args/skips)
RustPy-->>Workflow: report parallel test results
Workflow->>RustPy: run FLAKY_MP_TESTS (-j 1, matrix args/skips)
RustPy-->>Workflow: report flaky-mp test results
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem🚥 Pre-merge checks | ✅ 3 ✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches 🧪 Generate unit tests (beta)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
| test_unpack_ex | ||
| test_weakref | ||
| test_yield_from | ||
| # Tests that can be flaky when running with multiple processes `-j 2`. These tests will be ran using `-j 1` |
There was a problem hiding this comment.
@youknowone can you please let me know what you think about this change?
Sorry, something went wrong.
There was a problem hiding this comment.
Out of curiosity, how is this different from marking those tests with, say, @support.thread_unsafe("TODO: RUSTPYTHON, blah blah blah...")?
Sorry, something went wrong.
There was a problem hiding this comment.
Out of curiosity, how is this different from marking those tests with, say, @support.thread_unsafe("TODO: RUSTPYTHON, blah blah blah...")?
That's a good point. If this is the approach we are gonna choose then we need to add support for lib_updater for it as well, ATM it only looks at unittest patches
Sorry, something went wrong.
|
Whats the option cpython is using? |
Sorry, something went wrong.
CPython uses max processes for their CI (from what I can tell) |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yaml:
- Around line 243-249: The "Detect available cores" step is calling a
non-existent Python function os.process_cpu_count(); update the step (id: cores)
to avoid Python and use bash instead: detect CPU count with a POSIX-safe command
such as nproc or getconf _NPROCESSORS_ONLN (falling back to 2 if detection
fails), assign the result to the cores variable and then echo "cores=${cores}"
>> $GITHUB_OUTPUT; ensure the step retains shell: bash and preserves the output
key name and id (cores) so downstream jobs still read the value.
ℹ️ Review info ⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 23cc4bdc-425b-4286-9c21-25706e3ebe20
📥 CommitsReviewing files that changed from the base of the PR and between 3148024 and ccb66b1.
📒 Files selected for processing (1)- .github/workflows/ci.yaml
Sorry, something went wrong.
| - name: Detect available cores | ||
| id: cores | ||
| shell: bash | ||
| run: | | ||
| target/release/rustpython -m test -j 1 -u all --slowest --fail-env-changed --timeout 600 -v ${{ env.PLATFORM_INDEPENDENT_TESTS }} | ||
| timeout-minutes: 45 | ||
| cores=$(python -c 'print(__import__("os").process_cpu_count())') | ||
| echo "cores=${cores}" >> $GITHUB_OUTPUT | ||
|
|
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical
🧩 Analysis chain🌐 Web query:
In Python 3.14, can os.process_cpu_count() return None, and is a fallback required before using it as an integer worker count?
💡 Result:
No, os.process_cpu_count does not exist in Python 3.14, and thus cannot return None. The standard function is os.cpu_count, which returns the number of logical CPUs in the system or None if indeterminable. A fallback (e.g., defaulting to 1 or 4) is required when using it as an integer worker count, such as in multiprocessing.Pool(processes=os.cpu_count).
Citations:
- 1: os.process_cpu_count(): get the number CPUs usable by the current process python/cpython#109649
- 2: https://docs.python.org/3.14/library/os.html
- 3: https://docs.python.org/3/library/os.html
- 4: os.cpu_count() recommends non-portable len(os.sched_getaffinity(0)) to get usable CPUs python/cpython#105972
- 5: https://docs.python.org/3.11/library/os.html
- 6: gh-109649: Add affinity parameter to os.cpu_count() python/cpython#109652
- 7: https://github.com/python/cpython/blob/main/Lib/os.py
🏁 Script executed:
# Check the actual content of lines 243-249 in .github/workflows/ci.yaml
head -n 249 .github/workflows/ci.yaml | tail -n 7Repository: RustPython/RustPython
Length of output: 280
Fix non-existent function call in core detection.
Line 247 calls os.process_cpu_count(), which does not exist in Python—the correct function is os.cpu_count(), which can also return None. This will cause runtime failure. Additionally, Python for CI test orchestration introduces unnecessary complexity; use bash directly with fallbacks.
🛠️ Suggested fix - name: Detect available cores
id: cores
shell: bash
run: |
- cores=$(python -c 'print(__import__("os").process_cpu_count())')
- echo "cores=${cores}" >> $GITHUB_OUTPUT
+ cores="${NUMBER_OF_PROCESSORS:-}"
+ if [ -z "${cores}" ]; then
+ cores="$(getconf _NPROCESSORS_ONLN 2>/dev/null || nproc 2>/dev/null || echo 1)"
+ fi
+ echo "cores=${cores}" >> "$GITHUB_OUTPUT"‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Detect available cores | |
| id: cores | |
| shell: bash | |
| run: | | |
| target/release/rustpython -m test -j 1 -u all --slowest --fail-env-changed --timeout 600 -v ${{ env.PLATFORM_INDEPENDENT_TESTS }} | |
| timeout-minutes: 45 | |
| cores=$(python -c 'print(__import__("os").process_cpu_count())') | |
| echo "cores=${cores}" >> $GITHUB_OUTPUT | |
| - name: Detect available cores | |
| id: cores | |
| shell: bash | |
| run: | | |
| cores="${NUMBER_OF_PROCESSORS:-}" | |
| if [ -z "${cores}" ]; then | |
| cores="$(getconf _NPROCESSORS_ONLN 2>/dev/null || nproc 2>/dev/null || echo 1)" | |
| fi | |
| echo "cores=${cores}" >> "$GITHUB_OUTPUT" |
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yaml around lines 243 - 249, The "Detect available
cores" step is calling a non-existent Python function os.process_cpu_count();
update the step (id: cores) to avoid Python and use bash instead: detect CPU
count with a POSIX-safe command such as nproc or getconf _NPROCESSORS_ONLN
(falling back to 2 if detection fails), assign the result to the cores variable
and then echo "cores=${cores}" >> $GITHUB_OUTPUT; ensure the step retains shell:
bash and preserves the output key name and id (cores) so downstream jobs still
read the value.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1).github/workflows/ci.yaml (1)🤖 Prompt for all review comments with AI agents244-249: ⚠️ Potential issue | 🟠 Major
Fix os.process_cpu_count() None handling and quote $GITHUB_OUTPUT.
Line 248 calls os.process_cpu_count(), which returns None in Python 3.14 when the usable CPU count cannot be determined. This causes the step to emit cores=None, which will fail downstream when passed to -j. Additionally, $GITHUB_OUTPUT on line 249 is unquoted, triggering shellcheck SC2086.
🛠️ Proposed fix🤖 Prompt for AI Agents- name: Detect available cores id: cores shell: bash run: | - cores=$(python -c 'print(__import__("os").process_cpu_count())') - echo "cores=${cores}" >> $GITHUB_OUTPUT + cores="$(python -c 'import os; print(os.process_cpu_count() or 1)')" + echo "cores=${cores}" >> "$GITHUB_OUTPUT"Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 244 - 249, The GitHub Actions step named "Detect available cores" (id: cores) uses python to call os.process_cpu_count(), which can return None in Python 3.14 and writes unquoted $GITHUB_OUTPUT; update the python snippet to coerce a safe integer fallback (e.g., use os.cpu_count() or process_cpu_count() or default to 1 when None) and ensure you echo the output with quoted variable expansion (use "$GITHUB_OUTPUT") so downstream `-j` receives a numeric value and shellcheck SC2086 is avoided.
Verify each finding against the current code and only fix it if needed. Inline comments: In @.github/workflows/ci.yaml: - Around line 251-254: The workflow change altered which tests run while also changing parallelism; revert the test invocation in the "Run CPython tests" step so the matrix-driven test split is preserved and only the -j value is varied: keep the original joins/skip logic (the use of matrix.skips and matrix.extra_test_args) exactly as before and do not narrow the suite to only exclude FLAKY_MP_TESTS here; instead only change the -j argument coming from steps.cores.outputs.cores so this PR isolates the max-process (‑j) experiment without changing the test coverage shape. --- Duplicate comments: In @.github/workflows/ci.yaml: - Around line 244-249: The GitHub Actions step named "Detect available cores" (id: cores) uses python to call os.process_cpu_count(), which can return None in Python 3.14 and writes unquoted $GITHUB_OUTPUT; update the python snippet to coerce a safe integer fallback (e.g., use os.cpu_count() or process_cpu_count() or default to 1 when None) and ensure you echo the output with quoted variable expansion (use "$GITHUB_OUTPUT") so downstream `-j` receives a numeric value and shellcheck SC2086 is avoided.
ℹ️ Review info ⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5a9c912e-53d8-468b-84c8-c54d18ad90ad
📥 CommitsReviewing files that changed from the base of the PR and between b641625 and 04274b4.
📒 Files selected for processing (1)- .github/workflows/ci.yaml
Sorry, something went wrong.
| - name: Run CPython tests | ||
| run: | | ||
| target/release/rustpython -m test -j ${{ steps.cores.outputs.cores }} ${{ join(matrix.extra_test_args, ' ') }} --slowest --fail-env-changed --timeout 600 -v -x ${{ env.FLAKY_MP_TESTS }} ${{ join(matrix.skips, ' ') }} | ||
| timeout-minutes: ${{ matrix.timeout }} |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
This no longer isolates the -j experiment.
By excluding only FLAKY_MP_TESTS here, the workflow is no longer preserving the earlier large platform-independent split and now changes coverage shape at the same time it changes worker count. That makes the CI runtime/stability signal noisy, because this PR is measuring a wider suite distribution and a different parallelism policy in one shot. If the goal is to evaluate max-process CI in isolation, keep the old split for now and only vary -j in this PR.
Also applies to: 258-260
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 251 - 254, The workflow change altered which tests run while also changing parallelism; revert the test invocation in the "Run CPython tests" step so the matrix-driven test split is preserved and only the -j value is varied: keep the original joins/skip logic (the use of matrix.skips and matrix.extra_test_args) exactly as before and do not narrow the suite to only exclude FLAKY_MP_TESTS here; instead only change the -j argument coming from steps.cores.outputs.cores so this PR isolates the max-process (‑j) experiment without changing the test coverage shape.
Sorry, something went wrong.
There was a problem hiding this comment.
That's not the goal. it's fine
Sorry, something went wrong.
There was a problem hiding this comment.
@ShaharNaveh, understood — the goal is simply to reduce CI runtime by running with max processes, not to run a controlled experiment. Apologies for the noise.
🧠 Learnings used
Learnt from: reactive-firewall Repo: RustPython/RustPython PR: 0 File: :0-0 Timestamp: 2026-01-19T20:10:58.864Z Learning: In the RustPython project's CI/CD workflows (PR `#6176`), bash scripts are preferred over Python or Rust for test orchestration and result aggregation to maintain hermetic testing—avoiding circular dependencies where Python tools would depend on the RustPython interpreter being tested, and avoiding visibility loss into Rust runtime issues when using Rust tooling.
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: AGENTS.md:0-0 Timestamp: 2026-02-06T05:30:53.056Z Learning: When comparing behavior with CPython or running test suites, explicitly use the `python` command for CPython and `cargo run --` for RustPython
Learnt from: ShaharNaveh Repo: RustPython/RustPython PR: 5932 File: .github/workflows/comment-commands.yml:18-24 Timestamp: 2025-07-10T10:08:43.330Z Learning: In GitHub Actions workflows for the RustPython project, the maintainer ShaharNaveh prefers to keep workflows simple and doesn't mind if steps fail when the desired state is already achieved (e.g., user already assigned to an issue). Avoid suggesting complex error handling for edge cases they don't consider problematic.
Sorry, something went wrong.
* Run CI witb max cores * Step for flaky MP tests * Add `test_class` to flkay mp tests
* Run CI witb max cores * Step for flaky MP tests * Add `test_class` to flkay mp tests
Recently our CI has started to take around an hour, this will make it faster at the cost of stability. I want to see how faster / stable this effect the CI
Summary by CodeRabbit