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

Jump to bottom

[autoscaler] Fix semantics of request_resources #11820

Merged
merged 9 commits into from
Nov 9, 2020
Merged

[autoscaler] Fix semantics of request_resources #11820

merged 9 commits into from
Nov 9, 2020

Conversation

ericl
Copy link
Contributor

@ericl ericl commented Nov 5, 2020 •
edited

Why are these changes needed?

The semantics of request_resources was broken in the multi node autoscaler, since we did not take into account existing resource usage.

  • Fix it so the cluster will be scaled up to fit the requested bundles, as if the cluster was unutilized.
  • Allow request_resources() to bypass the launch rate limit.
  • Chunk node launches into 5 nodes launched per API request.

Related issue number

Closes #11417
Closes #10836

AmeerHajAli
AmeerHajAli requested changes Nov 5, 2020
View reviewed changes
Copy link
Contributor

@AmeerHajAli AmeerHajAli left a comment

Choose a reason for hiding this comment

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

Left a few comments, please take a look.

@ericl
Copy link
Contributor Author

ericl commented Nov 5, 2020

Please remember to add the author action required label!

@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Nov 5, 2020
@ericl ericl removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Nov 5, 2020
@AmeerHajAli AmeerHajAli self-requested a review November 6, 2020 11:05
@AmeerHajAli AmeerHajAli added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Nov 6, 2020
AmeerHajAli
AmeerHajAli requested changes Nov 6, 2020
View reviewed changes
Copy link
Contributor

@AmeerHajAli AmeerHajAli left a comment

Choose a reason for hiding this comment

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

See my two comments.

ericl
ericl commented Nov 7, 2020
View reviewed changes
Copy link
Contributor Author

@ericl ericl left a comment

Choose a reason for hiding this comment

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

@AmeerHajAli I think you're misunderstanding the logic; the nodes to add based on requests is a subset of nodes to add. It's already included due to this line:

 101:           resource_demands += resource_requests

@ericl ericl removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Nov 7, 2020
@AmeerHajAli AmeerHajAli added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Nov 9, 2020
@AmeerHajAli AmeerHajAli removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Nov 9, 2020
@AmeerHajAli
Copy link
Contributor

LGTM.

@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Nov 9, 2020
@ericl ericl merged commit a9cf014 into ray-project:master Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AmeerHajAli AmeerHajAli AmeerHajAli approved these changes

Assignees

@wuisawesome wuisawesome

@AmeerHajAli AmeerHajAli

Labels
@author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Autoscaler] Request Resources semantics fix [autoscaler] Stabilize and document request_resources() API
3 participants
@ericl @AmeerHajAli @wuisawesome