Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughReplaces Poetry with Pixi across CI and release workflows; updates GitHub Action versions and checkout; swaps Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant Repo as Repository
participant Pixi as prefix-dev/setup-pixi
participant Runner as Job Runner
participant PyPI as pypa/gh-action-pypi-publish
note over GH,Repo: CI flow migrated to Pixi (format → lint → test → coverage)
GH->>Repo: checkout (actions/checkout@v5)
GH->>Pixi: setup pixi (prefix-dev/setup-pixi@v0)
Pixi->>Runner: provide pixi CLI
Runner->>Pixi: pixi run format --check .
Runner->>Pixi: pixi run lint
Runner->>Pixi: pixi run test -v
Runner->>Pixi: pixi run coverage-report
opt Release flow (Pixi)
GH->>Repo: checkout (actions/checkout@v5)
GH->>Pixi: setup pixi
Runner->>Pixi: pixi run check-build
Runner->>PyPI: publish (token via secrets.PYPI_TOKEN)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
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: 1
🧹 Nitpick comments (4)
.gitignore (1)
162-162: Reconsider ignoring pixi.lock (reproducibility).If CI/dev tooling is driven by Pixi, committing pixi.lock improves determinism across CI and local runs. If you intend non-reproducible dev envs for a library, keep ignoring; otherwise, commit it.
Apply this diff if you decide to commit the lockfile:
-pixi.lock.github/workflows/ci.yml (3)
16-18: Enable Pixi caching to speed up CI.prefix-dev/setup-pixi supports caching; enabling it will cut resolve times on repeated runs.
Apply:
- - name: Setup pixi - uses: prefix-dev/setup-pixi@v0 + - name: Setup pixi + uses: prefix-dev/setup-pixi@v0 + with: + cache: true(Repeat for all three Setup pixi steps.)
Also applies to: 28-30, 39-41
48-52: Harden the test step and ensure module init failures fail fast.Use strict shell options so any failure in module setup stops the job; keep pixi invocation in the same shell block.
Apply:
- - name: Run pytest - run: | - source /etc/profile.d/modules.sh - module --help - pixi run test -v + - name: Run pytest + run: | + set -euo pipefail + source /etc/profile.d/modules.sh + module --version || module --help + pixi run test -v
10-10: Optional: cancel superseded CI runs.Add a concurrency group to auto-cancel prior runs on the same ref.
Apply at workflow top:
name: CI +concurrency: + group: ci-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!pyproject.toml
📒 Files selected for processing (4)
.github/workflows/ci.yml(2 hunks).github/workflows/release-please.yml(1 hunks).gitignore(1 hunks)tests/test_plugin.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
tests/test_plugin.py
🧬 Code graph analysis (1)
tests/test_plugin.py (1)
src/snakemake_software_deployment_plugin_envmodules/__init__.py (2)
Env(29-59)EnvSpec(15-26)
🪛 GitHub Actions: CI
tests/test_plugin.py
[error] 31-31: TypeError: Can't instantiate abstract class EnvSpec without an implementation for abstract method 'str' (EnvSpec('somecmd') at tests/test_plugin.py:31).
🔇 Additional comments (2)
tests/test_plugin.py (1)
15-16: LGTM on test class/interface alignment.The rename and base-class alignment with the new interface look good.
.github/workflows/release-please.yml (1)
27-41: Add minimal permissions, verify pixi build task, and migrate release-please action
- Apply the permissions diff below to give release-please the least privilege it needs and to ensure the publish job can read checkout artifacts:
name: release-please +permissions: + contents: read jobs: release-please: runs-on: ubuntu-latest + permissions: + contents: write + pull-requests: write outputs: release_created: ${{ steps.release.outputs.release_created }} steps: - uses: GoogleCloudPlatform/release-please-action@v3 id: release with: release-type: python @@ publish: runs-on: ubuntu-latest needs: release-please if: ${{ needs.release-please.outputs.release_created }} + permissions: + contents: read steps: - uses: actions/checkout@v5 @@ - name: Build source and wheel distribution + check build run: | pixi run check-build
- Remove the unused env var if truly unused:
-env: - PYTHON_VERSION: 3.11
Verification notes and required action:
- The repo search for pixi configuration failed: running the supplied script returned "pixi.toml: No such file or directory (os error 2)". Cannot confirm that "pixi run check-build" exists or that it creates dist/* artifacts required by pypa/gh-action-pypi-publish. Confirm where pixi config lives or run these checks locally and report results:
- rg -nC2 'check-build' -S
- rg -nC3 'python -m build|hatch build|twine upload' -S
- Or run: pixi run check-build && ls -la dist/
- Confirm the PYPI_TOKEN secret is present in repository secrets.
Action update:
- GoogleCloudPlatform/release-please-action@v3 is from an archived repo; migrate to googleapis/release-please-action@v4 (v3 is effectively unsupported).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
16-18: Pin GitHub Actions to a commit SHA (supply‑chain hardening).Use a commit SHA for prefix-dev/setup-pixi (and optionally actions/checkout) to avoid tag drift.
Also applies to: 28-30, 39-41
src/snakemake_software_deployment_plugin_envmodules/__init__.py (1)
51-51: Source Modules/Lmod init before using ‘module’ in decorated commands.New shells spawned by Snakemake may not have the function defined.
Apply this diff:
- return f"module purge && module load {' '.join(self.spec.names)} && {cmd}" + return ( + "if [ -f /etc/profile.d/modules.sh ]; then . /etc/profile.d/modules.sh; " + "elif [ -f /usr/share/lmod/lmod/init/bash ]; then . /usr/share/lmod/lmod/init/bash; fi; " + f"module purge && module load {' '.join(self.spec.names)} && {cmd}" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!pyproject.toml
📒 Files selected for processing (2)
.github/workflows/ci.yml(1 hunks)src/snakemake_software_deployment_plugin_envmodules/__init__.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
src/snakemake_software_deployment_plugin_envmodules/__init__.py
🪛 GitHub Actions: CI
src/snakemake_software_deployment_plugin_envmodules/__init__.py
[error] 40-40: WorkflowError: The module command is not available. Please make sure that environment modules are available on your system. (Occurred during tests/test_plugin.py::Test::test_shellcmd)
[error] 40-40: WorkflowError: The module command is not available. Please make sure that environment modules are available on your system. (Occurred during tests/test_plugin.py::Test::test_deploy)
[error] 40-40: WorkflowError: The module command is not available. Please make sure that environment modules are available on your system. (Occurred during tests/test_plugin.py::Test::test_cache)
[error] 40-40: WorkflowError: The module command is not available. Please make sure that environment modules are available on your system. (Occurred during tests/test_plugin.py::Test::test_pin)
[error] 40-40: WorkflowError: The module command is not available. Please make sure that environment modules are available on your system. (Occurred during tests/test_plugin.py::Test::test_report_software)
🔇 Additional comments (5)
.github/workflows/ci.yml (1)
48-53: CI failures stem from plugin check(), not the workflow.You already install and source environment‑modules here; the Python plugin’s subprocess check doesn’t see the shell function. Fix in the plugin (see init.py comments); no workflow change needed.
src/snakemake_software_deployment_plugin_envmodules/__init__.py (4)
18-18: Good: immutable tuple for names.Tuple[str, ...] + tuple() assignment improves identity/hash stability.
28-30: Good: concise string repr.Comma-joined names make logs/debugging clearer.
1-1: Confirm supported Python baseline for PEP 585 builtins.Using tuple[str, ...] requires Python ≥3.9. Ensure project metadata/tooling enforces this.
39-44: Fix false-negative “module command not available” check (causes CI failures).Check for Environment Modules / Lmod binaries or init scripts first and only fall back to probing the shell function; the current self.run_cmd("type module") can false-negative in CI. File: src/snakemake_software_deployment_plugin_envmodules/init.py — check().
Apply this diff:
@@ - def check(self) -> None: - if self.run_cmd("type module", stdout=sp.PIPE, stderr=sp.PIPE).returncode != 0: - raise WorkflowError( - "The module command is not available. " - "Please make sure that environment modules are " - "available on your system." - ) + def check(self) -> None: + # Accept either Environment Modules (modulecmd) or Lmod, or presence of init scripts. + from os.path import exists + from shutil import which + + available = any( + [ + exists("/etc/profile.d/modules.sh"), # Modules init (Debian/Ubuntu) + exists("/usr/share/lmod/lmod/init/bash"), # Lmod init (common path) + which("modulecmd") is not None, # Modules binary + which("lmod") is not None, # Lmod driver + ] + ) or ( + # As a last resort, probe whether a shell already has the function. + self.run_cmd("bash -lc 'type module'", + stdout=sp.PIPE, stderr=sp.PIPE).returncode == 0 + ) + + if not available: + raise WorkflowError( + "The module command is not available. " + "Please ensure Environment Modules or Lmod is installed and " + "its init script is present (e.g. /etc/profile.d/modules.sh or " + "/usr/share/lmod/lmod/init/bash)." + )EnvBase.run_cmd is defined in snakemake_interface_software_deployment_plugins (outside this repo); confirm it executes via a shell (or adapt checks to use shutil.which/os.path.exists) and then run CI.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/snakemake_software_deployment_plugin_envmodules/__init__.py (2)
39-45: Harden availability check and message decoding.
- Prefer
command -v modulefor POSIX portability;typeoutput varies by shell.- Decode with
errors="replace"to avoidUnicodeDecodeErroron odd locales.- If Ruff TRY003 is enforced, add
# noqa: TRY003on theraiseline.Suggested patch:
- res = self.run_cmd("module --help && type module", stdout=sp.PIPE, stderr=sp.STDOUT) + res = self.run_cmd( + "command -v module >/dev/null 2>&1 || module --help >/dev/null 2>&1", + stdout=sp.PIPE, + stderr=sp.STDOUT, + ) if res.returncode != 0: - raise WorkflowError( + raise WorkflowError( # noqa: TRY003 "The module command is not available. " - "Please make sure that environment modules are " - f"available on your system: {res.stdout.decode()}" + "Please make sure that environment modules are " + f"available on your system: {res.stdout.decode(errors='replace')}" )Ensure
EnvBase.run_cmdexecutes via a shell socommand -vand||behave as expected.
52-52: Handle empty module lists to avoidmodule loadwith no args.
Skip the load step ifself.spec.namesis empty.- return f"module purge && module load {' '.join(self.spec.names)} && {cmd}" + loads = f"module load {' '.join(self.spec.names)} && " if self.spec.names else "" + return f"module purge && {loads}{cmd}".github/workflows/ci.yml (2)
35-55: Login shell for modules — good; small consistency tweaks.
- Keeping
bash -el {0}for pytest is right to exposemodule. Consider the same shell at the job level or also for Coverage for consistency.- Optional: add an explicit
module purgebefore tests to ensure a clean state (tests don’t rely on ambient modules).- name: Run pytest - shell: bash -el {0} + shell: bash -el {0} run: | - command -v module + command -v module + module purge || true pixi run test -v
10-21: Pin actions to immutable SHAs (supply‑chain hardening)File: .github/workflows/ci.yml (lines 10–21) — replace floating tags with commit SHAs:
- actions/checkout@v5 → actions/checkout@08c6903 (v5.0.0, released Aug 11, 2025; requires runner ≥ v2.327.1).
- prefix-dev/setup-pixi@v0 → pin to the action's release commit SHA (replace @v0 with the specific commit SHA).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!pyproject.toml
📒 Files selected for processing (2)
.github/workflows/ci.yml(1 hunks)src/snakemake_software_deployment_plugin_envmodules/__init__.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
src/snakemake_software_deployment_plugin_envmodules/__init__.py
🪛 Ruff (0.12.2)
src/snakemake_software_deployment_plugin_envmodules/__init__.py
41-45: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testing
🔇 Additional comments (4)
src/snakemake_software_deployment_plugin_envmodules/__init__.py (3)
1-1: PEP 585 generics + trimmed imports are correct.
UsingIterableonly (droppingTuple) is appropriate withtuple[str, ...]below.
16-19: Makenamesimmutable/tuple[str, ...] — good choice.
This aligns with identity/hash usage and prevents accidental mutation.
28-30: String representation matches identity — good.
",".join(self.names)is stable and useful for logs/artifacts..github/workflows/ci.yml (1)
22-33: Confirm pixi task names exist.
CI runspixi run lint(.github/workflows/ci.yml lines 22–33); verify the pixi manifest (pyproject.toml or pixi.toml) declares tasks namedformatandlintunder[tool.pixi.tasks], or update the workflow to match existing task names.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_plugin.py (1)
17-17: Avoid mutable class attribute; use tuple.
Prevents accidental mutation and silences RUF012 without extra annotations.- shell_executable = ["bash", "-l", "-c"] + shell_executable = ("bash", "-l", "-c")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/snakemake_software_deployment_plugin_envmodules/__init__.py(4 hunks)tests/test_plugin.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
tests/test_plugin.pysrc/snakemake_software_deployment_plugin_envmodules/__init__.py
🧬 Code graph analysis (1)
tests/test_plugin.py (1)
src/snakemake_software_deployment_plugin_envmodules/__init__.py (2)
Env(32-65)EnvSpec(15-29)
🪛 Ruff (0.12.2)
tests/test_plugin.py
17-17: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
src/snakemake_software_deployment_plugin_envmodules/__init__.py
43-47: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testing
🔇 Additional comments (9)
src/snakemake_software_deployment_plugin_envmodules/__init__.py (4)
1-1: Import cleanup looks good.
Iterableonly is appropriate after switching to built-intuple[...].
18-18: Good: make names immutable.
Storingnamesastuple[str, ...]prevents accidental mutation.
28-30: Confirm expected str format (manual verification required)Current implementation returns a plain comma-joined string; some consumers expect a provider-prefixed form like envmodules(a,b). I couldn't verify any callers in the repo (ripgrep returned "No files were searched"). File: src/snakemake_software_deployment_plugin_envmodules/init.py (lines 28–30):
def __str__(self) -> str: return ",".join(self.names)Confirm whether the interface should return the plain comma-join or a prefixed form (e.g.,
envmodules({",".join(...)})). To locate callers locally run:
rg -nC2 -S -e 'envmodules(' -e 'str(' -e 'EnvSpec' --hidden --no-ignore
39-47: Verify run_cmd signature and login-shell behavior before changing .decode() → prefer text=True if supported.File: src/snakemake_software_deployment_plugin_envmodules/init.py Lines: 39–47. I couldn't find run_cmd in this repo; tests set shell_executable = ["bash","-l","-c"]. Confirm run_cmd forwards subprocess.run kwargs (accepts text=True / universal_newlines) and that it runs a login shell. If so, apply the patch below; otherwise update run_cmd or keep .decode().
Apply:
- res = self.run_cmd( - "module --help && type module", stdout=sp.PIPE, stderr=sp.STDOUT - ) + res = self.run_cmd( + "module --help && type module", + stdout=sp.PIPE, + stderr=sp.STDOUT, + text=True, + ) if res.returncode != 0: - raise WorkflowError( - "The module command is not available. " - "Please make sure that environment modules are " - f"available on your system: {res.stdout.decode()}" - ) + out = (res.stdout or "").strip() + raise WorkflowError( + "The module command is not available. Please ensure environment modules are installed and initialized. " + f"Output: {out}" + )tests/test_plugin.py (5)
3-3: Import change is correct.
UsingEnvSpecBasefrom the interface package aligns with the updated API.
10-10: Import target update looks right.
Directly importingEnv, EnvSpecfrom the plugin is expected.
15-16: Test activation/name change OK.
Class rename and__test__flag keep auto-discovery intact.
34-37: Settings hook stub is fine.
Explicitly returningNonematches a no-settings plugin.
38-44: Settings instance hook stub is fine.
ReturningNoneis consistent withget_settings_cls.
src/snakemake_software_deployment_plugin_envmodules/__init__.py
Outdated
Show resolved
Hide resolved
🤖 I have created a release *beep* *boop* --- ## [0.1.5](v0.1.4...v0.1.5) (2025-09-19) ### Bug Fixes * update to latest interface ([#10](#10)) ([1b472a4](1b472a4)) * update to latest interface ([#8](#8)) ([87d0e9d](87d0e9d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 participant
Summary by CodeRabbit