-
Notifications
You must be signed in to change notification settings - Fork 8
feat: add rule-specific container configuration option (#30) #36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughImplements per-rule container image overrides for AWS Batch jobs via an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/further.md (1)
31-48: Consider documenting the container image constraint.The documentation clearly explains the feature and provides a helpful example. However, per the PR description, custom container images must be based on the Snakemake Docker image. Consider adding a note about this constraint to help users avoid runtime errors.
Example addition:
This allows you to use different containers with specialized tools for different rules within the same workflow, rather than requiring all tools to be present in a single container. + +**Note:** Custom container images must be based on the Snakemake Docker image to ensure compatibility with the executor plugin's job execution requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/further.md(1 hunks)snakemake_executor_plugin_aws_batch/__init__.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.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.
Files:
snakemake_executor_plugin_aws_batch/__init__.py
🧬 Code graph analysis (1)
snakemake_executor_plugin_aws_batch/__init__.py (1)
snakemake_executor_plugin_aws_batch/batch_job_builder.py (1)
BatchJobBuilder(14-136)
🔇 Additional comments (1)
snakemake_executor_plugin_aws_batch/__init__.py (1)
135-145: LGTM! Clean implementation of per-rule container image override.The implementation correctly retrieves the rule-specific container image from job resources with a proper fallback to the global container image. The use of
dict.get()with a default value is the appropriate pattern, and the integration withBatchJobBuilderis correct.
This PR adds a resource configuration item
aws_batch_container_imagethat can be used to customize the container any given rule uses when running on AWS Batch. As discussed in #30 and #31, currently it's possible to specify a container image that every rule in a workflow uses, and if you want to run an individual rule in a different container you have use apptainer, with some extra hoops if you need to authenticate with a private registry (eg. ECR).There is a bit of a catch however - you need to base your image on the snakemake docker image. I don't think this is something that would be easy to work around and probably not a good fit with snakemake's design. I can update the documentation if this is going to be merged.
Summary by CodeRabbit
Release Notes
New Features
Documentation