Skip to content

fix: mount local storage prefix into containers#3840

Merged
johanneskoester merged 10 commits intomainfrom
fix/mount-local-storage-prefix
Nov 27, 2025
Merged

fix: mount local storage prefix into containers#3840
johanneskoester merged 10 commits intomainfrom
fix/mount-local-storage-prefix

Conversation

@johanneskoester
Copy link
Copy Markdown
Contributor

@johanneskoester johanneskoester commented Nov 13, 2025

Description

QC

  • The PR contains a test case for the changes or the changes are already covered by an existing test case.
  • The documentation (docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).

Summary by CodeRabbit

  • New Features
    • Workflows expose runtime paths that are propagated to scripts, notebooks, wrappers, CWL jobs, and executors.
    • A configurable local storage prefix is accepted and forwarded across relevant components.
    • Container executions now accept and apply runtime/local storage binds so runtime paths are available inside containers.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Nov 13, 2025

📝 Walkthrough

Walkthrough

Added propagation of runtime/local-storage paths: a new Workflow.runtime_paths property; threaded runtime_paths/local_storage_prefix through parser-generated signatures, entry points (script/notebook/cwl/wrapper), executors, shell/conda/singularity plumbing; Singularity gained a generic bind parameter.

Changes

Cohort / File(s) Summary
Parser code generation
src/snakemake/parser.py
Emitted function signatures and argument lists now include runtime_paths for Run.start and the argument emitters for Script, Notebook, Wrapper, and CWL.
Workflow API
src/snakemake/workflow.py
Added @property runtime_paths(self) -> List[Path] returning [self.storage_settings.local_storage_prefix, self.sourcecache.cache_path] (asserts storage_settings exists).
Local executor
src/snakemake/executors/local.py
run_wrapper() signature now accepts runtime_paths; job_args_and_prepare() returns self.workflow.runtime_paths; runtime_paths threaded into all run invocation paths.
Script API & executors
src/snakemake/script/__init__.py
ScriptBase.__init__ and script factory accept/store runtime_paths; _execute_cmd passes runtime_paths to shell execution; executor constructors receive runtime_paths.
Notebook, CWL & Wrapper entry points
src/snakemake/notebook.py, src/snakemake/cwl.py, src/snakemake/wrapper.py
notebook(), cwl(), and wrapper() signatures extended to accept and forward local_storage_prefix / runtime_paths into executor/script invocation.
Container tooling — Singularity
src/snakemake/deployment/singularity.py
shellcmd() signature changed to accept bind: Optional[List[Path]]; bind list handling added to construct --bind args and skip non-existent paths; removed prior source-cache-specific mount logic.
Conda integration
src/snakemake/deployment/conda.py
Env gains _runtime_paths and calls singularity.shellcmd(..., bind=self._runtime_paths) for environment creation/checks.
Shell plumbing
src/snakemake/shell.py
Execution context exposes runtime_paths; calls to singularity.shellcmd() switched to keyword args bind=runtime_paths and args=singularity_args.

Sequence Diagram(s)

sequenceDiagram
    participant Parser as Parser (codegen)
    participant Workflow as Workflow (decorator)
    participant Local as Local Executor
    participant Script as Script / Executor
    participant Sing as Singularity backend
    participant Conda as Conda Env

    Note over Parser,Workflow: Parser emits rule funcs including runtime_paths/local_storage_prefix
    Parser->>Workflow: register rule (includes runtime_paths)
    Workflow->>Local: schedule run_wrapper(..., runtime_paths)
    Local->>Local: job_args_and_prepare() -> returns runtime_paths
    Local->>Script: execute(..., runtime_paths / local_storage_prefix)
    Script->>Script: store runtime_paths / local_storage_prefix
    Script->>Sing: shellcmd(cmd, bind=runtime_paths, args=...)
    alt bind list present
        Sing->>Sing: append --bind entries for each existing path
    else
        Sing->>Sing: proceed without extra binds
    end
    Conda->>Sing: singularity.shellcmd(..., bind=_runtime_paths) for env ops
    Sing->>Sing: spawn container with final args
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Areas needing extra attention:
    • Singularity bind-list construction and path string formatting in src/snakemake/deployment/singularity.py.
    • Consistent threading and ordering of runtime_paths/local_storage_prefix across parser.py, entry points, executors, and script executor paths.
    • Calls in src/snakemake/deployment/conda.py where bind=self._runtime_paths is introduced.
    • API/backwards-compatibility impact of added parameters on public signatures (parser-generated functions and entry points).

Possibly related PRs

Suggested reviewers

  • cademirch

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 26.09% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: mount local storage prefix into containers' directly and clearly describes the main purpose of the PR changes, which focus on propagating local_storage_prefix through container execution paths.
Description check ✅ Passed The PR description follows the template structure with QC checklist items completed, but lacks substantive implementation details about what was changed and why.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/mount-local-storage-prefix

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

Please format your code with pixi run format

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e165388 and 4b65db3.

📒 Files selected for processing (3)
  • src/snakemake/deployment/conda.py (4 hunks)
  • src/snakemake/deployment/singularity.py (3 hunks)
  • src/snakemake/shell.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

Files:

  • src/snakemake/deployment/conda.py
  • src/snakemake/shell.py
  • src/snakemake/deployment/singularity.py
🧠 Learnings (1)
📚 Learning: 2024-10-11T13:12:35.827Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 3132
File: snakemake/deployment/conda.py:85-88
Timestamp: 2024-10-11T13:12:35.827Z
Learning: In the `snakemake/deployment/conda.py` file, within the `Env` class, validation of `env_dir` occurs later in the code, so it's unnecessary to validate `env_dir` in the `__init__` method.

Applied to files:

  • src/snakemake/deployment/conda.py
🧬 Code graph analysis (1)
src/snakemake/deployment/singularity.py (1)
src/snakemake/parser.py (7)
  • args (631-632)
  • args (678-679)
  • args (686-692)
  • args (699-705)
  • args (712-719)
  • args (726-727)
  • args (734-739)
🪛 Ruff (0.14.4)
src/snakemake/deployment/singularity.py

149-149: Use explicit conversion flag

Replace with conversion flag

(RUF010)

