Skip to content

Conversation

@fanninpm
Copy link
Contributor

@fanninpm fanninpm commented Jan 11, 2026

When I was working on #6410, I noticed that the CI job "snippets_cpython" was recompiling RustPython even though no Rust code was changed.
I assumed the reason for the recompilation was because the "Check whats_left is not broken" step is recompiling RustPython with different cargo features than were defined earlier in the same job.
This "whats_left" recompilation is what ultimately gets cached/restored by the rust-cache action.
Then, when the "build rustpython" step is run, the difference in Cargo features necessitates a recompilation before the snippets and CPython tests are run.
For each CI run in the "snippets_cpython" job, these two recompilations cost us about 4 minutes on the Ubuntu runner, about 5 minutes on the macOS runner, and about 7-8 minutes on the windows runner.
This PR aims to eliminate the useless recompilations, making the CI run a little bit faster.

Summary by CodeRabbit

  • Chores
    • Updated CI workflow to use OS-specific build configurations, ensuring proper feature compilation across different platforms.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 11, 2026

📝 Walkthrough

Walkthrough

The CI workflow is modified to apply OS-specific feature flags for WhatsLeft checks. A single unconditional step is replaced with two conditional steps: one for non-macOS runners with threading and jit features, and one for macOS with threading but explicitly excluding jit.

Changes

Cohort / File(s) Summary
CI Workflow Configuration
.github/workflows/ci.yaml
Replaced unconditional WhatsLeft step with two OS-specific conditional steps; non-macOS includes jit feature flag, macOS excludes it with TODO note.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A whisker-twitch through workflows we hop,
macOS takes one path, others don't stop,
JIT features dance on Linux so bright,
While Mac says "not yet"—we'll get it right! ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title check ⚠️ Warning The title 'Optimize CI cache usage' is partially related to the changeset. While the PR does address CI caching issues, the actual change shown in the raw_summary involves modifying the 'WhatsLeft check' step with OS-specific conditional logic and feature flags, not directly optimizing cache usage. Revise the title to better reflect the actual change, such as 'Make WhatsLeft check use consistent Cargo features across OS' or 'Run WhatsLeft with OS-specific feature flags to avoid redundant recompilations'.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Contributor

Code has been automatically formatted

The code in this PR has been formatted using cargo fmt --all.
Please pull the latest changes before pushing again:

git pull origin better-ci-cache

@fanninpm
Copy link
Contributor Author

Code has been automatically formatted

The code in this PR has been formatted using cargo fmt --all. Please pull the latest changes before pushing again:

git pull origin better-ci-cache

@youknowone @ShaharNaveh I didn't even edit the file that was reformatted.

@ShaharNaveh
Copy link
Contributor

Code has been automatically formatted
The code in this PR has been formatted using cargo fmt --all. Please pull the latest changes before pushing again:

git pull origin better-ci-cache

@youknowone @ShaharNaveh I didn't even edit the file that was reformatted.

I think it has something to do with the ruff version, I'll update it

@fanninpm
Copy link
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 11, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @.github/workflows/ci.yaml:
- Around line 422-429: The sed invocation in the CI job names "Check whats_left
is not broken" is incorrectly quoted so it sees a leading double-quote in the
value `"${{ env.CARGO_ARGS }}"`, preventing the pattern `^--` from matching;
update the run lines that call whats_left.py to remove the escaped quotes and
use `${{ env.CARGO_ARGS }}` directly (so the sed expression s/^--[^ ]*// works),
or alternatively change the sed pattern to handle optional leading quotes (e.g.,
s/^"*--[^ ]*//), ensuring both the non-macOS and macOS steps that call python -I
whats_left.py use the fixed form.
🧹 Nitpick comments (1)
.github/workflows/ci.yaml (1)

426-426: Consider creating a tracking issue for the macOS JIT fix.

The TODO comment documents the macOS JIT exclusion, but this workaround might be forgotten over time. Consider opening a GitHub issue to track resolving the underlying JIT issue on macOS.

Would you like me to help draft an issue to track this?

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 29bb8b4 and f88cd49.

📒 Files selected for processing (2)
  • .github/workflows/ci.yaml
  • scripts/fix_test.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.py: In most cases, Python code should not be edited; bug fixes should be made through Rust code modifications only
Follow PEP 8 style for custom Python code
Use ruff for linting Python code

Files:

  • scripts/fix_test.py
**/*test*.py

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*test*.py: NEVER comment out or delete any test code lines except for removing @unittest.expectedFailure decorators and upper TODO comments
NEVER modify test assertions, test logic, or test data in test files
When a test cannot pass due to missing language features, keep it as expectedFailure and document the reason; do not comment it out
The only acceptable modifications to test files are: (1) Removing @unittest.expectedFailure decorators and the upper TODO comments when tests actually pass, and (2) Adding @unittest.expectedFailure decorators when tests cannot be fixed

Files:

  • scripts/fix_test.py
🧠 Learnings (3)
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Always run clippy to lint code (`cargo clippy`) before completing tasks and fix any warnings or lints introduced by changes

Applied to files:

  • .github/workflows/ci.yaml
📚 Learning: 2025-08-30T14:40:05.858Z
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 6089
File: scripts/lib_updater.py:292-297
Timestamp: 2025-08-30T14:40:05.858Z
Learning: In scripts/lib_updater.py, the --inplace flag intentionally writes to orig_file (not remote_file) even though patches are applied to remote_file content. This workflow allows updating the original RustPython test file with patches applied to new upstream CPython content.

Applied to files:

  • scripts/fix_test.py
📚 Learning: 2025-09-07T05:38:31.690Z
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 6089
File: scripts/lib_updater.py:198-202
Timestamp: 2025-09-07T05:38:31.690Z
Learning: In scripts/lib_updater.py, the iter_patches function intentionally does not handle SyntaxError from ast.parse(contents). The author confirmed this behavior is fine and intended - the tool should fail fast on unparseable files rather than silently skip processing.

Applied to files:

  • scripts/fix_test.py
🔇 Additional comments (1)
scripts/fix_test.py (1)

30-30: LGTM!

The import reordering follows alphabetical convention and is consistent with ruff formatting mentioned in the PR discussion.

@fanninpm
Copy link
Contributor Author

Here's a git trick for everyone: Assuming you have no files staged, git commit --allow-empty -m 'Do nothing' produces a commit with no files changed and the message "Do nothing".

@fanninpm fanninpm marked this pull request as ready for review January 20, 2026 04:43
@fanninpm fanninpm requested a review from youknowone January 20, 2026 04:43
@ShaharNaveh
Copy link
Contributor

Here's a git trick for everyone: Assuming you have no files staged, git commit --allow-empty -m 'Do nothing' produces a commit with no files changed and the message "Do nothing".

I usually call the commit "Trigger CI"

@youknowone youknowone merged commit 122fac9 into RustPython:main Jan 21, 2026
13 checks passed
@fanninpm fanninpm deleted the better-ci-cache branch January 22, 2026 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants