feat: partition time handling #378
Conversation
|
Warning Rate limit exceeded@cmeesters has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds parsing and normalization of diverse time formats for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PartitionLimits
participant Utils as parse_time_to_minutes()
User->>PartitionLimits: instantiate(max_runtime = "2-03:30:00" / "120" / "3h")
PartitionLimits->>PartitionLimits: __post_init__()
PartitionLimits->>Utils: call parse_time_to_minutes(time_value)
rect rgb(235,247,255)
Note over Utils: Try numeric -> SLURM-like -> Snakemake units\nNormalize and round seconds (half-up)
end
Utils-->>PartitionLimits: return total_minutes (int)
PartitionLimits->>PartitionLimits: store normalized max_runtime
PartitionLimits-->>User: initialized instance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/further.md(2 hunks)snakemake_executor_plugin_slurm/partitions.py(2 hunks)snakemake_executor_plugin_slurm/utils.py(1 hunks)tests/test_time_conversion.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
snakemake_executor_plugin_slurm/utils.py
🧬 Code graph analysis (2)
snakemake_executor_plugin_slurm/partitions.py (1)
snakemake_executor_plugin_slurm/utils.py (1)
parse_time_to_minutes(14-128)
tests/test_time_conversion.py (2)
snakemake_executor_plugin_slurm/utils.py (1)
parse_time_to_minutes(14-128)snakemake_executor_plugin_slurm/partitions.py (2)
PartitionLimits(199-233)read_partition_file(14-51)
🪛 markdownlint-cli2 (0.18.1)
docs/further.md
107-107: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
108-108: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
109-109: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
110-110: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
111-111: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
112-112: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
113-113: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
114-114: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
115-115: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🪛 Ruff (0.14.5)
snakemake_executor_plugin_slurm/utils.py
42-42: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
49-49: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
66-66: Abstract raise to an inner function
(TRY301)
66-66: Avoid specifying long messages outside the exception class
(TRY003)
91-91: Abstract raise to an inner function
(TRY301)
91-91: Avoid specifying long messages outside the exception class
(TRY003)
95-95: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
107-114: Avoid specifying long messages outside the exception class
(TRY003)
128-128: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
⏰ 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). (1)
- GitHub Check: testing
🔇 Additional comments (10)
snakemake_executor_plugin_slurm/partitions.py (2)
11-11: LGTM!The import is clean and necessary for the time conversion functionality.
226-233: LGTM!The
__post_init__method correctly handles automatic conversion ofmax_runtimeto minutes. The type checking logic appropriately handles strings, finite numeric values, and infinity values.docs/further.md (2)
90-90: LGTM!The type update correctly reflects that
max_runtimenow accepts both integers and time strings.
119-143: LGTM!The example configuration clearly demonstrates the various time format options and is consistent with the documented behavior.
tests/test_time_conversion.py (3)
15-150: Comprehensive test coverage!The test suite thoroughly covers:
- Numeric inputs (int, float, string)
- Snakemake-style time strings (d, h, m, s, combined)
- SLURM-style formats (various combinations)
- Edge cases (whitespace, case sensitivity, invalid formats)
- Rounding behavior
The tests are well-organized and clearly documented.
However, please ensure all tests pass, especially
test_parse_time_slurm_minutes_seconds(lines 102-111), given the potential implementation issue I flagged insnakemake_executor_plugin_slurm/utils.py.
152-179: LGTM!The
PartitionLimitsintegration tests properly verify that the__post_init__method correctly converts various time formats and preserves infinity values.
181-373: LGTM!The partition file reading tests provide excellent coverage of:
- Various time formats in YAML configuration
- Mixed format handling
- Invalid format error handling
- Integration with the full partition reading pipeline
snakemake_executor_plugin_slurm/utils.py (3)
14-39: LGTM!The function signature and docstring are clear and comprehensive, documenting all supported time formats and expected behavior.
40-51: LGTM!The numeric input handling correctly processes integers, floats, and numeric strings, with appropriate rounding to the nearest minute.
101-128: LGTM!The Snakemake-style time string parsing correctly handles all unit types (d, h, m, s) and combined formats, with appropriate conversion and rounding.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_time_conversion.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
🧬 Code graph analysis (1)
tests/test_time_conversion.py (2)
snakemake_executor_plugin_slurm/utils.py (1)
parse_time_to_minutes(14-128)snakemake_executor_plugin_slurm/partitions.py (2)
PartitionLimits(199-233)read_partition_file(14-51)
🪛 GitHub Actions: CI
tests/test_time_conversion.py
[error] 1-1: Test failure: parse_time_to_minutes(120.5) expected 121 but got 120 (rounded to nearest minute).
[error] 1-1: Test failure: parse_time_to_minutes('120.5') expected 121 but got 120 (rounded to nearest minute).
[error] 1-1: Test failure: parse_time_to_minutes('30s') expected 1 but got 0 (seconds parsing incorrect).
[error] 1-1: Test failure: parse_time_to_minutes("6h30s") expected 361 but got 360 (seconds addition off by 1 minute).
[error] 1-1: Test failure: parse_time_to_minutes("60:30") expected 3630 but got 0 (SLURM time format parsing for minutes/seconds failed).
[error] 1-1: Test failure: parse_time_to_minutes("1-0:30") expected 1470 but got 1440 (days/hours/minutes parsing off by 30).
[error] 1-1: Test failure: parse_time_to_minutes('0-1:30:30') expected 91 but got 90 (days/hours/minutes/seconds parsing off by 1).
🔇 Additional comments (2)
tests/test_time_conversion.py (2)
153-180: LGTM! PartitionLimits integration tests are well-structured.These tests correctly verify that
PartitionLimits.__post_init__convertsmax_runtimeusing theparse_time_to_minutesfunction. The infinity handling tests (lines 171-179) are particularly important for ensuring the default/unlimited case works correctly.Once the
parse_time_to_minutesimplementation bugs are fixed, these tests should pass.
182-374: LGTM! Integration tests are comprehensive and well-organized.The
TestPartitionFileTimeConversionclass provides excellent end-to-end coverage:
- Tests verify the full pipeline from YAML parsing through
PartitionLimitsinstantiation- Proper resource management with
tempfileandfinallyblocks ensures cleanup- Error handling is tested (lines 271-290)
- The mixed format test (lines 343-374) elegantly verifies that all three representations of 6 days (Snakemake "6d", SLURM "6-0", numeric 8640) normalize to the same value
These tests will pass once the underlying
parse_time_to_minutesimplementation is corrected.
cademirch
left a comment
cademirch
left a comment
There was a problem hiding this comment.
great! was an oversight on the original PR - thanks for adding this.
|
@cademirch still working. Just made a logic fix - now there was an ansible error in the CI. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/utils.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Learnt from: johanneskoester
Repo: snakemake/snakemake-executor-plugin-slurm PR: 173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
Learnt from: cmeesters
Repo: snakemake/snakemake-executor-plugin-slurm PR: 249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
🪛 Ruff (0.14.5)
snakemake_executor_plugin_slurm/utils.py
16-16: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
71-71: Abstract raise to an inner function
(TRY301)
71-71: Avoid specifying long messages outside the exception class
(TRY003)
94-94: Abstract raise to an inner function
(TRY301)
94-94: Avoid specifying long messages outside the exception class
(TRY003)
110-117: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (4)
snakemake_executor_plugin_slurm/utils.py (4)
3-3: LGTM!The new imports are correctly added and used in the new time parsing functions.
Also applies to: 7-7
15-16: LGTM!The rounding implementation is correct. The static analysis hint about unnecessary
intcast is a false positive—math.floorreturns a float in Python 3, so the cast is required.
19-56: LGTM!The docstring is comprehensive and the numeric input handling is correct. The function properly handles both direct numeric types and numeric strings.
104-131: LGTM!The Snakemake-style time parsing logic is correct. The regex pattern properly matches time units, and the conversion calculations are accurate. The comprehensive error message is helpful for users.
🤖 I have created a release *beep* *boop* --- ## [2.0.0](v1.9.2...v2.0.0) (2025-11-24) ### ⚠ BREAKING CHANGES * proposal for dynamic partition selection ([#321](#321)) ### Features * partition time handling ([#378](#378)) ([53be508](53be508)) * proposal for dynamic partition selection ([#321](#321)) ([95821f9](95821f9)) ### Bug Fixes * naming SLURM logs, SLURM logs not just "log files" ([#372](#372)) ([ce7cc4b](ce7cc4b)) ### Documentation * Replace incorrect Markdown with a blockquote ([#377](#377)) ([b45709f](b45709f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **BREAKING CHANGES** * Version 2.0.0 introduces breaking changes * **New Features** * Dynamic partition selection functionality * **Bug Fixes** * Multiple bug fixes included * **Documentation** * Updated documentation for version 2.0.0 <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Even though, I do not expect admin folks to enter weird numerical values in the partition configs, the parsing should support the SLURM standard formats for time indicators.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.