From e17ccd283643a0fcadc0ee35a46c27cb94bcdabd Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Mon, 16 Sep 2024 16:46:28 -0700 Subject: [PATCH 01/14] value checking for application, ensemble, experiment --- smartsim/_core/utils/helpers.py | 2 + smartsim/entity/application.py | 2 +- smartsim/entity/ensemble.py | 11 ++-- smartsim/experiment.py | 16 +++-- tests/test_application.py | 113 ++++++++++++++++++++++++++++++++ tests/test_ensemble.py | 46 ++++++++++--- tests/test_experiment.py | 37 +++++++++-- 7 files changed, 202 insertions(+), 25 deletions(-) create mode 100644 tests/test_application.py diff --git a/smartsim/_core/utils/helpers.py b/smartsim/_core/utils/helpers.py index 1133358a67..9fdfaf6d46 100644 --- a/smartsim/_core/utils/helpers.py +++ b/smartsim/_core/utils/helpers.py @@ -140,6 +140,8 @@ def expand_exe_path(exe: str) -> str: :raises FileNotFoundError: if executable cannot be found """ + if not exe: + raise ValueError("No executable provided") # which returns none if not found in_path = which(exe) if not in_path: diff --git a/smartsim/entity/application.py b/smartsim/entity/application.py index a8302fc1ff..77d5269459 100644 --- a/smartsim/entity/application.py +++ b/smartsim/entity/application.py @@ -247,7 +247,7 @@ def attached_files_table(self) -> str: :returns: String version of table """ if not self.files: - return "No file attached to this application." + raise ValueError("No file attached to this application.") return str(self.files) @staticmethod diff --git a/smartsim/entity/ensemble.py b/smartsim/entity/ensemble.py index f228c4a8af..7d243c0fdf 100644 --- a/smartsim/entity/ensemble.py +++ b/smartsim/entity/ensemble.py @@ -255,7 +255,10 @@ def _create_applications(self) -> tuple[Application, ...]: ) def as_jobs(self, settings: LaunchSettings) -> tuple[Job, ...]: - apps = self._create_applications() - if not apps: - raise ValueError("There are no members as part of this ensemble") - return tuple(Job(app, settings, app.name) for app in apps) + if not settings is None: + apps = self._create_applications() + if not apps: + raise ValueError("There are no members as part of this ensemble") + return tuple(Job(app, settings, app.name) for app in apps) + else: + raise ValueError("The Launch Settings provided are empty") diff --git a/smartsim/experiment.py b/smartsim/experiment.py index 24709ccfd0..b62ef6896b 100644 --- a/smartsim/experiment.py +++ b/smartsim/experiment.py @@ -159,11 +159,14 @@ def start(self, *jobs: Job) -> tuple[LaunchedJobID, ...]: jobs that can be used to query or alter the status of that particular execution of the job. """ - # Create the run id - run_id = datetime.datetime.now().replace(microsecond=0).isoformat() - # Generate the root path - root = pathlib.Path(self.exp_path, run_id) - return self._dispatch(Generator(root), dispatch.DEFAULT_DISPATCHER, *jobs) + if jobs: + # Create the run id + run_id = datetime.datetime.now().replace(microsecond=0).isoformat() + # Generate the root path + root = pathlib.Path(self.exp_path, run_id) + return self._dispatch(Generator(root), dispatch.DEFAULT_DISPATCHER, *jobs) + else: + raise ValueError("No job ids provided to start") def _dispatch( self, @@ -240,6 +243,9 @@ def get_status( :returns: A tuple of statuses with order respective of the order of the calling arguments. """ + if not ids: + raise ValueError("No job ids provided to get status") + to_query = self._launch_history.group_by_launcher( set(ids), unknown_ok=True ).items() diff --git a/tests/test_application.py b/tests/test_application.py new file mode 100644 index 0000000000..154b503423 --- /dev/null +++ b/tests/test_application.py @@ -0,0 +1,113 @@ +# BSD 2-Clause License +# +# Copyright (c) 2021-2024, Hewlett Packard Enterprise +# All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are met: +# +# 1. Redistributions of source code must retain the above copyright notice, this +# list of conditions and the following disclaimer. +# +# 2. Redistributions in binary form must reproduce the above copyright notice, +# this list of conditions and the following disclaimer in the documentation +# and/or other materials provided with the distribution. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" +# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE +# DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE +# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL +# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR +# SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER +# CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +from glob import glob +from os import path as osp + +import pytest + +from smartsim.entity.application import Application +from smartsim.entity.files import EntityFiles +from smartsim.settings.launchSettings import LaunchSettings + +pytestmark = pytest.mark.group_a + + +@pytest.fixture +def get_gen_configure_dir(fileutils): + yield fileutils.get_test_conf_path(osp.join("generator_files", "tag_dir_template")) + + +@pytest.fixture +def mock_launcher_settings(wlmutils): + return LaunchSettings(wlmutils.get_test_launcher(), {}, {}) + + +def test_application_exe_property(): + a = Application( + "test_name", + exe="echo", + exe_args=["spam", "eggs"], + ) + exe = a.exe + assert exe == a.exe + + +def test_application_exe_args_property(): + a = Application("test_name", exe="echo", exe_args=["spam", "eggs"]) + exe_args = a.exe_args + assert exe_args == a.exe_args + + +def test_application_files_property(get_gen_configure_dir): + tagged_files = sorted(glob(get_gen_configure_dir + "/*")) + files = EntityFiles(tagged=tagged_files) + a = Application("test_name", exe="echo", exe_args=["spam", "eggs"], files=files) + files = a.files + assert files == a.files + + +def test_application_file_parameters_property(): + file_parameters = {"h": [5, 6, 7, 8]} + a = Application( + "test_name", + exe="echo", + file_parameters=file_parameters, + ) + file_parameters = a.file_parameters + + assert file_parameters == a.file_parameters + + +def test_application_incoming_entities_property(): + """Assert that incoming entities can be registered on the Application""" + application = Application( + "test_name", + exe="echo", + exe_args=["spam", "eggs"], + ) + application.incoming_entities = ["ensemble_0"] + assert len(application.incoming_entities) == 1 + + +def test_application_key_prefixing_property(): + key_prefixing_enabled = True + a = Application("test_name", exe="echo", exe_args=["spam", "eggs"]) + key_prefixing_enabled = a.key_prefixing_enabled + assert key_prefixing_enabled == a.key_prefixing_enabled + + +def test_empty_executable(): + """Test that an error is raised when the exe property is empty""" + with pytest.raises(ValueError): + Application(name="application", exe=None, exe_args=None) + + +def test_application_attached_files(): + """Test that an error is raised when there are no files attached to an application""" + a = Application("test_name", exe="echo", files=None) + with pytest.raises(ValueError): + a.attached_files_table diff --git a/tests/test_ensemble.py b/tests/test_ensemble.py index 5198681fe1..8915f96cab 100644 --- a/tests/test_ensemble.py +++ b/tests/test_ensemble.py @@ -34,6 +34,8 @@ from smartsim.entity.ensemble import Ensemble from smartsim.entity.files import EntityFiles from smartsim.entity.strategies import ParamSet +from smartsim.error.errors import SmartSimError +from smartsim.experiment import Experiment from smartsim.settings.launchSettings import LaunchSettings pytestmark = pytest.mark.group_a @@ -47,6 +49,19 @@ def get_gen_configure_dir(fileutils): yield fileutils.get_test_conf_path(osp.join("generator_files", "tag_dir_template")) +def user_created_function( + file_params: t.Mapping[str, t.Sequence[str]], + exe_arg_params: t.Mapping[str, t.Sequence[t.Sequence[str]]], + n_permutations: int = 0, +) -> list[ParamSet]: + return [ParamSet({}, {})] + + +@pytest.fixture +def mock_launcher_settings(wlmutils): + return LaunchSettings(wlmutils.get_test_launcher(), {}, {}) + + def test_exe_property(): e = Ensemble(name="test", exe="path/to/example_simulation_program") exe = e.exe @@ -86,21 +101,32 @@ def test_file_parameters_property(): file_parameters=file_parameters, ) file_parameters = e.file_parameters - assert file_parameters == e.file_parameters -def user_created_function( - file_params: t.Mapping[str, t.Sequence[str]], - exe_arg_params: t.Mapping[str, t.Sequence[t.Sequence[str]]], - n_permutations: int = 0, -) -> list[ParamSet]: - return [ParamSet({}, {})] +def test_ensemble_init_empty_params(test_dir: str) -> None: + """params supplied without run settings""" + exp = Experiment("test", exp_path=test_dir) + with pytest.raises(TypeError): + Ensemble() -@pytest.fixture -def mock_launcher_settings(wlmutils): - return LaunchSettings(wlmutils.get_test_launcher(), {}, {}) +def test_ensemble_as_jobs(): + """Test a call to as_jobs with empty launchsettings""" + ensemble = Ensemble("ensemble-name", "echo", replicas=2) + launch_settings = None + with pytest.raises(ValueError): + ensemble.as_jobs(launch_settings) + + +def test_ensemble_no_launch_settings(test_dir): + """test starting an ensemble with invalid launch settings""" + ensemble = Ensemble("ensemble-name", "echo", replicas=2) + launch_settings = "invalid" + job_list = ensemble.as_jobs(launch_settings) + exp = Experiment(name="exp_name", exp_path=test_dir) + with pytest.raises(AttributeError): + exp.start(*job_list) def test_ensemble_user_created_strategy(mock_launcher_settings, test_dir): diff --git a/tests/test_experiment.py b/tests/test_experiment.py index aff32604c0..dd7b4a771b 100644 --- a/tests/test_experiment.py +++ b/tests/test_experiment.py @@ -214,11 +214,6 @@ def as_executable_sequence(self): return ("echo", "Hello", "World!") -def test_start_raises_if_no_args_supplied(experiment): - with pytest.raises(TypeError, match="missing 1 required positional argument"): - experiment.start() - - # fmt: off @pytest.mark.parametrize( "num_jobs", [pytest.param(i, id=f"{i} job(s)") for i in (1, 2, 3, 5, 10, 100, 1_000)] @@ -611,3 +606,35 @@ def test_experiment_stop_does_not_raise_on_unknown_job_id( assert stat == InvalidJobStatus.NEVER_STARTED after_cancel = exp.get_status(*all_known_ids) assert before_cancel == after_cancel + + +def test_start_raises_if_no_args_supplied(test_dir): + exp = Experiment(name="exp_name", exp_path=test_dir) + with pytest.raises(ValueError, match="No job ids provided to start"): + exp.start() + + +def test_stop_raises_if_no_args_supplied(test_dir): + exp = Experiment(name="exp_name", exp_path=test_dir) + with pytest.raises(ValueError, match="No job ids provided"): + exp.stop() + + +def test_get_status_raises_if_no_args_supplied(test_dir): + exp = Experiment(name="exp_name", exp_path=test_dir) + with pytest.raises(ValueError, match="No job ids provided"): + exp.get_status() + + +def test_poll_raises_if_no_args_supplied(test_dir): + exp = Experiment(name="exp_name", exp_path=test_dir) + with pytest.raises( + TypeError, match="missing 2 required positional arguments: 'ids' and 'statuses'" + ): + exp._poll_for_statuses() + + +def test_wait_raises_if_no_args_supplied(test_dir): + exp = Experiment(name="exp_name", exp_path=test_dir) + with pytest.raises(ValueError, match="No job ids to wait on provided"): + exp.wait() From 8a914a71a92aa0aad6c3cd2fd684b6dc9a0d6d02 Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Tue, 24 Sep 2024 15:04:47 -0700 Subject: [PATCH 02/14] value checking in setters and other fixes --- smartsim/_core/utils/helpers.py | 1 + smartsim/entity/application.py | 42 ++++++- smartsim/entity/ensemble.py | 85 +++++++++++++- smartsim/experiment.py | 34 +++++- smartsim/launchable/job.py | 24 +++- tests/temp_tests/test_launchable.py | 25 ++++ tests/test_application.py | 171 +++++++++++++++++++++------- tests/test_ensemble.py | 143 +++++++++++++++++++++-- tests/test_experiment.py | 32 +++++- 9 files changed, 495 insertions(+), 62 deletions(-) diff --git a/smartsim/_core/utils/helpers.py b/smartsim/_core/utils/helpers.py index 9fdfaf6d46..e577a8928d 100644 --- a/smartsim/_core/utils/helpers.py +++ b/smartsim/_core/utils/helpers.py @@ -136,6 +136,7 @@ def expand_exe_path(exe: str) -> str: """Takes an executable and returns the full path to that executable :param exe: executable or file + :raises ValueError: if no executable is provided :raises TypeError: if file is not an executable :raises FileNotFoundError: if executable cannot be found """ diff --git a/smartsim/entity/application.py b/smartsim/entity/application.py index 77d5269459..8133ede126 100644 --- a/smartsim/entity/application.py +++ b/smartsim/entity/application.py @@ -70,7 +70,7 @@ def __init__( """The executable to run""" self._exe_args = self._build_exe_args(exe_args) or [] """The executable arguments""" - self._files = copy.deepcopy(files) if files else None + self._files = copy.deepcopy(files) if files else EntityFiles() """Files to be copied, symlinked, and/or configured prior to execution""" self._file_parameters = ( copy.deepcopy(file_parameters) if file_parameters else {} @@ -94,7 +94,12 @@ def exe(self, value: str) -> None: """Set executable to run. :param value: executable to run + :raises TypeError: exe argument is not int """ + if value: + if not isinstance(value, str): + raise TypeError("exe argument was not of type str") + self._exe = copy.deepcopy(value) @property @@ -136,7 +141,12 @@ def files(self, value: t.Optional[EntityFiles]) -> None: execution. :param value: files + :raises TypeError: files argument was not of type int """ + if value: + if not isinstance(value, EntityFiles): + raise TypeError("files argument was not of type EntityFiles") + self._files = copy.deepcopy(value) @property @@ -152,7 +162,21 @@ def file_parameters(self, value: t.Mapping[str, str]) -> None: """Set the file parameters. :param value: file parameters + :raises TypeError: file_parameters argument is not a mapping of str and str """ + if value: + if not ( + isinstance(value, t.Mapping) + and ( + all( + isinstance(key, str) and isinstance(val, str) + for key, val in value.items() + ) + ) + ): + raise TypeError( + "file_parameters argument was not of type mapping of str and str" + ) self._file_parameters = copy.deepcopy(value) @property @@ -168,7 +192,16 @@ def incoming_entities(self, value: t.List[SmartSimEntity]) -> None: """Set the incoming entities. :param value: incoming entities + :raises TypeError: incoming_entities argument is not a list of SmartSimEntity """ + if value: + if not isinstance(value, list) or not all( + isinstance(x, SmartSimEntity) for x in value + ): + raise TypeError( + "incoming_entities argument was not of type list of SmartSimEntity" + ) + self._incoming_entities = copy.copy(value) @property @@ -184,7 +217,12 @@ def key_prefixing_enabled(self, value: bool) -> None: """Set whether key prefixing is enabled for the application. :param value: key prefixing enabled + :raises TypeError: key prefixings enabled argument was not of type bool """ + if value: + if not isinstance(value, bool): + raise TypeError("key_prefixing_enabled argument was not of type bool") + self.key_prefixing_enabled = copy.deepcopy(value) def as_executable_sequence(self) -> t.Sequence[str]: @@ -246,8 +284,6 @@ def attached_files_table(self) -> str: :returns: String version of table """ - if not self.files: - raise ValueError("No file attached to this application.") return str(self.files) @staticmethod diff --git a/smartsim/entity/ensemble.py b/smartsim/entity/ensemble.py index 7d243c0fdf..1204dc2e85 100644 --- a/smartsim/entity/ensemble.py +++ b/smartsim/entity/ensemble.py @@ -26,6 +26,7 @@ from __future__ import annotations +import collections import copy import itertools import os @@ -37,6 +38,7 @@ from smartsim.entity.files import EntityFiles from smartsim.entity.strategies import ParamSet from smartsim.launchable.job import Job +from smartsim.settings.launchSettings import LaunchSettings if t.TYPE_CHECKING: from smartsim.settings.launchSettings import LaunchSettings @@ -109,7 +111,12 @@ def exe(self, value: str | os.PathLike[str]) -> None: """Set executable to run. :param value: executable to run + :raises TypeError: if exe is not str or PathLike str """ + if value: + if not isinstance(value, (str, os.PathLike)): + raise TypeError("exe argument was not of type str or PathLike str") + self._exe = os.fspath(value) @property @@ -125,7 +132,15 @@ def exe_args(self, value: t.Sequence[str]) -> None: """Set the executable arguments. :param value: executable arguments + :raises TypeError: if exe_args is not sequence of str """ + if value: + if not ( + isinstance(value, collections.abc.Sequence) + and (all(isinstance(x, str) for x in value)) + ): + raise TypeError("exe_args argument was not of type sequence of str") + self._exe_args = list(value) @property @@ -143,7 +158,32 @@ def exe_arg_parameters( """Set the executable arguments. :param value: executable arguments + :raises TypeError: if exe_arg_parameters is not mapping + of str and sequences of sequences of strings """ + if value: + if not ( + isinstance(value, collections.abc.Mapping) + and ( + all( + isinstance(key, str) + and isinstance(val, collections.abc.Sequence) + and all( + isinstance(subval, collections.abc.Sequence) + for subval in val + ) + and all( + isinstance(item, str) + for item in itertools.chain.from_iterable(val) + ) + for key, val in value.items() + ) + ) + ): + raise TypeError( + "exe_arg_parameters argument was not of type mapping of str and sequences of sequences of strings" + ) + self._exe_arg_parameters = copy.deepcopy(value) @property @@ -161,7 +201,11 @@ def files(self, value: EntityFiles) -> None: execution. :param value: files + :raises TypeError: if files is not of type EntityFiles """ + if value: + if not isinstance(value, EntityFiles): + raise TypeError("files argument was not of type EntityFiles") self._files = copy.deepcopy(value) @property @@ -177,7 +221,24 @@ def file_parameters(self, value: t.Mapping[str, t.Sequence[str]]) -> None: """Set the file parameters. :param value: file parameters + :raises TypeError: if file_parameters is not a mapping of str and sequence of str """ + if value: + if not ( + isinstance(value, t.Mapping) + and ( + all( + isinstance(key, str) + and isinstance(val, collections.abc.Sequence) + and all(isinstance(subval, str) for subval in val) + for key, val in value.items() + ) + ) + ): + raise TypeError( + "file_parameters argument was not of type mapping of str and sequence of str" + ) + self._file_parameters = dict(value) @property @@ -195,7 +256,13 @@ def permutation_strategy( """Set the permutation strategy :param value: permutation strategy + :raises TypeError: if permutation_strategy is not str or PermutationStrategyType """ + if value: + if not isinstance(value, t.Callable): + raise TypeError( + "permutation_strategy argument was not of type str or PermutationStrategyType" + ) self._permutation_strategy = value @property @@ -210,8 +277,13 @@ def max_permutations(self) -> int: def max_permutations(self, value: int) -> None: """Set the maximum permutations - :param value: the maxpermutations + :param value: the max permutations + :raises TypeError: max_permutations argument was not of type int """ + if value: + if not isinstance(value, int): + raise TypeError("max_permutations argument was not of type int") + self._max_permutations = value @property @@ -227,7 +299,12 @@ def replicas(self, value: int) -> None: """Set the number of replicas :return: the number of replicas + :raises TypeError: replicas argument was not of type int """ + if value: + if not isinstance(value, int): + raise TypeError("replicas argument was not of type int") + self._replicas = value def _create_applications(self) -> tuple[Application, ...]: @@ -255,7 +332,11 @@ def _create_applications(self) -> tuple[Application, ...]: ) def as_jobs(self, settings: LaunchSettings) -> tuple[Job, ...]: - if not settings is None: + + if settings: + if not isinstance(settings, LaunchSettings): + raise TypeError("ids argument was not of type LaunchSettings") + apps = self._create_applications() if not apps: raise ValueError("There are no members as part of this ensemble") diff --git a/smartsim/experiment.py b/smartsim/experiment.py index b62ef6896b..991452fceb 100644 --- a/smartsim/experiment.py +++ b/smartsim/experiment.py @@ -41,6 +41,7 @@ from smartsim._core.control.launch_history import LaunchHistory as _LaunchHistory from smartsim._core.utils import helpers as _helpers from smartsim.error import errors +from smartsim.launchable.job import Job from smartsim.status import TERMINAL_STATUSES, InvalidJobStatus, JobStatus from ._core import Generator, Manifest, previewrenderer @@ -123,7 +124,11 @@ def __init__(self, name: str, exp_path: str | None = None): :param name: name for the ``Experiment`` :param exp_path: path to location of ``Experiment`` directory """ - if not name: + + if name: + if not isinstance(name, str): + raise TypeError("name argument was not of type str") + else: raise TypeError("Experiment name must be non-empty string") self.name = name @@ -155,18 +160,23 @@ def start(self, *jobs: Job) -> tuple[LaunchedJobID, ...]: """Execute a collection of `Job` instances. :param jobs: A collection of other job instances to start + :raises TypeError: If jobs provided are not the correct type + :raises ValueError: No Jobs were provided. :returns: A sequence of ids with order corresponding to the sequence of jobs that can be used to query or alter the status of that particular execution of the job. """ if jobs: + if not all(isinstance(job, Job) for job in jobs): + raise TypeError("jobs argument was not of type Job") + # Create the run id run_id = datetime.datetime.now().replace(microsecond=0).isoformat() # Generate the root path root = pathlib.Path(self.exp_path, run_id) return self._dispatch(Generator(root), dispatch.DEFAULT_DISPATCHER, *jobs) else: - raise ValueError("No job ids provided to start") + raise ValueError("No jobs provided to start") def _dispatch( self, @@ -240,10 +250,15 @@ def get_status( unique. :param ids: A sequence of launched job ids issued by the experiment. + :raises TypeError: If ids provided are not the correct type + :raises ValueError: No IDs were provided. :returns: A tuple of statuses with order respective of the order of the calling arguments. """ - if not ids: + if ids: + if not all(isinstance(id, str) for id in ids): + raise TypeError("ids argument was not of type LaunchedJobID") + else: raise ValueError("No job ids provided to get status") to_query = self._launch_history.group_by_launcher( @@ -254,6 +269,7 @@ def get_status( stats = (stats_map.get(i, InvalidJobStatus.NEVER_STARTED) for i in ids) return tuple(stats) + # TODO: LaunchedJobID isinstance check def wait( self, *ids: LaunchedJobID, timeout: float | None = None, verbose: bool = True ) -> None: @@ -263,9 +279,13 @@ def wait( :param ids: The ids of the launched jobs to wait for. :param timeout: The max time to wait for all of the launched jobs to end. :param verbose: Whether found statuses should be displayed in the console. + :raises TypeError: If IDs provided are not the correct type :raises ValueError: No IDs were provided. """ - if not ids: + if ids: + if not all(isinstance(id, str) for id in ids): + raise TypeError("ids argument was not of type LaunchedJobID") + else: raise ValueError("No job ids to wait on provided") self._poll_for_statuses( ids, TERMINAL_STATUSES, timeout=timeout, verbose=verbose @@ -430,11 +450,15 @@ def stop(self, *ids: LaunchedJobID) -> tuple[JobStatus | InvalidJobStatus, ...]: """Cancel the execution of a previously launched job. :param ids: The ids of the launched jobs to stop. + :raises TypeError: If ids provided are not the correct type :raises ValueError: No job ids were provided. :returns: A tuple of job statuses upon cancellation with order respective of the order of the calling arguments. """ - if not ids: + if ids: + if not all(isinstance(id, str) for id in ids): + raise TypeError("ids argument was not of type LaunchedJobID") + else: raise ValueError("No job ids provided") by_launcher = self._launch_history.group_by_launcher(set(ids), unknown_ok=True) id_to_stop_stat = ( diff --git a/smartsim/launchable/job.py b/smartsim/launchable/job.py index a433319ac4..8850e000bc 100644 --- a/smartsim/launchable/job.py +++ b/smartsim/launchable/job.py @@ -59,8 +59,8 @@ def __init__( name: str | None = None, ): super().__init__() - self._entity = deepcopy(entity) - self._launch_settings = deepcopy(launch_settings) + self.entity = entity + self.launch_settings = launch_settings self._name = name if name else entity.name check_name(self._name) @@ -83,7 +83,16 @@ def entity(self) -> SmartSimEntity: @entity.setter def entity(self, value: SmartSimEntity) -> None: - """Sets the Job entity.""" + """Sets the Job entity. + + :param value: entity + :raises Type Error: if entity is not SmartSimEntity + """ + from smartsim.entity.entity import SmartSimEntity + + if not isinstance(value, SmartSimEntity): + raise TypeError("entity argument was not of type SmartSimEntity") + self._entity = deepcopy(value) @property @@ -93,7 +102,14 @@ def launch_settings(self) -> LaunchSettings: @launch_settings.setter def launch_settings(self, value: LaunchSettings) -> None: - """Sets the Job LaunchSettings.""" + """Sets the Job LaunchSettings. + + :param value: launch settings + :raises Type Error: if launch_settings is not a LaunchSettings + """ + if not isinstance(value, LaunchSettings): + raise TypeError("launch_settings argument was not of type LaunchSettings") + self._launch_settings = deepcopy(value) def get_launch_steps(self) -> LaunchCommands: diff --git a/tests/temp_tests/test_launchable.py b/tests/temp_tests/test_launchable.py index 9b2adb3e7a..f43cc5c6e5 100644 --- a/tests/temp_tests/test_launchable.py +++ b/tests/temp_tests/test_launchable.py @@ -115,6 +115,31 @@ def test_job_init_deepcopy(): assert job.launch_settings.launcher is not test +def test_job_type_entity(): + entity = "invalid" + settings = LaunchSettings("slurm") + with pytest.raises( + TypeError, + match="entity argument was not of type SmartSimEntity", + ): + Job(entity, settings) + + +def test_job_type_launch_settings(): + entity = Application( + "test_name", + exe="echo", + exe_args=["spam", "eggs"], + ) + settings = "invalid" + + with pytest.raises( + TypeError, + match="launch_settings argument was not of type LaunchSettings", + ): + Job(entity, settings) + + def test_add_mpmd_pair(): entity = EchoHelloWorldEntity() diff --git a/tests/test_application.py b/tests/test_application.py index 154b503423..484736148b 100644 --- a/tests/test_application.py +++ b/tests/test_application.py @@ -46,68 +46,163 @@ def mock_launcher_settings(wlmutils): return LaunchSettings(wlmutils.get_test_launcher(), {}, {}) -def test_application_exe_property(): - a = Application( +def test_application_key_prefixing_property(): + key_prefixing_enabled = True + a = Application("test_name", exe="echo", exe_args=["spam", "eggs"]) + key_prefixing_enabled = a.key_prefixing_enabled + assert key_prefixing_enabled == a.key_prefixing_enabled + + +def test_empty_executable(): + """Test that an error is raised when the exe property is empty""" + with pytest.raises(ValueError): + Application(name="application", exe=None, exe_args=None) + + +def test_executable_is_not_none(): + """Test that an error is raised when the exe property is empty""" + exe = "" + with pytest.raises(ValueError): + Application(name="application", exe=exe, exe_args=None) + + +def test_type_exe(): + with pytest.raises(TypeError): + Application( + "test_name", + exe=2, + exe_args=["spam", "eggs"], + ) + + +def test_type_exe_args(): + application = Application( + "test_name", + exe="echo", + ) + with pytest.raises(TypeError): + application.exe_args = [1, 2, 3] + + +def test_type_files_property(): + application = Application( + "test_name", + exe="echo", + ) + with pytest.raises(TypeError): + application.files = "/path/to/file" + + +def test_type_file_parameters_property(): + application = Application( + "test_name", + exe="echo", + ) + with pytest.raises(TypeError): + application.file_parameters = {1: 2} + + +def test_type_incoming_entities(): + application = Application( "test_name", exe="echo", exe_args=["spam", "eggs"], ) - exe = a.exe - assert exe == a.exe + with pytest.raises(TypeError): + application.incoming_entities = [1, 2, 3] -def test_application_exe_args_property(): - a = Application("test_name", exe="echo", exe_args=["spam", "eggs"]) - exe_args = a.exe_args - assert exe_args == a.exe_args +# application type checks +def test_application_type_exe(): + application = Application( + "test_name", + exe="echo", + exe_args=["spam", "eggs"], + ) + with pytest.raises(TypeError, match="exe argument was not of type str"): + application.exe = 2 -def test_application_files_property(get_gen_configure_dir): - tagged_files = sorted(glob(get_gen_configure_dir + "/*")) - files = EntityFiles(tagged=tagged_files) - a = Application("test_name", exe="echo", exe_args=["spam", "eggs"], files=files) - files = a.files - assert files == a.files +def test_application_type_exe_args(): + application = Application( + "test_name", + exe="echo", + exe_args=["spam", "eggs"], + ) + with pytest.raises( + TypeError, match="Executable arguments were not a list of str or a str." + ): + application.exe_args = [1, 2, 3] -def test_application_file_parameters_property(): - file_parameters = {"h": [5, 6, 7, 8]} - a = Application( +def test_application_type_files(): + application = Application( "test_name", exe="echo", - file_parameters=file_parameters, + exe_args=["spam", "eggs"], ) - file_parameters = a.file_parameters + with pytest.raises(TypeError, match="files argument was not of type EntityFiles"): + application.files = 2 + + +@pytest.mark.parametrize( + "file_params", + ( + pytest.param(["invalid"], id="Not a mapping"), + pytest.param({"1": 2}, id="Value is not mapping of str and str"), + pytest.param({1: "2"}, id="Key is not mapping of str and str"), + pytest.param({1: 2}, id="Values not mapping of str and str"), + ), +) +def test_application_type_file_parameters(file_params): + application = Application( + "test_name", + exe="echo", + exe_args=["spam", "eggs"], + ) + with pytest.raises( + TypeError, + match="file_parameters argument was not of type mapping of str and str", + ): + application.file_parameters = file_params - assert file_parameters == a.file_parameters +def test_application_type_incoming_entities(): -def test_application_incoming_entities_property(): - """Assert that incoming entities can be registered on the Application""" application = Application( "test_name", exe="echo", exe_args=["spam", "eggs"], ) - application.incoming_entities = ["ensemble_0"] - assert len(application.incoming_entities) == 1 + with pytest.raises( + TypeError, + match="incoming_entities argument was not of type list of SmartSimEntity", + ): + application.incoming_entities = [1, 2, 3] -def test_application_key_prefixing_property(): - key_prefixing_enabled = True - a = Application("test_name", exe="echo", exe_args=["spam", "eggs"]) - key_prefixing_enabled = a.key_prefixing_enabled - assert key_prefixing_enabled == a.key_prefixing_enabled +def test_application_type_key_prefixing_enabled(): + application = Application( + "test_name", + exe="echo", + exe_args=["spam", "eggs"], + ) + with pytest.raises( + TypeError, + match="key_prefixing_enabled argument was not of type bool", + ): + application.key_prefixing_enabled = "invalid" -def test_empty_executable(): - """Test that an error is raised when the exe property is empty""" - with pytest.raises(ValueError): - Application(name="application", exe=None, exe_args=None) +def test_application_type_build_exe_args(): + application = Application( + "test_name", + exe="echo", + exe_args=["spam", "eggs"], + ) + with pytest.raises( + TypeError, match="Executable arguments were not a list of str or a str." + ): -def test_application_attached_files(): - """Test that an error is raised when there are no files attached to an application""" - a = Application("test_name", exe="echo", files=None) - with pytest.raises(ValueError): - a.attached_files_table + application.exe_args = [1, 2, 3] diff --git a/tests/test_ensemble.py b/tests/test_ensemble.py index 8915f96cab..8b6060283a 100644 --- a/tests/test_ensemble.py +++ b/tests/test_ensemble.py @@ -24,7 +24,6 @@ # OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -import itertools import typing as t from glob import glob from os import path as osp @@ -105,8 +104,7 @@ def test_file_parameters_property(): def test_ensemble_init_empty_params(test_dir: str) -> None: - """params supplied without run settings""" - exp = Experiment("test", exp_path=test_dir) + """Ensemble created without required args""" with pytest.raises(TypeError): Ensemble() @@ -119,14 +117,141 @@ def test_ensemble_as_jobs(): ensemble.as_jobs(launch_settings) -def test_ensemble_no_launch_settings(test_dir): +def test_ensemble_no_launch_settings(): """test starting an ensemble with invalid launch settings""" ensemble = Ensemble("ensemble-name", "echo", replicas=2) launch_settings = "invalid" - job_list = ensemble.as_jobs(launch_settings) - exp = Experiment(name="exp_name", exp_path=test_dir) - with pytest.raises(AttributeError): - exp.start(*job_list) + with pytest.raises(TypeError): + ensemble.as_jobs(launch_settings) + + +def test_ensemble_type_exe(): + ensemble = Ensemble( + "ensemble-name", + exe="valid", + exe_args=["spam", "eggs"], + ) + with pytest.raises( + TypeError, match="exe argument was not of type str or PathLike str" + ): + ensemble.exe = 2 + + +def test_ensemble_type_exe_args(): + ensemble = Ensemble( + "ensemble-name", + exe="echo", + ) + with pytest.raises( + TypeError, match="exe_args argument was not of type sequence of str" + ): + ensemble.exe_args = [1, 2, 3] + + +@pytest.mark.parametrize( + "exe_arg_params", + ( + pytest.param(["invalid"], id="Not a mapping"), + pytest.param({"key": [1, 2, 3]}, id="Value is not sequence of sequences"), + pytest.param( + {"key": [[1, 2, 3], [4, 5, 6]]}, + id="Value is not sequence of sequence of str", + ), + pytest.param( + {1: 2}, + id="key and value wrong type", + ), + pytest.param({"1": 2}, id="Value is not mapping of str and str"), + pytest.param({1: "2"}, id="Key is not str"), + pytest.param({1: 2}, id="Values not mapping of str and str"), + ), +) +def test_ensemble_type_exe_arg_parameters(exe_arg_params): + ensemble = Ensemble( + "ensemble-name", + exe="echo", + exe_args=["spam", "eggs"], + ) + with pytest.raises( + TypeError, + match="exe_arg_parameters argument was not of type mapping of str and sequences of sequences of strings", + ): + ensemble.exe_arg_parameters = exe_arg_params + + +def test_ensemble_type_files(): + ensemble = Ensemble( + "ensemble-name", + exe="echo", + exe_args=["spam", "eggs"], + ) + with pytest.raises(TypeError, match="files argument was not of type EntityFiles"): + ensemble.files = 2 + + +@pytest.mark.parametrize( + "file_params", + ( + pytest.param(["invalid"], id="Not a mapping"), + pytest.param({"key": [1, 2, 3]}, id="Key is not sequence of sequences"), + ), +) +def test_ensemble_type_file_parameters(file_params): + ensemble = Ensemble( + "ensemble-name", + exe="echo", + exe_args=["spam", "eggs"], + ) + with pytest.raises( + TypeError, + match="file_parameters argument was not of type mapping of str and sequence of str", + ): + ensemble.file_parameters = file_params + + +def test_ensemble_type_permutation_strategy(): + ensemble = Ensemble( + "ensemble-name", + exe="echo", + exe_args=["spam", "eggs"], + ) + with pytest.raises( + TypeError, + match="permutation_strategy argument was not of type str or PermutationStrategyType", + ): + ensemble.permutation_strategy = 2 + + +def test_ensemble_type_max_permutations(): + ensemble = Ensemble( + "ensemble-name", + exe="echo", + exe_args=["spam", "eggs"], + ) + with pytest.raises( + TypeError, + match="max_permutations argument was not of type int", + ): + ensemble.max_permutations = "invalid" + + +def test_ensemble_type_replicas(): + ensemble = Ensemble( + "ensemble-name", + exe="echo", + exe_args=["spam", "eggs"], + ) + with pytest.raises( + TypeError, + match="replicas argument was not of type int", + ): + ensemble.replicas = "invalid" + + +def test_ensemble_type_as_jobs(): + ensemble = Ensemble("ensemble-name", "echo", replicas=2) + with pytest.raises(TypeError): + ensemble.as_jobs("invalid") def test_ensemble_user_created_strategy(mock_launcher_settings, test_dir): @@ -238,7 +363,7 @@ def test_all_perm_strategy( assert len(jobs) == expected_num_jobs -def test_all_perm_strategy_contents(): +def test_all_perm_strategy_contents(mock_launcher_settings): jobs = Ensemble( "test_ensemble", "echo", diff --git a/tests/test_experiment.py b/tests/test_experiment.py index dd7b4a771b..d90109fc97 100644 --- a/tests/test_experiment.py +++ b/tests/test_experiment.py @@ -48,6 +48,7 @@ from smartsim.settings import launchSettings from smartsim.settings.arguments import launchArguments from smartsim.status import InvalidJobStatus, JobStatus +from smartsim.types import LaunchedJobID pytestmark = pytest.mark.group_a @@ -610,7 +611,7 @@ def test_experiment_stop_does_not_raise_on_unknown_job_id( def test_start_raises_if_no_args_supplied(test_dir): exp = Experiment(name="exp_name", exp_path=test_dir) - with pytest.raises(ValueError, match="No job ids provided to start"): + with pytest.raises(ValueError, match="No jobs provided to start"): exp.start() @@ -638,3 +639,32 @@ def test_wait_raises_if_no_args_supplied(test_dir): exp = Experiment(name="exp_name", exp_path=test_dir) with pytest.raises(ValueError, match="No job ids to wait on provided"): exp.wait() + + +def test_type_experiment_name_parameter(test_dir): + with pytest.raises(TypeError, match="name argument was not of type str"): + Experiment(name=1, exp_path=test_dir) + + +def test_type_start_parameters(test_dir): + exp = Experiment(name="exp_name", exp_path=test_dir) + with pytest.raises(TypeError, match="jobs argument was not of type Job"): + exp.start("invalid") + + +def test_type_get_status_parameters(test_dir): + exp = Experiment(name="exp_name", exp_path=test_dir) + with pytest.raises(TypeError, match="ids argument was not of type LaunchedJobID"): + exp.get_status(2) + + +def test_type_wait_parameter(test_dir): + exp = Experiment(name="exp_name", exp_path=test_dir) + with pytest.raises(TypeError, match="ids argument was not of type LaunchedJobID"): + exp.wait(2) + + +def test_type_stop_parameter(test_dir): + exp = Experiment(name="exp_name", exp_path=test_dir) + with pytest.raises(TypeError, match="ids argument was not of type LaunchedJobID"): + exp.stop(2) From 33d8e273431b814549c7353adedfc34a2974eac5 Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Wed, 25 Sep 2024 10:37:56 -0700 Subject: [PATCH 03/14] readded shallow copy checks into tests --- tests/test_application.py | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/tests/test_application.py b/tests/test_application.py index 484736148b..f0859cd67a 100644 --- a/tests/test_application.py +++ b/tests/test_application.py @@ -46,6 +46,42 @@ def mock_launcher_settings(wlmutils): return LaunchSettings(wlmutils.get_test_launcher(), {}, {}) +def test_application_exe_property(): + a = Application( + "test_name", + exe="echo", + exe_args=["spam", "eggs"], + ) + exe = a.exe + assert exe is a.exe + + +def test_application_exe_args_property(): + a = Application("test_name", exe="echo", exe_args=["spam", "eggs"]) + exe_args = a.exe_args + assert exe_args is a.exe_args + + +def test_application_files_property(get_gen_configure_dir): + tagged_files = sorted(glob(get_gen_configure_dir + "/*")) + files = EntityFiles(tagged=tagged_files) + a = Application("test_name", exe="echo", exe_args=["spam", "eggs"], files=files) + files = a.files + assert files is a.files + + +def test_application_file_parameters_property(): + file_parameters = {"h": [5, 6, 7, 8]} + a = Application( + "test_name", + exe="echo", + file_parameters=file_parameters, + ) + file_parameters = a.file_parameters + + assert file_parameters is a.file_parameters + + def test_application_key_prefixing_property(): key_prefixing_enabled = True a = Application("test_name", exe="echo", exe_args=["spam", "eggs"]) From 6937f20cbe8324fd2826fe34cb91b99e026afac5 Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Wed, 25 Sep 2024 13:17:07 -0700 Subject: [PATCH 04/14] import fix --- tests/test_application.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_application.py b/tests/test_application.py index f0859cd67a..7700f05299 100644 --- a/tests/test_application.py +++ b/tests/test_application.py @@ -31,7 +31,7 @@ from smartsim.entity.application import Application from smartsim.entity.files import EntityFiles -from smartsim.settings.launchSettings import LaunchSettings +from smartsim.settings.launch_settings import LaunchSettings pytestmark = pytest.mark.group_a From 5e8a0bc8c6b19c57557a46f1cf7d952ce8552c7b Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Wed, 25 Sep 2024 13:30:03 -0700 Subject: [PATCH 05/14] mypy fixes --- smartsim/builders/ensemble.py | 17 +++++++---------- smartsim/entity/application.py | 6 +++--- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/smartsim/builders/ensemble.py b/smartsim/builders/ensemble.py index 2dfd1ede12..d17d8bae13 100644 --- a/smartsim/builders/ensemble.py +++ b/smartsim/builders/ensemble.py @@ -315,7 +315,7 @@ def permutation_strategy( PermutationStrategyType """ if value: - if not isinstance(value, t.Callable): + if not callable(value): raise TypeError( "permutation_strategy argument was not of type str or PermutationStrategyType" ) @@ -424,12 +424,9 @@ def build_jobs(self, settings: LaunchSettings) -> tuple[Job, ...]: :raises TypeError: if the ids argument is not type LaunchSettings :raises ValueError: if the LaunchSettings provided are empty """ - if settings: - if not isinstance(settings, LaunchSettings): - raise TypeError("ids argument was not of type LaunchSettings") - apps = self._create_applications() - if not apps: - raise ValueError("There are no members as part of this ensemble") - return tuple(Job(app, settings, app.name) for app in apps) - else: - raise ValueError("The Launch Settings provided are empty") + if not isinstance(settings, LaunchSettings): + raise TypeError("ids argument was not of type LaunchSettings") + apps = self._create_applications() + if not apps: + raise ValueError("There are no members as part of this ensemble") + return tuple(Job(app, settings, app.name) for app in apps) diff --git a/smartsim/entity/application.py b/smartsim/entity/application.py index cadfae5915..853910f6f0 100644 --- a/smartsim/entity/application.py +++ b/smartsim/entity/application.py @@ -163,9 +163,9 @@ def files(self, value: EntityFiles) -> None: :raises TypeError: files argument was not of type int """ - if value: - if not isinstance(value, EntityFiles): - raise TypeError("files argument was not of type EntityFiles") + + if not isinstance(value, EntityFiles): + raise TypeError("files argument was not of type EntityFiles") self._files = copy.deepcopy(value) From d514867dc5cd75d6d1b49e5f78d1c799140f2c1c Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Wed, 25 Sep 2024 15:03:56 -0700 Subject: [PATCH 06/14] format fix --- smartsim/entity/application.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/smartsim/entity/application.py b/smartsim/entity/application.py index 853910f6f0..759ada93ff 100644 --- a/smartsim/entity/application.py +++ b/smartsim/entity/application.py @@ -337,8 +337,7 @@ def print_attached_files(self) -> None: def __str__(self) -> str: # pragma: no cover exe_args_str = "\n".join(self.exe_args) entities_str = "\n".join(str(entity) for entity in self.incoming_entities) - return textwrap.dedent( - f"""\ + return textwrap.dedent(f"""\ Name: {self.name} Type: {self.type} Executable: @@ -350,5 +349,4 @@ def __str__(self) -> str: # pragma: no cover Incoming Entities: {entities_str} Key Prefixing Enabled: {self.key_prefixing_enabled} - """ - ) + """) From eb4774d2508bb87d59c113896ff33a8c83a10421 Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Tue, 1 Oct 2024 16:15:25 -0700 Subject: [PATCH 07/14] null and empty string tests; param tests; format changes --- smartsim/builders/ensemble.py | 113 ++++++++++++++++----------------- smartsim/entity/application.py | 57 +++++++++-------- smartsim/experiment.py | 28 ++++---- tests/test_application.py | 8 +-- tests/test_ensemble.py | 38 ++++++----- 5 files changed, 124 insertions(+), 120 deletions(-) diff --git a/smartsim/builders/ensemble.py b/smartsim/builders/ensemble.py index d17d8bae13..2623203e62 100644 --- a/smartsim/builders/ensemble.py +++ b/smartsim/builders/ensemble.py @@ -167,9 +167,8 @@ def exe(self, value: str | os.PathLike[str]) -> None: :param value: the executable :raises TypeError: if the exe argument is not str or PathLike str """ - if value: - if not isinstance(value, (str, os.PathLike)): - raise TypeError("exe argument was not of type str or PathLike str") + if not isinstance(value, (str, os.PathLike)): + raise TypeError("exe argument was not of type str or PathLike str") self._exe = os.fspath(value) @@ -188,12 +187,12 @@ def exe_args(self, value: t.Sequence[str]) -> None: :param value: the executable arguments :raises TypeError: if exe_args is not sequence of str """ - if value: - if not ( - isinstance(value, collections.abc.Sequence) - and (all(isinstance(x, str) for x in value)) - ): - raise TypeError("exe_args argument was not of type sequence of str") + + if not ( + isinstance(value, collections.abc.Sequence) + and (all(isinstance(x, str) for x in value)) + ): + raise TypeError("exe_args argument was not of type sequence of str") self._exe_args = list(value) @@ -215,28 +214,28 @@ def exe_arg_parameters( :raises TypeError: if exe_arg_parameters is not mapping of str and sequences of sequences of strings """ - if value: - if not ( - isinstance(value, collections.abc.Mapping) - and ( - all( - isinstance(key, str) - and isinstance(val, collections.abc.Sequence) - and all( - isinstance(subval, collections.abc.Sequence) - for subval in val - ) - and all( - isinstance(item, str) - for item in itertools.chain.from_iterable(val) - ) - for key, val in value.items() + + if not ( + isinstance(value, collections.abc.Mapping) + and ( + all( + isinstance(key, str) + and isinstance(val, collections.abc.Sequence) + and all( + isinstance(subval, collections.abc.Sequence) for subval in val ) + and all( + isinstance(item, str) + for item in itertools.chain.from_iterable(val) + ) + for key, val in value.items() ) - ): - raise TypeError( - "exe_arg_parameters argument was not of type mapping of str and sequences of sequences of strings" - ) + ) + ): + raise TypeError( + "exe_arg_parameters argument was not of type " + "mapping of str and sequences of sequences of strings" + ) self._exe_arg_parameters = copy.deepcopy(value) @@ -257,9 +256,9 @@ def files(self, value: t.Optional[EntityFiles]) -> None: and/or configured prior to execution :raises TypeError: if files is not of type EntityFiles """ - if value: - if not isinstance(value, EntityFiles): - raise TypeError("files argument was not of type EntityFiles") + + if not isinstance(value, EntityFiles): + raise TypeError("files argument was not of type EntityFiles") self._files = copy.deepcopy(value) @property @@ -278,21 +277,22 @@ def file_parameters(self, value: t.Mapping[str, t.Sequence[str]]) -> None: :raises TypeError: if file_parameters is not a mapping of str and sequence of str """ - if value: - if not ( - isinstance(value, t.Mapping) - and ( - all( - isinstance(key, str) - and isinstance(val, collections.abc.Sequence) - and all(isinstance(subval, str) for subval in val) - for key, val in value.items() - ) - ) - ): - raise TypeError( - "file_parameters argument was not of type mapping of str and sequence of str" + + if not ( + isinstance(value, t.Mapping) + and ( + all( + isinstance(key, str) + and isinstance(val, collections.abc.Sequence) + and all(isinstance(subval, str) for subval in val) + for key, val in value.items() ) + ) + ): + raise TypeError( + "file_parameters argument was not of type mapping of str " + "and sequence of str" + ) self._file_parameters = dict(value) @@ -314,11 +314,12 @@ def permutation_strategy( :raises TypeError: if permutation_strategy is not str or PermutationStrategyType """ - if value: - if not callable(value): - raise TypeError( - "permutation_strategy argument was not of type str or PermutationStrategyType" - ) + + if not callable(value): + raise TypeError( + "permutation_strategy argument was not of " + "type str or PermutationStrategyType" + ) self._permutation_strategy = value @property @@ -336,9 +337,8 @@ def max_permutations(self, value: int) -> None: :param value: the max permutations :raises TypeError: max_permutations argument was not of type int """ - if value: - if not isinstance(value, int): - raise TypeError("max_permutations argument was not of type int") + if not isinstance(value, int): + raise TypeError("max_permutations argument was not of type int") self._max_permutations = value @@ -357,9 +357,8 @@ def replicas(self, value: int) -> None: :return: the number of replicas :raises TypeError: replicas argument was not of type int """ - if value: - if not isinstance(value, int): - raise TypeError("replicas argument was not of type int") + if not isinstance(value, int): + raise TypeError("replicas argument was not of type int") self._replicas = value diff --git a/smartsim/entity/application.py b/smartsim/entity/application.py index 759ada93ff..ae2c8186b7 100644 --- a/smartsim/entity/application.py +++ b/smartsim/entity/application.py @@ -115,11 +115,13 @@ def exe(self, value: str) -> None: :raises TypeError: exe argument is not int """ - if value: - if not isinstance(value, str): - raise TypeError("exe argument was not of type str") + if not isinstance(value, str): + raise TypeError("exe argument was not of type str") - self._exe = copy.deepcopy(value) + if value == "": + raise ValueError("exe cannot be an empty str") + + self._exe = value @property def exe_args(self) -> t.MutableSequence[str]: @@ -184,19 +186,18 @@ def file_parameters(self, value: t.Mapping[str, str]) -> None: :param value: the file parameters :raises TypeError: file_parameters argument is not a mapping of str and str """ - if value: - if not ( - isinstance(value, t.Mapping) - and ( - all( - isinstance(key, str) and isinstance(val, str) - for key, val in value.items() - ) - ) - ): - raise TypeError( - "file_parameters argument was not of type mapping of str and str" + if not ( + isinstance(value, t.Mapping) + and ( + all( + isinstance(key, str) and isinstance(val, str) + for key, val in value.items() ) + ) + ): + raise TypeError( + "file_parameters argument was not of type mapping of str and str" + ) self._file_parameters = copy.deepcopy(value) @property @@ -214,13 +215,12 @@ def incoming_entities(self, value: t.List[SmartSimEntity]) -> None: :param value: incoming entities :raises TypeError: incoming_entities argument is not a list of SmartSimEntity """ - if value: - if not isinstance(value, list) or not all( - isinstance(x, SmartSimEntity) for x in value - ): - raise TypeError( - "incoming_entities argument was not of type list of SmartSimEntity" - ) + if not isinstance(value, list) or not all( + isinstance(x, SmartSimEntity) for x in value + ): + raise TypeError( + "incoming_entities argument was not of type list of SmartSimEntity" + ) self._incoming_entities = copy.copy(value) @@ -239,9 +239,8 @@ def key_prefixing_enabled(self, value: bool) -> None: :param value: key prefixing enabled :raises TypeError: key prefixings enabled argument was not of type bool """ - if value: - if not isinstance(value, bool): - raise TypeError("key_prefixing_enabled argument was not of type bool") + if not isinstance(value, bool): + raise TypeError("key_prefixing_enabled argument was not of type bool") self.key_prefixing_enabled = copy.deepcopy(value) @@ -337,7 +336,8 @@ def print_attached_files(self) -> None: def __str__(self) -> str: # pragma: no cover exe_args_str = "\n".join(self.exe_args) entities_str = "\n".join(str(entity) for entity in self.incoming_entities) - return textwrap.dedent(f"""\ + return textwrap.dedent( + f"""\ Name: {self.name} Type: {self.type} Executable: @@ -349,4 +349,5 @@ def __str__(self) -> str: # pragma: no cover Incoming Entities: {entities_str} Key Prefixing Enabled: {self.key_prefixing_enabled} - """) + """ + ) diff --git a/smartsim/experiment.py b/smartsim/experiment.py index c240d9bd8f..6079cd98f2 100644 --- a/smartsim/experiment.py +++ b/smartsim/experiment.py @@ -166,18 +166,18 @@ def start(self, *jobs: Job) -> tuple[LaunchedJobID, ...]: jobs that can be used to query or alter the status of that particular execution of the job. """ - if jobs: - if not all(isinstance(job, Job) for job in jobs): - raise TypeError("jobs argument was not of type Job") - - # Create the run id - run_id = datetime.datetime.now().replace(microsecond=0).isoformat() - # Generate the root path - root = pathlib.Path(self.exp_path, run_id) - return self._dispatch(Generator(root), dispatch.DEFAULT_DISPATCHER, *jobs) - else: + if not jobs: raise ValueError("No jobs provided to start") + if not all(isinstance(job, Job) for job in jobs): + raise TypeError("jobs argument was not of type Job") + + # Create the run id + run_id = datetime.datetime.now().replace(microsecond=0).isoformat() + # Generate the root path + root = pathlib.Path(self.exp_path, run_id) + return self._dispatch(Generator(root), dispatch.DEFAULT_DISPATCHER, *jobs) + def _dispatch( self, generator: Generator, @@ -255,11 +255,10 @@ def get_status( :returns: A tuple of statuses with order respective of the order of the calling arguments. """ - if ids: - if not all(isinstance(id, str) for id in ids): - raise TypeError("ids argument was not of type LaunchedJobID") - else: + if not ids: raise ValueError("No job ids provided to get status") + if not all(isinstance(id, str) for id in ids): + raise TypeError("ids argument was not of type LaunchedJobID") to_query = self._launch_history.group_by_launcher( set(ids), unknown_ok=True @@ -269,7 +268,6 @@ def get_status( stats = (stats_map.get(i, InvalidJobStatus.NEVER_STARTED) for i in ids) return tuple(stats) - # TODO: LaunchedJobID isinstance check def wait( self, *ids: LaunchedJobID, timeout: float | None = None, verbose: bool = True ) -> None: diff --git a/tests/test_application.py b/tests/test_application.py index 7700f05299..d329321504 100644 --- a/tests/test_application.py +++ b/tests/test_application.py @@ -95,11 +95,11 @@ def test_empty_executable(): Application(name="application", exe=None, exe_args=None) -def test_executable_is_not_none(): - """Test that an error is raised when the exe property is empty""" - exe = "" +def test_executable_is_not_empty_str(): + """Test that an error is raised when the exe property is and empty str""" + app = Application(name="application", exe="echo", exe_args=None) with pytest.raises(ValueError): - Application(name="application", exe=exe, exe_args=None) + app.exe = "" def test_type_exe(): diff --git a/tests/test_ensemble.py b/tests/test_ensemble.py index f88338b920..195bd57cbe 100644 --- a/tests/test_ensemble.py +++ b/tests/test_ensemble.py @@ -107,20 +107,15 @@ def test_ensemble_init_empty_params(test_dir: str) -> None: Ensemble() -def test_ensemble_build_jobs(): - """Test a call to build_jobs with empty launchsettings""" - ensemble = Ensemble("ensemble-name", "echo", replicas=2) - launch_settings = None - with pytest.raises(ValueError): - ensemble.build_jobs(launch_settings) - - -def test_ensemble_no_launch_settings(): +@pytest.mark.parametrize( + "bad_settings", + [pytest.param(None, id="Nullish"), pytest.param("invalid", id="String")], +) +def test_ensemble_incorrect_launch_settings_type(bad_settings): """test starting an ensemble with invalid launch settings""" ensemble = Ensemble("ensemble-name", "echo", replicas=2) - launch_settings = "invalid" with pytest.raises(TypeError): - ensemble.build_jobs(launch_settings) + ensemble.build_jobs(bad_settings) def test_ensemble_type_exe(): @@ -135,7 +130,15 @@ def test_ensemble_type_exe(): ensemble.exe = 2 -def test_ensemble_type_exe_args(): +@pytest.mark.parametrize( + "bad_settings", + [ + pytest.param([1, 2, 3], id="sequence of ints"), + pytest.param(0, id="null"), + pytest.param({"foo": "bar"}, id="dict"), + ], +) +def test_ensemble_type_exe_args(bad_settings): ensemble = Ensemble( "ensemble-name", exe="echo", @@ -143,7 +146,7 @@ def test_ensemble_type_exe_args(): with pytest.raises( TypeError, match="exe_args argument was not of type sequence of str" ): - ensemble.exe_args = [1, 2, 3] + ensemble.exe_args = bad_settings @pytest.mark.parametrize( @@ -172,7 +175,8 @@ def test_ensemble_type_exe_arg_parameters(exe_arg_params): ) with pytest.raises( TypeError, - match="exe_arg_parameters argument was not of type mapping of str and sequences of sequences of strings", + match="exe_arg_parameters argument was not of type mapping " + "of str and sequences of sequences of strings", ): ensemble.exe_arg_parameters = exe_arg_params @@ -202,7 +206,8 @@ def test_ensemble_type_file_parameters(file_params): ) with pytest.raises( TypeError, - match="file_parameters argument was not of type mapping of str and sequence of str", + match="file_parameters argument was not of type " + "mapping of str and sequence of str", ): ensemble.file_parameters = file_params @@ -215,7 +220,8 @@ def test_ensemble_type_permutation_strategy(): ) with pytest.raises( TypeError, - match="permutation_strategy argument was not of type str or PermutationStrategyType", + match="permutation_strategy argument was not of " + "type str or PermutationStrategyType", ): ensemble.permutation_strategy = 2 From e3130bb5ab0ca31deebbc40bccc8c88a757322eb Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Wed, 2 Oct 2024 12:58:17 -0700 Subject: [PATCH 08/14] format fix --- smartsim/entity/application.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/smartsim/entity/application.py b/smartsim/entity/application.py index ae2c8186b7..1400797baf 100644 --- a/smartsim/entity/application.py +++ b/smartsim/entity/application.py @@ -336,8 +336,7 @@ def print_attached_files(self) -> None: def __str__(self) -> str: # pragma: no cover exe_args_str = "\n".join(self.exe_args) entities_str = "\n".join(str(entity) for entity in self.incoming_entities) - return textwrap.dedent( - f"""\ + return textwrap.dedent(f"""\ Name: {self.name} Type: {self.type} Executable: @@ -349,5 +348,4 @@ def __str__(self) -> str: # pragma: no cover Incoming Entities: {entities_str} Key Prefixing Enabled: {self.key_prefixing_enabled} - """ - ) + """) From aa03d6fc97b4d3fd0e5074e9c22106c7763bf1ae Mon Sep 17 00:00:00 2001 From: Julia Putko <81587103+juliaputko@users.noreply.github.com> Date: Thu, 3 Oct 2024 14:51:43 -0700 Subject: [PATCH 09/14] Update smartsim/entity/application.py Co-authored-by: Matt Drozt --- smartsim/entity/application.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/smartsim/entity/application.py b/smartsim/entity/application.py index 1400797baf..402f0aa30a 100644 --- a/smartsim/entity/application.py +++ b/smartsim/entity/application.py @@ -188,11 +188,9 @@ def file_parameters(self, value: t.Mapping[str, str]) -> 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) and isinstance(val, str) + for key, val in value.items() ) ): raise TypeError( From 1000d118278a266cfb8b1be99b18aaf069d4be1b Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Thu, 3 Oct 2024 14:53:20 -0700 Subject: [PATCH 10/14] test and type change --- smartsim/builders/ensemble.py | 6 ++++-- tests/test_ensemble.py | 10 ++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/smartsim/builders/ensemble.py b/smartsim/builders/ensemble.py index 2623203e62..3208619f0f 100644 --- a/smartsim/builders/ensemble.py +++ b/smartsim/builders/ensemble.py @@ -240,7 +240,7 @@ def exe_arg_parameters( self._exe_arg_parameters = copy.deepcopy(value) @property - def files(self) -> t.Union[EntityFiles, None]: + def files(self) -> EntityFiles: """Return attached EntityFiles object. :return: the EntityFiles object of files to be copied, symlinked, @@ -249,7 +249,7 @@ def files(self) -> t.Union[EntityFiles, None]: return self._files @files.setter - def files(self, value: t.Optional[EntityFiles]) -> None: + def files(self, value: EntityFiles) -> None: """Set the EntityFiles object. :param value: the EntityFiles object of files to be copied, symlinked, @@ -359,6 +359,8 @@ def replicas(self, value: int) -> None: """ if not isinstance(value, int): raise TypeError("replicas argument was not of type int") + if value <= 0: + raise ValueError self._replicas = value diff --git a/tests/test_ensemble.py b/tests/test_ensemble.py index 195bd57cbe..3b763da9ad 100644 --- a/tests/test_ensemble.py +++ b/tests/test_ensemble.py @@ -251,6 +251,16 @@ def test_ensemble_type_replicas(): ): ensemble.replicas = "invalid" +def test_ensemble_type_replicas_negative(): + ensemble = Ensemble( + "ensemble-name", + exe="echo", + exe_args=["spam", "eggs"], + ) + with pytest.raises( + ValueError + ): + ensemble.replicas = -20 def test_ensemble_type_build_jobs(): ensemble = Ensemble("ensemble-name", "echo", replicas=2) From e4bc93d52043f9404aa429e79dd244f58d81d074 Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Thu, 3 Oct 2024 15:59:01 -0700 Subject: [PATCH 11/14] type check change --- smartsim/builders/ensemble.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/smartsim/builders/ensemble.py b/smartsim/builders/ensemble.py index 3208619f0f..bb2974b550 100644 --- a/smartsim/builders/ensemble.py +++ b/smartsim/builders/ensemble.py @@ -315,7 +315,7 @@ def permutation_strategy( PermutationStrategyType """ - if not callable(value): + if not (callable(value) or isinstance(value, str)): raise TypeError( "permutation_strategy argument was not of " "type str or PermutationStrategyType" From 9b8344243ba1941ffbea47e36b29a3726eec0070 Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Fri, 4 Oct 2024 12:54:13 -0700 Subject: [PATCH 12/14] mypy fix --- smartsim/builders/ensemble.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/smartsim/builders/ensemble.py b/smartsim/builders/ensemble.py index bb2974b550..84e307fae4 100644 --- a/smartsim/builders/ensemble.py +++ b/smartsim/builders/ensemble.py @@ -139,7 +139,7 @@ def __init__( copy.deepcopy(exe_arg_parameters) if exe_arg_parameters else {} ) """The parameters and values to be used when configuring entities""" - self._files = copy.deepcopy(files) if files else None + self._files = copy.deepcopy(files) if files else EntityFiles() """The files to be copied, symlinked, and/or configured prior to execution""" self._file_parameters = ( copy.deepcopy(file_parameters) if file_parameters else {} From df5d9f95a898394b2e53f50116d809e7d26ac4c5 Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Fri, 4 Oct 2024 14:15:22 -0700 Subject: [PATCH 13/14] format fix --- tests/test_ensemble.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_ensemble.py b/tests/test_ensemble.py index 3b763da9ad..00a3f66cfe 100644 --- a/tests/test_ensemble.py +++ b/tests/test_ensemble.py @@ -251,17 +251,17 @@ def test_ensemble_type_replicas(): ): ensemble.replicas = "invalid" + def test_ensemble_type_replicas_negative(): ensemble = Ensemble( "ensemble-name", exe="echo", exe_args=["spam", "eggs"], ) - with pytest.raises( - ValueError - ): + with pytest.raises(ValueError): ensemble.replicas = -20 + def test_ensemble_type_build_jobs(): ensemble = Ensemble("ensemble-name", "echo", replicas=2) with pytest.raises(TypeError): From bfc4e7711ccbb89abe3a4a63fbb441d6b0d7b834 Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Fri, 4 Oct 2024 14:21:30 -0700 Subject: [PATCH 14/14] style and description additions --- smartsim/builders/ensemble.py | 2 +- tests/test_ensemble.py | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/smartsim/builders/ensemble.py b/smartsim/builders/ensemble.py index 84e307fae4..d8a16880be 100644 --- a/smartsim/builders/ensemble.py +++ b/smartsim/builders/ensemble.py @@ -360,7 +360,7 @@ def replicas(self, value: int) -> None: if not isinstance(value, int): raise TypeError("replicas argument was not of type int") if value <= 0: - raise ValueError + raise ValueError("Number of replicas must be a positive integer") self._replicas = value diff --git a/tests/test_ensemble.py b/tests/test_ensemble.py index 00a3f66cfe..1bfbd0b67a 100644 --- a/tests/test_ensemble.py +++ b/tests/test_ensemble.py @@ -258,7 +258,10 @@ def test_ensemble_type_replicas_negative(): exe="echo", exe_args=["spam", "eggs"], ) - with pytest.raises(ValueError): + with pytest.raises( + ValueError, + match="Number of replicas must be a positive integer", + ): ensemble.replicas = -20