fix(spur-cli): sort squeue output and honor -S/--sort - #524
Conversation
squeue returned jobs in controller HashMap order and silently ignored -S/--sort. Sort client-side (as Slurm does): parse comma-separated sort fields with +/- direction prefixes, apply Slurm's default P,t,-p (plus a jobid tiebreak) when -S is absent, and allow -S -i to parse. State sort follows Slurm's base state order; unknown sort fields are rejected.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #524 +/- ##
==========================================
+ Coverage 73.66% 74.20% +0.54%
==========================================
Files 164 165 +1
Lines 56954 59035 +2081
==========================================
+ Hits 41953 43802 +1849
- Misses 15001 15233 +232 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
⚠️ Not ready to approve
-S L is currently accepted but cannot sort meaningfully due to missing %L field support, and the default sort’s partition comparison incurs avoidable per-comparison allocations.
Pull request overview
This PR fixes spur-cli’s squeue job ordering to match Slurm behavior by implementing client-side sorting, wiring up the previously-dead -S/--sort flag, and adding unit tests to ensure sort-spec parsing and ordering semantics behave as expected.
Changes:
- Implement client-side sorting for
squeue, including Slurm’s default sort order (P,t,-p) with a deterministic job-id tiebreak. - Enable descending sort parsing by allowing hyphen-prefixed values for
-S/--sortand parsing comma-separated multi-key sort specs with+/-direction prefixes. - Add unit tests covering sort parsing, default ordering, numeric/time/state ordering, and error cases.
File summaries
| File | Description |
|---|---|
| crates/spur-cli/src/squeue.rs | Implements -S/--sort parsing and client-side sorting for squeue, plus unit tests validating Slurm-aligned ordering. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ring clones squeue -S L was accepted but sorted as a no-op because compare_field had no 'L' arm and resolve_job_field returns "?" for it. Add a time-left comparator (time_limit - run_time, unlimited last). Also compare the common string specs (partition/user/name/account/qos) by borrowing the proto fields directly instead of cloning through resolve_job_field on every comparison.
shiv-tyagi
left a comment
shiv-tyagi
left a comment
There was a problem hiding this comment.
Client-side sort matching Slurm's architecture is the right call, and so is the default P,t,-p with a jobid tiebreak.
Verified locally: cargo test -p spur-cli passes (248 tests), cargo clippy -p spur-cli --all-targets is clean, and -S -i / --sort=-i both parse against the built binary.
One thing worth fixing before merge: the sort spec is validated after the gRPC fetch, so squeue -S <typo> reports a connection error instead of the invalid-spec error. The rest is a refactor suggestion, a missing test, and three nits.
…spur-core Parse the -S/--sort spec before any network I/O so an invalid spec reports "Invalid sort specification" immediately instead of a downstream connect failure, and a typo no longer costs a full get_jobs fetch. Move the job-state sort ordering into JobState::sort_rank() in spur-core next to code()/display() so squeue no longer keeps a fourth hand-maintained copy of the state list. The rank stays an explicit match because the enum's declaration order differs from Slurm's sort order (SUSPENDED after RUNNING); a spur-core test guards that. Also from review: extract the single-char sort spec via an explicit match instead of a side-effecting filter, add disable_help_flag so squeue's -h stays --noheader and a parser test exercises allow_hyphen_values (-S -i), drop a duplicated call-site comment, and correct the TIME_LEFT helper doc comment.
shiv-tyagi
left a comment
shiv-tyagi
left a comment
There was a problem hiding this comment.
All six points from the last round are addressed. Verified against f286190:
- Sort parse hoisted above the network call:
squeue -S xnow printsInvalid sort specification: xand-S ""printsInvalid sort specification: (empty), neither touching the controller. state_sort_rankdelegates toJobState::sort_rank()in spur-core, and the uniqueness-across-ALLassertion in the new test is a nice touch.- Duplicate comment gone, tuple match applied,
-S Lcomment corrected. - The clap test now runs
try_parse_fromunder debug assertions without panicking, so the-hfix is real.
cargo test -p spur-cli 249 passing, -p spur-core 389 passing, cargo clippy clean on both.
One regression to fix before merge: disable_help_flag also removed --help, so squeue --help no longer works. Details inline, with a fix I verified locally.
Separately, and outside this PR: sinfo, sprio, and sshare carry the identical -h collision and still panic on every invocation in a debug build (Command sinfo: Short option names must be unique for each argument, but '-h' is in use by both 'noheader' and 'help'). Release builds compile the assert out, which is why CI never caught it. Not yours to fix here, but this PR does leave squeue as the only one of the four that works. Happy to file an issue if you'd rather keep it separate.
disable_help_flag dropped both -h and --help; re-add --help via ArgAction::Help so -h stays --noheader while --help still prints help.
Thanks for the suggestion, I've fixed the |
shiv-tyagi
left a comment
shiv-tyagi
left a comment
There was a problem hiding this comment.
--help is restored and the two added tests pin down both halves of the -h / --help split. All points addressed.
Closes #525
What this fixes
squeuehad three sorting gaps versus Slurm:HashMapiteration order — effectively unsorted, and unstable across restarts.-S/--sortwas dead. The flag parsed into a field that nothing ever read, so-S ihad no effect.squeue -S -ifailed withunexpected argument '-i'because the arg rejected hyphen-prefixed values.Approach
Sort client-side in
squeue.rs, matching Slurm's architecture (Slurm'ssqueuesorts locally; every field needed is already onJobInfo). This keeps the controller a pure data source and avoids coupling display ordering to the core.-S/--sortas comma-separated fields, each optionally prefixed+(ascending, default) or-(descending), reusing the existing format-spec letters.P,t,-p(partition asc, job-state asc, priority desc) when-Sis absent, with a trailing job-id tiebreak so equal-priority jobs order deterministically.allow_hyphen_valuesto the-Sarg so-S -iparses.t/T(so SUSPENDED sorts right after RUNNING, ahead of terminal states), and lexically otherwise.Invalid sort specificationerror.Known limitations / deviations
-Sconsumes the following token (a consequence of allowing hyphen-prefixed values). Use the attached form for descending sorts —-S -ior--sort=-i— not-Ssplit from an unrelated following flag.Testing
P,t,-pordering, priority-desc with job-id tiebreak, state ordering (SUSPENDED placement), time-limit (UNLIMITED sorts last), and timestamp sort.cargo clippyclean; fullspur-clisuite passes.-S i/-S -i/ multi-key-S P,iorder correctly, unknown fields error, and outcomes match Slurm for the default sort and the-Svariants.