feat: xonsh support#3310
feat: xonsh support#3310johanneskoester merged 11 commits intosnakemake:mainfrom jsquaredosquared:xonsh-support
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis pull request integrates support for Xonsh scripts into Snakemake. The documentation now includes a section on using Xonsh scripts along with an example rule. In the core code, a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SnakemakeEngine
participant XonshScript
participant Shell
User->>SnakemakeEngine: Start workflow execution
SnakemakeEngine->>XonshScript: Invoke execute_script with .xsh file
XonshScript->>Shell: Execute "xonsh {fname:q}" command
Shell-->>XonshScript: Return execution result
XonshScript-->>SnakemakeEngine: Return script output
SnakemakeEngine-->>User: Complete workflow execution
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
🪧 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
🧹 Nitpick comments (1)
docs/snakefiles/rules.rst (1)
1609-1628: Clarify and Enhance Xonsh Integration DocumentationThe new Xonsh section is a clear and concise addition that demonstrates how to use a Xonsh script (with the
.xshextension) via thescript:directive. It properly provides the reference link (https://xon.sh) and follows the style of other language integrations in this document.A few suggestions:
- Consider adding a brief note advising users to ensure that the Xonsh interpreter is installed in their environment. For example, you could mention “Ensure that Xonsh is available (e.g. via pip install xonsh) to run these scripts.”
- For consistency and clarity, you might use a specific rule name (e.g.
rule my_xonsh_rule:) in the sample code instead of the placeholder “NAME.” This may help users immediately recognize the example’s intent.Overall, the integration instructions are clear and align well with the PR objectives.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/test_script_xsh/expected-results/test.outis excluded by!**/*.out
📒 Files selected for processing (5)
docs/snakefiles/rules.rst(1 hunks)snakemake/script/__init__.py(3 hunks)tests/test_script_xsh/Snakefile(1 hunks)tests/test_script_xsh/envs/xonsh.yaml(1 hunks)tests/test_script_xsh/scripts/test.xsh(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/test_script_xsh/scripts/test.xsh
- tests/test_script_xsh/envs/xonsh.yaml
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest ...
**/*.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.
snakemake/script/__init__.py
🔇 Additional comments (3)
tests/test_script_xsh/Snakefile (1)
1-13: LGTM! Well-structured test workflow.The Snakefile is well-organized with proper rule definitions, relative paths for conda environment and script files, and clear output specification.
snakemake/script/__init__.py (2)
1600-1603: LGTM! Clean and focused implementation.The
XonshScriptclass follows the single responsibility principle by only overriding the necessary method to support Xonsh execution.
1667-1668: LGTM! Consistent language support integration.The changes follow the established patterns for:
- File extension recognition (
.xsh)- Executor class mapping
- Error message updates
Also applies to: 1732-1732, 1736-1736
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/test_script_xsh/Snakefile(1 hunks)tests/tests.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_script_xsh/Snakefile
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest ...
**/*.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.
tests/tests.py
🔇 Additional comments (1)
tests/tests.py (1)
425-429: LGTM! The test function follows the established pattern.The new test function
test_script_xsh()is well-placed between similar script-related tests and follows the same pattern astest_script_python()andtest_script_rs(). The decorators@skip_on_windowsand@condaare appropriately used, consistent with other script tests.
|
Thanks for your contribution. Can you fix the conflicts? |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/snakefiles/rules.rst(1 hunks)tests/tests.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/snakefiles/rules.rst
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest ...
**/*.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.
tests/tests.py
🪛 Ruff (0.8.2)
tests/tests.py
416-416: Undefined name conda
(F821)
422-422: Undefined name conda
(F821)
🔇 Additional comments (2)
tests/tests.py (2)
415-428: Enhance Xonsh script test coverageWhile adding basic support for Xonsh scripts is good, the test coverage could be expanded as suggested in previous reviews.
Currently, the basic test in
tests/tests.pyruns the valid Xonsh script intests/test_script_xsh/scripts/test.xshvia the Snakefile intests/test_script_xsh/. While this confirms that the script executes correctly under normal conditions, further tests are required to ensure robust functionality. Specifically, please consider:
- Error Handling: Add tests that deliberately invoke invalid Xonsh scripts to verify graceful error handling.
- Xonsh-Specific Behavior: Incorporate tests to assess and validate unique Xonsh syntax or features.
- Workflow Integration: Expand integration tests in the Snakefile to cover edge cases and the interplay with Snakemake's workflow system.
🧰 Tools
🪛 Ruff (0.8.2)
416-416: Undefined name
conda(F821)
422-422: Undefined name
conda(F821)
421-428:Details
❓ Verification inconclusive
New test function for Rust script support
This test is similar to the previous one but includes additional parameters for deployment method and disables MD5 checks. Like the Xonsh test, it also has the same potential issue with the
condadecorator.
🏁 Script executed:
#!/bin/bash # Examine the test directory structure to understand test requirements echo "Checking test_script_rs directory contents:" find tests/test_script_rs -type f | sort # Check if there are any other tests that use the conda decorator grep -r "@conda" --include="*.py" .Length of output: 1849
Rust Test Function: Verify
@condaDecorator BehaviorThe new test (
test_script_rs) intests/tests.pycorrectly mirrors the existing pattern seen in similar tests (e.g., the Xonsh test) by setting the deployment method toDeploymentMethod.CONDAand disabling MD5 checks. Our directory inspection confirms that thetests/test_script_rsstructure is intact, and a repository-wide search shows that the@condadecorator is used consistently across several test files.However, as with the Xonsh test, please manually verify that the behavior of the
@condadecorator (especially under conditions where it might be problematic) meets expected requirements.
- Test Directory Structure:
tests/test_script_rscontains all expected scripts, configuration, and expected result files.- Decorator Consistency: The
@condadecorator is also present in other tests (e.g., intests/tests_using_conda.pyand elsewhere intests/tests.py), indicating its widespread use.Please review the decorator’s behavior in your test environment to ensure no unintended side effects occur.
🧰 Tools
🪛 Ruff (0.8.2)
422-422: Undefined name
conda(F821)
tests/tests.py
Outdated
| @skip_on_windows | ||
| @conda | ||
| def test_script_xsh(): | ||
| run(dpath("test_script_xsh")) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New test function for Xonsh script support
The test function appropriately uses the @skip_on_windows decorator since Xonsh may have platform-specific behaviors. However, the static analyzer indicates that conda might be undefined in this context.
🏁 Script executed:
#!/bin/bash
# Check if conda decorator is properly defined in the codebase
grep -r "def conda" --include="*.py" .Length of output: 846
Action Required: Properly Define or Import the 'conda' Decorator
The new test function uses the @conda decorator, but after verification, there’s no standalone decorator definition for conda in the codebase. The only mentions of def conda appear as methods within class implementations (such as in snakemake/deployment/conda.py), and thus don’t provide the expected decorator functionality.
- Confirm whether a dedicated, top-level
condadecorator should be defined or imported. - If it should be imported (e.g., from
snakemake/deployment/conda.py), update the imports intests/tests.pyaccordingly. - Otherwise, implement a proper decorator to handle the intended behavior in the test context.
🧰 Tools
🪛 Ruff (0.8.2)
416-416: Undefined name conda
(F821)
Hi, I think they are resolved now :) |
|
I think you need to add |
I have updated the test to create a conda environment with xonsh in it. This should allow the test to pass. |
johanneskoester
left a comment
There was a problem hiding this comment.
Very nice, thanks a lot!
|
🤖 I have created a release *beep* *boop* --- ## [8.30.0](v8.29.3...v8.30.0) (2025-03-11) ### Features * Add extra input size properties ([#2424](#2424)) ([359ae2e](359ae2e)) * shell function calls inside of the 'run' directive now use conda, container, or envmodules specifications ([#2289](#2289)) ([0193e34](0193e34)) * xonsh support for script directive ([#3310](#3310)) ([d1c369b](d1c369b)) ### Bug Fixes * include unit test templates in `setup.py` ([#3362](#3362)) ([b47252c](b47252c)) ### Documentation * clearly explain report rendering to ZIP archive ([#3357](#3357)) ([948e8fb](948e8fb)) --- 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 pull request adds the ability to use Xonsh scripts ([relevant issue](snakemake#1433)). Because Xonsh is a superset of Python, I just inherited the `PythonScript` class but overrode the `execute_script` method. ### QC <!-- Make sure that you can tick the boxes below. --> * [x] The PR contains a test case for the changes or the changes are already covered by an existing test case. * [x] 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 ## Summary by CodeRabbit - **New Features** - Introduced support for using Xonsh scripts in workflows, including automatic recognition of Xonsh files and improved script execution. - **Documentation** - Extended the user guide with a new section detailing how to leverage Xonsh scripting, complete with an illustrative example. - **Tests** - Added tests with new workflow rules, environment setups, and a sample script to verify the Xonsh integration. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Filipe G. Vieira <1151762+fgvieira@users.noreply.github.com> Co-authored-by: Johannes Koester <johannes.koester@uni-due.de> Co-authored-by: Johannes Köster <johannes.koester@tu-dortmund.de>
🤖 I have created a release *beep* *boop* --- ## [8.30.0](snakemake/snakemake@v8.29.3...v8.30.0) (2025-03-11) ### Features * Add extra input size properties ([snakemake#2424](snakemake#2424)) ([359ae2e](snakemake@359ae2e)) * shell function calls inside of the 'run' directive now use conda, container, or envmodules specifications ([snakemake#2289](snakemake#2289)) ([0193e34](snakemake@0193e34)) * xonsh support for script directive ([snakemake#3310](snakemake#3310)) ([d1c369b](snakemake@d1c369b)) ### Bug Fixes * include unit test templates in `setup.py` ([snakemake#3362](snakemake#3362)) ([b47252c](snakemake@b47252c)) ### Documentation * clearly explain report rendering to ZIP archive ([snakemake#3357](snakemake#3357)) ([948e8fb](snakemake@948e8fb)) --- 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 pull request adds the ability to use Xonsh scripts (relevant issue).
Because Xonsh is a superset of Python, I just inherited the
PythonScriptclass but overrode theexecute_scriptmethod.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