feat: add semantic helper functions choose_file/choose_folder/choose_tmp and allow all semantic helper functions to be used when parsing resources#3820
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new helper choose_tmp that selects a creatable temporary directory, exposes it via snakemake.ioutils globals, integrates ioutils into resource-expression evaluation so expressions can call ioutils helpers, and updates tests, a test profile, documentation, and CI config. Changes
Sequence DiagramsequenceDiagram
participant Expr as Resource expression
participant Res as eval_resource_expression
participant IOU as snakemake.ioutils
participant Tmp as choose_tmp
participant FS as Filesystem
Expr->>Res: request evaluation of expression
Res->>IOU: import snakemake.ioutils
IOU->>IOU: register_in_globals(ioutils_dict)
Res->>Res: eval(expression, globals=ioutils_dict, locals=provided_locals)
Expr->>Tmp: call choose_tmp(tmpdirs_list)
loop for each tmpdir candidate
Tmp->>FS: is_dir_creatable checks (parent exists, dir status, perms)
alt creatable
Tmp-->>Expr: return chosen tmpdir
else not creatable
Tmp->>Tmp: try next candidate
end
end
alt none creatable
Tmp-->>Expr: return "system_tmpdir"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/snakemake/ioutils/tmp.py (2)
5-5: Specify the element type in the List type hint.The type hint
Listshould beList[str]to indicate that the function expects a list of string paths.Apply this diff:
-def get_tmp(tmpdirs: List) -> str: +def get_tmp(tmpdirs: List[str]) -> str:
28-32: Simplify the loop by removing redundant else clause.The
else: continuestatement is redundant since the loop will naturally continue to the next iteration.Apply this diff:
for tmpdir in tmpdirs: if is_dir_creatable(Path(tmpdir)): return tmpdir - else: - continue
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/snakemake/ioutils/__init__.py(2 hunks)src/snakemake/ioutils/tmp.py(1 hunks)src/snakemake/resources.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/ioutils/tmp.pysrc/snakemake/resources.pysrc/snakemake/ioutils/__init__.py
🧠 Learnings (2)
📚 Learning: 2024-12-14T13:11:32.988Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 2927
File: snakemake/ioutils/evaluate.py:11-11
Timestamp: 2024-12-14T13:11:32.988Z
Learning: In `snakemake/ioutils/evaluate.py`, the use of `eval()` with `globals()` is acceptable because it's called by the user under consideration of the implications.
Applied to files:
src/snakemake/resources.py
📚 Learning: 2025-08-28T13:17:37.693Z
Learnt from: Hocnonsense
Repo: snakemake/snakemake PR: 3714
File: src/snakemake/workflow.py:637-665
Timestamp: 2025-08-28T13:17:37.693Z
Learning: In Snakemake, rule.module_globals["__name__"] always exists and defaults to "snakemake.workflow" when not modified, so direct dictionary access is safe without needing .get().
Applied to files:
src/snakemake/ioutils/__init__.py
🧬 Code graph analysis (2)
src/snakemake/resources.py (1)
src/snakemake/ioutils/__init__.py (1)
register_in_globals(12-31)
src/snakemake/ioutils/__init__.py (1)
src/snakemake/ioutils/tmp.py (1)
get_tmp(5-33)
⏰ 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). (46)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (10, windows-2022, py313)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (9, windows-2022, py313)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (7, windows-2022, py313)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (8, macos-latest, py313)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (8, windows-2022, py313)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (6, macos-latest, py313)
- GitHub Check: tests (6, windows-2022, py313)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (5, windows-2022, py313)
- GitHub Check: tests (3, windows-2022, py313)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (3, macos-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (2, windows-2022, py313)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (1, macos-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: apidocs
🔇 Additional comments (2)
src/snakemake/resources.py (1)
534-544: LGTM! Clean integration of ioutils into resource evaluation.The implementation correctly registers ioutils functions into the global namespace for eval, enabling resource expressions to access utilities like
get_tmp. The comment accurately explains precedence behavior.src/snakemake/ioutils/__init__.py (1)
9-9: LGTM! Consistent API extension.The import and registration of
get_tmpfollows the established pattern for exposing ioutils functions.Also applies to: 29-29
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/snakemake/ioutils/tmp.py (1)
33-33: Return actual system temp directory, not a placeholder string.When
get_tmpis called within a resource expression viaeval(), returning the literal string"system_tmpdir"will not trigger a lookup in the evaluation context. The function should return the actual system temporary directory path.Apply this diff:
+ import tempfile + for tmpdir in tmpdirs: if is_dir_creatable(Path(tmpdir)): return tmpdir - return "system_tmpdir" + return tempfile.gettempdir()
🧹 Nitpick comments (1)
src/snakemake/ioutils/tmp.py (1)
31-32: Remove redundant else clause.The
else: continuestatement is unnecessary since the loop will continue by default when theifcondition is false.Apply this diff:
for tmpdir in tmpdirs: if is_dir_creatable(Path(tmpdir)): return tmpdir - else: - continue return "system_tmpdir"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/snakefiles/rules.rst(1 hunks)src/snakemake/ioutils/tmp.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/ioutils/tmp.py
🧠 Learnings (3)
📚 Learning: 2024-12-14T13:11:32.988Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 2927
File: snakemake/ioutils/evaluate.py:11-11
Timestamp: 2024-12-14T13:11:32.988Z
Learning: In `snakemake/ioutils/evaluate.py`, the use of `eval()` with `globals()` is acceptable because it's called by the user under consideration of the implications.
Applied to files:
src/snakemake/ioutils/tmp.py
📚 Learning: 2024-10-06T14:09:54.370Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in `tests/test_wrapper/Snakefile`, are for testing purposes and do not require updates to the project documentation.
Applied to files:
docs/snakefiles/rules.rst
📚 Learning: 2024-12-14T13:10:48.450Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 2927
File: tests/test_subpath/Snakefile:11-11
Timestamp: 2024-12-14T13:10:48.450Z
Learning: In Snakemake, when a rule defines a single output file (even if the output is specified as a dictionary), `{output}` in the shell command can be used directly to refer to that file.
Applied to files:
docs/snakefiles/rules.rst
⏰ 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). (46)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (9, windows-2022, py313)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (8, windows-2022, py313)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (10, windows-2022, py313)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (7, windows-2022, py313)
- GitHub Check: tests (5, macos-latest, py313)
- GitHub Check: tests (6, macos-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (5, windows-2022, py313)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (6, windows-2022, py313)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (3, windows-2022, py313)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (3, macos-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (2, windows-2022, py313)
- GitHub Check: tests (1, macos-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: apidocs
🔇 Additional comments (3)
docs/snakefiles/rules.rst (3)
791-792: LGTM!Header formatting update is consistent with documentation style.
799-809: Clear documentation of the get_tmp feature.The documentation clearly explains the use case and provides a practical example. The description of falling back to the system temp directory is accurate for the intended behavior.
813-814: LGTM!Header formatting update maintains documentation consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/snakemake/ioutils/tmp.py (1)
33-33: Return actual system temp directory path.The function currently returns the literal string
"system_tmpdir", which won't resolve to a usable path when evaluated in resource expressions. This should return the actual system temporary directory.Apply this diff to fix:
+ import tempfile + for tmpdir in tmpdirs: if is_dir_creatable(Path(tmpdir)): return tmpdir - return "system_tmpdir" + return tempfile.gettempdir()
🧹 Nitpick comments (1)
src/snakemake/ioutils/tmp.py (1)
28-32: Optional: Remove redundant else clause.The
else: continueis implicit in a for loop and can be removed for cleaner code.Apply this diff if desired:
for tmpdir in tmpdirs: if is_dir_creatable(Path(tmpdir)): return tmpdir - else: - continue
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/snakefiles/rules.rst(1 hunks)src/snakemake/ioutils/__init__.py(2 hunks)src/snakemake/ioutils/tmp.py(1 hunks)tests/test_tmpdir/Snakefile(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
⚙️ 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/ioutils/tmp.pysrc/snakemake/ioutils/__init__.py
🧠 Learnings (7)
📚 Learning: 2024-12-14T13:11:32.988Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 2927
File: snakemake/ioutils/evaluate.py:11-11
Timestamp: 2024-12-14T13:11:32.988Z
Learning: In `snakemake/ioutils/evaluate.py`, the use of `eval()` with `globals()` is acceptable because it's called by the user under consideration of the implications.
Applied to files:
src/snakemake/ioutils/tmp.py
📚 Learning: 2025-08-28T13:17:37.693Z
Learnt from: Hocnonsense
Repo: snakemake/snakemake PR: 3714
File: src/snakemake/workflow.py:637-665
Timestamp: 2025-08-28T13:17:37.693Z
Learning: In Snakemake, rule.module_globals["__name__"] always exists and defaults to "snakemake.workflow" when not modified, so direct dictionary access is safe without needing .get().
Applied to files:
src/snakemake/ioutils/__init__.py
📚 Learning: 2024-10-06T14:09:54.370Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in `tests/test_wrapper/Snakefile`, are for testing purposes and do not require updates to the project documentation.
Applied to files:
tests/test_tmpdir/Snakefile
📚 Learning: 2024-12-21T15:10:31.992Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 2925
File: tests/test_nonstr_params/Snakefile:14-15
Timestamp: 2024-12-21T15:10:31.992Z
Learning: The file "test.jsonl" in tests/test_nonstr_params is automatically created by Snakemake, rather than manually generated in the Snakefile.
Applied to files:
tests/test_tmpdir/Snakefile
📚 Learning: 2024-08-13T16:09:09.726Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 3014
File: snakemake/dag.py:1265-1266
Timestamp: 2024-08-13T16:09:09.726Z
Learning: For the Snakemake project, avoid suggesting trailing commas as they use Black as an autoformatter.
Applied to files:
tests/test_tmpdir/Snakefile
📚 Learning: 2025-01-17T12:00:09.368Z
Learnt from: leoschwarz
Repo: snakemake/snakemake PR: 3176
File: tests/test_output_index.py:99-157
Timestamp: 2025-01-17T12:00:09.368Z
Learning: New test dependencies for Snakemake should be introduced in separate PRs rather than being added as part of feature or refactoring PRs.
Applied to files:
tests/test_tmpdir/Snakefile
📚 Learning: 2024-12-14T13:10:48.450Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 2927
File: tests/test_subpath/Snakefile:11-11
Timestamp: 2024-12-14T13:10:48.450Z
Learning: In Snakemake, when a rule defines a single output file (even if the output is specified as a dictionary), `{output}` in the shell command can be used directly to refer to that file.
Applied to files:
tests/test_tmpdir/Snakefile
🧬 Code graph analysis (1)
src/snakemake/ioutils/__init__.py (1)
src/snakemake/ioutils/tmp.py (1)
choose_tmp(5-33)
⏰ 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). (46)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (10, windows-2022, py313)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (9, windows-2022, py313)
- GitHub Check: tests (8, windows-2022, py313)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (7, windows-2022, py313)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (5, windows-2022, py313)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (3, macos-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (6, windows-2022, py313)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (3, windows-2022, py313)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (6, macos-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (5, macos-latest, py313)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (2, windows-2022, py313)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, macos-latest, py313)
- GitHub Check: apidocs
🔇 Additional comments (3)
tests/test_tmpdir/Snakefile (1)
14-14: LGTM! Test correctly validates fallback behavior.The test appropriately exercises
choose_tmpby providing a non-existent path first, ensuring the function falls back to the second option.src/snakemake/ioutils/__init__.py (1)
9-9: LGTM! Proper public API exposure.The import and registration of
choose_tmpfollow the established pattern for exposing ioutils functions.Also applies to: 29-29
src/snakemake/ioutils/tmp.py (1)
26-26: Good fix on the recursive return.The past review correctly identified that the recursive call needed an explicit
returnstatement, which has been properly addressed.
|
Why not just relying on the tmpdir resource, which can be configured by the admins or the user via the standard tmpdir env variable? |
|
The issue is when HPC have different hardware configurations. In my case, some have a fast The best solution would definitely be to have the admins define the The idea here is to define a helper function that can be passed to This function would be executed at runtime and, depending on the node, it would select the first folder that meets certain criteria to be a tmpdir. |
Co-authored-by: Johannes Köster <johannes.koester@uni-due.de>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/snakemake/ioutils/tmp.py (2)
12-26: Missing return statement breaks recursive directory check.Line 26 calls
is_dir_creatable(dir.parent)recursively but doesn't return the result, causing the function to implicitly returnNoneinstead of propagating the boolean result from parent directory checks.Apply this diff:
else: - return is_dir_creatable(dir.parent) + return is_dir_creatable(dir.parent)
33-33: Return actual system temp directory, not the string placeholder.When called within a resource expression via
eval(), returning the string"system_tmpdir"will not trigger a lookup in the evaluation context. The function should return the actual system temporary directory path.Apply this diff:
+ import tempfile + for tmpdir in tmpdirs: if is_dir_creatable(Path(tmpdir)): return tmpdir - return "system_tmpdir" + return tempfile.gettempdir()
🧹 Nitpick comments (1)
src/snakemake/ioutils/tmp.py (1)
28-32: Remove redundantelse: continueclause.The
else: continueon lines 31-32 is redundant since the loop naturally continues when the condition is not met.Apply this diff:
for tmpdir in tmpdirs: if is_dir_creatable(Path(tmpdir)): return tmpdir - else: - continue
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/snakemake/ioutils/tmp.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/ioutils/tmp.py
🧠 Learnings (1)
📚 Learning: 2024-12-14T13:11:32.988Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 2927
File: snakemake/ioutils/evaluate.py:11-11
Timestamp: 2024-12-14T13:11:32.988Z
Learning: In `snakemake/ioutils/evaluate.py`, the use of `eval()` with `globals()` is acceptable because it's called by the user under consideration of the implications.
Applied to files:
src/snakemake/ioutils/tmp.py
🪛 Ruff (0.14.7)
src/snakemake/ioutils/tmp.py
5-5: Undefined name Union
(F821)
⏰ 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). (16)
- GitHub Check: tests (10, windows-2022, py313)
- GitHub Check: tests (8, macos-latest, py313)
- GitHub Check: tests (9, windows-2022, py313)
- GitHub Check: tests (7, windows-2022, py313)
- GitHub Check: tests (8, windows-2022, py313)
- GitHub Check: tests (7, macos-latest, py313)
- GitHub Check: tests (6, windows-2022, py313)
- GitHub Check: tests (5, macos-latest, py313)
- GitHub Check: tests (5, windows-2022, py313)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (4, macos-latest, py313)
- GitHub Check: tests (3, macos-latest, py313)
- GitHub Check: tests (3, windows-2022, py313)
- GitHub Check: tests (2, windows-2022, py313)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: apidocs
🔇 Additional comments (1)
src/snakemake/ioutils/tmp.py (1)
5-10: Verify environment variable expansion is handled.The PR objectives mention example usage like
choose_tmp(["/fast_nvme", "/home/$USER/scratch", "/scratch/$SLURM_JOB_ID"])with environment variables, but the current implementation doesn't expand variables like$USERor$SLURM_JOB_ID. Please verify that variable expansion is handled at an earlier stage (e.g., during resource expression evaluation) or consider usingos.path.expandvars()on each tmpdir path before checking creatability.If expansion is needed here, apply this diff:
+ import os + def is_dir_creatable(dir: Path) -> bool: - import os if dir == dir.parent: return FalseAnd update the loop:
for tmpdir in tmpdirs: - if is_dir_creatable(Path(tmpdir)): - return tmpdir + expanded = os.path.expandvars(str(tmpdir)) + if is_dir_creatable(Path(expanded)): + return expanded
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/snakemake/ioutils/tmp.py (1)
33-33: Return actual system temp directory, not string placeholder.When used in resource expressions via
eval(), returning the literal string"system_tmpdir"will not resolve to an actual path. The function should return the actual system temporary directory.Apply this diff:
+ import tempfile + for tmpdir in tmpdirs: if is_dir_creatable(Path(tmpdir)): return tmpdir - else: - continue - return "system_tmpdir" + return tempfile.gettempdir()
🧹 Nitpick comments (1)
src/snakemake/ioutils/tmp.py (1)
32-32: Optional: Remove redundant continue.The
continuestatement is redundant at the end of the loop body.Apply this diff:
if is_dir_creatable(Path(tmpdir)): return tmpdir - else: - continue
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/snakemake/ioutils/tmp.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/ioutils/tmp.py
🧠 Learnings (1)
📚 Learning: 2024-12-14T13:11:32.988Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 2927
File: snakemake/ioutils/evaluate.py:11-11
Timestamp: 2024-12-14T13:11:32.988Z
Learning: In `snakemake/ioutils/evaluate.py`, the use of `eval()` with `globals()` is acceptable because it's called by the user under consideration of the implications.
Applied to files:
src/snakemake/ioutils/tmp.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). (47)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (7, windows-2022, py313)
- GitHub Check: tests (10, windows-2022, py313)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (6, windows-2022, py313)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (8, windows-2022, py313)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (9, windows-2022, py313)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (5, macos-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (8, macos-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (4, macos-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (3, windows-2022, py313)
- GitHub Check: tests (5, windows-2022, py313)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (3, macos-latest, py313)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (2, windows-2022, py313)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: formatting
- GitHub Check: apidocs
🔇 Additional comments (3)
src/snakemake/ioutils/tmp.py (3)
1-2: LGTM! Import issue resolved.The
Unionimport has been added as recommended in previous reviews. All necessary imports are now present.
5-10: LGTM!Function signature and docstring are clear and correctly typed.
12-26: LGTM! Recursive return fixed.The missing return statement on line 26 has been corrected. The helper function now properly propagates the boolean result from parent directory checks.
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 (4)
docs/snakefiles/rules.rst(3 hunks)tests/test_tmpdir/Snakefile(1 hunks)tests/test_tmpdir/profile/config.yaml(1 hunks)tests/tests.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/test_tmpdir/profile/config.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_tmpdir/Snakefile
🧰 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/tests.py
🧠 Learnings (5)
📚 Learning: 2024-10-06T14:09:54.370Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in `tests/test_wrapper/Snakefile`, are for testing purposes and do not require updates to the project documentation.
Applied to files:
docs/snakefiles/rules.rsttests/tests.py
📚 Learning: 2024-12-14T13:10:48.450Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 2927
File: tests/test_subpath/Snakefile:11-11
Timestamp: 2024-12-14T13:10:48.450Z
Learning: In Snakemake, when a rule defines a single output file (even if the output is specified as a dictionary), `{output}` in the shell command can be used directly to refer to that file.
Applied to files:
docs/snakefiles/rules.rst
📚 Learning: 2025-10-30T15:39:38.224Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 3813
File: tests/test_pathvars/Snakefile:22-23
Timestamp: 2025-10-30T15:39:38.224Z
Learning: In Snakemake, `<resources>` is a default pathvar that does not need to be explicitly defined in the pathvars section. It is available for use in rules and functions without requiring declaration.
Applied to files:
docs/snakefiles/rules.rst
📚 Learning: 2025-01-17T12:00:09.368Z
Learnt from: leoschwarz
Repo: snakemake/snakemake PR: 3176
File: tests/test_output_index.py:99-157
Timestamp: 2025-01-17T12:00:09.368Z
Learning: New test dependencies for Snakemake should be introduced in separate PRs rather than being added as part of feature or refactoring PRs.
Applied to files:
tests/tests.py
📚 Learning: 2024-12-21T15:10:31.992Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 2925
File: tests/test_nonstr_params/Snakefile:14-15
Timestamp: 2024-12-21T15:10:31.992Z
Learning: The file "test.jsonl" in tests/test_nonstr_params is automatically created by Snakemake, rather than manually generated in the Snakefile.
Applied to files:
tests/tests.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). (46)
- GitHub Check: apidocs
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (10, windows-2022, py313)
- GitHub Check: tests (9, macos-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (8, macos-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (9, windows-2022, py313)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (7, windows-2022, py313)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (8, windows-2022, py313)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (7, macos-latest, py313)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (6, macos-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (6, windows-2022, py313)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (4, macos-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (5, windows-2022, py313)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py313)
- 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, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (2, windows-2022, py313)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py312)
🔇 Additional comments (3)
docs/snakefiles/rules.rst (2)
1112-1119: Documentation of choose_tmp feature addresses prior reviewer feedback well.The section now properly:
- Places the example in a profile's
default_resourcesconfiguration (addressing the feedback to mention profile usage)- Explains runtime evaluation clearly without misleading about when the function executes
- Locates the content in the Dynamic Resources section under Resources, as requested
The example usage is clear and shows the intended pattern for heterogeneous environments.
979-1030: Standard Resources and Default Resources sections provide comprehensive documentation.The new sections clearly explain:
- Special handling of
tmpdir,runtime,disk,mem, andgpuresources- Default values and their rationale
- Integration points for user customization
This provides good context for the
choose_tmpfeature introduced later in the Dynamic Resources section.tests/tests.py (1)
1321-1328: Passcleanup=Falseto the firstrun()call to return the temporary directoryThe first
run()call uses the defaultcleanup=True, which returnsNone. This causesos.path.join(test_path, "profile")to raise aTypeError. Addcleanup=Falseto capture the temporary directory:test_path = run(dpath("test_tmpdir"), cleanup=False)Also verify whether this test should include the
@skip_on_windowsdecorator, as the precedingtest_tmpdirfunction uses it due to tmpdir environment variable behavior on Windows.
|
@fgvieira what do you think about the generalization towards arbitrary dirs? |
|
@johanneskoester from what I remember, I think that I have already implemented that with the additions (apart from Or are you thinking about something else? |
|
Ah, right, sorry, I now remember that I had already seen that. |
🤖 I have created a release *beep* *boop* --- ## [9.22.0](v9.21.1...v9.22.0) (2026-06-01) ### Features * add semantic helper functions choose_file/choose_folder/choose_tmp and allow all semantic helper functions to be used when parsing resources ([#3820](#3820)) ([d3c2386](d3c2386)) * add temp to default pathvars ([#4108](#4108)) ([917fb11](917fb11)) * add workflow info to log ([#4079](#4079)) ([e40e15b](e40e15b)) * support python 3.14 ([#3739](#3739)) ([7e3be0c](7e3be0c)) ### Bug Fixes * Adapt for dataclasses._MISSING_TYPE replaced with sentinel in Python 3.15 ([#4211](#4211)) ([b7fb8df](b7fb8df)) * ensure that storage is not actually retrieved with --touch ([#4212](#4212)) ([2726cae](2726cae)) * symlink to directory "has older modification time" than target ([#3782](#3782)) ([#3784](#3784)) ([41903d8](41903d8)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sometimes, HPC nodes have different options to write
tempdepending on the hardware (e.g. some might havenvme).This function allows the use to specify a list of possible temp folders that, on each job submission, is evaluated. The first valid
diris selected.Moved
dynamicresources to afterstandardanddefaultresources.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
New Features
Tests
Documentation
Chores