Skip to content

fix: Confusion with Overriding input After Snakemake Modularization#3714

Merged
johanneskoester merged 21 commits intomainfrom
fix-3713
Sep 19, 2025
Merged

fix: Confusion with Overriding input After Snakemake Modularization#3714
johanneskoester merged 21 commits intomainfrom
fix-3713

Conversation

@Hocnonsense
Copy link
Copy Markdown
Contributor

@Hocnonsense Hocnonsense commented Aug 28, 2025 •
edited by coderabbitai bot
Loading

will fix #3713

1: Support recursive module modification via WorkflowModifier

To make workflows recursively apply modifiers like path prefix and rule name changes across nested modules, I introduced the following changes:

  • src/snakemake/ruleinfo.py:
    • Made RuleInfo recursively apply parent_modifier to adjust paths and rule names.
  • src/snakemake/path_modifier.py:
    • Added inner_modifier to support chained path transformations in nested modules.
  • src/snakemake/modules.py:
    • Modified get_name_modifier_func and WorkflowModifier.modify_rulename to support recursive rule renaming.
    • Replaced WorkflowModifier.skip_rule with avail_rulename to ensure that rule_whitelist and rule_exclude_list are respected at every module level.

2: Make multiple use statements with specific_rule more reliable and strict

My understanding is that rule.name should be unique in a workflow. Therefore, a second use rule from ... with ... should only be allowed if the rule was previously imported with the same name via a wildcard (use * from ...) and is now being refined.

  • to clarify:

    • A given rule from a module can be used multiple times in the same workflow, as long as each usage assigns it a unique name.
    • However, using the same rule name more than once is not allowed, to prevent accidental overwrites, just like any rule rule defined out of module cannot be overwrite.
    • That’s the restriction behind the “only once” statement -- it applies per final rule name, not per source rule.

  • src/snakemake/rules.py:

    • The old logic stored rule dependencies as objects. If a rule was modified afterward, those changes wouldn't propagate.
    • Now, rule dependencies are resolved dynamically by name, ensuring correct updated rule with parameters modified.

  • src/snakemake/modules.py:

    • Stricter checks for rules overwriting. ModuleInfo.use_rules now allow_rule_overwrite for specific_rule only if it was previously imported via use * from ... and not already customized.

  • src/snakemake/workflow.py:

    • Adjusted to work with the new WorkflowModifier.

Other changes:

  • Simplify WorkflowModifier.__init__. The module’s __name__ is now assigned as WorkflowModifier.namespace, making workflow development and debugging easier.

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

    • Chained path modifiers, public module registry, richer modifier lifecycle, and refined rule-import APIs for modular workflows.

  • Bug Fixes

    • Detects and prevents unintended duplicate rule creation from wildcard/name modifications; stricter path-replacement guards; more reliable dependency resolution for rule inputs.

  • Tests

    • Updated tests and fixtures for renamed, aliased, nested modules and literal-input scenarios.

  • Documentation

    • Expanded guidance on module imports, per-rule customization, aliasing, and conflict-resolution.

@Hocnonsense
fix: Confusion with Overriding input After Snakemake Modularization
08e28ba 
1. use name to map dependence
2. restrict cases when rule can be overwritten
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Aug 28, 2025 •
edited
Loading

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Central refactor of module/modifier system: introduces WorkflowModifier.modules and context management, centralizes path logic with chainable PathModifier, changes name-modifier and overwrite handling, makes Rule.expand_input return job_depends, and adjusts Workflow rule creation/overwrite and modifier application ordering.

Changes

