bugfix/walltime-conversion

[bgunnar5]

This is still a draft but it's a start to a solution for #412. In the comments on that issue I came up with two solutions and I'm using the one I think is best. I'm open to feedback and willing to change this if we think there's a better solution.

This is most likely an issue that I'll revisit in the future since @jwhite242 and I have talked about creating a one-size-fits all walltime value in Maestro at some point.

[koning]

The flux calls need a different time format, should have tests for slurm, flux and pbs for worker alloc as well as run:

flux:
-t, --time-limit=FSD
Set a time limit for the job in Flux standard duration (RFC 23).
FSD is a floating point number with a single character units
suffix ("s", "m", "h", or "d"). If unspecified, the job is sub-
ject to the system default time limit.

slurm:
-t, --time=
Set a limit on the total run time of the job allocation. If the
requested time limit exceeds the partition's time limit, the job
will be left in a PENDING state (possibly indefinitely). The
default time limit is the partition's default time limit. When
the time limit is reached, each task in each job step is sent
SIGTERM followed by SIGKILL. The interval between signals is
specified by the Slurm configuration parameter KillWait. The
OverTimeLimit configuration parameter may permit the job to run
longer than scheduled. Time resolution is one minute and second
values are rounded up to the next minute.

          A time limit of zero requests that no  time  limit  be  imposed.
          Acceptable  time  formats  include "minutes", "minutes:seconds",
          "hours:minutes:seconds", "days-hours", "days-hours:minutes"  and
          "days-hours:minutes:seconds".

pbs:
qsub -l walltime=hours:minutes:seconds script

[lucpeterson]

I think instead we should use this function:
merlin.utils.convert_timestring

merlin/merlin/utils.py

Line 547 in 7e07b72

def convert_timestring(timestring: Union[str, int], format_method: str = "HMS") -> str:

it takes a string and returns another string in the right format, which is an argument, and already has hooks into the proper pre presentation for slurm and flux. It also checks for ints vs strings

Implementing should be something like (bad pseudo code)
if batch==flux: format_method=“FSD”
if batch==slurm: format_method=“HMS”
walltime = convert_timestring(yaml_str, format_method=format_method)

etc

[koning]

Yes, that function is used in the scriptadapter but not the batch alloc yet.

[bgunnar5]

Whoops I somehow didn't see that the convert_timestring function took in ints, I'll try switching to that.

Should we just ignore the walltime value if lsf is the scheduler since it doesn't seem to have a walltime flag? I did a little digging and they do have a walltime flag (-W) for bmod but not one for jsrun that I can find.

[koning]

I think your original plan to keep the string in the resulting yaml is a good one. We can just use convert_timestring internally. I don't think jsrun has a walltime flag either. On LSF you really need the bsub/jsrun combo. So the ultimate solution will be to emit a bsub script with the jsrun launch.

[bgunnar5]

I have a feeling that's going to require a lot of changes so maybe we hold off on that in this PR?

[koning]

Yes, that is a future improvement and a low priority.

[bgunnar5]

I got the nod of approval on this from both Luc and Joe via teams so I'm merging this now.