fix: various2 #150
fix: various2 #150
Conversation
WalkthroughThe changes involve updates to the GitHub Actions workflow for release management, introducing a new method for uploading release assets by zipping generated PDF files. Additionally, a new configuration file for managing sample data for the BWA tool is introduced, along with modifications to LaTeX documents and Snakemake workflow examples to enhance formatting and consistency. 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: 1
🧹 Outside diff range and nitpick comments (10)
setup_creators/solutions/config/config.yaml (2)
1-3: LGTM! Consider adding comments for clarity.The
samplessection is well-structured and correctly defines two samples with their corresponding FASTQ files. This setup is clear and easy to understand.Consider adding a brief comment above this section to explain its purpose, e.g.:
# Define samples and their corresponding FASTQ files samples: A: A.fastq B: B.fastq
1-6: Overall, the configuration file is well-structured and suitable for its purpose.The YAML file correctly defines the samples and BWA mapping parameters, making it a good example for teaching purposes. The structure is clear and easy to understand, which is crucial for educational material.
For future enhancements or more complex scenarios, consider:
- Adding more samples to demonstrate scalability.
- Including additional BWA parameters to show how to configure more complex tool options.
- Implementing a schema validation for the YAML file to ensure all required fields are present and correctly formatted.
These suggestions aim to make the configuration more robust and educational, but they're not necessary for the current scope of the teaching material.
.github/workflows/release-please.yml (2)
36-37: LGTM! Consider using a more specific zip filename.The addition of the Snakemake_HPC_Admins.tex render command and the zipping of PDF files are good improvements. They align with the PR objective of enhancing teaching materials and prepare for a more efficient upload process.
Consider using a more specific filename for the zip file, such as
snakemake_hpc_slides.zip, to make it easier to identify the content:- zip slides slides/*pdf + zip snakemake_hpc_slides slides/*pdf
48-57: Remove commented-out code.The commented-out lines represent the old upload process using
actions/upload-artifact@v4. While it's sometimes useful to keep old code for reference during development, it's generally better to remove it before merging to keep the codebase clean.Consider removing these commented-out lines entirely. If you need to reference this code in the future, you can always find it in the git history.
- # - name: Upload Release Artifacts - #if: ${{ steps.release.outputs.release_created }} - #uses: actions/upload-artifact@v4 - #with: - #name: Slides - #path: | - # slides/Snakemake_HPC_Creators.pdf; - # slides/Snakemake_HPC_Users.pdf; - # slides/Snakemake_HPC_User_Creator_Combi.pdf; - # slides/Snakemake_HPC_Admins.pdf;slides/common/mod_cs_mainz.tex (1)
15-16: Approved: Layout adjustment improves slide presentation.The addition of 18mm vertical space before the citation enhances the visual layout of the slide. This change aligns with the PR objective of implementing minor fixes to improve teaching materials.
For consistency and easier maintenance, consider using a LaTeX length variable for such spacing. For example:
\newlength{\citationvspace} \setlength{\citationvspace}{18mm} % Then use it in your document \vspace{\citationvspace}This approach allows for easier global adjustments to spacing throughout the document.
slides/creators/Decorating_the_Workflow.tex (5)
439-443: LGTM! Clear improvement in command line exampleThe changes enhance the clarity of the SLURM execution example by using placeholders for cluster-specific information. The added comment about quotation marks and argument formatting is helpful for users.
Consider wrapping the entire command in triple backticks (```) for better readability in markdown-rendered documentation.
459-463: LGTM! Helpful addition of-j unlimitedexplanationThe explanation of the
-j unlimitedoption and its inclusion in the command line example is valuable. It helps users understand how to allow Snakemake to submit an unlimited number of jobs to the cluster.Consider adding a brief note about the potential implications of using
-j unlimited, such as the risk of overwhelming the cluster or hitting job submission limits.
491-492: LGTM! Improved command line example with-FoptionThe addition of the
-Foption is helpful for enforcing execution. The conditional inclusion of the cluster account placeholder improves the flexibility of the example for different cluster setups.Consider adding a brief explanation of what the
-Foption does, either in the slide text or as a comment in the LaTeX source.
577-581: LGTM! Helpful addition of profile setup instructionsThe addition of instructions for copying a template profile and the inclusion of the
--workflow-profileoption in the command line example are helpful for users setting up their Snakemake workflow with a profile.Consider adding a brief explanation of what a Snakemake profile is and its benefits, either in the slide text or as a comment in the LaTeX source.
Line range hint
1-581: Overall, great improvements to the Snakemake workflow slidesThe changes in this file significantly enhance the clarity and educational value of the Snakemake workflow slides, particularly in the areas of SLURM execution and profile usage. The updated command line examples and added explanations provide users with more comprehensive guidance on running Snakemake workflows on HPC clusters.
Consider adding a section or slide that discusses best practices for organizing Snakemake workflows, including the use of profiles, config files, and modular rule definitions. This could help users create more maintainable and portable workflows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- .github/workflows/release-please.yml (1 hunks)
- setup_creators/solutions/config/config.yaml (1 hunks)
- slides/common/mod_cs_mainz.tex (1 hunks)
- slides/creators/Decorating_the_Workflow.tex (5 hunks)
🧰 Additional context used
actionlint
.github/workflows/release-please.yml
44-44: property "create_release" is not defined in object type {}
(expression)
🔇 Additional comments not posted (2)
.github/workflows/release-please.yml (1)
38-47:⚠️ Potential issueGood improvement, but there are some issues to address.
The new upload process using
actions/upload-release-asset@v1is a good improvement, as it's more efficient to upload a single zip file rather than multiple PDFs. However, there are a couple of issues that need to be addressed:
- The
upload_urlparameter references a step (create_release) that doesn't exist in this workflow. This will cause the upload to fail.- The
asset_content_typeis incorrect for a zip file.To fix these issues, please make the following changes:
Replace the
upload_urlwith the correct reference. If you're usingrelease-please-actionto create the release, you should use its output. Otherwise, you may need to add a step to create the release first.Correct the
asset_content_typefor a zip file.- upload_url: ${{ steps.create_release.outputs.upload_url }} + upload_url: ${{ steps.release.outputs.upload_url }} - asset_path: ./slides.zip + asset_path: ./snakemake_hpc_slides.zip - asset_name: slides.zip + asset_name: snakemake_hpc_slides.zip - asset_content_type: slide/zip + asset_content_type: application/zipAlso, ensure that the
release-please-actionstep (in therelease-pleasejob) is configured to create a release and provide theupload_urloutput.To verify the correct setup of the release-please action, run:
🧰 Tools
actionlint
44-44: property "create_release" is not defined in object type {}
(expression)
slides/creators/Decorating_the_Workflow.tex (1)
153-155: Clarify the source of thesamplesvariableThe change from
config["samples"]tosamplessimplifies the code, but it's unclear wheresamplesis defined or how it relates to the previousconfig["samples"]. Please provide context on howsamplesis initialized and whether this change maintains the same functionality.
| bwa_map: | ||
| rg=r"@RG\tID:{sample}\tSM:{sample}" |
There was a problem hiding this comment.
💡 Codebase verification
Missing FASTQ Files
The validation script indicates that A.fastq and B.fastq are not found in the expected directory. Please ensure these files are present or update the samples section with the correct file paths.
🔗 Analysis chain
LGTM! Consider adding comments and validating file paths.
The bwa_map section correctly defines the read group (RG) format string for BWA. The use of a raw string literal (r"...") is appropriate for handling escape characters, and the placeholders for {sample} will allow for dynamic replacement.
- Consider adding a comment to explain the purpose of this section and the format string:
# BWA mapping parameters
bwa_map:
# Read group format string: includes sample ID and sample name
rg: r"@RG\tID:{sample}\tSM:{sample}"- It might be beneficial to add a note or implement a validation step to ensure that the FASTQ files specified in the
samplessection actually exist in the expected directory.
To verify the existence of the FASTQ files, you can run the following script:
This script will help ensure that the specified FASTQ files are present in the expected location.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of FASTQ files specified in the config.yaml
# Test: Check if the FASTQ files exist
for file in A.fastq B.fastq; do
if [ ! -f "$file" ]; then
echo "Warning: $file not found in the current directory"
else
echo "$file exists"
fi
done
Length of output: 335
There was a problem hiding this comment.
these are tutorial data - they are provided seperately.
There was a problem hiding this comment.
@cmeesters, thanks for the clarification! Since the FASTQ files are tutorial data provided separately, it's acceptable that they are not present in the repository.
🐰❓
✏️ Learnings added
Learnt from: cmeesters
PR: snakemake/snakemake-hpc-teaching-material#150
File: setup_creators/solutions/config/config.yaml:5-6
Timestamp: 2024-09-25T20:32:40.282Z
Learning: The FASTQ files `A.fastq` and `B.fastq` are tutorial data provided separately and may not be present in the repository.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Artifacts |
some minor fixes after teaching
Summary by CodeRabbit