fix: select specific rule from module will raise "rule not defined" that cause #3751 failed#3753
fix: select specific rule from module will raise "rule not defined" that cause #3751 failed#3753johanneskoester merged 4 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughAdded a new test rule Changes
Sequence Diagram(s)sequenceDiagram
participant Decorator as rule decorator
participant Modifier as modifier / rule proxies
participant Workflow as Workflow.add_rule
participant RulesStore as workflow._rules
Note over Decorator,Modifier: Decorator builds final Rule object
Decorator->>Modifier: register RuleProxy(orig_name)
Decorator->>Workflow: add_rule(rule, allow_overwrite=false)
Workflow->>RulesStore: store Rule object under resolved final name
Workflow->>Workflow: set rule attributes (checkpoint, log, message, etc.)
Workflow->>Modifier: apply modifiers / finalize registration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_module_nested/module_deep.smk (1)
11-14: Clarify the intent of the actionless rule.This rule is intentionally actionless to reproduce #3751 via attribute reference. Add a brief comment so future maintainers don’t “fix” it and invalidate the test.
Apply this diff to document intent:
+ # Intentionally actionless: used only to reference its input via rules.ignored.input.rds. + # Reproduces #3751 (unwanted-rule filtering should not remove transitively referenced rules). rule ignored: input: rds=rules.run.output.txt,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_module_nested/module_deep.smk(1 hunks)
🔇 Additional comments (1)
tests/test_module_nested/module_deep.smk (1)
17-19: LGTM: Correctly models the transitive reference scenario.Switching run2’s input to rules.ignored.input.rds accurately reproduces the filtering bug and ensures run still needs to execute.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/snakemake/common/__init__.py(2 hunks)src/snakemake/workflow.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
src/snakemake/workflow.pysrc/snakemake/common/__init__.py
🧠 Learnings (1)
📚 Learning: 2025-09-17T04:03:59.943Z
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/modules.py:236-241
Timestamp: 2025-09-17T04:03:59.943Z
Learning: In Snakemake's WorkflowModifier, rule filtering via rule_whitelist and rule_exclude_list happens upstream during rule creation in the rule() decorator. The avail_rulename() method returns None for filtered-out rules, causing the entire rule creation to be skipped. This means child_modifier.rule_proxies only contains rules that passed the filtering, so inherit_rule_proxies doesn't need additional filtering logic.
Applied to files:
src/snakemake/workflow.py
🧬 Code graph analysis (1)
src/snakemake/workflow.py (5)
src/snakemake/rules.py (6)
Rule(76-1347)name(131-132)name(135-136)lineno(139-140)snakefile(143-144)RuleProxy(1392-1459)src/snakemake/ruleinfo.py (2)
RuleInfo(14-111)apply_modifier(62-111)src/snakemake/modules.py (1)
avail_rulename(242-252)src/snakemake/common/__init__.py (1)
_register_rule(260-261)src/snakemake/checkpoints.py (1)
register(44-48)
🪛 Ruff (0.13.1)
src/snakemake/workflow.py
650-654: 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). (16)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (5, windows-2022, py313)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (3, windows-2022, py313)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: apidocs
🔇 Additional comments (8)
src/snakemake/common/__init__.py (1)
143-145: LGTM on fallback Uri handling.The local namedtuple avoids any confusion with the imported symbol. Behavior unchanged.
src/snakemake/workflow.py (7)
649-655: LGTM on overwrite guard and error context.Exception uses the Rule’s metadata; good for diagnostics.
658-659: LGTM: default_target uses actual rule name.Sets a sensible default on first rule.
1788-1789: LGTM: capture module_globals for later attachment.Keeps rule.module_globals consistent with the active modifier context.
1792-1804: Rule creation and proxy registration order looks sound.
- Resolve final name via modifier.avail_rulename.
- Instantiate Rule with name or orig_name.
- Register RuleProxy under orig_name before applying modifier.
- Apply modifier to ruleinfo using the computed rule name.
This should preserve proxy visibility even when filtered.
Please confirm that this ordering fixes the reproduction in #3751 by keeping proxies for rules filtered out by a module, while not adding them to workflow._rules.
1823-1827: LGTM: only add/register checkpoints for non-filtered rules.Ensures filtered rules aren’t scheduled while still allowing earlier proxy registration.
1828-1830: LGTM: attach module_globals post‑add.Assignment after add_rule aligns the Rule with the correct globals for later evaluation.
643-646: add_rule now takes a Rule object — internal calls OK; verify external callersRepo scan: only call site is src/snakemake/workflow.py:1823 and it already passes a Rule instance; no other .add_rule / add_rule(...) usages found. Because this is a public API change, verify third‑party callers/plugins and update docs/changelog if needed.
src/snakemake/common/__init__.py
Outdated
| def __hasattr__(self, name): | ||
| return name in self._rules | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
hasattr won’t be used by hasattr(); provide a real existence check.
Python doesn’t recognize hasattr; hasattr() only treats AttributeError specially and won’t call this. Given getattr raises WorkflowError, hasattr(rules, "...") will raise instead of returning False.
Replace with explicit methods and/or containment:
- Add has(name) and contains.
- Optionally keep getattr as-is for error messaging.
Apply:
- def __hasattr__(self, name):
- return name in self._rules
+ def has(self, name):
+ return name in self._rules
+
+ def __contains__(self, name):
+ return name in self._rulesTo verify no code is relying on __hasattr and to assess any hasattr(...) usage on Rules/RuleProxy containers:
🏁 Script executed:
#!/bin/bash
# Find any (mis)uses of __hasattr and hasattr with rules/rule_proxies.
rg -nP '__hasattr\s*\(' -S
rg -nP '\bhasattr\s*\(\s*(?:rules|self\.modifier\.rule_proxies)\s*,\s*' -S -C2Length of output: 0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Showing src/snakemake/common/__init__.py (full) ==="
sed -n '1,400p' src/snakemake/common/__init__.py || true
echo "=== End file dump ==="
echo
echo "=== Search for usages of hasattr / __hasattr / rule_proxies / RuleProxy / Rules ==="
rg -n --hidden --no-ignore -S '\bhasattr\s*\(' || true
rg -n --hidden --no-ignore -S '__hasattr' || true
rg -n --hidden --no-ignore -S 'rule_proxies' -C2 || true
rg -n --hidden --no-ignore -S '\bRuleProxy\b' -C2 || true
rg -n --hidden --no-ignore -S '\bclass\s+Rules\b' -n -S || true
rg -n --hidden --no-ignore -S '\bclass\s+\w*Rule\w*\b' -C2 -S || trueLength of output: 27328
hasattr won’t be used by hasattr(); provide a real existence check.
hasattr() only treats AttributeError as "missing"; hasattr is ignored and Rules.getattr raises WorkflowError, so hasattr(rules, "x") can raise instead of returning False. Search shows uses of hasattr(rules, ...) in tests and modules — this must be fixed.
Apply:
- def __hasattr__(self, name):
- return name in self._rules
+ def has(self, name):
+ return name in self._rules
+
+ def __contains__(self, name):
+ return name in self._rulesAlso either (A) change getattr to raise AttributeError for unknown names (so hasattr(...) behaves), or (B) update callers to use has()/in. Prefer adding has()/contains and making getattr raise AttributeError (or raise AttributeError with the WorkflowError message) so both hasattr(...) and direct attribute access behave sensibly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __hasattr__(self, name): | |
| return name in self._rules | |
| def has(self, name): | |
| return name in self._rules | |
| def __contains__(self, name): | |
| return name in self._rules |
🤖 Prompt for AI Agents
In src/snakemake/common/__init__.py around lines 274 to 276, the class defines
__hasattr__ which Python ignores so hasattr(rules, "x") can raise WorkflowError
via __getattr__; add a public existence checker (e.g., def has(self, name):
return name in self._rules) and implement __contains__ to delegate to that
checker, and change __getattr__ to raise AttributeError for unknown names
(optionally preserving the WorkflowError message text inside the AttributeError)
so both hasattr(...) and attribute access behave correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/snakemake/common/__init__.py (1)
268-271: Minor Ruff TRY003 (non‑blocking): long message outside exceptionStyle‑only; safe to ignore in this PR.
tests/test_module_nested/module_deep.smk (1)
34-35: Prefer rules.has(...) over hasattr(...) for clarityAvoid hasattr pitfalls with dynamic getattr. Using has() keeps behavior explicit.
Apply:
-assert hasattr(rules, "run"), f"bug: rule cannot be accessed as run: {dir(rules)}" +assert rules.has("run"), f"bug: rule cannot be accessed as run: {dir(rules)}"src/snakemake/workflow.py (1)
650-654: Minor Ruff TRY003 (non‑blocking)Long f‑string in CreateRuleException; style‑only.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/snakemake/common/__init__.py(2 hunks)src/snakemake/workflow.py(6 hunks)tests/test_module_nested/module_deep.smk(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 theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
src/snakemake/common/__init__.pysrc/snakemake/workflow.py
🧠 Learnings (1)
📚 Learning: 2025-09-17T04:03:59.943Z
Learnt from: Hocnonsense
PR: snakemake/snakemake#3714
File: src/snakemake/modules.py:236-241
Timestamp: 2025-09-17T04:03:59.943Z
Learning: In Snakemake's WorkflowModifier, rule filtering via rule_whitelist and rule_exclude_list happens upstream during rule creation in the rule() decorator. The avail_rulename() method returns None for filtered-out rules, causing the entire rule creation to be skipped. This means child_modifier.rule_proxies only contains rules that passed the filtering, so inherit_rule_proxies doesn't need additional filtering logic.
Applied to files:
src/snakemake/common/__init__.pysrc/snakemake/workflow.py
🧬 Code graph analysis (1)
src/snakemake/workflow.py (6)
src/snakemake/persistence.py (2)
rule(433-434)log(439-440)src/snakemake/rules.py (12)
Rule(76-1347)name(131-132)name(135-136)lineno(139-140)snakefile(143-144)RuleProxy(1392-1459)log(611-612)log(1429-1430)benchmark(236-237)benchmark(240-251)benchmark(1425-1426)add(1354-1358)src/snakemake/modules.py (1)
avail_rulename(242-252)src/snakemake/common/__init__.py (1)
_register_rule(260-261)src/snakemake/ruleinfo.py (1)
apply_modifier(62-111)src/snakemake/checkpoints.py (1)
register(44-48)
🪛 Ruff (0.13.1)
src/snakemake/common/__init__.py
268-271: Avoid specifying long messages outside the exception class
(TRY003)
src/snakemake/workflow.py
650-654: 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). (15)
- GitHub Check: tests (10, windows-2022, py313)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
🔇 Additional comments (8)
src/snakemake/common/__init__.py (2)
143-145: parse_uri fallback LGTMGraceful fallback to a namedtuple matches smart_open’s shape and avoids breakage.
266-274: Make hasattr/rules membership checks predictable; prefer AttributeError and add containshas() is a good addition. However, getattr raising WorkflowError breaks hasattr(...) semantics (should return False on missing). Recommend:
- Raise AttributeError in getattr for missing names.
- Implement contains so
"run" in rulesworks.Apply:
- def __getattr__(self, name): - from snakemake.exceptions import WorkflowError - - if name in self._rules: - return self._rules[name] - raise WorkflowError( - f"Rule {name} is not defined in this workflow. " - f"Available rules: {', '.join(self._rules)}" - ) + def __getattr__(self, name): + if name in self._rules: + return self._rules[name] + # AttributeError ensures hasattr(...) returns False instead of raising. + raise AttributeError( + f"Rule {name} is not defined in this workflow. " + f"Available rules: {', '.join(self._rules)}" + ) @@ def has(self, name): return name in self._rules + + def __contains__(self, name): + return name in self._rulestests/test_module_nested/module_deep.smk (1)
11-21: Solid minimal repro for #3751Introducing an intermediate rule and referencing it transitively via rules.* reproduces the filtering regression succinctly.
src/snakemake/workflow.py (5)
657-659: Default target assignment via rule.name LGTMKeeps existing behavior intact post‑refactor.
1791-1803: Registering RuleProxy under orig_name before filtering resolves transitive referencesThis preserves rules.* proxy access (e.g., rules.ignored.input.rds) even when the rule is excluded from the workflow, which addresses #3751.
Please confirm no “use rule …” logic relies on rule_proxies containing only allowed rules; filtering should still occur at avail_rulename/use_rules time.
1830-1833: Early return for filtered rules now returns the original functionThis fixes the decorator contract (previous lambda issue). Good catch.
2014-2042: Name‑based bookkeeping consistentUsing the final name for localrules/cache/default_target matches rule.name.
643-654: Approve: add_rule now takes Rule; callers verified
Only call site found: src/snakemake/workflow.py:1834 — passesrule; no otheradd_rule(...)usages detected.
🤖 I have created a release *beep* *boop* --- ## [9.11.6](v9.11.5...v9.11.6) (2025-09-25) ### Bug Fixes * select specific rule from module will raise "rule not defined" that cause [#3751](#3751) failed ([#3753](#3753)) ([d6fa3ca](d6fa3ca)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
…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 -->
🤖 I have created a release *beep* *boop* --- ## [9.11.6](snakemake/snakemake@v9.11.5...v9.11.6) (2025-09-25) ### Bug Fixes * select specific rule from module will raise "rule not defined" that cause [snakemake#3751](snakemake#3751) failed ([snakemake#3753](snakemake#3753)) ([d6fa3ca](snakemake@d6fa3ca)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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 rulestar_alignreferred in "v3.13.3/meta/bio/star_arriba/test/Snakefile", line 49":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
Tests
Chores
Developer-facing