fix: Fastq screen missing PNG #4284
Conversation
Co-authored-by: Filipe G. Vieira <[email protected]>
📝 Walkthrough""" WalkthroughThe updates enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant Snakemake
participant Wrapper (wrapper.py)
participant fastq_screen
participant TempDir
Snakemake->>Wrapper (wrapper.py): Run with inputs, outputs, params (possibly config dict)
Wrapper (wrapper.py)->>TempDir: Create temporary directory (context manager)
alt Config is dict
Wrapper (wrapper.py)->>TempDir: Write config file
else Config is file path
Wrapper (wrapper.py)->>TempDir: Use config file directly
end
Wrapper (wrapper.py)->>fastq_screen: Call fastq_screen with appropriate config and outputs
fastq_screen-->>Wrapper (wrapper.py): Generate outputs (txt, png, optional conf)
Wrapper (wrapper.py)->>TempDir: Move outputs to final locations (conditional on snakemake.output)
TempDir-->>Wrapper (wrapper.py): Cleanup temporary directory (automatic)
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)`**/*.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 `s...
`**/wrapper.py`: Do not complain about use of undefined variable called `snakemake`.
🪛 Ruff (0.11.9)bio/fastq_screen/wrapper.py11-11: Undefined name (F821) 13-13: Undefined name (F821) 14-14: Undefined name (F821) 15-15: Undefined name (F821) 16-16: Undefined name (F821) 20-20: Undefined name (F821) 55-55: Undefined name (F821) 59-59: Undefined name (F821) 63-63: Undefined name (F821) 🪛 Pylint (3.3.7)bio/fastq_screen/wrapper.py[error] 11-11: Undefined variable 'snakemake' (E0602) [error] 13-13: Undefined variable 'snakemake' (E0602) [error] 14-14: Undefined variable 'snakemake' (E0602) [error] 15-15: Undefined variable 'snakemake' (E0602) [error] 16-16: Undefined variable 'snakemake' (E0602) [error] 20-20: Undefined variable 'snakemake' (E0602) [error] 55-55: Undefined variable 'snakemake' (E0602) [error] 59-59: Undefined variable 'snakemake' (E0602) [error] 63-63: Undefined variable 'snakemake' (E0602) ⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (5)
✨ 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
bio/fastq_screen/environment.linux-64.pin.txt(1 hunks)bio/fastq_screen/environment.yaml(1 hunks)bio/fastq_screen/meta.yaml(2 hunks)bio/fastq_screen/test/Snakefile(1 hunks)bio/fastq_screen/wrapper.py(1 hunks)test_wrappers.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.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 `s...
**/*.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.
bio/fastq_screen/wrapper.pytest_wrappers.py
`**/wrapper.py`: Do not complain about use of undefined variable called `snakemake`.
**/wrapper.py: Do not complain about use of undefined variable calledsnakemake.
bio/fastq_screen/wrapper.py
🧠 Learnings (1)
bio/fastq_screen/meta.yaml (2)
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: 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`.
🪛 Ruff (0.11.9)
bio/fastq_screen/wrapper.py
11-11: Undefined name snakemake
(F821)
13-13: Undefined name snakemake
(F821)
14-14: Undefined name snakemake
(F821)
15-15: Undefined name snakemake
(F821)
16-16: Undefined name snakemake
(F821)
20-20: Undefined name snakemake
(F821)
54-54: Undefined name snakemake
(F821)
58-58: Undefined name snakemake
(F821)
62-62: Undefined name snakemake
(F821)
🪛 Pylint (3.3.7)
bio/fastq_screen/wrapper.py
[error] 11-11: Undefined variable 'snakemake'
(E0602)
[error] 13-13: Undefined variable 'snakemake'
(E0602)
[error] 14-14: Undefined variable 'snakemake'
(E0602)
[error] 15-15: Undefined variable 'snakemake'
(E0602)
[error] 16-16: Undefined variable 'snakemake'
(E0602)
[error] 20-20: Undefined variable 'snakemake'
(E0602)
[error] 54-54: Undefined variable 'snakemake'
(E0602)
[error] 58-58: Undefined variable 'snakemake'
(E0602)
[error] 62-62: Undefined variable 'snakemake'
(E0602)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: testing
- GitHub Check: docs
- GitHub Check: Summary
🔇 Additional comments (12)
bio/fastq_screen/environment.yaml (1)
9-9: Verify availability and pin consistency of the new Perl package.
Ensure thatperl-gdgraph-histogram=1.1is published on both theconda-forgeandbiocondachannels to avoid resolution failures, and confirm that the corresponding Linux-64 pin file (environment.linux-64.pin.txt) has been updated.#!/usr/bin/env bash # Check availability on conda-forge and bioconda conda search perl-gdgraph-histogram=1.1 -c conda-forge -c bioconda # Verify the Linux-64 pin file includes this entry grep -R "perl-gdgraph-histogram =1.1" bio/fastq_screen/environment.linux-64.pin.txtbio/fastq_screen/meta.yaml (2)
4-5: Add new author and project URL.
Thibault Dayris is correctly added to the authors list, and theurlfield now points to the official FastQ-Screen GitHub repository.
46-48: ```shell
#!/usr/bin/env bashConfirm whether the wrapper writes to snakemake.output.conf
rg -n "snakemake\.output\.conf" bio/fastq_screen/wrapper.py || echo "No direct reference to snakemake.output.conf"
Look for any handling of 'conf' or 'fastq_screen_config' in wrapper logic
rg -n "conf" bio/fastq_screen/wrapper.py
rg -n "fastq_screen_config" bio/fastq_screen/wrapper.py</details> <details> <summary>test_wrappers.py (1)</summary> `3218-3225`: **LGTM! Proper test coverage for new fastq_screen functionality.** The additional test cases appropriately cover the new output options (config file and no-PNG mode) that were added to handle PNG generation issues on certain architectures. The implementation follows the existing test pattern and maintains consistency with the codebase. </details> <details> <summary>bio/fastq_screen/wrapper.py (4)</summary> `6-8`: **LGTM! Author metadata update is appropriate.** The addition of the second contributor to the author and email fields is properly formatted. --- `16-16`: **Good improvement to logging configuration.** Adding `append=True` ensures log entries are preserved across multiple runs, which is helpful for debugging. --- `23-51`: **Excellent refactoring using TemporaryDirectory context manager.** The refactoring to use `TemporaryDirectory` is a significant improvement that ensures automatic cleanup of temporary files and directories, making the code more robust and maintainable. --- `53-64`: **Excellent improvement with conditional output file moves.** The conditional checks using `snakemake.output.get()` make the output operations optional, which aligns perfectly with the PR objectives to accommodate architectures that may have problems with PNG file creation. The verbose logging is also helpful for debugging. </details> <details> <summary>bio/fastq_screen/test/Snakefile (3)</summary> `3-15`: **Good formatting improvements to the existing rule.** The trailing commas, quote consistency, and explicit log/threads specifications improve readability and maintainability. --- `18-36`: **Excellent test rule for config file output.** This rule properly tests the new functionality where the wrapper generates a config file from a dictionary and outputs it. The inline config dictionary structure matches the expected format used in the wrapper. --- `39-55`: **Good test rule for the no-PNG scenario.** This rule effectively tests the optional PNG output functionality, which is crucial for the PR objective of accommodating architectures that have problems with PNG file creation. </details> <details> <summary>bio/fastq_screen/environment.linux-64.pin.txt (1)</summary> `4-66`: **Appropriate package version updates and dependency additions.** The environment file updates include: - Core system library refreshes with newer versions - Python 3.12 upgrade for better performance and features - Addition of `perl-gdgraph-histogram` and other Perl graphics packages to support PNG generation - Updated bioinformatics tool versions (bowtie, bowtie2) These changes align well with the PR objectives to improve PNG generation support and maintain current package versions. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Co-authored-by: Filipe G. Vieira <[email protected]>
|
@tdayris Can you also address/check the issue raised by coderabbitai? |
🤖 I have created a release *beep* *boop* --- ## [7.1.0](v7.0.0...v7.1.0) (2025-06-20) ### Features * Pyfaidx ([#4159](#4159)) ([4bb2242](4bb2242)) ### Bug Fixes * Fastq screen missing PNG ([#4284](#4284)) ([d7e7503](d7e7503)) ### Performance Improvements * autobump bio/bismark/bismark_methylation_extractor ([#4260](#4260)) ([bfd4627](bfd4627)) * autobump bio/encode_fastq_downloader ([#4263](#4263)) ([534231d](534231d)) * autobump bio/fastp ([#4275](#4275)) ([a315cff](a315cff)) * autobump bio/gatk3/baserecalibrator ([#4268](#4268)) ([3941eec](3941eec)) * autobump bio/gatk3/indelrealigner ([#4266](#4266)) ([7142888](7142888)) * autobump bio/gatk3/printreads ([#4267](#4267)) ([a70ac2a](a70ac2a)) * autobump bio/gatk3/realignertargetcreator ([#4265](#4265)) ([7aac10d](7aac10d)) * autobump bio/gfatools ([#4264](#4264)) ([6df1c85](6df1c85)) * autobump bio/igv-reports ([#4214](#4214)) ([b93892b](b93892b)) * autobump bio/mapdamage2 ([#4269](#4269)) ([677d564](677d564)) * autobump bio/minimap2/aligner ([#4276](#4276)) ([2944f38](2944f38)) * autobump bio/minimap2/index ([#4277](#4277)) ([d92345b](d92345b)) * autobump bio/reference/ensembl-variation ([#4278](#4278)) ([5b55c94](5b55c94)) * autobump bio/spades/metaspades ([#4270](#4270)) ([2d1b106](2d1b106)) * autobump bio/tabix/index ([#4252](#4252)) ([0152771](0152771)) * autobump bio/trf ([#4271](#4271)) ([1e80067](1e80067)) * autobump bio/ucsc/faToTwoBit ([#4281](#4281)) ([0037df1](0037df1)) * autobump bio/ucsc/twoBitInfo ([#4279](#4279)) ([e0048ce](e0048ce)) * autobump bio/ucsc/twoBitToFa ([#4280](#4280)) ([ff31ef7](ff31ef7)) * autobump bio/vembrane/filter ([#4282](#4282)) ([870f1b0](870f1b0)) * autobump bio/vembrane/table ([#4283](#4283)) ([d4797c9](d4797c9)) * autobump bio/vep/plugins ([#4272](#4272)) ([7c47ce0](7c47ce0)) * autobump bio/whatshap/haplotag ([#4273](#4273)) ([cd59f63](cd59f63)) * autobump utils/datavzrd ([#4262](#4262)) ([aa0dc22](aa0dc22)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This PR fixes the issue (on
github actionsandalpine-linux containers) with FastqScreen not findingGD::Graph::barsand not building PNG files. This also makes all IO optional, in case some architecture still have issues with PNG file.Finally, I used
withstatement to handle temporary directory and its content.PS: I unfortunately made the mistake to start this branch from my previous
pyfaidxbranch, so it's a bit noisy in the commit logs... Now thatpyfaidxhas been accepted, its a bit better to read.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
Bug Fixes
Documentation
Chores