BatchSettings, LaunchSettings, Command, CommandList and LaunchCommand Refactor #587
Conversation
amandarichardsonn
added
type: feature
amandarichardsonn
requested review from
MattToast
and removed request for
mellis13
amandarichardsonn
changed the title
amandarichardsonn
changed the title
MattToast
left a comment
MattToast
left a comment
There was a problem hiding this comment.
Looks great! A couple of minor nits here and there, and looks like you might have a couple of nasty conflicts, but as soon as that is sorted out, this looks about ready to go on my end!!
| } | ||
| localLauncher = LaunchSettings(launcher=LauncherType.Local, env_vars=env_vars) | ||
| assert isinstance(localLauncher._arg_builder, LocalArgBuilder) | ||
| assert localLauncher.format_env_vars() == ["A=a", "B=", "C=", "D=12"] No newline at end of file |
There was a problem hiding this comment.
Not wrong, just a confirmation; is an env var dict {"KEY": ""} functionally equivalent to {"KEY": None}? It looks like that is what this test is testing for, but my intuition would be that {"KEY": None} would make sure that the "KEY" environment variable was unset from the executing environment. Might be worth getting more by in form the group on this issue, as I think we have been inconsistent in the past.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## smartsim-refactor #587 +/- ##
====================================================
Coverage ? 33.09%
====================================================
Files ? 99
Lines ? 6191
Branches ? 0
====================================================
Hits ? 2049
Misses ? 4142
Partials ? 0
|
…martSim into settings-refactor
MattToast
left a comment
MattToast
left a comment
There was a problem hiding this comment.
Rubber stamping as promised! Looks good to me!! Just add a from __future__ import annotations to smartsim/settings/common.py to get past the CI with Python 3.9
2 participants
No description provided.