refactor: ci using pixi and ruff/flake8#472
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe project toolchain moves from Poetry, Black, and flake8 to Pixi and Ruff. ChangesPixi/Ruff Toolchain Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
setup.py.bck (1)
36-36: ⚡ Quick winAvoid clobbering externally provided
CFLAGS.Line 36 overwrites existing build flags from CI/users. Append
-O3instead of replacing to keep upstream build controls intact.Proposed fix
-os.environ["CFLAGS"] = "-O3" +os.environ["CFLAGS"] = f'{os.environ.get("CFLAGS", "").strip()} -O3'.strip()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@setup.py.bck` at line 36, The assignment at line 36 where os.environ["CFLAGS"] is set to "-O3" overwrites any existing CFLAGS value that may have been set externally by CI or users. Instead of using direct assignment with the equals operator, append the "-O3" flag to the existing CFLAGS environment variable. Retrieve the current CFLAGS value (defaulting to an empty string if not set) and concatenate it with "-O3" before reassigning it back to os.environ["CFLAGS"].
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 31-32: The "Check code" step in the workflow is using `poetry run
lint` which is incompatible with the Pixi-based job configuration. Replace the
`poetry run lint` command in the run field of the "Check code" step with the
equivalent Pixi command to run the lint task directly, using the format `pixi
run lint` to maintain consistency with Pixi's task runner.
In `@pyproject.toml`:
- Around line 42-48: The ruff configuration has redundant settings for
tests/test_scontrol_parsing.py where the file is listed in extend-exclude (which
completely removes it from linting) and also has a per-file-ignores entry for
E501 that never gets applied because excluded files skip linting entirely.
Choose one strategy: either remove tests/test_scontrol_parsing.py from the
extend-exclude list and keep the per-file-ignores entry to lint the file with
E501 ignored, OR remove the per-file-ignores entry for that file from the
tool.ruff.lint.per-file-ignores section and keep it fully excluded.
In `@setup.py.bck`:
- Around line 22-33: The current approach of combining all Python files from the
base_dir into a single Extension named snakemake_executor_plugin_slurm._core
will make submodules like utils, accounts, and partitions non-importable,
breaking imports throughout the codebase and tests. Instead of iterating through
all .py files and combining them into one extension with py_files, create a
separate Extension object for each submodule in the list (e.g., one Extension
for snakemake_executor_plugin_slurm.utils, one for
snakemake_executor_plugin_slurm.accounts, etc.) with each submodule's
corresponding Python file. This preserves the submodule import structure that
the codebase depends on.
- Line 47: The install_requires parameter in setup.py is using
list(dependencies.keys()) which extracts only the package names and drops all
version constraints from the dependencies dictionary. This allows unconstrained
versions to be installed, breaking compatibility. Replace the
list(dependencies.keys()) call to preserve the full dependency specifications
including their version ranges, either by using the appropriate dictionary
values or by combining package names with their version constraints in the
proper format expected by install_requires.
---
Nitpick comments:
In `@setup.py.bck`:
- Line 36: The assignment at line 36 where os.environ["CFLAGS"] is set to "-O3"
overwrites any existing CFLAGS value that may have been set externally by CI or
users. Instead of using direct assignment with the equals operator, append the
"-O3" flag to the existing CFLAGS environment variable. Retrieve the current
CFLAGS value (defaulting to an empty string if not set) and concatenate it with
"-O3" before reassigning it back to os.environ["CFLAGS"].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f5bbc66-a549-4082-bd88-0d2f7803dda6
📒 Files selected for processing (5)
.github/workflows/ci.ymlpixi.tomlpyproject.tomlsetup.cfgsetup.py.bck
💤 Files with no reviewable changes (1)
- setup.cfg
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
10-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeclare explicit least-privilege
permissionsfor the workflow token.No
permissions:block is defined, so default token scopes apply. Add explicit minimal permissions to reduce blast radius and satisfy policy checks.Suggested fix
name: CI +permissions: + contents: read on: push:If any job later needs extra scopes, elevate only in that specific job.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 10 - 57, The workflow lacks an explicit permissions block, which means the default token scopes are being used. Add a top-level permissions block at the workflow level (before the jobs section) that declares minimal required permissions. Set contents to read since the workflow only needs checkout access. If any individual job later requires elevated permissions, those can be specified at the job level to override the workflow-level defaults while keeping other jobs restricted.Source: Linters/SAST tools
🧹 Nitpick comments (2)
.github/workflows/release-please.yml (2)
34-39: ⚖️ Poor tradeoffConsider using PyPI Trusted Publishing for enhanced security.
The current token-based authentication works, but PyPI's Trusted Publishing (OIDC) provides stronger security by eliminating long-lived secrets. This requires one-time configuration on PyPI to trust this GitHub repository/workflow.
To enable trusted publishing, you would:
- Configure the publisher on PyPI for this package
- Simplify the publish step:
- name: Publish to PyPI if: ${{ steps.release.outputs.release_created }} uses: pypa/gh-action-pypi-publish@release/v1 # No credentials needed - uses OIDCThis is optional but recommended when feasible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-please.yml around lines 34 - 39, In the "Publish to PyPI" step using the pypa/gh-action-pypi-publish@release/v1 action, remove the `user` and `password` fields (which currently use `__token__` and `secrets.PYPI_TOKEN`). After removing these authentication parameters, configure PyPI Trusted Publishing by creating a trusted publisher in your PyPI project settings for this GitHub repository and workflow to enable OIDC-based authentication. The action will automatically use OIDC once the publisher is configured on PyPI.Source: Linters/SAST tools
12-12: Consider pinning actions to commit SHAs for enhanced supply-chain security.While the action versions used (
actions/checkout@v6,actions/setup-python@v6,googleapis/release-please-action@v5, andpypa/gh-action-pypi-publish@release/v1) are valid and current, pinning to full commit SHAs instead of version tags provides stronger security guarantees against tag tampering or unexpected updates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-please.yml at line 12, Replace the version tags in all GitHub Actions with their full commit SHAs for enhanced security. Specifically, update googleapis/release-please-action from `@v5` to its full commit SHA, and also pin actions/checkout, actions/setup-python, and pypa/gh-action-pypi-publish to their respective full commit SHAs instead of using version tags. This prevents potential tag tampering and ensures reproducible, secure builds by explicitly specifying the exact commit being used.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 17: Replace all three occurrences of the mutable tag
`prefix-dev/setup-pixi@v0` with an immutable commit SHA reference in the GitHub
Actions workflow file. Specifically, at the three locations where `uses:
prefix-dev/setup-pixi@v0` appears (lines 17, 29, and 50), change the tag from
`@v0` to `@<commit-sha>` where commit-sha is the full commit hash of a specific
release from the setup-pixi repository. This prevents action supply-chain drift
by pinning to an immutable commit digest rather than a mutable version tag.
In @.github/workflows/release-please.yml:
- Around line 18-21: Add the `persist-credentials: false` parameter to the
`actions/checkout@v6` step to disable credential persistence and prevent
potential credential leakage. This should be added as a new line in the `with`
section alongside the existing `fetch-depth: 0` parameter.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 10-57: The workflow lacks an explicit permissions block, which
means the default token scopes are being used. Add a top-level permissions block
at the workflow level (before the jobs section) that declares minimal required
permissions. Set contents to read since the workflow only needs checkout access.
If any individual job later requires elevated permissions, those can be
specified at the job level to override the workflow-level defaults while keeping
other jobs restricted.
---
Nitpick comments:
In @.github/workflows/release-please.yml:
- Around line 34-39: In the "Publish to PyPI" step using the
pypa/gh-action-pypi-publish@release/v1 action, remove the `user` and `password`
fields (which currently use `__token__` and `secrets.PYPI_TOKEN`). After
removing these authentication parameters, configure PyPI Trusted Publishing by
creating a trusted publisher in your PyPI project settings for this GitHub
repository and workflow to enable OIDC-based authentication. The action will
automatically use OIDC once the publisher is configured on PyPI.
- Line 12: Replace the version tags in all GitHub Actions with their full commit
SHAs for enhanced security. Specifically, update
googleapis/release-please-action from `@v5` to its full commit SHA, and also pin
actions/checkout, actions/setup-python, and pypa/gh-action-pypi-publish to their
respective full commit SHAs instead of using version tags. This prevents
potential tag tampering and ensures reproducible, secure builds by explicitly
specifying the exact commit being used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f49d0fd9-2dcc-455a-85ee-64b0169c82da
📒 Files selected for processing (5)
.github/workflows/ci.yml.github/workflows/release-please.ymlsnakemake_executor_plugin_slurm/__init__.pytests/tests.pyupgrade_dependencies.py
✅ Files skipped from review due to trivial changes (3)
- upgrade_dependencies.py
- tests/tests.py
- snakemake_executor_plugin_slurm/init.py
| release-type: python | ||
| token: ${{ secrets.RELEASE_PLEASE_PR_CI_TOKEN }} | ||
|
|
||
| publish: |
There was a problem hiding this comment.
The separation into a release-please job and a publish job was actually a good thing. I would suggest to keep that, while still switching to pixi for the publish job.
There was a problem hiding this comment.
yes. I reverted that change. This PR should focus on the CI, only. However, switching to pixi in for release-please might require some help: I have been tinkering a looong time to get the CI running as I was used to. Repeating this with release-please is not advisable as releases should be working without irritating users.
There was a problem hiding this comment.
I don't see the revert. The new release please still has just a single job. It should hovever have two jobs as it had before. One running release-please, and a second one, depending on the first, that publishes the package after release-please has been executed.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release-please.yml:
- Line 26: Replace all GitHub Actions references in the release-please.yml
workflow file from version tags to immutable commit SHAs. Update the uses
statements for prefix-dev/setup-pixi, actions/checkout,
google-github-actions/release-please-action, and pypa/gh-action-pypi-publish to
use their respective commit SHA values instead of version tags (v0.9.6, v6, v5,
and release/v1). Find the correct commit SHA for each action version from their
respective GitHub repositories and replace the tag reference with the full
commit hash to satisfy the unpinned-uses policy.
- Around line 24-34: The workflow references Pixi environments and tasks that
don't exist in pyproject.toml. In the "Set up Pixi" step, update the
environments value from quality to match the environments actually defined in
pyproject.toml. In the "Build and check package" step, update the pixi run
command to use the correct environment name (matching what's defined in
pyproject.toml) and verify that the build-check task is properly defined in
pyproject.toml and outputs build artifacts to the dist/ directory as expected by
the subsequent publish step.
In `@pyproject.toml`:
- Around line 76-93: The pyproject.toml file is missing the `quality` and
`publish` environments and the `build` feature with its `build-check` task that
are referenced in the release-please.yml workflow. Add the missing environments
under `[tool.pixi.environments]` to define `quality` (with style feature) and
`publish` (with build feature), create a new
`[tool.pixi.feature.build.pypi-dependencies]` section with build and twine
dependencies, and add a `[tool.pixi.feature.build.tasks]` section containing the
`build-check` task that runs the python build command and twine validation to
produce artifacts in the dist directory for the downstream
pypa/gh-action-pypi-publish step.
- Around line 64-71: Remove the Poetry dependency and task from the Pixi
configuration since the project has migrated to setuptools as the build backend.
Delete the line poetry = ">=2.4.1" from the [tool.pixi.dependencies] section,
and delete the entire install task line (install = { cmd = "poetry install" })
from the [tool.pixi.tasks] section, as editable package installation is already
handled by [tool.pixi.pypi-dependencies].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 624639be-f96c-4fc1-bb8d-c3c6b6448414
📒 Files selected for processing (2)
.github/workflows/release-please.ymlpyproject.toml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
10-32: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAdd a least-privilege
permissionsblock.No
permissions:block is declared, so jobs inherit the repository default token scope, which is broader than these read-only quality jobs need. Add a top-levelpermissionsblock (or per-job) to restrict theGITHUB_TOKEN.Add near the top of the workflow (outside the selected range):
permissions: contents: read🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 10 - 32, The CI workflow jobs are read-only checks but currently inherit the default broad GITHUB_TOKEN scope. Add a top-level permissions block in the workflow so the formatting and linting jobs only get the minimum access needed, using the workflow’s top-level configuration rather than changing the individual job steps. Keep the scope limited to contents: read and place it near the top of the workflow alongside the existing formatting and linting jobs.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 10-32: The CI workflow jobs are read-only checks but currently
inherit the default broad GITHUB_TOKEN scope. Add a top-level permissions block
in the workflow so the formatting and linting jobs only get the minimum access
needed, using the workflow’s top-level configuration rather than changing the
individual job steps. Keep the scope limited to contents: read and place it near
the top of the workflow alongside the existing formatting and linting jobs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 568bc4ee-845f-423e-8471-d0c7bb504ceb
📒 Files selected for processing (1)
.github/workflows/ci.yml
|
|
||
| # Workspace table (required for preview features) | ||
| [tool.pixi.workspace] | ||
| preview = ["pixi-build"] |
There was a problem hiding this comment.
Why do you want to have pixi-build here?
| python = ">=3.11,<3.14" | ||
| pip = ">=24,<26" | ||
| packaging = ">=24,<26" |
There was a problem hiding this comment.
Why do you need these dependencies?
There was a problem hiding this comment.
Ideally, avoid conda dependencies and remove the entire section.
The python version should instead be defined via the requires-python key under [project].
There was a problem hiding this comment.
will try. Might have to do with my mingling with conda and pixi. Meanwhile, I have pixi-only env, too.
There was a problem hiding this comment.
see the other comment: Locally, not within the CI, I still seem to need pip to install in working conda envs. Yes, pixi allows for different envs. If we want to rely on pixi, only, we need to make a few adjustments, e.g. setting the pixi cache - which seems(!) impossible in the pyproject.toml (so, such settings cannot be shipped, if I am correct?).
| [tool.setuptools.packages.find] | ||
| include = ["snakemake_executor_plugin_slurm*"] | ||
| # Exclude the `pr_logo` directory (and any sub‑files) from the packaged source | ||
| exclude = ["pr_logo*"] |
There was a problem hiding this comment.
Why is setuptools still needed now what you have migrated to pixi?
There was a problem hiding this comment.
hm, I tried without and got errors. Again, might be a leftover. Will test.
There was a problem hiding this comment.
ah, I remember: I tried installing locally into a conda env with pixi and failed. So, I switched to pip. Keeping the spec in the toml lets me do that. It is not strictly necessary for pixi in the CI, yet for mixing Conda and pixi it is convenient.
I am open to alternatives: As of now, I am still new to pixi (and conda, infact).
There was a problem hiding this comment.
I am making a statement out of ignorance and because I failed so far. So, is this statement correct: If we want pixi-only dependency resolvement for Snakemake. Snakemake and its plugings need to share a common environment defined in all pyproject.tomls?
| release-type: python | ||
| token: ${{ secrets.RELEASE_PLEASE_PR_CI_TOKEN }} | ||
|
|
||
| publish: |
There was a problem hiding this comment.
I don't see the revert. The new release please still has just a single job. It should hovever have two jobs as it had before. One running release-please, and a second one, depending on the first, that publishes the package after release-please has been executed.
|
@johanneskoester now, the revert should be pushed. |
… but installment with pip works - testing in the CI now
the situation with black and python version mismatches on local and CI environments is not bearable any longer. Hence, the switch to a CI supporting pixi, ruff and flake8
Summary by CodeRabbit
throttlerdependency.