feat: Add support for seqkit split2 #3575
Conversation
|
Warning Rate limit exceeded@fgvieira has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request introduces updates to the Changes
Sequence DiagramsequenceDiagram
participant Wrapper as SeqKit Wrapper
participant Snakefile as Snakemake Rule
participant Input as Input FASTA
participant Output as Split Output Files
Snakefile->>Wrapper: Invoke with split2 command
Wrapper->>Input: Read input file
Wrapper->>Wrapper: Determine splitting criteria
Wrapper->>Output: Generate split files
Wrapper-->>Snakefile: Return split results
This sequence diagram illustrates the high-level interaction between the Snakemake rule, SeqKit wrapper, input file, and output files during the sequence splitting process. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 1
🧹 Nitpick comments (1)
bio/seqkit/wrapper.py (1)
5-8: Remove the unused import to keep the code clean.Line 5 (
import tempfile) is never referenced in the code, so it should be removed to avoid confusion and satisfy static analysis.- import tempfile from pathlib import Path from snakemake.shell import shell from snakemake_wrapper_utils.snakemake import is_arg🧰 Tools
🪛 Ruff (0.8.2)
5-5:
tempfileimported but unusedRemove unused import:
tempfile(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bio/seqkit/environment.linux-64.pin.txt(1 hunks)bio/seqkit/environment.yaml(1 hunks)bio/seqkit/test/Snakefile(1 hunks)bio/seqkit/wrapper.py(4 hunks)test_wrappers.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
bio/seqkit/wrapper.py (2)
Pattern **/*.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.
Pattern **/wrapper.py: Do not complain about use of undefined variable called snakemake.
test_wrappers.py (1)
Pattern **/*.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.
🪛 Ruff (0.8.2)
bio/seqkit/wrapper.py
5-5: tempfile imported but unused
Remove unused import: tempfile
(F401)
11-11: Undefined name snakemake
(F821)
12-12: Undefined name snakemake
(F821)
16-16: Undefined name snakemake
(F821)
17-17: Undefined name snakemake
(F821)
19-19: Undefined name snakemake
(F821)
20-20: Undefined name snakemake
(F821)
22-22: Undefined name snakemake
(F821)
51-51: Undefined name snakemake
(F821)
64-64: Undefined name snakemake
(F821)
67-67: Undefined name snakemake
(F821)
68-68: Undefined name snakemake
(F821)
73-73: Undefined name snakemake
(F821)
73-73: Undefined name snakemake
(F821)
75-75: Undefined name snakemake
(F821)
92-92: Undefined name snakemake
(F821)
93-93: Loop control variable idx not used within loop body
Rename unused idx to _idx
(B007)
93-93: Loop control variable output_file not used within loop body
Rename unused output_file to _output_file
(B007)
93-93: Undefined name snakemake
(F821)
🔇 Additional comments (12)
bio/seqkit/wrapper.py (9)
15-17: Looks good for handling multiple input files.No issues spotted. The logic is concise and clear.
🧰 Tools
🪛 Ruff (0.8.2)
16-16: Undefined name
snakemake(F821)
17-17: Undefined name
snakemake(F821)
19-20: Single-file input logic is correct.Clever approach to handle potentially multiple inputs by concatenation.
🧰 Tools
🪛 Ruff (0.8.2)
19-19: Undefined name
snakemake(F821)
20-20: Undefined name
snakemake(F821)
Line range hint
25-36: Potential edge case with slicing input items.
[1:]usage may cause an empty list if there's only one item insnakemake.input. If the wrapper expects at least two items, this is fine. Otherwise, consider guarding against an IndexError or verifying item count.🧰 Tools
🪛 Ruff (0.8.2)
5-5:
tempfileimported but unusedRemove unused import:
tempfile(F401)
11-11: Undefined name
snakemake(F821)
12-12: Undefined name
snakemake(F821)
16-16: Undefined name
snakemake(F821)
17-17: Undefined name
snakemake(F821)
19-19: Undefined name
snakemake(F821)
20-20: Undefined name
snakemake(F821)
22-22: Undefined name
snakemake(F821)
Line range hint
38-49: Similar slicing concern for output items.Here
[1:]is used again. Make sure there's more than one item insnakemake.outputto avoid unexpected behavior.
51-63: Split logic is well structured.The differentiation by ID, part, region, size, or length is thorough. This is a neat approach for various splitting modes.
🧰 Tools
🪛 Ruff (0.8.2)
51-51: Undefined name
snakemake(F821)
64-65: Output directory handling.Storing split files in
snakemake.output[0]'s parent directory is a solid approach.🧰 Tools
🪛 Ruff (0.8.2)
64-64: Undefined name
snakemake(F821)
67-76: Conditional output compression is well-implemented.Verb-piping through bgzip is a standard approach. The assertion for valid file extensions makes sense.
🧰 Tools
🪛 Ruff (0.8.2)
67-67: Undefined name
snakemake(F821)
68-68: Undefined name
snakemake(F821)
73-73: Undefined name
snakemake(F821)
73-73: Undefined name
snakemake(F821)
75-75: Undefined name
snakemake(F821)
Line range hint
77-86: Shell command invocation looks good.You’ve built the command thoughtfully with placeholders for input, output, threads, and extras.
🧰 Tools
🪛 Ruff (0.8.2)
51-51: Undefined name
snakemake(F821)
64-64: Undefined name
snakemake(F821)
67-67: Undefined name
snakemake(F821)
68-68: Undefined name
snakemake(F821)
73-73: Undefined name
snakemake(F821)
73-73: Undefined name
snakemake(F821)
75-75: Undefined name
snakemake(F821)
92-94: False positives from static analysis.
idxandoutput_fileare indeed used in the shell command. No need to rename them.🧰 Tools
🪛 Ruff (0.8.2)
92-92: Undefined name
snakemake(F821)
93-93: Loop control variable
idxnot used within loop bodyRename unused
idxto_idx(B007)
93-93: Loop control variable
output_filenot used within loop bodyRename unused
output_fileto_output_file(B007)
93-93: Undefined name
snakemake(F821)
test_wrappers.py (1)
464-477: Tests align with the new rule; watch extension vs. bgzip.The test function is correct. However, if
out_bgzip=Trueremains set in the rule, the.fasextension could fail. Ensure your output extension in both the Snakefile and test is consistent with the bgzip logic.bio/seqkit/environment.yaml (1)
8-8: New dependency addition looks good.Including
snakemake-wrapper-utils=0.6.2is appropriate for advanced wrapper features.bio/seqkit/environment.linux-64.pin.txt (1)
4-43: Pinned versions updated correctly.All updated packages and URLs appear consistent. No issues noted with the pinned references.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bio/seqkit/wrapper.py (1)
72-83: Consider making file extension configurable.The extension
.fasis hardcoded in the output configuration. Consider making this configurable through Snakemake parameters to support other sequence file formats (e.g., .fasta, .fa).- output = f"--out-dir {out_dir} --by-{split_by}-prefix output_part. --extension .fas" + extension = snakemake.params.get("extension", ".fas") + output = f"--out-dir {out_dir} --by-{split_by}-prefix output_part. --extension {extension}"🧰 Tools
🪛 Ruff (0.8.2)
72-72: Undefined name
snakemake(F821)
75-75: Undefined name
snakemake(F821)
76-76: Undefined name
snakemake(F821)
81-81: Undefined name
snakemake(F821)
81-81: Undefined name
snakemake(F821)
83-83: Undefined name
snakemake(F821)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bio/seqkit/test/Snakefile(1 hunks)bio/seqkit/wrapper.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bio/seqkit/test/Snakefile
🧰 Additional context used
📓 Path-based instructions (1)
bio/seqkit/wrapper.py (2)
Pattern **/*.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.
Pattern **/wrapper.py: Do not complain about use of undefined variable called snakemake.
🪛 Ruff (0.8.2)
bio/seqkit/wrapper.py
5-5: tempfile imported but unused
Remove unused import: tempfile
(F401)
11-11: Undefined name snakemake
(F821)
12-12: Undefined name snakemake
(F821)
16-16: Undefined name snakemake
(F821)
17-17: Undefined name snakemake
(F821)
19-19: Undefined name snakemake
(F821)
28-28: Undefined name snakemake
(F821)
30-30: Undefined name snakemake
(F821)
59-59: Undefined name snakemake
(F821)
72-72: Undefined name snakemake
(F821)
75-75: Undefined name snakemake
(F821)
76-76: Undefined name snakemake
(F821)
81-81: Undefined name snakemake
(F821)
81-81: Undefined name snakemake
(F821)
83-83: Undefined name snakemake
(F821)
99-99: Undefined name snakemake
(F821)
100-100: Loop control variable idx not used within loop body
Rename unused idx to _idx
(B007)
100-100: Loop control variable output_file not used within loop body
Rename unused output_file to _output_file
(B007)
100-100: Undefined name snakemake
(F821)
🔇 Additional comments (4)
bio/seqkit/wrapper.py (4)
5-5: Verify temp file handling implementation.The
tempfilemodule is imported but not used in the code. According to the PR objectives, temporary files should be managed usingtempfile.gettempdir()or a hidden folder.Would you like me to help implement proper temporary file handling for this wrapper?
🧰 Tools
🪛 Ruff (0.8.2)
5-5:
tempfileimported but unusedRemove unused import:
tempfile(F401)
59-71: LGTM! Comprehensive split type detection.The implementation correctly handles all SeqKit split options (id, part, region, size, length) through command-line argument detection.
🧰 Tools
🪛 Ruff (0.8.2)
59-59: Undefined name
snakemake(F821)
Line range hint
87-95: LGTM! Shell command follows best practices.The shell command construction properly includes all necessary components and follows Snakemake wrapper best practices.
🧰 Tools
🪛 Ruff (0.8.2)
99-99: Undefined name
snakemake(F821)
100-100: Loop control variable
idxnot used within loop bodyRename unused
idxto_idx(B007)
100-100: Loop control variable
output_filenot used within loop bodyRename unused
output_fileto_output_file(B007)
100-100: Undefined name
snakemake(F821)
98-101: LGTM! Output file renaming logic is correct.The implementation correctly renames the split output files to match Snakemake's output specifications. Note: The static analysis warning about unused loop variables (
idxandoutput_file) is a false positive as they are used within the shell command's f-string.🧰 Tools
🪛 Ruff (0.8.2)
99-99: Undefined name
snakemake(F821)
100-100: Loop control variable
idxnot used within loop bodyRename unused
idxto_idx(B007)
100-100: Loop control variable
output_filenot used within loop bodyRename unused
output_fileto_output_file(B007)
100-100: Undefined name
snakemake(F821)
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
New Features
snakemake-wrapper-utilspackage to enhance workflow capabilities.Dependency Updates
Testing