Move linting/formatting check into separate CI workflow - #138
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe pull request adds a dedicated Ruff workflow, moves Ruff into a lint dependency group, removes duplicate checks from the test workflow, and includes lint status in drift reporting. ChangesCI workflow separation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Linting is now executed separately from tests, while drift reporting includes both results. The workflow retains lint enforcement and does not leave an actionable current-head merge risk. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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. Comment |
There was a problem hiding this comment.
🟡 Changes recommended
The new lint workflow introduces avoidable CI security/reproducibility and performance issues (token permissions, action pinning, heavy dependency sync, and unpinned uv version).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors CI responsibilities by moving ruff formatting/lint checks out of the main test workflow (run_tests.yml) into a dedicated GitHub Actions workflow, keeping the test workflow focused on executing tests and uploading coverage.
Changes:
- Removed ruff format/lint steps from
.github/workflows/run_tests.yml. - Added
.github/workflows/linting_formatting.ymlto run ruff formatting and linting checks on pull requests.
File summaries
| File | Description |
|---|---|
| .github/workflows/run_tests.yml | Removes formatting/linting steps so the workflow focuses on tests + Codecov upload. |
| .github/workflows/linting_formatting.yml | Introduces a new PR-triggered workflow to run ruff format checks and linting. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟢 Approval recommended
The workflow split and drift integration are straightforward and do not change application/runtime behavior, with only minor CI hardening/efficiency follow-ups suggested.
Review details
Suppressed comments (2)
.github/workflows/linting_formatting.yml:30
- This lint workflow installs the entire
devdependency group before running ruff. In this repo,devincludes heavyweight ML/runtime deps (e.g., keras/bayesflow/sbi/onnxruntime), which will significantly slow lint-only CI and can introduce avoidable install failures. Consider introducing a lightweight dependency group (e.g.lint) that contains only ruff (and any other lint tools) and syncing that here instead.
- name: Install package
run: uv sync --group dev
.github/workflows/linting_formatting.yml:9
- Add explicit minimal token permissions for this job so it doesn't run with broader defaults than necessary (it only needs to read the repository to lint/format-check).
jobs:
lint:
runs-on: ubuntu-latest
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The new lint workflow currently uses floating/unpinned tool versions (and doesn’t pin Python), reducing CI reproducibility and increasing supply-chain risk.
Review details
Suppressed comments (1)
.github/workflows/linting_formatting.yml:20
- This workflow uses a floating uv version ("latest") and unpinned action tags, which makes CI less reproducible and increases supply-chain risk. It also doesn’t pin a Python version, so uv will rely on whatever Python happens to be preinstalled on
ubuntu-latest, which can drift outside the project’s supported range.
- name: Install uv
uses: astral-sh/setup-uv@v7
with:
version: "latest"
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The new lint workflow does not pin/set up a Python version before running uv, which can lead to CI instability relative to the repo’s requires-python constraints.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.github/workflows/linting_formatting.yml:23
- This workflow relies on the runner’s default Python, but the repo’s other
uv-based workflows explicitly pin Python first (e.g..github/workflows/run_tests.yml:23-27). Withoutactions/setup-python,uv synccan start failing ifubuntu-latestchanges its default Python and it no longer satisfiesrequires-python (>=3.12, <3.15).
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The new lint workflow doesn’t pin a supported Python version (>=3.12), which can make lint CI runs interpreter-dependent and potentially flaky on ubuntu-latest.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.github/workflows/linting_formatting.yml:23
- This workflow doesn’t pin a Python version before running
uv sync/uv run. The project declaresrequires-python = ">=3.12, <3.15"(pyproject.toml:20) and other workflows explicitly set up Python, so leaving this to whatever happens to be preinstalled onubuntu-latestcan make lint CI flaky or run under an unsupported interpreter.
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
…py-on-write-in-trainers' into 102-separate-linting-workflow-in-ci-v2
There was a problem hiding this comment.
🟡 Changes recommended
The PR scope/description doesn’t currently match the included trainer code changes, and the new lint workflow should pin a supported Python version to avoid CI flakiness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
.github/workflows/linting_formatting.yml:23
- This workflow runs
uv sync/uv runwithout pinning a Python version. Since the project declaresrequires-python = ">=3.12, <3.15", relying on the runner's default Python can make lint CI flaky ifubuntu-latestchanges. Consider addingactions/setup-python(and matching uv cache settings) likerun_tests.ymldoes.
src/lanfactory/trainers/jax_mlp.py:595 - The PR title/description focus on splitting lint/formatting into a separate CI workflow, but this also changes trainer behavior (training history row assignment). Please either update the PR description/title to mention these code changes (and why they're needed), or split the trainer edits into a separate PR to keep CI refactors easy to review/rollback.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
…py-on-write-in-trainers' into 102-separate-linting-workflow-in-ci-v2
There was a problem hiding this comment.
🔵 Needs a closer look
The new linting workflow should be made deterministic (pin Python and align uv caching) and the PR scope/description should be reconciled with the added runtime training-code refactor.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.github/workflows/linting_formatting.yml:23
- The linting workflow doesn’t pin a Python version or enable uv caching, so lint/format results (and runtime) can vary with whatever Python happens to be on
ubuntu-latest, and reruns won’t benefit from the same cache behavior asrun_tests.yml. Add an explicitactions/setup-pythonstep (e.g. 3.12) and enable the uv cache to make linting deterministic and faster.
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
This pull request refactors the project's GitHub Actions workflows by separating linting and formatting checks into a dedicated workflow. The main test workflow (
run_tests.yml) is now focused solely on running tests and uploading coverage, while all code style and linting checks are handled in a new workflow.Workflow improvements:
.github/workflows/linting_formatting.yml, to run linting and formatting checks usingruffanduv..github/workflows/run_tests.yml, to avoid redundancy and streamline responsibilities.Summary by CodeRabbit