working memory attribute on info

[Bubballoo3]

What does this PR do?

Reworks #879 to add a field for total memory to the job info object. While inconsistent values from MinMemory halted that effort the last time, this PR fetches memory data from the tres-alloc flag, which consistently shows the total for the whole job, as can be seen in the following output

[bsingleton@ascend-login02 ~]$ sbatch --account=PZS0714 --mem=100 --cpus-per-task=3 /users/PZS0714/bsingleton/ondemand/data/sys/dashboard/projects/37nntbyn/sleep_60_min.sh
Submitted batch job 6750802
[bsingleton@ascend-login02 ~]$ sbatch --account=PZS0714 --mem-per-cpu=100 --cpus-per-task=3 /users/PZS0714/bsingleton/ondemand/data/sys/dashboard/projects/37nntbyn/sleep_60_min.sh
Submitted batch job 6750903
[bsingleton@ascend-login02 ~]$ squeue -u bsingleton -O "JobID,MinMemory,NumCPUs,tres-alloc"
JOBID               MIN_MEMORY          CPUS                TRES_ALLOC          
6750903             100M                3                   cpu=3,mem=300M,node=
6750802             100M                3                   cpu=3,mem=100M,node=

Unfortunately the tres-alloc field does not respect the --noconvert flag, and so we include the unit conversion piece from #879.

After these changes, we will be able to access total memory (in bytes) from the info object, which will allow us to add it to the session card and active jobs in ondemand (OSC/ondemand#1006 and OSC/ondemand#1007)

Related issue

Fixes #878 (if applicable)

Testing

Testing is in progress and will be included soon

• Tests included
• No tests needed — reason: ___

Checklist

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

Anything else?

Screenshots, context, or anything reviewers should pay close attention to.

[johrstrom]

By golly that looks ugly. Does this look better? In any case

  number, factor = match[1].split(/([KMGTP])/)
  UNIT_FACTORS[factor] * number.to_i

Can't get match[1].split(/([UNIT_FACTORS.keys.join])/) to work so there's a duplication in keys there.

Either way you don't need the mem_value assignment and there should likely be some safeguard for factor being nil because we can run into a undefined method *' for nil (NoMethodError)` here.

[johrstrom]

By golly that looks ugly.

It's the double [][] calls that's getting to me. I think at the very least we can give match[1] a name.

[Bubballoo3]

OK I think I have this figured out in the cleanest way possible. I'll quote the updated method here for convenience.

match = tres.match(/(?:^|,)mem=(\w+)(?:,|$)/)
return unless match

(number, unit) = match[1].scan(/\d+|\D+/)
factor = UNIT_FACTORS[unit]
mem_value = factor.to_i * number.to_i 
return mem_value == 0 ? nil : mem_value

This way we only call match[1] once. We scan to split cleanly into digits and characters (either of which could be nil or blank, depending). No matter what value unit has we are either getting a number or nil from UNIT_FACTORS[unit]. So when we cast both to integers, nil.to_i == 0 and "".to_i == 0. So if the final value is 0 we know something was missing and return nil. Add to that the fact that there should never be a valid '0M' value coming out of slurm, and I think this effectively boils down individual checks on each variable to a single check on the final value. What do you think?

[johrstrom]

What do you think?

That'll work for me, though I just thought of changing /(?:^|,)mem=(\w+)(?:,|$)/ to pull both the number and the factor out of it.

But really it's an issue of readability for me and not so much the performance. just having match = match[1] would have worked for me.

[Bubballoo3]

Ok, in that case I made one more update keeping the zero check but opting for plain string indexing over the regex, since that is much more readable to me. I also realized that to_i will ignore the final non-digit character, so we don't even need to bother with the match_str[...-1], since '1234M'.to_i == 1234 and 'M'.to_i == 0.

[Bubballoo3]

This should be passing tests now. Do we want to query for memory in sacct as well, or would you prefer we implement that separately? For reference, the sacct equivalent of squeue's tres-alloc is AllocTRES and the parsing would be identical

[johrstrom]

This should be passing tests now. Do we want to query for memory in sacct as well, or would you prefer we implement that separately? For reference, the sacct equivalent of squeue's tres-alloc is AllocTRES and the parsing would be identical

We should implement it separately if at all. Are they different values?

[johrstrom]

LGTM

[Bubballoo3]

We should implement it separately if at all. Are they different values?

It should be the same value, just sacct allows querying after job completion. Though I am not convinced we need to update it at all or, if we do, that it is at all urgent. Created #964 to discuss further