feat!: Use composite action with conda instead of docker action #39
Conversation
📝 WalkthroughWalkthroughThe changes involve the removal of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Action
participant Snakemake
User->>Action: Trigger action with parameters
Action->>Action: Validate inputs
alt Valid inputs
Action->>Action: Setup environment
Action->>Snakemake: Execute Snakemake with parameters
alt Execution successful
Snakemake-->>Action: Return success
else Execution failed
Snakemake-->>Action: Return error
Action->>User: Show error message
alt Show disk usage
Action->>Action: Display disk usage information
end
end
else Invalid inputs
Action-->>User: Show error message
end
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: 3
🧹 Outside diff range and nitpick comments (3)
environment.yaml.template (1)
5-6: Consider pinning Python version for reproducibility.While snakemake version is specified, the Python version is not pinned. This could lead to inconsistent environments across different systems.
dependencies: - snakemake =={{ snakemake_version }} + - python >=3.8,<3.12action.yml (2)
33-36: Consider version constraint validation.While the default version is specified, there's no validation for user-provided versions. Consider adding a pattern check to ensure valid semver format.
snakemake-version: description: Which snakemake version to use required: false default: 8.25.5 + pattern: '^[0-9]+\.[0-9]+\.[0-9]+$'
49-55: Consider caching conda environment.To improve action performance, consider using GitHub's cache action for the conda environment.
+ - name: Cache conda env + uses: actions/cache@v3 + with: + path: /usr/share/miniconda3/envs/ + key: conda-${{ hashFiles('environment.yaml') }} - name: Setup conda uses: conda-incubator/setup-miniconda@v3 with:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
Dockerfile(0 hunks)action.yml(1 hunks)entrypoint.sh(0 hunks)environment.yaml.template(1 hunks)
💤 Files with no reviewable changes (2)
- Dockerfile
- entrypoint.sh
🔇 Additional comments (2)
environment.yaml.template (1)
1-4: LGTM! Channel configuration follows best practices.
The channel configuration is well-structured:
- Using conda-forge and bioconda is appropriate for bioinformatics workflows
- Including nodefaults prevents package conflicts
action.yml (1)
41-46: LGTM! Input validation is well implemented.
The validation step properly checks for valid task values and provides a clear error message.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
action.yml (3)
33-36: Consider using a more flexible version strategyWhile version pinning provides stability, it requires regular maintenance to stay up-to-date with new Snakemake releases. Consider using a major version range (e.g., "^8.0.0") to automatically get compatible updates.
snakemake-version: description: Which snakemake version to use required: false - default: 8.25.5 + default: ^8.0.0
41-46: Improve error message formattingThe error message could be more user-friendly and avoid potential escaping issues.
- name: Validate inputs if: ${{ ! (inputs.task == 'containerize' || inputs.task == 'run' )}} shell: bash -el {0} run: | - echo 'Invalid value for "task": "${{ inputs.task }}". Options: "containerize", "run".' + echo "Error: Invalid value for 'task': '${{ inputs.task }}'" + echo "Available options: 'containerize', 'run'" exit 1
39-39: Consider documenting the implications of switching to composite actionsWhile composite actions typically offer faster execution compared to Docker actions, they rely on the runner's environment. Consider:
- Documenting any specific runner requirements
- Adding environment validation steps
- Testing the action across different runner types (Ubuntu, Windows, macOS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
action.yml(1 hunks)
🔇 Additional comments (3)
action.yml (3)
47-48: Add error handling for template processing.
The sed command should check if the template file exists and handle potential errors.
60-69: Fix disk usage error handling.
There are several issues in the error handling block:
$1is not defined - should be${{ inputs.directory }}
70-73: Add error handling for containerization step.
The containerization step should check the exit status and provide appropriate error messages.
|
Things that could be improved:
|
tedil
changed the title
All three ideas are good. I would say the first one would be great still in this PR. The other ones can be postponed to follow up PRs. |
Or maybe it is always available in github CI? |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
action.yml (1)
61-70: Simplify error handling.The error handling can be simplified by removing the redundant exit status check.
- snakemake --directory ${{ inputs.directory }} --snakefile ${{ inputs.snakefile }} --show-failed-logs ${{ inputs.args }} - if [[ "$?" -ne 0 ]]; then + if ! snakemake --directory ${{ inputs.directory }} --snakefile ${{ inputs.snakefile }} --show-failed-logs ${{ inputs.args }}; then if [[ ${{ inputs.show-disk-usage-on-error }} = true ]]; then # return disk usage and space on failing df -h printf "disk usage working directory" du -h -d3 ${{ inputs.directory }} fi exit 1 fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
action.yml(1 hunks)
🧰 Additional context used
📓 Learnings (1)
action.yml (2)
Learnt from: johanneskoester
PR: snakemake/snakemake-github-action#39
File: action.yml:70-73
Timestamp: 2024-12-09T16:41:37.740Z
Learning: In Snakemake workflows, Snakemake itself provides proper error messages during the containerization step, so additional error handling in the GitHub Action script is unnecessary.
Learnt from: tedil
PR: snakemake/snakemake-github-action#39
File: action.yml:47-48
Timestamp: 2024-12-09T10:52:48.855Z
Learning: In the `action.yml` file for `snakemake-github-action`, since `environment.yaml.template` is part of the repository and is expected to exist, adding error handling to check for its presence is unnecessary.
🔇 Additional comments (2)
action.yml (2)
56-56: LGTM: Environment name set to 'snakemake'.
Good implementation of the suggested change to use an explicit environment name instead of the default 'test'.
71-74: LGTM: Clean containerization implementation.
The implementation is correct and appropriately relies on Snakemake's built-in error handling.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
action.yml (3)
40-45: Enhance input validation robustness.While the validation logic is correct, consider these improvements:
- Use double quotes for string interpolation
- Add input trimming to handle whitespace
- echo 'Invalid value for "task": "${{ inputs.task }}". Options: "containerize", "run".' + task="${{ inputs.task }}" + task="${task// /}" # trim whitespace + echo "Invalid value for \"task\": \"$task\". Options: \"containerize\", \"run\"."
48-56: Consider adding Apptainer installation support.As discussed in the PR comments, Apptainer installation needs to be addressed. Consider adding:
- A new input parameter to control Apptainer installation
- A conditional step to install Apptainer when needed
Example implementation:
inputs: install-apptainer: description: Whether to install Apptainer for container support required: false default: false # Add after conda setup steps: - name: Install Apptainer if: ${{ inputs.install-apptainer == 'true' }} shell: bash -el {0} run: | sudo add-apt-repository -y ppa:apptainer/ppa sudo apt update sudo apt install -y apptainer
57-69: Improve error handling readability.Consider these improvements:
- Add newline before disk usage message
- Simplify error handling using command chaining
- snakemake --directory ${{ inputs.directory }} --snakefile ${{ inputs.snakefile }} --show-failed-logs ${{ inputs.args }} - if [[ "$?" -ne 0 ]]; then + if ! snakemake --directory ${{ inputs.directory }} --snakefile ${{ inputs.snakefile }} --show-failed-logs ${{ inputs.args }}; then if [[ ${{ inputs.show-disk-usage-on-error }} = true ]]; then # return disk usage and space on failing df -h - printf "disk usage working directory" + printf "\nDisk usage working directory:\n" du -h -d3 ${{ inputs.directory }} fi exit 1 fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
action.yml(1 hunks)
🧰 Additional context used
📓 Learnings (1)
action.yml (2)
Learnt from: johanneskoester
PR: snakemake/snakemake-github-action#39
File: action.yml:70-73
Timestamp: 2024-12-09T16:41:37.740Z
Learning: In Snakemake workflows, Snakemake itself provides proper error messages during the containerization step, so additional error handling in the GitHub Action script is unnecessary.
Learnt from: tedil
PR: snakemake/snakemake-github-action#39
File: action.yml:47-48
Timestamp: 2024-12-09T10:52:48.855Z
Learning: In the `action.yml` file for `snakemake-github-action`, since `environment.yaml.template` is part of the repository and is expected to exist, adding error handling to check for its presence is unnecessary.
🔇 Additional comments (3)
action.yml (3)
30-35: LGTM! Input parameters are well-defined.
The description fix and the new snakemake-version input are well-documented. The absence of a default value for snakemake-version aligns with the discussion about using the latest version by default.
38-39: LGTM! Composite action setup is correct.
The transition from Docker to composite action is implemented correctly and aligns with the PR objectives.
70-73: LGTM! Containerization step is correctly implemented.
The containerization step is properly implemented and relies on Snakemake's built-in error handling as discussed.
not sure, but I've included an (optional) installation via apt as proposed by the 🐰 |
johanneskoester
changed the title
|
Nice! Testing this now here: snakemake-workflows/dna-seq-varlociraptor#345 |
|
Not sure if it is my fault or a bug though. |
Probably a bug, not sure what the template is relative to (upon invocation of the action).
Take your pick ;) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
action.yml (3)
33-36: Consider implications of using '*' as default versionWhile using '*' allows for automatic updates to the latest version, it might impact workflow reproducibility. Consider:
- Using an empty default to clearly indicate latest version behavior
- Adding a warning in the description about reproducibility implications
snakemake-version: - description: Snakemake version to use. If not specified, uses latest version. Pin a specific version (e.g., '8.25.5') for reproducibility. - required: false - default: '*' + description: | + Snakemake version to use. + - If not specified or empty: Uses latest version (recommended for development) + - For production/reproducible workflows: Pin a specific version (e.g., '8.25.5') + required: false
45-51: Enhance error message clarityThe error message could be more user-friendly and informative.
- name: Validate inputs if: ${{ ! (inputs.task == 'containerize' || inputs.task == 'run' )}} shell: bash -el {0} run: | - echo 'Invalid value for "task": "${{ inputs.task }}". Options: "containerize", "run".' + echo "Error: Invalid value for input parameter 'task'" + echo "Provided: '${{ inputs.task }}'" + echo "Expected: 'containerize' or 'run'" exit 1
99-103: Fix file formattingRemove extra blank lines at the end of the file to comply with YAML style guidelines.
- name: Containerize snakemake if: ${{ inputs.task == 'containerize' }} shell: bash -el {0} run: snakemake --directory ${{ inputs.directory }} --snakefile ${{ inputs.snakefile }} --show-failed-logs ${{ inputs.args }} --containerize > Dockerfile -🧰 Tools
🪛 yamllint (1.35.1)
[warning] 103-103: too many blank lines
(1 > 0) (empty-lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
action.yml(1 hunks)
🧰 Additional context used
📓 Learnings (1)
action.yml (2)
Learnt from: johanneskoester
PR: snakemake/snakemake-github-action#39
File: action.yml:70-73
Timestamp: 2024-12-09T16:41:37.740Z
Learning: In Snakemake workflows, Snakemake itself provides proper error messages during the containerization step, so additional error handling in the GitHub Action script is unnecessary.
Learnt from: tedil
PR: snakemake/snakemake-github-action#39
File: action.yml:47-48
Timestamp: 2024-12-09T10:52:48.855Z
Learning: In the `action.yml` file for `snakemake-github-action`, since `environment.yaml.template` is part of the repository and is expected to exist, adding error handling to check for its presence is unnecessary.
🪛 yamllint (1.35.1)
action.yml
[warning] 103-103: too many blank lines
(1 > 0) (empty-lines)
[error] 62-62: syntax error: expected , but found ''
(syntax)
🔇 Additional comments (2)
action.yml (2)
43-44: LGTM: Good transition to composite action
The switch from Docker to composite action provides better flexibility and control over the execution environment.
80-98: LGTM: Good error handling and version display
The implementation includes helpful debugging information with version display and proper error handling with disk usage reporting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
action.yml (1)
45-51: Enhance error message with valid optionsThe error message could be more helpful by explicitly listing the valid options.
- name: Validate inputs if: ${{ ! (inputs.task == 'containerize' || inputs.task == 'run' )}} shell: bash -el {0} run: | - echo 'Invalid value for "task": "${{ inputs.task }}". Options: "containerize", "run".' + echo 'Error: Invalid value for "task": "${{ inputs.task }}"' + echo 'Valid options are:' + echo ' - "containerize": Generate a container image specification' + echo ' - "run": Execute the Snakemake workflow' exit 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
action.yml(1 hunks)
🧰 Additional context used
📓 Learnings (1)
action.yml (2)
Learnt from: johanneskoester
PR: snakemake/snakemake-github-action#39
File: action.yml:70-73
Timestamp: 2024-12-09T16:41:37.740Z
Learning: In Snakemake workflows, Snakemake itself provides proper error messages during the containerization step, so additional error handling in the GitHub Action script is unnecessary.
Learnt from: tedil
PR: snakemake/snakemake-github-action#39
File: action.yml:47-48
Timestamp: 2024-12-09T10:52:48.855Z
Learning: In the `action.yml` file for `snakemake-github-action`, since `environment.yaml.template` is part of the repository and is expected to exist, adding error handling to check for its presence is unnecessary.
🔇 Additional comments (5)
action.yml (5)
84-98: LGTM: Run step with error handling looks good
The implementation correctly handles errors and provides disk usage information when requested.
99-102: LGTM: Containerization step looks good
As per the learnings from @johanneskoester, Snakemake itself provides proper error messages during containerization, so additional error handling is not needed.
33-36: Consider adding version constraint validation
While using '*' as default allows using the latest version, it might be worth adding validation to ensure that when a specific version is provided, it follows semantic versioning format (e.g., '8.25.5').
snakemake-version:
description: Snakemake version to use. If not specified, uses latest version. Pin a specific version (e.g., '8.25.5') for reproducibility.
required: false
- default: '*'
+ default: 'latest'✅ Verification successful
Let me check how the version is actually used in the action to determine if validation is really necessary.
Using '*' as default version is safe, no validation needed
The version parameter is used directly with pip's version specifier syntax (==) which safely handles both specific versions and wildcards. When '*' is used, it will install the latest version, and when a specific version is provided (e.g., '8.25.5'), pip's built-in version parsing will validate and enforce it. The current implementation aligns well with pip's version specification format and Snakemake's semantic versioning releases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Snakemake follows semantic versioning in its releases
gh api graphql -f query='
{
repository(owner: "snakemake", name: "snakemake") {
releases(first: 5, orderBy: {field: CREATED_AT, direction: DESC}) {
nodes {
tagName
}
}
}
}'
Length of output: 365
Script:
#!/bin/bash
# Check how the version parameter is used in the action
rg "snakemake-version" -A 5
Length of output: 584
52-58:
Add runner compatibility check and error handling
The current implementation assumes an Ubuntu-based runner and doesn't handle potential apt-get failures.
- name: Install Apptainer
if: ${{ inputs.install-apptainer == 'true' }}
shell: bash -el {0}
run: |
+ if ! grep -q "Ubuntu" /etc/os-release; then
+ echo "Error: Apptainer installation is only supported on Ubuntu runners"
+ exit 1
+ fi
+ if ! sudo apt-get update; then
+ echo "Error: Failed to update package list"
+ exit 1
+ fi
+ if ! sudo apt-get install -y apptainer; then
+ echo "Error: Failed to install Apptainer"
+ exit 1
+ fi
- sudo apt-get update
- sudo apt-get install -y apptainerLikely invalid or redundant comment.
71-79: 🛠️ Refactor suggestion
Add environment setup validation
The setup doesn't verify if the conda environment was successfully created and activated.
- name: Setup conda
uses: conda-incubator/setup-miniconda@v3
with:
channels: conda-forge,bioconda
channel-priority: strict
miniforge-version: latest
environment-file: .snakemake.environment.yaml
activate-environment: snakemake
+ - name: Verify conda environment
+ shell: bash -el {0}
+ run: |
+ if ! conda info --envs | grep -q "snakemake"; then
+ echo "Error: Failed to create conda environment"
+ exit 1
+ fiLikely invalid or redundant comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
action.yml (2)
37-40: Add runner compatibility warning in descriptionThe Apptainer installation currently only works on Ubuntu runners. This should be clearly documented in the input description.
install-apptainer: - description: Install Apptainer (true/false) + description: Install Apptainer (true/false). Note: Currently only supported on Ubuntu runners. required: false default: false
45-50: Improve error message clarity and consistencyThe error message uses inconsistent quote styles and could be more informative.
- echo 'Invalid value for "task": "${{ inputs.task }}". Options: "containerize", "run".' + echo "Invalid value for 'task': '${{ inputs.task }}'. Valid options are: 'containerize' or 'run'."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
action.yml(1 hunks)
🧰 Additional context used
📓 Learnings (1)
action.yml (2)
Learnt from: johanneskoester
PR: snakemake/snakemake-github-action#39
File: action.yml:70-73
Timestamp: 2024-12-09T16:41:37.740Z
Learning: In Snakemake workflows, Snakemake itself provides proper error messages during the containerization step, so additional error handling in the GitHub Action script is unnecessary.
Learnt from: tedil
PR: snakemake/snakemake-github-action#39
File: action.yml:47-48
Timestamp: 2024-12-09T10:52:48.855Z
Learning: In the `action.yml` file for `snakemake-github-action`, since `environment.yaml.template` is part of the repository and is expected to exist, adding error handling to check for its presence is unnecessary.
🔇 Additional comments (2)
action.yml (2)
74-82: 🛠️ Refactor suggestion
Add error handling for conda setup
The conda setup step should be verified to ensure the environment is properly created and activated.
Add a verification step after conda setup:
- name: Setup conda
uses: conda-incubator/setup-miniconda@v3
with:
channels: conda-forge,bioconda
channel-priority: strict
miniforge-version: latest
environment-file: .snakemake.environment.yaml
activate-environment: snakemake
+ - name: Verify conda setup
+ shell: bash -el {0}
+ run: |
+ if ! conda info | grep -q "active environment : snakemake"; then
+ echo "Error: Failed to activate conda environment"
+ exit 1
+ fiLikely invalid or redundant comment.
52-61: 🛠️ Refactor suggestion
Add comprehensive error handling for Apptainer installation
The installation step needs additional error handling for sudo access and installation verification.
- name: Install Apptainer
if: ${{ inputs.install-apptainer == 'true' }}
shell: bash -el {0}
run: |
if ! command -v apt-get &> /dev/null; then
echo "Error: This action currently supports Apptainer installation only on Ubuntu runners"
exit 1
fi
+ if ! command -v sudo &> /dev/null; then
+ echo "Error: sudo is required for Apptainer installation"
+ exit 1
+ fi
sudo apt-get update
- sudo apt-get install -y apptainer
+ if ! sudo apt-get install -y apptainer; then
+ echo "Error: Failed to install Apptainer"
+ exit 1
+ fi
+ if ! command -v apptainer &> /dev/null; then
+ echo "Error: Apptainer installation verification failed"
+ exit 1
+ fiLikely invalid or redundant comment.
| snakemake-version: | ||
| description: Snakemake version to use. If not specified, uses latest version. Pin a specific version (e.g., '8.25.5') for reproducibility. | ||
| required: false | ||
| default: '*' |
There was a problem hiding this comment.
Fix invalid conda version specifier
The default value '*' is not a valid version specifier for conda packages. When this default is used, the environment creation will fail because the conda dependency specification snakemake ==* is invalid.
Consider these options:
- Remove the default value entirely to use the latest version
- Use a specific version as default (e.g., '8.25.5')
| if: ${{ inputs.task == 'run' }} | ||
| shell: bash -el {0} | ||
| run: | | ||
| snakemake --directory ${{ inputs.directory }} --snakefile ${{ inputs.snakefile }} --show-failed-logs ${{ inputs.args }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Quote paths to handle spaces and special characters
The directory and snakefile paths should be quoted to handle spaces and special characters correctly.
- snakemake --directory ${{ inputs.directory }} --snakefile ${{ inputs.snakefile }} --show-failed-logs ${{ inputs.args }}
+ snakemake --directory "${{ inputs.directory }}" --snakefile "${{ inputs.snakefile }}" --show-failed-logs ${{ inputs.args }}- run: snakemake --directory ${{ inputs.directory }} --snakefile ${{ inputs.snakefile }} --show-failed-logs ${{ inputs.args }} --containerize > Dockerfile
+ run: snakemake --directory "${{ inputs.directory }}" --snakefile "${{ inputs.snakefile }}" --show-failed-logs ${{ inputs.args }} --containerize > DockerfileAlso applies to: 105-105
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Dockerfileandentrypoint.shscript as part of the transition to a composite action model.