Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct ExecArgs Handling During RunSetting #517

Merged
merged 17 commits into from
Mar 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions doc/changelog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ To be released at some future point in time

Description

- ExecArgs handling correction
- ReadTheDocs config file added and enabled on PRs
- Enforce changelog updates
- Remove deprecated SmartSim modules
Expand All @@ -28,6 +29,8 @@ Description

Detailed Notes

- Add checks and tests to ensure SmartSim users cannot initialize run settings
with a list of lists as the exe_args argument. (SmartSim-PR517_)
- Add readthedocs configuration file and enable readthedocs builds
on pull requests. Additionally added robots.txt file generation
when readthedocs environment detected. (SmartSim-PR512_)
Expand All @@ -46,6 +49,7 @@ Detailed Notes
(SmartSim-PR504_)
- Update the generic `t.Any` typehints in Experiment API. (SmartSim-PR501_)

.. _SmartSim-PR517: https://github.com/CrayLabs/SmartSim/pull/517
.. _SmartSim-PR512: https://github.com/CrayLabs/SmartSim/pull/512
.. _SmartSim-PR518: https://github.com/CrayLabs/SmartSim/pull/518
.. _SmartSim-PR514: https://github.com/CrayLabs/SmartSim/pull/514
Expand Down
49 changes: 21 additions & 28 deletions smartsim/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -446,15 +446,8 @@

:param args: executable arguments
:type args: str | list[str]
:raises TypeError: if exe args are not strings
"""
if isinstance(args, str):
args = args.split()

for arg in args:
if not isinstance(arg, str):
raise TypeError("Executable arguments should be a list of str")

args = self._build_exe_args(args)

Check warning on line 450 in smartsim/settings/base.py

View check run for this annotation

Codecov / codecov/patch

smartsim/settings/base.py#L450

Added line #L450 was not covered by tests
self._exe_args.extend(args)

def set(
Expand Down Expand Up @@ -533,26 +526,26 @@

@staticmethod
def _build_exe_args(exe_args: t.Optional[t.Union[str, t.List[str]]]) -> t.List[str]:
"""Convert exe_args input to a desired collection format"""
if exe_args:
if isinstance(exe_args, str):
return exe_args.split()
if isinstance(exe_args, list):
exe_args = copy.deepcopy(exe_args)
plain_type = all(isinstance(arg, (str)) for arg in exe_args)
if not plain_type:
nested_type = all(
all(isinstance(arg, (str)) for arg in exe_args_list)
for exe_args_list in exe_args
)
if not nested_type:
raise TypeError(
"Executable arguments were not list of str or str"
)
return exe_args
return exe_args
raise TypeError("Executable arguments were not list of str or str")
return []
"""Check and convert exe_args input to a desired collection format"""
if not exe_args:
return []

if isinstance(exe_args, list):
exe_args = copy.deepcopy(exe_args)

if not (
isinstance(exe_args, str)
or (
isinstance(exe_args, list)
and all(isinstance(arg, str) for arg in exe_args)
)
):
raise TypeError("Executable arguments were not a list of str or a str.")

Check warning on line 543 in smartsim/settings/base.py

View check run for this annotation

Codecov / codecov/patch

smartsim/settings/base.py#L543

Added line #L543 was not covered by tests

if isinstance(exe_args, str):
return exe_args.split()

return exe_args

def format_run_args(self) -> t.List[str]:
"""Return formatted run arguments
Expand Down
55 changes: 17 additions & 38 deletions tests/test_run_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -185,55 +185,34 @@ def test_add_exe_args_list_of_mixed():
settings.add_exe_args(["1", "2", 3])


def test_add_exe_args_space_delimited_string():
def test_add_exe_args_list_of_lists():
"""Ensure that any non-string exe arg fails validation for all"""
settings = RunSettings("python")
expected = ["1", "2", "3"]
settings.add_exe_args("1 2 3")

assert settings.exe_args == expected


def test_add_exe_args_list_of_mixed_lists():
"""Ensure that any non-string exe arg fails validation for all"""
settings = RunSettings("python")
with pytest.raises(TypeError) as type_error:
settings.add_exe_args([["1", "2", 3], ["4", "5", 6]])

assert "Executable arguments should be a list of str" in type_error.value.args


def test_add_exe_args_list_of_mixed_lists_init():
"""Ensure that any non-string exe arg fails validation for all"""
exe_args = [["1", "2", 3], ["4", "5", 6]]

with pytest.raises(TypeError) as type_error:
settings = RunSettings("python", exe_args=exe_args)

assert "Executable arguments were not list of str or str" in type_error.value.args
with pytest.raises(TypeError):
settings.add_exe_args(["1", "2", "3"], ["1", "2", "3"])


def test_add_exe_args_list_of_str_lists_init():
"""Ensure that list[list[str]] pass validation"""
def test_init_exe_args_list_of_lists():
"""Ensure that a list of lists exe arg fails validation"""
exe_args = [["1", "2", "3"], ["4", "5", "6"]]
with pytest.raises(TypeError):
_ = RunSettings("python", exe_args=exe_args)

settings = RunSettings("python", exe_args=exe_args)

assert settings.exe_args == exe_args
def test_init_exe_args_list_of_lists_mixed():
"""Ensure that a list of lists exe arg fails validation"""
exe_args = [["1", "2", 3], ["4", "5", 6]]
with pytest.raises(TypeError):
_ = RunSettings("python", exe_args=exe_args)


def test_add_exe_args_list_of_str_lists():
"""Ensure that list[list[str]] fail validation when added via method"""
exe_args = [["1", "2", "3"], ["4", "5", "6"]]

def test_add_exe_args_space_delimited_string():
"""Ensure that any non-string exe arg fails validation for all"""
settings = RunSettings("python")
expected = ["1", "2", "3"]
settings.add_exe_args("1 2 3")

with pytest.raises(TypeError) as type_error:
settings.add_exe_args(exe_args)

# NOTE that this behavior differs from sending constructor args like
# tested in test_add_exe_args_list_of_str_lists_init where it's allowed
assert "Executable arguments should be a list of str" in type_error.value.args
assert settings.exe_args == expected


def test_format_run_args():
Expand Down
Loading