Conversation
📝 WalkthroughWalkthroughThe 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
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
|
Code has been automatically formatted The code in this PR has been formatted using 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 |
8d167e1 to
3f41b76
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
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
📒 Files selected for processing (2)
.github/workflows/ci.yamlscripts/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.expectedFailuredecorators 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.expectedFailuredecorators and the upper TODO comments when tests actually pass, and (2) Adding@unittest.expectedFailuredecorators 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.
|
Here's a git trick for everyone: Assuming you have no files staged, |
1018896 to
1411f9e
Compare
1411f9e to
7b54963
Compare
I usually call the commit "Trigger CI" |
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-cacheaction.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
✏️ Tip: You can customize this high-level summary in your review settings.