⏰ 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). (36)
  • GitHub Check: tests (9, ubuntu-latest, py313)
  • GitHub Check: tests (10, windows-2022, py313)
  • GitHub Check: tests (10, ubuntu-latest, py311)
  • GitHub Check: tests (10, macos-latest, py313)
  • GitHub Check: tests (9, macos-latest, py313)
  • GitHub Check: tests (9, ubuntu-latest, py311)
  • GitHub Check: tests (9, windows-2022, py313)
  • GitHub Check: tests (10, ubuntu-latest, py313)
  • GitHub Check: tests (10, ubuntu-latest, py312)
  • GitHub Check: tests (7, macos-latest, py313)
  • GitHub Check: tests (9, ubuntu-latest, py312)
  • GitHub Check: tests (8, macos-latest, py313)
  • GitHub Check: tests (8, ubuntu-latest, py312)
  • GitHub Check: tests (8, ubuntu-latest, py313)
  • GitHub Check: tests (7, ubuntu-latest, py313)
  • GitHub Check: tests (7, ubuntu-latest, py312)
  • GitHub Check: tests (7, windows-2022, py313)
  • GitHub Check: tests (7, ubuntu-latest, py311)
  • GitHub Check: tests (6, windows-2022, py313)
  • GitHub Check: tests (3, macos-latest, py313)
  • GitHub Check: tests (4, ubuntu-latest, py311)
  • GitHub Check: tests (5, ubuntu-latest, py311)
  • GitHub Check: tests (4, ubuntu-latest, py312)
  • GitHub Check: tests (5, windows-2022, py313)
  • GitHub Check: tests (5, ubuntu-latest, py313)
  • GitHub Check: tests (4, windows-2022, py313)
  • GitHub Check: tests (5, ubuntu-latest, py312)
  • GitHub Check: tests (4, ubuntu-latest, py313)
  • GitHub Check: tests (3, windows-2022, py313)
  • GitHub Check: tests (3, ubuntu-latest, py311)
  • GitHub Check: tests (3, ubuntu-latest, py312)
  • GitHub Check: tests (2, ubuntu-latest, py312)
  • GitHub Check: tests (3, ubuntu-latest, py313)
  • GitHub Check: tests (2, windows-2022, py313)
  • GitHub Check: tests (2, ubuntu-latest, py313)
  • GitHub Check: tests (2, ubuntu-latest, py311)
🔇 Additional comments (5)
src/snakemake/deployment/singularity.py (1)

11-11: LGTM!

The type annotations improve code clarity and are correctly applied.

Also applies to: 96-97

src/snakemake/shell.py (2)

209-209: LGTM!

The extraction of local_storage_prefix from the context follows the established pattern and is correctly implemented.


254-263: LGTM!

The parameter passing is correct and the use of keyword arguments improves code clarity. The integration with the updated singularity.shellcmd signature is properly implemented.

src/snakemake/deployment/conda.py (2)

107-107: LGTM!

The initialization of _local_storage_prefix follows the established pattern and correctly extracts the value from workflow settings.


461-461: Add local_storage_prefix parameter to one missed call site.

The verification found that all singularity.shellcmd() calls have been updated with the local_storage_prefix parameter EXCEPT one:

  • src/snakemake/deployment/conda.py:740 is missing the parameter. Update this line from:
    return singularity.shellcmd(self.container_img, cmd, quiet=True)
    to include local_storage_prefix=self._local_storage_prefix,
⛔ Skipped due to learnings
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 3132
File: snakemake/deployment/conda.py:85-88
Timestamp: 2024-10-11T13:12:35.827Z
Learning: In the `snakemake/deployment/conda.py` file, within the `Env` class, validation of `env_dir` occurs later in the code, so it's unnecessary to validate `env_dir` in the `__init__` method.

Comment thread src/snakemake/deployment/singularity.py Outdated
@GaspardR
Copy link
Copy Markdown
Contributor

Hi @johanneskoester

Would it be possible to add the same logic when the CLI arg --cache is used? Currently, when --cache is used with --sdm apptainer, the user has to add the extra CLI args --apptainer-args "-B $SNAKEMAKE_OUTPUT_CACHE:$SNAKEMAKE_OUTPUT_CACHE", else the cache is not mounted to the container at runtime and it fails. This addition would allow for a seamless usage between --cache and --sdm apptainer.

Let me know if you think this is out of scope for this PR, I can open an new issue and/or PR if you prefer! Thanks in advance!

@johanneskoester
Copy link
Copy Markdown
Contributor Author

Hi @johanneskoester

Would it be possible to add the same logic when the CLI arg --cache is used? Currently, when --cache is used with --sdm apptainer, the user has to add the extra CLI args --apptainer-args "-B $SNAKEMAKE_OUTPUT_CACHE:$SNAKEMAKE_OUTPUT_CACHE", else the cache is not mounted to the container at runtime and it fails. This addition would allow for a seamless usage between --cache and --sdm apptainer.

Let me know if you think this is out of scope for this PR, I can open an new issue and/or PR if you prefer! Thanks in advance!

Hi, good catch! Indeed, we should solve both here. I will try to solve it generically and future proof directly in this PR.

@GaspardR
Copy link
Copy Markdown
Contributor

GaspardR commented Nov 17, 2025

Great, thank you very much!

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/snakemake/deployment/singularity.py (1)

144-149: Refine the implementation to fix RUF010 warning and add debug logging.

Good progress on addressing the previous review—the existence check is now present. However, two improvements remain:

  1. Fix RUF010 warning: Replace repr(str(local_storage_prefix)) with the f-string conversion flag {local_storage_prefix!r} for cleaner code.
  2. Add debug logging: When the path doesn't exist, log a debug message for consistency with the source cache handling pattern (lines 137-142).

Apply this diff:

-    if local_storage_prefix is not None and not local_storage_prefix.is_relative_to(
-        Path.cwd()
-    ) and local_storage_prefix.exists():
-        # if the local storage prefix is outside of the working directory,
-        # bind mount it into the container
-        args += f" --bind {repr(str(local_storage_prefix))}"
+    if local_storage_prefix is not None and not local_storage_prefix.is_relative_to(
+        Path.cwd()
+    ):
+        if local_storage_prefix.exists():
+            # if the local storage prefix is outside of the working directory,
+            # bind mount it into the container
+            args += f" --bind {local_storage_prefix!r}"
+        else:
+            logger.debug(
+                f"Local storage prefix {local_storage_prefix} does not exist, skipping bind mount"
+            )
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4b65db3 and 398b34a.

