Refactor cluster factory #1591
Refactor cluster factory #1591
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
carolineechen
force-pushed
the
cluster-refactor
branch
6 times, most recently
from
53330c2 to
c70bd63
Compare
| @@ -445,6 +445,10 @@ def config(self, condensed: bool = True): | |||
|
|
|||
| return config | |||
|
|
|||
| def _update_values(self, new_values: Dict[str, str]): | |||
There was a problem hiding this comment.
are these values always guaranteed to be a string? thinking about an image arg for example which we might need to serialize or convert to RNS address?
There was a problem hiding this comment.
will update the type to Dict[str, Any], I think serialization should happen through config()/from_config() but will double check the comparison of name to config
carolineechen
force-pushed
the
cluster-refactor
branch
3 times, most recently
from
c5429ab to
9a6c165
Compare
runhouse/resources/hardware/utils.py
Outdated
| subprocess.run(cmd, shell=True, check=True) | ||
| logger.info(f"Kubernetes context has been set to: {context}") | ||
| except subprocess.CalledProcessError as e: | ||
| logger.info(f"Error setting context: {e}") |
There was a problem hiding this comment.
wonder if we should actually raise an exception if we fail to set the context or namespace? we don't do it in den either, but this could lead to undesired behavior the user wouldn't want (ex: launching in a different namespace)
| ssl_certfile=ssl_certfile, | ||
| domain=domain, | ||
| den_auth=den_auth, | ||
| cluster_args = locals().copy() |
There was a problem hiding this comment.
small nit and maybe just me, but was initially confused by the naming here, since cluster_args really behaves like kwargs
There was a problem hiding this comment.
yea using it as an easy way to get a dict out of the input args to easily pass around and use to check against cluster config, instead of manually creating a dict by passing in each arg as before (easy to miss an arg / forget to update when we add new factory args), but open to other approaches or naming, did you have one in mind?
There was a problem hiding this comment.
ya i like the way its extracted, its cleaner that way.. i think we can leave as is since it's not really "user facing" anyway
carolineechen
force-pushed
the
cluster-refactor
branch
4 times, most recently
from
c4dabdd to
52339b5
Compare
carolineechen
force-pushed
the
cluster-refactor
branch
from
52339b5 to
e846656
Compare
No description provided.