Add spread allocation policy for maximum per-pod distinct-GPU coverage - #1936
Conversation
31db61f to
0d2cb9f
Compare
7c290ad to
65c4fc3
Compare
65c4fc3 to
acdf40a
Compare
5102bfb to
83ba5e7
Compare
7cf1fbd to
f3eb95c
Compare
f3eb95c to
6a77d8a
Compare
|
THanks @jonathan-meiri ! Can you rebase the PR? |
6a77d8a to
90ce189
Compare
|
Rebased on latest main 🙏 |
90ce189 to
a82bb98
Compare
a82bb98 to
046b76b
Compare
abrarshivani
left a comment
There was a problem hiding this comment.
@jonathan-meiri Thanks for working on this. LGTM.
|
/ok to test 046b76b |
| spec.AllocationPolicySpread: func(i, j *gpuAllocState) bool { | ||
| if i.pickedFrom != j.pickedFrom { | ||
| return i.pickedFrom < j.pickedFrom | ||
| } | ||
| return i.count.allocated() < j.count.allocated() |
There was a problem hiding this comment.
What about MIG devices?
With MIG, spread would treat each MIG UUID as a distinct physical GPU, yes?. Candidate buckets use device UUIDs, while multiple MIG devices can share one parent. A multi-slot allocation can remain on one physical GPU.
There was a problem hiding this comment.
Good catch, you're right. Buckets key on the device UUID (GetID), and each MIG instance has its own UUID, so spread spans distinct MIG instances that can share one physical GPU. This is pre-existing (distributed/packed too).
I've kept this PR to the spread comparator and opened #2036 to bucket by parent GPU (from Device.Index) for all policies.
Let me know if you'd prefer to fold that into this PR, or if you're happy keeping it separate.
| // Seed pickedFrom from required so a GPU that already holds a required | ||
| // replica counts as touched by this allocation. Otherwise spread, which | ||
| // orders primarily by pickedFrom, could pick that same physical GPU again | ||
| // instead of spanning a new one. | ||
| for _, req := range required { | ||
| if item, ok := byGPU[AnnotatedID(req).GetID()]; ok { | ||
| item.pickedFrom++ | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
If I am not wrong, this changes the tie-breaking logic and it would apply to the distributed and packed allocation policies. This goes beyond the scope of this PR - which is an introduction of a new allocation policy
There was a problem hiding this comment.
Good point, seeding pickedFrom leaked into distributed/packed. Fixed: pickedFrom stays pure, required accounting is now spread-only (touched()), with a test to lock it in.
Packed's tie-break could improve too, but that's pre-existing. I'll follow up separately.
3d1abfb to
96de15f
Compare
Allocation buckets keyed on the device UUID, so each MIG instance looked like a distinct physical GPU. distributed/packed/spread could therefore stack a multi-slot request onto MIG instances of one physical card. Bucket by physical GPU instead: MIG instances group by parent (from Device.Index), other devices by UUID. Follow-up to NVIDIA#1936. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: runatom-ai <258621014+runatom-ai@users.noreply.github.com> Signed-off-by: Jonathan Meiri <33288957+Meiri28@users.noreply.github.com>
96de15f to
3f86af2
Compare
Allocation buckets keyed on the device UUID, so each MIG instance looked like a distinct physical GPU. distributed/packed/spread could therefore stack a multi-slot request onto MIG instances of one physical card. Bucket by physical GPU instead: MIG instances group by parent (from Device.Index), other devices by UUID. Follow-up to NVIDIA#1936. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: runatom-ai <258621014+runatom-ai@users.noreply.github.com> Signed-off-by: Jonathan Meiri <33288957+Meiri28@users.noreply.github.com>
There was a problem hiding this comment.
This godoc needs to be updated now that the spread comparator has been updated to use the touched() method as a primary sort key.
3f86af2 to
37fece5
Compare
`distributed` minimizes cluster-wide load imbalance — it prefers the GPU with the fewest replicas already allocated across all pods. `packed` does the opposite for bin-packing. Neither guarantees that a single pod's multi-slot request touches distinct physical GPUs: given GPU-0 with 5 allocated and GPU-1 with 3 allocated, a pod requesting 2 slots under `distributed` gets both slots on GPU-1, because GPU-1 has the lower cluster-wide allocated count both before and after the first pick. Add a third policy, `spread`, that primarily orders by pickedFrom (the per-allocation counter of how many slots the current pod has taken from each GPU) and only tie-breaks by allocated(). The pod's own picks therefore drive selection: after taking one slot from GPU-1, GPU-0 becomes preferred (pickedFrom=0 < 1) regardless of cluster-wide load. Result: the pod's slots span as many distinct physical GPUs as possible, which is what multi-GPU workloads (data-parallel training, NCCL, tensor-parallel) actually need. The `replicaComparator` signature is enriched from `func(i, j *replicaCount) bool` to `func(i, j *gpuAllocState) bool` so a comparator can freely mix cluster-wide state (allocated()) and per-allocation state (pickedFrom). Each policy now owns both its primary ordering and its tie-break; the queue's Less becomes a pure passthrough. distributed and packed are behavior-preserving — the primary+tie-break they used to get from greedyAlloc's wrapping is spelled out in the comparator body itself. Validate `spread` in main.go alongside distributed and packed. Add TestSpreadAlloc mirroring the existing policy suites plus TestSpreadPrefersUntouchedGPU and TestSpreadPrefersDistinctGPUsEvenWhenUnbalanced for the defining behavior. TestComparatorsOrderSolelyByAllocated narrows to distributed/packed since spread intentionally violates that invariant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-Authored-By: runatom-ai <258621014+runatom-ai@users.noreply.github.com> Signed-off-by: Jonathan Meiri <33288957+Meiri28@users.noreply.github.com>
37fece5 to
7062bd0
Compare
Allocation buckets keyed on the device UUID, so each MIG instance looked like a distinct physical GPU. distributed/packed/spread could therefore stack a multi-slot request onto MIG instances of one physical card. Bucket by physical GPU instead: MIG instances group by parent (from Device.Index), other devices by UUID. Follow-up to NVIDIA#1936. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: runatom-ai <258621014+runatom-ai@users.noreply.github.com> Signed-off-by: Jonathan Meiri <33288957+Meiri28@users.noreply.github.com>
|
/ok to test 7062bd0 |
|
Thanks @tariq1890 and @abrarshivani for the thorough reviews and the merge, really appreciate it! The MIG parent-GPU bucketing follow-up we discussed is up as #2036 whenever you have a chance. |
Allocation buckets keyed on the device UUID, so each MIG instance looked like a distinct physical GPU. distributed/packed/spread could therefore stack a multi-slot request onto MIG instances of one physical card. Bucket by physical GPU instead: MIG instances group by parent (from Device.Index), other devices by UUID. Follow-up to NVIDIA#1936. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: runatom-ai <258621014+runatom-ai@users.noreply.github.com> Signed-off-by: Jonathan Meiri <33288957+Meiri28@users.noreply.github.com>
Summary
spreadas a third value for--shared-devices-allocation-policy, alongsidedistributed(default) andpacked.spreadprefers physical GPUs the current pod has not yet picked from, maximizing distinct-GPU coverage for a single multi-slot allocation.distributed) is unchanged.Depends on #1826 (heap refactor) — the current diff includes that commit until #1826 lands, then collapses to just the
spreadpolicy addition. Opened as draft for now.Contributed by @Meiri28 on behalf of @runatom-ai.
Motivation
distributedminimizes cluster-wide load imbalance — it prefers the GPU with the fewest replicas already allocated across all pods.packeddoes the opposite for bin-packing. Neither guarantees that a single pod's multi-slot request touches distinct physical GPUs.Consider a node where GPU-0 has 3 free replicas (5 allocated) and GPU-1 has 5 free (3 allocated). A pod requests 2:
Concrete workloads that benefit:
@rajatchopra explicitly floated additional policies on #1788:
This adds the third policy that #1621's abstraction was built for.
Design
Small refactor: the
replicaComparatorsignature is enriched fromfunc(i, j *replicaCount) booltofunc(i, j *gpuAllocState) bool. Each policy now owns both its primary ordering and tie-break — the queue'sLessis a pure passthrough:distributedandpackedare behavior-preserving — the primary+tie-break they used to get fromgreedyAlloc's wrapping is now spelled out in the comparator body itself. All existing tests (TestDistributedAlloc,TestPackedAlloc,TestPackedVsDistributedContrast) pass unchanged.Usage
--shared-devices-allocation-policy=spread # or SHARED_DEVICES_ALLOCATION_POLICY=spreadConfig file:
Commits are DCO-signed.