Skip to content

Commit bfc288c

Browse files
authored
fix(spur-cli): accept Slurm's --noconvert on squeue and sinfo (#647)
* fix(spur-cli): accept Slurm's --noconvert on squeue and sinfo Slurm scripts pass --noconvert to keep output machine-parseable. Neither command defined it, so clap rejected it as an unknown argument and a wrapper migrated from Slurm exited non-zero having done nothing. Spur never humanizes units in these commands: sinfo renders memory as a raw integer and the format engine performs no conversion anywhere. The flag is therefore accepted and deliberately inert rather than stubbed out, and output is byte-identical whether or not it is passed. A test pins that equivalence so the flag must be revisited if unit conversion is ever introduced. * refactor(spur-cli): tighten --noconvert help text and test the render path Both arg structs set disable_help_flag and re-add a long --help, and clap uses a single-paragraph doc comment as both short and long help, so the two-sentence rationale became the entire --help entry for the flag, beside terse neighbours like "Don't print header". Collapse it to one line and keep the reasoning here. That reasoning is also narrower than the old wording claimed. On sinfo the flag is inert because the render path already emits raw values: %m and %e resolve to memory_mb and free_memory_mb unchanged. On squeue it is inert for a different reason -- resolve_job_field has no size-valued specifier at all, so there is nothing to convert either way. Replace the parse-only test on sinfo with one that drives render_sinfo_output over a NodeInfo fixture and asserts the memory cells render as raw megabytes with and without the flag, so humanizing either column fails the test. Drop the squeue counterpart: its two argv vectors differed only by the flag under test, so no assertion could diverge, and it used %m, which squeue does not implement. * docs(migration): give --noconvert a home in the limitations table That table is the canonical list of flags Spur accepts without acting on, and the compatibility page points at it as the full list, so a flag missing from it has no documented home. The row also records that the other four commands Slurm defines `--noconvert` on do not accept it yet, which is the part a migrating script actually trips over. Two rows go the other way. `squeue --sort` and `sinfo --states` were listed as accepted but not applied, and both have since been implemented: sorting runs after `parse_sort_arg`, and node states are filtered server-side from the request. A limitations table that lists working features misleads exactly the reader it exists for. * docs(spur-cli): tighten the --noconvert comment and migration wording Review nits: collapse the three-line test comment, and say plainly that squeue has no memory column rather than implying it renders raw values. * ci: re-trigger E2E after a harness Postgres port collision
1 parent 1b2826b commit bfc288c

3 files changed

Lines changed: 57 additions & 5 deletions

File tree

crates/spur-cli/src/sinfo.rs

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,10 @@ pub struct SinfoArgs {
4545
#[arg(short = 'h', long)]
4646
pub noheader: bool,
4747

48+
/// Accepted for Slurm compatibility; has no effect
49+
#[arg(long)]
50+
pub noconvert: bool,
51+
4852
/// Controller address
4953
#[arg(
5054
long,
@@ -350,7 +354,7 @@ mod tests {
350354
use super::*;
351355
use spur_proto::proto as pb;
352356
use spur_proto::proto::slurm_controller_server::SlurmController;
353-
use spur_proto::proto::NodeState;
357+
use spur_proto::proto::{NodeState, ResourceSet};
354358
use std::sync::{Arc, Mutex};
355359
use tonic::{Request, Response, Status};
356360

@@ -392,6 +396,45 @@ mod tests {
392396
assert_eq!(req.states, vec![NodeState::NodeIdle as i32]);
393397
}
394398

399+
#[test]
400+
fn noconvert_is_accepted() {
401+
// Slurm scripts pass --noconvert to keep output machine-parseable.
402+
let args = parse_sinfo_args(&["sinfo", "--noconvert"]);
403+
assert!(args.noconvert);
404+
}
405+
406+
#[test]
407+
fn memory_columns_render_as_raw_megabytes_with_and_without_noconvert() {
408+
// Pins that %m/%e are raw MB today; humanizing them later turns this red.
409+
let mut node = make_node("gpu001", NodeState::NodeIdle, "gpu");
410+
node.total_resources = Some(ResourceSet {
411+
memory_mb: 2_321_924,
412+
..Default::default()
413+
});
414+
node.free_memory_mb = 11_077;
415+
let partitions = vec![make_partition("gpu", true)];
416+
417+
for argv in [
418+
["sinfo", "-N", "-h", "-o", "%m %e"].as_slice(),
419+
["sinfo", "-N", "-h", "-o", "%m %e", "--noconvert"].as_slice(),
420+
] {
421+
let args = parse_sinfo_args(argv);
422+
let fields = format_engine::parse_format(
423+
args.format.as_deref().expect("-o sets format"),
424+
&format_engine::sinfo_header,
425+
);
426+
427+
let lines = render_sinfo_output(
428+
&fields,
429+
&partitions,
430+
std::slice::from_ref(&node),
431+
args.node_oriented,
432+
);
433+
434+
assert_eq!(lines, ["2321924 11077"], "argv: {argv:?}");
435+
}
436+
}
437+
395438
#[test]
396439
fn state_filter_accepts_comma_separated_short_and_long_names() {
397440
let args = parse_sinfo_args(&["sinfo", "--states", "alloc,DOWN,draining"]);

crates/spur-cli/src/squeue.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,10 @@ pub struct SqueueArgs {
5757
#[arg(short = 'h', long)]
5858
pub noheader: bool,
5959

60+
/// Accepted for Slurm compatibility; has no effect
61+
#[arg(long)]
62+
pub noconvert: bool,
63+
6064
/// Print help
6165
#[arg(long, action = clap::ArgAction::Help)]
6266
pub help: Option<bool>,
@@ -460,6 +464,13 @@ mod tests {
460464
assert_eq!(err.kind(), clap::error::ErrorKind::DisplayHelp);
461465
}
462466

467+
#[test]
468+
fn noconvert_is_accepted() {
469+
// Slurm scripts pass --noconvert to keep output machine-parseable.
470+
let args = SqueueArgs::try_parse_from(["squeue", "--noconvert"]).unwrap();
471+
assert!(args.noconvert);
472+
}
473+
463474
#[test]
464475
fn short_h_is_noheader_not_help() {
465476
let args = SqueueArgs::try_parse_from(["squeue", "-h"]).unwrap();

docs/migration-from-slurm.rst

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,10 +55,8 @@ noted as "accepted for compatibility" parse without error but have no effect yet
5555
- Difference
5656
* - Partitions
5757
- Defined in ``spur.conf``; there is no runtime ``scontrol create/update/delete partition``. Edit the config and reload the controller to change a partition.
58-
* - ``squeue --sort``
59-
- Accepted for compatibility; result ordering is not yet applied.
60-
* - ``sinfo --states``
61-
- Accepted for compatibility; the state filter is not yet applied (all node states are returned).
58+
* - ``squeue``/``sinfo`` ``--noconvert``
59+
- Accepted for compatibility; Spur already reports raw values where applicable (``sinfo``'s memory columns are MB integers; ``squeue`` has no memory column yet), so there is nothing to suppress. Slurm also defines the flag on ``sacct``, ``sstat``, ``sshare`` and ``sreport``, where it is not accepted yet.
6260
* - ``sacct --jobs``
6361
- Accepted for compatibility; the job-id filter is not yet applied server-side.
6462
* - ``sacct`` ``ReqMem``

0 commit comments

Comments
 (0)