-
Notifications
You must be signed in to change notification settings - Fork 42
fix(geak): forward the run's actual GPU pin in the handoff #1321
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
Open
zihaoanllm
wants to merge
6
commits into
main
Choose a base branch
from
fix/geak-handoff-gpu-pin
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
397c5b0
fix(geak): forward the run's actual GPU pin in the handoff
zihaoanllm 110189e
fix(geak): do not let the recipe's autofilled ROCR mask pose as a pin
zihaoanllm 34bb9bc
fix(geak): make the forwarded pin survive every mask spelling and shape
zihaoanllm 258111a
docs(geak): close the two round-1 wording items the round-2 fixes reo…
zihaoanllm 158ca2c
fix(geak): close the two round-2 items that were only half-fixed
zihaoanllm d812fcf
Fix CI lint and two CodeQL notes on visible_devices
zihaoanllm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,173 @@ | ||
| # SPDX-FileCopyrightText: 2026 Advanced Micro Devices, Inc. | ||
| # SPDX-License-Identifier: MIT | ||
| """Visible-device mask names and parsing, shared by every layer that reads one. | ||
|
|
||
| The same tuple of mask variables and the same "split on ``,``/``;``, keep the | ||
| first occurrence of each non-negative int" parser had accumulated five separate | ||
| copies (``bus/gpu_pool``, ``policy/gate``, ``actions/executors/_ray_serving``, | ||
| ``common/env_safety``, ``loop/coordinator_helpers``), and their empty-mask | ||
| semantics had already drifted apart. This module is the single definition; it | ||
| imports nothing outside the standard library so the pure-helper layers can use | ||
| it without dragging in the SQLite connection ``gpu_pool`` owns. | ||
|
|
||
| Two var tuples, deliberately distinct: | ||
|
|
||
| * :data:`COUNTING_VISIBLE_DEVICE_VARS` — what ``gpu_pool`` / ``gate`` consult to | ||
| answer "how many GPUs does this process have". Identical to what those layers | ||
| always used; widening it would change GPU accounting repo-wide. It is DERIVED | ||
| from the chain below by subtracting an explicit exclusion set, so the two can | ||
| never drift: a new var is counted unless someone names it as uncounted, and a | ||
| test asserts every chain member is classified. | ||
| * :data:`VISIBLE_DEVICE_VARS` — the full ROCm pin-resolution chain, used when | ||
| answering "where is this run pinned". ``HSA_VISIBLE_DEVICES`` is ROCr's legacy | ||
| name and ``GPU_DEVICE_ORDINAL`` is the legacy HIP-level filter; a run pinned | ||
| with either is really pinned, and omitting them left it reported as unpinned. | ||
|
|
||
| The ROCr-level and HIP-level groups are exposed separately because the | ||
| distinction is load-bearing: a ROCr-level mask renumbers the devices the child | ||
| sees (so ids inside it are LOGICAL), while a HIP-level mask indexes into | ||
| whatever ROCr already exposed (so its ids are absolute unless a ROCr mask is | ||
| also in force). | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from typing import Any | ||
|
|
||
| #: ROCr-level masks: these slice the device set and renumber it ``0..N-1``. | ||
| #: ``HSA_VISIBLE_DEVICES`` is the legacy spelling; ``ROCR_VISIBLE_DEVICES`` | ||
| #: wins when both are set. | ||
| ROCR_LEVEL_VARS: tuple[str, ...] = ( | ||
| "ROCR_VISIBLE_DEVICES", | ||
| "HSA_VISIBLE_DEVICES", | ||
| ) | ||
|
|
||
| #: HIP-level masks: these index INTO whatever ROCr exposed. | ||
| HIP_LEVEL_VARS: tuple[str, ...] = ( | ||
| "HIP_VISIBLE_DEVICES", | ||
| "CUDA_VISIBLE_DEVICES", | ||
| "GPU_DEVICE_ORDINAL", | ||
| ) | ||
|
|
||
| #: Full pin-resolution precedence: ROCr level before HIP level, canonical | ||
| #: spelling before legacy within each level. | ||
| VISIBLE_DEVICE_VARS: tuple[str, ...] = ROCR_LEVEL_VARS + HIP_LEVEL_VARS | ||
|
|
||
| #: Vars the capacity-counting layers deliberately do NOT read. | ||
| #: | ||
| #: Both are legacy aliases of a var already in the counting set, and both are | ||
| #: honoured only when their modern spelling is absent — so counting them would | ||
| #: not find a GPU the modern spelling missed, it would only change the answer on | ||
| #: hosts that happen to export the legacy name. That is a repo-wide GPU | ||
| #: accounting change, not a bugfix, so it stays out until someone makes it | ||
| #: deliberately. | ||
| _UNCOUNTED_VISIBLE_DEVICE_VARS: frozenset[str] = frozenset( | ||
| { | ||
| "HSA_VISIBLE_DEVICES", | ||
| "GPU_DEVICE_ORDINAL", | ||
| } | ||
| ) | ||
|
|
||
| #: The subset the capacity-counting layers read — ``gpu_pool``, ``policy.gate``, | ||
| #: ``actions.executors._ray_serving``. DERIVED from | ||
| #: :data:`VISIBLE_DEVICE_VARS` rather than re-listed, so a var added to the | ||
| #: precedence chain is counted by default and can only be left out by naming it | ||
| #: in :data:`_UNCOUNTED_VISIBLE_DEVICE_VARS`. The two tuples cannot silently | ||
| #: drift apart: ``test_visible_devices.py`` asserts every chain member is | ||
| #: classified exactly once. | ||
| COUNTING_VISIBLE_DEVICE_VARS: tuple[str, ...] = tuple( | ||
|
github-advanced-security[bot] marked this conversation as resolved.
Fixed
|
||
| var for var in VISIBLE_DEVICE_VARS if var not in _UNCOUNTED_VISIBLE_DEVICE_VARS | ||
| ) | ||
|
|
||
| #: Every name that selects hardware rather than tuning it (superset of the | ||
| #: precedence chain), for env scrubbing. | ||
| GPU_MASK_ENV_NAMES: frozenset[str] = frozenset(VISIBLE_DEVICE_VARS) | ||
|
|
||
|
|
||
| def is_rocr_level(var: str) -> bool: | ||
| """Does ``var`` slice and renumber the device set (rather than index it)? | ||
|
|
||
| Args: | ||
| var: A visible-devices env var name. | ||
|
|
||
| Returns: | ||
| ``True`` for the ROCr-level masks, whose member ids are logical | ||
| positions from the child's point of view. | ||
| """ | ||
| return str(var or "") in ROCR_LEVEL_VARS | ||
|
|
||
|
|
||
| def mask_tokens(raw: Any) -> list[str]: | ||
| """Split a visible-devices mask into its device tokens. | ||
|
|
||
| Tokens are NOT required to be numeric: ROCm accepts GPU UUID masks | ||
| (``ROCR_VISIBLE_DEVICES=GPU-a1b2c3,GPU-d4e5f6``), and those still say how | ||
| MANY devices the child will see, which is all the logical-index arithmetic | ||
| needs. | ||
|
|
||
| Args: | ||
| raw: A ``,``/``;``-separated mask, or an already-parsed YAML sequence. | ||
|
|
||
| Returns: | ||
| Non-empty tokens in order, duplicates preserved. | ||
| """ | ||
| if isinstance(raw, (list, tuple)): | ||
| parts = [str(p) for p in raw] | ||
| else: | ||
| parts = str(raw if raw is not None else "").replace(";", ",").split(",") | ||
| return [tok for tok in (p.strip() for p in parts) if tok] | ||
|
|
||
|
|
||
| def effective_mask_tokens(raw: Any) -> list[str]: | ||
| """The devices a mask actually exposes, in the order the runtime sees them. | ||
|
|
||
| :func:`mask_tokens` is the LITERAL split; this is the *effective* set. ROCm | ||
| exposes ``ROCR_VISIBLE_DEVICES="3,3,2"`` as two devices, not three, and | ||
| drops a negative ordinal. Deriving both the device COUNT and the forwarded | ||
| id list from this one function is what keeps them from disagreeing: counting | ||
| literal tokens inflates the count (logical index 2 of a 2-device set is | ||
| invalid), while re-serializing from the parsed ints deflates it. | ||
|
|
||
| Non-numeric tokens are kept — a UUID mask names real devices — so this | ||
| cannot filter out a genuinely invalid non-numeric entry; it removes only the | ||
| two forms that are unambiguously not extra devices. | ||
|
|
||
| Args: | ||
| raw: A ``,``/``;``-separated mask, or an already-parsed YAML sequence. | ||
|
|
||
| Returns: | ||
| Tokens with duplicates and negative ordinals removed, first-seen order. | ||
| """ | ||
| out: list[str] = [] | ||
| for tok in mask_tokens(raw): | ||
| if tok in out: | ||
| continue | ||
| try: | ||
| if int(tok) < 0: | ||
| continue | ||
| except ValueError: | ||
|
github-code-quality[bot] marked this conversation as resolved.
Fixed
github-advanced-security[bot] marked this conversation as resolved.
Fixed
|
||
| pass | ||
| out.append(tok) | ||
| return out | ||
|
|
||
|
|
||
| def parse_device_list(raw: Any) -> list[int]: | ||
| """Parse a visible-devices mask into absolute NUMERIC device ids. | ||
|
|
||
| Args: | ||
| raw: A ``,``/``;``-separated mask (``"4,5,6,7"``) or a YAML sequence; | ||
| ``None`` and malformed entries are tolerated. | ||
|
|
||
| Returns: | ||
| Unique non-negative ids in first-seen order; ``[]`` for an empty mask | ||
| and for a well-formed but non-numeric one (e.g. a UUID mask), which is | ||
| why callers that need a device COUNT must use | ||
| :func:`effective_mask_tokens`. | ||
| """ | ||
| out: list[int] = [] | ||
| for tok in effective_mask_tokens(raw): | ||
| try: | ||
| out.append(int(tok)) | ||
| except ValueError: | ||
| continue | ||
| return out | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.