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] Allow GPU upscaling for non-GPU workloads, with lowest priority #31202

Merged
merged 23 commits into from
Dec 21, 2022
Merged

[Autoscaler] Allow GPU upscaling for non-GPU workloads, with lowest priority #31202

merged 23 commits into from
Dec 21, 2022

Conversation

DmitriGekhtman
Copy link
Contributor

@DmitriGekhtman DmitriGekhtman commented Dec 19, 2022 •
edited
Loading

Why are these changes needed?

Closes #20476:
Instead of preventing GPU upscaling due to non-GPU tasks, prefer non-GPU nodes by assigning low utilization score to the GPU nodes.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@DmitriGekhtman
Comment
425e714 
Signed-off-by: Dmitri Gekhtman <[email protected]>
@DmitriGekhtman
Re-order.
c519f80 
Signed-off-by: Dmitri Gekhtman <[email protected]>
wuisawesome
wuisawesome reviewed Dec 20, 2022
View reviewed changes
Copy link
Contributor

@wuisawesome wuisawesome left a comment

Choose a reason for hiding this comment

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

I'm really not a fan of making this logic more and more complicated here. Could we achieve the same result or better by just making gpu nodes lower in the utilization scorer if the resource demand doesn't include gpu requests?

@DmitriGekhtman
style
4401a1a 
Signed-off-by: Dmitri Gekhtman <[email protected]>
@DmitriGekhtman
Copy link
Contributor Author

I'm really not a fan of making this logic more and more complicated here. Could we achieve the same result or better by just making gpu nodes lower in the utilization scorer if the resource demand doesn't include gpu requests?

Brilliant, yes.

@DmitriGekhtman
Revert resource demand scheduler.
a821415 
Signed-off-by: Dmitri Gekhtman <[email protected]>
@DmitriGekhtman
Implement Alex's common sense.
ba06d14 
Signed-off-by: Dmitri Gekhtman <[email protected]>
@DmitriGekhtman
Fix types.
da3831a 
Signed-off-by: Dmitri Gekhtman <[email protected]>
@DmitriGekhtman
Insert reference.
9569cb8 
Signed-off-by: Dmitri Gekhtman <[email protected]>
@DmitriGekhtman
Fix comment.
9797ea4 
Signed-off-by: Dmitri Gekhtman <[email protected]>
@DmitriGekhtman
Rearrange.
804e843 
Signed-off-by: Dmitri Gekhtman <[email protected]>
@DmitriGekhtman
Simplify test.
26ffb40 
Signed-off-by: Dmitri Gekhtman <[email protected]>
@DmitriGekhtman
Explain.
a595afa 
Signed-off-by: Dmitri Gekhtman <[email protected]>
@DmitriGekhtman
Decrease timeout.
4da06da 
Signed-off-by: Dmitri Gekhtman <[email protected]>
@DmitriGekhtman
Test name.
a77f823 
Signed-off-by: Dmitri Gekhtman <[email protected]>
@DmitriGekhtman DmitriGekhtman added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Dec 20, 2022
@DmitriGekhtman
Copy link
Contributor Author

Actually, to retain the relative ranking, will implement this by pre-pending an extra field to the priority score.

@DmitriGekhtman
Prepend a field to the tuple.
0e571a4 
Signed-off-by: Dmitri Gekhtman <[email protected]>
@DmitriGekhtman DmitriGekhtman removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Dec 20, 2022
@DmitriGekhtman
Copy link
Contributor Author

Actually, to retain the relative ranking, will implement this by pre-pending an extra field to the priority score.

Implemented that.

@DmitriGekhtman
Tests.
7f50567 
Signed-off-by: Dmitri Gekhtman <[email protected]>
@DmitriGekhtman
lint
e0bc69d 
Signed-off-by: Dmitri Gekhtman <[email protected]>
@DmitriGekhtman
remove breakpoint
27ffedd 
Signed-off-by: Dmitri Gekhtman <[email protected]>
@DmitriGekhtman DmitriGekhtman mentioned this pull request Dec 20, 2022
Merged
@DmitriGekhtman
Fix test.
b812676 
Signed-off-by: Dmitri Gekhtman <[email protected]>
@DmitriGekhtman
Add to test.
83c6e15 
Signed-off-by: Dmitri Gekhtman <[email protected]>
@DmitriGekhtman DmitriGekhtman changed the title Dmitri/fix gpu upscaling [Autoscaler] Allow GPU upscaling for non-GPU workloads, with lowest priority Dec 20, 2022
@DmitriGekhtman DmitriGekhtman added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Dec 20, 2022
@DmitriGekhtman
Copy link
Contributor Author

Can be reviewed again.

@DmitriGekhtman
Comment wording.
d151ac8 
Signed-off-by: Dmitri Gekhtman <[email protected]>
wuisawesome
wuisawesome approved these changes Dec 20, 2022
View reviewed changes
Copy link
Contributor

@wuisawesome wuisawesome left a comment

Choose a reason for hiding this comment

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

beautiful

@DmitriGekhtman DmitriGekhtman merged commit c56d135 into ray-project:master Dec 21, 2022
AmeerHajAli pushed a commit that referenced this pull request Jan 12, 2023
@DmitriGekhtman @AmeerHajAli
[Autoscaler] Allow GPU upscaling for non-GPU workloads, with lowest p…
49959e8 
…riority (#31202)

Closes #20476:
Instead of preventing GPU upscaling due to non-GPU tasks, prefer non-GPU nodes by assigning low utilization score to the GPU nodes.

Signed-off-by: Dmitri Gekhtman <[email protected]>
tamohannes pushed a commit to ju2ez/ray that referenced this pull request Jan 25, 2023
@DmitriGekhtman @tamohannes
[Autoscaler] Allow GPU upscaling for non-GPU workloads, with lowest p…
b036a02 
…riority (ray-project#31202)

Closes ray-project#20476:
Instead of preventing GPU upscaling due to non-GPU tasks, prefer non-GPU nodes by assigning low utilization score to the GPU nodes.

Signed-off-by: Dmitri Gekhtman <[email protected]>
Signed-off-by: tmynn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wuisawesome wuisawesome wuisawesome approved these changes

@ericl ericl Awaiting requested review from ericl

Assignees

@ericl ericl

@wuisawesome wuisawesome

Labels
tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] Autoscaler doesn't scale CPU-only workloads to workers with GPU
3 participants
@DmitriGekhtman @wuisawesome @ericl