Run CI tests with max processes#7254
Conversation
|
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: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces a large platform-independent test allowlist with a smaller 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)
📝 Coding Plan
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 |
9ad8f9f to
7f1ca40
Compare
.github/workflows/ci.yaml
Outdated
| 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?
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...")?
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
314686f to
ea0d06e
Compare
|
Whats the option cpython is using? |
CPython uses max processes for their CI (from what I can tell) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/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
📒 Files selected for processing (1)
.github/workflows/ci.yaml
| - 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.
🧩 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"📝 Committable suggestion
‼️ 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" |
🤖 Prompt for AI Agents
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.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/ci.yaml (1)
244-249:⚠️ Potential issue | 🟠 MajorFix
os.process_cpu_count()None handling and quote$GITHUB_OUTPUT.Line 248 calls
os.process_cpu_count(), which returnsNonein Python 3.14 when the usable CPU count cannot be determined. This causes the step to emitcores=None, which will fail downstream when passed to-j. Additionally,$GITHUB_OUTPUTon line 249 is unquoted, triggering shellcheck SC2086.🛠️ Proposed 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="$(python -c 'import os; print(os.process_cpu_count() or 1)')" + echo "cores=${cores}" >> "$GITHUB_OUTPUT"🤖 Prompt for AI Agents
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.
🤖 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/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
📒 Files selected for processing (1)
.github/workflows/ci.yaml
| - 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.
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 Agents
Verify 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.
There was a problem hiding this comment.
That's not the goal. it's fine
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.
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