diff --git a/.agents/specs/rocm-device-fit-bounded-memory.md b/.agents/specs/rocm-device-fit-bounded-memory.md new file mode 100644 index 000000000..a0b182561 --- /dev/null +++ b/.agents/specs/rocm-device-fit-bounded-memory.md @@ -0,0 +1,284 @@ +# BACKEND-ROCM-DEVICE-FIT-BOUNDED-MEMORY — make the #1123/#1870 refusal reachable on ROCm, without moving GDN kernel defaults + +Issue: [#1934](https://github.com/mudler/vllm.cpp/issues/1934). +Related: [#1870](https://github.com/mudler/vllm.cpp/issues/1870) (stays open; +this row is its actual remaining blocker), [#1928](https://github.com/mudler/vllm.cpp/issues/1928) +(unrelated ROCm gap, not touched here). +Base: `2a42cb369` (`upstream/main` at the claim). + +## Scope + +`CheckDeviceWeightFit`'s one production call site (`model_loader.cpp`) reads +`target.needs_weight_staging()` to decide whether the load-time device-fit +refusal (#1123, and the arithmetic fix #1870/#1935 made to it) runs at all. +`RocmPlatform::needs_weight_staging()` hardcodes `false` — a W0-era comment +says a discrete AMD card "will eventually answer true... Revisit at M2", never +revisited despite ROCm landing grouped-GEMM, MoE combine/gate, ROCM_ATTN and +hipGraph decode capture since. Measured directly on this box (gfx1200): +`VT_DEVICE_WEIGHT_BUDGET_BYTES=1` against a real checkpoint produced no +refusal at all, because `CheckDeviceWeightFit` returns before computing +anything when its `needs_weight_staging` argument is false. Meanwhile the +actual device allocation the refusal exists to guard — `d.b.Alloc(nb)` in +`qwen3_5.cpp`'s `ResidentWeight` — is NOT gated by that flag; it runs +unconditionally on any non-CPU platform (since issue #125's fix keyed the +alias-vs-upload branch on `is_cpu()`, not `needs_weight_staging()`). So #1870's +`hipMalloc: out of memory` stays reachable regardless of the arithmetic fix. + +IN SCOPE: + +- Make the device-fit refusal reachable on ROCm: a real, once-probed + `ResidencyPolicy::device_memory_total_bytes` and a way for the ONE call site + to know a budget check applies here. +- Do it WITHOUT flipping `needs_weight_staging()` itself, because that flag is + documented (`interface.h`) to also gate the indexed/merged/packed GDN + kernel-dispatch defaults, at least one of which (`MergedGdnBaEnabled` / + `MergedGdnQkvzEnabled` / the packed-decode and fp8-resident-prep gates) has + NO op-registration fallback — it would newly activate unconditionally on + ROCm the moment the flag moves, exactly the "assume kernels a device might + not have" failure `IndexedGdnStateIoEnabled` was written to avoid for the + ONE consumer that already checks op registration. Auditing and (where + needed) porting each of those kernel paths for ROCm correctness is a + separate, larger undertaking than this row. + +OUT OF SCOPE: + +- `needs_weight_staging()` itself, and every one of its other consumers + (`DirectDeviceLoadEligible`, `IndexedGdnStateIoEnabled`, `MergedGdnBaEnabled`, + `MergedGdnQkvzEnabled`, `PackedGdnDecodeEligibility`'s eligibility struct, + `PrepareGdnFp8Resident`, `PrepareBf16Resident`). None of their behavior moves + in this row, and the tests prove it. +- `ResidencyPolicy::release_host_weights_after_upload` and + `uses_device_memory_pool` for ROCm. CUDA sets both `true` for its own + Marlin/DevicePool reasons; this row leaves ROCm's answer at the existing + default (`false`) because those are separate, unmeasured policy questions, + not a side effect of making the budget check reachable. +- The `kMoeGroupedGemmBf16` ROCm provider gap (#1928). Unrelated to this row. + +## Upstream chain + +No upstream vLLM mirror for the new predicate — `Platform::needs_weight_staging()` +itself has none (it is a vllm.cpp-original policy seam, per its own doc), and +this row adds a narrower sibling to it for the same reason. The +`device_memory_total_bytes` probe mirrors `platforms/cuda.cpp`'s own +`cudaMemGetInfo` call at registration, HIP's exact analog. + +## Design + +**A new `Platform` method, not a flipped flag.** `Platform::allocates_bounded_device_memory()` +(`interface.h`) answers the narrow physical question `CheckDeviceWeightFit` +actually needs: does a load here draw from a bounded, budget-checkable device +memory pool? Default implementation delegates to `needs_weight_staging()` — +byte-identical for CUDA (already true) and every platform that overrides +neither method (still false). `RocmPlatform` overrides ONLY this new method to +`true`, leaving `needs_weight_staging()` at its existing `false`. + +`model_loader.cpp`'s ONE `CheckDeviceWeightFit` call site reads +`target.allocates_bounded_device_memory()` instead of +`target.needs_weight_staging()`. The OTHER `needs_weight_staging()` read in the +same function (the streamed-expert-lane condition, a few lines above) is +UNTOUCHED — that one genuinely asks "is the fully-optimized device-resident +forward what's running", which `needs_weight_staging()` still correctly +answers. + +**Why not just flip `needs_weight_staging()`.** Read literally: +`IndexedGdnStateIoEnabled` (`qwen3_5.cpp`) ALREADY takes ROCm's fast arm today, +independent of `needs_weight_staging()` — its `!needs_weight_staging()` branch +falls through to `IndexedGdnOpsNative(device)` for any non-CPU platform, +checking `kCausalConv1dUpdate`/`kGdnDecode`/`kGdnStateGather`/`kGdnStateScatter` +registration directly (all four ARE registered for ROCm — confirmed via +`rocm_ops.hip`). Flipping `needs_weight_staging()` would make this consumer +take the OTHER branch (`return indexed == nullptr || indexed[0] != '0';`), +which DROPS the op-registration check and assumes the ops exist — a +regression, not an improvement, for this one consumer. `MergedGdnBaEnabled` +and its siblings have no such fallback at all: they are a bare +`enabled && needs_weight_staging()`, so flipping the flag activates them on +ROCm unconditionally, and whether the underlying merged/packed GEMM and +fp8-resident-prep paths are numerically correct there is unverified. A new, +narrow predicate sidesteps all of it. + +**The probe.** `RocmPlatform` (`rocm.cpp`) is deliberately HIP-header-free — +compiled as a stub object even in non-HIP builds, reaching the device only +through the `vt::rocm::` free-function seam (`rocm_runtime.h`), the same +discipline `DeviceAvailable()`/`HostMemoryIsDeviceAddressable()` already use. +A NEW free function, `vt::rocm::DeviceMemoryTotalBytes(int index)`, declared +there and implemented in `rocm_backend.hip` (`hipSetDevice` + `hipMemGetInfo`, +mirroring `Backend::DeviceMemoryInfo`'s existing body without calling it — +that method is a LIVE per-request probe with its own single consumer, +`Gemma4MoE`'s expert LRU, and reusing it for `ResidencyPolicy` would be the +exact "wake a currently-dead-on-CUDA seam" scope-widening `interface.h`'s own +comment on `device_memory_total_bytes` names as issue #1126's territory — a +DIFFERENT platform's DIFFERENT concern, not this row's). The platform +registrar calls it ONCE, device 0, and threads the result through a new +`RocmPlatform` constructor parameter into a stored member, exactly mirroring +`CudaPlatform`'s own constructor-injected `device_memory_total_bytes_`. + +`RocmPlatform::residency_policy()` returns `{device_memory_total_bytes = }` +with every other field at its existing default (`false`/`0`) — see Out of +Scope. + +## Risks and decisions + +- **The new predicate's name.** `allocates_bounded_device_memory()` was chosen + over reusing `needs_weight_staging` under a different value, or deriving the + gate implicitly from `residency_policy().device_memory_total_bytes != 0` + alone. An explicit, documented predicate matches this codebase's own + established style (`is_integrated_gpu`/`is_unified_memory`/ + `host_memory_is_device_addressable`/`needs_weight_staging` are all + deliberately separate, cross-referenced concepts with a paragraph each + explaining why they are not one another) rather than an implicit + budget-nonzero proxy a future reader would have to reverse-engineer. +- **Two predicates that look related but move independently is exactly the + shape issue #125 already burned this codebase on once** + (`needs_weight_staging()` vs. `is_cpu()` for `ResidentWeight`'s alias-vs-upload + branch). This row adds a THIRD closely-related predicate rather than fixing + the second one's scope, which could read as compounding the confusion. Argued + against a merge: `needs_weight_staging()` and `allocates_bounded_device_memory()` + answer genuinely different questions (device-resident FORWARD KERNELS vs. a + BUDGET GUARD on ordinary weight upload) that happen to coincide on CUDA and + deliberately diverge on ROCm — the tests pin the divergence rather than + asserting it away. +- **`vt::rocm::DeviceMemoryTotalBytes` vs. `Backend::DeviceMemoryInfo`.** + Considered reusing the existing backend method directly from the platform + registrar via `vt::GetBackend(DeviceType::kROCM).DeviceMemoryInfo(...)` and + rejected: static-init order across translation units is unspecified (the + registrar's own comment states this as the reason it probes the device + itself rather than trusting `RocmBackend`'s registrar to have already run), + so a call through `GetBackend` at platform-registration time risks reading + an unregistered backend. The free-function seam sidesteps the ordering + question entirely, the same way `DeviceAvailable()` already does. +- **CPU cost of a `hipSetDevice`+`hipMemGetInfo` call at every process start + on a ROCm build.** Bounded: it runs exactly once, in the platform registrar, + which already makes comparable device-attribute probe calls + (`ProbeDevice`'s own `hipGetDeviceProperties`-class calls) at the same point. + +## Tests + +**`tests/vllm/platforms/test_platform.cpp`**: + +- CPU and the existing `FakeCapabilityPlatform` (`sm121`) cases gain a + `CHECK(x.allocates_bounded_device_memory() == x.needs_weight_staging())` + pin, proving the default delegation is a true no-op for every platform that + does not override the new method — red before this row (the method does not + exist), trivially green after. +- A new fake platform, `FakeBoundedNonStagingPlatform`, constructing ROCm's + own real shape (`needs_weight_staging()=false`, + `allocates_bounded_device_memory()=true`) on the CPU tier — proves the two + predicates are independently settable, mirroring the existing + `FakeUnifiedAddressablePlatform` pattern this file already uses for + `host_memory_is_device_addressable()` vs. its neighbours. +- A new HIP-build-only case, skipped via `if (!HasPlatform(DeviceType::kROCM)) return;` + (matching the existing CUDA-only case's pattern), asserting on THIS box's + real `RocmPlatform`: `needs_weight_staging()` still false, + `allocates_bounded_device_memory()` true, `residency_policy().device_memory_total_bytes > 0` + (the probe actually ran), and the two policy fields this row deliberately + left alone still read false. + +**`tests/vllm/entrypoints/test_gguf_device_fit_reach.cpp`**: `StagingPlatform` +gains two independently-settable flags +(`needs_weight_staging_flag`, `allocates_bounded_device_memory_flag`, +previously the class hardcoded `needs_weight_staging()` to `true` and had no +override for the new method). Two new cases: one with +`(false, true)` — ROCm's exact real state — proving the refusal STILL fires; +one with `(true, false)` proving it does NOT (the call site reads the new +predicate, not the old one, in both directions). Every EXISTING case in this +file is unaffected: the default-constructed flags (`true`, `true`) reproduce +today's behavior byte for byte because the new method's default delegates to +the old one, which the class already overrides to `true`. + +## Gates + +- `./build-hip/tests/test_platform` (ROCm build, this box, gfx1200) — the + HIP-only case above is the one that can only run here +- `./build-hip/tests/test_gguf_device_fit_reach` +- `./build-nix-cpu/tests/test_platform` / `test_gguf_device_fit_reach` (CPU + build, same box) — every other case +- A real hardware repro: with this row, `VT_DEVICE_WEIGHT_BUDGET_BYTES=1` + against a real checkpoint on this box's gfx1200 card refuses (it did not, + before this row) — the exact measurement #1934 reports as missing. +- `scripts/agent-preflight.sh --staged` (against the true fork point, not + `origin/main` — see `.agents/developer-preferences.md`) + +## Evidence + +Recorded in the pull request body: red-before/green-after for both test +files, and the real-hardware before/after transcript +(`VT_DEVICE_WEIGHT_BUDGET_BYTES=1` refusing where it previously did not). + +## Owed + +- **The GDN kernel-dispatch audit.** `needs_weight_staging()` itself stays + false on ROCm, so `DirectDeviceLoadEligible`, `IndexedGdnStateIoEnabled` + (already independently safe — see Design), `MergedGdnBaEnabled`, + `MergedGdnQkvzEnabled`, the packed-decode eligibility struct, and the fp8/bf16 + GDN resident-prep gates all keep their current ROCm behavior. Whether ROCm + SHOULD eventually take the optimized arm for the ones with no op-registration + fallback is a real, separate question this row does not answer — it needs a + per-consumer op-registration and token-exact audit, not a flag flip. No + issue filed yet for this specific follow-up beyond the standing note in + #1934 itself; file one before starting that audit. +- **`release_host_weights_after_upload` / `uses_device_memory_pool` for ROCm.** + Left at `false`/default deliberately (see Out of Scope); a discrete ROCm + card may want different values once measured, same as the CUDA leg's own + comment describes for its choices. + +## Stop conditions + +Stop and report rather than widening scope if closing the "Owed" GDN audit +turns out to be a precondition for something THIS row needs — it should not +be: every existing GDN dispatch path is provably unmoved by this row's tests. + +Stop rather than reusing `Backend::DeviceMemoryInfo` directly from the +platform registrar (see Risks) even if it looks like less code — the +static-init-order hazard is real and already named as the reason the +registrar probes the device itself elsewhere in this same file. + +## Now + +`DONE`. Implementation, tests, CPU and real-hardware (gfx1200) evidence are +in. This row closes #1934 and unblocks #1870, which stays open only pending +the operator's merge of this PR (per that issue's own record). + +## Outcome + +**What was measured.** `RocmPlatform::needs_weight_staging()` gates the ONE +production call site of `CheckDeviceWeightFit`. It hardcodes `false`, so the +refusal never ran on ROCm — confirmed directly, before this row, with +`VT_DEVICE_WEIGHT_BUDGET_BYTES=1` (a budget nothing could satisfy) against a +real checkpoint producing no refusal and proceeding to the next load stage. +Fixed by adding `Platform::allocates_bounded_device_memory()`, a narrower +predicate `RocmPlatform` overrides to `true` while leaving +`needs_weight_staging()` itself untouched, and a real, once-probed +`ResidencyPolicy::device_memory_total_bytes` via a new HIP-free +`vt::rocm::DeviceMemoryTotalBytes` free function. + +**Real hardware, both directions, same command, same file, same box.** +`vllm-cli --model /models/Bonsai-27B-Q1_0.gguf --device auto --max-tokens 4` +with `VT_DEVICE_WEIGHT_BUDGET_BYTES=1`: + +- BEFORE (stashed the fix, rebuilt): no refusal, 2.72s to reach an unrelated + dequant error three load-stages later. +- AFTER: refuses in 0.28s — + `device 'rocm' cannot serve this GGUF: staging its weights needs at least + 3787168768 bytes (3.52 GiB) of device memory across 851 tensors, ... and + this device's memory pool is 1 bytes (0.00 GiB)`. + +**What was rejected.** Flipping `needs_weight_staging()` itself — considered +first, and rejected once reading `IndexedGdnStateIoEnabled` showed it already +takes ROCm's fast arm today via an op-registration check that flipping the +flag would DELETE (see Design), and that at least one sibling consumer +(`MergedGdnBaEnabled` and its relatives) has no such fallback and would +activate unconditionally on unverified kernels. Reusing +`Backend::DeviceMemoryInfo` directly from the platform registrar — rejected +for the static-init-order hazard the registrar's own existing comment already +names, and because it is a live per-request probe with a different, currently +CUDA-dead consumer (issue #1126) that reusing it would silently wake. + +**An unrelated pre-existing build break was found and fixed separately, not +in this row.** A fresh CPU build of the claim base (`2a42cb369`) failed under +GCC 15's `-Werror=nonnull` in Tenstorrent debug-dump code +(`qwen3_5.cpp:4019`), unrelated to ROCm or this row's files. Filed as #2021 +and fixed as its own tiny PR (#2022, `BACKEND-TENSTORRENT-QWEN35`) rather than +riding this branch, per the same one-issue-one-unit-of-work reasoning this +spec applies to itself — this row's own verification needed it, so it was +applied locally and reverted before every commit in this branch, never +landing here. diff --git a/include/vllm/platforms/interface.h b/include/vllm/platforms/interface.h index 1955c2c4c..9c5a2e727 100644 --- a/include/vllm/platforms/interface.h +++ b/include/vllm/platforms/interface.h @@ -372,6 +372,47 @@ class Platform { // memory in place) — the decoupling a bare `kCUDA` cannot express. virtual bool needs_weight_staging() const { return false; } + // BACKEND-ROCM, issue #1934. Does a load on this platform allocate BOUNDED, + // budget-checkable device memory for resident weights — the narrower + // PHYSICAL question `needs_weight_staging()` deliberately does NOT answer. + // + // `needs_weight_staging()`'s own doc above says it "governs the model's + // device-resident forward as a whole", listing `ResidentWeight` among what it + // gates — that listing is STALE. `qwen3_5.cpp::ResidentWeight` was fixed + // under issue #125 to key its alias-vs-upload branch on `is_cpu()`, not + // `needs_weight_staging()`, precisely because the old predicate answered + // false for every non-CUDA device (Vulkan, Metal, XPU, ROCm) and each of + // them aliased a HOST pointer into a DEVICE kernel. So today, on ANY + // non-CPU platform, `ResidentWeight::Alloc` genuinely allocates bounded + // device memory regardless of `needs_weight_staging()` — confirmed on ROCm + // by issue #1870's own reproduction, a real `hipMalloc: out of memory`. + // + // What `needs_weight_staging()` correctly still gates is a DIFFERENT + // question: should the OPTIMIZED device-resident forward run — the indexed + // GDN state-I/O kernels (though `IndexedGdnStateIoEnabled` already + // special-cases a non-staging, non-CPU device by checking op registration + // directly, so ROCm already takes the fast arm there without this method), + // the merged/packed GDN projections, and the fp8/bf16 GDN resident-prep + // passes. Those default to the ROW-COPY REFERENCE path today on ROCm, and + // this method changes NONE of them: flipping `needs_weight_staging()` + // itself was considered and rejected (see the row's spec) because at least + // one of those consumers has no op-registration fallback and would silently + // assume kernels a device might not have, exactly the failure mode + // `IndexedGdnStateIoEnabled` was written to avoid for the ONE consumer that + // already checks. + // + // Consumed by the ONE production call site of `CheckDeviceWeightFit` + // (`gguf_device_fit.h`, issue #1123): the load-time refusal only needs to + // know "will this load draw from a bounded device memory pool, and do we + // know its size" — not "should the fully-optimized forward run". + // + // Default DELEGATES to `needs_weight_staging()`, so CUDA's answer (true) is + // unchanged and every platform that overrides neither method reads exactly + // as it did before this method existed — a pure additive seam. + virtual bool allocates_bounded_device_memory() const { + return needs_weight_staging(); + } + // Does this platform have the fused flash-attention-2 (native-bf16) attention // fast path? The FA2 dispatch (qwen3_5.cpp GdnBlockPaged / full-attn preamble) // emits bf16 q/k and a bf16 attention output — the combo the vendored CUDA diff --git a/include/vt/rocm/rocm_runtime.h b/include/vt/rocm/rocm_runtime.h index d51d55f64..9f14b7620 100644 --- a/include/vt/rocm/rocm_runtime.h +++ b/include/vt/rocm/rocm_runtime.h @@ -10,6 +10,7 @@ // at load time on a machine that merely happens to have HIP installed. #pragma once +#include #include namespace vt::rocm { @@ -55,4 +56,17 @@ bool HostMemoryIsDeviceAddressable(int index) noexcept; // path their silicon took without reading driver internals. bool ManagedAllocActive(int index) noexcept; +// BACKEND-ROCM, issue #1934. `hipMemGetInfo`'s `total` for device `index`, in +// bytes; 0 when the device is absent or the probe fails. HIP-free so the +// PLATFORM registrar (static-init time, unspecified cross-TU order — the same +// reasoning `DeviceAvailable()` above states) can read it without depending on +// `RocmBackend`'s own registrar having already run. Mirrors +// `platforms/cuda.cpp`'s own `cudaMemGetInfo` probe at registration, which +// this project's ResidencyPolicy::device_memory_total_bytes doc already +// specifies as "TOTAL rather than FREE, because free at load time carries the +// page cache and whatever else the box is doing" — same reasoning applies to +// HIP's allocator. 0 == UNKNOWN, which `gguf_device_fit.h`'s load-time +// refusal already reads as "do not decide", never as "nothing fits". +size_t DeviceMemoryTotalBytes(int index) noexcept; + } // namespace vt::rocm diff --git a/src/vllm/entrypoints/model_loader.cpp b/src/vllm/entrypoints/model_loader.cpp index f8cb0793e..3c4e3f412 100644 --- a/src/vllm/entrypoints/model_loader.cpp +++ b/src/vllm/entrypoints/model_loader.cpp @@ -2402,9 +2402,18 @@ std::unique_ptr LoadedEngine::FromModelDir( // already excludes every load it could apply to. const bool policy_forces_full_expand = GgufPolicyForcesFullExpand(gguf_load_policy); + // BACKEND-ROCM (#1934): `allocates_bounded_device_memory()`, not + // `needs_weight_staging()`. The two questions differ (see the interface + // doc): this one asks whether `ResidentWeight` draws from a bounded + // device pool at all -- true on every non-CPU platform since issue + // #125's `is_cpu()` fix -- while `needs_weight_staging()` asks whether + // the FULLY-OPTIMIZED device-resident forward (several GDN kernel + // defaults) should run. Using the narrower predicate here is what makes + // this refusal reachable on ROCm without moving any of the other one's + // consumers; the row's spec records why that flag stays untouched. const DeviceWeightFit fit = CheckDeviceWeightFit( gguf, vt::DeviceTypeName(target.device_type()), - target.needs_weight_staging(), + target.allocates_bounded_device_memory(), DeviceWeightBudgetBytes( target.residency_policy().device_memory_total_bytes), /*model_dtype_bytes=*/2, lane, policy_forces_full_expand); diff --git a/src/vllm/platforms/rocm.cpp b/src/vllm/platforms/rocm.cpp index dca5b962f..6567f7be2 100644 --- a/src/vllm/platforms/rocm.cpp +++ b/src/vllm/platforms/rocm.cpp @@ -18,6 +18,7 @@ // value here is a guess dressed as a decision. #include "vllm/platforms/interface.h" +#include #include #include "vt/backend.h" @@ -28,6 +29,15 @@ namespace { class RocmPlatform final : public Platform { public: + // Issue #1934. `device_memory_total_bytes` is `vt::rocm::DeviceMemoryTotalBytes(0)`, + // probed once by the registrar below at static init, exactly mirroring how + // `CudaPlatform` threads its own `cudaMemGetInfo` probe through its + // constructor (`platforms/cuda.cpp`). 0 means the probe failed or no device + // is present; `residency_policy()` passes it through unexamined, and + // `gguf_device_fit.h` already reads 0 as UNKNOWN rather than "nothing fits". + explicit RocmPlatform(size_t device_memory_total_bytes) + : device_memory_total_bytes_(device_memory_total_bytes) {} + DeviceType device_type() const override { return DeviceType::kROCM; } Backend& backend() const override { return vt::GetBackend(DeviceType::kROCM); } @@ -86,24 +96,46 @@ class RocmPlatform final : public Platform { // snapshot. This flag still stays false because flipping it to engage a // real model's decode-graph path is W2, not W1. // needs_weight_staging() stays false: this is the memory-model POLICY that - // selects the device-resident forward over the host-resident reference path. - // HIP's programming model does stage (hipMalloc hands back a distinct - // address), so a discrete AMD card will eventually answer true — but in W0 - // there is one registered op, so the only path that can run at all is the - // host-resident one the reference tier serves on a unified part. Answering - // true today would route a model into a path with no kernels. Revisit at M2. + // selects the FULLY-OPTIMIZED device-resident forward (indexed GDN state + // I/O with no op-registration fallback for a couple of its consumers, + // merged/packed GDN projections, fp8/bf16 GDN resident prep) over the + // host-resident reference path for THOSE specific kernels. Issue #1934 + // measured that several of those consumers have no per-op fallback and + // would silently assume kernels this device might not register, so + // flipping this blindly was rejected — see that issue and + // `allocates_bounded_device_memory()` below, which answers the NARROWER + // question the load-time device-fit check actually needs without moving + // any of this flag's other consumers. `IndexedGdnStateIoEnabled` already + // takes ROCm's fast arm today regardless of this flag, by checking op + // registration directly rather than trusting this policy bit — the same + // move #1934 makes for the device-fit check. // supports_fa2_attention() / opaque_attention_op() stay false: no ROCm // attention kernel exists yet. See get_attn_backend_priority below. - // The residency/memory-model policy. DEFAULTS in W0, and the reason is not - // laziness: on a unified part (780M, Strix Halo) freeing the host copy after - // "upload" would free the ONLY copy, which is the same answer the CPU, Metal - // and Vulkan platforms give for the same reason. A DISCRETE card genuinely - // wants release_host_weights_after_upload=true and a pooled allocator, and - // that is a per-DEVICE answer this per-DEVICE-TYPE seam cannot express yet - // (the CUDA leg has the same shape and has not needed to). Flip it when a - // discrete board actually loads weights, with the measurement in the record. - ResidencyPolicy residency_policy() const override { return {}; } + // BACKEND-ROCM, issue #1934. The ONE narrow question the load-time + // device-fit refusal (`gguf_device_fit.h`, issue #1123) needs answered: + // does a load here allocate device memory `ResidentWeight` cannot exceed + // unnoticed? Yes, unconditionally, on any non-CPU platform since issue + // #125's fix (see the interface doc on this method) — independent of + // `needs_weight_staging()`, which this method deliberately does not touch. + bool allocates_bounded_device_memory() const override { return true; } + + // The residency/memory-model policy. `device_memory_total_bytes` is now a + // REAL probe (issue #1934, mirrors `CudaPlatform`'s own `cudaMemGetInfo` + // probe) — the ONE field the device-fit check reads. The other two fields + // stay DEFAULT/false, unlike CUDA's: `release_host_weights_after_upload` + // and `uses_device_memory_pool` are separate policy questions (a discrete + // card's host-copy release and DevicePool reuse) this row does not touch, + // because on a unified part (780M, Strix Halo) freeing the host copy after + // "upload" would free the ONLY copy — the same answer CPU, Metal and Vulkan + // give for the same reason, and per-DEVICE (not per-DEVICE-TYPE) besides. + // Flip those when a discrete board's release/pool behavior is actually + // measured, not as a side effect of making the budget check reachable. + ResidencyPolicy residency_policy() const override { + ResidencyPolicy p; + p.device_memory_total_bytes = device_memory_total_bytes_; + return p; + } // Attention-backend priority — M3 (issue #41). Mirrors rocm.py:407-441 // `_get_backend_priorities` at pin 555967922. The dense branch is @@ -130,6 +162,9 @@ class RocmPlatform final : public Platform { return {"ROCM_ATTN", "ROCM_AITER_FA", "ROCM_AITER_UNIFIED_ATTN", "TRITON_ATTN", "TURBOQUANT"}; } + + private: + size_t device_memory_total_bytes_ = 0; }; // Registers kROCM during static init (registration completes before main() per @@ -143,7 +178,13 @@ class RocmPlatform final : public Platform { struct Registrar { Registrar() noexcept { if (!vt::rocm::DeviceAvailable()) return; - static RocmPlatform platform; + // Issue #1934. Device 0, matching this leg's other single-device probes + // (`host_memory_is_device_addressable()` above states the same choice). + // HIP-free free function, not `Backend::DeviceMemoryInfo`: the backend's + // OWN registrar (`rocm_backend.hip`) may not have run yet at this point — + // static-init order across TUs is unspecified, the same reason this + // registrar probes the device itself rather than trusting one. + static RocmPlatform platform(vt::rocm::DeviceMemoryTotalBytes(0)); RegisterPlatform(DeviceType::kROCM, &platform); } } registrar; diff --git a/src/vt/rocm/rocm_backend.hip b/src/vt/rocm/rocm_backend.hip index 121aacf6d..e09f3b292 100644 --- a/src/vt/rocm/rocm_backend.hip +++ b/src/vt/rocm/rocm_backend.hip @@ -460,4 +460,15 @@ bool ManagedAllocActive(int index) noexcept { return caps.valid && UseManagedAlloc(caps); } +size_t DeviceMemoryTotalBytes(int index) noexcept { + // Mirrors `Backend::DeviceMemoryInfo` (above) byte-for-byte, deliberately + // NOT calling it: this is a HIP-free, backend-registration-independent + // free function meant for the platform registrar (see the declaration), + // where `GetBackend(kROCM)` may not have run yet. + if (hipSetDevice(index) != hipSuccess) return 0; + size_t free_b = 0, tot_b = 0; + if (hipMemGetInfo(&free_b, &tot_b) != hipSuccess) return 0; + return tot_b; +} + } // namespace vt::rocm diff --git a/tests/vllm/entrypoints/test_gguf_device_fit_reach.cpp b/tests/vllm/entrypoints/test_gguf_device_fit_reach.cpp index e22d8b807..8337cad18 100644 --- a/tests/vllm/entrypoints/test_gguf_device_fit_reach.cpp +++ b/tests/vllm/entrypoints/test_gguf_device_fit_reach.cpp @@ -100,7 +100,16 @@ class StagingPlatform final : public vllm::platforms::Platform { std::vector supported_dtypes() const override { return {vt::DType::kBF16}; } - bool needs_weight_staging() const override { return true; } + bool needs_weight_staging() const override { return needs_weight_staging_flag; } + // BACKEND-ROCM (#1934). Overridden explicitly, NOT left to the default + // delegation to `needs_weight_staging()`, and independently settable: the + // two are proven-independent inputs to `CheckDeviceWeightFit`'s call site + // by a case that sets them to DIFFERENT values, mirroring ROCm's own real + // production state (`needs_weight_staging()=false`, + // `allocates_bounded_device_memory()=true`). + bool allocates_bounded_device_memory() const override { + return allocates_bounded_device_memory_flag; + } // ENG-EXPERT-STREAM-DEVICE W0d (#1124). The second half of the loader's lane // condition. A settable field for the same reason `create_queue_throws` is // one: the platform registry is process-global, so a second registration would @@ -115,6 +124,8 @@ class StagingPlatform final : public vllm::platforms::Platform { } bool host_addressable = false; + bool needs_weight_staging_flag = true; + bool allocates_bounded_device_memory_flag = true; private: HostBackend& backend_; @@ -882,3 +893,63 @@ TEST_CASE("device fit: the VARIABLE beats the config key, through the loader") { CHECK(message.find("cannot serve this GGUF") != std::string::npos); CHECK(message.find(std::to_string(kStagedLowerBound - 1)) != std::string::npos); } + +// --- BACKEND-ROCM (#1934): the refusal is gated on -------------------------- +// --- `allocates_bounded_device_memory()`, never on `needs_weight_staging()` - +// +// Issue #1934: `RocmPlatform::needs_weight_staging()` is stale-false, so +// before this row the ONE production call site of `CheckDeviceWeightFit` +// never ran on ROCm regardless of budget. The fix is `model_loader.cpp` +// reading `target.allocates_bounded_device_memory()` instead. These two cases +// pin that the call site reads the NEW predicate and NOT the old one, in both +// directions, so a regression that reverted the call site to +// `needs_weight_staging()` — or one that read a `||` instead of the plain +// predicate — goes red here. + +TEST_CASE( + "device fit: ROCm's own state (staging=false, bounded-memory=true) " + "still refuses") { + RegisterFakeStagingPlatform(); + Platform().needs_weight_staging_flag = false; + Platform().allocates_bounded_device_memory_flag = true; + TempFile f(BuildSyntheticMoeGguf()); + + vllm_test::SetEnv("VT_DEVICE_WEIGHT_BUDGET_BYTES", + std::to_string(kStagedLowerBound - 1)); + const std::string message = ThrownMessage(f.path(), vllm::Device::kNamedPlatform); + vllm_test::UnsetEnv("VT_DEVICE_WEIGHT_BUDGET_BYTES"); + Platform().needs_weight_staging_flag = true; + Platform().allocates_bounded_device_memory_flag = true; + + REQUIRE_FALSE(message.empty()); + CAPTURE(message); + CHECK(message.find("cannot serve this GGUF") != std::string::npos); + CHECK(message.find(std::to_string(kStagedLowerBound)) != std::string::npos); + CHECK(message.find(std::to_string(kStagedLowerBound - 1)) != std::string::npos); + CHECK(message.find("tokenizer") == std::string::npos); +} + +TEST_CASE( + "device fit: a platform that stages but reports no bounded memory is " + "NEVER refused") { + RegisterFakeStagingPlatform(); + Platform().needs_weight_staging_flag = true; + Platform().allocates_bounded_device_memory_flag = false; + TempFile f(BuildSyntheticMoeGguf()); + + // One byte under the footprint, exactly the budget the positive-control case + // above refuses at. The ONLY thing that moved is + // `allocates_bounded_device_memory_flag`. + vllm_test::SetEnv("VT_DEVICE_WEIGHT_BUDGET_BYTES", + std::to_string(kStagedLowerBound - 1)); + const std::string message = ThrownMessage(f.path(), vllm::Device::kNamedPlatform); + vllm_test::UnsetEnv("VT_DEVICE_WEIGHT_BUDGET_BYTES"); + Platform().allocates_bounded_device_memory_flag = true; + + REQUIRE_FALSE(message.empty()); + CAPTURE(message); + CHECK(message.find("cannot serve this GGUF") == std::string::npos); + // The LATER error, asserted positively: without it, "no refusal" would also + // be true of a load that died earlier for an unrelated reason. + CHECK(message.find("tokenizer: GGUF missing kv") != std::string::npos); +} diff --git a/tests/vllm/platforms/test_platform.cpp b/tests/vllm/platforms/test_platform.cpp index 90cc8ba4a..e1a02e42f 100644 --- a/tests/vllm/platforms/test_platform.cpp +++ b/tests/vllm/platforms/test_platform.cpp @@ -69,6 +69,12 @@ TEST_CASE("CPU platform is self-registered and advertises CPU capabilities") { // in place (no staging) and has no FA2 kernel, exactly what the converted // `device==kCUDA` gates answered on a CPU device (byte-identical). CHECK_FALSE(cpu.needs_weight_staging()); + // BACKEND-ROCM (#1934). The CPU leg overrides neither method, so the new + // predicate's default delegation to needs_weight_staging() is what answers + // here — proving the default is a true no-op for a platform that has not + // opted into the narrower question. + CHECK_FALSE(cpu.allocates_bounded_device_memory()); + CHECK(cpu.allocates_bounded_device_memory() == cpu.needs_weight_staging()); CHECK_FALSE(cpu.supports_fa2_attention()); // Proof that needs_weight_staging() is NOT is_unified_memory() in disguise: CPU // is UNIFIED (host==device memory) yet does NOT stage. The two predicates @@ -325,6 +331,11 @@ TEST_CASE("is_device_capability_family matches any .x (interface.py:481-4 CHECK_FALSE(sm121.is_cpu()); CHECK_FALSE(sm121.needs_weight_staging()); CHECK(sm121.is_cpu() == sm121.needs_weight_staging()); + // BACKEND-ROCM (#1934). Same base-default proof as needs_weight_staging() + // itself: this stub overrides neither method, so the delegation is what + // answers, and it answers false here exactly as needs_weight_staging() does. + CHECK_FALSE(sm121.allocates_bounded_device_memory()); + CHECK(sm121.allocates_bounded_device_memory() == sm121.needs_weight_staging()); CHECK_FALSE(sm121.opaque_attention_op()); CHECK_FALSE(sm121.support_static_graph_mode()); @@ -398,6 +409,81 @@ TEST_CASE("host_memory_is_device_addressable is independent of every neighbour") gb10.backend().DeviceMemoryIsHostAddressable()); } +namespace { +// BACKEND-ROCM, issue #1934. ROCm's own real shape, constructible on a CPU +// tier because `RocmPlatform` only compiles into a HIP build: a platform that +// does NOT need the fully-optimized device-resident forward +// (`needs_weight_staging() == false`, so none of that flag's other GDN +// kernel-dispatch consumers move) but DOES allocate bounded, budget-checkable +// device memory for resident weights (`allocates_bounded_device_memory() == +// true`, so the load-time GGUF fit refusal runs). If the two predicates were +// secretly the same bit, this platform could not exist. +class FakeBoundedNonStagingPlatform final : public Platform { + public: + DeviceType device_type() const override { return DeviceType::kROCM; } + vt::Backend& backend() const override { return vt::GetBackend(DeviceType::kCPU); } + DeviceCapability get_device_capability() const override { return {12, 0}; } + std::vector supported_dtypes() const override { return {DType::kBF16}; } + ResidencyPolicy residency_policy() const override { + ResidencyPolicy p; + p.device_memory_total_bytes = 17095983104; // this box's real gfx1200 probe + return p; + } + bool needs_weight_staging() const override { return false; } + bool allocates_bounded_device_memory() const override { return true; } +}; +} // namespace + +TEST_CASE("allocates_bounded_device_memory is independent of needs_weight_staging") { + FakeBoundedNonStagingPlatform rocm_like; + + // The value threads through the virtual at all: a platform that overrides it + // answers its own value, not the delegated default. + CHECK(rocm_like.allocates_bounded_device_memory()); + + // AGAINST needs_weight_staging, in ROCm's own real direction (issue #1934): + // FALSE there, TRUE here. A load-time refusal keyed on needs_weight_staging() + // would never run on this platform; one keyed on this predicate does — which + // is the entire fix `model_loader.cpp`'s CheckDeviceWeightFit call site makes. + CHECK_FALSE(rocm_like.needs_weight_staging()); + CHECK(rocm_like.allocates_bounded_device_memory() != + rocm_like.needs_weight_staging()); + + // The default delegation is a SEPARATE code path from this override, so + // proving the override wins is not enough on its own — confirm the DEFAULT + // really would have answered false here (matching needs_weight_staging()), + // by reading it through a platform that overrides neither, already pinned + // above via `cpu` and `sm121`. Restated here for locality: the override on + // this class is what makes the two diverge, not a change to the default. + CHECK_FALSE(GetPlatform(DeviceType::kCPU).allocates_bounded_device_memory()); +} + +// BACKEND-ROCM, issue #1934. The ROCm leg's real answers, exercised only where +// a real ROCm platform is registered (a HIP build on a machine with a device — +// this project's own gfx1200 box qualifies). `needs_weight_staging()` is +// UNCHANGED by this row (still false, on purpose — see rocm.cpp); the new +// method and the real probe are what move. +TEST_CASE("ROCm leg values (real hardware, HIP build only)") { + if (!HasPlatform(DeviceType::kROCM)) return; // no device: nothing to assert + Platform& rocm = GetPlatform(DeviceType::kROCM); + + CHECK_FALSE(rocm.needs_weight_staging()); + CHECK(rocm.allocates_bounded_device_memory()); + CHECK(rocm.allocates_bounded_device_memory() != rocm.needs_weight_staging()); + + // The probe actually ran: a real card reports a nonzero total, and 0 would + // read as UNKNOWN at the load-time refusal — silently disabling it again, + // the exact shape #1934 reports. `hipMemGetInfo` failing on a REGISTERED + // platform (device already proven present by `HasPlatform` above) would be + // its own bug, not an acceptable "0 == unknown" outcome here. + const ResidencyPolicy policy = rocm.residency_policy(); + CHECK(policy.device_memory_total_bytes > 0); + // Neither of the two fields this row deliberately left at their safe + // default (see rocm.cpp's residency_policy() comment) moved. + CHECK_FALSE(policy.release_host_weights_after_upload); + CHECK_FALSE(policy.uses_device_memory_pool); +} + // The CUDA leg's capability ANSWERS, exercised only where a real CUDA platform is // registered (the dgx CUDA build / a GPU box). This is the executable proof that // each S3-converted gate is byte-identical: the predicate returns `true` on this