design(amd): update amd design docs - #2067
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe AMD vGPU documentation updates memory allocation to MiB, compute allocation to percentages, and CU partitioning to a scheduler/device-plugin workflow using per-container ChangesAMD vGPU protocol
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the AMD vGPU design document (docs/develop/amd-vgpu.md) to reflect architectural changes. Specifically, it updates the memory allocation unit to MiB, changes the compute allocation (amd.com/gpucores) from a raw CU count to a percentage, and switches the environment variable from ROC_GLOBAL_CU_MASK to HSA_CU_MASK. It also refines the allocation protocol annotations and documents a new limitation regarding mixed GPU types on a single node. The review comments correctly identify grammatical errors, typos, and capitalization inconsistencies in the newly added sections, providing clear and actionable corrections.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
docs/develop/amd-vgpu.md (1)
91-94: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winName the consumer for
HIP_DEVICE_MEMORY_LIMIT.docs/develop/amd-vgpu.md:91-94should say that HAMi’s AMDLD_AUDITlayer consumes this variable and document the expected<MiB>mformat/version contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/develop/amd-vgpu.md` around lines 91 - 94, Update the AMD vGPU documentation around the `HIP_DEVICE_MEMORY_LIMIT` description to identify HAMi’s AMD `LD_AUDIT` layer as the consumer, and explicitly document that the value must use the `<MiB>m` format as the expected version contract. Preserve the existing container-scoping and per-container allocation details.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/develop/amd-vgpu.md`:
- Around line 98-104: Update the AMD vGPU documentation around amd.com/gpucores
to define the accepted percentage range and a single deterministic conversion
rule for fractional CU results, then state that the scheduler records that
rounded CU count in the standard AMD allocation annotation consumed by the
device-plugin. Ensure the documented behavior consistently prevents over- or
under-allocation.
- Around line 111-115: Update the mixed-device limitation bullet in amd-vgpu.md
to clearly state that a node cannot support multiple GPU types because the
device plugin derives the GPU type from the single amd.com/gpu.product-name
label. Correct the grammar while preserving the existing limitation and
rationale.
- Around line 23-24: Update the HSA_CU_MASK description in the AMD vGPU
documentation to replace “arbitrary” with “fine-grained, hardware-valid,”
preserving the existing explanation of per-pod CU partitioning and the
contrasting hardware-partitioning statement.
- Around line 64-73: Update the CU allocation contract in the device-plugin
documentation to define each allocated UUID’s container-local GPU index after
ROCR_VISIBLE_DEVICES reordering. Specify how the UUID-to-CU-range entries are
paired with those indices when generating HSA_CU_MASK, while preserving the
existing CU ID-list grammar and annotation format.
- Around line 64-73: Clarify the scope of hami.io/amd-cu-allocated in the
documentation: state explicitly whether it is written as a pod annotation or
returned in the container allocation response, and identify the component that
reads it to inject HSA_CU_MASK at container start. Align the surrounding wording
so the allocation source and consumer are unambiguous.
---
Nitpick comments:
In `@docs/develop/amd-vgpu.md`:
- Around line 91-94: Update the AMD vGPU documentation around the
`HIP_DEVICE_MEMORY_LIMIT` description to identify HAMi’s AMD `LD_AUDIT` layer as
the consumer, and explicitly document that the value must use the `<MiB>m`
format as the expected version contract. Preserve the existing container-scoping
and per-container allocation details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f2024e4d-3e0a-40a8-966b-0da74986f847
📒 Files selected for processing (1)
docs/develop/amd-vgpu.md
|
pls also address the coderabbit points on wgp cu pairing, the gpu index ambiguity in hsa_cu_mask, and the percentage rounding rule, those look like real gaps not just style |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/develop/amd-vgpu.md`:
- Around line 133-143: Update the cuCount conversion documentation to define how
round() handles exact .5 ties, specifying round-half-up (or another single
deterministic rule), and state that this rule is applied before clamping and
recording the result. Keep the existing percentage formula, examples, and [1,
devcore] bounds unchanged.
- Around line 145-147: Update the AMD vGPU documentation around the cuCount and
HSA_CU_MASK description to present CU range selection as intended behavior
rather than an enforced exclusivity guarantee. Also revise the related wording
near AMDDevices.LockNode and ReleaseNodeLock to avoid claiming node-level
exclusivity until shared-state locking is implemented.
- Around line 39-43: Clarify the WGP-capable device behavior in the design text
by explicitly defining how odd CU counts are handled before constructing
HSA_CU_MASK: state whether the scheduler rounds the count, rejects it, or stores
an adjusted pair-aligned value, and ensure the documented mask-building behavior
follows that decision.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8710261c-2a2f-4936-b4ab-2dd6660c4346
📒 Files selected for processing (1)
docs/develop/amd-vgpu.md
coderabbitai
Bot
added
the
kind/documentation
github-actions
Bot
removed
the
kind/documentation
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/lgtm |
Signed-off-by: Jifei Wang <jifei.wang@dynamia.ai>
Signed-off-by: Jifei Wang <jifei.wang@dynamia.ai>
Signed-off-by: Jifei Wang <jifei.wang@dynamia.ai>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, FouoF The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
What type of PR is this?
/kind design
What this PR does / why we need it:
This pr update the design docs in following points:
major changes:
HSA_CU_MASK supports per GPU CU mask which is essential for multi-gpu tasks. As tasks request different num of vGPU will create a cross using for GPUs. For example, task1 request 25% one gpu, it get gpu 0 CU 0-76. Then task2 requests 25%
but 2 GPUs, it can not use gpu 1 CU 0-76 though they are ideal for gpu 0 is allocated.
This design is intended to align with other device apis and it also guarantee the consistency in metrics. Besides, CU total count may not exposed to the user.
HAMi api only allow same gpumem allocation for all request devices. So one env for all devices can satisfy it.
Device related logic better place in device plugin.
minor changes:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Summary by CodeRabbit
amd.com/gpumem, reflected asHIP_DEVICE_MEMORY_LIMIT=<MiB>m) and compute-unit partitioning as an inclusive 1–100% (amd.com/gpucores).ROCR_VISIBLE_DEVICESordering andHSA_CU_MASKexpressed asGPU_list:CU_listfor non-overlapping CU ranges.