📒 Files selected for processing (1)
  • src/snakemake/deployment/singularity.py (3 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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

Files:

  • src/snakemake/deployment/singularity.py
🧬 Code graph analysis (1)
src/snakemake/deployment/singularity.py (1)
src/snakemake/parser.py (7)
  • args (631-632)
  • args (678-679)
  • args (686-692)
  • args (699-705)
  • args (712-719)
  • args (726-727)
  • args (734-739)
🪛 Ruff (0.14.5)
src/snakemake/deployment/singularity.py

149-149: Use explicit conversion flag

Replace with conversion flag

(RUF010)

⏰ 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). (22)
  • GitHub Check: tests (7, ubuntu-latest, py312)
  • GitHub Check: tests (7, windows-2022, py313)
  • GitHub Check: tests (6, windows-2022, py313)
  • GitHub Check: tests (4, ubuntu-latest, py313)
  • GitHub Check: tests (4, windows-2022, py313)
  • GitHub Check: tests (5, windows-2022, py313)
  • GitHub Check: tests (5, ubuntu-latest, py313)
  • GitHub Check: tests (5, ubuntu-latest, py312)
  • GitHub Check: tests (3, windows-2022, py313)
  • GitHub Check: tests (3, ubuntu-latest, py313)
  • GitHub Check: tests (4, ubuntu-latest, py312)
  • GitHub Check: tests (3, ubuntu-latest, py311)
  • GitHub Check: tests (3, ubuntu-latest, py312)
  • GitHub Check: tests (2, ubuntu-latest, py311)
  • GitHub Check: tests (2, ubuntu-latest, py313)
  • GitHub Check: tests (2, windows-2022, py313)
  • GitHub Check: tests (1, ubuntu-latest, py311)
  • GitHub Check: tests (1, windows-2022, py313)
  • GitHub Check: tests (1, ubuntu-latest, py312)
  • GitHub Check: tests (1, macos-latest, py313)
  • GitHub Check: tests (1, ubuntu-latest, py313)
  • GitHub Check: apidocs
🔇 Additional comments (2)
src/snakemake/deployment/singularity.py (2)

11-11: LGTM!

The Optional import is correctly added to support the type annotation for local_storage_prefix.


94-104: LGTM!

The function signature changes are well-structured:

  • Type annotation for cmd improves clarity
  • local_storage_prefix parameter with appropriate typing and default value aligns with the PR's objective to thread this parameter through the codebase

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/snakemake/deployment/singularity.py (1)

144-152: Consider simplifying repr usage and adding debug logging for consistency.

