-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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] make 0 default min/max workers for head node #17757
Conversation
It's customary to add a tag to the title to aid in release tracking -- added "[autoscaler]". |
python/ray/tests/test_autoscaler.py
Outdated
@@ -2857,6 +2855,15 @@ def metrics_incremented(): | |||
self.waitFor( | |||
metrics_incremented, fail_msg="Expected metrics to update") | |||
|
|||
def testValidateDefaultConfigi2(self): |
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.
typo in function name
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.
thanks
max_workers=config["max_workers"], | ||
version=ray.__version__)) | ||
if node_type_name == config["head_node_type"]: | ||
node_type_data.setdefault("min_workers", 0) |
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.
We want max_workers
for the head node type defaulted to 0 (and to global max_workers for the rest of the node types).
I think the autoscaler infers min_workers internally 0 if min_workers is missing for a node type.
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.
Changed it to 0 form the head node, setting min_workers to 0 as default.
Other things that need to be changed: There's a test in The documentation on node type max workers should be updated The helm chart template ray/deploy/charts/ray/templates/raycluster.yaml Lines 22 to 23 in 55680a1
needs to be updated to ignore min_workers and max_workers if they're not specified in values.yaml ray/deploy/charts/ray/values.yaml Lines 16 to 17 in 55680a1
There's another example that needs to be modified here ray/deploy/components/example_cluster.yaml Lines 33 to 35 in 55680a1
|
thanks, I missed those |
This spot should mention the default behavior for the head node type max workers |
done |
lgtm -- I'm just going to run some quick manual checks that k8s stuff works |
The somewhat unorthodox convention we use for Ray github is to add reviewers as assignees (makes searching PRs to be reviewed easier for reviewers) |
LGTM -- would you mind doing a manual check by launching a cluster on AWS with these changes checked out locally? |
Co-authored-by: Dmitri Gekhtman <62982571+DmitriGekhtman@users.noreply.github.com>
Looks like there's a Mac build failure. Could you rebase or merge master to retry that? |
eh, sry, realized there's one more issue with the helm chart. As is, there's an incompatibility with the default To avoid the incompatibility, could you modify this part to default to 0 if minWorkers/maxWorkers is not present? There's Helm syntax for defaults which simplifies this. |
|
looks good now! |
Why are these changes needed?
Related issue number
Checks
scripts/format.sh
to lint the changes in this PR.