fix: Add support for pathlib in notebook field#3811
fix: Add support for pathlib in notebook field#3811johanneskoester merged 3 commits intosnakemake:mainfrom
notebook field#3811Conversation
📝 WalkthroughWalkthroughConvert Path-like Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Rule evaluation
participant NotebookFn as notebook()
participant Processor as Formatting/Processing
Caller->>NotebookFn: pass notebook path (str or Path)
alt notebook is Path-like
Note right of NotebookFn: convert Path -> str
NotebookFn-->>NotebookFn: path = str(path)
end
NotebookFn->>Processor: format/process notebook path (now str)
Processor-->>Caller: continue normal flow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (7)📓 Common learnings📚 Learning: 2024-10-06T14:09:54.370ZApplied to files:
📚 Learning: 2024-10-29T17:14:13.585ZApplied to files:
📚 Learning: 2025-01-17T12:00:09.368ZApplied to files:
📚 Learning: 2025-01-13T18:50:40.791ZApplied to files:
📚 Learning: 2024-12-21T15:10:31.992ZApplied to files:
📚 Learning: 2024-10-11T13:12:35.827ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (45)
🔇 Additional comments (7)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/snakemake/notebook.py(1 hunks)tests/test_jupyter_notebook_pathlike.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:
src/snakemake/notebook.pytests/test_jupyter_notebook_pathlike.py
🧠 Learnings (1)
📚 Learning: 2024-10-29T17:14:13.585Z
Learnt from: zmbc
PR: snakemake/snakemake#2857
File: snakemake/notebook.py:84-90
Timestamp: 2024-10-29T17:14:13.585Z
Learning: In `snakemake/notebook.py`, when suggesting enhancements to the `execute_script` method in the `JupyterNotebook` class, ensure that changes align with both papermill and nbconvert use cases.
Applied to files:
src/snakemake/notebook.py
🧬 Code graph analysis (1)
tests/test_jupyter_notebook_pathlike.py (1)
tests/common.py (1)
dpath(33-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (45)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (10, macos-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (10, windows-2022, py313)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (9, windows-2022, py313)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (8, windows-2022, py313)
- GitHub Check: tests (5, windows-2022, py313)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (7, windows-2022, py313)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (6, windows-2022, py313)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (4, macos-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (3, macos-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (3, windows-2022, py313)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (2, windows-2022, py313)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: tests (1, macos-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py313)
🔇 Additional comments (2)
src/snakemake/notebook.py (1)
283-284: LGTM! Clean fix for pathlib support.The guard correctly converts Path instances to strings before the
format()call, enabling pathlib.Path objects in the notebook field without breaking existing string-based paths. The implementation handles all Path subclasses (PosixPath, WindowsPath, etc.).tests/test_jupyter_notebook_pathlike.py (1)
7-41: Well-structured test fixtures and data.The test correctly validates pathlike notebook specifications using
notebook: DIR / "process_data.ipynb"in the Snakefile. The fixture data (SNAKEFILE, ENV_YAML, NOTEBOOK) and setup are appropriate for this regression test.
|
Now all tests should pass. I consider this ready to merge. Let me know if anything else is needed from my end. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_jupyter_notebook_pathlike.py (2)
40-44: Consider simplifying the directory structure.The fixture creates a nested "testdir" subdirectory within tmpdir, which appears unnecessary. You could simplify by using tmpdir directly:
@pytest.fixture def testdir(tmpdir): - p = tmpdir.mkdir("testdir") - p.mkdir("scripts") - return p + tmpdir.mkdir("scripts") + return tmpdirThis reduces nesting while maintaining the same functionality.
58-63: Consider adding a docstring.While the test is clear, a docstring would help document its purpose:
def test_jupyter_notebook_pathlike(testdir_notebook_pathlike): """Verify that pathlib.Path objects can be used in the notebook field. Regression test for issue #3636. """ run( Path(testdir_notebook_pathlike), check_results=False, deployment_method={DeploymentMethod.CONDA}, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_jupyter_notebook_pathlike.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:
tests/test_jupyter_notebook_pathlike.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (45)
- GitHub Check: tests (9, windows-2022, py313)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (10, macos-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (10, windows-2022, py313)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (7, windows-2022, py313)
- GitHub Check: tests (8, windows-2022, py313)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (7, macos-latest, py313)
- GitHub Check: tests (6, macos-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (6, windows-2022, py313)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (4, macos-latest, py313)
- GitHub Check: tests (5, windows-2022, py313)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (3, windows-2022, py313)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (2, windows-2022, py313)
- GitHub Check: tests (1, macos-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (1, ubuntu-latest, py312)
🔇 Additional comments (4)
tests/test_jupyter_notebook_pathlike.py (4)
1-6: LGTM: Imports are correct and necessary.All imports are used appropriately in the test.
8-17: LGTM: Test case correctly exercises Path object in notebook field.The Snakefile uses
Path("scripts") / "process_data.ipynb"which creates a pathlib.Path object, directly testing the fix for issue #3636.
19-37: LGTM: Test fixtures are minimal and appropriate.The conda environment and empty notebook structure are sufficient for testing pathlib support without unnecessary complexity.
47-55: LGTM: Fixture correctly sets up the test environment.The fixture creates all necessary files (notebook, conda env, Snakefile) in the correct locations to match the workflow's expectations.
|
Regarding the nitpicks of your bot: I added the |
johanneskoester
left a comment
johanneskoester
left a comment
There was a problem hiding this comment.
You would also have to add the new test file to the test-all command in the pyproject.toml
This is totally fine. The AI is often stupid, but sometimes it catches important stuff and it helps to sort out the most severe issues before a human has a look at things. |
PR snakemake#3636 added support for Pathlike `notebook` specifications but @staadecker (the original author of that PR) did not provide a matching test. This commit add such a test based on @staadecker's example. I have verified that this test * fails (with the expected `TypeError`: ``` TypeError in file "</path/to/testdir>/Snakefile", line 7: expected str, got PosixPath ``` ) at commit 8bd1df9 (v9.6.0; parent of @staadecker's original PR snakemake#3636), * passes after applying the changes of commit 4445bb8 (@staadecker's original PR snakemake#3636), * still fails (with the same expected `TypeError`, see above) at commit 395a5e6 (current `main`), and * still passes at commit 756656d (after merging the current `main` (see above) on top of @staadecker's original PR snakemake#3636).
|
@johanneskoester: Good catch. Thank you and sorry for not noticing this myself. I force-pushed the requested change along an updated merge of the latest changes in |
|
@johanneskoester: Anything missing from my end? Do you want me to merge in current |
|
All fine, I am just slow these days! Always feel free to ping me via discord. |
🤖 I have created a release *beep* *boop* --- ## [9.14.0](v9.13.7...v9.14.0) (2025-11-27) ### Features * Support Hy in script directive ([#3824](#3824)) ([2329c9e](2329c9e)) ### Bug Fixes * Add support for pathlib in `notebook` field ([#3811](#3811)) ([7b2180a](7b2180a)) * Addressed race condition in workdir_handler.py ([#3844](#3844)) ([8dbfcfb](8dbfcfb)) * cleanup update-marked output files of failed jobs if there was no backup to restore them ([#3843](#3843)) ([41f1ce8](41f1ce8)) * correct Windows callable path handling ([#3832](#3832)) ([5caad70](5caad70)) * expand env vars on resources ([#3823](#3823)) ([fcfa1bc](fcfa1bc)) * fix backup for output marked by `update` ([#3839](#3839)) ([09c64b7](09c64b7)) * Minor fixes/additions to logging module. ([#3802](#3802)) ([3b3986d](3b3986d)) * mount local storage prefix into containers ([#3840](#3840)) ([f1e8b62](f1e8b62)) * properly format input/output files in case of missing rule to produce them ([#3849](#3849)) ([69d5d24](69d5d24)) * Unpack AnnotatedString in _apply_wildcards ([#3798](#3798)) ([7886508](7886508)) ### Performance Improvements * retrieve storage inputs immediately before scheduling jobs instead of before running the entire workflow ([#3850](#3850)) ([4ac6cda](4ac6cda)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
PR snakemake#3636 added support for Pathlike `notebook` specifications but @staadecker (the original author of that PR) did not provide a matching test. This PR superseeds @staadecker's original PR snakemake#3636 by merging in current `main` and adding a commit that adds such a test based on @staadecker's example. I have verified that this test * fails (with the expected `TypeError`: ``` TypeError in file "</path/to/testdir>/Snakefile", line 7: expected str, got PosixPath ``` ) at commit 8bd1df9 (v9.6.0; parent of @staadecker's original PR snakemake#3636), * passes after applying the changes of commit 4445bb8 (@staadecker's original PR snakemake#3636), * still fails (with the same expected `TypeError`, see above) at commit 395a5e6 (current `main`), and * still passes at commit 756656d merging the current `main` (see above) on top of @staadecker's original PR snakemake#3636). --- closes snakemake#3636 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Ensure pathlike notebook resources are converted to strings before processing to avoid inconsistent handling. * **Tests** * Added test coverage verifying Jupyter notebook pathlike object support with Conda deployment. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Martin <[email protected]>
🤖 I have created a release *beep* *boop* --- ## [9.14.0](snakemake/snakemake@v9.13.7...v9.14.0) (2025-11-27) ### Features * Support Hy in script directive ([snakemake#3824](snakemake#3824)) ([2329c9e](snakemake@2329c9e)) ### Bug Fixes * Add support for pathlib in `notebook` field ([snakemake#3811](snakemake#3811)) ([7b2180a](snakemake@7b2180a)) * Addressed race condition in workdir_handler.py ([snakemake#3844](snakemake#3844)) ([8dbfcfb](snakemake@8dbfcfb)) * cleanup update-marked output files of failed jobs if there was no backup to restore them ([snakemake#3843](snakemake#3843)) ([41f1ce8](snakemake@41f1ce8)) * correct Windows callable path handling ([snakemake#3832](snakemake#3832)) ([5caad70](snakemake@5caad70)) * expand env vars on resources ([snakemake#3823](snakemake#3823)) ([fcfa1bc](snakemake@fcfa1bc)) * fix backup for output marked by `update` ([snakemake#3839](snakemake#3839)) ([09c64b7](snakemake@09c64b7)) * Minor fixes/additions to logging module. ([snakemake#3802](snakemake#3802)) ([3b3986d](snakemake@3b3986d)) * mount local storage prefix into containers ([snakemake#3840](snakemake#3840)) ([f1e8b62](snakemake@f1e8b62)) * properly format input/output files in case of missing rule to produce them ([snakemake#3849](snakemake#3849)) ([69d5d24](snakemake@69d5d24)) * Unpack AnnotatedString in _apply_wildcards ([snakemake#3798](snakemake#3798)) ([7886508](snakemake@7886508)) ### Performance Improvements * retrieve storage inputs immediately before scheduling jobs instead of before running the entire workflow ([snakemake#3850](snakemake#3850)) ([4ac6cda](snakemake@4ac6cda)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
3 participants
PR #3636 added support for Pathlike
notebookspecifications but @staadecker (the original author of that PR) did not provide a matching test.This PR superseeds @staadecker's original PR #3636 by merging in current
mainand adding a commit that adds such a test based on @staadecker's example.I have verified that this test
fails (with the expected
TypeError:) at commit 8bd1df9 (v9.6.0; parent of @staadecker's original PR fix: Add support for pathlib in
notebookfield #3636),passes after applying the changes of commit 4445bb8 (@staadecker's original PR fix: Add support for pathlib in
notebookfield #3636),still fails (with the same expected
TypeError, see above) at commit 395a5e6 (currentmain), andstill passes at commit 756656d merging the current
main(see above) on top of @staadecker's original PR fix: Add support for pathlib innotebookfield #3636).closes #3636
Summary by CodeRabbit
Bug Fixes
Tests