Skip to content

Commit

Permalink
address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
amandarichardsonn committed Sep 16, 2024
1 parent 460a86a commit 00770fe
Show file tree
Hide file tree
Showing 8 changed files with 21 additions and 20 deletions.
4 changes: 4 additions & 0 deletions smartsim/settings/arguments/batch/lsf.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@


class BsubBatchArguments(BatchArguments):
"""A class to represent the arguments required for submitting batch
jobs using the bsub command.
"""

def scheduler_str(self) -> str:
"""Get the string representation of the scheduler
Expand Down
4 changes: 4 additions & 0 deletions smartsim/settings/arguments/batch/pbs.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@


class QsubBatchArguments(BatchArguments):
"""A class to represent the arguments required for submitting batch
jobs using the qsub command.
"""

def scheduler_str(self) -> str:
"""Get the string representation of the scheduler
Expand Down
4 changes: 4 additions & 0 deletions smartsim/settings/arguments/batch/slurm.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@


class SlurmBatchArguments(BatchArguments):
"""A class to represent the arguments required for submitting batch
jobs using the sbatch command.
"""

def scheduler_str(self) -> str:
"""Get the string representation of the scheduler
Expand Down
4 changes: 1 addition & 3 deletions smartsim/settings/baseSettings.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,8 @@
# 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.

# fmt: off

class BaseSettings:
"""
A base class for LaunchSettings and BatchSettings.
"""
...
# fmt: on
3 changes: 2 additions & 1 deletion smartsim/settings/batchSettings.py
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ def scheduler_args(self) -> BatchArguments:
@property
def env_vars(self) -> StringArgument:
"""Return an immutable list of attached environment variables."""
return copy.deepcopy(self._env_vars)
return self._env_vars

@env_vars.setter
def env_vars(self, value: t.Dict[str, str | None]) -> None:
Expand All @@ -145,6 +145,7 @@ def _get_arguments(self, scheduler_args: StringArgument | None) -> BatchArgument
:param scheduler_args: A mapping of arguments names to values to be
used to initialize the arguments
:returns: The appropriate type for the settings instance.
:raises ValueError: An invalid scheduler type was provided.
"""
if self._batch_scheduler == SchedulerType.Slurm:
return SlurmBatchArguments(scheduler_args)
Expand Down
4 changes: 2 additions & 2 deletions smartsim/settings/launchSettings.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ class LaunchSettings(BaseSettings):
- The set environment variables (LaunchSettings.env_vars).
"""

# Manage Environment Setup, set configuration parameters based on your machines launcher
def __init__(
self,
launcher: t.Union[LauncherType, str],
Expand Down Expand Up @@ -152,7 +151,7 @@ def env_vars(self) -> t.Mapping[str, str | None]:
:returns: An environment mapping
"""
return copy.deepcopy(self._env_vars)
return self._env_vars

@env_vars.setter
def env_vars(self, value: dict[str, str | None]) -> None:
Expand All @@ -171,6 +170,7 @@ def _get_arguments(self, launch_args: StringArgument | None) -> LaunchArguments:
:param launch_args: A mapping of arguments names to values to be used
to initialize the arguments
:returns: The appropriate type for the settings instance.
:raises ValueError: An invalid launcher type was provided.
"""
if self._launcher == LauncherType.Slurm:
return SlurmLaunchArguments(launch_args)
Expand Down
9 changes: 2 additions & 7 deletions tests/temp_tests/test_settings/test_batchSettings.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,5 @@ def test_launcher_property():
def test_env_vars_property():
bs = BatchSettings(batch_scheduler="slurm", env_vars={"ENV": "VAR"})
assert bs.env_vars == {"ENV": "VAR"}


def test_env_vars_property_deep_copy():
bs = BatchSettings(batch_scheduler="slurm", env_vars={"ENV": "VAR"})
copy_env_vars = bs.env_vars
copy_env_vars.update({"test": "no_update"})
assert bs.env_vars == {"ENV": "VAR"}
ref = bs.env_vars
assert ref == bs.env_vars
9 changes: 2 additions & 7 deletions tests/temp_tests/test_settings/test_launchSettings.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,8 @@ def test_launcher_property():
def test_env_vars_property():
ls = LaunchSettings(launcher="local", env_vars={"ENV": "VAR"})
assert ls.env_vars == {"ENV": "VAR"}


def test_env_vars_property_deep_copy():
ls = LaunchSettings(launcher="local", env_vars={"ENV": "VAR"})
copy_env_vars = ls.env_vars
copy_env_vars.update({"test": "no_update"})
assert ls.env_vars == {"ENV": "VAR"}
ref = ls.env_vars
assert ref == ls.env_vars


def test_update_env_vars():
Expand Down

0 comments on commit 00770fe

Please sign in to comment.