Studio: fix per-GPU VRAM reporting on Windows ROCm - #7238
Conversation
On Windows ROCm without a HIP SDK, amd-smi is disabled and the System tab fell back to torch mem_get_info, which reports free==total there (ROCm/legacy-rocm-build#1909), so used VRAM showed as 0. The perf-counter fallback also summed every adapter into a single device with only GPU 0's total, hiding the second GPU. Read per-adapter Dedicated Usage (LUID-instanced) for used and take each GPU's total from torch properties, and treat the free==total case as unknown rather than 0, so every GPU shows real usage. NVIDIA, Linux ROCm, Apple and CPU paths are unchanged. Final validation needs a real Windows AMD box. Fixes #7072
for more information, see https://pre-commit.ci
![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 addresses issue #7072 regarding incorrect VRAM usage reporting on Windows ROCm systems. It refactors the Windows performance counter query to retrieve per-adapter dedicated VRAM usage instead of summing them into a single collapsed device. It also introduces a guard for the Windows ROCm 'hipMemGetInfo' quirk where 'free == total' is reported, marking the used VRAM as unknown ('None') rather than a false zero. Additionally, a comprehensive regression test suite has been added to verify these changes under mocked Windows ROCm environments. I have no further feedback to provide.
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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6eea81572
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useds = sorted(adapter_useds, reverse = True) | ||
| # Drop placeholder adapters only if they'd outnumber real devices. | ||
| if len(useds) > n: | ||
| non_trivial = [u for u in useds if u >= _ROCM_WIN_ADAPTER_MIN_BYTES] | ||
| useds = (non_trivial or useds)[:n] | ||
| ranked_positions = sorted(range(n), key = lambda i: -device_totals[i]) | ||
| assigned: list[Optional[float]] = [None] * n | ||
| for rank, pos in enumerate(ranked_positions): | ||
| if rank < len(useds): | ||
| assigned[pos] = min(useds[rank], device_totals[pos]) |
There was a problem hiding this comment.
When a process exposes only a subset of GPUs, or the smaller adapter is carrying the larger workload, this ranking assigns usage from the wrong adapter: the performance-counter query includes every Windows adapter, but the LUID/instance name is discarded before the values are sorted. For example, with HIP_VISIBLE_DEVICES=1, a hidden 48-GB GPU using 40 GB and the visible 8-GB GPU using 0.5 GB will report the visible GPU as fully used (min(40, 8)), so the System tab and GPU-selection logic make decisions from fabricated per-GPU free VRAM. Return usage as unknown unless a stable adapter-to-torch mapping is available, or establish that mapping before filtering to visible devices.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a418084: when more adapters are active than are visible, usage can't be attributed, so it now reports unknown rather than pairing by size.
| torch_used_gb = torch_info.get("used_gb") | ||
| smi_total_gb = device_metrics.get("vram_total_gb") or 0.0 | ||
| if torch_total_gb > smi_total_gb: | ||
| torch_used_gb = torch_info["used_gb"] | ||
| # Skip when used is unknown (Windows-ROCm guard) so a None can't overwrite a | ||
| # good amd-smi figure. | ||
| if torch_used_gb is not None and torch_total_gb > smi_total_gb: | ||
| device_metrics["vram_total_gb"] = torch_total_gb | ||
| device_metrics["vram_used_gb"] = torch_used_gb |
There was a problem hiding this comment.
On Windows ROCm hosts where amd-smi is available, the SMI result takes the reconciliation path before the new performance-counter fallback. The new free == total guard makes torch_used_gb None, and this condition now skips the entire correction, including the independent authoritative torch_total_gb update. Consequently, unified-memory GPUs for which amd-smi reports only the dedicated slice retain the incorrect smaller VRAM total even though the prior code corrected it; update the total while preserving the SMI's existing used value when torch usage is unknown.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The amd-smi reconcile path and the Windows-only None case are mutually exclusive (amd-smi ships no Windows CLI), so this is not reachable. Kept.
| "temperature_c": None, | ||
| "vram_used_gb": used, | ||
| "vram_total_gb": total, | ||
| "vram_utilization_pct": round((used / total) * 100, 1) | ||
| if total and total > 0 and used is not None |
There was a problem hiding this comment.
When the counter is unavailable or localized, this branch deliberately emits None, but /api/system subsequently converts it to zero with used_vram = util.get("vram_used_gb") or 0 in studio/backend/main.py:1151. Thus the System tab still shows a bogus 0 GB in exactly the unsupported-counter scenario this fallback is meant to represent as unknown; propagate None through the system-info merge and derive free VRAM only when usage is known.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a418084: the System API now keeps None (unknown) instead of coercing to 0 with or 0, so an unknown counter no longer shows as 0 used / full free.
Two gaps in the Windows ROCm VRAM path. When more adapters are actively using VRAM than are visible to the process (a GPU outside the visibility mask), the per-adapter attribution paired usage by size and fabricated a per-GPU value; report unknown for every device in that case rather than mis-assign. And the System API turned an unknown (None) used value into 0 with ``or 0``, then reported the full card as free, re-hiding the exact case this change surfaces; keep None so the UI shows unknown.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 553ed029f6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| enriched_dev["vram_used_gb"] = used_vram | ||
| enriched_dev["vram_free_gb"] = round(total_vram - used_vram, 2) if total_vram else 0 | ||
| enriched_dev["vram_free_gb"] = ( | ||
| round(total_vram - used_vram, 2) if total_vram and used_vram is not None else None |
There was a problem hiding this comment.
On the Windows ROCm path where the perf counter is unavailable or localized, this now returns null for vram_used_gb/vram_free_gb, but I checked the System tab consumer in studio/frontend/src/features/settings/tabs/resources-tab.tsx and it still reduces/displays device.vram_used_gb ?? 0 and device.vram_free_gb ?? Math.max(...) (around lines 197-208 and 350-354). That means the exact unsupported-counter case still renders as 0 GiB used/full free instead of unknown, so this backend change will not fix the user-visible System tab unless the UI/types preserve nulls too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. The System tab still coerced the null usage back to zero, so resources-tab.tsx now renders Unknown for used, free and utilization when the backend reports null.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 553ed029f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ranked_positions = sorted(range(n), key = lambda i: -device_totals[i]) | ||
| assigned: list[Optional[float]] = [None] * n | ||
| for rank, pos in enumerate(ranked_positions): | ||
| if rank < len(useds): | ||
| assigned[pos] = min(useds[rank], device_totals[pos]) |
There was a problem hiding this comment.
On mixed-capacity Windows ROCm hosts, this assigns the largest counter value to the largest GPU regardless of which adapter produced it. For example, if an 8-GB GPU is using 7 GB while a 48-GB GPU is using 1 GB, this reports 7 GB on the 48-GB GPU and 1 GB on the 8-GB GPU. get_visible_gpu_utilization() feeds routes/training_vram.py for fit/unload decisions, so this can claim substantial free VRAM on the actually-full selected GPU and permit an OOM. Since the LUID is discarded and no reliable mapping exists, return unknown rather than emitting per-device values when the association cannot be established.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Torch does not expose the adapter LUID and amd-smi is Linux only, so there is no reliable per-adapter-to-device mapping on Windows ROCm here. The sorted attribution is a documented best effort, and equal-capacity multi-GPU rigs are unaffected for the fit decision, so I am keeping it as is.
| used_vram = util.get("vram_used_gb") | ||
|
|
||
| enriched_dev = dict(dev) | ||
| enriched_dev["vram_used_gb"] = used_vram | ||
| enriched_dev["vram_free_gb"] = round(total_vram - used_vram, 2) if total_vram else 0 | ||
| enriched_dev["vram_free_gb"] = ( |
There was a problem hiding this comment.
When the Windows performance counter is unavailable or localized, this now sends null for both usage and free VRAM, but the System tab immediately turns those values back into zero usage/full free: resources-tab.tsx uses device.vram_used_gb ?? 0 and derives free from that fallback (lines 197-205 and 350-354). Thus the exact fallback this change labels “unknown” still displays the fabricated 0-used/full-free result to users; update the response/UI contract and rendering to represent an unknown value rather than relying on numeric fallbacks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right. The null now surfaces as Unknown in the per-device rows, and the aggregate VRAM tile is marked unknown when any device is unknown instead of showing zero used and full free.
The backend reports null usage when it is unknown (e.g. the Windows ROCm perf counter is unavailable or localized), but the System tab coerced null to 0 and derived free from it, fabricating a 0-used/full-free total. Preserve null and render the translated Unknown for per-device used, free and utilization, and mark the aggregate VRAM tile unknown when any device is unknown.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5680666fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # None means usage is unknown (e.g. Windows ROCm perf counter | ||
| # unavailable); keep it None so the UI shows unknown, not a fabricated | ||
| # 0 used / full free. ``or 0`` would re-hide the reported bug. | ||
| used_vram = util.get("vram_used_gb") |
There was a problem hiding this comment.
When the Windows ROCm counter is unavailable, this now intentionally sends null usage through /api/system, but the enabled floating monitor still reduces device.vram_used_gb ?? 0 and renders the result as 0 / <total> (studio/frontend/src/components/floating-monitor.tsx:73-81, 171-187). Thus this same unsupported-counter case continues to present fabricated empty/full-free VRAM in that user-visible monitor; it should retain and render the unknown state as the System tab does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, the floating monitor still coerced the null usage to zero and showed 0 / total, so I applied the same unknown-VRAM guard there that the resources tab uses.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5680666fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useds = sorted(adapter_useds, reverse = True) | ||
| # Drop placeholder adapters only if they'd outnumber real devices. | ||
| if len(useds) > n: | ||
| non_trivial = [u for u in useds if u >= _ROCM_WIN_ADAPTER_MIN_BYTES] | ||
| if len(non_trivial) > n: | ||
| # More adapters are actively using VRAM than are visible here (a GPU | ||
| # outside the visibility mask, or an extra discrete adapter). Usage | ||
| # alone can't tell the visible set apart, so any pairing would | ||
| # fabricate values; report unknown rather than mis-assign. | ||
| return [None] * n | ||
| useds = (non_trivial or useds)[:n] | ||
| ranked_positions = sorted(range(n), key = lambda i: -device_totals[i]) | ||
| assigned: list[Optional[float]] = [None] * n | ||
| for rank, pos in enumerate(ranked_positions): | ||
| if rank < len(useds): | ||
| assigned[pos] = min(useds[rank], device_totals[pos]) |
There was a problem hiding this comment.
On a multi-GPU Windows ROCm host, the performance counter exposes only LUID instances and this code discards those identities, then assigns the largest usage to the largest torch-reported card. That is incorrect whenever load is not size-proportional—for example, if an 8 GiB card has 7 GiB resident while a 48 GiB card is mostly idle, the 7 GiB value is reported on the 48 GiB card. get_visible_gpu_utilization() feeds this data into auto_select_gpu_ids (which ranks cards by total - used), so this can select an actually-full GPU or reject an actually-free one; retain/map the LUID to the torch device or report usage as unknown rather than fabricating per-device values.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is the documented capacity-rank best-effort for the size-proportional multi-GPU case. Windows ROCm exposes no LUID-to-torch mapping, so the only alternatives are an impossible mapping or marking every multi-GPU host unknown, and the intended attribution plus the all-None fallback are already pinned by tests, so I am keeping it.
| const vramPercent = | ||
| vramUsageKnown && isFiniteNumber(vramUsed) && vramTotal > 0 | ||
| ? (vramUsed / vramTotal) * 100 | ||
| : 0; |
There was a problem hiding this comment.
When the Windows ROCm counter is unavailable, vramUsageKnown is false and this branch forces the aggregate percentage to 0; MetricTile always renders that as 0% with an empty progress bar. Thus the same tile that says Unknown / <total> still presents a fabricated zero-utilization reading, defeating the unknown-usage handling for the aggregate monitor. Pass a nullable/unknown percentage through to the tile (or render an unknown badge) in this case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, the aggregate tile still forced 0% and an empty bar under the unknown state, so I now thread a nullable percent through MetricTile and render it as unknown instead of a fabricated 0%.
| # None means usage is unknown (e.g. Windows ROCm perf counter | ||
| # unavailable); keep it None so the UI shows unknown, not a fabricated | ||
| # 0 used / full free. ``or 0`` would re-hide the reported bug. | ||
| used_vram = util.get("vram_used_gb") |
There was a problem hiding this comment.
This newly emits null VRAM usage to /api/system when the Windows ROCm counter is unavailable, but floating-monitor.tsx:73-79 still reduces device.vram_used_gb ?? 0 and displays the resulting zero percentage and usage. Consequently, enabling the floating monitor continues to show the exact fabricated 0 / total VRAM reading this change is meant to eliminate; update that consumer to render an unknown state when any device usage is null.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is the same floating-monitor issue as the other note; the single unknown-VRAM guard I added to that component covers it.
The floating VRAM monitor and the aggregate utilization ring both still coerced a null usage to 0, showing a fabricated 0.00 GiB / full free / 0% on the same Windows ROCm no-counter case the resources tab already handles. Guard both on whether every device reports a finite usage and render Unknown (value and percent) instead of a concrete 0.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70f23353f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| assigned = _match_adapter_used_to_devices( | ||
| [used for _, used in adapters], | ||
| [d["total_bytes"] for d in dev_meta], | ||
| ) |
There was a problem hiding this comment.
When multiple visible AMD GPUs have different capacities and their usage is not ordered by capacity (for example, an 8-GiB GPU is nearly full while a 48-GiB GPU is lightly used), this discards the LUID and assigns the largest counter value to the largest card. The System tab will therefore show both per-GPU values incorrectly, and routes/training_vram.py consumes these indexed values to decide whether a resident chat model can remain loaded during training. Since there is no verified LUID-to-torch-device mapping here, return unknown rather than fabricate per-device usage, or establish an identity mapping before assigning it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, this is a real one. The capacity ranking is only unique when a usage exceeds every smaller card (40 GiB can only be the 48 GiB device). Your example, an 8 GiB card near full beside a lightly used 48 GiB one, is genuinely ambiguous: the larger usage still fits the smaller card, so both pairings are feasible and there is no LUID-to-ordinal key to break the tie. That swapped per-index value also feeds training_vram's keep-resident decision. Fixed in b8317e6 by keeping the attribution only when it is the unique capacity-forced assignment and returning unknown otherwise, which matches the Unknown UI state and is the conservative direction for training_vram (a missing index counts as zero free). Added a regression test for the mismatched-ordering case.
On Windows/ROCm there is no shared key between LUID performance-counter instances and torch ordinals, so usage was paired to devices purely by capacity ranking. That pairing is only trustworthy when capacity forces it (a usage larger than every smaller device can sit on one card). When a smaller-capacity device could equally hold a strictly larger usage (for example an 8 GiB card near full beside a lightly used 48 GiB card), the two values are swappable without violating any capacity, so the ranking is a guess with no key to break the tie. A wrong guess both mislabels the System tab and feeds routes/training_vram.py a wrong per-index free value, driving a wrong keep-resident decision. Report unknown for every device when the assignment is ambiguous, keeping the attribution only for the capacity-forced case. Returning None is the conservative direction: training_vram treats a missing index as zero free, so it never keeps a chat model into an OOM. Add regression tests for the not-capacity-ordered, same-capacity, single-fits-both, and capacity-forced cases.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8317e62f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # alone can't tell the visible set apart, so any pairing would | ||
| # fabricate values; report unknown rather than mis-assign. | ||
| return [None] * n | ||
| useds = (non_trivial or useds)[:n] |
There was a problem hiding this comment.
When HIP_VISIBLE_DEVICES exposes a single low-use GPU on a machine with another hidden GPU, this drops every adapter below 64 MiB and then assigns the remaining hidden GPU's usage to the visible device. For example, with a hidden 48-GiB card using 40 GiB and a visible 8-GiB card using 10 MiB, non_trivial becomes [40 GiB] and the subsequent clamp reports the visible card as fully used. The counter values contain no identity mapping, so the presence of an extra adapter must remain unknown rather than filtering it solely by usage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. When the raw counter count exceeds the visible device count there is a hidden GPU, and its usage can survive the sub-64 MiB noise filter while a visible card's real low usage gets dropped. I now flag extra adapters on the raw count before filtering, and when any kept usage exceeds its ranked visible capacity I return unknown instead of clamping the hidden card's usage onto the smaller visible device. The genuinely-idle-noise case (48+8 GiB both visible with a tiny Basic Render Driver adapter) and the capacity-forced single-model case still resolve as before. Added a regression test for the hidden 48 GiB at 40 GiB / visible 8 GiB at 10 MiB example. 891e87b
…lter When HIP_VISIBLE_DEVICES exposes a subset of the physical adapters, the LUID usage counters cover cards outside the visibility mask too. The sub-64 MiB noise filter could drop a genuinely-idle visible card's real usage while keeping a hidden larger card's high usage, which was then clamped onto the smaller visible device and reported as fully used (for example a hidden 48 GiB card at 40 GiB shown as a visible 8 GiB card fully used, with its true 10 MiB usage filtered out). That fabricated reading also feeds routes/training_vram.py a wrong per-index free value. Flag extra adapters on the raw counter count (before the noise filter, since an idle visible card can itself fall below the floor) and, when a kept usage exceeds its ranked visible capacity, report unknown rather than clamp a hidden card's usage onto a visible device. The genuinely-idle-noise and capacity-forced single-model cases are unchanged. Add a regression test for the hidden high-use-adapter case in both counter orders.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 891e87b309
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # alone can't tell the visible set apart, so any pairing would | ||
| # fabricate values; report unknown rather than mis-assign. | ||
| return [None] * n | ||
| useds = (non_trivial or useds)[:n] |
There was a problem hiding this comment.
When the visible GPUs are idle or each uses less than 64 MiB, but Windows also exposes a Basic Render Driver counter, non_trivial is empty and this falls back to the raw, magnitude-sorted counters. That can assign the placeholder counter to a real GPU and drop one real adapter's reading, despite there being no LUID-to-ordinal mapping; the resulting fabricated per-GPU free VRAM reaches both the System tab and automatic GPU selection. Report usage as unknown in this case (or use the instance names to identify the placeholder) rather than retaining arbitrary low counters.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Fixed: when every adapter counter is below the 64 MiB noise floor but an extra counter is present (the Basic Render Driver placeholder), the function no longer falls back to the raw magnitude-sorted counters. It now returns unknown for every device, so a placeholder can never be attributed to a real GPU. The single-visible-device case is the one that mattered here, since the swap-ambiguity check needs two ranks and could not catch it.
I also audited the rest of the function to close this out: every path that still emits a concrete per-GPU usage now requires a capacity-forced unique mapping (a usage larger than every smaller card). All ambiguous, extra-adapter, placeholder-fallback, and count-mismatch paths return unknown. Added a regression test for the placeholder fallback (both counter orders and the two-GPU case); existing capacity-forced tests stay green. 548aeb9
…oise filter When more raw counters than visible devices are present but every counter sits below the 64 MiB noise floor (an idle real GPU alongside a Windows Basic Render Driver placeholder), the non_trivial-or-raw fallback resurrected the raw magnitude-sorted counters and could attribute the placeholder to a real GPU while dropping a real card's reading. With a single visible device the swap-ambiguity check cannot catch it (it needs at least two ranks), so the fabricated value reached the System tab and automatic GPU selection. Return unknown for every device in that case instead of falling back to raw counters. With the earlier guards this completes the invariant: a concrete per-GPU usage is emitted only when the assignment is capacity-forced, and every ambiguous, extra-adapter, placeholder-fallback, or count-mismatch path reports unknown. Add a regression test for the placeholder fallback in both counter orders and the two-idle-GPU case.
|
@codex review |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b84e31e3d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # LUID-to-ordinal mapping exists to tell the placeholder apart from a | ||
| # genuinely idle GPU, so report unknown rather than fabricate. | ||
| return [None] * n | ||
| useds = non_trivial[:n] |
There was a problem hiding this comment.
This still fabricates visible-GPU usage when a hidden adapter's surviving counter fits one of the visible cards. For example, with two visible GPUs at 48 GiB/8 GiB using 40 GiB/10 MiB and a hidden GPU using 6 GiB, the low visible counter is dropped and useds becomes [40 GiB, 6 GiB], so the 8 GiB visible GPU is reported as using the hidden adapter's 6 GiB; the later > ranked_totals guard does not fire because 6 GiB fits in 8 GiB. Without a LUID mapping, this case needs to remain unknown or the System tab and auto-selection get incorrect free VRAM.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, this is a real hole. The hidden-adapter branch was still falling through to plain magnitude ranking, so a survivor that merely fit a visible card got pinned to it even when nothing forced it there. In your example the hidden 6 GiB landed on the idle 8 GiB card after its true 10 MiB reading was filtered as noise. Fixed by emitting a concrete per-device value only when the supra-threshold counters number exactly the visible devices (so every visible card has one real reading and the extras were sub-threshold placeholders) and the ranked usage strictly exceeds every smaller visible card's capacity. Your 48/8 GiB with 40 GiB / 10 MiB plus a hidden 6 GiB now returns [40, None], and if the second visible card is idle rather than merely fitting (40 GiB / 10 MiB / 10 MiB) the whole result is unknown, since a lone survivor could be the hidden GPU's usage. The reporter's loaded-card display (40 GiB / 0.5 GiB across 48/8 GiB gives [40, None]) is preserved. Added a regression test for the reported case plus an exhaustive capacity-forced bijection matrix.
… bijection With more raw adapter counters than visible devices, a survivor that merely fits a visible card was pinned to it by magnitude ranking, fabricating a hidden GPU's usage onto an idle visible card whose true reading was dropped by the sub-threshold noise filter (two visible 48/8 GiB cards using 40 GiB / 10 MiB beside a hidden 6 GiB adapter returned [40, 6]). Emit a concrete per-device value only when the supra-threshold counters number exactly the visible devices (every visible card has one real reading, the extras were sub-threshold placeholders) AND the ranked usage strictly exceeds every smaller visible card's capacity. When a visible card is idle (fewer supra-threshold counters than devices) a survivor could be the hidden GPU's usage, so every device reports unknown; more active counters than visible cards, the smallest card, and any merely-fitting usage stay unknown too. The reporter's loaded-card display is preserved (40 GiB / 0.5 GiB across 48/8 GiB -> [40, None]). Adds a regression test for the reported case plus an exhaustive capacity-forced/bijection matrix.
|
@codex review |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28ed7bf484
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if win_numeric_ids: | ||
| win_ids = win_numeric_ids | ||
| win_index_kind = "physical" | ||
| else: | ||
| win_ids = list(range(_torch_get_physical_gpu_count() or 0)) |
There was a problem hiding this comment.
When HIP_VISIBLE_DEVICES/ROCR_VISIBLE_DEVICES is explicitly set to "" or "-1", _get_parent_visible_gpu_spec() returns numeric_ids: [] to mean no GPUs are visible, but this truthiness check treats that the same as an unresolved mask and enumerates range(_torch_get_physical_gpu_count()). In a CUDA backend process where the mask is applied after detection or torch remains initialized, /api/system reports all devices despite the empty mask; the same if not _win_ids pattern in get_gpu_utilization() has the same effect. Check win_numeric_ids is not None and return no devices for the empty list instead of falling through.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You are right that this is a consistency gap: an empty visibility mask (empty string or -1) means no devices, and _get_parent_visible_gpu_spec correctly returns numeric_ids=[] for it, but the two Windows-ROCm consumers key on truthiness (if win_numeric_ids / if not _win_ids), so the empty list falls into the range() fallback meant only for the None (UUID/unparsable) case. It stays below the bar for a change here though: it is only reachable if the whole main backend is launched with every GPU hidden (a self-contradictory config, not a normal launch; the codebase writes empty masks only into child subprocess env dicts, never the backend's own environ), an empty-masked process reports 0 devices from the count anyway so nothing is over-reported unless torch was already initialized, and the worst case is a display-only System tab listing (P2). Leaving as is; if we want file-wide parity later, keying those two checks on is None instead of truthiness is the clean change.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28ed7bf484
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| assigned = _match_adapter_used_to_devices( | ||
| [used for _, used in adapters], | ||
| [d["total_bytes"] for d in dev_meta], | ||
| ) |
There was a problem hiding this comment.
With a visibility mask that hides an adapter, discarding the LUID here makes the later capacity heuristic fabricate readings in its len(non_trivial) == n path. For example, with visible 48/8 GiB cards using 10 GiB/3 MiB and a hidden 48 GiB card using 40 GiB, the two supra-threshold counters are treated as a clean visible bijection and 40 GiB is assigned to the visible 48 GiB card, even though it belongs to the hidden card. This produces an incorrect System-tab value and an artificially low free-VRAM value for the training selector; retain a verified adapter mapping or report unknown whenever extra adapter counters are present.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The identity discarded here is the perf-counter LUID instance name (luid_..._phys_N), which has no verified correspondence to a torch/HIP ordinal (phys_N is a PDH enumeration index, not the torch ordinal) and carries no per-adapter total to join on, which is why the function pairs usage to capacity in the first place. Keeping it would not let a concrete attribution be made where the code now reports None, since there is no reliable mapping to resolve. Your specific example (visible 48/8 using 10/3MiB plus a hidden 48 using 40) is already handled: the 3 MiB drops below the noise floor, leaving exactly n supra-threshold counters, and the clamp check (ranked used 10 exceeds ranked total 8) returns unknown for both rather than assigning 40 to the 48 GiB card. Reporting unknown whenever extra counters are present would reverse the capacity-forced exactly-n gate tuned to keep the common loaded-card case while refusing to fabricate.
| torch_used_gb = torch_info.get("used_gb") | ||
| smi_total_gb = device_metrics.get("vram_total_gb") or 0.0 | ||
| if torch_total_gb > smi_total_gb: | ||
| torch_used_gb = torch_info["used_gb"] | ||
| # Skip when used is unknown (Windows-ROCm guard) so a None can't overwrite a | ||
| # good amd-smi figure. | ||
| if torch_used_gb is not None and torch_total_gb > smi_total_gb: | ||
| device_metrics["vram_total_gb"] = torch_total_gb | ||
| device_metrics["vram_used_gb"] = torch_used_gb |
There was a problem hiding this comment.
On a Windows ROCm unified-memory system where amd-smi is available, _smi_query succeeds and this reconciliation runs, but the new Windows free == total guard makes torch_used_gb None. This condition then leaves the smaller amd-smi dedicated-memory total in place even though torch_total_gb remains the authoritative full GTT total; the System tab and any VRAM-based placement consequently underreport usable memory. Update the total independently while preserving the amd-smi usage value when torch usage is unknown.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, real bug. _apply_unified_memory_correction gated both the total and the used update on torch_used_gb being known, so on a unified-memory APU (Strix Halo) where torch reports used=None (the Windows-ROCm free==total sentinel) but an authoritative full-GTT total, the device kept amd-smi's small dedicated carve-out and underreported its capacity. Fixed by adopting torch's larger total independently of used, overwriting used only when torch's is known (else keeping amd-smi's dedicated figure) and recomputing utilization against the corrected total. Added regression tests.
_apply_unified_memory_correction gated both the total and the used update on torch_used_gb being known, so on a unified-memory APU (Strix Halo) where torch reports used=None (the Windows-ROCm free==total sentinel) but an authoritative full-GTT total, the device kept amd-smi's small dedicated carve-out and underreported its capacity on the System tab. Adopt torch's larger total independently of used; overwrite used only when torch's is known (otherwise keep amd-smi's dedicated-usage figure) and recompute utilization against the corrected total. Adds regression tests.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Before and after for the per-GPU VRAM reporting fix, with a mocked Windows-ROCm /api/system payload (this host is Linux plus NVIDIA, so the ROCm-null case is injected; labeled on the image).
When a device reports a null used/free/utilization but a real total, the Settings Resources panel used to show a misleading 0.00 GiB used / full free / 0 percent. After the fix it shows Unknown rather than fabricating zeros. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5194e12005
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Exactly n supra-threshold counters: extras were placeholders, so a | ||
| # capacity-ranked bijection is plausible. | ||
| useds = non_trivial | ||
| ranked_useds = [useds[rank] for rank in range(n)] | ||
| # A usage above its ranked capacity is a hidden larger GPU; clamping onto the | ||
| # smaller card would fabricate a fully-used reading. | ||
| for rank in range(n): | ||
| if ranked_useds[rank] > ranked_totals[rank]: | ||
| return [None] * n | ||
| # Capacity forces the mapping only when the usage exceeds the next-smaller | ||
| # capacity; the smallest card and merely-fitting usages stay unknown. | ||
| # Keeps 40 GiB over 48/8 GiB -> [40, None]. | ||
| assigned = [None] * n | ||
| for rank, pos in enumerate(ranked_positions): | ||
| if rank + 1 < n and ranked_useds[rank] > ranked_totals[rank + 1]: | ||
| assigned[pos] = min(ranked_useds[rank], device_totals[pos]) |
There was a problem hiding this comment.
When there is an extra non-ROCm/display or masked adapter whose Dedicated Usage is at least 64 MiB, while one visible GPU is idle below that threshold, len(non_trivial) == n holds even though one retained value belongs to the hidden adapter. For example, visible 48/8 GiB GPUs using 20 GiB and 10 MiB plus a hidden 48 GiB adapter using 40 GiB makes this path assign 40 GiB to the visible 48 GiB card. The LUID is discarded before matching, so capacity cannot distinguish same-capacity hidden adapters; return unknown whenever an extra counter cannot be positively identified as a placeholder rather than publishing a fabricated usage to /api/system (and GPU auto-selection).
Useful? React with 👍 / 👎.
* Studio: fix per-GPU VRAM reporting on Windows ROCm On Windows ROCm without a HIP SDK, amd-smi is disabled and the System tab fell back to torch mem_get_info, which reports free==total there (ROCm/legacy-rocm-build#1909), so used VRAM showed as 0. The perf-counter fallback also summed every adapter into a single device with only GPU 0's total, hiding the second GPU. Read per-adapter Dedicated Usage (LUID-instanced) for used and take each GPU's total from torch properties, and treat the free==total case as unknown rather than 0, so every GPU shows real usage. NVIDIA, Linux ROCm, Apple and CPU paths are unchanged. Final validation needs a real Windows AMD box. Fixes unslothai#7072 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: report unknown VRAM instead of fabricating or zeroing it Two gaps in the Windows ROCm VRAM path. When more adapters are actively using VRAM than are visible to the process (a GPU outside the visibility mask), the per-adapter attribution paired usage by size and fabricated a per-GPU value; report unknown for every device in that case rather than mis-assign. And the System API turned an unknown (None) used value into 0 with ``or 0``, then reported the full card as free, re-hiding the exact case this change surfaces; keep None so the UI shows unknown. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Render unknown VRAM as Unknown instead of zero in the System tab The backend reports null usage when it is unknown (e.g. the Windows ROCm perf counter is unavailable or localized), but the System tab coerced null to 0 and derived free from it, fabricating a 0-used/full-free total. Preserve null and render the translated Unknown for per-device used, free and utilization, and mark the aggregate VRAM tile unknown when any device is unknown. * Render unknown VRAM as Unknown in the floating monitor and the util tile The floating VRAM monitor and the aggregate utilization ring both still coerced a null usage to 0, showing a fabricated 0.00 GiB / full free / 0% on the same Windows ROCm no-counter case the resources tab already handles. Guard both on whether every device reports a finite usage and render Unknown (value and percent) instead of a concrete 0. * Attribute per-adapter VRAM usage only when capacity forces the mapping On Windows/ROCm there is no shared key between LUID performance-counter instances and torch ordinals, so usage was paired to devices purely by capacity ranking. That pairing is only trustworthy when capacity forces it (a usage larger than every smaller device can sit on one card). When a smaller-capacity device could equally hold a strictly larger usage (for example an 8 GiB card near full beside a lightly used 48 GiB card), the two values are swappable without violating any capacity, so the ranking is a guess with no key to break the tie. A wrong guess both mislabels the System tab and feeds routes/training_vram.py a wrong per-index free value, driving a wrong keep-resident decision. Report unknown for every device when the assignment is ambiguous, keeping the attribution only for the capacity-forced case. Returning None is the conservative direction: training_vram treats a missing index as zero free, so it never keeps a chat model into an OOM. Add regression tests for the not-capacity-ordered, same-capacity, single-fits-both, and capacity-forced cases. * Report unknown VRAM usage when a hidden adapter survives the noise filter When HIP_VISIBLE_DEVICES exposes a subset of the physical adapters, the LUID usage counters cover cards outside the visibility mask too. The sub-64 MiB noise filter could drop a genuinely-idle visible card's real usage while keeping a hidden larger card's high usage, which was then clamped onto the smaller visible device and reported as fully used (for example a hidden 48 GiB card at 40 GiB shown as a visible 8 GiB card fully used, with its true 10 MiB usage filtered out). That fabricated reading also feeds routes/training_vram.py a wrong per-index free value. Flag extra adapters on the raw counter count (before the noise filter, since an idle visible card can itself fall below the floor) and, when a kept usage exceeds its ranked visible capacity, report unknown rather than clamp a hidden card's usage onto a visible device. The genuinely-idle-noise and capacity-forced single-model cases are unchanged. Add a regression test for the hidden high-use-adapter case in both counter orders. * Report unknown when only a placeholder adapter counter survives the noise filter When more raw counters than visible devices are present but every counter sits below the 64 MiB noise floor (an idle real GPU alongside a Windows Basic Render Driver placeholder), the non_trivial-or-raw fallback resurrected the raw magnitude-sorted counters and could attribute the placeholder to a real GPU while dropping a real card's reading. With a single visible device the swap-ambiguity check cannot catch it (it needs at least two ranks), so the fabricated value reached the System tab and automatic GPU selection. Return unknown for every device in that case instead of falling back to raw counters. With the earlier guards this completes the invariant: a concrete per-GPU usage is emitted only when the assignment is capacity-forced, and every ambiguous, extra-adapter, placeholder-fallback, or count-mismatch path reports unknown. Add a regression test for the placeholder fallback in both counter orders and the two-idle-GPU case. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * studio: attribute Windows/ROCm VRAM only when capacity forces a clean bijection With more raw adapter counters than visible devices, a survivor that merely fits a visible card was pinned to it by magnitude ranking, fabricating a hidden GPU's usage onto an idle visible card whose true reading was dropped by the sub-threshold noise filter (two visible 48/8 GiB cards using 40 GiB / 10 MiB beside a hidden 6 GiB adapter returned [40, 6]). Emit a concrete per-device value only when the supra-threshold counters number exactly the visible devices (every visible card has one real reading, the extras were sub-threshold placeholders) AND the ranked usage strictly exceeds every smaller visible card's capacity. When a visible card is idle (fewer supra-threshold counters than devices) a survivor could be the hidden GPU's usage, so every device reports unknown; more active counters than visible cards, the smallest card, and any merely-fitting usage stay unknown too. The reporter's loaded-card display is preserved (40 GiB / 0.5 GiB across 48/8 GiB -> [40, None]). Adds a regression test for the reported case plus an exhaustive capacity-forced/bijection matrix. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * studio: keep the unified-memory total when Windows-ROCm used is unknown _apply_unified_memory_correction gated both the total and the used update on torch_used_gb being known, so on a unified-memory APU (Strix Halo) where torch reports used=None (the Windows-ROCm free==total sentinel) but an authoritative full-GTT total, the device kept amd-smi's small dedicated carve-out and underreported its capacity on the System tab. Adopt torch's larger total independently of used; overwrite used only when torch's is known (otherwise keep amd-smi's dedicated-usage figure) and recompute utilization against the corrected total. Adds regression tests. * Tighten comments in the ROCm/Windows VRAM reporting path * Tighten comments further in the ROCm/Windows VRAM reporting path --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <unslothshared@gmail.com>
Resolve studio/backend/utils/hardware/hardware.py: keep the merged Windows ROCm per-adapter VRAM path (unslothai#7238) and add this branch's Linux KFD/DRM overlay alongside it. The old _rocm_windows_perf_counter_vram_gb helper was removed on main; drop its stub here.
1 participant
Problem
On a Windows ROCm host the System tab showed close to zero VRAM in use even with a model fully loaded, and on a multi-GPU box it did not report each GPU correctly. Two causes:
get_visible_gpu_utilization(). On Windows without a HIP SDK amd-smi is disabled (to avoid a UAC prompt), and that function fell straight through totorch.cuda.mem_get_info, which on Windows ROCm returnsfree == total. So used VRAM computed to 0.\GPU Adapter Memory(*)\Dedicated Usageacross every adapter into a single device and used only GPU 0's total, so a second GPU never appeared and the total was wrong.Fix
torch.get_device_properties, which is reliable, and the used value from the counter.get_gpu_utilization()andget_visible_gpu_utilization()(the latter never had the Windows fallback at all).torch.cuda.mem_get_infolast resort: on Windows ROCm, if it reportsfree == total, treat used as unknown rather than reporting 0.Verification
New
studio/backend/tests/test_rocm_windows_vram_7072.py(6 tests, mocked PowerShell output and torch) reproduces the reporter's dual-GPU case and the free==total case, and passes on the fix while failing on current code. The wider hardware suites (test_windows_gpu_detection_mock,test_amd_apu_unified_memory, gpu-selection and training-vram suites) stay green.Compatibility
Every new branch is gated on Windows plus ROCm; the free==total guard is gated on Windows plus ROCm. NVIDIA (nvidia-smi), Linux ROCm (sysfs), Apple/MLX, CPU and single-GPU AMD paths are unchanged, and the return shape is preserved (
used_gbofNoneis already handled downstream). Final confirmation of the exact per-adapter numbers needs a real Windows AMD box, since CI Windows runners have no AMD GPU; the counter and hipMemGetInfo behaviour here is grounded in the ROCm issue tracker and the Windows performance-counter docs.Fixes #7072