-
Notifications
You must be signed in to change notification settings - Fork 12
[Bugfix] Misc bug fixes #208
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -127,6 +127,7 @@ def allocate( | |||||||||||||||||||||||||
| owner: str, | ||||||||||||||||||||||||||
| must_colocate: bool = True, | ||||||||||||||||||||||||||
| node_selection_policy: Literal["pack", "spread"] = "spread", | ||||||||||||||||||||||||||
| require_consecutive: bool = True, | ||||||||||||||||||||||||||
| ) -> list[GPU]: | ||||||||||||||||||||||||||
| """Allocate a number of GPUs to a owner. | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
@@ -149,6 +150,14 @@ def allocate( | |||||||||||||||||||||||||
| owner: Owner name | ||||||||||||||||||||||||||
| must_colocate: Whether GPU colocation is required | ||||||||||||||||||||||||||
| node_selection_policy: Node selection policy ("pack" or "spread") | ||||||||||||||||||||||||||
| require_consecutive: If True (the default; only takes effect when | ||||||||||||||||||||||||||
| `must_colocate` and `num_gpus > 1`), the allocated GPUs must have | ||||||||||||||||||||||||||
| consecutive local ranks on the node. A task executor with tensor | ||||||||||||||||||||||||||
| parallelism shares a single contiguous slice of the per-node | ||||||||||||||||||||||||||
| sidecar shared-memory slab keyed by local rank (see | ||||||||||||||||||||||||||
| `sidecar.utils.init_shmem`), so its GPUs must be consecutive -- | ||||||||||||||||||||||||||
| not merely colocated. Set to False only for allocations that do | ||||||||||||||||||||||||||
| not back a single TP group. | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Raises: | ||||||||||||||||||||||||||
| CannotColocateError: If `must_colocate` is True and GPUs cannot | ||||||||||||||||||||||||||
|
|
@@ -207,8 +216,29 @@ def allocate( | |||||||||||||||||||||||||
| allocated_gpus: list[GPU] = [] | ||||||||||||||||||||||||||
| while num_allocated < num_gpus and node_priority: | ||||||||||||||||||||||||||
| _, _, node = heapq.heappop(node_priority) | ||||||||||||||||||||||||||
| gpus = self.node_to_gpus[node] | ||||||||||||||||||||||||||
| gpus = [gpu for gpu in gpus if gpu.is_free][: num_gpus - num_allocated] | ||||||||||||||||||||||||||
| # `node_to_gpus[node]` is sorted by local rank. | ||||||||||||||||||||||||||
| free_gpus = [gpu for gpu in self.node_to_gpus[node] if gpu.is_free] | ||||||||||||||||||||||||||
| if require_consecutive and num_gpus > 1 and must_colocate: | ||||||||||||||||||||||||||
| # Scan for the first run of `num_gpus` consecutive local ranks. | ||||||||||||||||||||||||||
| # Unlike taking the lowest-N free GPUs, this avoids straddling | ||||||||||||||||||||||||||
| # GPUs owned by other task managers (e.g. picking local ranks | ||||||||||||||||||||||||||
| # 2 and 6 when 3, 4, 5 are busy), which would violate the | ||||||||||||||||||||||||||
| # consecutive-local-rank invariant of the sidecar shared memory. | ||||||||||||||||||||||||||
| run: list[GPU] = [] | ||||||||||||||||||||||||||
| for gpu in free_gpus: | ||||||||||||||||||||||||||
| if run and gpu.local_rank != run[-1].local_rank + 1: | ||||||||||||||||||||||||||
| run = [] | ||||||||||||||||||||||||||
| run.append(gpu) | ||||||||||||||||||||||||||
| if len(run) == num_gpus: | ||||||||||||||||||||||||||
| break | ||||||||||||||||||||||||||
| if len(run) != num_gpus: | ||||||||||||||||||||||||||
| raise CannotColocateError( | ||||||||||||||||||||||||||
| f"Cannot allocate {num_gpus} GPUs with consecutive local ranks on a " | ||||||||||||||||||||||||||
| f"single node. Free GPUs on {node} are too fragmented.", | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
|
Comment on lines
+234
to
+238
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Raising
Suggested change
|
||||||||||||||||||||||||||
| gpus = run | ||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||
| gpus = free_gpus[: num_gpus - num_allocated] | ||||||||||||||||||||||||||
| for gpu in gpus: | ||||||||||||||||||||||||||
| allocated_gpus.append(gpu.allocate_to(owner)) | ||||||||||||||||||||||||||
| num_allocated += 1 | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
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.
If the instance name is not yet committed because a concurrent deployment is still in Phase 2, skipping the refcount bump entirely will cause the cluster-wide refcount to be undercounted. This can lead to premature teardown of the shared task when one of the active users unregisters it. Instead of skipping, we should poll and wait for the concurrent deployment to commit the instance name.