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.

Sign up for GitHub

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

Include launcher type in cluster factory check #1436

Closed
wants to merge 1 commit into from
Closed

Include launcher type in cluster factory check #1436

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions runhouse/resources/hardware/cluster_factory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ def cluster(
domain=domain,
den_auth=den_auth,
default_env=default_env,
launcher_type=launcher_type,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

kwargs=kwargs if len(kwargs) > 0 else None,
)
# Filter out None/default values
Expand Down Expand Up @@ -448,6 +449,7 @@ def ondemand_cluster(
domain=domain,
den_auth=den_auth,
default_env=default_env,
launcher_type=launcher_type,
)
# Filter out None/default values
alt_options = {k: v for k, v in alt_options.items() if v is not None}
Expand Down
Loading