Repository navigation
[core] Fix GPU fractional scheduling by tracking per-instance availability - #65871
dancingactor wants to merge 26 commits into
Conversation
e93241f to
45f8d0c
Compare
…ility Signed-off-by: dancingactor <s990346@gmail.com>
Signed-off-by: dancingactor <s990346@gmail.com>
45f8d0c to
0457ac8
Compare
b918a99 to
ad25b64
Compare
Signed-off-by: dancingactor <s990346@gmail.com>
cd44e53 to
e5d6c45
Compare
…uster resource view Signed-off-by: dancingactor <s990346@gmail.com>
6d21227 to
3cebedc
Compare
There was a problem hiding this comment.
Code Review
This pull request transitions Ray's resource tracking and scheduling from aggregate scalar values to per-instance resource availability, preventing fractional resource requests (such as GPUs) from being scheduled on nodes with fragmented, insufficient instances. The review feedback highlights critical backward compatibility concerns during rolling upgrades. Specifically, the reviewer recommends retaining the deprecated resources_available field in the protobuf definition, ensuring upgraded raylets continue to populate it, and implementing fallback logic in the GCS and raylet resource managers when the new resources_available_instances field is absent. Additionally, the reviewer suggests a minor cleanup in the tests to use GetAvailableSum directly for better efficiency.
| /// the bundle with index 2 contains 2 GPU. | ||
| /// | ||
| /// The current node resource can be as follows: | ||
| /// resource id: total, available |
There was a problem hiding this comment.
New APIs missing Doxygen tags
Low Severity
/** ... */ block comments using @ tags (@brief, and @param/@return where applicable).
Additional Locations (2)
Triggered by project rule: Bugbot Rules
Reviewed by Cursor Bugbot for commit 7543863. Configure here.
Signed-off-by: dancingactor <s990346@gmail.com>
a8f6939 to
faf4ac7
Compare
Signed-off-by: Ding-Xin Chen <s990346@gmail.com>
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
|
unstale |
…vailable resources (#66318) ## Description This PR skips placement group resource parsing when updating existing available resources. Since this updating logic is going to be used in global scheduling (selecting a node within the cluster to assign a task) and local allocation in the upcoming PR #65871, this critical path needs to be optimized. ## Performance Test The optimization is **1.7×–24× faster** on the alloc/free paths: | Scenario | Before | After | Speedup | | --------------------------- | ------- | ------- | ---------- | | CPU alloc+free | 4252 ns | 2448 ns | **1.74×** | | CPU alloc failure | 2261 ns | 2229 ns | **~1.0×** | | GPU alloc+free (full) | 4283 ns | 2357 ns | **1.82×** | | GPU alloc+free (fractional) | 4294 ns | 2412 ns | **1.78×** | | GPU alloc failure | 2214 ns | 2306 ns | **~1.0×** | | Free only | 980 ns | 41 ns | **24.0×** | | Subtract | 1016 ns | 101 ns | **10.1×** | ## Related PR Related to #65871 --------- Signed-off-by: dancingactor <s990346@gmail.com> Co-authored-by: Yicheng-Lu-llll <51814063+Yicheng-Lu-llll@users.noreply.github.com>
Signed-off-by: Ding-Xin Chen <s990346@gmail.com>
Signed-off-by: Ding-Xin Chen <s990346@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Reviewed by Cursor Bugbot for commit 0f80747. Configure here.
| // performance regressions in spillback. | ||
|
|
||
| return true; | ||
| return allocation; |
There was a problem hiding this comment.
Bundle reserve ignores packing failures
High Severity
SubtractNodeAvailableResources now bin-packs per instance and can fail, but AcquireBundleResources still ignores the result and walks an unordered BundleLocations map. A placement group that just scheduled can fail to reserve in a different order, leaving GCS availability too high so another group is placed on the same GPUs.
Reviewed by Cursor Bugbot for commit 0f80747. Configure here.


Background & Problem
In short, the issue is essentially the different checking logic between global scheduling (selects a node with "sufficient resources" from all nodes) and local allocation (Raylet allocates specific GPU instances on the selected node).
More specifically, for unit resources like GPU, the accurate resource usage representation needs to fully show the GPU topology. For example,
[0.5, 0.5]represents a node with 2 GPUs, each instance having 0.5 availability. So when you want to run a task with 0.8 GPUs, there is no sufficient room to fit even though the aggregate available is larger than 0.8(In Ray, a task request 0.8 GPU means requiring 80% of a single GPU, it cannot use 40% on GPU 0 + 40% on GPU 1). However, at the scheduling level, Ray currently uses scalars to represent resource usage. It falsely assumes the node has sufficient capacity because 1.0 > 0.8, leading to a false positiveThe goal of this PR is to make global scheduling also use per-instance vectors to represent unit-instance resources, so that both scheduling and allocation use the same and correct representation
Here is a minimal reproduction script to demonstrate (you could also see #52133 and #54729 for more details):
In this example, the actor gets stuck in an infinite spill back loop because the scheduler keeps picking the fragmented head node.
Actually this is also a big problem in k8s. The different logic between k8s scheduler and allocation logic in kubelet causes scheduling failures, and was an issue at my previous company since these two parts are managed by different teams. The solution is always trying to unify them and ensure no big perf regression.
If we think deeper, the root cause is that the logic in scheduling and allocation is different, and this causes a lot of problems. So this PR is essentially to make sure scheduling and allocation use the same computing logic.
Solution
You might think this is simple! We could either:
Approach 1 is not ideal. When multiple nodes are fragmented, it could cause a ping-pong infinite loop since each node passes the aggregate check but fails local allocation.
So let's focus on changing unit resources to also use per-instance vectors.
Changes
Here is a summary of the key changes:
NodeResources.availablefrom scalar to per-instance vectorCanAllocateso that scheduling checks and local allocation use the same per instance logic for each resource typeMake RaySyncer carry unit resource (GPU) topology in sync messages
Fix STRICT_PACK scheduling policy for per instance available
STRICT_PACK puts all bundles on one node. Previously it aggregated all bundle
demands into one request. This aggregation is problematic: e.g. node with GPU=[0.7, 0.3], two bundles each need 0.5 GPU.
Aggregate 1.0 >= 1.0 passes (note: this aggregation happens inside the STRICT_PACK policy itself, not in the general scheduling checking logic), but actual per bundle allocation fails (no single instance has 0.5 free for the second bundle). Causes unnecessary reschedules.
We already have per instance checking logic from the previous changes. The extra work here is that for this policy, we try allocating each bundle one by one and rollback after, instead of aggregating all bundle demands into one request.
Additional updates
Updated
LocalResourceManagerto prevent broadcasting zero-availability non-implicit resources. Previously, the old class constructor implicitly pruned 0-availability resources. Because the refactored code uses a new class constructor that preserves them, explicit filtering was addedPerformance Testing
Config: 200 PGs, 0.04 GPU/CPU per bundle, 5 nodes, STRICT_SPREAD