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

Refactor: Move "exe_args" from "model" to "run_settings" object in manifest.json #4

Open
MattToast opened this issue Oct 18, 2023 · 0 comments
Labels
area: telemetry Issues related to dashboard telemetry short task Issues that can be completed and reviewed quickly type: refactor Issues focused on refactoring existing code

Comments

@MattToast
Copy link
Member

MattToast commented Oct 18, 2023

Description

Currently in the agreed upon structure for the manifest.json file that is used as a handshake between SS and SSDash, the model.run_settings.exec_args list of strings is serialized under the manifest["runs"][index]["model"][index_2]["exe_args"] instead of under a ...[index_2]["run_settings"]["exe_args"]. We should consider moving this information there.

Justification

In memory, SmartSim has exe_args: list[str] as an attr on RunSettings. Ideally the manifest.json should follow the structure of SS's data structures as that makes serialization easier, and reduces the amount of context switching that needs to be done by developers.

Refactor for SS

At time of writing (likely that it has moved in push for MVP):

--- a/smartsim/_core/utils/serialize.py
+++ b/smartsim/_core/utils/serialize.py
@@ -96,7 +96,6 @@ def _dictify_model(
     return {
         "name": model.name,
         "path": model.path,
-        "exe_args": model.run_settings.exe_args,
         "run_settings": _dictify_run_settings(model.run_settings),
         "batch_settings": _dictify_batch_settings(model.batch_settings)
         if model.batch_settings
@@ -171,8 +170,7 @@ def _dictify_ensemble(
 def _dictify_run_settings(run_settings: RunSettings) -> t.Dict[str, t.Any]:
     return {
         "exe": run_settings.exe,
-        # TODO: We should try to move this back
-        # "exe_args": run_settings.exe_args,
+        "exe_args": run_settings.exe_args,
         "run_command": run_settings.run_command,
         "run_args": run_settings.run_args,
         # TODO: We currently do not have a way to represent MPMD commands!

Refactor for SSDash

At time of writing (VERY likely to change in push for an MVP):

--- a/smartdashboard/utils/helpers.py
+++ b/smartdashboard/utils/helpers.py
@@ -28,10 +28,8 @@ def get_exe_args(entity: Optional[Dict[str, Any]]) -> List[str]:
     :return: exe_args of the entity
     :rtype: List[str]
     """
-    if entity:
-        return list(entity.get("exe_args", []))
-
-    return []
+    rs = dict((entity or {}).get("run_settings", {}))
+    return list(rs.get("exe_args", []))
@MattToast MattToast added short task Issues that can be completed and reviewed quickly type: refactor Issues focused on refactoring existing code area: telemetry Issues related to dashboard telemetry labels Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: telemetry Issues related to dashboard telemetry short task Issues that can be completed and reviewed quickly type: refactor Issues focused on refactoring existing code
Projects
None yet
Development

No branches or pull requests

1 participant