feat: add temp dir and code refactor #4319
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 Walkthrough""" WalkthroughThe changes refactor the mosdepth Snakemake wrapper and its associated test suite. The wrapper script now infers parameters and output files dynamically, executes mosdepth in a temporary directory, and moves outputs post-run. Test coverage is consolidated by removing redundant test cases, and some test and configuration files are simplified. Changes
Sequence Diagram(s)sequenceDiagram
participant Snakemake
participant Wrapper
participant Mosdepth
participant TempDir
Snakemake->>Wrapper: Invoke with inputs/outputs/params
Wrapper->>TempDir: Create temporary directory
Wrapper->>Mosdepth: Build command with inferred params, run in TempDir
Mosdepth-->>TempDir: Write output files (e.g., .bam, .bed, .txt)
Wrapper->>TempDir: Move output files to Snakemake-specified locations
Wrapper-->>Snakemake: Return control after outputs are in place
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for...
⚙️ Source: CodeRabbit Configuration File List of files the instruction was applied to:
`**/wrapper.py`: Do not complain about use of undefined variable called `snakemake`.
⚙️ Source: CodeRabbit Configuration File List of files the instruction was applied to:
🧠 Learnings (2)📓 Common learningsbio/mosdepth/wrapper.py (19)🪛 Ruff (0.11.9)bio/mosdepth/wrapper.py11-11: Undefined name (F821) 12-12: Undefined name (F821) 15-15: Undefined name (F821) 19-19: Undefined name (F821) 20-20: Undefined name (F821) 23-23: Undefined name (F821) 24-24: Undefined name (F821) 27-27: Undefined name (F821) 27-27: Undefined name (F821) 28-28: Undefined name (F821) 28-28: Undefined name (F821) 44-44: Undefined name (F821) 44-44: Undefined name (F821) 52-52: Undefined name (F821) 57-57: Undefined name (F821) 58-58: Undefined name (F821) 63-63: Undefined name (F821) 64-64: Undefined name (F821) 69-69: Undefined name (F821) 70-70: Undefined name (F821) 75-75: Undefined name (F821) 80-80: Undefined name (F821) 85-85: Undefined name (F821) 86-86: Undefined name (F821) ⏰ 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). (3)
🔇 Additional comments (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bio/mosdepth/wrapper.py (1)
71-97: File movement logic is clear and complete!The explicit handling of each output type with verbose logging provides good visibility into the wrapper's operations.
Consider extracting the file movement pattern into a helper function to reduce repetition:
+def move_if_exists(src_pattern, dest_var, log): + if dest_var: + shell(f"mv --verbose {src_pattern} {dest_var} {log}") + with tempfile.TemporaryDirectory() as tmpdir: shell( "{precision} mosdepth {threads} {fasta} {extra} {tmpdir}/temp {snakemake.input.bam} {log}" ) - if snakemake.output.get("summary"): - shell( - "mv --verbose {tmpdir}/temp.mosdepth.summary.txt {snakemake.output.summary} {log}" - ) + move_if_exists(f"{tmpdir}/temp.mosdepth.summary.txt", snakemake.output.get("summary"), log)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
bio/mosdepth/test/Snakefile(5 hunks)bio/mosdepth/test/test.bed(1 hunks)bio/mosdepth/wrapper.py(2 hunks)test_wrappers.py(0 hunks)
💤 Files with no reviewable changes (1)
- test_wrappers.py
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for...
**/*.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.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
bio/mosdepth/wrapper.py
`**/wrapper.py`: Do not complain about use of undefined variable called `snakemake`.
**/wrapper.py: Do not complain about use of undefined variable calledsnakemake.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
bio/mosdepth/wrapper.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: johanneskoester
PR: snakemake/snakemake-wrappers#3478
File: bio/varlociraptor/estimate-alignment-properties/test/Snakefile:7-10
Timestamp: 2024-11-15T18:31:15.447Z
Learning: In the Snakemake wrappers repository, avoid suggesting refactoring that involves using `tempfile.gettempdir()` or changing output paths to temporary directories.
Learnt from: johanneskoester
PR: snakemake/snakemake-wrappers#3478
File: bio/varlociraptor/estimate-alignment-properties/wrapper.py:5-12
Timestamp: 2024-11-21T10:23:03.427Z
Learning: In the Snakemake wrappers project, avoid suggesting extensive error handling or temporary file management in simple wrapper scripts when it may be unnecessary, to prevent overcomplicating the code.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3501
File: meta/bio/varscan2_snpeff/test/Snakefile:58-71
Timestamp: 2024-11-26T10:49:54.765Z
Learning: In test Snakefiles within the snakemake-wrappers repository, it is acceptable to use simplified paths and logging configurations that may differ from real-life pipelines.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3499
File: bio/ngscheckmate/makesnvpattern/test/Snakefile:14-22
Timestamp: 2024-11-26T09:16:39.570Z
Learning: In the `snakemake-wrappers` repository, when writing test `Snakefile`s (e.g., `bio/ngscheckmate/makesnvpattern/test/Snakefile`), hardcoded input/output paths are acceptable because these are examples and the IO can be chosen freely.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3496
File: bio/mtnucratio/test/Snakefile:2-6
Timestamp: 2024-11-26T08:31:00.099Z
Learning: In test files for Snakemake wrappers, such as `bio/mtnucratio/test/Snakefile`, hard-coded input and output paths are acceptable as examples and do not need to use wildcards to make paths flexible.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3502
File: bio/ngsbits/sampleancestry/wrapper.py:18-23
Timestamp: 2024-11-26T14:59:03.678Z
Learning: In Snakemake wrapper scripts, Snakemake validates input and output paths, so explicit shell quoting is not necessary.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3497
File: bio/sexdeterrmine/wrapper.py:23-26
Timestamp: 2024-11-26T08:35:42.140Z
Learning: In the `bio/sexdeterrmine/wrapper.py` file (Python), we rely on Samtools to handle input validation for the depth file provided by the user, so additional file existence checks are not necessary.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3496
File: bio/mtnucratio/wrapper.py:27-28
Timestamp: 2024-11-26T08:30:23.818Z
Learning: In Snakemake wrappers (e.g., `wrapper.py` files), it's unnecessary to verify the availability of tools like `mtnucratio` within the code, because Snakemake with Conda ensures that the required tools are installed and available.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3499
File: bio/ngscheckmate/makesnvpattern/test/Snakefile:1-13
Timestamp: 2024-11-26T09:16:24.981Z
Learning: In test `Snakefile`s (e.g., `test/Snakefile`), it's acceptable to use fixed input and output file names instead of wildcards.
Learnt from: johanneskoester
PR: snakemake/snakemake-wrappers#3123
File: utils/datavzrd/wrapper.py:31-32
Timestamp: 2024-10-08T17:41:54.542Z
Learning: In `wrapper.py` scripts, do not flag the use of an undefined variable called `snakemake`.
bio/mosdepth/test/Snakefile (10)
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3501
File: meta/bio/varscan2_snpeff/test/Snakefile:58-71
Timestamp: 2024-11-26T10:49:54.765Z
Learning: In test Snakefiles within the snakemake-wrappers repository, it is acceptable to use simplified paths and logging configurations that may differ from real-life pipelines.
Learnt from: johanneskoester
PR: snakemake/snakemake-wrappers#3302
File: bio/vg/giraffe/test/Snakefile:26-29
Timestamp: 2024-11-22T07:35:06.812Z
Learning: For the `vg_giraffe_map` rule in `bio/vg/giraffe/test/Snakefile`, adding explicit parameter validation functions like `validate_sort_params` is considered unnecessary and may be overkill.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3499
File: bio/ngscheckmate/makesnvpattern/test/Snakefile:1-13
Timestamp: 2024-11-26T09:16:24.981Z
Learning: In test `Snakefile`s (e.g., `test/Snakefile`), it's acceptable to use fixed input and output file names instead of wildcards.
Learnt from: johanneskoester
PR: snakemake/snakemake-wrappers#3478
File: bio/varlociraptor/estimate-alignment-properties/wrapper.py:5-12
Timestamp: 2024-11-21T10:23:03.427Z
Learning: In the Snakemake wrappers project, avoid suggesting extensive error handling or temporary file management in simple wrapper scripts when it may be unnecessary, to prevent overcomplicating the code.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3499
File: bio/ngscheckmate/makesnvpattern/test/Snakefile:14-22
Timestamp: 2024-11-26T09:16:39.570Z
Learning: In the `snakemake-wrappers` repository, when writing test `Snakefile`s (e.g., `bio/ngscheckmate/makesnvpattern/test/Snakefile`), hardcoded input/output paths are acceptable because these are examples and the IO can be chosen freely.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3496
File: bio/mtnucratio/test/Snakefile:2-6
Timestamp: 2024-11-26T08:31:00.099Z
Learning: In test files for Snakemake wrappers, such as `bio/mtnucratio/test/Snakefile`, hard-coded input and output paths are acceptable as examples and do not need to use wildcards to make paths flexible.
Learnt from: dlaehnemann
PR: snakemake/snakemake-wrappers#3115
File: CHANGELOG.md:5-5
Timestamp: 2024-10-08T17:41:54.542Z
Learning: Do not review release-please commits in the Snakemake wrappers repository as they are auto-formatted.
Learnt from: dlaehnemann
PR: snakemake/snakemake-wrappers#3115
File: CHANGELOG.md:5-5
Timestamp: 2024-08-14T15:21:37.230Z
Learning: Do not review release-please commits in the Snakemake wrappers repository as they are auto-formatted.
Learnt from: johanneskoester
PR: snakemake/snakemake-wrappers#3478
File: bio/varlociraptor/call-variants/test/Snakefile:29-47
Timestamp: 2024-11-21T10:50:09.006Z
Learning: In example Snakefiles, using hardcoded sample names is acceptable.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3497
File: bio/sexdeterrmine/wrapper.py:23-26
Timestamp: 2024-11-26T08:35:42.140Z
Learning: In the `bio/sexdeterrmine/wrapper.py` file (Python), we rely on Samtools to handle input validation for the depth file provided by the user, so additional file existence checks are not necessary.
bio/mosdepth/wrapper.py (18)
Learnt from: johanneskoester
PR: snakemake/snakemake-wrappers#3478
File: bio/varlociraptor/estimate-alignment-properties/wrapper.py:5-12
Timestamp: 2024-11-21T10:23:03.427Z
Learning: In the Snakemake wrappers project, avoid suggesting extensive error handling or temporary file management in simple wrapper scripts when it may be unnecessary, to prevent overcomplicating the code.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3497
File: bio/sexdeterrmine/wrapper.py:23-26
Timestamp: 2024-11-26T08:35:42.140Z
Learning: In the `bio/sexdeterrmine/wrapper.py` file (Python), we rely on Samtools to handle input validation for the depth file provided by the user, so additional file existence checks are not necessary.
Learnt from: johanneskoester
PR: snakemake/snakemake-wrappers#3478
File: bio/varlociraptor/estimate-alignment-properties/test/Snakefile:7-10
Timestamp: 2024-11-15T18:31:15.447Z
Learning: In the Snakemake wrappers repository, avoid suggesting refactoring that involves using `tempfile.gettempdir()` or changing output paths to temporary directories.
Learnt from: rohan-ibn-tariq
PR: snakemake/snakemake-wrappers#4160
File: bio/trf/wrapper.py:18-26
Timestamp: 2025-06-04T06:32:20.090Z
Learning: For Snakemake wrappers, it's preferable to keep parameter dictionaries and constants directly visible in wrapper.py files rather than importing from config modules, to maintain minimal, self-documenting code that doc viewers can understand at a glance.
Learnt from: johanneskoester
PR: snakemake/snakemake-wrappers#3123
File: utils/datavzrd/wrapper.py:31-32
Timestamp: 2024-10-08T17:41:54.542Z
Learning: In `wrapper.py` scripts, do not flag the use of an undefined variable called `snakemake`.
Learnt from: johanneskoester
PR: snakemake/snakemake-wrappers#3123
File: utils/datavzrd/wrapper.py:31-32
Timestamp: 2024-08-21T08:30:42.757Z
Learning: In `wrapper.py` scripts, do not flag the use of an undefined variable called `snakemake`.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3502
File: bio/ngsbits/sampleancestry/wrapper.py:18-23
Timestamp: 2024-11-26T14:59:03.678Z
Learning: In Snakemake wrapper scripts, Snakemake validates input and output paths, so explicit shell quoting is not necessary.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3497
File: bio/sexdeterrmine/wrapper.py:0-0
Timestamp: 2024-11-26T08:33:32.457Z
Learning: In the `bio/sexdeterrmine/wrapper.py` file for the Sex.DetERRmine wrapper, missing input/output errors are raised by the shell command and captured in the logs. Therefore, additional error handling for file operations is not necessary.
Learnt from: johanneskoester
PR: snakemake/snakemake-wrappers#3498
File: bio/ngscheckmate/ncm/wrapper.py:68-68
Timestamp: 2024-12-06T14:25:43.922Z
Learning: In the `bio/ngscheckmate/ncm/wrapper.py` file for the NGSCheckMate wrapper, do not suggest adding file existence checks for the list file input, as the files are already required by the wrapper.
Learnt from: johanneskoester
PR: snakemake/snakemake-wrappers#3478
File: bio/varlociraptor/preprocess-variants/wrapper.py:0-0
Timestamp: 2024-11-15T13:48:33.759Z
Learning: In Snakemake wrappers, security considerations like input sanitization are unnecessary, as the wrappers are under full control of the user.
Learnt from: dlaehnemann
PR: snakemake/snakemake-wrappers#3432
File: bio/reference/ensembl-sequence/wrapper.py:79-87
Timestamp: 2025-01-30T14:19:53.384Z
Learning: In Snakemake wrappers, error reporting should use `print(..., file=sys.stderr)` for direct error messages, while `snakemake.log_fmt_shell()` is used for capturing command output and errors.
Learnt from: johanneskoester
PR: snakemake/snakemake-wrappers#3478
File: bio/varlociraptor/call-variants/wrapper.py:15-23
Timestamp: 2024-11-15T18:36:04.660Z
Learning: In the Snakemake wrappers repository, using `shell=True` and redirecting within shell commands is acceptable.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3501
File: meta/bio/varscan2_snpeff/test/Snakefile:58-71
Timestamp: 2024-11-26T10:49:54.765Z
Learning: In test Snakefiles within the snakemake-wrappers repository, it is acceptable to use simplified paths and logging configurations that may differ from real-life pipelines.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3499
File: bio/ngscheckmate/makesnvpattern/test/Snakefile:14-22
Timestamp: 2024-11-26T09:16:39.570Z
Learning: In the `snakemake-wrappers` repository, when writing test `Snakefile`s (e.g., `bio/ngscheckmate/makesnvpattern/test/Snakefile`), hardcoded input/output paths are acceptable because these are examples and the IO can be chosen freely.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3496
File: bio/mtnucratio/test/Snakefile:2-6
Timestamp: 2024-11-26T08:31:00.099Z
Learning: In test files for Snakemake wrappers, such as `bio/mtnucratio/test/Snakefile`, hard-coded input and output paths are acceptable as examples and do not need to use wildcards to make paths flexible.
Learnt from: tedil
PR: snakemake/snakemake-wrappers#4274
File: bio/mehari/annotate-seqvars/wrapper.py:11-19
Timestamp: 2025-06-15T07:43:03.263Z
Learning: In Snakemake wrappers, a common pattern is to have flag variables that are either empty strings "" or the actual flag strings (e.g., "--keep-intergenic") for direct interpolation in shell commands. This avoids conditionals in the shell expression and keeps the command construction clean.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3728
File: bio/bwameth/memx/wrapper.py:32-32
Timestamp: 2025-05-15T07:35:14.369Z
Learning: In the bwameth wrapper, when `snakemake.threads` equals 1, `samtools_threads` is intentionally set to 0. This design allows the wrapper to skip adding the `--threads` parameter to samtools, prioritizing the single available thread for the primary alignment task.
Learnt from: tdayris
PR: snakemake/snakemake-wrappers#3501
File: meta/bio/varscan2_snpeff/test/Snakefile:1-10
Timestamp: 2024-11-26T10:49:04.406Z
Learning: Using generic output filenames like "genome.fasta" is acceptable in the `snakemake-wrappers` project.
🪛 Ruff (0.11.9)
bio/mosdepth/wrapper.py
11-11: Undefined name snakemake
(F821)
12-12: Undefined name snakemake
(F821)
19-19: Undefined name snakemake
(F821)
39-39: Undefined name snakemake
(F821)
43-43: Undefined name snakemake
(F821)
47-47: Undefined name snakemake
(F821)
47-47: Undefined name snakemake
(F821)
63-63: Undefined name snakemake
(F821)
63-63: Undefined name snakemake
(F821)
71-71: Undefined name snakemake
(F821)
⏰ 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). (3)
- GitHub Check: docs
- GitHub Check: testing
- GitHub Check: Summary
🔇 Additional comments (6)
bio/mosdepth/test/test.bed (1)
1-1: LGTM!Valid BED format change for test data.
bio/mosdepth/test/Snakefile (1)
1-93: Test configuration cleanup looks good!The removal of inline comments and simplification of parameters aligns well with the wrapper refactoring that now handles parameter detection dynamically.
bio/mosdepth/wrapper.py (4)
1-13: Good use of tempfile and logging configuration!The addition of
tempfilefor temporary directory management andappend=Truefor logging are appropriate for the refactored multi-step execution approach.
15-32: Clean output file detection implementation!The suffix-based detection approach is clear and stores the actual file paths for later use in file movement operations.
34-64: Elegant parameter inference from outputs!The conditional parameter building based on detected outputs simplifies usage and ensures consistency between requested outputs and command parameters.
66-70: Robust temporary directory handling!Using a context manager ensures proper cleanup, and the fixed "temp" prefix simplifies subsequent file operations.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
QC
snakemake-wrappers.While the contributions guidelines are more extensive, please particularly ensure that:
test.pywas updated to call any added or updated example rules in aSnakefileinput:andoutput:file paths in the rules can be chosen arbitrarilyinput:oroutput:)tempfile.gettempdir()points tometa.yamlcontains a link to the documentation of the respective tool or command underurl:Summary by CodeRabbit
Refactor
Style
Tests
Chores