The existence check requested in previous reviews is now correctly present (line 147). However, two minor improvements would enhance consistency with the established pattern (lines 137-142):

  1. Simplify repr usage: Line 151 uses repr(str(local_storage_prefix)) which can be simplified to the more idiomatic f-string conversion flag {local_storage_prefix!r}.

  2. Add debug logging: When local_storage_prefix is not mounted (doesn't exist or is relative to cwd), consider logging a debug message similar to the source_cache_path pattern above for easier troubleshooting.

Apply this diff to address both points:

-    if (
-        local_storage_prefix is not None
-        and not local_storage_prefix.is_relative_to(Path.cwd())
-        and local_storage_prefix.exists()
-    ):
-        # if the local storage prefix is outside of the working directory,
-        # bind mount it into the container
-        args += f" --bind {repr(str(local_storage_prefix))}"
+    if local_storage_prefix is not None and not local_storage_prefix.is_relative_to(
+        Path.cwd()
+    ):
+        if local_storage_prefix.exists():
+            # if the local storage prefix is outside of the working directory,
+            # bind mount it into the container
+            args += f" --bind {local_storage_prefix!r}"
+        else:
+            logger.debug(
+                f"Local storage prefix {local_storage_prefix} does not exist, skipping bind mount"
+            )
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 398b34a and bc89a59.

📒 Files selected for processing (1)
  • src/snakemake/deployment/singularity.py (3 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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

Files:

  • src/snakemake/deployment/singularity.py
🧬 Code graph analysis (1)
src/snakemake/deployment/singularity.py (1)
src/snakemake/parser.py (7)
  • args (631-632)
  • args (678-679)
  • args (686-692)
  • args (699-705)
  • args (712-719)
  • args (726-727)
  • args (734-739)
🪛 Ruff (0.14.5)
src/snakemake/deployment/singularity.py

151-151: Use explicit conversion flag

Replace with conversion flag

(RUF010)

⏰ 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). (46)
  • GitHub Check: tests (8, windows-2022, py313)
  • GitHub Check: tests (10, ubuntu-latest, py312)
  • GitHub Check: tests (10, windows-2022, py313)
  • GitHub Check: tests (9, ubuntu-latest, py312)
  • GitHub Check: tests (10, ubuntu-latest, py313)
  • GitHub Check: tests (9, macos-latest, py313)
  • GitHub Check: tests (9, ubuntu-latest, py311)
  • GitHub Check: tests (10, ubuntu-latest, py311)
  • GitHub Check: tests (9, windows-2022, py313)
  • GitHub Check: tests (9, ubuntu-latest, py313)
  • GitHub Check: tests (8, ubuntu-latest, py313)
  • GitHub Check: tests (8, ubuntu-latest, py312)
  • GitHub Check: tests (8, ubuntu-latest, py311)
  • GitHub Check: tests (7, windows-2022, py313)
  • GitHub Check: tests (7, ubuntu-latest, py312)
  • GitHub Check: tests (7, ubuntu-latest, py313)
  • GitHub Check: tests (7, ubuntu-latest, py311)
  • GitHub Check: tests (6, windows-2022, py313)
  • GitHub Check: tests (6, ubuntu-latest, py312)
  • GitHub Check: tests (4, ubuntu-latest, py312)
  • GitHub Check: tests (4, windows-2022, py313)
  • GitHub Check: tests (5, ubuntu-latest, py313)
  • GitHub Check: tests (5, windows-2022, py313)
  • GitHub Check: tests (6, macos-latest, py313)
  • GitHub Check: tests (6, ubuntu-latest, py313)
  • GitHub Check: tests (6, ubuntu-latest, py311)
  • GitHub Check: tests (3, macos-latest, py313)
  • GitHub Check: tests (4, ubuntu-latest, py313)
  • GitHub Check: tests (4, ubuntu-latest, py311)
  • GitHub Check: tests (3, ubuntu-latest, py313)
  • GitHub Check: tests (5, ubuntu-latest, py312)
  • GitHub Check: tests (5, ubuntu-latest, py311)
  • GitHub Check: tests (3, windows-2022, py313)
  • GitHub Check: tests (4, macos-latest, py313)
  • GitHub Check: tests (1, ubuntu-latest, py313)
  • GitHub Check: tests (3, ubuntu-latest, py311)
  • GitHub Check: tests (3, ubuntu-latest, py312)
  • GitHub Check: tests (2, ubuntu-latest, py313)
  • GitHub Check: tests (1, macos-latest, py313)
  • GitHub Check: tests (1, windows-2022, py313)
  • GitHub Check: tests (2, windows-2022, py313)
  • GitHub Check: tests (2, ubuntu-latest, py312)
  • GitHub Check: tests (2, ubuntu-latest, py311)
  • GitHub Check: tests (1, ubuntu-latest, py311)
  • GitHub Check: tests (1, ubuntu-latest, py312)
  • GitHub Check: apidocs
🔇 Additional comments (2)
src/snakemake/deployment/singularity.py (2)

11-11: LGTM! Type import added for parameter annotation.

The Optional import is correctly added to support the type annotation for local_storage_prefix parameter below.


94-97: LGTM! Function signature properly extended with type annotations.

The addition of local_storage_prefix parameter with appropriate type hints aligns with the parameter propagation through the codebase shown in parser.py and other modules.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/snakemake/workflow.py (1)

2089-2095: Guard runtime_paths against None local_storage_prefix

If storage_settings.local_storage_prefix can be None, runtime_paths will contain a non-Path despite the type hint and may later be passed to bind logic. Consider filtering it out and always returning only real paths:

     @property
     def runtime_paths(self) -> List[Path]:
         assert self.storage_settings is not None
-        return [
-            self.storage_settings.local_storage_prefix,
-            self.sourcecache.cache_path,
-        ]
+        paths: List[Path] = [self.sourcecache.cache_path]
+        local_prefix = self.storage_settings.local_storage_prefix
+        if local_prefix is not None:
+            paths.append(local_prefix)
+        return paths

This keeps semantics while avoiding None entries leaking into downstream consumers.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bc89a59 and 96ca6f2.

📒 Files selected for processing (7)
  • src/snakemake/deployment/conda.py (4 hunks)
  • src/snakemake/deployment/singularity.py (3 hunks)
  • src/snakemake/executors/local.py (5 hunks)
  • src/snakemake/parser.py (5 hunks)
  • src/snakemake/script/__init__.py (5 hunks)
  • src/snakemake/shell.py (2 hunks)
  • src/snakemake/workflow.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/snakemake/shell.py
  • src/snakemake/deployment/conda.py
🧰 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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

Files:

  • src/snakemake/executors/local.py
  • src/snakemake/parser.py
  • src/snakemake/script/__init__.py
  • src/snakemake/workflow.py
  • src/snakemake/deployment/singularity.py
🧬 Code graph analysis (3)
src/snakemake/executors/local.py (1)
src/snakemake/workflow.py (1)
  • runtime_paths (2090-2095)
src/snakemake/script/__init__.py (1)
src/snakemake/workflow.py (1)
  • runtime_paths (2090-2095)
src/snakemake/deployment/singularity.py (1)
src/snakemake/parser.py (7)
  • args (631-632)
  • args (678-679)
  • args (686-692)
  • args (699-705)
  • args (712-719)
  • args (726-727)
  • args (734-739)
🪛 Ruff (0.14.5)
src/snakemake/deployment/singularity.py

134-134: Use explicit conversion flag

Replace with conversion flag

(RUF010)

⏰ 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). (46)
  • GitHub Check: tests (8, ubuntu-latest, py312)
  • GitHub Check: tests (10, ubuntu-latest, py313)
  • GitHub Check: tests (10, ubuntu-latest, py312)
  • GitHub Check: tests (10, windows-2022, py313)
  • GitHub Check: tests (9, ubuntu-latest, py311)
  • GitHub Check: tests (9, ubuntu-latest, py312)
  • GitHub Check: tests (10, ubuntu-latest, py311)
  • GitHub Check: tests (8, ubuntu-latest, py313)
  • GitHub Check: tests (9, windows-2022, py313)
  • GitHub Check: tests (9, ubuntu-latest, py313)
  • GitHub Check: tests (8, windows-2022, py313)
  • GitHub Check: tests (8, ubuntu-latest, py311)
  • GitHub Check: tests (7, windows-2022, py313)
  • GitHub Check: tests (6, windows-2022, py313)
  • GitHub Check: tests (7, ubuntu-latest, py311)
  • GitHub Check: tests (7, ubuntu-latest, py313)
  • GitHub Check: tests (4, windows-2022, py313)
  • GitHub Check: tests (7, ubuntu-latest, py312)
  • GitHub Check: tests (6, macos-latest, py313)
  • GitHub Check: tests (5, windows-2022, py313)
  • GitHub Check: tests (6, ubuntu-latest, py311)
  • GitHub Check: tests (6, ubuntu-latest, py313)
  • GitHub Check: tests (5, macos-latest, py313)
  • GitHub Check: tests (6, ubuntu-latest, py312)
  • GitHub Check: tests (5, ubuntu-latest, py313)
  • GitHub Check: tests (5, ubuntu-latest, py311)
  • GitHub Check: tests (4, ubuntu-latest, py313)
  • GitHub Check: tests (5, ubuntu-latest, py312)
  • GitHub Check: tests (4, ubuntu-latest, py312)
  • GitHub Check: tests (3, ubuntu-latest, py311)
  • GitHub Check: tests (3, windows-2022, py313)
  • GitHub Check: tests (2, windows-2022, py313)
  • GitHub Check: tests (3, macos-latest, py313)
  • GitHub Check: tests (3, ubuntu-latest, py313)
  • GitHub Check: tests (4, ubuntu-latest, py311)
  • GitHub Check: tests (1, ubuntu-latest, py311)
  • GitHub Check: tests (1, macos-latest, py313)
  • GitHub Check: tests (2, macos-latest, py313)
  • GitHub Check: tests (1, windows-2022, py313)
  • GitHub Check: tests (1, ubuntu-latest, py312)
  • GitHub Check: tests (3, ubuntu-latest, py312)
  • GitHub Check: tests (2, ubuntu-latest, py313)
  • GitHub Check: tests (2, ubuntu-latest, py311)
  • GitHub Check: tests (2, ubuntu-latest, py312)
  • GitHub Check: tests (1, ubuntu-latest, py313)
  • GitHub Check: apidocs
🔇 Additional comments (4)
src/snakemake/script/__init__.py (1)

574-601: Runtime path propagation for scripts is wired correctly

