-
Notifications
You must be signed in to change notification settings - Fork 37
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
Runtime value checking for LaunchSettings and BatchSettings #740
base: smartsim-refactor
Are you sure you want to change the base?
Runtime value checking for LaunchSettings and BatchSettings #740
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## smartsim-refactor #740 +/- ##
=====================================================
- Coverage 40.45% 37.90% -2.56%
=====================================================
Files 110 109 -1
Lines 7326 6583 -743
=====================================================
- Hits 2964 2495 -469
+ Misses 4362 4088 -274
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great so far! Just some initial feedback for you to take a look at while we expand this out into {Batch,Launch}Arguments
classes as well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work as always! One larger suggestion is to move the setter testing to the associated launcher and scheduler test files!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of small nits, but otherwise looks about ready to go!!
:raises TypeError: if not an int | ||
""" | ||
if not isinstance(num_nodes, int): | ||
raise TypeError("num_nodes argument was not of type int") | ||
|
||
self.set("nodes", str(num_nodes)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More philosophy than science question here: Should we check for inapropriate values here as well? Something like:
if num_nodes <= 0:
raise ValueError("Number of nodes must be a positive value")
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A similar question exists for other places we expect integer inputs.
smartsim/settings/batch_settings.py
Outdated
if batch_args is not None: | ||
if not ( | ||
isinstance(batch_args, dict) | ||
and all(isinstance(key, str) for key, val in batch_args.items()) | ||
): | ||
raise TypeError( | ||
"batch_args argument was not of type mapping of str and str" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: Typehint and runtime check state that this must be a dict[str, str | None]
, but the error messages says it can be a Mapping[str, str]
if batch_args is not None: | |
if not ( | |
isinstance(batch_args, dict) | |
and all(isinstance(key, str) for key, val in batch_args.items()) | |
): | |
raise TypeError( | |
"batch_args argument was not of type mapping of str and str" | |
) | |
if batch_args is not None: | |
if not ( | |
isinstance(batch_args, dict) | |
and all(isinstance(key, str) for key, val in batch_args.items()) | |
): | |
raise TypeError( | |
"batch_args argument was not of type dict of str and str or None" | |
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, since we are already iterating over batch_args
, should we runtime type check the values as well as the keys of batch_args
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the type hint for it is t.Dict[str, t.Optional[str]], so what should the value type check look like? how do I check for optional str?
smartsim/settings/batch_settings.py
Outdated
@env_vars.setter | ||
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()) | ||
): | ||
raise TypeError("env_vars argument was not of type dic of str and str") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar inconsistency here between dict
and Mapping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a similar question about whether we should runtime check the types of the values of value
smartsim/settings/launch_settings.py
Outdated
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()) | ||
): | ||
raise TypeError( | ||
"batch_args argument was not of type mapping of str and str" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar 'dict
vs Mapping
inconsistency' and 'should we runtime type check the values of launch_args
' comment here as with BatchSettings
smartsim/settings/launch_settings.py
Outdated
@@ -165,6 +174,12 @@ def env_vars(self, value: dict[str, str | None]) -> None: | |||
|
|||
:param value: The new environment mapping | |||
""" | |||
if not ( | |||
isinstance(value, dict) | |||
and all(isinstance(key, str) for key, val in value.items()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar 'should we runtime type check the values of value
' comment here as with BatchSettings.env_vars
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" |
) | ||
def test_type_launch_args(launch_args): | ||
with pytest.raises( | ||
TypeError, match="batch_args argument was not of type mapping of str and str" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeError, match="batch_args argument was not of type mapping of str and str" | |
TypeError, match="launch_args argument was not of type mapping of str and str" |
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" |
No description provided.