Use AllocTRES for GPU and memory in info_historic - #976
Conversation
|
Also fixes a latent bug in |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
What does this PR do?
info_historicreports GPU count fromReqTRES— what a job requested at submission —and does not populate
total_memoryat all. The squeue path (parse_job_info) alreadysources
total_memoryfrom allocated TRES, so the sameInfoattribute means differentthings depending on which code path produced it.
This adds
AllocTRESto thesacctfield list and uses it for bothgpusandtotal_memoryininfo_historic, so historic jobs report what was actually allocatedand both Slurm code paths populate
Infoconsistently.ReqTRESis still requested and still available to callers vianative, so nothing islost — this changes which value drives the typed attributes.
It also adds
gpus_from_tres.AllocTRESreports GPUs twice, as a typed entry and anuntyped rollup:
gpus_from_gressums every match, so readingAllocTRESwith it returns 32 for thatjob.
gpus_from_tresreads the rollup instead, falling back to summing typed entries ifno rollup is present.
gpus_from_gresis unchanged — it is correct for its othercallers, which receive
tres-per-node(squeue) and sinfo gres strings, neither of whichduplicates.
Related issue
Closes #964
Testing
Adds
test_info_historic_uses_alloc_tresintest/job/adapters/slurm_test.rb. Thefixture's first job requested
gpu=1/mem=16Gbut was allocatedgres/gpu:a100=2,gres/gpu=2/mem=32G, so the test fails against the old behavior bothon the requested-vs-allocated change and on the double count.
Also updates
spec/fixtures/scripts/sacct.rbto emit the newAllocTREScolumn. Valuesmirror
ReqTRESon each row, so existing assertions inslurm_spec.rbare unchanged.Without this the field count no longer matches and
Hash[fields.keys.zip(values)]misaligns.
bundle exec rake testandbundle exec rake specare both green locally.Checklist
Anything else?
Behavior change worth reviewing:
Info#gpusfrominfo_historicmay now differ fromwhat 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 —
ReqTREScarries only the untyped rollup,AllocTREScarries both.#964asked whether to use requested or allocated. I went with keeping both—
AllocTRESdrives the typed attributes,ReqTRESstays innative. Happy to change if the preference is otherwise.One thing noticed but not changed:
gresmaps totres-per-nodeon the squeue path andReqTRESon the sacct path, so the same key means different things depending on origin.Sourcing
gpusfromtres_allocon both paths sidesteps it here, but the key itself isstill ambiguous.
More broadly,
Infoattributes have no documented semantics across adapters, and thisdivergence probably isn't unique to Slurm. I'll open a separate issue for that rather
than expand scope here.