Skip to content

fix(BACKEND-ROCM): a narrow predicate makes the device-fit refusal reachable on ROCm - #2027

Open
joral wants to merge 3 commits into
mudler:mainfrom
joral:claude/issue-1934-fixflag
Open

fix(BACKEND-ROCM): a narrow predicate makes the device-fit refusal reachable on ROCm#2027
joral wants to merge 3 commits into
mudler:mainfrom
joral:claude/issue-1934-fixflag

Conversation

@joral

@joral joral commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Makes the #1123/#1870 load-time device-fit refusal reachable on ROCm —
the actual remaining blocker #1870 was reopened against — without moving
needs_weight_staging() or any of its other consumers.

CheckDeviceWeightFit's one call site reads target.needs_weight_staging()
to decide whether to run at all. RocmPlatform hardcodes that 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. Meanwhile the real device
allocation the refusal exists to guard (ResidentWeight's d.b.Alloc) runs
unconditionally on any non-CPU platform since issue #125's is_cpu() fix —
so #1870's crash stayed reachable regardless of that row's arithmetic fix.

Why not just flip needs_weight_staging()

Read literally: IndexedGdnStateIoEnabled (qwen3_5.cpp) already takes
ROCm's fast arm today, independent of this flag — its non-staging branch
falls through to an op-registration check
(kCausalConv1dUpdate/kGdnDecode/kGdnStateGather/kGdnStateScatter, all
four registered for ROCm) rather than trusting the flag. Flipping it would
make this consumer take the other branch, which drops that check and just
assumes the ops exist — a regression for this one consumer, not an
improvement. MergedGdnBaEnabled/MergedGdnQkvzEnabled and the packed-decode
/ fp8-resident-prep gates have no such fallback at all: flipping the flag
would activate them on ROCm unconditionally, with no verification that the
underlying merged/packed GEMM and fp8 paths are numerically correct there.

So this PR adds a new, narrow Platform::allocates_bounded_device_memory()
instead — default-delegates to needs_weight_staging() (byte-identical for
every platform that overrides neither), RocmPlatform overrides only this
new method to true. needs_weight_staging() itself, and all six-plus of
its other GDN-dispatch consumers, are unmoved — tests pin this in both
directions. residency_policy() now carries a real, once-probed
device_memory_total_bytes via a new HIP-free vt::rocm::DeviceMemoryTotalBytes
(mirrors DeviceAvailable()'s registrar-independence reasoning; deliberately
not Backend::DeviceMemoryInfo, a live per-request probe with a different,
currently CUDA-dead consumer per issue #1126).

Real hardware, both directions, same command, same box

vllm-cli --model .../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).

Red-before/green-after on both test_platform and
test_gguf_device_fit_reach: every new case fails to compile against the
unfixed tree (stash + rebuild), both suites pass in full afterward on this
box — CPU build and, separately, a full ROCm build run on real gfx1200
hardware (17/17 cases, 133/81 assertions).

Note: an unrelated build break found along the way

A fresh CPU build of the claim base failed under GCC 15's -Werror=nonnull
in unrelated Tenstorrent debug-dump code. Filed and fixed separately as
#2021/#2022 rather than riding this branch — this PR's own local
verification applied that fix and reverted it before every commit here, so
it does not appear in this diff.

Spec: .agents/specs/rocm-device-fit-bounded-memory.md (this PR).

FOLLOWING_AGENTS_PROTOCOL

Following-Agents-Protocol: true
AI-Assisted: true
Assisted-by: AGENT:claude-sonnet-5 [claude-code]

joral added 3 commits August 26, 2026 17:25
Issue mudler#1934: RocmPlatform::needs_weight_staging() is stale-false, so
CheckDeviceWeightFit's one call site never runs on ROCm regardless
of mudler#1870/mudler#1935's arithmetic fix -- measured directly, a real
checkpoint load with VT_DEVICE_WEIGHT_BUDGET_BYTES=1 produced no
refusal. Records why flipping needs_weight_staging() itself was
rejected (IndexedGdnStateIoEnabled already checks op registration
directly and would regress; several sibling GDN consumers have no
such fallback and would activate unverified kernels) in favor of a
new, narrower Platform predicate before any implementation lands.

FOLLOWING_AGENTS_PROTOCOL

Following-Agents-Protocol: true
AI-Assisted: true
Assisted-by: AGENT:claude-sonnet-5 [claude-code]
…achable on ROCm

Issue mudler#1934. CheckDeviceWeightFit's one call site read
target.needs_weight_staging(), which RocmPlatform hardcodes false --
a stale W0-era placeholder never revisited despite ROCm landing
grouped-GEMM, MoE combine/gate, ROCM_ATTN and hipGraph decode capture
since. Measured directly: VT_DEVICE_WEIGHT_BUDGET_BYTES=1 against a
real checkpoint produced no refusal at all, while the actual device
allocation the refusal guards (ResidentWeight's d.b.Alloc) runs
unconditionally on any non-CPU platform since issue mudler#125's is_cpu()
fix -- so mudler#1870's crash stayed reachable regardless of that row's
arithmetic fix.

Rejected flipping needs_weight_staging() itself: that flag also gates
several GDN kernel-dispatch defaults (merged/packed projections,
fp8/bf16 resident prep) with no op-registration fallback, which would
newly activate unconditionally on ROCm the moment it moved --
IndexedGdnStateIoEnabled already takes ROCm's fast arm today by
checking op registration directly rather than trusting this flag, and
flipping it would remove that check for that one consumer while
blindly trusting the others.

Adds Platform::allocates_bounded_device_memory(), default-delegating
to needs_weight_staging() (byte-identical for every platform that
overrides neither), RocmPlatform overrides only this new method to
true. residency_policy() now carries a real, once-probed
device_memory_total_bytes via a new HIP-free vt::rocm::DeviceMemoryTotalBytes
free function (mirrors DeviceAvailable()'s registrar-independence
reasoning; not Backend::DeviceMemoryInfo, whose live per-request probe
is a different, currently-CUDA-dead seam per issue mudler#1126). The other
two ResidencyPolicy fields stay at their existing default.

model_loader.cpp's ONE CheckDeviceWeightFit call site now reads the
new predicate; the other needs_weight_staging() read in the same
function (the streamed-expert-lane condition) is untouched.

Red-before: every new test references the method before it exists,
captured by stashing the implementation and rebuilding (test_platform
and test_gguf_device_fit_reach both fail to compile). Green-after:
both pass in full, including new cases proving the two predicates
move independently in both directions and that every existing GDN
kernel-dispatch consumer of needs_weight_staging() is unmoved.

FOLLOWING_AGENTS_PROTOCOL

Following-Agents-Protocol: true
AI-Assisted: true
Assisted-by: AGENT:claude-sonnet-5 [claude-code]
…same box

Records the before/after vllm-cli transcript on this gfx1200 card
(no refusal -> named refusal, same command, same file,
VT_DEVICE_WEIGHT_BUDGET_BYTES=1) and the rejected alternatives
(flipping needs_weight_staging(), reusing Backend::DeviceMemoryInfo)
with the reasons. Also notes mudler#2021/mudler#2022, the unrelated GCC 15
build break this row's own verification needed applied and reverted
locally, never landing on this branch.

FOLLOWING_AGENTS_PROTOCOL

Following-Agents-Protocol: true
AI-Assisted: true
Assisted-by: AGENT:claude-sonnet-5 [claude-code]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant