fix(core)!: read -t MM:SS as minutes:seconds and accept bare days-hours - #811
fix(core)!: read -t MM:SS as minutes:seconds and accept bare days-hours#811sunxxuns wants to merge 2 commits into
-t MM:SS as minutes:seconds and accept bare days-hours#811Conversation
Slurm documents six accepted `--time` formats: "minutes", "minutes:seconds", "hours:minutes:seconds", "days-hours", "days-hours:minutes" and "days-hours:minutes:seconds". Two of them were parsed incorrectly. Both time parsers read the two-field colon form as hours:minutes, so every such limit was 60x longer than requested -- `-t 4:00` meant four minutes but granted four hours. And bare `days-hours` failed to parse at all: the remainder after `days-` was handed to a helper that required at least two colon-separated fields, so `-t 2-12` returned None, which is the value that also means INFINITE, yielding a job with no time limit. `days-hours:minutes` and `days-hours:minutes:seconds` were already correct, since a colon in the remainder satisfied that helper. Only the colon-less form fell through. Fixes the two-field arm in both `parse_slurm_time_seconds` and `parse_slurm_time_minutes`, teaches `parse_hms_seconds` and `parse_hms` to accept bare hours after `days-`, and corrects the doc comments that described the old reading as intended. Three test assertions encoded the incorrect reading and are updated, along with a partition fixture whose `max_time = "1:00"` was written to mean one hour -- the same mistake this change prevents. The overflow test for the two-field arm needed a new input, because minutes are no longer multiplied by 3600 and so no longer overflow at the old value. BREAKING CHANGE: two-field colon time values now mean minutes:seconds rather than hours:minutes, so they resolve to 1/60th of their previous duration. This affects `-t`/`--time` on sbatch, srun and salloc, and partition `MaxTime`/`DefaultTime`. Job scripts and site configs relying on the old reading should switch to the explicit `HH:MM:SS` form, which was and remains unambiguous. Bare `days-hours` now produces a real limit where it previously produced an unlimited job. Refs ROCm#804 Co-authored-by: Cursor <[email protected]>
There was a problem hiding this comment.
🟡 Changes recommended
The new MM:SS→minutes rounding logic can silently compute an incorrect result for inputs where the seconds field is ≥60, and should be made consistent or reject/normalize such values.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes Spur’s Slurm-compatible --time parsing to match Slurm’s documented grammar, correcting a 60× inflation bug for two-field colon values and adding support for the previously-unparsed days-hours form (which could silently become “no limit” in some paths).
Changes:
- Interpret the 2-field colon form as
MM:SS(notHH:MM) in both the minutes and seconds parsers, with minute-granularity rounding consistent with suffixed durations. - Accept bare
days-hoursby allowing thedays-remainder to be parsed as hours-only. - Update and expand tests to cover all documented Slurm
--timeforms and adjust fixtures that previously depended on the incorrect interpretation.
File summaries
| File | Description |
|---|---|
| crates/spur-core/src/config.rs | Fixes core time parsing semantics, extends days- remainder parsing, updates docs/tests for full Slurm grammar coverage. |
| crates/spur-tests/src/t52_config.rs | Updates/extends config parsing tests and corrects a partition fixture to use an unambiguous HH:MM:SS form. |
| crates/spur-cli/src/scontrol.rs | Updates reservation-duration parser documentation to reflect the corrected shared --time grammar. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // remainders round up, matching the suffixed-duration path. | ||
| let m: u32 = parts[0].parse().ok()?; | ||
| let sec: u32 = parts[1].parse().ok()?; | ||
| m.checked_add(if sec > 0 { 1 } else { 0 }) |
There was a problem hiding this comment.
Good catch, fixed in 4913079. The seconds field isn't constrained to 0..59, so the flat one-minute round-up undercounted anything at or above 60s: 1:90 is 150 seconds but yielded 2 minutes instead of 3.
Rather than div_ceil on the seconds field alone, I sum first and then round the total, which keeps the two parsers consistent by construction:
let total = m.checked_mul(60)?.checked_add(sec)?;
u32::try_from(total.div_ceil(60)).ok()Widening to u64 for the intermediate also makes the overflow boundary exact instead of relying on the multiply to overflow first. Added parse_time_minutes_carries_an_unnormalised_seconds_field covering 0:59, 0:60, 0:61 and 1:90 as a regression guard.
| /// Parse the part after `days-`: bare hours, `hours:minutes`, or | ||
| /// `hours:minutes:seconds`. The bare-hours case backs Slurm's `days-hours`. | ||
| fn parse_hms(s: &str) -> Option<u32> { |
There was a problem hiding this comment.
You're right, that comment was inaccurate — parse_hms also backs the bare hours:minutes:seconds form via the three-field arm of parse_slurm_time_minutes, not just the part after days-. Reworded in 4913079 to describe it as an hours-first duration serving both callers, with the bare-hours case called out as the one that backs days-hours.
Left the sibling parse_hms_seconds doc as-is, since that one really is only reached from the days branch.
The MM:SS minutes parser added a flat one minute whenever the seconds field was non-zero, which undercounts any input at or above 60 seconds: "1:90" is 150 seconds but yielded 2 minutes instead of 3. The field is not constrained to 0..59, so sum first and round up. Also correct the parse_hms doc comment, which described the helper as days-only even though it backs the bare hours:minutes:seconds form too. Co-authored-by: Cursor <[email protected]>
75cc814 to
4913079
Compare
shiv-tyagi
left a comment
shiv-tyagi
left a comment
There was a problem hiding this comment.
Thanks for the PR. One comment. PTAL.
| fn parse_hms(s: &str) -> Option<u32> { | ||
| let parts: Vec<&str> = s.split(':').collect(); | ||
| if parts.len() != 3 && parts.len() != 2 { | ||
| if parts.len() > 3 { | ||
| return None; | ||
| } | ||
| let h: u32 = parts[0].parse().ok()?; | ||
| let m: u32 = parts[1].parse().ok()?; | ||
| let m: u32 = if parts.len() >= 2 { | ||
| parts[1].parse().ok()? | ||
| } else { | ||
| 0 | ||
| }; |
There was a problem hiding this comment.
parse_hms and parse_hms_seconds parse the same grammar (bare hours / H:M / H:M:S) but round differently. parse_hms_seconds returns exact seconds; parse_hms computes minutes as h*60 + m + (1 if s > 0 else 0), a flat one-minute bump regardless of how large s is.
That's the same class of bug this PR fixes in the MM:SS arm above (an unnormalised seconds field should carry into whole minutes, not add a flat one). parse_hms("0:0:90") gives 1 minute; the correct ceiling is 2 (ceil(90/60)). This is reachable from bare H:MM:SS and from the part after days-, so "2-0:0:90" also undercounts by a minute.
Rather than re-deriving the rounding, parse_hms could delegate to parse_hms_seconds:
fn parse_hms(s: &str) -> Option<u32> {
u32::try_from(parse_hms_seconds(s)?.div_ceil(60)).ok()
}This drops the duplicate parsing logic and fixes the rounding inconsistency using the same div_ceil + try_from pattern already used in the MM:SS arm.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #811 +/- ##
==========================================
+ Coverage 80.15% 80.15% +0.01%
==========================================
Files 184 184
Lines 87772 87811 +39
==========================================
+ Hits 70348 70384 +36
- Misses 17424 17427 +3 🚀 New features to boost your workflow:
|
Fixes #804.
What was wrong
Slurm documents six accepted
--timeformats:minutes,minutes:seconds,hours:minutes:seconds,days-hours,days-hours:minutes,days-hours:minutes:seconds. Two were parsed incorrectly, and both failures were silent.The two-field colon form was read as
hours:minutes, so every such limit came out 60x longer than requested.-t 4:00means four minutes; it was granting four hours, andscontrol show jobreportedTimeLimit=04:00:00, which looks entirely deliberate.Bare
days-hoursfailed to parse at all. The remainder afterdays-went to a helper requiring at least two colon-separated fields, so2-12returnedNone— the same value that meansINFINITE— producing a job with no time limit from a request for two and a half days.Measured against the documented grammar before this change:
days-hours:minutesanddays-hours:minutes:secondswere already correct, because a colon in the remainder satisfied that helper. Only the colon-less form fell through, which is why this went unnoticed.What this changes
parse_slurm_time_secondsandparse_slurm_time_minutesnow readsMM:SS. The minutes variant rounds a sub-minute remainder up, matching what the suffixed-duration path already did.parse_hms_secondsandparse_hmsaccept bare hours afterdays-, which is whatdays-hoursneeds. Both helpers are only reachable from thedays-branch, so this does not affect other forms.H:MMas intended are corrected, includingparse_reservation_durationinscontrol.rs, whose comment documented the quirk as something to work around.Tests
Three existing assertions encoded the old reading and are updated:
config.rstest_parse_timeandtest_parse_time_seconds, andspur-testst52_2. Added a test per parser covering all six documented forms.Two changes worth calling out for review:
t52_13_build_partitionsused a fixture ofmax_time = "1:00"while asserting 60 minutes — a config author writing the two-field form to mean one hour, which is exactly the mistake this change prevents. Changed to"1:00:00"so the fixture states the intent it always had.parse_time_rejects_overflowing_slurm_durationsexercised overflow on the two-field arm with"71582789:00", which only overflowed because minutes were multiplied by 3600. That arm now takes minutes directly, so the input no longer overflows; replaced with"4294967295:01", where the round-up pushes pastu32::MAX, plus a value that does not fitu32at all. I deliberately did not assert thatu32::MAXminutes parses successfully, sinceaccounting::INFINITEisu32::MAXand that would invite a sentinel collision.cargo test --workspace,cargo fmt --all --checkandcargo clippy --workspace --all-targetsare clean.Breaking change
Two-field colon values now resolve to 1/60th of their previous duration. This affects
-t/--timeonsbatch,srunandsalloc, and partitionMaxTime/DefaultTime. Baredays-hoursnow produces a real limit where it previously produced an unlimited job.The explicit
HH:MM:SSform was and remains unambiguous, so that is the migration path for anything relying on the old reading. Partition configs are the case to flag in release notes: a site withMaxTime = "30:00"has been enforcing thirty hours and will start enforcing thirty minutes.Worth considering as a follow-up, but deliberately not in this PR: a startup warning when a partition time value uses the two-field form, so operators are told rather than surprised.
Deliberately out of scope
parse_wall_timeinsacctmgr.rsis a third, independent implementation of the same grammar, serving QOSgrpwall/maxwall. It has the same two-field bug, also rejects baredays-hours, and uses unchecked arithmetic. I left it alone because it is a different user-facing surface with its own migration story, anddocs/admin-guide/accounting.rstcurrently documents its behaviour explicitly (grpwall=600andgrpwall=10:00"both mean ten hours"). Happy to follow up there, or in this PR if you would rather it land together.Longer term, three parallel implementations of one grammar is what allowed the same defect to exist in two of them. Consolidating them would prevent recurrence, but that felt like more than a bug fix should take on without maintainer input.