fix: clean env vars in apptainer#3199
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve a modification to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
snakemake/deployment/singularity.py (1)
Line range hint
1-130: Documentation update needed for --cleanenv behaviorThe addition of --cleanenv is a behavioral change that affects environment variable handling. This should be documented to help users understand:
- Host environment variables are no longer passed to containers
- How to explicitly pass environment variables using the
envvarsparameterWould you like me to help draft the documentation updates?
Also applies to: 131-238
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
snakemake/deployment/singularity.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
snakemake/deployment/singularity.py (1)
Pattern **/*.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 self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
🔇 Additional comments (1)
snakemake/deployment/singularity.py (1)
130-130: LGTM! The --cleanenv flag addition improves container isolation.
The change correctly addresses the issue of host environment variables leaking into the container, which enhances reproducibility of Snakemake workflows.
Let's verify compatibility with different singularity/apptainer versions:
✅ Verification successful
LGTM! The --cleanenv flag addition improves container isolation.
The change correctly addresses the issue of host environment variables leaking into the container, enhancing the reproducibility of Snakemake workflows.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if --cleanenv flag is supported across versions
# Search for any version-specific handling of --cleanenv flag
rg -A 5 "cleanenv|SINGULARITYENV_" .
# Check if there are any existing version checks for command flags
ast-grep --pattern 'if self.version $$$cleanenv$$$'
Length of output: 932
|
🤖 I have created a release *beep* *boop* --- ## [8.25.4](v8.25.3...v8.25.4) (2024-11-27) ### Bug Fixes * clean env vars in apptainer ([#3199](#3199)) ([76d5329](76d5329)) * ensure that intermediate files in job groups do not cause spurious mtime errors when checking for consistency with output files ([#3220](#3220)) ([4ba2bdf](4ba2bdf)) * Remove incomplete marker also when drop-metadata is active ([#3215](#3215)) ([a4f2e5c](a4f2e5c)) * Remove incomplete marker for job finished only after metadata is written ([#3197](#3197)) ([6567e5f](6567e5f)) * Support versioned URLs in Asset class and fix missing versions in Snakemake report ([#3203](#3203)) ([f086f6c](f086f6c)) * update rust-script usage to recent version (v0.35.0) [#3183](#3183) ([#3208](#3208)) ([43885d7](43885d7)) ### Documentation * clarify continuously updated input section ([#3219](#3219)) ([72a6994](72a6994)) * Fix typo in CHANGELOG.md ([#3198](#3198)) ([0e445ed](0e445ed)) * refer to Merkle trees instead of "blockchain" in caching.rst ([#3216](#3216)) ([282e5d9](282e5d9)) * remove twitter in favor of bluesky and mastodon ([#3217](#3217)) ([231c6df](231c6df)) * use "dictionary" not "array" wording in config docs ([#3156](#3156)) ([17aed41](17aed41)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
Dear @laf070810, @johanneskoester I am having trouble with this change, because I have the scenario that the container needs environment variables from the host. E.g. I am consuming information like a SLURM_JOB_ID and in order to be able to run multi-node Apptainer jobs via Slurm`s PMI2 interface. Is it really necessary to hardcode this into the command? After all, it is not the default behavior for Apptainer, so why should it be forced by a downstream tool, like Snakemake in this case. Is it not possible to achieve this via Thanks for the feedback! |
|
Dear @laf070810, have you had a chance to test the proposed approach of just adding Thanks a lot! |
|
OK, no comment, I'll ask for reverting via PR. |
I did some tests and it seems OK to use just |
|
Good to know this works for you. Reverting the change in PR together with a comment in the docs. |
The apptainer command used in snakemake now does not have
--cleanenv(or-e) option. The envinronment variables in the host (e.g. PYTHONPATH or CONDA_EXE) will be passed to the container. This may affect reproducibility and even cause some errors. This PR proposes adding the--cleanenvoption to the apptainer commmand to avoid passing host environment variables.QC
docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).Summary by CodeRabbit
--cleanenvoption, ensuring a clean environment for better handling of environment variables during execution.