From 948d97c96d1bb5059c8507eba6b4bbd72397521e Mon Sep 17 00:00:00 2001 From: amandarichardsonn <30413257+amandarichardsonn@users.noreply.github.com> Date: Wed, 31 Jan 2024 15:34:57 -0600 Subject: [PATCH] Validate Slurm Timing format (#471) This PR merges in functionality to validate the timing format when requesting a slurm allocation. Previously, no check was required leading to the WLM responsibility to throw an error. With the new code, SmartSim will catch and throw. [ reviewed by @MattToast ] [ committed by @amandarichardsonn ] --- smartsim/settings/slurmSettings.py | 29 ++++++++++++++++++++----- smartsim/wlm/slurm.py | 22 ++++++++++++++++++- tests/full_wlm/test_slurm_allocation.py | 23 ++++++++++++++++++++ tests/test_slurm_get_alloc.py | 4 ++-- 4 files changed, 69 insertions(+), 9 deletions(-) diff --git a/smartsim/settings/slurmSettings.py b/smartsim/settings/slurmSettings.py index 8da8659e1..09689366e 100644 --- a/smartsim/settings/slurmSettings.py +++ b/smartsim/settings/slurmSettings.py @@ -256,13 +256,9 @@ def _fmt_walltime(hours: int, minutes: int, seconds: int) -> str: :param seconds: number of seconds to run job :type seconds: int :returns: Formatted walltime - :rtype + :rtype: str """ - delta = datetime.timedelta(hours=hours, minutes=minutes, seconds=seconds) - fmt_str = str(delta) - if delta.seconds // 3600 < 10: - fmt_str = "0" + fmt_str - return fmt_str + return fmt_walltime(hours, minutes, seconds) def set_walltime(self, walltime: str) -> None: """Set the walltime of the job @@ -390,6 +386,27 @@ def format_comma_sep_env_vars(self) -> t.Tuple[str, t.List[str]]: return fmt_exported_env, compound_env +def fmt_walltime(hours: int, minutes: int, seconds: int) -> str: + """Helper function walltime format conversion + + Converts time to format HH:MM:SS + + :param hours: number of hours to run job + :type hours: int + :param minutes: number of minutes to run job + :type minutes: int + :param seconds: number of seconds to run job + :type seconds: int + :returns: Formatted walltime + :rtype: str + """ + delta = datetime.timedelta(hours=hours, minutes=minutes, seconds=seconds) + fmt_str = str(delta) + if delta.seconds // 3600 < 10: + fmt_str = "0" + fmt_str + return fmt_str + + class SbatchSettings(BatchSettings): def __init__( self, diff --git a/smartsim/wlm/slurm.py b/smartsim/wlm/slurm.py index ba46fb64c..4fbe5a96f 100644 --- a/smartsim/wlm/slurm.py +++ b/smartsim/wlm/slurm.py @@ -39,6 +39,7 @@ SSReservedKeywordError, ) from ..log import get_logger +from ..settings.slurmSettings import fmt_walltime logger = get_logger(__name__) @@ -248,7 +249,7 @@ def _get_alloc_cmd( "SmartSim", ] if time: - salloc_args.extend(["-t", time]) + salloc_args.extend(["-t", _validate_time_format(time)]) if account: salloc_args.extend(["-A", str(account)]) @@ -273,6 +274,25 @@ def _get_alloc_cmd( return salloc_args +def _validate_time_format(time: str) -> str: + """Convert time into valid walltime format + + By defualt the formatted wall time is the total number of seconds. + + :param time: number of hours to run job + :type time: str + :returns: Formatted walltime + :rtype: str + """ + try: + hours, minutes, seconds = map(int, time.split(":")) + except ValueError as e: + raise ValueError( + "Input time must be formatted as `HH:MM:SS` with valid Integers." + ) from e + return fmt_walltime(hours, minutes, seconds) + + def get_hosts() -> t.List[str]: """Get the name of the nodes used in a slurm allocation. diff --git a/tests/full_wlm/test_slurm_allocation.py b/tests/full_wlm/test_slurm_allocation.py index 01d40bf2f..76867d3dd 100644 --- a/tests/full_wlm/test_slurm_allocation.py +++ b/tests/full_wlm/test_slurm_allocation.py @@ -36,6 +36,29 @@ pytestmark = pytest.mark.skip(reason="Test is only for Slurm WLM systems") +def test_invalid_time_format(wlmutils): + """test slurm interface for formatting walltimes""" + account = wlmutils.get_test_account() + with pytest.raises(ValueError) as e: + alloc = slurm.get_allocation(nodes=1, time="000500", account=account) + assert ( + "Input time must be formatted as `HH:MM:SS` with valid Integers." + in e.value.args[0] + ) + with pytest.raises(ValueError) as e: + alloc = slurm.get_allocation(nodes=1, time="00-05-00", account=account) + assert ( + "Input time must be formatted as `HH:MM:SS` with valid Integers." + in e.value.args[0] + ) + with pytest.raises(ValueError) as e: + alloc = slurm.get_allocation(nodes=1, time="TE:HE:HE", account=account) + assert ( + "Input time must be formatted as `HH:MM:SS` with valid Integers." + in e.value.args[0] + ) + + def test_get_release_allocation(wlmutils): """test slurm interface for obtaining allocations""" account = wlmutils.get_test_account() diff --git a/tests/test_slurm_get_alloc.py b/tests/test_slurm_get_alloc.py index 270bbf014..72a208c2f 100644 --- a/tests/test_slurm_get_alloc.py +++ b/tests/test_slurm_get_alloc.py @@ -33,7 +33,7 @@ def test_get_alloc_format(): - time = "10:00:00" + time = "10:00:70" nodes = 5 account = "A35311" options = {"ntasks-per-node": 5} @@ -45,7 +45,7 @@ def test_get_alloc_format(): "-J", "SmartSim", "-t", - "10:00:00", + "10:01:10", "-A", "A35311", "--ntasks-per-node=5",