From 6aa99915643ef83fafb3eee5e2cdfe15ca745317 Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Wed, 9 Oct 2024 15:57:17 -0700 Subject: [PATCH 1/4] tests and value checks for batch and launch settings --- smartsim/settings/batch_settings.py | 19 ++++++++++++ smartsim/settings/launch_settings.py | 27 +++++++++++++++++ .../test_settings/test_batchSettings.py | 30 +++++++++++++++++++ .../test_settings/test_launchSettings.py | 26 ++++++++++++++++ 4 files changed, 102 insertions(+) diff --git a/smartsim/settings/batch_settings.py b/smartsim/settings/batch_settings.py index 734e919ce..a88600d9f 100644 --- a/smartsim/settings/batch_settings.py +++ b/smartsim/settings/batch_settings.py @@ -117,6 +117,15 @@ def __init__( """The scheduler type""" except ValueError: raise ValueError(f"Invalid scheduler type: {batch_scheduler}") from None + + if batch_args: + if not ( + isinstance(batch_args, t.Mapping) + and all(isinstance(key, str) for key, val in batch_args.items()) + ): + raise TypeError( + "batch_args argument was not of type mapping of str and str" + ) self._arguments = self._get_arguments(batch_args) """The BatchSettings child class based on scheduler type""" self.env_vars = env_vars or {} @@ -140,6 +149,16 @@ def env_vars(self) -> StringArgument: @env_vars.setter def env_vars(self, value: t.Dict[str, str | None]) -> None: """Set the environment variables.""" + + if not ( + isinstance(value, t.Mapping) + and all( + isinstance(key, str) and isinstance(val, str) + for key, val in value.items() + ) + ): + raise TypeError("env_vars argument was not of type dic of str and str") + self._env_vars = copy.deepcopy(value) def _get_arguments(self, batch_args: StringArgument | None) -> BatchArguments: diff --git a/smartsim/settings/launch_settings.py b/smartsim/settings/launch_settings.py index 7b6083022..9e37e8ff3 100644 --- a/smartsim/settings/launch_settings.py +++ b/smartsim/settings/launch_settings.py @@ -126,6 +126,15 @@ def __init__( """The launcher type""" except ValueError: raise ValueError(f"Invalid launcher type: {launcher}") + + if launch_args: + if not ( + isinstance(launch_args, t.Mapping) + and all(isinstance(key, str) for key, val in launch_args.items()) + ): + raise TypeError( + "batch_args argument was not of type mapping of str and str" + ) self._arguments = self._get_arguments(launch_args) """The LaunchSettings child class based on launcher type""" self.env_vars = env_vars or {} @@ -165,6 +174,15 @@ def env_vars(self, value: dict[str, str | None]) -> None: :param value: The new environment mapping """ + if not ( + isinstance(value, t.Mapping) + and all( + isinstance(key, str) and isinstance(val, str) + for key, val in value.items() + ) + ): + raise TypeError("env_vars argument was not of type dic of str and str") + self._env_vars = copy.deepcopy(value) def _get_arguments(self, launch_args: StringArgument | None) -> LaunchArguments: @@ -209,6 +227,15 @@ def update_env(self, env_vars: t.Dict[str, str | None]) -> None: :param env_vars: environment variables to update or add :raises TypeError: if env_vars values cannot be coerced to strings """ + if not ( + isinstance(env_vars, t.Mapping) + and all( + isinstance(key, str) and isinstance(val, str) + for key, val in env_vars.items() + ) + ): + raise TypeError("env_vars argument was not of type dic of str and str") + # Coerce env_vars values to str as a convenience to user for env, val in env_vars.items(): if not isinstance(env, str): diff --git a/tests/temp_tests/test_settings/test_batchSettings.py b/tests/temp_tests/test_settings/test_batchSettings.py index 37fd3a33f..212ef9af1 100644 --- a/tests/temp_tests/test_settings/test_batchSettings.py +++ b/tests/temp_tests/test_settings/test_batchSettings.py @@ -78,3 +78,33 @@ def test_env_vars_property(): assert bs.env_vars == {"ENV": "VAR"} ref = bs.env_vars assert ref is bs.env_vars + + +def test_type_batch_scheduler(): + batch_scheduler = "invalid" + with pytest.raises(ValueError, match="Invalid scheduler type: invalid"): + BatchSettings( + batch_scheduler=batch_scheduler, + batch_args={"launch": "var"}, + env_vars={"ENV": "VAR"}, + ) + + +def test_type_batch_args(): + batch_args = "invalid" + with pytest.raises( + TypeError, match="batch_args argument was not of type mapping of str and str" + ): + BatchSettings( + batch_scheduler="slurm", + batch_args=batch_args, + env_vars={"ENV": "VAR"}, + ) + + +def test_type_env_vars(): + env_vars = "invalid" + with pytest.raises( + TypeError, match="env_vars argument was not of type dic of str and str" + ): + BatchSettings(batch_scheduler="slurm", env_vars=env_vars) diff --git a/tests/temp_tests/test_settings/test_launchSettings.py b/tests/temp_tests/test_settings/test_launchSettings.py index 3fc5e544a..b80da973e 100644 --- a/tests/temp_tests/test_settings/test_launchSettings.py +++ b/tests/temp_tests/test_settings/test_launchSettings.py @@ -87,3 +87,29 @@ def test_update_env_vars_errors(): # and that the function is atomic ls.update_env({"test": "test", "test": 1}) assert ls.env_vars == {"ENV": "VAR"} + + +def test_type_launcher(): + launcher = "invalid" + with pytest.raises(ValueError, match="Invalid launcher type: invalid"): + LaunchSettings( + launcher=launcher, launch_args={"launch": "var"}, env_vars={"ENV": "VAR"} + ) + + +def test_type_launch_args(): + launch_args = "invalid" + with pytest.raises( + TypeError, match="batch_args argument was not of type mapping of str and str" + ): + LaunchSettings( + launcher="local", launch_args=launch_args, env_vars={"ENV": "VAR"} + ) + + +def test_type_env_vars(): + env_vars = "invalid" + with pytest.raises( + TypeError, match="env_vars argument was not of type dic of str and str" + ): + LaunchSettings(launcher="local", env_vars=env_vars) From 67ba3a52e5046c7b042a8896b6ab315b405eb96b Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Tue, 15 Oct 2024 10:55:46 -0700 Subject: [PATCH 2/4] addressing review comments; added value checks for batch and launch arguments --- smartsim/settings/arguments/batch/lsf.py | 25 ++++- smartsim/settings/arguments/batch/pbs.py | 27 +++++- smartsim/settings/arguments/batch/slurm.py | 19 +++- smartsim/settings/batch_settings.py | 9 +- smartsim/settings/launch_settings.py | 17 +--- .../test_settings/test_batchSettings.py | 93 ++++++++++++++++++- .../test_settings/test_launchSettings.py | 12 ++- 7 files changed, 168 insertions(+), 34 deletions(-) diff --git a/smartsim/settings/arguments/batch/lsf.py b/smartsim/settings/arguments/batch/lsf.py index 23f948bd0..6daed05d0 100644 --- a/smartsim/settings/arguments/batch/lsf.py +++ b/smartsim/settings/arguments/batch/lsf.py @@ -72,7 +72,10 @@ def set_smts(self, smts: int) -> None: takes precedence. :param smts: SMT (e.g on Summit: 1, 2, or 4) + :raises TypeError: if not an int """ + if not isinstance(smts, int): + raise TypeError("smts argument was not of type int") self.set("alloc_flags", str(smts)) def set_project(self, project: str) -> None: @@ -81,7 +84,10 @@ def set_project(self, project: str) -> None: This sets ``-P``. :param time: project name + :raises TypeError: if not a str """ + if not isinstance(project, str): + raise TypeError("project argument was not of type str") self.set("P", project) def set_account(self, account: str) -> None: @@ -90,7 +96,10 @@ def set_account(self, account: str) -> None: this function is an alias for `set_project`. :param account: project name + :raises TypeError: if not a str """ + if not isinstance(account, str): + raise TypeError("account argument was not of type str") return self.set_project(account) def set_nodes(self, num_nodes: int) -> None: @@ -99,7 +108,10 @@ def set_nodes(self, num_nodes: int) -> None: This sets ``-nnodes``. :param nodes: number of nodes + :raises TypeError: if not an int """ + if not isinstance(num_nodes, int): + raise TypeError("num_nodes argument was not of type int") self.set("nnodes", str(num_nodes)) def set_hostlist(self, host_list: t.Union[str, t.List[str]]) -> None: @@ -110,10 +122,11 @@ def set_hostlist(self, host_list: t.Union[str, t.List[str]]) -> None: """ if isinstance(host_list, str): host_list = [host_list.strip()] - if not isinstance(host_list, list): + if not ( + isinstance(host_list, list) + and all(isinstance(item, str) for item in host_list) + ): raise TypeError("host_list argument must be a list of strings") - if not all(isinstance(host, str) for host in host_list): - raise TypeError("host_list argument must be list of strings") self.set("m", '"' + " ".join(host_list) + '"') def set_tasks(self, tasks: int) -> None: @@ -122,7 +135,10 @@ def set_tasks(self, tasks: int) -> None: This sets ``-n`` :param tasks: number of tasks + :raises TypeError: if not an int """ + if not isinstance(tasks, int): + raise TypeError("tasks argument was not of type int") self.set("n", str(tasks)) def set_queue(self, queue: str) -> None: @@ -131,7 +147,10 @@ def set_queue(self, queue: str) -> None: This sets ``-q`` :param queue: The queue to submit the job on + :raises TypeError: if not a str """ + if not isinstance(queue, str): + raise TypeError("queue argument was not of type str") self.set("q", queue) def format_batch_args(self) -> t.List[str]: diff --git a/smartsim/settings/arguments/batch/pbs.py b/smartsim/settings/arguments/batch/pbs.py index 126207665..de9194ecb 100644 --- a/smartsim/settings/arguments/batch/pbs.py +++ b/smartsim/settings/arguments/batch/pbs.py @@ -26,6 +26,7 @@ from __future__ import annotations +import re import typing as t from copy import deepcopy @@ -61,7 +62,10 @@ def set_nodes(self, num_nodes: int) -> None: nodes here is sets the 'nodes' resource. :param num_nodes: number of nodes + :raises TypeError: if not an int """ + if not isinstance(num_nodes, int): + raise TypeError("num_nodes argument was not of type int") self.set("nodes", str(num_nodes)) @@ -73,9 +77,10 @@ def set_hostlist(self, host_list: t.Union[str, t.List[str]]) -> None: """ if isinstance(host_list, str): host_list = [host_list.strip()] - if not isinstance(host_list, list): - raise TypeError("host_list argument must be a list of strings") - if not all(isinstance(host, str) for host in host_list): + if not ( + isinstance(host_list, list) + and all(isinstance(item, str) for item in host_list) + ): raise TypeError("host_list argument must be a list of strings") self.set("hostname", ",".join(host_list)) @@ -89,14 +94,22 @@ def set_walltime(self, walltime: str) -> None: this value will be overridden :param walltime: wall time + :raises ValueError: if walltime format is invalid """ - self.set("walltime", walltime) + pattern = r"^\d{2}:\d{2}:\d{2}$" + if walltime and re.match(pattern, walltime): + self.set("walltime", walltime) + else: + raise ValueError("Invalid walltime format. Please use 'HH:MM:SS' format.") def set_queue(self, queue: str) -> None: """Set the queue for the batch job :param queue: queue name + :raises TypeError: if not a str """ + if not isinstance(queue, str): + raise TypeError("queue argument was not of type str") self.set("q", str(queue)) def set_ncpus(self, num_cpus: int) -> None: @@ -107,14 +120,20 @@ def set_ncpus(self, num_cpus: int) -> None: this value will be overridden :param num_cpus: number of cpus per node in select + :raises TypeError: if not an int """ + if not isinstance(num_cpus, int): + raise TypeError("num_cpus argument was not of type int") self.set("ppn", str(num_cpus)) def set_account(self, account: str) -> None: """Set the account for this batch job :param acct: account id + :raises TypeError: if not a str """ + if not isinstance(account, str): + raise TypeError("account argument was not of type str") self.set("A", str(account)) def format_batch_args(self) -> t.List[str]: diff --git a/smartsim/settings/arguments/batch/slurm.py b/smartsim/settings/arguments/batch/slurm.py index 26f9cf854..bb71b161a 100644 --- a/smartsim/settings/arguments/batch/slurm.py +++ b/smartsim/settings/arguments/batch/slurm.py @@ -56,6 +56,7 @@ def set_walltime(self, walltime: str) -> None: format = "HH:MM:SS" :param walltime: wall time + :raises ValueError: if walltime format is invalid """ pattern = r"^\d{2}:\d{2}:\d{2}$" if walltime and re.match(pattern, walltime): @@ -69,7 +70,10 @@ def set_nodes(self, num_nodes: int) -> None: This sets ``--nodes``. :param num_nodes: number of nodes + :raises TypeError: if not an int """ + if not isinstance(num_nodes, int): + raise TypeError("num_nodes argument was not of type int") self.set("nodes", str(num_nodes)) def set_account(self, account: str) -> None: @@ -78,7 +82,10 @@ def set_account(self, account: str) -> None: This sets ``--account``. :param account: account id + :raises TypeError: if not a str """ + if not isinstance(account, str): + raise TypeError("account argument was not of type str") self.set("account", account) def set_partition(self, partition: str) -> None: @@ -96,7 +103,10 @@ def set_queue(self, queue: str) -> None: Sets the partition for the slurm batch job :param queue: the partition to run the batch job on + :raises TypeError: if not a str """ + if not isinstance(queue, str): + raise TypeError("queue argument was not of type str") return self.set_partition(queue) def set_cpus_per_task(self, cpus_per_task: int) -> None: @@ -118,10 +128,13 @@ def set_hostlist(self, host_list: t.Union[str, t.List[str]]) -> None: """ if isinstance(host_list, str): host_list = [host_list.strip()] - if not isinstance(host_list, list): + + if not ( + isinstance(host_list, list) + and all(isinstance(item, str) for item in host_list) + ): raise TypeError("host_list argument must be a list of strings") - if not all(isinstance(host, str) for host in host_list): - raise TypeError("host_list argument must be list of strings") + self.set("nodelist", ",".join(host_list)) def format_batch_args(self) -> t.List[str]: diff --git a/smartsim/settings/batch_settings.py b/smartsim/settings/batch_settings.py index a88600d9f..e2da28a54 100644 --- a/smartsim/settings/batch_settings.py +++ b/smartsim/settings/batch_settings.py @@ -118,9 +118,9 @@ def __init__( except ValueError: raise ValueError(f"Invalid scheduler type: {batch_scheduler}") from None - if batch_args: + if batch_args is not None: if not ( - isinstance(batch_args, t.Mapping) + isinstance(batch_args, dict) and all(isinstance(key, str) for key, val in batch_args.items()) ): raise TypeError( @@ -152,10 +152,7 @@ def env_vars(self, value: t.Dict[str, str | None]) -> None: if not ( isinstance(value, t.Mapping) - and all( - isinstance(key, str) and isinstance(val, str) - for key, val in value.items() - ) + and all(isinstance(key, str) for key, val in value.items()) ): raise TypeError("env_vars argument was not of type dic of str and str") diff --git a/smartsim/settings/launch_settings.py b/smartsim/settings/launch_settings.py index 9e37e8ff3..2edad4159 100644 --- a/smartsim/settings/launch_settings.py +++ b/smartsim/settings/launch_settings.py @@ -127,7 +127,7 @@ def __init__( except ValueError: raise ValueError(f"Invalid launcher type: {launcher}") - if launch_args: + if launch_args is not None: if not ( isinstance(launch_args, t.Mapping) and all(isinstance(key, str) for key, val in launch_args.items()) @@ -175,11 +175,8 @@ def env_vars(self, value: dict[str, str | None]) -> None: :param value: The new environment mapping """ if not ( - isinstance(value, t.Mapping) - and all( - isinstance(key, str) and isinstance(val, str) - for key, val in value.items() - ) + isinstance(value, dict) + and all(isinstance(key, str) for key, val in value.items()) ): raise TypeError("env_vars argument was not of type dic of str and str") @@ -227,14 +224,6 @@ def update_env(self, env_vars: t.Dict[str, str | None]) -> None: :param env_vars: environment variables to update or add :raises TypeError: if env_vars values cannot be coerced to strings """ - if not ( - isinstance(env_vars, t.Mapping) - and all( - isinstance(key, str) and isinstance(val, str) - for key, val in env_vars.items() - ) - ): - raise TypeError("env_vars argument was not of type dic of str and str") # Coerce env_vars values to str as a convenience to user for env, val in env_vars.items(): diff --git a/tests/temp_tests/test_settings/test_batchSettings.py b/tests/temp_tests/test_settings/test_batchSettings.py index 212ef9af1..5db3c16dd 100644 --- a/tests/temp_tests/test_settings/test_batchSettings.py +++ b/tests/temp_tests/test_settings/test_batchSettings.py @@ -90,8 +90,16 @@ def test_type_batch_scheduler(): ) -def test_type_batch_args(): - batch_args = "invalid" +@pytest.mark.parametrize( + "batch_args", + [ + pytest.param("invalid", id="invalid"), + pytest.param("", id="empty string"), + pytest.param(0, id="0"), + pytest.param([], id="empty list"), + ], +) +def test_type_batch_args(batch_args): with pytest.raises( TypeError, match="batch_args argument was not of type mapping of str and str" ): @@ -108,3 +116,84 @@ def test_type_env_vars(): TypeError, match="env_vars argument was not of type dic of str and str" ): BatchSettings(batch_scheduler="slurm", env_vars=env_vars) + + +@pytest.mark.parametrize( + "scheduler", + [ + pytest.param("slurm", id="slurm scheduler"), + pytest.param("lsf", id="bsub scheduler"), + pytest.param("pbs", id="qsub scheduler"), + ], +) +def test_batch_arguments_type_set_nodes(scheduler): + bs = BatchSettings(batch_scheduler=scheduler, env_vars={"ENV": "VAR"}) + with pytest.raises(TypeError, match="num_nodes argument was not of type int"): + bs.batch_args.set_nodes("invalid") + + +@pytest.mark.parametrize( + "scheduler", + [ + pytest.param("slurm", id="slurm scheduler"), + pytest.param("lsf", id="bsub scheduler"), + pytest.param("pbs", id="qsub scheduler"), + ], +) +def test_batch_arguments_type_set_account(scheduler): + bs = BatchSettings(batch_scheduler=scheduler, env_vars={"ENV": "VAR"}) + + with pytest.raises(TypeError, match="account argument was not of type str"): + bs.batch_args.set_account(27) + + +@pytest.mark.parametrize( + "scheduler", + [ + pytest.param("slurm", id="slurm scheduler"), + pytest.param("lsf", id="bsub scheduler"), + pytest.param("pbs", id="qsub scheduler"), + ], +) +def test_batch_arguments_type_set_queue(scheduler): + bs = BatchSettings(batch_scheduler=scheduler, env_vars={"ENV": "VAR"}) + with pytest.raises(TypeError, match="queue argument was not of type str"): + bs.batch_args.set_queue(27) + + +@pytest.mark.parametrize( + "scheduler", + [ + pytest.param("slurm", id="slurm scheduler"), + pytest.param("lsf", id="bsub scheduler"), + pytest.param("pbs", id="qsub scheduler"), + ], +) +def test_batch_arguments_type_set_hostlist(scheduler): + bs = BatchSettings(batch_scheduler=scheduler, env_vars={"ENV": "VAR"}) + with pytest.raises(TypeError, match="host_list argument must be a list of strings"): + bs.batch_args.set_hostlist([25, 37]) + + +def test_batch_arguments_type_set_ncpus(): + bs = BatchSettings(batch_scheduler="pbs", env_vars={"ENV": "VAR"}) + with pytest.raises(TypeError, match="num_cpus argument was not of type int"): + bs.batch_args.set_ncpus("invalid") + + +def test_batch_arguments_type_set_smts(): + bs = BatchSettings(batch_scheduler="lsf", env_vars={"ENV": "VAR"}) + with pytest.raises(TypeError, match="smts argument was not of type int"): + bs.batch_args.set_smts("invalid") + + +def test_batch_arguments_type_set_project(): + bs = BatchSettings(batch_scheduler="lsf", env_vars={"ENV": "VAR"}) + with pytest.raises(TypeError, match="project argument was not of type str"): + bs.batch_args.set_project(27) + + +def test_batch_arguments_type_set_tasks(): + bs = BatchSettings(batch_scheduler="lsf", env_vars={"ENV": "VAR"}) + with pytest.raises(TypeError, match="tasks argument was not of type int"): + bs.batch_args.set_tasks("invalid") diff --git a/tests/temp_tests/test_settings/test_launchSettings.py b/tests/temp_tests/test_settings/test_launchSettings.py index b80da973e..fdaf581c0 100644 --- a/tests/temp_tests/test_settings/test_launchSettings.py +++ b/tests/temp_tests/test_settings/test_launchSettings.py @@ -97,8 +97,16 @@ def test_type_launcher(): ) -def test_type_launch_args(): - launch_args = "invalid" +@pytest.mark.parametrize( + "launch_args", + [ + pytest.param("invalid", id="invalid"), + pytest.param("", id="empty string"), + pytest.param(0, id="0"), + pytest.param([], id="empty list"), + ], +) +def test_type_launch_args(launch_args): with pytest.raises( TypeError, match="batch_args argument was not of type mapping of str and str" ): From bb04488ebdab2e3f05927e3b0b990969c8bd0bfb Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Thu, 17 Oct 2024 14:23:51 -0700 Subject: [PATCH 3/4] more value checking, move tests to launcher specific tests --- smartsim/settings/arguments/batch/lsf.py | 3 ++ smartsim/settings/arguments/batch/pbs.py | 7 +++- smartsim/settings/arguments/batch/slurm.py | 11 +++++- .../test_settings/test_batchSettings.py | 38 ------------------- .../test_settings/test_lsfScheduler.py | 30 +++++++++++++++ .../test_settings/test_pbsScheduler.py | 18 +++++++++ .../test_settings/test_slurmScheduler.py | 24 ++++++++++++ 7 files changed, 90 insertions(+), 41 deletions(-) diff --git a/smartsim/settings/arguments/batch/lsf.py b/smartsim/settings/arguments/batch/lsf.py index 6daed05d0..7289a5dd3 100644 --- a/smartsim/settings/arguments/batch/lsf.py +++ b/smartsim/settings/arguments/batch/lsf.py @@ -57,8 +57,11 @@ def set_walltime(self, walltime: str) -> None: :param walltime: Time in hh:mm format, e.g. "10:00" for 10 hours, if time is supplied in hh:mm:ss format, seconds will be ignored and walltime will be set as ``hh:mm`` + :raises TypeError: if not type str """ # For compatibility with other launchers, as explained in docstring + if not isinstance(walltime, str): + raise TypeError("walltime argument was not of type str") if walltime: if len(walltime.split(":")) > 2: walltime = ":".join(walltime.split(":")[:2]) diff --git a/smartsim/settings/arguments/batch/pbs.py b/smartsim/settings/arguments/batch/pbs.py index de9194ecb..ba248d1fb 100644 --- a/smartsim/settings/arguments/batch/pbs.py +++ b/smartsim/settings/arguments/batch/pbs.py @@ -95,7 +95,10 @@ def set_walltime(self, walltime: str) -> None: :param walltime: wall time :raises ValueError: if walltime format is invalid + :raises TypeError: if not type str """ + if not isinstance(walltime, str): + raise TypeError("walltime argument was not of type str") pattern = r"^\d{2}:\d{2}:\d{2}$" if walltime and re.match(pattern, walltime): self.set("walltime", walltime) @@ -110,7 +113,7 @@ def set_queue(self, queue: str) -> None: """ if not isinstance(queue, str): raise TypeError("queue argument was not of type str") - self.set("q", str(queue)) + self.set("q", queue) def set_ncpus(self, num_cpus: int) -> None: """Set the number of cpus obtained in each node. @@ -134,7 +137,7 @@ def set_account(self, account: str) -> None: """ if not isinstance(account, str): raise TypeError("account argument was not of type str") - self.set("A", str(account)) + self.set("A", account) def format_batch_args(self) -> t.List[str]: """Get the formatted batch arguments for a preview diff --git a/smartsim/settings/arguments/batch/slurm.py b/smartsim/settings/arguments/batch/slurm.py index bb71b161a..51a99f7bb 100644 --- a/smartsim/settings/arguments/batch/slurm.py +++ b/smartsim/settings/arguments/batch/slurm.py @@ -57,7 +57,10 @@ def set_walltime(self, walltime: str) -> None: :param walltime: wall time :raises ValueError: if walltime format is invalid + :raises TypeError: if not str """ + if not isinstance(walltime, str): + raise TypeError("walltime argument was not of type str") pattern = r"^\d{2}:\d{2}:\d{2}$" if walltime and re.match(pattern, walltime): self.set("time", str(walltime)) @@ -94,8 +97,11 @@ def set_partition(self, partition: str) -> None: This sets ``--partition``. :param partition: partition name + :raises TypeError: if not a str """ - self.set("partition", str(partition)) + if not isinstance(partition, str): + raise TypeError("partition argument was not of type str") + self.set("partition", partition) def set_queue(self, queue: str) -> None: """alias for set_partition @@ -115,7 +121,10 @@ def set_cpus_per_task(self, cpus_per_task: int) -> None: This sets ``--cpus-per-task`` :param num_cpus: number of cpus to use per task + :raises TypeError: if not int """ + if not isinstance(cpus_per_task, int): + raise TypeError("cpus_per_task argument was not of type int") self.set("cpus-per-task", str(cpus_per_task)) def set_hostlist(self, host_list: t.Union[str, t.List[str]]) -> None: diff --git a/tests/temp_tests/test_settings/test_batchSettings.py b/tests/temp_tests/test_settings/test_batchSettings.py index 5db3c16dd..01e61214b 100644 --- a/tests/temp_tests/test_settings/test_batchSettings.py +++ b/tests/temp_tests/test_settings/test_batchSettings.py @@ -159,41 +159,3 @@ def test_batch_arguments_type_set_queue(scheduler): bs = BatchSettings(batch_scheduler=scheduler, env_vars={"ENV": "VAR"}) with pytest.raises(TypeError, match="queue argument was not of type str"): bs.batch_args.set_queue(27) - - -@pytest.mark.parametrize( - "scheduler", - [ - pytest.param("slurm", id="slurm scheduler"), - pytest.param("lsf", id="bsub scheduler"), - pytest.param("pbs", id="qsub scheduler"), - ], -) -def test_batch_arguments_type_set_hostlist(scheduler): - bs = BatchSettings(batch_scheduler=scheduler, env_vars={"ENV": "VAR"}) - with pytest.raises(TypeError, match="host_list argument must be a list of strings"): - bs.batch_args.set_hostlist([25, 37]) - - -def test_batch_arguments_type_set_ncpus(): - bs = BatchSettings(batch_scheduler="pbs", env_vars={"ENV": "VAR"}) - with pytest.raises(TypeError, match="num_cpus argument was not of type int"): - bs.batch_args.set_ncpus("invalid") - - -def test_batch_arguments_type_set_smts(): - bs = BatchSettings(batch_scheduler="lsf", env_vars={"ENV": "VAR"}) - with pytest.raises(TypeError, match="smts argument was not of type int"): - bs.batch_args.set_smts("invalid") - - -def test_batch_arguments_type_set_project(): - bs = BatchSettings(batch_scheduler="lsf", env_vars={"ENV": "VAR"}) - with pytest.raises(TypeError, match="project argument was not of type str"): - bs.batch_args.set_project(27) - - -def test_batch_arguments_type_set_tasks(): - bs = BatchSettings(batch_scheduler="lsf", env_vars={"ENV": "VAR"}) - with pytest.raises(TypeError, match="tasks argument was not of type int"): - bs.batch_args.set_tasks("invalid") diff --git a/tests/temp_tests/test_settings/test_lsfScheduler.py b/tests/temp_tests/test_settings/test_lsfScheduler.py index 5e6b7fd0c..81ab25c8a 100644 --- a/tests/temp_tests/test_settings/test_lsfScheduler.py +++ b/tests/temp_tests/test_settings/test_lsfScheduler.py @@ -75,3 +75,33 @@ def test_create_bsub(): lsfScheduler.batch_args.set_queue("default") args = lsfScheduler.format_batch_args() assert args == ["-core_isolation", "-nnodes", "1", "-W", "10:10", "-q", "default"] + + +def test_batch_arguments_type_set_hostlist(scheduler): + bs = BatchSettings(batch_scheduler="lsf", env_vars={"ENV": "VAR"}) + with pytest.raises(TypeError, match="host_list argument must be a list of strings"): + bs.batch_args.set_hostlist([25, 37]) + + +def test_batch_arguments_type_set_smts(): + bs = BatchSettings(batch_scheduler="lsf", env_vars={"ENV": "VAR"}) + with pytest.raises(TypeError, match="smts argument was not of type int"): + bs.batch_args.set_smts("invalid") + + +def test_batch_arguments_type_set_project(): + bs = BatchSettings(batch_scheduler="lsf", env_vars={"ENV": "VAR"}) + with pytest.raises(TypeError, match="project argument was not of type str"): + bs.batch_args.set_project(27) + + +def test_batch_arguments_type_set_tasks(): + bs = BatchSettings(batch_scheduler="lsf", env_vars={"ENV": "VAR"}) + with pytest.raises(TypeError, match="tasks argument was not of type int"): + bs.batch_args.set_tasks("invalid") + + +def test_batch_arguments_type_set_walltime(): + bs = BatchSettings(batch_scheduler="lsf", env_vars={"ENV": "VAR"}) + with pytest.raises(TypeError, match="walltime argument was not of type str"): + bs.batch_args.set_walltime(27) diff --git a/tests/temp_tests/test_settings/test_pbsScheduler.py b/tests/temp_tests/test_settings/test_pbsScheduler.py index 36fde6776..718311a32 100644 --- a/tests/temp_tests/test_settings/test_pbsScheduler.py +++ b/tests/temp_tests/test_settings/test_pbsScheduler.py @@ -86,3 +86,21 @@ def test_format_pbs_batch_args(): "-A", "myproject", ] + + +def test_batch_arguments_type_set_hostlist(scheduler): + bs = BatchSettings(batch_scheduler="pbs", env_vars={"ENV": "VAR"}) + with pytest.raises(TypeError, match="host_list argument must be a list of strings"): + bs.batch_args.set_hostlist([25, 37]) + + +def test_batch_arguments_type_set_ncpus(): + bs = BatchSettings(batch_scheduler="pbs", env_vars={"ENV": "VAR"}) + with pytest.raises(TypeError, match="num_cpus argument was not of type int"): + bs.batch_args.set_ncpus("invalid") + + +def test_batch_arguments_type_set_walltime(): + bs = BatchSettings(batch_scheduler="pbs", env_vars={"ENV": "VAR"}) + with pytest.raises(TypeError, match="walltime argument was not of type str"): + bs.batch_args.set_walltime(27) diff --git a/tests/temp_tests/test_settings/test_slurmScheduler.py b/tests/temp_tests/test_settings/test_slurmScheduler.py index 8ab489cc8..b0f32b7a2 100644 --- a/tests/temp_tests/test_settings/test_slurmScheduler.py +++ b/tests/temp_tests/test_settings/test_slurmScheduler.py @@ -134,3 +134,27 @@ def test_sbatch_manual(): formatted = slurmScheduler.format_batch_args() result = ["--nodes=5", "--account=A3531", "--time=10:00:00"] assert formatted == result + + +def test_batch_arguments_type_set_walltime(): + bs = BatchSettings(batch_scheduler="slurm", env_vars={"ENV": "VAR"}) + with pytest.raises(TypeError, match="walltime argument was not of type str"): + bs.batch_args.set_walltime(27) + + +def test_batch_arguments_type_set_cpus_per_task(): + bs = BatchSettings(batch_scheduler="slurm", env_vars={"ENV": "VAR"}) + with pytest.raises(TypeError, match="cpus_per_task argument was not of type int"): + bs.batch_args.set_cpus_per_task("invalid") + + +def test_batch_arguments_type_set_partition(): + bs = BatchSettings(batch_scheduler="slurm", env_vars={"ENV": "VAR"}) + with pytest.raises(TypeError, match="partition argument was not of type str"): + bs.batch_args.set_partition(27) + + +def test_batch_arguments_type_set_hostlist(scheduler): + bs = BatchSettings(batch_scheduler="slurm", env_vars={"ENV": "VAR"}) + with pytest.raises(TypeError, match="host_list argument must be a list of strings"): + bs.batch_args.set_hostlist([25, 37]) From e9d6ed8fb5799e69d32869fee34a554b9bdf8751 Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Wed, 23 Oct 2024 15:19:52 -0700 Subject: [PATCH 4/4] PR review comments addressed --- smartsim/settings/arguments/batch/lsf.py | 9 +++++++++ smartsim/settings/arguments/batch/pbs.py | 6 ++++++ smartsim/settings/arguments/batch/slurm.py | 6 ++++++ smartsim/settings/batch_settings.py | 16 +++++++++++----- smartsim/settings/launch_settings.py | 16 +++++++++++----- .../test_settings/test_batchSettings.py | 6 ++++-- .../test_settings/test_launchSettings.py | 6 ++++-- 7 files changed, 51 insertions(+), 14 deletions(-) diff --git a/smartsim/settings/arguments/batch/lsf.py b/smartsim/settings/arguments/batch/lsf.py index 7289a5dd3..ea5747f44 100644 --- a/smartsim/settings/arguments/batch/lsf.py +++ b/smartsim/settings/arguments/batch/lsf.py @@ -76,9 +76,12 @@ def set_smts(self, smts: int) -> None: :param smts: SMT (e.g on Summit: 1, 2, or 4) :raises TypeError: if not an int + :raises ValueError: if not positive int """ if not isinstance(smts, int): raise TypeError("smts argument was not of type int") + if smts <= 0: + raise ValueError("smts must be a positive value") self.set("alloc_flags", str(smts)) def set_project(self, project: str) -> None: @@ -112,9 +115,12 @@ def set_nodes(self, num_nodes: int) -> None: :param nodes: number of nodes :raises TypeError: if not an int + :raises ValueError: if not positive int """ if not isinstance(num_nodes, int): raise TypeError("num_nodes argument was not of type int") + if num_nodes <= 0: + raise ValueError("Number of nodes must be a positive value") self.set("nnodes", str(num_nodes)) def set_hostlist(self, host_list: t.Union[str, t.List[str]]) -> None: @@ -139,9 +145,12 @@ def set_tasks(self, tasks: int) -> None: :param tasks: number of tasks :raises TypeError: if not an int + :raises ValueError: if not positive int """ if not isinstance(tasks, int): raise TypeError("tasks argument was not of type int") + if tasks <= 0: + raise ValueError("Number of tasks must be a positive value") self.set("n", str(tasks)) def set_queue(self, queue: str) -> None: diff --git a/smartsim/settings/arguments/batch/pbs.py b/smartsim/settings/arguments/batch/pbs.py index ba248d1fb..c2affd88b 100644 --- a/smartsim/settings/arguments/batch/pbs.py +++ b/smartsim/settings/arguments/batch/pbs.py @@ -63,9 +63,12 @@ def set_nodes(self, num_nodes: int) -> None: :param num_nodes: number of nodes :raises TypeError: if not an int + :raises ValueError: if not positive int """ if not isinstance(num_nodes, int): raise TypeError("num_nodes argument was not of type int") + if num_nodes <= 0: + raise ValueError("Number of nodes must be a positive value") self.set("nodes", str(num_nodes)) @@ -124,9 +127,12 @@ def set_ncpus(self, num_cpus: int) -> None: :param num_cpus: number of cpus per node in select :raises TypeError: if not an int + :raises ValueError: if not positive int """ if not isinstance(num_cpus, int): raise TypeError("num_cpus argument was not of type int") + if num_cpus <= 0: + raise ValueError("Number of CPUs must be a positive value") self.set("ppn", str(num_cpus)) def set_account(self, account: str) -> None: diff --git a/smartsim/settings/arguments/batch/slurm.py b/smartsim/settings/arguments/batch/slurm.py index 51a99f7bb..6a9d6e340 100644 --- a/smartsim/settings/arguments/batch/slurm.py +++ b/smartsim/settings/arguments/batch/slurm.py @@ -74,9 +74,12 @@ def set_nodes(self, num_nodes: int) -> None: :param num_nodes: number of nodes :raises TypeError: if not an int + :raises ValueError: if not positive int """ if not isinstance(num_nodes, int): raise TypeError("num_nodes argument was not of type int") + if num_nodes <= 0: + raise ValueError("Number of nodes must be a positive value") self.set("nodes", str(num_nodes)) def set_account(self, account: str) -> None: @@ -122,9 +125,12 @@ def set_cpus_per_task(self, cpus_per_task: int) -> None: :param num_cpus: number of cpus to use per task :raises TypeError: if not int + :raises ValueError: if not positive int """ if not isinstance(cpus_per_task, int): raise TypeError("cpus_per_task argument was not of type int") + if cpus_per_task <= 0: + raise ValueError("CPUs per task must be a positive value") self.set("cpus-per-task", str(cpus_per_task)) def set_hostlist(self, host_list: t.Union[str, t.List[str]]) -> None: diff --git a/smartsim/settings/batch_settings.py b/smartsim/settings/batch_settings.py index e2da28a54..7219e5e70 100644 --- a/smartsim/settings/batch_settings.py +++ b/smartsim/settings/batch_settings.py @@ -121,10 +121,13 @@ def __init__( if batch_args is not None: if not ( isinstance(batch_args, dict) - and all(isinstance(key, str) for key, val in batch_args.items()) + and all( + isinstance(key, str) and isinstance(val, (str, type(None))) + for key, val in batch_args.items() + ) ): raise TypeError( - "batch_args argument was not of type mapping of str and str" + "batch_args argument was not of type dict of str and str or None" ) self._arguments = self._get_arguments(batch_args) """The BatchSettings child class based on scheduler type""" @@ -151,10 +154,13 @@ def env_vars(self, value: t.Dict[str, str | None]) -> None: """Set the environment variables.""" if not ( - isinstance(value, t.Mapping) - and all(isinstance(key, str) for key, val in value.items()) + isinstance(value, t.Dict) + and all( + isinstance(key, str) and isinstance(val, (str, type(None))) + for key, val in value.items() + ) ): - raise TypeError("env_vars argument was not of type dic of str and str") + raise TypeError("env_vars argument was not of type dict of str and str") self._env_vars = copy.deepcopy(value) diff --git a/smartsim/settings/launch_settings.py b/smartsim/settings/launch_settings.py index 2edad4159..6ed7695e4 100644 --- a/smartsim/settings/launch_settings.py +++ b/smartsim/settings/launch_settings.py @@ -129,11 +129,14 @@ def __init__( if launch_args is not None: if not ( - isinstance(launch_args, t.Mapping) - and all(isinstance(key, str) for key, val in launch_args.items()) + isinstance(launch_args, t.Dict) + and all( + isinstance(key, str) and isinstance(val, (str, type(None))) + for key, val in launch_args.items() + ) ): raise TypeError( - "batch_args argument was not of type mapping of str and str" + "launch_args argument was not of type dict of str and str or None" ) self._arguments = self._get_arguments(launch_args) """The LaunchSettings child class based on launcher type""" @@ -176,9 +179,12 @@ def env_vars(self, value: dict[str, str | None]) -> None: """ if not ( isinstance(value, dict) - and all(isinstance(key, str) for key, val in value.items()) + and all( + isinstance(key, str) and isinstance(val, (str, type(None))) + for key, val in value.items() + ) ): - raise TypeError("env_vars argument was not of type dic of str and str") + raise TypeError("env_vars argument was not of type dict of str and str") self._env_vars = copy.deepcopy(value) diff --git a/tests/temp_tests/test_settings/test_batchSettings.py b/tests/temp_tests/test_settings/test_batchSettings.py index 01e61214b..67cbc15fe 100644 --- a/tests/temp_tests/test_settings/test_batchSettings.py +++ b/tests/temp_tests/test_settings/test_batchSettings.py @@ -97,11 +97,13 @@ def test_type_batch_scheduler(): pytest.param("", id="empty string"), pytest.param(0, id="0"), pytest.param([], id="empty list"), + pytest.param({"valid": 1}, id="value not str or None"), ], ) def test_type_batch_args(batch_args): with pytest.raises( - TypeError, match="batch_args argument was not of type mapping of str and str" + TypeError, + match="batch_args argument was not of type dict of str and str or None", ): BatchSettings( batch_scheduler="slurm", @@ -113,7 +115,7 @@ def test_type_batch_args(batch_args): def test_type_env_vars(): env_vars = "invalid" with pytest.raises( - TypeError, match="env_vars argument was not of type dic of str and str" + TypeError, match="env_vars argument was not of type dict of str and str" ): BatchSettings(batch_scheduler="slurm", env_vars=env_vars) diff --git a/tests/temp_tests/test_settings/test_launchSettings.py b/tests/temp_tests/test_settings/test_launchSettings.py index fdaf581c0..303b41eb0 100644 --- a/tests/temp_tests/test_settings/test_launchSettings.py +++ b/tests/temp_tests/test_settings/test_launchSettings.py @@ -104,11 +104,13 @@ def test_type_launcher(): pytest.param("", id="empty string"), pytest.param(0, id="0"), pytest.param([], id="empty list"), + pytest.param({"valid", 2}, id="val not str or None"), ], ) def test_type_launch_args(launch_args): with pytest.raises( - TypeError, match="batch_args argument was not of type mapping of str and str" + TypeError, + match="launch_args argument was not of type dict of str and str or None", ): LaunchSettings( launcher="local", launch_args=launch_args, env_vars={"ENV": "VAR"} @@ -118,6 +120,6 @@ def test_type_launch_args(launch_args): def test_type_env_vars(): env_vars = "invalid" with pytest.raises( - TypeError, match="env_vars argument was not of type dic of str and str" + TypeError, match="env_vars argument was not of type dict of str and str" ): LaunchSettings(launcher="local", env_vars=env_vars)