fix: Revert cleaning of env vars in apptainer#3285
fix: Revert cleaning of env vars in apptainer#3285johanneskoester merged 3 commits intosnakemake:mainfrom
Conversation
📝 WalkthroughWalkthroughThe update modifies the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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 (
|
|
To any maintainer reviewing this, I was considering adding a hint on this to the documentation. However, other than in the command line interface documentation, there is no information on Apptainer. Should there be an FAQ? Should there be a general hint on system environment variables here? Happy to write a few sentences, let me know where. |
|
There is a small section here. Do you think it would make sense to add some lines here? |
johanneskoester
left a comment
There was a problem hiding this comment.
Thanks a lot! I agree that it would be nice to have a small hint in the respective section of the docs. Would you mind adding that? (here is the source: https://github.com/snakemake/snakemake/blob/main/docs/snakefiles/deployment.rst)
|
And could you please reformat the source file with black? |
…ith Apptainer/Singularity
|
I did add a hint. However, what felt a bit inconsistent is that there is not way to pass arguments when using Docker. Or is there? |
I did over and over and it didn't change anything until I realized that it is actually a file that I didn't touch: snakemake/snakemake/io.py I changed it anyway. Out of curiosity: Is always the entire code checked or just the diffs in each pull request? |
|
The entire code is checked for correct format. |
|
I'd be happy about a release as well in case you are not planning to do so directly anyway :-) Not sure what the rule is for a new release... |
🤖 I have created a release *beep* *boop* --- ## [8.28.0](v8.27.1...v8.28.0) (2025-02-12) ### Features * do not fail if --cache is active but no SNAKEMAKE_OUTPUT_CACHE env var is defined. Instead, print a warning that explains the options. ([#3270](#3270)) ([9610f7c](9610f7c)) ### Bug Fixes * do not use outdated metadata for rerun triggers (only warn about it) ([#3259](#3259)) ([d766a48](d766a48)) * ensure that exceptions print storage queries instead of local copies of remote files ([#3258](#3258)) ([e5d8ec1](e5d8ec1)) * fix error message of evaluate helper function ([#3282](#3282)) ([9483a64](9483a64)) * Revert cleaning of env vars in apptainer ([#3285](#3285)) ([e79a51b](e79a51b)) ### Performance Improvements * compare checksums of input files <= 1MB (before (10KB) ([#3267](#3267)) ([ba017bb](ba017bb)) * query updated input files in parallel ([#3266](#3266)) ([bc4fcee](bc4fcee)) ### Documentation * Adds instructions for using syntax highlighting with lazy.nvim ([#3246](#3246)) ([7a75043](7a75043)) * Fix typos in basic API example ([#3277](#3277)) ([8782219](8782219)) --- 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>
<!--Add a description of your PR here--> This PR reverts the change introduced by 76d5329 which, when using Apptainer/Singularity, forces cleaning of system environment variables going into the container. This is neither a fixed nor default behavior of Apptainer itself since it can be desirable to not fully isolate from the system. Hence Snakemake shouldn't dictate it. With the change in 76d5329, users do not have any chance to deactivate the behavior. On the other hand, the `--cleanenv` can easily be added through `--aptainer-args`. ### QC <!-- Make sure that you can tick the boxes below. --> * [ ] The PR contains a test case for the changes or the changes are already covered by an existing test case. * [ ] The documentation (`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). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Chores** - Updated container execution behavior by removing the `--cleanenv` option, allowing host environment variables to be passed through to container tasks for a more consistent execution context. - **Documentation** - Enhanced documentation on Snakemake workflow structure, adding requirements for a "Code of Conduct," "Contribution instructions," and a "Test directory." - Clarified the importance of maintaining a `.tests` directory and specified subdirectories for integration and unit tests. - Updated repository structure requirements to include an SVG-formatted rule graph and technical documentation file. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
🤖 I have created a release *beep* *boop* --- ## [8.28.0](snakemake/snakemake@v8.27.1...v8.28.0) (2025-02-12) ### Features * do not fail if --cache is active but no SNAKEMAKE_OUTPUT_CACHE env var is defined. Instead, print a warning that explains the options. ([snakemake#3270](snakemake#3270)) ([9610f7c](snakemake@9610f7c)) ### Bug Fixes * do not use outdated metadata for rerun triggers (only warn about it) ([snakemake#3259](snakemake#3259)) ([d766a48](snakemake@d766a48)) * ensure that exceptions print storage queries instead of local copies of remote files ([snakemake#3258](snakemake#3258)) ([e5d8ec1](snakemake@e5d8ec1)) * fix error message of evaluate helper function ([snakemake#3282](snakemake#3282)) ([9483a64](snakemake@9483a64)) * Revert cleaning of env vars in apptainer ([snakemake#3285](snakemake#3285)) ([e79a51b](snakemake@e79a51b)) ### Performance Improvements * compare checksums of input files <= 1MB (before (10KB) ([snakemake#3267](snakemake#3267)) ([ba017bb](snakemake@ba017bb)) * query updated input files in parallel ([snakemake#3266](snakemake#3266)) ([bc4fcee](snakemake@bc4fcee)) ### Documentation * Adds instructions for using syntax highlighting with lazy.nvim ([snakemake#3246](snakemake#3246)) ([7a75043](snakemake@7a75043)) * Fix typos in basic API example ([snakemake#3277](snakemake#3277)) ([8782219](snakemake@8782219)) --- 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>
This PR reverts the change introduced by 76d5329 which, when using Apptainer/Singularity, forces cleaning of system environment variables going into the container. This is neither a fixed nor default behavior of Apptainer itself since it can be desirable to not fully isolate from the system. Hence Snakemake shouldn't dictate it. With the change in 76d5329, users do not have any chance to deactivate the behavior. On the other hand, the
--cleanenvcan easily be added through--aptainer-args.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
Chores
--cleanenvoption, allowing host environment variables to be passed through to container tasks for a more consistent execution context.Documentation
.testsdirectory and specified subdirectories for integration and unit tests.