runtime_paths is accepted by script(...), stored on ScriptBase, and forwarded to shell(...) via _execute_cmd. All concrete script types inherit the updated ScriptBase.__init__, so the additional argument is consistently applied without duplicating logic.

No issues from a correctness or API-usage perspective.

Also applies to: 627-627, 674-687, 1715-1740, 1770-1796

src/snakemake/parser.py (2)

583-589: run-function signature extension with runtime_paths matches executor calls

The generated __rule_{rulename} now accepts runtime_paths just before the {rule_func_marker} default. This ordering matches how run_wrapper now calls run(...) (with basedir, sourcecache_path, runtime_sourcecache_path, runtime_paths at the tail), so the additional parameter is threaded through without changing call semantics for existing code.

Looks consistent with the executor-side changes.


687-692: Script/Notebook/Wrapper/CWL argument lists are aligned with new runtime path plumbing

All four emitters append runtime_paths at the end of their argument lists:

  • script(...): ..., sourcecache_path, runtime_sourcecache_path, runtime_paths
  • notebook(...): ..., edit_notebook, sourcecache_path, runtime_sourcecache_path, runtime_paths
  • wrapper(...): ..., cleanup_scripts, shadow_dir, sourcecache_path, runtime_sourcecache_path, runtime_paths
  • cwl(...): ..., sourcecache_path, runtime_sourcecache_path, runtime_paths

This matches the updated public signatures in the respective modules and keeps positional ordering stable for preceding parameters.

Also applies to: 699-705, 713-719, 735-739

src/snakemake/executors/local.py (1)

136-174: Code changes verified as correct—parameter threading is consistent

run_wrapper has exactly 27 parameters matching the 27 return elements from job_args_and_prepare, with runtime_paths correctly positioned as the final parameter. The mechanism via cached_or_run properly unpacks and forwards all arguments. No external call sites exist outside local.py.

Comment thread src/snakemake/deployment/singularity.py
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d801df1 and 33c4bf6.

