Skip to content

Use AllocTRES for GPU and memory in info_historic - #976

Open
Oglopf wants to merge 4 commits into
masterfrom
sacct-alloc-tres
Open

Use AllocTRES for GPU and memory in info_historic#976
Oglopf wants to merge 4 commits into
masterfrom
sacct-alloc-tres

Conversation

@Oglopf

@Oglopf Oglopf commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

info_historic reports GPU count from ReqTRES — what a job requested at submission —
and does not populate total_memory at all. The squeue path (parse_job_info) already
sources total_memory from allocated TRES, so the same Info attribute means different
things depending on which code path produced it.

This adds AllocTRES to the sacct field list and uses it for both gpus and
total_memory in info_historic, so historic jobs report what was actually allocated
and both Slurm code paths populate Info consistently.

ReqTRES is still requested and still available to callers via native, so nothing is
lost — this changes which value drives the typed attributes.

It also adds gpus_from_tres. AllocTRES reports GPUs twice, as a typed entry and an
untyped rollup:

ReqTRES:   billing=144,cpu=144,gres/gpu=16,mem=512G,node=16
AllocTRES: billing=144,cpu=144,gres/gpu:a100=16,gres/gpu=16,mem=512G,node=16

gpus_from_gres sums every match, so reading AllocTRES with it returns 32 for that
job. gpus_from_tres reads the rollup instead, falling back to summing typed entries if
no rollup is present. gpus_from_gres is unchanged — it is correct for its other
callers, which receive tres-per-node (squeue) and sinfo gres strings, neither of which
duplicates.

Related issue

Closes #964

Testing

  • Tests included
  • No tests needed — reason: ___
    Adds test_info_historic_uses_alloc_tres in test/job/adapters/slurm_test.rb. The
    fixture's first job requested gpu=1/mem=16G but was allocated
    gres/gpu:a100=2,gres/gpu=2/mem=32G, so the test fails against the old behavior both
    on the requested-vs-allocated change and on the double count.

Also updates spec/fixtures/scripts/sacct.rb to emit the new AllocTRES column. Values
mirror ReqTRES on each row, so existing assertions in slurm_spec.rb are unchanged.
Without this the field count no longer matches and Hash[fields.keys.zip(values)]
misaligns.

bundle exec rake test and bundle exec rake spec are both green locally.

Checklist

  • Follows project code style and conventions
  • Documentation provided (if new feature, adapter or behavior change) — see note below
  • This is a large feature and was discussed in an issue first (if applicable) — N/A

Anything else?

Behavior change worth reviewing: Info#gpus from info_historic may now differ from
what it returned before for any job where the allocation didn't match the request. Sites
reading historic GPU counts will see allocated rather than requested values. That's the
intent, but it is a change in reported numbers, not just added data.

The TRES formats above were confirmed against a live Slurm cluster (Ascend at OSC) rather
than inferred — ReqTRES carries only the untyped rollup, AllocTRES carries both.

#964 asked whether to use requested or allocated. I went with keeping both
AllocTRES drives the typed attributes, ReqTRES stays in
native. Happy to change if the preference is otherwise.

One thing noticed but not changed: gres maps to tres-per-node on the squeue path and
ReqTRES on the sacct path, so the same key means different things depending on origin.
Sourcing gpus from tres_alloc on both paths sidesteps it here, but the key itself is
still ambiguous.

More broadly, Info attributes have no documented semantics across adapters, and this
divergence probably isn't unique to Slurm. I'll open a separate issue for that rather
than expand scope here.

@Oglopf

Oglopf commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Also fixes a latent bug in gpus_from_tres's typed-entry fallback: consuming the trailing comma meant adjacent typed entries were skipped, so gres/gpu:a100=2,gres/gpu:v100=1 summed to 2 instead of 3. Only reachable when no untyped rollup is present.

Comment thread test/job/adapters/slurm_test.rb

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have to update this file anyway is there a reason that we wouldn't use it for the test below? Or put another way, why add sacct_alloc_tres.txt if there is nothing preventing us from showing the same case (reqTres and allocTres differing) with the output here?

What got my attention on this was finding #979 while testing this change. This fixture file includes decimal valued memory that would have caught that if it had been used, so why not? I could be convinced otherwise, but it seems like even if we want to keep the additional fixture we should update the #info_historic returns an array of all the jobs when jobs spec test so that these updates get hit by something.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I can see what you mean, I'll consolidate onto sacct.rb and drop sacct_alloc_tres.txt and restructure the test.

One question first: once info_historic populates total_memory from that fixture's mem=0.98G, it'll hit #979 and return nil. Do you want that fixed in this PR, or should I leave memory out of the assertions and let #979 go separately?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with fixing that in this PR if you are, since it should just be updating the regex. I think it's probably easier to do that than remember to come back and complete the tests once it is fixed.

Comment thread test/job/adapters/slurm_test.rb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update sacct call with AllocTRES

3 participants