-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
[autoscaler] SDK fixes #11517
[autoscaler] SDK fixes #11517
Conversation
def teardown_cluster(cluster_config: Union[dict, str]) -> None: | ||
def teardown_cluster(cluster_config: Union[dict, str], | ||
workers_only: bool = False, | ||
keep_min_workers: bool = False) -> None: | ||
"""Destroys all nodes of a Ray cluster described by a config json. | ||
|
||
Args: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't forget docstrings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! Will do!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, just needs documentation
let me know if you need help merging after tests pass |
Sounds good, I think *the need for this* is actually going away (for the
exact reasons mentioned).
…On Thu, Oct 22, 2020 at 12:16 PM Eric Liang ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In python/ray/autoscaler/sdk.py
<#11517 (comment)>:
> @@ -102,7 +108,8 @@ def rsync(cluster_config: Union[dict, str],
down: bool,
ip_address: str = None,
use_internal_ip: bool = False,
- no_config_cache: bool = False):
+ no_config_cache: bool = False,
+ all_nodes: bool = False):
It's exposed there, but only as a best effort kind of thing for clusters
that are up. I want to discourage users from using that option however
(maybe we should even remove it).
It's not a reliable building block given that clusters are inherently
autoscaling.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#11517 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFC5KQS2QOGALZE5ZKV6YNLSMCAH7ANCNFSM4SZJOLRQ>
.
|
Why are these changes needed?
Expose more of the API in the autoscaler SDK.
Related issue number
Checks
scripts/format.sh
to lint the changes in this PR.