Include launcher type in cluster factory check #1436
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: runhouse/resources/hardware/cluster_factory.py
Did you find this useful? React with a 👍 or 👎 |
| @@ -108,6 +108,7 @@ def cluster( | |||
| domain=domain, | |||
| den_auth=den_auth, | |||
| default_env=default_env, | |||
| launcher_type=launcher_type, | |||
There was a problem hiding this comment.
curious what the motivation is for adding it here? if the user gives a new launcher_type value but with a name that is already saved in Den, this would prevent us from reloading an already live cluster and lead to some weird behavior where we try to relaunch it
There was a problem hiding this comment.
that's true. at the same time though, if the user gives a new launcher_type for a saved cluster, the cluster is reloaded and will return with the old launcher_type that was saved. in that case we'd have to add something like c.launcher_type = launcher_type or c.launcher_type after loading from name. are there any other properties that fall under this category?
also I'd like to rewrite cluster factories to be a bit more intuitive / less convoluted at some point, we've run into small issues like this before, but will probably tackle this when there's less higher priority things going on
There was a problem hiding this comment.
hmm yea i think this can get a little tricky. one thought - what if we try to reload from name initially, ignoring any other params provided. if the cluster is already saved and conflicting param values were provided, we throw an error instructing the user to teardown and delete that cluster if they want to relaunch with that same name?
|
#1575 1575 |
No description provided.