Repository navigation
Move linting/formatting check into separate CI workflow - #138
Conversation
|
Important Review skippedThe saved review history does not include the base for the last reviewed commit. This saved history cannot establish the base for an incremental review. Comment You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly and concisely describes the main change: moving linting and formatting checks into a separate CI workflow. |
✨ Finishing Touches 💡 1
-
🛠️ Fix failing CI checks 💡
-
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
- Commit unit tests in branch
102-separate-linting-workflow-in-ci-v2
-
Comment @coderabbitai help to get the list of available commands.
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
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:
|
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
164a2db to
b673de9
Compare
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