feat: add function get_tmp to ioutils, and allow ioutils functions to be used when parsing resources#3820
feat: add function get_tmp to ioutils, and allow ioutils functions to be used when parsing resources#3820fgvieira wants to merge 44 commits intosnakemake:mainfrom
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 | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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. |
| "parse_input": parse_input, | ||
| "extract_checksum": extract_checksum, | ||
| "flatten": flatten, | ||
| "choose_tmp": choose_tmp, |
There was a problem hiding this comment.
I think in addition this needs to be present in the eval environment for resource expressions, right?
There was a problem hiding this comment.
Yes, but I think I have already included it in src/snakemake/resources.py, no?
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.
|
This is a nice addition @fgvieira - are you wanting to wait until the hackathon to get this merged? If its okay I'll see if I can get the tests passing and merge this sooner. |
|
I had a thought on the design: instead of exposing a default-resources:
tmpdir:
- "/scratch/nvme"
- "/home/$USER/scratch"
- "/scratch/$SLURM_JOB_ID"Snakemake would iterate through and pick the first valid path, falling back to |
Pixi install is failing with "failed to parse pypi name mapping" errors likely due to rate limiting
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/main.yml (1)
49-49: Clarify the rationale for limiting parallelism.Adding
max-parallel: 10will limit concurrent jobs to 10 out of ~50 total matrix combinations, which will increase overall CI execution time. The PR description focuses on tmpdir functionality and doesn't mention this CI configuration change.Consider adding an inline comment explaining why this limit is necessary (e.g., runner availability, cost management, rate limiting, or resource contention).
Example of documenting the rationale
strategy: fail-fast: false + # Limit parallelism to manage runner availability and costs max-parallel: 10
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/main.yml
⏰ 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). (10)
- GitHub Check: tests (2, windows-2022, py313)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (1, macos-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, ubuntu-latest, py313)
|
@cademirch I also thought about that, but I wondered if it would be too disruptive. An io_utils function is completely independent from the main code and the users are free to use or not. But if @johanneskoester thinks it is a better solution, I am all for it! |
Pixi install github action is failing with "failed to parse pypi name mapping" errors likely due to rate limiting when 30+ jobs are kicked off nearly simultaneously I tested this fix on #3820 since the tests kept failing due to the `pixi` install action failing. After committing this change, [the actions ran successfully](https://github.com/snakemake/snakemake/actions/runs/20684893321). In [this failing run's](https://github.com/snakemake/snakemake/actions/runs/20682879051/job/59383597583#step:3:3261) debug logs we see: ``` pixi install -e py311 [...] WARN resolve_conda{group=py313 platform=win-64}: reqwest_retry::middleware: Retry attempt #1. Sleeping 1.225245051s before the next attempt Error: × failed to parse pypi name mapping ├─▶ error decoding response body ╰─▶ expected value at line 1 column 1 ``` This warning is repeated many times until finally pixi stops retrying - this is what suggested to me that some sort of rate limit was the issue. One downside is that this does make the CI take a bit longer to run. We could consider using the `cache` feature of the pixi action. And turning up the max-parallel, or reducing the number of test-groups ### 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 development toolchain dependencies for improved build and test infrastructure. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
For reference: we discussed to generalize this function into something that can work with any list of directories. |
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