quarto fix #209
quarto fix #209
Conversation
📝 WalkthroughWalkthroughThis pull request standardizes the help text for the Changes
Possibly related PRs
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: 0
🧹 Nitpick comments (1)
harpy/reports/naibr.qmd (1)
3-15: LGTM! The CSS changes effectively address the tooltip visibility issue.The added styles ensure tooltips appear above the plots by setting appropriate z-index values.
Consider consolidating the two style blocks to reduce duplication:
- - text: | - <style> - .BioCircosSNPTooltip { - z-index: 10; - } - </style> - - text: | - <style> - .BioCircosARCTooltip { - z-index: 10; - } - </style> + - text: | + <style> + .BioCircosSNPTooltip, + .BioCircosARCTooltip { + z-index: 10; + } + </style>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
harpy/align.py(3 hunks)harpy/assembly.py(1 hunks)harpy/deconvolve.py(1 hunks)harpy/demultiplex.py(1 hunks)harpy/downsample.py(1 hunks)harpy/impute.py(1 hunks)harpy/metassembly.py(1 hunks)harpy/phase.py(1 hunks)harpy/preflight.py(2 hunks)harpy/qc.py(1 hunks)harpy/reports/align_stats.qmd(1 hunks)harpy/reports/leviathan.qmd(1 hunks)harpy/reports/leviathan_pop.qmd(1 hunks)harpy/reports/naibr.qmd(1 hunks)harpy/reports/naibr_pop.qmd(1 hunks)harpy/resume.py(1 hunks)harpy/simulate_linkedreads.py(1 hunks)harpy/simulate_variants.py(4 hunks)harpy/snp.py(2 hunks)harpy/sv.py(2 hunks)
✅ Files skipped from review due to trivial changes (14)
- harpy/impute.py
- harpy/demultiplex.py
- harpy/reports/leviathan.qmd
- harpy/qc.py
- harpy/phase.py
- harpy/deconvolve.py
- harpy/sv.py
- harpy/assembly.py
- harpy/align.py
- harpy/simulate_variants.py
- harpy/snp.py
- harpy/resume.py
- harpy/preflight.py
- harpy/reports/align_stats.qmd
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: align strobe
- GitHub Check: simulate linkedreads
- GitHub Check: impute
- GitHub Check: align EMA
- GitHub Check: align BWA
- GitHub Check: Rebuild Container
- GitHub Check: assembly
🔇 Additional comments (6)
harpy/downsample.py (2)
33-33: LGTM! Help text standardization improves clarity.The help text changes across multiple files standardize the
--quietoption description, making it more concise while maintaining clarity about the verbosity levels.
33-33: Verify PR objectives alignment.The PR description mentions fixing tooltip visibility in quarto reports, but these changes appear to be unrelated to that objective.
Could you clarify if there are additional changes related to the tooltip visibility fix mentioned in the PR objectives?
harpy/metassembly.py (1)
41-41: LGTM! Consistent help text standardization.The help text update aligns with the standardization across other files.
harpy/simulate_linkedreads.py (1)
41-41: LGTM! Help text matches standardization.The help text update maintains consistency with other files in the codebase.
harpy/reports/leviathan_pop.qmd (1)
3-15: Same changes as in naibr.qmd.The CSS changes are identical to those in naibr.qmd and serve the same purpose.
harpy/reports/naibr_pop.qmd (1)
3-15: Same changes as in naibr.qmd and leviathan_pop.qmd.The CSS changes are identical to those in the other files and serve the same purpose.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
harpy/_cli_types_generic.py (1)
100-101: Consider moving option lists to a configuration file.The lists of Snakemake options are quite long and could benefit from being moved to a configuration file. This would improve maintainability and make it easier to update the lists when new Snakemake versions are released.
Consider creating a new YAML configuration file (e.g.,
snakemake_options.yaml) with the following structure:forbidden_options: - "--rerun-incomplete" - "--ri" - "--show-failed-logs" # ... rest of forbidden options available_options: - "--profile" - "--cache" - "--jobs" # ... rest of available optionsThen modify the code to load these options from the configuration file:
import yaml from pathlib import Path class SnakemakeParams(click.ParamType): def __init__(self): super().__init__() config_path = Path(__file__).parent / "config" / "snakemake_options.yaml" with open(config_path) as f: options = yaml.safe_load(f) self.forbidden = options["forbidden_options"] self.available = options["available_options"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
harpy/_cli_types_generic.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: simulate linkedreads
- GitHub Check: impute
- GitHub Check: align EMA
- GitHub Check: assembly
🔇 Additional comments (1)
harpy/_cli_types_generic.py (1)
100-101:❓ Verification inconclusive
Verify Snakemake options and their compatibility.
The changes to the
forbiddenandavailablelists look good, but let's verify that all options are valid and that no critical options are missing from the forbidden list.Run the following script to verify the Snakemake options:
🏁 Script executed:
#!/bin/bash # Description: Verify Snakemake options by checking the help output. # Get Snakemake help output snakemake --help > snakemake_help.txt 2>&1 # Check if all available options exist in Snakemake help for opt in "--profile" "--cache" "--jobs" "-j" "--local-cores" "--resources" "--res" "--set-threads" "--max-threads" "--set-resources" "--set-scatter" "--set-resource-scopes" "--default-resources" "--default-res" "--preemptible-rules" "--preemptible-retries" "--envvars" "--touch" "-t" "--keep-going" "-k" "--force" "-f" "--executor" "-e" "--forceall" "-F" "--forcerun" "-R" "--prioritize" "-P" "--batch" "--until" "-U" "--omit-from" "-O" "--shadow-prefixDIR" "--scheduler" "--wms-monitor" "--wms-monitor-arg" "--scheduler-ilp-solver" "--conda-base-path" "--no-subworkflows" "--nosw" "--precommand" "--groups" "--group-components" "--report" "--report-stylesheet" "--reporterPLUGIN" "--draft-notebook" "--edit-notebook" "--notebook-listen" "--lint" "--generate-unit-tests" "--containerize" "--export-cwl" "--list-rules" "--list" "-l" "--list-target-rules" "--lt" "--dag" "--rulegraph" "--filegraph" "--d3dag" "--summary" "-S" "--detailed-summary" "-D" "--archive" "--cleanup-metadata" "--cmFILE" "--cleanup-shadow" "--skip-script-cleanup" "--unlock" "--list-changes" "--lc" "--list-input-changes" "--li" "--list-params-changes" "--lp" "--list-untracked" "--lu" "--delete-all-output" "--delete-temp-output" "--keep-incomplete" "--drop-metadata" "--version" "-v" "--printshellcmds" "-p" "--nocolor" "--print-compilation" "--force-use-threads" "--allow-ambiguity" "-a" "--ignore-incomplete" "--ii" "--max-inventory-time" "--latency-wait" "--output-wait" "-w" "--wait-for-files" "--wait-for-files-file" "--queue-input-wait-time" "--notemp" "--nt" "--all-temp" "--unneeded-temp-files" "--keep-storage-local-copies" "--target-files-omit-workdir-adjustment" "--allowed-rules" "--max-jobs-per-timespan" "--max-jobs-per-second" "--max-status-checks-per-second" "--seconds-between-status-checks" "--retries" "--restart-times" "-T" "--default-storage-provider" "--default-storage-prefix" "--local-storage-prefix" "--remote-job-local-storage-prefix" "--shared-fs-usage" "--scheduler-greediness" "--greediness" "--runtime-profile" "--local-groupid" "--attempt" "--log-handler-script" "--log-service" "--job-deploy-sources" "--benchmark-extended" "--container-image" "--immediate-submit" "--is" "--jobscript" "--js" "--jobname" "--jn" "--flux" "--container-cleanup-images" "--conda-not-block-search-path-envvars" "--conda-frontend" "--apptainer-args" "--singularity-args" "--use-envmodules" "--scheduler-solver-path" "--deploy-sources" "--target-jobs" "--mode" "--report-html-path" "--report-html-stylesheet-path"; do if ! grep -q "$opt" snakemake_help.txt; then echo "Warning: Option $opt not found in Snakemake help" fi done # Clean up rm snakemake_help.txtLength of output: 69265
Manual verification required for Snakemake options and maintainability improvements
The modifications in
harpy/_cli_types_generic.py(lines 100–101) to update theforbiddenandavailablelists appear reasonable at a glance. However, our automated verification script did not yield reliable results because grep misinterprets option strings (e.g.--profile,--cache, etc.) as its own flags. This means the script’s warnings aren’t a definitive indication of missing or invalid options.Action items:
- Manual Check Needed: Please manually verify that each option in both lists matches the options provided by your Snakemake installation (by inspecting the output of
snakemake --help).- Maintainability Suggestion: Consider migrating these long static lists to a separate configuration file or module to improve readability and manageability.
- Script Improvement: If you wish to automate this check in the future, update the verification script (for example, by prefixing grep searches with
--to disable option parsing) so that Snakemake options aren’t misinterpreted.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
harpy/_cli_types_params.py (1)
259-263: Remove unnecessary blank lines at the end of the file.Consider removing the extra blank lines at the end of the file to maintain cleaner code.
- - - - -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
harpy/__main__.py(2 hunks)harpy/_cli_types_params.py(2 hunks)harpy/align.py(4 hunks)harpy/assembly.py(2 hunks)harpy/deconvolve.py(2 hunks)harpy/demultiplex.py(2 hunks)harpy/downsample.py(2 hunks)harpy/hpc.py(1 hunks)harpy/impute.py(2 hunks)harpy/metassembly.py(2 hunks)harpy/phase.py(2 hunks)harpy/preflight.py(2 hunks)harpy/qc.py(2 hunks)harpy/simulate_linkedreads.py(2 hunks)harpy/simulate_variants.py(5 hunks)harpy/snp.py(3 hunks)harpy/sv.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- harpy/assembly.py
- harpy/impute.py
- harpy/demultiplex.py
- harpy/deconvolve.py
- harpy/align.py
- harpy/simulate_variants.py
- harpy/qc.py
- harpy/metassembly.py
- harpy/simulate_linkedreads.py
- harpy/downsample.py
- harpy/preflight.py
- harpy/phase.py
- harpy/sv.py
- harpy/snp.py
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: align strobe
- GitHub Check: simulate variants
- GitHub Check: simulate linkedreads
- GitHub Check: qc
- GitHub Check: preflight
- GitHub Check: phase
- GitHub Check: sv naibr
- GitHub Check: mpileup
- GitHub Check: sv leviathan
- GitHub Check: impute
- GitHub Check: freebayes
- GitHub Check: assembly
- GitHub Check: align EMA
- GitHub Check: demux gen1
- GitHub Check: deconvolve
- GitHub Check: align BWA
- GitHub Check: Rebuild Container
🔇 Additional comments (8)
harpy/hpc.py (1)
29-29: LGTM! But verify alignment with PR objectives.The addition of blue border styling for the "Submission Systems" panel is a safe cosmetic improvement that enhances visual hierarchy in the CLI help output.
This change appears unrelated to the PR's main objective of fixing tooltip visibility in quarto reports. Please verify if this change should be part of this PR or if it would be better suited for a separate PR focused on CLI styling improvements.
harpy/_cli_types_params.py (3)
133-133: LGTM! Clear docstring update.The updated docstring better describes the class's functionality, particularly its role in sanitizing different input styles.
140-144: Improved input handling in StitchParams.The changes enhance input flexibility by:
- Allowing inputs without dashes
- Properly sanitizing arguments
- Maintaining consistent format with STITCH CLI requirements
Let's verify the input handling with some test cases:
#!/usr/bin/env python3 # Test various input formats test_inputs = [ "param=value", # no dash "--param=value", # with dash "param1=val1 param2=val2" # multiple params ] print("Testing StitchParams input handling:") for input in test_inputs: print(f"\nInput: {input}") print(f"Expected: --param=value" if "param1" not in input else "--param1=val1 --param2=val2")Also applies to: 146-150, 154-154, 156-156, 159-159
207-209: Enhanced NaibrParams input processing.The changes improve the handling of NAIBR parameters by:
- Properly handling argument-value pairs
- Maintaining consistent formatting
- Providing clearer error messages
Let's verify the NAIBR parameter handling:
#!/usr/bin/env python3 # Test NAIBR parameter handling test_inputs = [ "blacklist file.txt", # valid format "-blacklist file.txt", # with dash (should be stripped) "blacklist file1 candidates file2" # multiple params ] print("Testing NaibrParams input handling:") for input in test_inputs: print(f"\nInput: {input}") print(f"Expected output after stripping dashes: {input.replace('-', '')}")Also applies to: 216-216, 218-219
harpy/__main__.py (4)
62-63: LGTM! Visual styling enhances command group distinction.The added panel styles help users visually distinguish between different types of simulate commands, improving the CLI's usability.
Also applies to: 67-68
104-105: LGTM! Improved command organization and visual hierarchy.The changes enhance the CLI's usability through:
- Logical grouping of commands
- Consistent visual styling for each group
- Alphabetical sorting for predictable command listing
Also applies to: 109-110, 114-115
1-123: Verify the relationship between CLI styling changes and tooltip fix.The changes in this file focus on CLI styling and organization, which seem unrelated to the PR's stated objective of fixing tooltip visibility in quarto reports. Please clarify if these changes are intentional or if they should be in a separate PR.
120-120:❓ Verification inconclusive
Verify the impact of removing options panel title on help text readability.
While removing the options panel title creates a cleaner look, please ensure this doesn't impact the help text's clarity and accessibility.
Run this script to check the help output format:
🏁 Script executed:
#!/bin/bash # Description: Check help text format for various commands # Test main help python -m harpy --help # Test a few subcommands to ensure consistent formatting for cmd in "simulate" "align" "qc"; do echo -e "\nTesting help for: $cmd" python -m harpy $cmd --help doneLength of output: 1569
Action Required: Manually verify help text readability and dependency management.
The change to set
OPTIONS_PANEL_TITLEtoNonealigns with the minimalist design principle, but our automated help output check failed due to a missing dependency (rich_click). This prevents us from confirming that help text readability and accessibility remain intact.
- Dependency Issue: The error
ModuleNotFoundError: No module named 'rich_click'indicates thatrich_clickis either missing from dependencies or not properly installed. Please verify that this dependency is correctly specified and available.- Manual Verification: In the meantime, manually verify the help outputs for the main command and its subcommands to ensure that the removal of the options panel title does not adversely affect the help text's clarity and accessibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
harpy/snakefiles/demultiplex_gen1.smk (4)
18-24: Consider logging file-removal outcomes.
Silently ignoring missing files might mask potential configuration issues. It may help to log a warning if the files are not found, rather than passing silently.
53-66: Validate the plus-line counting logic and small-read edge cases.
Usingzgrep -c -x '+'(lines 55-56) can fail if the '+' lines in FASTQs sometimes contain extra text or if the file is not strictly standard. Also, edge cases whereread_min < 20may lead to zero chunks. Consider adding guards to handle such scenarios more explicitly.- count_r1 = subprocess.check_output(["zgrep", "-c", "-x", "+", fq1]) - count_r2 = subprocess.check_output(["zgrep", "-c", "-x", "+", fq2]) + # Example alternative (count lines whose entire content is "+"): + count_r1 = subprocess.check_output(["zgrep", "-c", '^+$', fq1]) + count_r2 = subprocess.check_output(["zgrep", "-c", '^+$', fq2])
89-96: Handle concurrency risk or stale symlinks carefully.
Removing existing files/symlinks and creating new symlinks is fine. Still, if concurrent runs or partial files write is possible, consider additional checks.
270-272: Maintain consistency in chunking documentation.
The summary referencesseqkit -r start:stop, while the code usesseqkit range. Ensure the snippet matches the actual command usage to prevent confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
harpy/_launch.py(1 hunks)harpy/snakefiles/demultiplex_gen1.smk(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: align strobe
- GitHub Check: simulate variants
- GitHub Check: phase
- GitHub Check: simulate linkedreads
- GitHub Check: qc
- GitHub Check: preflight
- GitHub Check: sv naibr
- GitHub Check: mpileup
- GitHub Check: sv leviathan
- GitHub Check: impute
- GitHub Check: freebayes
- GitHub Check: align EMA
- GitHub Check: demux gen1
- GitHub Check: Rebuild Container
- GitHub Check: assembly
- GitHub Check: deconvolve
- GitHub Check: align BWA
🔇 Additional comments (10)
harpy/snakefiles/demultiplex_gen1.smk (9)
5-5: Add-on import for subprocess looks fine.
The new import forsubprocessis justified since you usesubprocess.check_outputfor counting reads.
12-12: Document or verify the numeric cap on chunks.
Cappingn_chunksat 999 may be intentional, but consider documenting why 999 is chosen or making this value configurable.
32-32: Type hints are a welcome addition.
This signature clarification (def parse_schema(smpl: str, keep_unknown: bool) -> dict:) improves maintainability. Optionally, specify a more explicit return type, e.g.-> Dict[str, Union[List[str], str]], if desired.
68-69: Chunk dictionary creation looks correct.
Definingchunk_dictandfastq_partsis clear and straightforward, ensuring easy iteration over chunk keys.
80-80: Renaming torule link_inputclarifies intention.
This more explicit naming is good for readability and indicates the rule’s purpose more precisely than a generic name.
83-85: Preview external config references.
Addingi1andi2alongsider1andr2ensures all necessary index files are pulled from config. Consider verifying that they exist inconfig["inputs"]to avoid KeyError.
97-108: Verify partitioning logic with external QA tests.
rule partition_readsusesseqkit rangeto slice reads into chunked FASTQs. Test with small input files to confirm correctness, especially with successive chunk boundaries.
111-115: Reuse of partitioning logic for index reads is consistent.
Usinguse rule partition_reads as partition_indexproperly avoids code duplication. Good approach for DRY (Don't Repeat Yourself).
117-119: Checkpoint with high priority.
Elevatingdemultiplexto a checkpoint withpriority: 100may help ensure it proceeds promptly. Verify that this doesn’t inadvertently block concurrent tasks with lower priority.harpy/_launch.py (1)
157-157: Expanding rule matching to include checkpoints is beneficial.
The updated regexr"(rule|checkpoint)\s\w+:"will correctly capture checkpoint references. This should help track progress and properly handle these blocks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/tests.yml (1)
640-640: UpdatednaibrCommand: Removal of the-x "-min_sv 5000"OptionThe updated command now no longer includes the
-x "-min_sv 5000"parameter. Please verify that this change is intentional and aligns with other modules’ updated behavior and documentation (e.g., inharpy/sv.py). Ensure that any downstream expectations regarding minimum SV size filtering are addressed or documented as needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/tests.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: simulate variants
- GitHub Check: simulate linkedreads
- GitHub Check: sv naibr
- GitHub Check: align strobe
- GitHub Check: assembly
- GitHub Check: phase
- GitHub Check: mpileup
- GitHub Check: sv leviathan
- GitHub Check: impute
- GitHub Check: freebayes
- GitHub Check: align EMA
- GitHub Check: align BWA
- GitHub Check: demux gen1
- GitHub Check: qc
- GitHub Check: Rebuild Container
Add header text to quarto reports to fix tooltip being hidden beneath plot as addressed here: quarto-dev/quarto-cli#12107
Summary by CodeRabbit
Documentation
Style