From 7e77620b4830d2a50825d69f51171dca98de48f0 Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Mon, 26 Aug 2024 17:32:16 -0500 Subject: [PATCH 1/6] added type checking to modelparams --- smartsim/entity/model.py | 20 +++++++++++++++++++- tests/test_model.py | 15 +++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/smartsim/entity/model.py b/smartsim/entity/model.py index 3f78e042c..fb089db89 100644 --- a/smartsim/entity/model.py +++ b/smartsim/entity/model.py @@ -46,6 +46,24 @@ logger = get_logger(__name__) +def _read_model_parameters(params_dict: t.Dict[str, t.Any]) -> t.Dict[str, str]: + """Convert the values in a params dict to strings + :raises TypeError: if params are of the wrong type + :return: param dictionary with values and keys cast as strings + """ + param_names: t.List[str] = [] + parameters: t.List[t.List[str]] = [] + for name, val in params_dict.items(): + param_names.append(name) + if isinstance(val, (int, str)): + parameters.append(str(val)) + else: + raise TypeError( + "Incorrect type for ensemble parameters\n" + "Must be int, or string." + ) + return dict(zip(param_names, parameters)) + + class Model(SmartSimEntity): def __init__( self, @@ -70,7 +88,7 @@ def __init__( model as a batch job """ super().__init__(name, str(path), run_settings) - self.params = params + self.params = _read_model_parameters(params) self.params_as_args = params_as_args self.incoming_entities: t.List[SmartSimEntity] = [] self._key_prefixing_enabled = False diff --git a/tests/test_model.py b/tests/test_model.py index 64a68b299..7f85a9f6b 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -32,6 +32,7 @@ from smartsim._core.control.manifest import LaunchedManifestBuilder from smartsim._core.launcher.step import SbatchStep, SrunStep from smartsim.entity import Ensemble, Model +from smartsim.entity.model import _read_model_parameters from smartsim.error import EntityExistsError, SSUnsupportedError from smartsim.settings import RunSettings, SbatchSettings, SrunSettings from smartsim.settings.mpiSettings import _BaseMPISettings @@ -176,3 +177,17 @@ def test_models_batch_settings_are_ignored_in_ensemble( step_cmd = step.step_cmds[0] assert any("srun" in tok for tok in step_cmd) # call the model using run settings assert not any("sbatch" in tok for tok in step_cmd) # no sbatch in sbatch + + +def test_bad_model_params(): + params_1 = {"j": 5} + params_2 = {"P": ["eggs"]} + params_3 = {"m": {"n": 5}} + + params_1 = _read_model_parameters(params_1) + + with pytest.raises(TypeError): + params_2 = _read_model_parameters(params_2) + + with pytest.raises(TypeError): + params_3 = _read_model_parameters(params_3) From e574fb07cc06605b975c8edab56d1465baeeaa2d Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Wed, 28 Aug 2024 18:32:47 -0500 Subject: [PATCH 2/6] fix mypy --- smartsim/entity/model.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/smartsim/entity/model.py b/smartsim/entity/model.py index fb089db89..cc9bb57c5 100644 --- a/smartsim/entity/model.py +++ b/smartsim/entity/model.py @@ -52,7 +52,7 @@ def _read_model_parameters(params_dict: t.Dict[str, t.Any]) -> t.Dict[str, str]: :return: param dictionary with values and keys cast as strings """ param_names: t.List[str] = [] - parameters: t.List[t.List[str]] = [] + parameters: t.List[str] = [] for name, val in params_dict.items(): param_names.append(name) if isinstance(val, (int, str)): From a11b97b8f7f7a32561a35656f82932b30a8edb99 Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Thu, 29 Aug 2024 15:27:43 -0500 Subject: [PATCH 3/6] fixed test, and func --- smartsim/entity/model.py | 10 ++++++---- tests/test_model.py | 20 ++++++++++---------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/smartsim/entity/model.py b/smartsim/entity/model.py index cc9bb57c5..a11a594fc 100644 --- a/smartsim/entity/model.py +++ b/smartsim/entity/model.py @@ -27,6 +27,7 @@ from __future__ import annotations import itertools +import numbers import re import sys import typing as t @@ -46,7 +47,7 @@ logger = get_logger(__name__) -def _read_model_parameters(params_dict: t.Dict[str, t.Any]) -> t.Dict[str, str]: +def _parse_model_parameters(params_dict: t.Dict[str, t.Any]) -> t.Dict[str, str]: """Convert the values in a params dict to strings :raises TypeError: if params are of the wrong type :return: param dictionary with values and keys cast as strings @@ -55,11 +56,12 @@ def _read_model_parameters(params_dict: t.Dict[str, t.Any]) -> t.Dict[str, str]: parameters: t.List[str] = [] for name, val in params_dict.items(): param_names.append(name) - if isinstance(val, (int, str)): + if isinstance(val, (str, numbers.Number)): parameters.append(str(val)) else: raise TypeError( - "Incorrect type for ensemble parameters\n" + "Must be int, or string." + "Incorrect type for model parameters\n" + + "Must be numeric value or string." ) return dict(zip(param_names, parameters)) @@ -88,7 +90,7 @@ def __init__( model as a batch job """ super().__init__(name, str(path), run_settings) - self.params = _read_model_parameters(params) + self.params = _parse_model_parameters(params) self.params_as_args = params_as_args self.incoming_entities: t.List[SmartSimEntity] = [] self._key_prefixing_enabled = False diff --git a/tests/test_model.py b/tests/test_model.py index 7f85a9f6b..152ce2058 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -26,13 +26,14 @@ from uuid import uuid4 +import numpy as np import pytest from smartsim import Experiment from smartsim._core.control.manifest import LaunchedManifestBuilder from smartsim._core.launcher.step import SbatchStep, SrunStep from smartsim.entity import Ensemble, Model -from smartsim.entity.model import _read_model_parameters +from smartsim.entity.model import _parse_model_parameters from smartsim.error import EntityExistsError, SSUnsupportedError from smartsim.settings import RunSettings, SbatchSettings, SrunSettings from smartsim.settings.mpiSettings import _BaseMPISettings @@ -179,15 +180,14 @@ def test_models_batch_settings_are_ignored_in_ensemble( assert not any("sbatch" in tok for tok in step_cmd) # no sbatch in sbatch -def test_bad_model_params(): - params_1 = {"j": 5} - params_2 = {"P": ["eggs"]} - params_3 = {"m": {"n": 5}} +@pytest.mark.parametrize("dtype", [int, np.float32, str]) +def test_good_model_params(dtype): + print(dtype(0.6)) + params = {"foo": dtype(0.6)} + assert all(isinstance(val, str) for val in _parse_model_parameters(params).values()) - params_1 = _read_model_parameters(params_1) +@pytest.mark.parametrize("bad_val", [["eggs"], {"n": 5}, object]) +def test_bad_model_params(bad_val): with pytest.raises(TypeError): - params_2 = _read_model_parameters(params_2) - - with pytest.raises(TypeError): - params_3 = _read_model_parameters(params_3) + _parse_model_parameters({"foo": bad_val}) From 31726a220ca66448b9c6d2c364aac6b92e2f8037 Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Tue, 3 Sep 2024 17:02:55 -0500 Subject: [PATCH 4/6] added to changelog --- doc/changelog.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/changelog.md b/doc/changelog.md index 23bbed5c6..1c48917cd 100644 --- a/doc/changelog.md +++ b/doc/changelog.md @@ -15,6 +15,7 @@ To be released at some future point in time Description +- Add type check to model params - Pin watchdog to 4.x - Update codecov to 4.5.0 - Remove build of Redis from setup.py @@ -31,6 +32,8 @@ Description Detailed Notes +- Added type checking to params on model + ([SmartSim-PR676](https://github.com/CrayLabs/SmartSim/pull/676)) - Pin watchdog to 4.x because v5 introduces new types and requires updates to the type-checking ([SmartSim-PR690](https://github.com/CrayLabs/SmartSim/pull/690)) From 3b38ece38c3f8a23876621e81bf8e4ba4e2ed956 Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Wed, 4 Sep 2024 15:27:37 -0500 Subject: [PATCH 5/6] change type in preview test for params --- tests/test_preview.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_preview.py b/tests/test_preview.py index 3c7bed6fe..a18d10728 100644 --- a/tests/test_preview.py +++ b/tests/test_preview.py @@ -357,7 +357,7 @@ def test_model_preview_properties(test_dir, wlmutils): assert hw_rs == hello_world_model.run_settings.exe_args[0] assert None == hello_world_model.batch_settings assert "port" in list(hello_world_model.params.items())[0] - assert hw_port in list(hello_world_model.params.items())[0] + assert str(hw_port) in list(hello_world_model.params.items())[0] assert "password" in list(hello_world_model.params.items())[1] assert hw_password in list(hello_world_model.params.items())[1] From c4a9991ef74e4a2a991f86e9e5267c12ec20f1ac Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Thu, 5 Sep 2024 15:26:30 -0500 Subject: [PATCH 6/6] update changelog --- doc/changelog.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/doc/changelog.md b/doc/changelog.md index 1c48917cd..26388a05e 100644 --- a/doc/changelog.md +++ b/doc/changelog.md @@ -15,7 +15,8 @@ To be released at some future point in time Description -- Add type check to model params +- Allow specifying Model and Ensemble parameters with + number-like types (e.g. numpy types) - Pin watchdog to 4.x - Update codecov to 4.5.0 - Remove build of Redis from setup.py @@ -32,7 +33,10 @@ Description Detailed Notes -- Added type checking to params on model +- The serializer would fail if a parameter for a Model or Ensemble + was specified as a numpy dtype. The constructors for these + methods now validate that the input is number-like and convert + them to strings ([SmartSim-PR676](https://github.com/CrayLabs/SmartSim/pull/676)) - Pin watchdog to 4.x because v5 introduces new types and requires updates to the type-checking