feat: support per-group scenario templates#435
feat: support per-group scenario templates#435johanneskoester wants to merge 6 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughGroup-aware scenario selection was added: workflows now look up a per-group Changes
Sequence Diagram(s)sequenceDiagram
participant Rule as Snakemake rule render_scenario
participant Groups as group_annotation (e.g., config/groups.tsv)
participant Config as config (global scenario)
participant Renderer as Template renderer
Rule->>Groups: lookup(query="group == '{group}'", cols="scenario")
alt per-group scenario found
Groups-->>Rule: return per-group scenario path
Rule->>Renderer: render using per-group template
else no per-group scenario
Rule->>Config: read config["calling"]["scenario"]
Config-->>Rule: return global scenario path
Rule->>Renderer: render using global template
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Super-linter summary
All files and directories linted successfully For more information, see the GitHub Actions workflow run Powered by Super-linter |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workflow/rules/common.smk`:
- Around line 91-92: The current logic only sets group_annotation["scenario"]
from lookup("calling/scenario", within=config) when the entire "scenario" column
is missing; instead, ensure that for rows where group_annotation["scenario"] is
empty/NA you replace those individual entries with the lookup default. Update
the handling around group_annotation and the "scenario" column so that existing
non-empty overrides are preserved but empty/NA cells are filled from
lookup("calling/scenario", within=config) (e.g., perform a per-row fill/where
operation on group_annotation["scenario"] using the lookup value).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8373db8d-06bf-47a0-897b-1da6b9c010f0
📒 Files selected for processing (3)
config/README.mdworkflow/rules/calling.smkworkflow/rules/common.smk
Super-linter summary
All files and directories linted successfully For more information, see the GitHub Actions workflow run Powered by Super-linter |
Super-linter summary
All files and directories linted successfully For more information, see the GitHub Actions workflow run Powered by Super-linter |
…lows/dna-seq-varlociraptor into feat/per-group-scenarios
Super-linter summary
All files and directories linted successfully For more information, see the GitHub Actions workflow run Powered by Super-linter |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
workflow/rules/common.smk (1)
91-94:⚠️ Potential issue | 🔴 Critical
default_scenariois undefined and will crash at runtime.At Line 94,
default_scenariois used but never defined in this file, so workflows with ascenariocolumn ingroups.tsvwill fail withNameError.🐛 Suggested fix
+default_scenario = lookup("calling/scenario", within=config) if "scenario" not in group_annotation.columns: - group_annotation["scenario"] = lookup("calling/scenario", within=config) + group_annotation["scenario"] = default_scenario else: - group_annotation.loc[:, "scenario"].fillna(default_scenario, inplace=True) + group_annotation["scenario"] = ( + group_annotation["scenario"].replace("", pd.NA).fillna(default_scenario) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflow/rules/common.smk` around lines 91 - 94, The code uses default_scenario in group_annotation.loc[:, "scenario"].fillna(default_scenario, inplace=True) but default_scenario is never defined; define it by calling the same lookup used above (e.g. default_scenario = lookup("calling/scenario", within=config)) before the if/else or replace the fillna argument with lookup("calling/scenario", within=config) so group_annotation.fillna has a valid value; reference symbols: group_annotation, default_scenario, lookup("calling/scenario", within=config).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@workflow/rules/common.smk`:
- Around line 91-94: The code uses default_scenario in group_annotation.loc[:,
"scenario"].fillna(default_scenario, inplace=True) but default_scenario is never
defined; define it by calling the same lookup used above (e.g. default_scenario
= lookup("calling/scenario", within=config)) before the if/else or replace the
fillna argument with lookup("calling/scenario", within=config) so
group_annotation.fillna has a valid value; reference symbols: group_annotation,
default_scenario, lookup("calling/scenario", within=config).
1 participant
Summary by CodeRabbit
New Features
Documentation
Bug Fixes