Cohort / File(s) Summary
Module & Modifier Core
src/snakemake/modules.py		 Reworked WorkflowModifier API (now uses is_module and a PathModifier), added public modules: dict, context-manager support, name/path helpers (avail_rulename, modify_rulename, _modify_rulename, modify_path, modify_wrapper_uri), simplified get_name_modifier_func, ModuleInfo now builds/propagates path_modifier, and added wildcard-overwrite detection (check_overwrite).
Path modifier chaining
src/snakemake/path_modifier.py		 PathModifier supports inner_modifier chaining and nullable prefix args; split replacement into _replace_prefix and chained replace_prefix; added guards (parent-dir, absolute, storage/callable) and delegation to inner modifier.
Rule internals & dependencies
src/snakemake/rules.py		 Rule.__init__ typed and gains module_globals; _set_inoutput_item stores IOFile rule dependencies as rule-name strings; expand_input now returns (input, mapping, job_depends, incomplete) and builds job_depends by resolving rule names via workflow.get_rule.
Workflow rule management
src/snakemake/workflow.py		 _rules typed; add_rule returns Rule and enforces overwrite via allow_overwrite (raises CreateRuleException); decoration uses avail_rulename and modifier flow; added modules property exposing modifier.modules; userule/include wired to new modifier APIs.
RuleInfo modifier application
src/snakemake/ruleinfo.py		 apply_modifier defers path_modifier init until after ruleinfo_overwrite and applies parent modifier recursively (chained modifier application).
Tests — module scenarios
tests/.../*.smk, tests/test_modules_all/Snakefile, tests/tests.py		 Added/updated Snakefiles and tests exercising module chaining, wildcard exports, literal input overrides, nested-module aliasing, and overwrite scenarios.
Documentation
docs/snakefiles/*.rst		 Expanded modularization docs clarifying per-rule with: overrides, aliasing/rename constraints, overwrite/collision rules, examples for multiple modified imports, and minor editorial fixes.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant User as Snakefile/User
    participant MI as ModuleInfo
    participant WM as WorkflowModifier
    participant PM as PathModifier
    participant WF as Workflow
    participant R as Rule

    User->>MI: use_rules(rules, name_modifier, ...)
    MI->>WM: construct WorkflowModifier(is_module?, path_modifier, rule_whitelist)
    WM->>PM: attach/chain PathModifier(s)
    MI->>WM: enter context (WM.__enter__) to apply module scope
    WM->>WF: request avail_rulename(orig_name)
    WF->>WM: avail_rulename / modify_rulename
    alt name allowed
        WF->>WF: add_rule(name, allow_overwrite?)
        WF->>R: instantiate Rule (module_globals set)
        R->>R: expand_input(...) → (input,mapping,job_depends,incomplete)
        R->>PM: modify_path(...) (chained)
        WF->>WF: resolve job_depends via workflow.get_rule(...)
    else name blocked / duplicate
        WM->>WF: raise CreateRuleException / skip
    end
    MI->>WM: exit context (WM.__exit__) to restore parent scope
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Pre-merge checks (3 passed, 2 warnings)

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Out of Scope Changes Check ⚠️ Warning The PR introduces several public API and behavioral changes that appear outside the narrow linked-issue objective and could impact external callers: notably Workflow.add_rule now returns a Rule object instead of a rule name (workflow.py), rules.expand_input changed its return tuple shape (rules.py), Rule.init gained/changed typed parameters and module_globals (rules.py), and public surface changes such as exposing Workflow.modules and altering WorkflowModifier constructor arguments. While these may be required for the implementation, they are breaking/visible changes that should be segregated, documented, or justified separately. Other changes (path_modifier chaining, RuleInfo recursion, ModuleInfo overwrite checks, and accompanying tests/docs) are within scope. Isolate or clearly justify the API-breaking changes: either split them into a separate, explicitly labeled API-change PR or add compatibility layers and thorough release notes; update unit tests and docs to demonstrate the new return semantics and run a compatibility sweep for external tooling that may depend on the previous signatures. Also add explicit tests and/or runtime guards for recursive modifier chains to avoid potential infinite recursion.
Docstring Coverage ⚠️ Warning Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title accurately names the bug being addressed (overriding inputs after modularization) and succinctly conveys the core intent to fix override propagation across modules; it directly relates to the module/rule/path modifications described in the changeset. The title is specific, not generic, and uses an appropriate "fix:" prefix to signal intent.
Linked Issues Check ✅ Passed The code changes implement the linked-issue objectives: modifiers are applied recursively (RuleInfo.apply_modifier and WorkflowModifier/path_modifier changes), path transformations are chainable via inner_modifier, dependencies are resolved dynamically by name (rules.expand_input) so later overrides propagate, and ModuleInfo/use_rules enforces the stricter overwrite rules described in #3713; tests and documentation were updated accordingly. These modifications directly address the reproducer and the reported loss of outer-module overrides when chaining imports. Reviewers should still verify modifier-cycle safety and that the new behavior is covered by the added tests.
Description Check ✅ Passed The pull request description follows the repository template, includes a clear problem statement linking to #3713, a detailed summary of per-file changes and motivations, and the QC checklist with both boxes ticked. It documents the intended behavioral changes (recursive modifiers, dynamic dependency resolution, and overwrite restrictions) and notes included tests and docs updates. Overall the description provides sufficient context for reviewers to understand objectives and scope.
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0b460b5 and 43f7bf6.

📒 Files selected for processing (1)
  • docs/snakefiles/modularization.rst (5 hunks)

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.

coderabbitai[bot]
coderabbitai bot reviewed Aug 28, 2025
View reviewed changes
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: 4

🧹 Nitpick comments (1)
tests/test_modules_all/module-test/Snakefile (1)

9-9: Intentional broken input is fine; add a clarifying comment.

This “not exist” sentinel is clearly here to ensure the override path is exercised. Add a short comment to avoid future confusion during test maintenance.

 rule a:
-    input: "not exist"
+    # Intentionally invalid; will be overridden by 'use rule a ... with:'
+    input: "not exist"
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 26fcd38 and 08e28ba.

📒 Files selected for processing (5)
  • src/snakemake/modules.py (1 hunks)
  • src/snakemake/rules.py (5 hunks)
  • src/snakemake/workflow.py (5 hunks)
  • tests/test_modules_all/Snakefile (1 hunks)
  • tests/test_modules_all/module-test/Snakefile (1 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/modules.py
  • src/snakemake/workflow.py
  • src/snakemake/rules.py
🧠 Learnings (1)
📚 Learning: 2024-10-06T14:09:54.370Z 
Learnt from: johanneskoester
PR: snakemake/snakemake#3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in `tests/test_wrapper/Snakefile`, are for testing purposes and do not require updates to the project documentation.

Applied to files:

  • tests/test_modules_all/Snakefile
  • tests/test_modules_all/module-test/Snakefile
🧬 Code graph analysis (3)
src/snakemake/modules.py (1)
src/snakemake/workflow.py (1)
  • globals (551-552)
src/snakemake/workflow.py (3)
src/snakemake/rules.py (5)
  • Rule (77-1340)
  • name (131-132)
  • name (135-136)
  • lineno (139-140)
  • snakefile (143-144)
src/snakemake/parser.py (4)
  • Rule (745-854)
  • name (785-798)
  • name (962-975)
  • lineno (74-75)
src/snakemake/exceptions.py (1)
  • CreateRuleException (526-527)
src/snakemake/rules.py (1)
src/snakemake/workflow.py (6)
  • name (2305-2310)
  • snakefile (1564-1569)
  • rule (1762-2044)
  • get_rule (675-686)
  • input (2053-2058)
  • check (619-627)
🪛 Ruff (0.12.2)
src/snakemake/rules.py

15-15: typing.Union imported but unused

Remove unused import: typing.Union

(F401)

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

165-183: Do not inherit parent namespace into child modifiers — LGTM.

Dropping namespace inheritance prevents cross-namespace overwrites; aligns with the PR’s goal to constrain overwrites to the originating namespace.

tests/test_modules_all/Snakefile (1)

18-19: Good targeted override to reproduce #3713.

This narrow “use rule a … with: input” correctly asserts the fix without broadening test scope.

src/snakemake/rules.py (4)

78-79: Constructor type annotation — LGTM.

Clearer signature; no behavior change.

128-129: Explicit typing for module_globals — LGTM.

Simple clarity improvement; consistent with usage.

464-467: Confirm rename safety: set_input() runs after rule renaming
The decorate function applies ruleinfo.name to rule.name before invoking rule.set_input(), so the stored item.rule.name is always the final name (snakemake.readthedocs.io).

898-916: expand_input return-shape change: audit call sites.

Only one call site found in src/snakemake/jobs.py (lines 251–256), where the unpacking already matches the new 4-tuple signature:

(
  self.input,
  input_mapping,
  self.dependencies,
  self.incomplete_input_expand,
) = self.rule.expand_input(...)

No other consumers of expand_input exist. Confirm that all downstream code uses self.dependencies as the new job_depends (mapping from file to Rule) rather than the old raw dependencies mapping.

src/snakemake/workflow.py (2)

177-177: Typed rules registry — LGTM.

Improves readability without behavior changes.

1804-1807: Rename mapping simplification — LGTM.

Directly updating the mapping is cleaner and preserves order.

@Hocnonsense
Copy link
Copy Markdown
Contributor Author

Hocnonsense commented Aug 28, 2025

Now I notice that there is another bug:
A.smk:

rule run:
    shell: "cp {input.rds} {output.txt}"

B.smk:

module A:
    snakefile: "A.smk"

use rule run from A as A_run with:
    input: rds=config.get("b", "B.smk"),
    log: "aaalog",

C.smk:

rule all:
    input: "output_run.txt",

module B:
    snakefile: "B.smk"
    config: {"b": "invalid-file"}

use rule A_run from B as B_A_run with:
    input: rds="README.md",

run:

/demo/workflows$ snakemake -s C.smk output_run.txt -n -p
host: Matebook14
Building DAG of jobs...
MissingInputException in rule B_A_run in file "/demo/workflows/A.smk", line 1:
Missing input files for rule B_A_run:
    output: output_run.txt
    affected files:
        invalid file
/demo/workflows$ snakemake -s B.smk output_run.txt-n -p
host: Matebook14
Building DAG of jobs...
Job stats:
job      count
-----  -------
A_run        1
total        1


[Thu Aug 28 21:32:43 2025]
rule A_run:
    input: B.smk
    output: output_run.txt
    log: aaalog
    jobid: 0
    reason: Missing output files: output_run.txt
    resources: tmpdir=<TBD>
Shell command: None
Job stats:
job      count
-----  -------
A_run        1
total        1

Reasons:
    (check individual jobs above for details)
    output files have to be generated:
        A_run
This was a dry-run (flag -n). The order of jobs does not reflect the order of execution.

It seems that the overwrite ruleinfo in C.smk (outer one) is lost...

coderabbitai[bot]
coderabbitai bot reviewed Aug 28, 2025
View reviewed changes
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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/snakemake/workflow.py (1)

1782-1794: Do not mutate rule.module_globals["__name__"] on overwrite.

Setting it to None alters the shared module globals, breaking subsequent same-namespace overwrites and causing nested-import overrides to be lost (matches the issue this PR targets).

         rule.is_checkpoint = checkpoint
         rule.module_globals = self.modifier.globals
-        if is_overwrite:
-            rule.module_globals["__name__"] = None
♻️ Duplicate comments (1)
src/snakemake/modules.py (1)

162-176: Fresh modifier state for top-level context is correct.

Initializing new globals/rules/rule_proxies/modules and setting __name__ to the namespace matches the intended design.

🧹 Nitpick comments (3)
src/snakemake/rules.py (1)

34-49: Deduplicate import: flag is imported twice.

Remove the second flag from the import list to avoid F401/F811 noise.

 from snakemake.io import (
@@
-    flag,
     get_flag_value,
@@
-    flag,
     is_callable,
     ReportObject,
 )
src/snakemake/workflow.py (1)

648-667: Improve overwrite error to include origin namespace.

Helpful for diagnosing why an overwrite is blocked.

-            if not is_overwrite:
-                raise CreateRuleException(
-                    f"The name {name} is already used by another rule",
-                    lineno=lineno,
-                    snakefile=snakefile,
-                )
+            if not is_overwrite:
+                origin_ns = self.get_rule(name).module_globals["__name__"]
+                raise CreateRuleException(
+                    f"The name {name} is already used by a rule from namespace {origin_ns!r}. "
+                    f"Only same-namespace overwrites are allowed here.",
+                    lineno=lineno,
+                    snakefile=snakefile,
+                )
src/snakemake/modules.py (1)

190-200: Type-ignore on PathModifier: optional cleanup.

If feasible, align PathModifier signature or local types to drop # type: ignore[reportArgumentType]. Non-blocking.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 08e28ba and d108819.

📒 Files selected for processing (3)
  • src/snakemake/modules.py (4 hunks)
  • src/snakemake/rules.py (4 hunks)
  • src/snakemake/workflow.py (6 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/rules.py
  • src/snakemake/workflow.py
  • src/snakemake/modules.py
🧠 Learnings (2)
📚 Learning: 2025-08-28T13:17:37.677Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/workflow.py:637-665
Timestamp: 2025-08-28T13:17:37.677Z
Learning: In Snakemake, rule.module_globals["__name__"] always exists and defaults to "snakemake.workflow" when not modified, so direct dictionary access is safe without needing .get().

Applied to files:

  • src/snakemake/rules.py
  • src/snakemake/workflow.py
  • src/snakemake/modules.py
📚 Learning: 2025-08-28T13:15:55.211Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/modules.py:195-195
Timestamp: 2025-08-28T13:15:55.211Z
Learning: In src/snakemake/modules.py, the line `self.globals["__name__"] = namespace` is intentionally designed to set __name__ to None when namespace is None (i.e., when not inside a module context, for top-level workflows). This is correct behavior and should not be guarded.

Applied to files:

  • src/snakemake/workflow.py
  • src/snakemake/modules.py
🧬 Code graph analysis (3)
src/snakemake/rules.py (1)
src/snakemake/workflow.py (6)
  • name (2308-2313)
  • snakefile (1567-1572)
  • rule (1765-2047)
  • get_rule (678-689)
  • input (2056-2061)
  • check (622-630)
src/snakemake/workflow.py (2)
src/snakemake/rules.py (5)
  • Rule (76-1339)
  • name (130-131)
  • name (134-135)
  • lineno (138-139)
  • snakefile (142-143)
src/snakemake/exceptions.py (1)
  • CreateRuleException (526-527)
src/snakemake/modules.py (3)
src/snakemake/workflow.py (7)
  • modifier (542-543)
  • globals (554-555)
  • wildcard_constraints (550-551)
  • rules (589-590)
  • rules (769-770)
  • modules (546-547)
  • config (1756-1757)
src/snakemake/common/__init__.py (1)
  • Rules (253-271)
src/snakemake/path_modifier.py (2)
  • PathModifier (22-145)
  • replace_prefix (69-99)
⏰ 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). (56)
  • GitHub Check: tests (10, ubuntu-latest, py311)
  • GitHub Check: tests (10, ubuntu-latest, py313)
  • GitHub Check: tests (10, ubuntu-latest, py312)
  • GitHub Check: tests (9, windows-latest, py313)
  • GitHub Check: tests (10, windows-latest, py313)
  • GitHub Check: tests (10, windows-latest, py312)
  • GitHub Check: tests (9, ubuntu-latest, py313)
  • GitHub Check: tests (8, macos-latest, py312)
  • GitHub Check: tests (9, windows-latest, py312)
  • GitHub Check: tests (9, ubuntu-latest, py311)
  • GitHub Check: tests (9, macos-latest, py313)
  • GitHub Check: tests (9, ubuntu-latest, py312)
  • GitHub Check: tests (8, windows-latest, py313)
  • GitHub Check: tests (8, ubuntu-latest, py312)
  • GitHub Check: tests (8, windows-latest, py312)
  • GitHub Check: tests (4, ubuntu-latest, py312)
  • GitHub Check: tests (8, ubuntu-latest, py313)
  • GitHub Check: tests (8, ubuntu-latest, py311)
  • GitHub Check: tests (5, ubuntu-latest, py312)
  • GitHub Check: tests (7, ubuntu-latest, py313)
  • GitHub Check: tests (6, windows-latest, py312)
  • GitHub Check: tests (6, windows-latest, py313)
  • GitHub Check: tests (7, windows-latest, py313)
  • GitHub Check: tests (7, windows-latest, py312)
  • GitHub Check: tests (6, ubuntu-latest, py312)
  • GitHub Check: tests (7, ubuntu-latest, py312)
  • GitHub Check: tests (6, ubuntu-latest, py311)
  • GitHub Check: tests (7, ubuntu-latest, py311)
  • GitHub Check: tests (5, windows-latest, py312)
  • GitHub Check: tests (5, windows-latest, py313)
  • GitHub Check: tests (6, ubuntu-latest, py313)
  • GitHub Check: tests (4, windows-latest, py312)
  • GitHub Check: tests (4, windows-latest, py313)
  • GitHub Check: tests (5, ubuntu-latest, py311)
  • GitHub Check: tests (5, ubuntu-latest, py313)
  • GitHub Check: tests (4, ubuntu-latest, py311)
  • GitHub Check: tests (4, ubuntu-latest, py313)
  • GitHub Check: tests (3, windows-latest, py313)
  • GitHub Check: tests (3, ubuntu-latest, py313)
  • GitHub Check: tests (3, windows-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 (1, ubuntu-latest, py313)
  • GitHub Check: tests (2, windows-latest, py312)
  • GitHub Check: tests (2, windows-latest, py313)
  • GitHub Check: tests (2, ubuntu-latest, py313)
  • GitHub Check: tests (1, windows-latest, py313)
  • GitHub Check: tests (2, macos-latest, py313)
  • GitHub Check: tests (1, windows-latest, py312)
  • GitHub Check: tests (1, macos-latest, py313)
  • GitHub Check: tests (1, ubuntu-latest, py312)
  • GitHub Check: tests (2, ubuntu-latest, py312)
  • GitHub Check: tests (1, macos-latest, py312)
  • GitHub Check: tests (1, ubuntu-latest, py311)
  • GitHub Check: apidocs
🔇 Additional comments (7)
src/snakemake/rules.py (2)

463-466: Good: store dependency by rule name, not object.

Switching to item.rule.name avoids stale object refs across modules and eases (re)resolution via workflow.get_rule().

127-128: Initialize or assert module_globals before any access.

Attribute is only annotated here; it’s assigned later in workflow.rule(). If any code path accesses it earlier, it will raise. If guaranteed safe, add a brief comment; otherwise set a default.

-        self.module_globals: dict
+        # Set by Workflow.rule() immediately after add_rule(); intentionally uninitialized here.
+        self.module_globals: dict
src/snakemake/workflow.py (3)

177-177: Typed rules mapping: good.

OrderedDict[str, Rule] improves clarity for readers and type checkers.

545-548: Expose modules via property: good.

Makes module registry consistently reflect the current modifier context.

1765-1810: Name reassignment logic: safe; deletion and reinsertion are adjacent with no intervening lookups. No change requested.

src/snakemake/modules.py (2)

55-55: Capture parent modifier explicitly: good.

This makes intent clear and simplifies rule proxy inheritance.

82-106: Module use isolation + proxy inheritance: good.

Creating a fresh modifier for the module and then inheriting proxies into the parent preserves isolation while exposing rule proxies as expected.

coderabbitai[bot]
coderabbitai bot reviewed Aug 29, 2025
View reviewed changes
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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/snakemake/path_modifier.py (1)

77-84: Robustness: path.startswith("..") may break for non-string paths.

path can be a pathlib.Path or AnnotatedString. Calling startswith directly can raise for Path. Convert to str to be safe.

Apply:

-            or path.startswith("..")
+            or str(path).startswith("..")
🧹 Nitpick comments (1)
src/snakemake/path_modifier.py (1)

18-25: PathModifier chaining: constructor looks good, but consider guarding against accidental cycles.

Since chaining now allows arbitrary composition, a defensive check (e.g., detect self in the inner chain) would prevent rare infinite-recursion bugs if a modifier is mistakenly reused.

Apply this minimal guard:

     def __init__(
         self,
         replace_prefix: dict | None,
         prefix: str | None,
         workflow,
         inner_modifier: "PathModifier | None" = None,
     ):
         self.skip_properties: set = set()
         self.workflow = workflow
-        self.inner_modifier = inner_modifier
+        # Guard: prevent accidental self-references in chains
+        self.inner_modifier = inner_modifier if inner_modifier is not self else None

Also applies to: 27-27

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between c3ffcf7 and 0d8ec54.

📒 Files selected for processing (3)
  • src/snakemake/modules.py (6 hunks)
  • src/snakemake/path_modifier.py (2 hunks)
  • src/snakemake/ruleinfo.py (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/snakemake/ruleinfo.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/path_modifier.py
  • src/snakemake/modules.py
🧠 Learnings (2)
📚 Learning: 2025-08-28T13:15:55.232Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/modules.py:195-195
Timestamp: 2025-08-28T13:15:55.232Z
Learning: In src/snakemake/modules.py, the line `self.globals["__name__"] = namespace` is intentionally designed to set __name__ to None when namespace is None (i.e., when not inside a module context, for top-level workflows). This is correct behavior and should not be guarded.

Applied to files:

  • src/snakemake/modules.py
📚 Learning: 2025-08-28T13:17:37.693Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/workflow.py:637-665
Timestamp: 2025-08-28T13:17:37.693Z
Learning: In Snakemake, rule.module_globals["__name__"] always exists and defaults to "snakemake.workflow" when not modified, so direct dictionary access is safe without needing .get().

Applied to files:

  • src/snakemake/modules.py
🧬 Code graph analysis (2)
src/snakemake/path_modifier.py (1)
src/snakemake/io/__init__.py (7)
  • is_callable (310-311)
  • is_callable (940-941)
  • is_callable (1147-1152)
  • is_flagged (955-958)
  • AnnotatedString (929-949)
  • flag (961-971)
  • get_flag_value (1155-1160)
src/snakemake/modules.py (3)
src/snakemake/workflow.py (7)
  • modifier (542-543)
  • globals (554-555)
  • wildcard_constraints (550-551)
  • rules (589-590)
  • rules (769-770)
  • config (1756-1757)
  • modules (546-547)
src/snakemake/path_modifier.py (2)
  • PathModifier (16-153)
  • replace_prefix (71-75)
src/snakemake/common/__init__.py (1)
  • Rules (253-271)
⏰ 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). (55)
  • GitHub Check: tests (10, windows-latest, py312)
  • GitHub Check: tests (8, windows-latest, py313)
  • GitHub Check: tests (10, windows-latest, py313)
  • GitHub Check: tests (10, ubuntu-latest, py312)
  • GitHub Check: tests (10, ubuntu-latest, py313)
  • GitHub Check: tests (9, windows-latest, py313)
  • GitHub Check: tests (9, 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, windows-latest, py312)
  • GitHub Check: tests (8, ubuntu-latest, py313)
  • GitHub Check: tests (8, ubuntu-latest, py311)
  • GitHub Check: tests (8, ubuntu-latest, py312)
  • GitHub Check: tests (8, windows-latest, py312)
  • GitHub Check: tests (7, windows-latest, py312)
  • GitHub Check: tests (7, windows-latest, py313)
  • GitHub Check: tests (7, ubuntu-latest, py311)
  • GitHub Check: tests (7, ubuntu-latest, py312)
  • GitHub Check: tests (7, ubuntu-latest, py313)
  • GitHub Check: tests (6, ubuntu-latest, py313)
  • GitHub Check: tests (5, windows-latest, py313)
  • GitHub Check: tests (5, macos-latest, py312)
  • GitHub Check: tests (6, windows-latest, py313)
  • GitHub Check: tests (6, windows-latest, py312)
  • GitHub Check: tests (5, ubuntu-latest, py313)
  • GitHub Check: tests (6, ubuntu-latest, py311)
  • GitHub Check: tests (5, windows-latest, py312)
  • GitHub Check: tests (5, ubuntu-latest, py312)
  • GitHub Check: tests (6, ubuntu-latest, py312)
  • GitHub Check: tests (4, ubuntu-latest, py312)
  • GitHub Check: tests (5, ubuntu-latest, py311)
  • GitHub Check: tests (3, ubuntu-latest, py313)
  • GitHub Check: tests (4, windows-latest, py313)
  • GitHub Check: tests (4, windows-latest, py312)
  • GitHub Check: tests (4, ubuntu-latest, py313)
  • GitHub Check: tests (3, windows-latest, py312)
  • GitHub Check: tests (3, windows-latest, py313)
  • GitHub Check: tests (4, ubuntu-latest, py311)
  • GitHub Check: tests (2, windows-latest, py312)
  • GitHub Check: tests (2, windows-latest, py313)
  • GitHub Check: tests (3, ubuntu-latest, py311)
  • GitHub Check: tests (3, ubuntu-latest, py312)
  • GitHub Check: tests (2, macos-latest, py313)
  • GitHub Check: tests (2, macos-latest, py312)
  • GitHub Check: tests (1, macos-latest, py313)
  • GitHub Check: tests (2, ubuntu-latest, py313)
  • GitHub Check: tests (2, ubuntu-latest, py312)
  • GitHub Check: tests (2, ubuntu-latest, py311)
  • GitHub Check: tests (1, macos-latest, py312)
  • GitHub Check: tests (1, windows-latest, py312)
  • GitHub Check: tests (1, ubuntu-latest, py311)
  • GitHub Check: tests (1, ubuntu-latest, py312)
  • GitHub Check: tests (1, ubuntu-latest, py313)
  • GitHub Check: tests (1, windows-latest, py313)
🔇 Additional comments (7)
src/snakemake/path_modifier.py (1)

71-76: Chaining order is child-first then parent; confirm this matches expected override semantics.

replace_prefix() applies the current modifier first, then delegates to the parent via inner_modifier. This makes child mappings run before inherited mappings. That seems right (parent won’t re-map absolute paths), but please confirm with nested module overrides from #3713 that top-level overrides always prevail.

Would you run the new tests with a case where the child maps to an absolute path and the parent has a non-absolute replace_prefix, ensuring the parent does not re-prefix the child result?

src/snakemake/modules.py (6)

55-56: Good: path modification now composes via a chained PathModifier.

Passing the parent modifier’s path_modifier into the module-level PathModifier enables deterministic, ordered composition and should address path handling in nested imports.

Also applies to: 70-72

143-159: Constructor surface changed substantially; ensure all call sites are updated.

WorkflowModifier.__init__ dropped several params and added parent_modifier, path_modifier, and rule_proxies. Please verify there are no stale call sites (plugins, wrappers, or downstream libs) still using the old signature.

Run a repo-wide search for WorkflowModifier( and check for positional-arg construction that could be misaligned after this change.

160-181: Two init paths: clarify and test both branches.

  • Branch A (no parent, globals is None) pulls from workflow.vanilla_globals and inherits parent_modifier = workflow.modifier.
  • Branch B (no parent, globals provided) creates fresh Rules() and a root PathModifier.

Edge cases to verify:

  • That rule_proxies and path_modifier are always non-None (covered when passed by ModuleInfo.use_rules, but also validate direct construction paths).
  • That self.globals["rules"] always points to the intended Rules namespace for the include being processed.

Please add/confirm tests that exercise:

  • use rule within a module (globals is None path),
  • top-level module modifier (globals provided path).

182-190: Potential parent chain skip in nested “use rule … with” flow.

Setting self.parent_modifier = parent_modifier.parent_modifier intentionally skips one level. This changes ancestry for further resolutions (e.g., name/path modifiers). If that’s required to avoid double-application, great—otherwise it can cause surprising behavior for deeply nested use/with chains.

Can you confirm with a 3-level nested import/use/with scenario that:

  • name mapping and ruleinfo overrides are taken from the expected scope, and
  • path modification chain still includes all intended ancestors exactly once?

196-196: Behavioral change: skip_configfile now tied to config is not None.

Previously this may have been controlled differently. Confirm that providing a config dict to WorkflowModifier disables configfile loading only where intended, and does not inadvertently suppress configfile usage for unrelated includes.

Consider a test where a module receives config while sibling includes rely on configfile, ensuring only the module’s include is affected.

226-228: LGTM: path modification now flows through the chained PathModifier.

This centralizes path handling and should resolve the override propagation issues described in #3713.

coderabbitai[bot]
coderabbitai bot reviewed Aug 29, 2025
View reviewed changes
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: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/snakemake/workflow.py (1)

633-658: Overwrite semantics are broken: no namespace check and is_overwrite is never set.

Current logic:

  • Allows overwrites when allow_overwrite=True without checking origin namespace.
  • Always returns is_overwrite=False (even on overwrite), so callers can’t react.
  • Increments rule_count on overwrite.

This undermines the PR goal (“restrict when a rule can be overwritten”). Fix by: (a) gating overwrite to same namespace, (b) setting is_overwrite=True when replacing, and (c) not bumping counters or default target on overwrite.

 def add_rule(
     self,
-        name: str,
+        name: str,
     ...
-):
+):
     """
     Add a rule.
     """
-    if self.is_rule(name) and not allow_overwrite:
-        raise CreateRuleException(
-            f"The name {name} is already used by another rule",
-            lineno=lineno,
-            snakefile=snakefile,
-        )
-    else:
-        is_overwrite = False
-        self.rule_count += 1
-        if not self.default_target:
-            self.default_target = name
+    if self.is_rule(name):
+        if not allow_overwrite:
+            raise CreateRuleException(
+                f"The name {name} is already used by another rule",
+                lineno=lineno,
+                snakefile=snakefile,
+            )
+        # Restrict overwrite to same namespace
+        origin_ns = self.get_rule(name).module_globals["__name__"]
+        if origin_ns != self.modifier.namespace:
+            raise CreateRuleException(
+                f"Cannot overwrite rule {name} from a different module "
+                f"(existing={origin_ns!r}, current={self.modifier.namespace!r})",
+                lineno=lineno,
+                snakefile=snakefile,
+            )
+        is_overwrite = True
+    else:
+        is_overwrite = False
+        self.rule_count += 1
+        if not self.default_target:
+            self.default_target = name
     rule = Rule(name, self, lineno=lineno, snakefile=snakefile)
     self._rules[rule.name] = rule
     self.modifier.rules.add(rule)
     return is_overwrite
♻️ Duplicate comments (2)
src/snakemake/workflow.py (1)

1777-1779: Do not mutate module_globals["__name__"] on overwrite.

This was previously flagged; it can corrupt module identity and block subsequent same-namespace overwrites. Remove the mutation and rely on the namespace gate in add_rule().

-        if is_overwrite:
-            rule.module_globals["__name__"] = None
+        # no mutation of module globals on overwrite
src/snakemake/modules.py (1)

193-193: LGTM: set name from namespace (intentional).
Setting __name__ directly—allowing None outside a module—is by design per prior discussion.

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

2388-2391: Guard avail_rulename call for None alias.

name_modifier can be None in local inheritance; calling avail_rulename(None) may be unintended. Consider short-circuiting when name_modifier is None.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 0d8ec54 and f24c9f3.

📒 Files selected for processing (6)
  • src/snakemake/modules.py (5 hunks)
  • src/snakemake/workflow.py (10 hunks)
  • tests/test_module_nested/Snakefile (1 hunks)
  • tests/test_module_nested/module_deep.smk (1 hunks)
  • tests/test_module_nested/module_shallow.smk (2 hunks)
  • tests/tests.py (1 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:

  • tests/tests.py
  • src/snakemake/modules.py
  • src/snakemake/workflow.py
🧠 Learnings (3)
📚 Learning: 2024-10-06T14:09:54.370Z 
Learnt from: johanneskoester
PR: snakemake/snakemake#3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in `tests/test_wrapper/Snakefile`, are for testing purposes and do not require updates to the project documentation.

Applied to files:

  • tests/tests.py
  • tests/test_module_nested/Snakefile
📚 Learning: 2025-08-28T13:15:55.232Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/modules.py:195-195
Timestamp: 2025-08-28T13:15:55.232Z
Learning: In src/snakemake/modules.py, the line `self.globals["__name__"] = namespace` is intentionally designed to set __name__ to None when namespace is None (i.e., when not inside a module context, for top-level workflows). This is correct behavior and should not be guarded.

Applied to files:

  • src/snakemake/modules.py
  • src/snakemake/workflow.py
📚 Learning: 2025-08-28T13:17:37.693Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/workflow.py:637-665
Timestamp: 2025-08-28T13:17:37.693Z
Learning: In Snakemake, rule.module_globals["__name__"] always exists and defaults to "snakemake.workflow" when not modified, so direct dictionary access is safe without needing .get().

Applied to files:

  • src/snakemake/modules.py
  • src/snakemake/workflow.py
🧬 Code graph analysis (3)
tests/tests.py (2)
src/snakemake/workflow.py (3)
  • run (2300-2301)
  • snakefile (1557-1562)
  • config (1746-1747)
tests/common.py (2)
  • run (152-496)
  • dpath (33-36)
src/snakemake/modules.py (4)
src/snakemake/common/__init__.py (2)
  • Rules (253-271)
  • _register_rule (259-260)
src/snakemake/exceptions.py (1)
  • CreateRuleException (526-527)
src/snakemake/path_modifier.py (2)
  • PathModifier (16-153)
  • replace_prefix (71-75)
src/snakemake/workflow.py (6)
  • rules (589-590)
  • rules (759-760)
  • modifier (542-543)
  • snakefile (1557-1562)
  • name (2293-2298)
  • rule (1755-2032)
src/snakemake/workflow.py (3)
src/snakemake/rules.py (6)
  • Rule (76-1339)
  • name (130-131)
  • name (134-135)
  • lineno (138-139)
  • snakefile (142-143)
  • add (1346-1350)
src/snakemake/modules.py (1)
  • avail_rulename (239-249)
src/snakemake/ruleinfo.py (1)
  • RuleInfo (14-111)
⏰ 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). (42)
  • GitHub Check: tests (10, windows-latest, py313)
  • GitHub Check: tests (4, ubuntu-latest, py311)
  • GitHub Check: tests (10, ubuntu-latest, py311)
  • GitHub Check: tests (9, ubuntu-latest, py312)
  • GitHub Check: tests (9, windows-latest, py313)
  • GitHub Check: tests (8, ubuntu-latest, py313)
  • GitHub Check: tests (10, ubuntu-latest, py313)
  • GitHub Check: tests (8, windows-latest, py313)
  • GitHub Check: tests (10, ubuntu-latest, py312)
  • GitHub Check: tests (9, ubuntu-latest, py311)
  • GitHub Check: tests (9, ubuntu-latest, py313)
  • GitHub Check: tests (8, ubuntu-latest, py311)
  • GitHub Check: tests (7, windows-latest, py313)
  • GitHub Check: tests (6, macos-latest, py313)
  • GitHub Check: tests (8, ubuntu-latest, py312)
  • GitHub Check: tests (7, ubuntu-latest, py312)
  • GitHub Check: tests (7, windows-latest, py312)
  • GitHub Check: tests (7, ubuntu-latest, py311)
  • GitHub Check: tests (7, ubuntu-latest, py313)
  • GitHub Check: tests (5, macos-latest, py313)
  • GitHub Check: tests (6, windows-latest, py313)
  • GitHub Check: tests (5, windows-latest, py312)
  • GitHub Check: tests (6, ubuntu-latest, py311)
  • GitHub Check: tests (5, ubuntu-latest, py311)
  • GitHub Check: tests (6, ubuntu-latest, py312)
  • GitHub Check: tests (5, windows-latest, py313)
  • GitHub Check: tests (5, ubuntu-latest, py312)
  • GitHub Check: tests (5, macos-latest, py312)
  • GitHub Check: tests (6, ubuntu-latest, py313)
  • GitHub Check: tests (5, ubuntu-latest, py313)
  • GitHub Check: tests (4, windows-latest, py312)
  • GitHub Check: tests (3, windows-latest, py312)
  • GitHub Check: tests (4, macos-latest, py312)
  • GitHub Check: tests (4, windows-latest, py313)
  • GitHub Check: tests (3, ubuntu-latest, py313)
  • GitHub Check: tests (3, macos-latest, py313)
  • GitHub Check: tests (3, ubuntu-latest, py311)
  • GitHub Check: tests (2, windows-latest, py313)
  • GitHub Check: tests (3, ubuntu-latest, py312)
  • GitHub Check: tests (3, windows-latest, py313)
  • GitHub Check: tests (1, ubuntu-latest, py313)
  • GitHub Check: tests (2, windows-latest, py312)
🔇 Additional comments (24)
tests/tests.py (2)

1580-1588: Confirm targeting a log file is the intended failure mode.

targets={"aaalog"} points to a log, not an output. In dry-run, DAG resolution typically doesn’t consider log: as producible targets, so failure may stem from “no rule to produce” rather than overwrite semantics. Please confirm this is the expected failure reason. If not, target a rule or an actual output.

1589-1596: Ignore the suggestion to target B_A_run here—this test invokes module_shallow.smk, where the imported rule is aliased as A_run, not B_A_run.

Likely an incorrect or invalid review comment.

tests/test_module_nested/Snakefile (2)

1-7: LGTM: module config/replace_prefix setup.

The replace_prefix: {"b-/A/": ""} composition looks intentional for the nested module path adjustments.

10-16: Verify that the input override matches the aliased rule’s interface.

with: input: rds="Snakefile" assumes the alias A_run (defined in the shallow module) exposes rds. This is fine if the prior alias sets that input. Please confirm this chain is guaranteed at parse time.

tests/test_module_nested/module_shallow.smk (1)

11-15: Double alias relies on overwrite semantics; ensure tests assert this behavior.

Defining A_run twice is a good probe for “same-module overwrite only.” Make sure the engine only permits this overwrite within the same namespace, and that tests will fail if cross-namespace overwrites slip through.

Also applies to: 17-21

tests/test_module_nested/module_deep.smk (1)

4-9: LGTM: rule graph for run → run2 and alias run3.

Clear outputs and simple shell. Good assertions.

Also applies to: 11-18, 20-22, 25-27

src/snakemake/workflow.py (3)

177-177: LGTM: typed _rules initialization.

Explicit OrderedDict[str, Rule] improves clarity.

545-548: LGTM: expose modules via modifier.

Property passthrough keeps state centralized in WorkflowModifier.

1757-1766: Fallback name generation may regress uniqueness; consider prior counter.

Using len(self._rules)+1 can reuse names if an overwrite occurs before decoration. Consider reverting to the monotonic self.rule_count for unnamed rules.

-        orig_name = name or str(len(self._rules) + 1)
+        orig_name = name or str(self.rule_count + 1)
src/snakemake/modules.py (15)

9-9: LGTM: typing imports.

12-12: LGTM: exception import added.

18-30: LGTM: simplified name-modifier helper.
Behavior matches prior intent; wildcard vs literal handled cleanly.

50-53: LGTM: parent modifier capture and per-module rule proxy.
Capturing workflow.modifier here is correct for nesting; dedicated Rules() namespace is appropriate.

66-69: LGTM: path modifier chaining.
Composes module path changes over parent modifier.

126-133: LGTM: rule whitelist resolution.
Wildcard constraints validated; returning None for “use rule *” is fine.

166-182: LGTM: constructor signature and typing.
The added types and parameters align with the refactor.

205-215: Double-check parent chain: using parent_modifier.parent_modifier skips one level.
This flattens the chain and can change how name/path modifiers compose across nested use rule ... with:. Validate against the nested import/override reproducer (A <- B <- C) to ensure outer overrides are applied exactly once and not lost.

If issues arise, consider setting self.parent_modifier = parent_modifier and relying on _modify_rulename to avoid double application.

219-219: LGTM: skip_configfile toggled via config presence.

233-238: LGTM: inherit rule proxies with local name modifier.
Ensures parent exposes the child’s rules under the locally requested names.

239-250: LGTM: whitelist/exclude gating and delegated availability.
Recursive name modification before delegating upward is correct.

252-256: LGTM: rulename modification composition.

257-260: LGTM: internal modifier application.

262-264: LGTM: path modification routed via PathModifier.

135-160: Overwrite gating logic correctly restricted to matching wildcard imports
The revised implementation only permits overwrites when a prior wildcard import produced the same modified name and tracks per-rule duplicates.

coderabbitai[bot]
coderabbitai bot reviewed Aug 29, 2025
View reviewed changes
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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/snakemake/workflow.py (2)

632-658: add_rule() never signals overwrites and may mis-handle replacements.

  • is_overwrite is always False, so overwrite-specific flows never trigger.
  • Overwrites don’t differentiate from first-time adds; default_target may be (re)assigned on overwrite.
  • Replacing an existing key without popping first can change order semantics unexpectedly if not intended.

Apply:

 def add_rule(
     self,
     name: str,
     lineno=None,
     snakefile=None,
     checkpoint=False,
     allow_overwrite=False,
 ):
@@
-        if self.is_rule(name) and not allow_overwrite:
+        existed = self.is_rule(name)
+        if existed and not allow_overwrite:
             raise CreateRuleException(
                 f"The name {name} is already used by another rule",
                 lineno=lineno,
                 snakefile=snakefile,
             )
-        else:
-            is_overwrite = False
-            self.rule_count += 1
-            if not self.default_target:
-                self.default_target = name
+        is_overwrite = existed
+        self.rule_count += 1
+        if not self.default_target and not is_overwrite:
+            self.default_target = name
         rule = Rule(name, self, lineno=lineno, snakefile=snakefile)
-        self._rules[rule.name] = rule
+        if is_overwrite:
+            # ensure replacement while keeping new insertion position explicit
+            self._rules.pop(rule.name, None)
+        self._rules[rule.name] = rule
         self.modifier.rules.add(rule)
         return is_overwrite

2388-2399: Wrong selector used for local ‘use rule’ gating.

avail_rulename() expects the original rule name but receives name_modifier (alias), causing unintended skips.

-                if not self.modifier.avail_rulename(name_modifier):
+                if not self.modifier.avail_rulename(rules[0]):
                     # The parent use rule statement is specific for a different particular rule
                     # hence this local use rule statement can be skipped.
                     return
♻️ Duplicate comments (2)
src/snakemake/workflow.py (2)

1777-1779: Do not mutate module_globals["name"] on overwrite.

Overwriting shouldn’t alter the namespace identity stored in globals. Remove this block.

-        if is_overwrite:
-            rule.module_globals["__name__"] = None

1791-1795: Missing collision check when renaming a rule.

Renaming can clobber an existing rule silently. Add the same guard used in add_rule().

-            if ruleinfo.name:
-                del self._rules[name]
-                name = rule.name = ruleinfo.name
-                self._rules[name] = rule
+            if ruleinfo.name:
+                new_name = ruleinfo.name
+                if new_name != name and self.is_rule(new_name):
+                    raise CreateRuleException(
+                        f"The name {new_name} is already used by another rule",
+                        lineno=lineno,
+                        snakefile=snakefile,
+                    )
+                del self._rules[name]
+                name = rule.name = new_name
+                self._rules[name] = rule
🧹 Nitpick comments (1)
src/snakemake/workflow.py (1)

1757-1766: Minor: unnamed rule numbering should use rule_count for stability.

len(self._rules) can fluctuate with overwrites/renames; rule_count is monotonic.

-        orig_name = name or str(len(self._rules) + 1)
+        orig_name = name or str(self.rule_count + 1)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between f24c9f3 and f3331af.

📒 Files selected for processing (2)
  • src/snakemake/modules.py (5 hunks)
  • src/snakemake/workflow.py (11 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/modules.py
  • src/snakemake/workflow.py
🧠 Learnings (3)
📚 Learning: 2025-08-28T13:15:55.232Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/modules.py:195-195
Timestamp: 2025-08-28T13:15:55.232Z
Learning: In src/snakemake/modules.py, the line `self.globals["__name__"] = namespace` is intentionally designed to set __name__ to None when namespace is None (i.e., when not inside a module context, for top-level workflows). This is correct behavior and should not be guarded.

Applied to files:

  • src/snakemake/modules.py
  • src/snakemake/workflow.py
📚 Learning: 2025-08-28T13:17:37.693Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/workflow.py:637-665
Timestamp: 2025-08-28T13:17:37.693Z
Learning: In Snakemake, rule.module_globals["__name__"] always exists and defaults to "snakemake.workflow" when not modified, so direct dictionary access is safe without needing .get().

Applied to files:

  • src/snakemake/modules.py
  • src/snakemake/workflow.py
📚 Learning: 2025-08-29T11:51:44.598Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/modules.py:72-99
Timestamp: 2025-08-29T11:51:44.598Z
Learning: In Snakemake's module system, rule overrides are only permitted when a rule was previously imported via wildcard (`rule_whitelist is None`) and is now being imported again with specific modifications. The check_overwrite method should return True only for specific rule imports that were previously imported via wildcard, not for any non-wildcard imports.

Applied to files:

  • src/snakemake/modules.py
  • src/snakemake/workflow.py
🧬 Code graph analysis (2)
src/snakemake/modules.py (4)
src/snakemake/common/__init__.py (2)
  • Rules (253-271)
  • _register_rule (259-260)
src/snakemake/exceptions.py (1)
  • CreateRuleException (526-527)
src/snakemake/path_modifier.py (2)
  • PathModifier (16-153)
  • replace_prefix (71-75)
src/snakemake/workflow.py (9)
  • rules (589-590)
  • rules (759-760)
  • modifier (542-543)
  • snakefile (1557-1562)
  • name (2293-2298)
  • rule (1755-2032)
  • globals (554-555)
  • wildcard_constraints (550-551)
  • modules (546-547)
src/snakemake/workflow.py (4)
src/snakemake/rules.py (3)
  • Rule (76-1339)
  • name (130-131)
  • name (134-135)
src/snakemake/exceptions.py (1)
  • CreateRuleException (526-527)
src/snakemake/modules.py (2)
  • avail_rulename (237-247)
  • get_name_modifier_func (18-29)
src/snakemake/ruleinfo.py (1)
  • RuleInfo (14-111)
⏰ 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). (56)
  • GitHub Check: tests (9, ubuntu-latest, py312)
  • GitHub Check: tests (9, ubuntu-latest, py313)
  • GitHub Check: tests (10, windows-latest, py313)
  • GitHub Check: tests (10, ubuntu-latest, py311)
  • GitHub Check: tests (10, windows-latest, py312)
  • GitHub Check: tests (10, ubuntu-latest, py313)
  • GitHub Check: tests (9, windows-latest, py313)
  • GitHub Check: tests (10, ubuntu-latest, py312)
  • GitHub Check: tests (8, windows-latest, py312)
  • GitHub Check: tests (8, windows-latest, py313)
  • GitHub Check: tests (9, macos-latest, py312)
  • GitHub Check: tests (9, windows-latest, py312)
  • GitHub Check: tests (5, windows-latest, py312)
  • GitHub Check: tests (9, ubuntu-latest, py311)
  • GitHub Check: tests (8, ubuntu-latest, py311)
  • GitHub Check: tests (6, ubuntu-latest, py313)
  • GitHub Check: tests (7, windows-latest, py313)
  • GitHub Check: tests (7, ubuntu-latest, py313)
  • GitHub Check: tests (4, windows-latest, py312)
  • GitHub Check: tests (8, ubuntu-latest, py313)
  • GitHub Check: tests (6, windows-latest, py312)
  • GitHub Check: tests (6, windows-latest, py313)
  • GitHub Check: tests (7, ubuntu-latest, py311)
  • GitHub Check: tests (5, windows-latest, py313)
  • GitHub Check: tests (7, ubuntu-latest, py312)
  • GitHub Check: tests (8, ubuntu-latest, py312)
  • GitHub Check: tests (7, windows-latest, py312)
  • GitHub Check: tests (6, ubuntu-latest, py311)
  • GitHub Check: tests (6, ubuntu-latest, py312)
  • GitHub Check: tests (5, ubuntu-latest, py311)
  • GitHub Check: tests (2, windows-latest, py313)
  • GitHub Check: tests (5, ubuntu-latest, py312)
  • GitHub Check: tests (5, ubuntu-latest, py313)
  • GitHub Check: tests (4, ubuntu-latest, py313)
  • GitHub Check: tests (3, ubuntu-latest, py312)
  • GitHub Check: tests (3, macos-latest, py313)
  • GitHub Check: tests (4, windows-latest, py313)
  • GitHub Check: tests (4, ubuntu-latest, py311)
  • GitHub Check: tests (3, ubuntu-latest, py313)
  • GitHub Check: tests (3, windows-latest, py312)
  • GitHub Check: tests (4, ubuntu-latest, py312)
  • GitHub Check: tests (3, windows-latest, py313)
  • GitHub Check: tests (1, macos-latest, py313)
  • GitHub Check: tests (1, ubuntu-latest, py312)
  • GitHub Check: tests (2, windows-latest, py312)
  • GitHub Check: tests (2, ubuntu-latest, py312)
  • GitHub Check: tests (2, macos-latest, py312)
  • GitHub Check: tests (3, ubuntu-latest, py311)
  • GitHub Check: tests (2, ubuntu-latest, py311)
  • GitHub Check: tests (2, ubuntu-latest, py313)
  • GitHub Check: tests (1, macos-latest, py312)
  • GitHub Check: tests (1, ubuntu-latest, py311)
  • GitHub Check: tests (1, ubuntu-latest, py313)
  • GitHub Check: tests (1, windows-latest, py313)
  • GitHub Check: tests (1, windows-latest, py312)
  • GitHub Check: apidocs
🔇 Additional comments (9)
src/snakemake/workflow.py (3)

177-177: LGTM: explicit OrderedDict typing for _rules.

Clearer intent and preserves rule insertion order.

545-548: LGTM: expose modules via current modifier.

Public accessor aligns with modifier-stack model.

1600-1604: LGTM: duplicate include suppression only for non-module context.

Prevents blocking legitimate repeated module uses.

src/snakemake/modules.py (6)

18-30: LGTM: simpler name-modifier resolver.

Covers wildcard and single-rule alias cases cleanly.

50-68: LGTM: parent-aware PathModifier chaining.

Ensures consistent prefix replacement across nested modules.

70-100: LGTM: use_rules wiring and explicit allow_rule_overwrite.

Typed params and passing path_modifier/rule_proxies look correct.

183-202: LGTM: module vs. local modifier initialization.

State propagation (globals/rules/modules/path_modifier) is coherent; skip_configfile semantics preserved.

231-236: LGTM: inherit_rule_proxies applies local name modifier.

Keeps proxies consistent across nested modifiers.

237-247: LGTM: avail_rulename respects whitelist/exclude and parent chain.

Correctly short-circuits disallowed rules and propagates to parent.

coderabbitai[bot]
coderabbitai bot reviewed Aug 29, 2025
View reviewed changes
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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/snakemake/modules.py (1)

1-284: Enable postponed annotation evaluation
Add a from __future__ import annotations at the top of src/snakemake/modules.py to postpone evaluation of annotations and avoid a runtime NameError on the unquoted forward reference to WorkflowModifier at line 50.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between f3331af and ebf230c.

📒 Files selected for processing (1)
  • src/snakemake/modules.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 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/modules.py
🧠 Learnings (3)
📚 Learning: 2025-08-28T13:15:55.232Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/modules.py:195-195
Timestamp: 2025-08-28T13:15:55.232Z
Learning: In src/snakemake/modules.py, the line `self.globals["__name__"] = namespace` is intentionally designed to set __name__ to None when namespace is None (i.e., when not inside a module context, for top-level workflows). This is correct behavior and should not be guarded.

Applied to files:

  • src/snakemake/modules.py
📚 Learning: 2025-08-28T13:17:37.693Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/workflow.py:637-665
Timestamp: 2025-08-28T13:17:37.693Z
Learning: In Snakemake, rule.module_globals["__name__"] always exists and defaults to "snakemake.workflow" when not modified, so direct dictionary access is safe without needing .get().

Applied to files:

  • src/snakemake/modules.py
📚 Learning: 2025-08-29T11:51:44.598Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/modules.py:72-99
Timestamp: 2025-08-29T11:51:44.598Z
Learning: In Snakemake's module system, rule overrides are only permitted when a rule was previously imported via wildcard (`rule_whitelist is None`) and is now being imported again with specific modifications. The check_overwrite method should return True only for specific rule imports that were previously imported via wildcard, not for any non-wildcard imports.

Applied to files:

  • src/snakemake/modules.py
🧬 Code graph analysis (1)
src/snakemake/modules.py (4)
src/snakemake/common/__init__.py (2)
  • Rules (253-271)
  • _register_rule (259-260)
src/snakemake/exceptions.py (1)
  • CreateRuleException (526-527)
src/snakemake/path_modifier.py (2)
  • PathModifier (16-153)
  • replace_prefix (71-75)
src/snakemake/workflow.py (10)
  • rules (589-590)
  • rules (759-760)
  • modifier (542-543)
  • snakefile (1557-1562)
  • config (1746-1747)
  • name (2293-2298)
  • rule (1755-2032)
  • globals (554-555)
  • wildcard_constraints (550-551)
  • modules (546-547)
⏰ 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). (55)
  • GitHub Check: tests (10, windows-latest, py313)
  • GitHub Check: tests (10, windows-latest, py312)
  • GitHub Check: tests (10, ubuntu-latest, py313)
  • GitHub Check: tests (9, ubuntu-latest, py312)
  • GitHub Check: tests (8, ubuntu-latest, py313)
  • GitHub Check: tests (10, ubuntu-latest, py312)
  • GitHub Check: tests (9, ubuntu-latest, py313)
  • GitHub Check: tests (10, ubuntu-latest, py311)
  • GitHub Check: tests (8, windows-latest, py313)
  • GitHub Check: tests (9, windows-latest, py313)
  • GitHub Check: tests (9, windows-latest, py312)
  • GitHub Check: tests (8, windows-latest, py312)
  • GitHub Check: tests (8, ubuntu-latest, py312)
  • GitHub Check: tests (9, ubuntu-latest, py311)
  • GitHub Check: tests (7, windows-latest, py312)
  • GitHub Check: tests (8, ubuntu-latest, py311)
  • GitHub Check: tests (6, windows-latest, py313)
  • GitHub Check: tests (6, ubuntu-latest, py312)
  • GitHub Check: tests (7, ubuntu-latest, py313)
  • GitHub Check: tests (7, ubuntu-latest, py311)
  • GitHub Check: tests (7, ubuntu-latest, py312)
  • GitHub Check: tests (7, windows-latest, py313)
  • GitHub Check: tests (5, ubuntu-latest, py312)
  • GitHub Check: tests (6, ubuntu-latest, py313)
  • GitHub Check: tests (6, ubuntu-latest, py311)
  • GitHub Check: tests (5, windows-latest, py313)
  • GitHub Check: tests (3, windows-latest, py313)
  • GitHub Check: tests (5, ubuntu-latest, py313)
  • GitHub Check: tests (2, ubuntu-latest, py312)
  • GitHub Check: tests (6, windows-latest, py312)
  • GitHub Check: tests (4, macos-latest, py313)
  • GitHub Check: tests (4, ubuntu-latest, py311)
  • GitHub Check: tests (5, windows-latest, py312)
  • GitHub Check: tests (5, ubuntu-latest, py311)
  • GitHub Check: tests (4, ubuntu-latest, py312)
  • GitHub Check: tests (4, ubuntu-latest, py313)
  • GitHub Check: tests (4, windows-latest, py312)
  • GitHub Check: tests (3, macos-latest, py313)
  • GitHub Check: tests (4, windows-latest, py313)
  • GitHub Check: tests (2, windows-latest, py312)
  • GitHub Check: tests (3, ubuntu-latest, py313)
  • GitHub Check: tests (3, ubuntu-latest, py311)
  • GitHub Check: tests (3, windows-latest, py312)
  • GitHub Check: tests (3, ubuntu-latest, py312)
  • GitHub Check: tests (2, windows-latest, py313)
  • GitHub Check: tests (2, macos-latest, py312)
  • GitHub Check: tests (1, macos-latest, py313)
  • GitHub Check: tests (2, ubuntu-latest, py313)
  • GitHub Check: tests (2, ubuntu-latest, py311)
  • GitHub Check: tests (1, ubuntu-latest, py313)
  • GitHub Check: tests (1, windows-latest, py313)
  • GitHub Check: tests (1, windows-latest, py312)
  • GitHub Check: tests (1, macos-latest, py312)
  • GitHub Check: tests (1, ubuntu-latest, py312)
  • GitHub Check: tests (1, ubuntu-latest, py311)
🔇 Additional comments (9)
src/snakemake/modules.py (9)

18-30: Name-modifier handling looks correct.

Wildcard replacement and the single-rule constraint without a wildcard are implemented as expected.

66-68: PathModifier chaining LGTM.

Chaining to self.parent_modifier.path_modifier correctly composes prefix replacement across module layers.

72-99: Good: centralizing rule import via WorkflowModifier with explicit whitelist/excludes.

Propagation of rule_whitelist, local/resolved name modifiers, and path_modifier is coherent.

185-206: Module vs. local modifier initialization reads correctly.

  • Inheriting globals/checkpoints for modules and reusing parent state for local “use rule … as … with” is consistent.
  • OR-ing allow_rule_overwrite with the parent preserves nested-override intent.

221-221: Semantics change acknowledged: skip_configfile derived from presence of config.

Deriving skip_configfile from config is not None is reasonable and avoids another constructor arg.

235-240: Rule proxy inheritance is correct with local renaming.

Applying local_rulename_modifier during proxy registration prevents double-renaming upstream.

241-251: Whitelist/exclude gating before delegation is correct.

Early return None to skip non-selected rules, then delegate modified names to the parent.

277-284: Namespace materialization LGTM.

Exporting the modifier’s globals under self.namespace into workflow.globals is consistent with the module model.

50-53: Fix forward-reference type annotation that will NameError at runtime.

At class definition time, WorkflowModifier isn’t defined yet. Without from __future__ import annotations, self.parent_modifier: WorkflowModifier = ... evaluates immediately and raises. Quote the annotation (or add the future import project-wide).

Apply:

-        self.parent_modifier: WorkflowModifier = self.workflow.modifier
+        self.parent_modifier: "WorkflowModifier" = self.workflow.modifier

Optionally, consider adding from __future__ import annotations at the top of this file instead.

Likely an incorrect or invalid review comment.

coderabbitai[bot]
coderabbitai bot reviewed Aug 29, 2025
View reviewed changes
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

♻️ Duplicate comments (1)
src/snakemake/modules.py (1)

135-162: Overwrite gating logic looks correct.

  • Wildcard import records modifier and returns False.
  • Specific import only allows overwrite when every rule matches a prior wildcard-imported name; duplicate overwrites raise CreateRuleException.
🧹 Nitpick comments (1)
src/snakemake/modules.py (1)

50-53: Nit: rename for clarity.

Consider renaming wildcards_modifier_overwrited → wildcards_modifier_overwritten (grammar/readability).

Minimal rename (all occurrences in this file):

-        self.wildcards_modifier_overwrited = {}
+        self.wildcards_modifier_overwritten = {}
@@
-            self.wildcards_modifier_overwrited[rulename_modifier] = set()
+            self.wildcards_modifier_overwritten[rulename_modifier] = set()
@@
-            for modified in self.wildcards_modifier_overwrited:
+            for modified in self.wildcards_modifier_overwritten:
@@
-                    if rule in self.wildcards_modifier_overwrited[modified]:
+                    if rule in self.wildcards_modifier_overwritten[modified]:
@@
-                    self.wildcards_modifier_overwrited[modified].add(rule)
+                    self.wildcards_modifier_overwritten[modified].add(rule)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between ebf230c and d8782fb.

📒 Files selected for processing (1)
  • src/snakemake/modules.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 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/modules.py
🧠 Learnings (3)
📚 Learning: 2025-08-28T13:15:55.232Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/modules.py:195-195
Timestamp: 2025-08-28T13:15:55.232Z
Learning: In src/snakemake/modules.py, the line `self.globals["__name__"] = namespace` is intentionally designed to set __name__ to None when namespace is None (i.e., when not inside a module context, for top-level workflows). This is correct behavior and should not be guarded.

Applied to files:

  • src/snakemake/modules.py
📚 Learning: 2025-08-28T13:17:37.693Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/workflow.py:637-665
Timestamp: 2025-08-28T13:17:37.693Z
Learning: In Snakemake, rule.module_globals["__name__"] always exists and defaults to "snakemake.workflow" when not modified, so direct dictionary access is safe without needing .get().

Applied to files:

  • src/snakemake/modules.py
📚 Learning: 2025-08-29T11:51:44.598Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/modules.py:72-99
Timestamp: 2025-08-29T11:51:44.598Z
Learning: In Snakemake's module system, rule overrides are only permitted when a rule was previously imported via wildcard (`rule_whitelist is None`) and is now being imported again with specific modifications. The check_overwrite method should return True only for specific rule imports that were previously imported via wildcard, not for any non-wildcard imports.

Applied to files:

  • src/snakemake/modules.py
🧬 Code graph analysis (1)
src/snakemake/modules.py (4)
src/snakemake/common/__init__.py (2)
  • Rules (253-271)
  • _register_rule (259-260)
src/snakemake/exceptions.py (1)
  • CreateRuleException (526-527)
src/snakemake/path_modifier.py (2)
  • PathModifier (16-153)
  • replace_prefix (71-75)
src/snakemake/workflow.py (8)
  • rules (589-590)
  • rules (759-760)
  • modifier (542-543)
  • snakefile (1557-1562)
  • config (1746-1747)
  • name (2293-2298)
  • rule (1755-2032)
  • globals (554-555)
⏰ 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). (56)
  • GitHub Check: tests (2, ubuntu-latest, py313)
  • GitHub Check: tests (1, ubuntu-latest, py311)
  • GitHub Check: tests (10, ubuntu-latest, py312)
  • GitHub Check: tests (10, windows-latest, py313)
  • GitHub Check: tests (9, windows-latest, py313)
  • GitHub Check: tests (9, ubuntu-latest, py313)
  • GitHub Check: tests (10, ubuntu-latest, py311)
  • GitHub Check: tests (10, ubuntu-latest, py313)
  • GitHub Check: tests (10, windows-latest, py312)
  • GitHub Check: tests (9, ubuntu-latest, py312)
  • GitHub Check: tests (9, windows-latest, py312)
  • GitHub Check: tests (6, ubuntu-latest, py313)
  • GitHub Check: tests (7, windows-latest, py312)
  • GitHub Check: tests (8, windows-latest, py313)
  • GitHub Check: tests (7, ubuntu-latest, py311)
  • GitHub Check: tests (8, ubuntu-latest, py311)
  • GitHub Check: tests (8, ubuntu-latest, py313)
  • GitHub Check: tests (6, ubuntu-latest, py312)
  • GitHub Check: tests (9, ubuntu-latest, py311)
  • GitHub Check: tests (8, windows-latest, py312)
  • GitHub Check: tests (7, ubuntu-latest, py313)
  • GitHub Check: tests (8, ubuntu-latest, py312)
  • GitHub Check: tests (4, ubuntu-latest, py312)
  • GitHub Check: tests (7, ubuntu-latest, py312)
  • GitHub Check: tests (7, windows-latest, py313)
  • GitHub Check: tests (6, windows-latest, py312)
  • GitHub Check: tests (6, macos-latest, py312)
  • GitHub Check: tests (5, windows-latest, py313)
  • GitHub Check: tests (6, windows-latest, py313)
  • GitHub Check: tests (6, ubuntu-latest, py311)
  • GitHub Check: tests (5, windows-latest, py312)
  • GitHub Check: tests (5, ubuntu-latest, py312)
  • GitHub Check: tests (4, ubuntu-latest, py313)
  • GitHub Check: tests (5, ubuntu-latest, py311)
  • GitHub Check: tests (5, ubuntu-latest, py313)
  • GitHub Check: tests (4, windows-latest, py312)
  • GitHub Check: tests (4, windows-latest, py313)
  • GitHub Check: tests (1, windows-latest, py312)
  • GitHub Check: tests (4, ubuntu-latest, py311)
  • GitHub Check: tests (3, windows-latest, py313)
  • GitHub Check: tests (3, macos-latest, py312)
  • GitHub Check: tests (3, macos-latest, py313)
  • GitHub Check: tests (3, windows-latest, py312)
  • GitHub Check: tests (2, ubuntu-latest, py312)
  • GitHub Check: tests (3, ubuntu-latest, py313)
  • GitHub Check: tests (2, windows-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 (1, macos-latest, py313)
  • GitHub Check: tests (1, macos-latest, py312)
  • GitHub Check: tests (1, ubuntu-latest, py313)
  • GitHub Check: tests (2, windows-latest, py313)
  • GitHub Check: tests (1, windows-latest, py313)
  • GitHub Check: tests (1, ubuntu-latest, py312)
  • GitHub Check: apidocs
🔇 Additional comments (11)
src/snakemake/modules.py (11)

9-12: LGTM on imports.

New imports (Set, CreateRuleException) align with usage below.

66-68: LGTM: path modifier chaining.

Passing parent path_modifier into a new PathModifier correctly composes prefix replacement across module boundaries.

72-99: LGTM: use_rules wiring.

Whitelist, local/resolved name modifiers, overwrite flag, and proxy inheritance look consistent with workflow.py expectations.

126-133: LGTM: whitelist computation.

Wildcard detection with “*” → None is correct and validated.

185-205: LGTM: module modifier init is robust.

Fallbacks for rule_proxies/path_modifier avoid earlier None pitfalls; parent allow_rule_overwrite propagation is correct; name handling is intentional per prior discussion.

206-216: LGTM: non-module modifier inherits parent state.

State propagation (globals, proxies, modules, path_modifier, overwrite flag) is consistent.

221-221: LGTM: skip_configfile derivation.

Using presence of config to infer skip_configfile aligns with constructor semantics.

235-240: LGTM: proxy inheritance with local renaming.

Using child local_rulename_modifier when registering upstream preserves visible names.

241-251: LGTM: availability check cascades correctly.

Whitelist/exclude evaluated at each layer, then delegated upward with name modification.

254-258: LGTM: rulename modification cascade.

Local modification then delegation to parent is correct.

267-271: LGTM: wrapper URI tag replacement.

Skips URLs; uses compiled pattern; behavior matches docs.

coderabbitai[bot]
coderabbitai bot reviewed Aug 29, 2025
View reviewed changes
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 (2)
tests/test_module_nested/module_shallow.smk (2)

20-24: Nit: avoid tuple-izing log by trailing comma.
A trailing comma makes "aaalog" a 1-tuple; Snakemake accepts it, but a plain string is clearer.

 use rule run from A as A_run with:
     input:
         rds=config.get("bb", ".snakemake"),
     log:
-        "aaalog",
+        "aaalog"

33-37: Update stale comment to match the assertion.
The comment still references deep_work; the assertion checks A_run. Align them for clarity.

-# rules.deep_work has to work even if the rule is renamed in a parent module
-# The rulename itself can be already modified.
+# rules.A_run must be accessible even if the rule was renamed in a parent module.
+# The rulename may already be modified via wildcard export.
 assert hasattr(
-    rules, "A_run"
-), f"bug: rule cannot be accessed as A_run: {dir(rules)}"
+    rules, "A_run"
+), f"bug: rule cannot be accessed as A_run: {dir(rules)}"
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between d8782fb and 88b9399.

📒 Files selected for processing (2)
  • tests/test_module_nested/Snakefile (1 hunks)
  • tests/test_module_nested/module_shallow.smk (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/test_module_nested/Snakefile
⏰ 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). (57)
  • GitHub Check: tests (10, ubuntu-latest, py311)
  • GitHub Check: tests (10, ubuntu-latest, py313)
  • GitHub Check: tests (9, ubuntu-latest, py313)
  • GitHub Check: tests (9, windows-latest, py312)
  • GitHub Check: tests (10, windows-latest, py312)
  • GitHub Check: tests (9, ubuntu-latest, py311)
  • GitHub Check: tests (10, ubuntu-latest, py312)
  • GitHub Check: tests (9, ubuntu-latest, py312)
  • GitHub Check: tests (8, windows-latest, py313)
  • GitHub Check: tests (8, ubuntu-latest, py311)
  • GitHub Check: tests (8, ubuntu-latest, py312)
  • GitHub Check: tests (9, windows-latest, py313)
  • GitHub Check: tests (10, windows-latest, py313)
  • GitHub Check: tests (6, windows-latest, py312)
  • GitHub Check: tests (8, windows-latest, py312)
  • GitHub Check: tests (7, macos-latest, py312)
  • GitHub Check: tests (8, ubuntu-latest, py313)
  • GitHub Check: tests (2, ubuntu-latest, py313)
  • GitHub Check: tests (6, windows-latest, py313)
  • GitHub Check: tests (7, windows-latest, py313)
  • GitHub Check: tests (7, ubuntu-latest, py312)
  • GitHub Check: tests (7, windows-latest, py312)
  • GitHub Check: tests (6, ubuntu-latest, py311)
  • GitHub Check: tests (4, windows-latest, py313)
  • GitHub Check: tests (6, ubuntu-latest, py312)
  • GitHub Check: tests (5, windows-latest, py313)
  • GitHub Check: tests (4, ubuntu-latest, py313)
  • GitHub Check: tests (7, ubuntu-latest, py311)
  • GitHub Check: tests (7, ubuntu-latest, py313)
  • GitHub Check: tests (5, ubuntu-latest, py313)
  • GitHub Check: tests (5, ubuntu-latest, py312)
  • GitHub Check: tests (5, ubuntu-latest, py311)
  • GitHub Check: tests (6, ubuntu-latest, py313)
  • GitHub Check: tests (5, windows-latest, py312)
  • GitHub Check: tests (3, windows-latest, py312)
  • GitHub Check: tests (2, windows-latest, py312)
  • GitHub Check: tests (1, windows-latest, py312)
  • GitHub Check: tests (3, ubuntu-latest, py311)
  • GitHub Check: tests (4, ubuntu-latest, py312)
  • GitHub Check: tests (4, ubuntu-latest, py311)
  • GitHub Check: tests (4, windows-latest, py312)
  • GitHub Check: tests (2, macos-latest, py313)
  • GitHub Check: tests (2, windows-latest, py313)
  • GitHub Check: tests (3, ubuntu-latest, py312)
  • GitHub Check: tests (3, ubuntu-latest, py313)
  • GitHub Check: tests (3, macos-latest, py312)
  • GitHub Check: tests (3, windows-latest, py313)
  • GitHub Check: tests (1, ubuntu-latest, py311)
  • GitHub Check: tests (1, macos-latest, py312)
  • GitHub Check: tests (2, ubuntu-latest, py311)
  • GitHub Check: tests (1, macos-latest, py313)
  • GitHub Check: tests (2, ubuntu-latest, py312)
  • GitHub Check: tests (1, ubuntu-latest, py312)
  • GitHub Check: tests (1, windows-latest, py313)
  • GitHub Check: tests (1, ubuntu-latest, py313)
  • GitHub Check: formatting
  • GitHub Check: apidocs
🔇 Additional comments (3)
tests/test_module_nested/module_shallow.smk (3)

1-1: LGTM: deterministic shell for tests.
Using bash explicitly is fine for the test suite.

14-18: Double “use … as A_run” may trip the new overwrite restrictions.
You export A_run via the wildcard (A_*) and then use rule run as A_run again to set threads. Ensure the new “restrict overwrite” logic treats this as an additive modifier of the same origin rule, not an illegal overwrite.

4-8: No module alias collisions detected Verified that module A: appears only once in tests/test_module_nested (module_shallow.smk), so the b- prefix is safe.

@Hocnonsense
Copy link
Copy Markdown
Contributor Author

Hocnonsense commented Aug 29, 2025

Dear Professor @johanneskoester,

I believe this PR is ready to be merged -- would you mind having a look when you get a chance?
In the meantime, a few of my other PRs are also ready for review :)

Thanks again for your time and for maintaining this repository!

Best regards,
Aoran Hu

coderabbitai[bot]
coderabbitai bot reviewed Sep 4, 2025
View reviewed changes
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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/snakemake/workflow.py (1)

641-665: Fix overwrite semantics in add_rule (is_overwrite, counters, stale rule cleanup).

  • is_overwrite is never set to True, so overwrite-specific paths never trigger.
  • rule_count and default_target should not be modified on overwrite.
  • Replacing the rule object without removing the old one from modifier.rules can leave stale references.

Apply:

@@
-    if self.is_rule(name) and not allow_overwrite:
+    existed = self.is_rule(name)
+    if existed and not allow_overwrite:
         raise CreateRuleException(
             f"The name {name} is already used by another rule",
             lineno=lineno,
             snakefile=snakefile,
         )
-    else:
-        is_overwrite = False
-        self.rule_count += 1
-        if not self.default_target:
-            self.default_target = name
+    is_overwrite = existed
+    if not is_overwrite:
+        self.rule_count += 1
+        if self.default_target is None:
+            self.default_target = name
+    else:
+        # avoid stale references to the old rule object
+        old_rule = self._rules[name]
+        try:
+            self.modifier.rules.discard(old_rule)
+        except AttributeError:
+            try:
+                self.modifier.rules.remove(old_rule)
+            except Exception:
+                pass
@@
     rule = Rule(name, self, lineno=lineno, snakefile=snakefile)
     self._rules[rule.name] = rule
     self.modifier.rules.add(rule)
     return is_overwrite
♻️ Duplicate comments (2)
src/snakemake/workflow.py (2)

1790-1792: Do not mutate rule.module_globals["name"] on overwrite.

Overwriting a rule should not clobber the module’s __name__; this can break subsequent same-namespace overrides and pollute exported state.

-        if is_overwrite:
-            rule.module_globals["__name__"] = None
+        # Overwrite permission is enforced via add_rule() and the modifier; leave module __name__ intact.

1805-1808: Guard rule rename against collisions.

Renaming without checking collisions can clobber an existing rule.

-            if ruleinfo.name:
-                del self._rules[name]
-                name = rule.name = ruleinfo.name
-                self._rules[name] = rule
+            if ruleinfo.name:
+                new_name = ruleinfo.name
+                if new_name != name and self.is_rule(new_name):
+                    raise CreateRuleException(
+                        f"The name {new_name} is already used by another rule",
+                        lineno=lineno,
+                        snakefile=snakefile,
+                    )
+                del self._rules[name]
+                name = rule.name = new_name
+                self._rules[name] = rule
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 88b9399 and 000c13f.

📒 Files selected for processing (2)
  • src/snakemake/workflow.py (11 hunks)
  • tests/tests.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/tests.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/workflow.py
🧠 Learnings (3)
📚 Learning: 2025-08-29T11:51:44.625Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/modules.py:72-99
Timestamp: 2025-08-29T11:51:44.625Z
Learning: In Snakemake's module system, rule overrides are only permitted when a rule was previously imported via wildcard (`rule_whitelist is None`) and is now being imported again with specific modifications. The check_overwrite method should return True only for specific rule imports that were previously imported via wildcard, not for any non-wildcard imports.

Applied to files:

  • src/snakemake/workflow.py
📚 Learning: 2025-08-28T13:17:37.693Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/workflow.py:637-665
Timestamp: 2025-08-28T13:17:37.693Z
Learning: In Snakemake, rule.module_globals["__name__"] always exists and defaults to "snakemake.workflow" when not modified, so direct dictionary access is safe without needing .get().

Applied to files:

  • src/snakemake/workflow.py
📚 Learning: 2025-08-28T13:15:55.232Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/modules.py:195-195
Timestamp: 2025-08-28T13:15:55.232Z
Learning: In src/snakemake/modules.py, the line `self.globals["__name__"] = namespace` is intentionally designed to set __name__ to None when namespace is None (i.e., when not inside a module context, for top-level workflows). This is correct behavior and should not be guarded.

Applied to files:

  • src/snakemake/workflow.py
🧬 Code graph analysis (1)
src/snakemake/workflow.py (3)
src/snakemake/rules.py (6)
  • Rule (76-1339)
  • name (130-131)
  • name (134-135)
  • lineno (138-139)
  • snakefile (142-143)
  • add (1346-1350)
src/snakemake/modules.py (2)
  • avail_rulename (241-251)
  • get_name_modifier_func (18-29)
src/snakemake/ruleinfo.py (1)
  • RuleInfo (14-111)
🪛 Ruff (0.12.2)
src/snakemake/workflow.py

651-655: Avoid specifying long messages outside the exception class

(TRY003)

🔇 Additional comments (9)
src/snakemake/workflow.py (9)

180-180: LGTM: explicit typing of _rules.

Clearer intent; preserves insertion order semantics.

1770-1774: LGTM: orig_name handling.

Keeps legacy unnamed-rule numbering intact.

1776-1779: LGTM: rulename availability gate.

Early-return to ignore non-matching local inheritance is appropriate.

1781-1786: Couples to add_rule() fix.

Once add_rule() sets is_overwrite correctly (see above), downstream logic here will behave as intended.

After applying the add_rule() patch, please run the module overwrite tests to confirm behavior.

1793-1793: LGTM: annotate decorate(ruleinfo: RuleInfo).

Harmless and clarifies intent.

2340-2344: LGTM: userule signature with explicit typing.

Clearer API surface; no behavioral concerns.

2413-2419: LGTM: WorkflowModifier usage for local inheritance.

Passing is_module=False and a resolved name modifier aligns with the new modifier model.

552-555: Confirmed: no direct assignments to Workflow.modules exist; resolving.

2402-2404: Verify avail_rulename(None) handling in local inheritance
In src/snakemake/workflow.py (around line 2401), name_modifier: str | None can be None, so self.modifier.avail_rulename(name_modifier) will evaluate to falsy and invoke the early return, silently skipping the local use rule block. Confirm that calling avail_rulename(None) here is intentional—otherwise add a guard to require a concrete alias (or adjust avail_rulename to treat None as “always available”).

johanneskoester
johanneskoester reviewed Sep 4, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@johanneskoester johanneskoester left a comment

Choose a reason for hiding this comment

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

THanks a lot. The changes are quite extensive. Can you provide an outline of what you changed where and why?

@Hocnonsense
Copy link
Copy Markdown
Contributor Author

Hocnonsense commented Sep 5, 2025

Thanks! Here's an outline of the changes I made, organized by purpose and file:

1: Support recursive module modification via WorkflowModifier

To make workflows recursively apply modifiers like path prefix and rule name changes across nested modules, I introduced the following changes:

  • src/snakemake/ruleinfo.py:
    • Made RuleInfo recursively apply parent_modifier to adjust paths and rule names.
  • src/snakemake/path_modifier.py:
    • Added inner_modifier to support chained path transformations in nested modules.
  • src/snakemake/modules.py:
    • Modified get_name_modifier_func and WorkflowModifier.modify_rulename to support recursive rule renaming.
    • Replaced WorkflowModifier.skip_rule with avail_rulename to ensure that rule_whitelist and rule_exclude_list are respected at every module level.

2: Make multiple use statements with specific_rule more reliable and strict

My understanding is that rule.name should be unique in a workflow. Therefore, a second use rule from ... with ... should only be allowed if the rule was previously imported via a wildcard (use * from ...) and is now being refined.

  • src/snakemake/rules.py:
    • The old logic stored rule dependencies as objects. If a rule was modified afterward, those changes wouldn't propagate.
    • Now, rule dependencies are resolved dynamically by name, ensuring correct updated rule with parameters modified.
  • src/snakemake/modules.py:
    • Stricter checks for rules overwriting. ModuleInfo.use_rules now allow_rule_overwrite for specific_rule only if it was previously imported via use * from ... and not already customized.
  • src/snakemake/workflow.py:
    • Adjusted to work with the new WorkflowModifier.

Other changes:

  • Simplify WorkflowModifier.__init__.
  • The module’s __name__ is now assigned as WorkflowModifier.namespace, making workflow development and debugging easier.

coderabbitai[bot]
coderabbitai bot reviewed Sep 5, 2025
View reviewed changes
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

♻️ Duplicate comments (2)
src/snakemake/workflow.py (2)

1614-1617: Duplicate include check by object identity (not path).

This was discussed earlier; switching to a stable key (path/URI) avoids missed duplicates. Fine to defer to a separate PR as noted.

1823-1826: Guard against renaming collisions when applying ruleinfo.name.

Renaming currently replaces the dict key without checking for an existing different rule of the same name.

Apply:

-            if ruleinfo.name:
-                del self._rules[name]
-                name = rule.name = ruleinfo.name
-                self._rules[name] = rule
+            if ruleinfo.name:
+                new_name = ruleinfo.name
+                if new_name != name and self.is_rule(new_name):
+                    raise CreateRuleException(
+                        f"The name {new_name} is already used by another rule",
+                        lineno=lineno,
+                        snakefile=snakefile,
+                    )
+                del self._rules[name]
+                name = rule.name = new_name
+                self._rules[name] = rule
🧹 Nitpick comments (4)
src/snakemake/workflow.py (1)

652-665: Overwriting a rule should evict the previous Rule object from modifier.rules.

When allow_overwrite=True, the old Rule remains in self.modifier.rules (set now contains both objects). Prefer discarding the old one to avoid duplicate processing in places that iterate this set.

Apply:

-        rule = Rule(name, self, lineno=lineno, snakefile=snakefile)
-        self._rules[rule.name] = rule
-        self.modifier.rules.add(rule)
+        prev = self._rules.get(name) if allow_overwrite else None
+        rule = Rule(name, self, lineno=lineno, snakefile=snakefile)
+        self._rules[name] = rule
+        if prev is not None:
+            self.modifier.rules.discard(prev)
+        self.modifier.rules.add(rule)

Also minor lint nit on long error string (TRY003); optional to keep.

src/snakemake/modules.py (3)

18-31: Name-modifier validation is good; tiny linter nit for constant renamer.

Use an underscore to appease linters about the unused lambda param.

Apply:

-            return lambda rulename: name_modifier
+            return lambda _rulename: name_modifier

51-54: Nit: rename ‘wildcards_modifier_overwrited’ for clarity.

“overwritten” reads better and avoids confusion.

Apply (example):

-        self.wildcards_modifier_overwrited = {}
+        self.wildcards_modifier_overwritten = {}
@@
-        if not rule_whitelist:  # all will be used, exclude rules do not matter
-            self.wildcards_modifier_overwrited[rulename_modifier] = set()
+        if not rule_whitelist:  # wildcard import: record and disallow overwrite now
+            self.wildcards_modifier_overwritten[rulename_modifier] = set()
             return False
@@
-            for modified in self.wildcards_modifier_overwrited:
+            for modified in self.wildcards_modifier_overwritten:
                 if (modified(rule) if modified else rule) == modified_this:
                     matched.add(rule)
-                    if rule in self.wildcards_modifier_overwrited[modified]:
+                    if rule in self.wildcards_modifier_overwritten[modified]:
                         raise CreateRuleException(
                             f"The rule {rule} is imported with same name modifier and modified more than once",
                             snakefile=self.snakefile,
                         )
-                    self.wildcards_modifier_overwrited[modified].add(rule)
+                    self.wildcards_modifier_overwritten[modified].add(rule)

Also applies to: 144-158

136-163: Overwrite-check logic — LGTM; minor message nit.

Algorithm matches the intended “specific import can overwrite prior wildcard import” semantics. Optional: shorten the exception message to satisfy TRY003.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 000c13f and f15dfa5.

📒 Files selected for processing (4)
  • src/snakemake/modules.py (5 hunks)
  • src/snakemake/rules.py (4 hunks)
  • src/snakemake/workflow.py (10 hunks)
  • tests/tests.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/snakemake/rules.py
  • tests/tests.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/modules.py
  • src/snakemake/workflow.py
🧠 Learnings (4)
📚 Learning: 2025-08-28T13:15:55.232Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/modules.py:195-195
Timestamp: 2025-08-28T13:15:55.232Z
Learning: In src/snakemake/modules.py, the line `self.globals["__name__"] = namespace` is intentionally designed to set __name__ to None when namespace is None (i.e., when not inside a module context, for top-level workflows). This is correct behavior and should not be guarded.

Applied to files:

  • src/snakemake/modules.py
  • src/snakemake/workflow.py
📚 Learning: 2025-08-28T13:17:37.693Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/workflow.py:637-665
Timestamp: 2025-08-28T13:17:37.693Z
Learning: In Snakemake, rule.module_globals["__name__"] always exists and defaults to "snakemake.workflow" when not modified, so direct dictionary access is safe without needing .get().

Applied to files:

  • src/snakemake/modules.py
  • src/snakemake/workflow.py
📚 Learning: 2025-08-29T11:51:44.625Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/modules.py:72-99
Timestamp: 2025-08-29T11:51:44.625Z
Learning: In Snakemake's module system, rule overrides are only permitted when a rule was previously imported via wildcard (`rule_whitelist is None`) and is now being imported again with specific modifications. The check_overwrite method should return True only for specific rule imports that were previously imported via wildcard, not for any non-wildcard imports.

Applied to files:

  • src/snakemake/modules.py
  • src/snakemake/workflow.py
📚 Learning: 2024-11-12T12:08:20.342Z 
Learnt from: johanneskoester
PR: snakemake/snakemake#3148
File: snakemake/dag.py:1332-1336
Timestamp: 2024-11-12T12:08:20.342Z
Learning: In `snakemake/dag.py`, when code is outdated and will disappear upon resolving merge conflicts, avoid making code review suggestions on that code.

Applied to files:

  • src/snakemake/workflow.py
🧬 Code graph analysis (2)
src/snakemake/modules.py (4)
src/snakemake/common/__init__.py (2)
  • Rules (253-271)
  • _register_rule (259-260)
src/snakemake/exceptions.py (1)
  • CreateRuleException (526-527)
src/snakemake/path_modifier.py (2)
  • PathModifier (16-153)
  • replace_prefix (71-75)
src/snakemake/workflow.py (6)
  • rules (598-599)
  • rules (767-768)
  • modifier (551-552)
  • snakefile (1571-1576)
  • name (2326-2331)
  • rule (1789-2065)
src/snakemake/workflow.py (2)
src/snakemake/rules.py (5)
  • Rule (76-1347)
  • name (131-132)
  • name (135-136)
  • lineno (139-140)
  • snakefile (143-144)
src/snakemake/modules.py (2)
  • avail_rulename (242-252)
  • get_name_modifier_func (18-30)
🪛 Ruff (0.12.2)
src/snakemake/modules.py

27-29: Avoid specifying long messages outside the exception class

(TRY003)

30-30: Unused lambda argument: rulename

(ARG005)

154-157: Avoid specifying long messages outside the exception class

(TRY003)

src/snakemake/workflow.py

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

182-182: Type-annotated rules map looks good.

No issues with making _rules an OrderedDict[str, Rule].

554-557: Expose modules via Workflow.property — LGTM.

Returning self.modifier.modules is consistent with the new modifier model.

1792-1807: Rule name selection and overwrite wiring — LGTM.

avail_rulename() gating and allow_rule_overwrite propagation are consistent.

2361-2365: Typed parameters for userule — LGTM.

The updated signature improves clarity without changing behavior.

src/snakemake/modules.py (4)

73-101: Module use_rules wiring — LGTM.

Whitelist, name-modifier resolution, and WorkflowModifier construction look consistent.

186-206: WorkflowModifier (module branch) initialization — LGTM.

Parent propagation and globals/namespace materialization are coherent with the new model.

207-217: WorkflowModifier (local branch) state sharing — LGTM.

Correctly inherits from parent for local ‘use rule’ blocks.

242-253: avail_rulename chaining — LGTM.

Whitelist/exclude filtering and parent delegation are sound.

coderabbitai[bot]
coderabbitai bot reviewed Sep 12, 2025
View reviewed changes
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: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
docs/snakefiles/modularization.rst (2)

319-320: Grammar: “its” not “it’s”.

Possessive pronoun required here.

-And finally, we overwrite the input directive of the rule ``bwa_mem`` such that the raw data is taken from the place where our workflow configures it via it's config file.
+And finally, we overwrite the input directive of the rule ``bwa_mem`` such that the raw data is taken from the place where our workflow configures it via its config file.

329-341: Brand capitalization: GitHub/GitLab.

Use official names.

-Currently, Github
+Currently, GitHub
@@
-and Gitlab are supported:
+and GitLab are supported:
🧹 Nitpick comments (4)
src/snakemake/modules.py (2)

18-31: Silence ARG005 by using a throwaway arg in the constant renamer.

The constant renamer’s lambda doesn’t use its parameter; switch to _ to satisfy linters without changing behavior.

-            return lambda rulename: name_modifier.replace("*", rulename)
+            return lambda rulename: name_modifier.replace("*", rulename)
@@
-            return lambda rulename: name_modifier
+            return lambda _: name_modifier

53-54: Rename “overwrited” → “overwritten” for clarity.

Minor wording fix; keeps code intent unchanged.

-        self.wildcards_modifier_overwrited: Dict[Callable | None, Set[str]] = {}
+        self.wildcards_modifier_overwritten: Dict[Callable | None, Set[str]] = {}
@@
-        if not rule_whitelist:  # all will be used, exclude rules do not matter
-            self.wildcards_modifier_overwrited[rulename_modifier] = set()
+        if not rule_whitelist:  # all will be used, exclude rules do not matter
+            self.wildcards_modifier_overwritten[rulename_modifier] = set()
             return False
@@
-            for modified in self.wildcards_modifier_overwrited:
+            for modified in self.wildcards_modifier_overwritten:
                 if (modified(rule) if modified else rule) == modified_this:
-                    if rule in self.wildcards_modifier_overwrited[modified]:
+                    if rule in self.wildcards_modifier_overwritten[modified]:
                         raise CreateRuleException(
                             f"The rule {rule} is imported with same name modifier and modified more than once",
                             snakefile=self.snakefile,
                         )
                     matched.add(rule)
-                    self.wildcards_modifier_overwrited[modified].add(rule)
+                    self.wildcards_modifier_overwritten[modified].add(rule)

Also applies to: 145-159

docs/snakefiles/modularization.rst (2)

247-271: Clarify dynamic “as” alias phrasing to avoid confusion.

Make explicit that the alias after as must be given via a variable (not a literal) and may include *. This avoids misreading “module_name*” as a literal.

-In particular, it is not possible to modify the alias name in the ``use rule`` statement (e.g., writing directly ``use rule * from module as module_*`` is not allowed for dynamic modules).
+In particular, you cannot use a literal alias there (e.g., ``use rule * from module as module_*`` is not allowed). Instead, the alias after ``as`` must be provided via a variable (e.g., ``as alias*`` where ``alias`` is a variable).

120-139: Note about constant alias with multi/wildcard imports.

Consider adding a short note that as must include * when importing multiple rules or using a wildcard, to mirror the implementation guard.

.. note::

    When importing multiple rules or using a wildcard (``use rule *``), the alias given after ``as`` must include ``*`` (e.g., ``as other_*``). A constant alias without ``*`` is only allowed when importing a single, specific rule.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 96e5004 and 0b460b5.

📒 Files selected for processing (3)
  • docs/snakefiles/modularization.rst (6 hunks)
  • docs/snakefiles/rules.rst (1 hunks)
  • src/snakemake/modules.py (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/snakefiles/rules.rst
🧰 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/modules.py
🧠 Learnings (4)
📚 Learning: 2025-08-29T11:51:44.625Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/modules.py:72-99
Timestamp: 2025-08-29T11:51:44.625Z
Learning: In Snakemake's module system, rule overrides are only permitted when a rule was previously imported via wildcard (`rule_whitelist is None`) and is now being imported again with specific modifications. The check_overwrite method should return True only for specific rule imports that were previously imported via wildcard, not for any non-wildcard imports.

Applied to files:

  • docs/snakefiles/modularization.rst
  • src/snakemake/modules.py
📚 Learning: 2025-01-23T08:17:08.873Z 
Learnt from: johanneskoester
PR: snakemake/snakemake#3263
File: snakemake/sourcecache.py:195-196
Timestamp: 2025-01-23T08:17:08.873Z
Learning: Including authentication tokens in Git repository URLs is accepted in the Snakemake codebase when using the HostedGitRepo class, as secure authentication handling through GitPython's credential helpers is not feasible in this context.

Applied to files:

  • docs/snakefiles/modularization.rst
📚 Learning: 2025-08-28T13:15:55.232Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/modules.py:195-195
Timestamp: 2025-08-28T13:15:55.232Z
Learning: In src/snakemake/modules.py, the line `self.globals["__name__"] = namespace` is intentionally designed to set __name__ to None when namespace is None (i.e., when not inside a module context, for top-level workflows). This is correct behavior and should not be guarded.

Applied to files:

  • src/snakemake/modules.py
📚 Learning: 2025-08-28T13:17:37.693Z 
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/workflow.py:637-665
Timestamp: 2025-08-28T13:17:37.693Z
Learning: In Snakemake, rule.module_globals["__name__"] always exists and defaults to "snakemake.workflow" when not modified, so direct dictionary access is safe without needing .get().

Applied to files:

  • src/snakemake/modules.py
🧬 Code graph analysis (1)
src/snakemake/modules.py (4)
src/snakemake/common/__init__.py (2)
  • Rules (254-272)
  • _register_rule (260-261)
src/snakemake/exceptions.py (1)
  • CreateRuleException (526-527)
src/snakemake/path_modifier.py (2)
  • PathModifier (16-153)
  • replace_prefix (71-75)
src/snakemake/workflow.py (5)
  • rules (598-599)
  • rules (767-768)
  • modifier (551-552)
  • name (2326-2331)
  • rule (1789-2065)
🪛 Ruff (0.12.2)
src/snakemake/modules.py

27-29: Avoid specifying long messages outside the exception class

(TRY003)

30-30: Unused lambda argument: rulename

(ARG005)

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

136-163: Overwrite gating logic matches intended semantics.

Allows overwriting only when a specific import targets rules previously imported via wildcard (same effective final name) and prevents multiple modifications of the same rule/name-modifier pair. Looks good.

242-259: Recursive name-availability check is correct.

Whitelist/exclude filtering before delegating to the parent preserves module-level constraints and ensures collisions are resolved at the outermost scope. LGTM.

docs/snakefiles/modularization.rst (2)

192-196: Import-time override semantics read clearly and match implementation.

Selective replacement for params keywords, full overwrite for other directives, execution step preserved. Good.

197-238: Overwrite precondition is well-documented.

Requirement to previously import via use rule * and prohibition of re-modifying under the same final name is explicit and aligned with code. LGTM.

@Hocnonsense
Copy link
Copy Markdown
Contributor Author

Hocnonsense commented Sep 12, 2025

Thanks for the suggestion! I've added a section in the docs to explain the behavior with examples, as discussed.
Please have a look when you get a chance, happy to refine further if needed.

@johanneskoester
Copy link
Copy Markdown
Contributor

johanneskoester commented Sep 19, 2025

Fantastic work, as always!!

@johanneskoester @coderabbitai
Update docs/snakefiles/modularization.rst
43f7bf6 
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@johanneskoester johanneskoester merged commit bd94dc1 into main Sep 19, 2025
55 of 57 checks passed
@johanneskoester johanneskoester deleted the fix-3713 branch September 19, 2025 06:08
@snakemake-bot snakemake-bot mentioned this pull request Sep 19, 2025
Merged
johanneskoester pushed a commit that referenced this pull request Sep 19, 2025
@snakemake-bot
chore(main): release 9.11.4 (#3746)
889dd6f 
🤖 I have created a release *beep* *boop*
---


##
[9.11.4](v9.11.3...v9.11.4)
(2025-09-19)


### Bug Fixes

* Confusion with Overriding input After Snakemake Modularization
([#3714](#3714))
([bd94dc1](bd94dc1))
* properly handle temp files in group jobs that are not needed outside
of the group (do not upload them to storage and delete them early)
([#3730](#3730))
([3ffd8e1](3ffd8e1))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
@johanneskoester
Copy link
Copy Markdown
Contributor

johanneskoester commented Sep 23, 2025

This most likely caused #3751. I am waiting for a testcase PR from #3751 and then probably revert this PR to fix the issue for now, which is more pressing than what this PR has actually solved. But let's see.

@Hocnonsense
Copy link
Copy Markdown
Contributor Author

Hocnonsense commented Sep 23, 2025

Apologies for the trouble this may have introduced.
I’ll look into this issue right away and will try my best to assist in debugging or preparing a fix.

@Hocnonsense Hocnonsense mentioned this pull request Sep 23, 2025
Merged
2 tasks
@coderabbitai coderabbitai bot mentioned this pull request Sep 23, 2025
Closed
2 tasks
johanneskoester pushed a commit that referenced this pull request Sep 25, 2025
@Hocnonsense
fix: select specific rule from module will raise "rule not defined" t…
d6fa3ca 
…hat cause #3751 failed (#3753)

will fix #3751 and fix #2838 and fix #3741

This is caused by a more strict filter of unwanted rule applyed in #3714

Here, when `use rule delly from dna_seq_varlociraptor as calling_delly
with:`, all rules will be ignored, including the rule `star_align`
referred in "v3.13.3/meta/bio/star_arriba/test/Snakefile", line 49":

```python
rule arriba:
    input:
        bam=rules.star_align.output.aln,
```

* [x] 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).


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

* **Tests**
* Expanded nested-module test to add an intermediate rule and verify
sequential handoff between consecutive rules, reproducing and guarding
against a previously observed filtering issue.

* **Chores**
* Improved rule registration and decorator behavior to better handle
dynamic rule names, module-level globals, and checkpoint registration.

* **Developer-facing**
* Updated rule-addition interface and related internals; callers that
programmatically register rules may need minor adjustments.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
@coderabbitai coderabbitai bot mentioned this pull request Sep 29, 2025
Merged
2 tasks
@coderabbitai coderabbitai bot mentioned this pull request Oct 13, 2025
Open
2 tasks
kjohnsen pushed a commit to kjohnsen/snakemake that referenced this pull request Dec 15, 2025
@Hocnonsense @johanneskoester
@coderabbitai @kjohnsen
fix: Confusion with Overriding input After Snakemake Modularization (s…
4eb7add 
…nakemake#3714)

will fix snakemake#3713

## 1: Support recursive module modification via `WorkflowModifier`
To make workflows recursively apply modifiers like path prefix and rule
name changes across nested modules, I introduced the following changes:

- src/snakemake/ruleinfo.py:
- Made `RuleInfo` recursively apply `parent_modifier` to adjust paths
and rule names.
- src/snakemake/path_modifier.py:
- Added `inner_modifier` to support chained path transformations in
nested modules.
- src/snakemake/modules.py:
- Modified `get_name_modifier_func` and
`WorkflowModifier.modify_rulename` to support recursive rule renaming.
- Replaced `WorkflowModifier.skip_rule` with `avail_rulename` to ensure
that `rule_whitelist` and `rule_exclude_list` are respected at every
module level.

## 2: Make multiple use statements with specific_rule more reliable and
strict
My understanding is that `rule.name` should be unique in a workflow.
Therefore, a second `use rule from ... with ...` should only be allowed
if the rule was previously imported with the same name via a wildcard
(`use * from ...`) and is now being refined.
- to clarify:
- A given rule from a module can be used **multiple times** in the same
workflow, as long as each usage assigns it a **unique name**.
- However, using the **same rule name** more than once is **not
allowed**, to prevent accidental overwrites, just like any rule rule
defined **out of** module **cannot** be overwrite.
- That’s the restriction behind the “only once” statement -- it applies
per final rule name, not per source rule.

- src/snakemake/rules.py:
- The old logic stored rule dependencies as objects. If a rule was
modified afterward, those changes wouldn't propagate.
- Now, rule dependencies are resolved dynamically by name, ensuring
correct updated rule with parameters modified.
- src/snakemake/modules.py:
- Stricter checks for rules overwriting. `ModuleInfo.use_rules` now
`allow_rule_overwrite` for specific_rule only if it was previously
imported via `use * from ...` and not already customized.
- src/snakemake/workflow.py:
  - Adjusted to work with the new `WorkflowModifier`.

## Other changes:
- Simplify `WorkflowModifier.__init__`. The module’s `__name__` is now
assigned as `WorkflowModifier.namespace`, making workflow development
and debugging easier.


### QC
* [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**
* Multi-stage path modifiers, a public module registry, and improved
name/path/wrapper modifier lifecycle for more predictable module
imports.

* **Bug Fixes**
* Prevents unintended duplicate rule creation from wildcard/name
modifications and enforces explicit overwrite semantics; tighter
path-replacement guards and more reliable dependency resolution.

* **Tests**
* Updated fixtures for renamed, aliased and nested modules/rules to
reflect modifier behavior changes.

* **Documentation**
* Expanded module-import and rule-customization guidance with examples
and conflict-resolution advice.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Johannes Köster <johannes.koester@tu-dortmund.de>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
kjohnsen pushed a commit to kjohnsen/snakemake that referenced this pull request Dec 15, 2025
@snakemake-bot @kjohnsen
chore(main): release 9.11.4 (snakemake#3746)
0625d06 
🤖 I have created a release *beep* *boop*
---


##
[9.11.4](snakemake/snakemake@v9.11.3...v9.11.4)
(2025-09-19)


### Bug Fixes

* Confusion with Overriding input After Snakemake Modularization
([snakemake#3714](snakemake#3714))
([bd94dc1](snakemake@bd94dc1))
* properly handle temp files in group jobs that are not needed outside
of the group (do not upload them to storage and delete them early)
([snakemake#3730](snakemake#3730))
([3ffd8e1](snakemake@3ffd8e1))

---
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
@Hocnonsense @kjohnsen
fix: select specific rule from module will raise "rule not defined" t…
8aa4eb9 
…hat cause snakemake#3751 failed (snakemake#3753)

will fix snakemake#3751 and fix snakemake#2838 and fix snakemake#3741

This is caused by a more strict filter of unwanted rule applyed in snakemake#3714

Here, when `use rule delly from dna_seq_varlociraptor as calling_delly
with:`, all rules will be ignored, including the rule `star_align`
referred in "v3.13.3/meta/bio/star_arriba/test/Snakefile", line 49":

```python
rule arriba:
    input:
        bam=rules.star_align.output.aln,
```

* [x] 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).


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

* **Tests**
* Expanded nested-module test to add an intermediate rule and verify
sequential handoff between consecutive rules, reproducing and guarding
against a previously observed filtering issue.

* **Chores**
* Improved rule registration and decorator behavior to better handle
dynamic rule names, module-level globals, and checkpoint registration.

* **Developer-facing**
* Updated rule-addition interface and related internals; callers that
programmatically register rules may need minor adjustments.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
@coderabbitai coderabbitai bot mentioned this pull request Jan 8, 2026
Merged
2 tasks
This was referenced Jan 9, 2026
Closed
Closed
This was referenced Mar 9, 2026
Closed
Closed
Closed
@Hocnonsense Hocnonsense self-assigned this Mar 18, 2026
This was referenced Mar 22, 2026
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@johanneskoester johanneskoester johanneskoester approved these changes

Assignees

@Hocnonsense Hocnonsense

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Confusion with Overriding input After Snakemake Modularization

2 participants

@Hocnonsense @johanneskoester