PR review comments addressed

Author: juliaputko

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:

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:

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:

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)
 

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)
 

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)
 

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)