📒 Files selected for processing (1)
  • src/snakemake/deployment/singularity.py (3 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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

Files:

  • src/snakemake/deployment/singularity.py
🧬 Code graph analysis (1)
src/snakemake/deployment/singularity.py (1)
src/snakemake/common/__init__.py (1)
  • get_snakemake_searchpaths (51-55)
🪛 Ruff (0.14.6)
src/snakemake/deployment/singularity.py

124-126: zip() without an explicit strict= parameter

Add explicit value for parameter strict=

(B905)

🔇 Additional comments (5)
src/snakemake/deployment/singularity.py (5)

11-11: LGTM: Typing imports correctly added.

The List and Optional imports support the new bind parameter's type annotation.


96-97: LGTM: Generic bind parameter enables flexible mount support.

The addition of bind: Optional[List[Path]] provides a clean, type-safe interface for mounting arbitrary paths into containers, directly supporting the PR's objective to fix local storage prefix mounting.


123-123: LGTM: Conversion flag correctly addresses RUF010.

The change from repr(str(searchpath)) to str(searchpath)!r satisfies the linter while maintaining identical behavior. The str() conversion is necessary to get the string path (rather than PosixPath(...) representation) before applying repr() for proper shell quoting.


130-130: LGTM: Conversion flag correctly applied.

The change to str(container_workdir)!r addresses RUF010 while maintaining proper shell quoting.


124-126: Based on my verification, I can now provide the rewritten review comment.

The zip() call has guaranteed matching lengths by construction, but adding strict=True requires Python 3.10+ compatibility confirmation.

The lengths are guaranteed to match because get_snakemake_searchpath_mountpoints() explicitly uses range(len(paths)) where paths = get_snakemake_searchpaths(). However, the codebase currently does not use strict=True anywhere, and the minimum Python version requirement could not be clearly determined from configuration files. Before suggesting this refactor, verify that the project officially requires Python 3.10 or higher. If it does not, this suggestion should be deferred or conditional.

Comment thread src/snakemake/deployment/singularity.py
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/snakemake/deployment/singularity.py (1)

120-127: Consider adding strict=True to zip() for clearer invariants (Ruff B905).

Ruff’s B905 hint is relevant here: get_snakemake_searchpaths() and get_snakemake_searchpath_mountpoints() are expected to stay in lock‑step. Using strict=True makes that assumption explicit and will fail fast if it’s ever violated, rather than silently truncating.

If your minimum Python version is ≥3.10, you can adopt this and satisfy the linter:

-        args += " ".join(
-            f" --bind {str(searchpath)!r}:{str(mountpoint)!r}"
-            for searchpath, mountpoint in zip(
-                get_snakemake_searchpaths(), get_snakemake_searchpath_mountpoints()
-            )
-        )
+        args += " ".join(
+            f" --bind {str(searchpath)!r}:{str(mountpoint)!r}"
+            for searchpath, mountpoint in zip(
+                get_snakemake_searchpaths(),
+                get_snakemake_searchpath_mountpoints(),
+                strict=True,
+            )
+        )
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 33c4bf6 and b7e1ad1.

📒 Files selected for processing (1)
  • src/snakemake/deployment/singularity.py (3 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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

Files:

  • src/snakemake/deployment/singularity.py
🧬 Code graph analysis (1)
src/snakemake/deployment/singularity.py (1)
src/snakemake/common/__init__.py (1)
  • get_snakemake_searchpaths (51-55)
🪛 Ruff (0.14.6)
src/snakemake/deployment/singularity.py

124-126: zip() without an explicit strict= parameter

Add explicit value for parameter strict=

(B905)

⏰ 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). (41)
  • GitHub Check: tests (10, ubuntu-latest, py313)
  • GitHub Check: tests (10, windows-2022, py313)
  • GitHub Check: tests (10, ubuntu-latest, py312)
  • GitHub Check: tests (10, ubuntu-latest, py311)
  • GitHub Check: tests (9, ubuntu-latest, py311)
  • GitHub Check: tests (9, ubuntu-latest, py313)
  • GitHub Check: tests (9, ubuntu-latest, py312)
  • GitHub Check: tests (8, windows-2022, py313)
  • GitHub Check: tests (9, windows-2022, py313)
  • GitHub Check: tests (8, ubuntu-latest, py313)
  • GitHub Check: tests (7, windows-2022, py313)
  • GitHub Check: tests (8, ubuntu-latest, py311)
  • GitHub Check: tests (8, ubuntu-latest, py312)
  • GitHub Check: tests (7, ubuntu-latest, py313)
  • GitHub Check: tests (7, ubuntu-latest, py311)
  • GitHub Check: tests (5, windows-2022, py313)
  • GitHub Check: tests (6, ubuntu-latest, py312)
  • GitHub Check: tests (6, windows-2022, py313)
  • GitHub Check: tests (7, ubuntu-latest, py312)
  • GitHub Check: tests (6, ubuntu-latest, py311)
  • GitHub Check: tests (6, ubuntu-latest, py313)
  • GitHub Check: tests (5, ubuntu-latest, py313)
  • GitHub Check: tests (5, ubuntu-latest, py312)
  • GitHub Check: tests (4, ubuntu-latest, py313)
  • GitHub Check: tests (4, windows-2022, py313)
  • GitHub Check: tests (5, ubuntu-latest, py311)
  • GitHub Check: tests (3, windows-2022, py313)
  • GitHub Check: tests (1, windows-2022, py313)
  • GitHub Check: tests (3, ubuntu-latest, py312)
  • GitHub Check: tests (3, ubuntu-latest, py313)
  • GitHub Check: tests (2, windows-2022, py313)
  • GitHub Check: tests (4, ubuntu-latest, py312)
  • GitHub Check: tests (4, ubuntu-latest, py311)
  • GitHub Check: tests (3, ubuntu-latest, py311)
  • GitHub Check: tests (2, ubuntu-latest, py312)
  • GitHub Check: tests (1, ubuntu-latest, py311)
  • GitHub Check: tests (2, ubuntu-latest, py313)
  • GitHub Check: tests (1, ubuntu-latest, py313)
  • GitHub Check: tests (1, ubuntu-latest, py312)
  • GitHub Check: tests (2, ubuntu-latest, py311)
  • GitHub Check: apidocs
🔇 Additional comments (3)
src/snakemake/deployment/singularity.py (3)

11-11: Typing imports match new usage.

List and Optional are used in the updated shellcmd signature and imported correctly; nothing else to change here.


129-140: New bind handling (exists check + debug logging) looks robust.

The bind loop correctly:

  • Treats bind as an optional list of Path objects,
  • Skips non‑existent paths instead of letting Singularity fail opaquely, and
  • Emits a debug log when a path is skipped, which will help diagnose missing cache/local-storage binds.

This aligns well with the PR’s goal of reliably mounting runtime/local-storage prefixes into containers.


94-104: I'll help you verify the impact of the parameter order change in the shellcmd function. Let me search the codebase to analyze how this function is being called.
<function_calls>

cd /tmp && git clone https://github.com/snakemake/snakemake.git 2>&1 | head -20

</function_calls>

Let me search for all calls to the shellcmd function:
<function_calls>

cd /tmp/snakemake && rg -n "shellcmd\s*(" src/ --type py -C 2

</function_calls>

Let me get more specific information about the function definition and all its call sites:
<function_calls>

cd /tmp/snakemake && cat -n src/snakemake/deployment/singularity.py | head -150

</function_calls>

Now let me search comprehensively for all shellcmd calls:
<function_calls>

cd /tmp/snakemake && rg "shellcmd" src/ --type py -B 2 -A 2

</function_calls>

Let me check if shellcmd is exported and used externally:
<function_calls>

cd /tmp/snakemake && rg "from.*singularity.*import.*shellcmd|import.*shellcmd" src/ --type py

</function_calls>

Let me look at the actual function definition and surrounding context more carefully:
<function_calls>

cd /tmp/snakemake && sed -n '85,115p' src/snakemake/deployment/singularity.py

</function_calls>

Now let me find where shellcmd is being called:
<function_calls>

cd /tmp/snakemake && python3 << 'EOF'
import re
import os

Search for shellcmd calls in all Python files

results = []
for root, dirs, files in os.walk("src/"):
# Skip test directories for now, we'll check them separately
if "test" in root:
continue
for file in files:
if file.endswith(".py"):
filepath = os.path.join(root, file)
try:
with open(filepath, 'r', encoding='utf-8', errors='ignore') as f:
lines = f.readlines()
for i, line in enumerate(lines, 1):
# Look for shellcmd calls (function calls, not definition)
if re.search(r'\bshellcmd\s*(', line):
# Skip the definition line itself
if "def shellcmd" not in line:
results.append((filepath, i, line.rstrip()))
except:
pass

Print results

for filepath, lineno, line in results:
print(f"{filepath}:{lineno}: {line}")
EOF


</function_calls>

Let me search more broadly for where shellcmd might be called:
<function_calls>

cd /tmp/snakemake && grep -rn "shellcmd" src/ --include="*.py" | grep -v "def shellcmd" | head -30

</function_calls>

Let me check the singularity.py file more carefully to understand the context and look for internal uses:
<function_calls>

cd /tmp/snakemake && wc -l src/snakemake/deployment/singularity.py

</function_calls>

<function_calls>

cd /tmp/snakemake && cat src/snakemake/deployment/singularity.py | grep -n "shellcmd" -C 3

</function_calls>

<function_calls>

cd /tmp/snakemake && cat -n src/snakemake/deployment/singularity.py | sed -n '94,200p'

</function_calls>

<function_calls>

cd /tmp/snakemake && python3 << 'EOF'

Extract and analyze shellcmd usage in singularity.py

with open('src/snakemake/deployment/singularity.py', 'r') as f:
lines = f.readlines()

Find all lines mentioning shellcmd

shellcmd_lines = []
for i, line in enumerate(lines, 1):
if 'shellcmd' in line:
shellcmd_lines.append((i, line.rstrip()))

print("All references to 'shellcmd' in singularity.py:")
print("=" * 60)
for lineno, line in shellcmd_lines:
print(f"{lineno:4d}: {line}")
EOF


</function_calls>

Now let me check if shellcmd is part of any public API by checking all or module exports:
<function_calls>

cd /tmp/snakemake && head -50 src/snakemake/deployment/singularity.py

</function_calls>

<function_calls>

cd /tmp/snakemake && grep -n "all" src/snakemake/deployment/singularity.py

</function_calls>

Let me check if there are any tests that use shellcmd:
<function_calls>

cd /tmp/snakemake && find . -type f -name ".py" -path "/test*" -exec grep -l "shellcmd" {} ;

</function_calls>

Let me look at how the deployment module is used in general:
<function_calls>

cd /tmp/snakemake && rg "from.*deployment.*singularity|from.*singularity" src/ --type py

</function_calls>

<function_calls>

cd /tmp/snakemake && python3 << 'EOF'
import os
import re

Search for imports from singularity.py across the entire codebase

results = []
for root, dirs, files in os.walk("src/"):
for file in files:
if file.endswith(".py"):
filepath = os.path.join(root, file)
try:
with open(filepath, 'r', encoding='utf-8', errors='ignore') as f:
content = f.read()
# Look for imports from singularity
if 'from' in content and 'singularity' in content:
lines = content.split('\n')
for i, line in enumerate(lines, 1):
if re.search(r'from\s+.*singularity', line) or re.search(r'import.*singularity', line):
results.append((filepath, i, line.strip()))
except:
pass

if results:
print("Imports from singularity module:")
print("=" * 60)
for filepath, lineno, line in results:
print(f"{filepath}:{lineno}: {line}")
else:
print("No imports found from singularity module in src/")
EOF


</function_calls>

Let me look at the deployment module structure:
<function_calls>

cd /tmp/snakemake && ls -la src/snakemake/deployment/ && cat src/snakemake/deployment/init.py

</function_calls>

Now let me get the complete view of the function signature change by examining the git history or PR:
<function_calls>

cd /tmp/snakemake && git log --oneline src/snakemake/deployment/singularity.py | head -20

</function_calls>

<function_calls>

cd /tmp/snakemake && git show HEAD:src/snakemake/deployment/singularity.py | sed -n '94,110p'

</function_calls>

Let me check if there's a current branch with the change:
<function_calls>

cd /tmp/snakemake && git branch -a

</function_calls>

Let me look at recent diffs:
<function_calls>

cd /tmp/snakemake && git diff HEAD~5 HEAD -- src/snakemake/deployment/singularity.py | head -100

</function_calls>

Let me directly examine the current state of the function and check for all calls within the file itself:
<function_calls>

cd /tmp/snakemake && python3 << 'EOF'

Parse singularity.py to understand function usage

with open('src/snakemake/deployment/singularity.py', 'r') as f:
lines = f.readlines()

print("FUNCTION DEFINITION (lines 94-110):")
print("=" * 70)
for i in range(93, min(110, len(lines))):
print(f"{i+1:3d}: {lines[i]}", end='')

print("\n\nALL CALLS TO shellcmd IN THE FILE:")
print("=" * 70)
import re
for i, line in enumerate(lines, 1):
# Find calls to shellcmd (not definitions)
if re.search(r'\bshellcmd\s*(', line) and 'def shellcmd' not in line:
# Print context (3 lines before and after)
start = max(0, i - 4)
end = min(len(lines), i + 3)
print(f"\nFound at line {i}:")
for j in range(start, end):
marker = ">>>" if j == i-1 else " "
print(f"{marker} {j+1:3d}: {lines[j]}", end='')
EOF


</function_calls>

<function_calls>

cd /tmp/snakemake && sed -n '94,250p' src/snakemake/deployment/singularity.py

</function_calls>

Perfect! Now let me search for all actual calls to shellcmd in the entire codebase to understand the calling patterns:
<function_calls>

cd /tmp/snakemake && python3 << 'EOF'
import os
import re

print("Searching for ALL calls to shellcmd() across entire codebase:")
print("=" * 80)

call_count = 0
for root, dirs, files in os.walk("."):
# Skip git and build artifacts
dirs[:] = [d for d in dirs if d not in ['.git', 'pycache', '.pytest_cache', 'build', 'dist', '.venv']]

for file in files:
    if file.endswith(".py"):
        filepath = os.path.join(root, file)
        try:
            with open(filepath, 'r', encoding='utf-8', errors='ignore') as f:
                lines = f.readlines()
                for i, line in enumerate(lines, 1):
                    # Match shellcmd calls but not definitions
                    if re.search(r'\bshellcmd\s*\(', line) and 'def shellcmd' not in line:
                        call_count += 1
                        # Get 2 lines of context
                        start = max(0, i - 3)
                        end = min(len(lines), i + 2)
                        print(f"\n[Call #{call_count}] {filepath}:{i}")
                        print("-" * 80)
                        for j in range(start, end):
                            marker = ">>>" if j == i-1 else "   "
                            print(f"{marker} {j+1:4d}: {lines[j]}", end='')
        except Exception as e:
            pass

print(f"\n\nTOTAL CALLS FOUND: {call_count}")
EOF


</function_calls>

Excellent! Now I have comprehensive information. Let me analyze the calling patterns:
<function_calls>

cd /tmp/snakemake && python3 << 'EOF'

Analyze the calling patterns found

calls = [
{
"location": "src/snakemake/deployment/singularity.py:232",
"context": """return shellcmd(
img_path=img_path,
cmd=cmd,
bind=bind,
args=args,
quiet=quiet,
envvars=envvars,
shell_executable=shell_executable,
container_workdir=container_workdir,
is_python_script=is_python_script,
)""",
"style": "keyword arguments"
},
{
"location": "src/snakemake/deployment/singularity.py:274",
"context": """containerized = shellcmd(
img_path,
cmd,
bind=bind,
args=args,
quiet=True,
envvars=container_env_vars,
shell_executable=shell_executable,
container_workdir=container_workdir,
)""",
"style": "mixed positional (img_path, cmd) + keyword (rest)"
},
{
"location": "src/snakemake/deployment/singularity.py:420",
"context": """containerized = shellcmd(
img_path,
cmd,
bind=bind,
args="",
quiet=True,
envvars=container_env_vars,
shell_executable=shell_executable,
container_workdir=container_workdir,
)""",
"style": "mixed positional (img_path, cmd) + keyword (rest)"
}
]

print("ANALYSIS OF shellcmd CALLS:")
print("=" * 80)
for i, call in enumerate(calls, 1):
print(f"\nCall #{i}: {call['location']}")
print(f"Style: {call['style']}")
print(f"Context:\n{call['context']}")

print("\n" + "=" * 80)
print("\nKEY FINDINGS:")
print("-" * 80)
print("✓ ALL calls use KEYWORD arguments for 'bind' and 'args'")
print("✓ Only 'img_path' and 'cmd' are passed positionally")
print("✓ Even though some calls pass positional args, none pass 'args' positionally")
print("\nCONCLUSION: The parameter reordering is SAFE because:")
print(" - No calls pass 'args' as the 3rd positional argument")
print(" - All uses of 'bind' and 'args' are explicit keyword arguments")
print(" - The only positional args are img_path and cmd (unchanged positions)")
EOF


</function_calls>

Perfect! Now let me verify the module is not exported as public API:
<function_calls>

cd /tmp/snakemake && python3 << 'EOF'

Check if shellcmd is documented or exported as part of public API

import os

print("Checking for public API references:")
print("=" * 80)

Check for all

with open('src/snakemake/deployment/singularity.py', 'r') as f:
content = f.read()
if 'all' in content:
print("\n✓ Found all in singularity.py:")
for line in content.split('\n'):
if 'all' in line:
print(f" {line}")
else:
print("\n✗ No all defined in singularity.py (no explicit public API)")

Check if module is imported in main init.py

init_file = 'src/snakemake/init.py'
if os.path.exists(init_file):
with open(init_file, 'r') as f:
init_content = f.read()
if 'singularity' in init_content or 'shellcmd' in init_content:
print("\n⚠ Found reference to singularity in main init.py")
else:
print("\n✓ No reference to singularity in main init.py")

Check README or docs for API references

readme_files = ['README.md', 'docs/api.md', 'docs/index.md']
for readme in readme_files:
if os.path.exists(readme):
with open(readme, 'r') as f:
content = f.read()
if 'shellcmd' in content:
print(f"\n⚠ Found 'shellcmd' mentioned in {readme}")
EOF


</function_calls>

Based on my comprehensive analysis of the codebase, I can now provide the rewritten review comment.


Parameter reordering in shellcmd is safe—all internal calls already use keyword arguments.

The concern about breaking positional callers is unfounded. All three calls to shellcmd in the codebase (lines 232, 274, and 420 of singularity.py) already pass bind and args as keyword arguments:

# Line 274, 420 patterns:
shellcmd(
    img_path,      # positional
    cmd,           # positional
    bind=bind,     # keyword
    args=args,     # keyword (or args="")
    ...
)

No call passes args positionally as the third argument, so inserting bind between cmd and args poses no compatibility risk. Additionally, shellcmd does not appear to be part of a public or semi-public API—it is an internal deployment utility with no __all__ export and no documented or re-exported interface.

@johanneskoester johanneskoester merged commit f1e8b62 into main Nov 27, 2025
60 checks passed
@johanneskoester johanneskoester deleted the fix/mount-local-storage-prefix branch November 27, 2025 15:16
johanneskoester pushed a commit that referenced this pull request Nov 27, 2025
🤖 I have created a release *beep* *boop*
---


##
[9.14.0](v9.13.7...v9.14.0)
(2025-11-27)


### Features

* Support Hy in script directive
([#3824](#3824))
([2329c9e](2329c9e))


### Bug Fixes

* Add support for pathlib in `notebook` field
([#3811](#3811))
([7b2180a](7b2180a))
* Addressed race condition in workdir_handler.py
([#3844](#3844))
([8dbfcfb](8dbfcfb))
* cleanup update-marked output files of failed jobs if there was no
backup to restore them
([#3843](#3843))
([41f1ce8](41f1ce8))
* correct Windows callable path handling
([#3832](#3832))
([5caad70](5caad70))
* expand env vars on resources
([#3823](#3823))
([fcfa1bc](fcfa1bc))
* fix backup for output marked by `update`
([#3839](#3839))
([09c64b7](09c64b7))
* Minor fixes/additions to logging module.
([#3802](#3802))
([3b3986d](3b3986d))
* mount local storage prefix into containers
([#3840](#3840))
([f1e8b62](f1e8b62))
* properly format input/output files in case of missing rule to produce
them ([#3849](#3849))
([69d5d24](69d5d24))
* Unpack AnnotatedString in _apply_wildcards
([#3798](#3798))
([7886508](7886508))


### Performance Improvements

* retrieve storage inputs immediately before scheduling jobs instead of
before running the entire workflow
([#3850](#3850))
([4ac6cda](4ac6cda))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
kjohnsen pushed a commit to kjohnsen/snakemake that referenced this pull request Dec 15, 2025
### Description

<!--Add a description of your PR here-->

### QC
<!-- Make sure that you can tick the boxes below. -->

* [x] The PR contains a test case for the changes or the changes are
already covered by an existing test case.
* [x] The documentation (`docs/`) is updated to reflect the changes or
this is not necessary (e.g. if the change does neither modify the
language nor the behavior or functionalities of Snakemake).


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Workflows expose runtime paths that are propagated to scripts,
notebooks, wrappers, CWL jobs, and executors.
* A configurable local storage prefix is accepted and forwarded across
relevant components.
* Container executions now accept and apply runtime/local storage binds
so runtime paths are available inside containers.

<sub>✏️ Tip: You can customize this high-level summary in your review
settings.</sub>
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
kjohnsen pushed a commit to kjohnsen/snakemake that referenced this pull request Dec 15, 2025
🤖 I have created a release *beep* *boop*
---


##
[9.14.0](snakemake/snakemake@v9.13.7...v9.14.0)
(2025-11-27)


### Features

* Support Hy in script directive
([snakemake#3824](snakemake#3824))
([2329c9e](snakemake@2329c9e))


### Bug Fixes

* Add support for pathlib in `notebook` field
([snakemake#3811](snakemake#3811))
([7b2180a](snakemake@7b2180a))
* Addressed race condition in workdir_handler.py
([snakemake#3844](snakemake#3844))
([8dbfcfb](snakemake@8dbfcfb))
* cleanup update-marked output files of failed jobs if there was no
backup to restore them
([snakemake#3843](snakemake#3843))
([41f1ce8](snakemake@41f1ce8))
* correct Windows callable path handling
([snakemake#3832](snakemake#3832))
([5caad70](snakemake@5caad70))
* expand env vars on resources
([snakemake#3823](snakemake#3823))
([fcfa1bc](snakemake@fcfa1bc))
* fix backup for output marked by `update`
([snakemake#3839](snakemake#3839))
([09c64b7](snakemake@09c64b7))
* Minor fixes/additions to logging module.
([snakemake#3802](snakemake#3802))
([3b3986d](snakemake@3b3986d))
* mount local storage prefix into containers
([snakemake#3840](snakemake#3840))
([f1e8b62](snakemake@f1e8b62))
* properly format input/output files in case of missing rule to produce
them ([snakemake#3849](snakemake#3849))
([69d5d24](snakemake@69d5d24))
* Unpack AnnotatedString in _apply_wildcards
([snakemake#3798](snakemake#3798))
([7886508](snakemake@7886508))


### Performance Improvements

* retrieve storage inputs immediately before scheduling jobs instead of
before running the entire workflow
([snakemake#3850](snakemake#3850))
([4ac6cda](snakemake@4ac6cda))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants