From f7c41abdcc4b406113e3f8ccca44b9f2dfa152f0 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 24 Aug 2026 01:56:00 +0000 Subject: [PATCH 1/3] fix(BACKEND-ATTN-REGISTRY): a platform reports an SM version or nothing, and Vulkan was refusing FLASH_ATTN too (#1823) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `FlashAttentionBackend::supports_compute_capability` is upstream's NVIDIA `capability >= (8, 0)` (flash_attn.py:200-202). `MetalPlatform::get_device_capability` answered with the MTLGPUFamilyApple generation and `VulkanPlatform` with the Vulkan API version, so an SM-version bar was compared against two numbers that are not SM versions. Apple family 9 on the M4 gate box cleared it by coincidence; a GitHub `macos-15` runner reported lower and `macos-metal-mlx` has been red since 369ea7fd4. The oracle is unambiguous and had already met this case. `Platform.get_device_capability` is documented as the "stateless version of torch.cuda.get_device_capability" (interface.py:420-431), `validate_configuration` calls the predicate with no None guard (backend.py:366-367) because its only callers are CudaPlatform (cuda.py:381,410, after `assert device_capability is not None` at :404) and RocmPlatform (rocm.py:531,558), and every other platform selects its backend without ever evaluating it (cpu.py:75-87). When upstream hit a platform whose capability format differs from CUDA's it returned None and said why: xpu.py:228-236, "capacity format differs from cuda's and will cause unexpected failure". Metal and Vulkan now answer the same way. The Apple family and the Vulkan API version stay on `vt::Backend`, which is the seam that owns device-unit questions, and the tests assert them there so this is not a number deleted to make a gate pass. Grounding the premise turned up a second live instance. Vulkan names FLASH_ATTN as the only entry in its priority list while reporting `major == 1`, which is true of every Vulkan device that will ever exist, so `SelectAttentionBackendName` threw on kVULKAN unconditionally. Nothing saw it because the lane's test asserted that FLASH_ATTN is NAMED in the priority list and never that the selector REACHES it — the same gap the Metal test had. That gap is what made the defect locally reproducible without a Mac, and the red-before below is a Vulkan run. Measured, this moves two device types rather than the four the issue estimated: kCPU and kTENSTORRENT already report an absent capability and do not move at all, and kCUDA/kROCM answer in the unit the predicate expects. No selection rework was needed. RED-BEFORE, lavapipe, base df1ee2058, assertion added and no fix applied: `present=true major=1 minor=4`, then `CHECK( SelectAttentionBackendName(p) == "FLASH_ATTN" ) THREW "No valid attention backend for device type 3 from {FLASH_ATTN: [compute capability not supported]}"` — the same message test_metal_backend.cpp:170 produced for device type 2. GREEN-AFTER: `present=false major=-1 minor=-1; vt::Backend API version 1.4`, selector resolves. test_vulkan_backend 35/35, 2109 assertions, SUCCESS!, 0 skipped; test_backend_cross_device 25/25, 80140 assertions, SUCCESS!; test_platform on the Vulkan tier 15/15, 118 assertions, SUCCESS!. Restoring the defective Vulkan body reds both new gates and the tree restores byte-identical. `test_platform` gains the contract as a class gate rather than a list of platforms, so a platform added later is covered with no edit, and the Vulkan lane now runs it — on build-test-cpu only kCPU is registered, which cannot catch the defect the case exists for. Closes #1823 FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code] --- .agents/specs/attn-capability-unit.md | 252 +++++++++++++++++++ .agents/specs/attn-validate-configuration.md | 7 + .github/workflows/ci.yml | 30 ++- include/vllm/platforms/interface.h | 28 ++- src/vllm/platforms/metal.cpp | 31 ++- src/vllm/platforms/vulkan.cpp | 33 ++- src/vllm/v1/attention/backend.cpp | 21 +- tests/vllm/platforms/test_platform.cpp | 54 ++++ tests/vt/test_metal_backend.cpp | 19 +- tests/vt/test_vulkan_backend.cpp | 33 ++- 10 files changed, 469 insertions(+), 39 deletions(-) create mode 100644 .agents/specs/attn-capability-unit.md diff --git a/.agents/specs/attn-capability-unit.md b/.agents/specs/attn-capability-unit.md new file mode 100644 index 000000000..1e853f254 --- /dev/null +++ b/.agents/specs/attn-capability-unit.md @@ -0,0 +1,252 @@ +# SPEC — `Platform::get_device_capability()` is a CUDA-SM value, or it is absent + +Issue: [#1823](https://github.com/mudler/vllm.cpp/issues/1823) +Owning row: `BACKEND-ATTN-REGISTRY` (`.agents/backend-matrix.md`), which owns the +shared attention selector seam and filed this defect against itself under +`## Found in flow, filed, not fixed here` in +[attn-validate-configuration.md](attn-validate-configuration.md). +Scope of this change: the UNIT of the Platform capability value, and the two +platforms that answered it in a foreign unit. Nothing else. + +## Now + +`BACKEND-ATTN-REGISTRY` stays in its recorded state. This change repairs a +defect in the capability layer that row landed; it does not move the row's +lifecycle, add a backend, or change the selector's algorithm. + +## M0 — reconciliation, performed before any code + +Checked at `origin/main` `df1ee2058`: + +- `git log --oneline --grep '1823'` returns no landing for this fix. +- `git log -S'get_device_capability' --oneline -- src/vllm/platforms/metal.cpp + src/vllm/platforms/vulkan.cpp` returns exactly the two skeleton commits that + introduced the values (`13bb7241f` Metal W0, `1cb5f643f` Vulkan W0). Neither + value has ever been revised. +- `gh pr list --state open --limit 100`: 21 open pull requests, none touching + `src/vllm/platforms/**`, `src/vllm/v1/attention/**` or the Metal/Vulkan tests. +- `git branch -r | grep -iE '1823|capability-unit'`: no match. +- The issue index already carries #1823 at line 673 owned by + `BACKEND-ATTN-REGISTRY`. The index is append-only and a row is never edited, so + this spec is linked from the existing row rather than appended again. + +## The oracle determination + +**vLLM owns attention-backend selection, and it owns the unit of the capability +value.** Read at the parity pin `555967922`. + +1. `vllm/platforms/interface.py:420-431 Platform.get_device_capability` — the + docstring defines the unit outright: *"Stateless version of + [torch.cuda.get_device_capability][]"*. The base returns `None`. +2. `vllm/v1/attention/backend.py:366-367 + AttentionBackend.validate_configuration` calls + `cls.supports_compute_capability(device_capability)` **unconditionally** — + there is no `is not None` guard, because the caller guarantees the unit. +3. The callers are the guarantee. `validate_configuration` is reached from + exactly three places, and every one of them is an NVIDIA or AMD selector: + `vllm/platforms/cuda.py:381` (`CudaPlatform.get_valid_backends`) and + `cuda.py:410` (`CudaPlatform.get_attn_backend_cls`, immediately after + `assert device_capability is not None` at `cuda.py:404-405`); + `vllm/platforms/rocm.py:531` and `:558`; and + `vllm/v1/attention/backends/mla/prefill/selector.py:125,167`, the CUDA MLA + prefill selector. **Every other platform implements `get_attn_backend_cls` + itself and never evaluates the predicate at all** — `CpuPlatform.get_attn_backend_cls` + (`vllm/platforms/cpu.py:75-87`) returns `CPU_ATTN` outright. +4. `vllm/v1/attention/backends/flash_attn.py:200-202 + FlashAttentionBackend.supports_compute_capability` is therefore a statement + about NVIDIA SM versions, by construction of who may ask it. +5. **Upstream met this exact defect and recorded the answer.** + `vllm/platforms/xpu.py:228-236 XpuPlatform.get_device_capability`: + + ```python + def get_device_capability(cls, device_id: int = 0) -> DeviceCapability | None: + # capacity format differs from cuda's and will cause unexpected + # failure, so use None directly + return None + ``` + + A platform whose capability is not in CUDA's unit reports **None**. That is + the mirror, in upstream's own words, for the situation this issue describes. + +**What this tree does differently, and why the rule still lands where upstream +puts it.** `SelectAttentionBackendName` +(`src/vllm/v1/attention/registry.cpp:90-140`) is ONE selector shared by every +`DeviceType`, which is the recorded attn-registry seam. Under that shape the SM +predicate is asked of every platform, so upstream's per-caller guarantee has to +become a per-platform contract on the value: **absent means "no CUDA-SM answer", +and the predicate is skipped — which is what upstream's CPU/XPU/TPU platforms get +by never reaching `validate_configuration`; present means a CUDA-SM value — which +is what upstream's CUDA/ROCm platforms get from `assert device_capability is not +None`.** With that contract the shared selector is behaviourally identical to +upstream's per-platform selectors. Without it the comparison is undefined, which +is the bug. + +Rejected: making `FlashAttentionBackend` device-type aware. Upstream's predicate +takes a capability and nothing else (`flash_attn.py:201`), and a backend that +branches on `DeviceType` would be an invention with no upstream anchor, in the +one class that is a 1:1 port. + +Rejected: dropping the `capability.present()` guard at +`src/vllm/v1/attention/backend.cpp:197` to match upstream's unguarded call. The +guard is what stands in for upstream's caller-side `assert`; removing it would +refuse FLASH_ATTN on kCPU and kTENSTORRENT, which upstream serves. + +## Design + +`MetalPlatform::get_device_capability` and `VulkanPlatform::get_device_capability` +return an ABSENT `DeviceCapability`, mirroring `XpuPlatform` and citing it. + +The device-specific numbers are not lost and were never the Platform seam's to +report. The Apple GPU family stays on +`vt::Backend::DeviceCapabilityMajor/Minor` (`src/vt/metal/metal_backend.mm:127-130`) +and the Vulkan API version stays on `vt::Backend::DeviceCapabilityMajor/Minor` +(`src/vt/vulkan/vulkan_backend.cpp:144-145`) and `VulkanContext::api_major/minor`. +Nothing in the tree consumed the Platform-level value for either device: the only +consumer of `Platform::has_device_capability` is +`CudaPlatform::supports_fp8` (`src/vllm/platforms/cuda.cpp:47`, +`has_device_capability(8, 9)`), an unmistakably CUDA-SM question. + +`include/vllm/platforms/interface.h` states the unit contract on the virtual, so +the next platform does not rediscover it, and +`src/vllm/v1/attention/backend.cpp` replaces the premise the issue identified as +false with the one that is now true and enforced. + +## What each device type selects, before and after + +| DeviceType | capability before | predicate | selects before | selects after | +|---|---|---|---|---| +| kCUDA | SM, e.g. `{12,1}` | applied, correct unit | FLASH_ATTN | UNCHANGED | +| kROCM | gfx-derived, e.g. `{9,4}` | applied, correct unit | per priority list | UNCHANGED | +| kCPU | absent `{-1,-1}` | skipped | FLASH_ATTN / CPU_ATTN | UNCHANGED | +| kTENSTORRENT | absent `{-1,-1}` | skipped | per priority list | UNCHANGED | +| kMETAL | Apple family `{N,0}` | applied to a FOREIGN unit | FLASH_ATTN only when family >= 8, by coincidence; THROWS below it | absent, skipped, FLASH_ATTN on every Apple family | +| kVULKAN | Vulkan API `{1,4}` | applied to a FOREIGN unit | **THROWS ALWAYS** (`1 >= 8` is false on every Vulkan device that will ever exist) | absent, skipped, FLASH_ATTN | + +Two device types change, and both changes are the repair. The issue estimated +four; measured, kCPU and kTENSTORRENT already report an absent capability +(`src/vllm/platforms/cpu.cpp:18`, `src/vllm/platforms/tenstorrent.cpp:34`) and do +not move at all. No selection rework is required, so this row does not need to +return `NEEDS_DECISION`. + +## The second instance, found while grounding the premise + +The issue asked for `vulkan.cpp` and `tenstorrent.cpp` to be checked against the +same premise. Tenstorrent is clean. **Vulkan carries the identical defect and is +strictly worse than Metal's**: `VulkanPlatform::get_attn_backend_priority` +returns `{"FLASH_ATTN"}` while the platform reports the Vulkan API version as its +capability, so `major=1` fails an SM-8.0 bar on *every* device, always — there is +no coincidence to save it. It went unseen because +`tests/vt/test_vulkan_backend.cpp` asserted that FLASH_ATTN is NAMED in the +priority list and never that the selector REACHES it. That is the same gap the +Metal test had, and closing it on both lanes is part of this change. + +## Tests + +1. `tests/vt/test_vulkan_backend.cpp` — the red-before. The platform case now + prints the capability it reports and asserts + `SelectAttentionBackendName(p) == "FLASH_ATTN"`. Runs on the existing + `build-test-vulkan` lane, GPU-free on lavapipe, and locally. +2. `tests/vt/test_metal_backend.cpp` — the assertion at :153-154 that Metal + reports a `present()` capability is INVERTED to the corrected contract, and + the Apple family is asserted where it actually lives, on `vt::Backend`. The + existing `SelectAttentionBackendName(p) == "FLASH_ATTN"` at :170 is the + green-after. Runs on `macos-metal-mlx`. +3. `tests/vllm/platforms/test_platform.cpp` — the class gate. Every REGISTERED + platform either reports an absent capability or is a platform whose + capability is a CUDA-SM value (kCUDA/kROCM). A future platform that reports a + foreign unit reds on whichever lane registers it. This is deliberately a + contract test rather than a per-platform list, so it does not have to be + edited when a platform is added. + +## Gates + +- `build-test-vulkan` (the red-before/green-after this change can run locally). +- `macos-metal-mlx` — RED on `main` at `df1ee2058`; clearing it is the + deliverable, read at JOB level from a dispatched run. +- `build-test-cpu` full ctest, for the shared `interface.h`/`backend.cpp` edits. +- `scripts/agent-preflight.sh`. + +## Evidence + +Host: this Linux workstation, GCC, `-DVLLM_CPP_VULKAN=ON`, lavapipe (`lvp_icd.json`) +software ICD — the same GPU-free arrangement `build-test-vulkan` uses. + +**RED-BEFORE, run locally at base `df1ee2058` with only the missing assertion +added and no fix applied.** The capability numbers are printed by the case +itself, so the mechanism is in the log rather than in the argument: + +``` +tests/vt/test_vulkan_backend.cpp:487: MESSAGE: kVULKAN Platform::get_device_capability() present=true major=1 minor=4 +tests/vt/test_vulkan_backend.cpp:489: ERROR: CHECK( vllm::v1::SelectAttentionBackendName(p) == "FLASH_ATTN" ) THREW exception: + "No valid attention backend for device type 3 from {FLASH_ATTN: [compute capability not supported]} (use_mla=false, use_sparse=false)" +[doctest] test cases: 1 | 0 passed | 1 failed | 34 skipped +[doctest] assertions: 15 | 14 passed | 1 failed | +[doctest] Status: FAILURE! +``` + +`major=1 minor=4` is the Vulkan API version, compared against an SM-8.0 bar. The +message is the same one `test_metal_backend.cpp:170` produced in run +[32668677681](https://github.com/mudler/vllm.cpp/actions/runs/32668677681), with +device type 3 (kVULKAN) instead of 2 (kMETAL). One root cause, two platforms. + +**GREEN-AFTER, same host, same build directory.** + +``` +tests/vt/test_vulkan_backend.cpp:503: MESSAGE: kVULKAN Platform::get_device_capability() present=false major=-1 minor=-1; vt::Backend API version 1.4 +tests/vt/test_vulkan_backend.cpp:507: SUCCESS: CHECK( vllm::v1::SelectAttentionBackendName(p) == "FLASH_ATTN" ) is correct! +``` + +The second half of that line is the point: the API version is still 1.4 and still +probed. It moved to the seam that owns Vulkan-unit questions; it was not deleted +to make a test pass. + +| Suite | compile rc | test cases | assertions | Status | `[SKIP]` lines | +|---|---|---|---|---|---| +| `test_vulkan_backend` | 0 | 35 / 35 passed / 0 failed / 0 skipped | 2109 / 2109 passed | SUCCESS! | 0 | +| `test_backend_cross_device` | 0 | 25 / 25 passed / 0 failed / 0 skipped | 80140 / 80140 passed | SUCCESS! | 0 | +| `test_platform` (Vulkan tier) | 0 | 15 / 15 passed / 0 failed / 0 skipped | 118 / 118 passed | SUCCESS! | 0 | + +`src/vllm/platforms/metal.cpp` and `tests/vt/test_metal_backend.cpp` are plain +C++ and both pass `g++ -fsyntax-only -std=c++20` on this Linux host, rc 0. That +is a syntax and type gate, not a build against the real Metal SDK; the executing +verdict comes from `macos-metal-mlx`. + +**Mutation.** Restoring the defective `VulkanPlatform::get_device_capability` +body (1 hunk, `git diff --stat` 21 insertions / 7 deletions, compile rc 0) reds +BOTH new gates, and the tree was restored byte-identical afterwards +(`diff -q` IDENTICAL, both suites green again): + +``` +test_platform: 15 cases | 14 passed | 1 failed Status: FAILURE! +test_vulkan_backend: tests/vt/test_vulkan_backend.cpp:458: ERROR: CHECK_FALSE( p.get_device_capability().present() ) is NOT correct! + tests/vt/test_vulkan_backend.cpp:507: ERROR: ... THREW "compute capability not supported" +``` + +## Outcome + +What the bug actually was, for the next reader: **not a wrong threshold, and not +a platform that "cannot answer".** `DeviceCapability::present()` was doing a job +it cannot do. It reports WHETHER a platform answered and can say nothing about +the UNIT of the answer, and the comment at `backend.cpp:197` read the first as +the second. Two platforms answered helpfully, in units natural to their own +device, and helpfulness was the defect. + +The reason Metal looked fine for four days is worth keeping: Apple family 9 on +the M4 gate box clears `>= (8, 0)`. A gate that passes because two unrelated +integers happened to order correctly is not a passing gate, and the value that +made it pass is the one that hid it. + +Two smaller things fell out of the same discipline. The doctest `-tc` filter +splits on commas, so the new case is named without one — with a comma it reported +`0 cases ran ... SUCCESS!`. And doctest stringifies a bare `char*` as a bool, so +`vt::DeviceTypeName(type)` printed `1` for every platform until it was wrapped in +`std::string`; a diagnostic line that cannot tell two platforms apart is worse +than no line. + +## Stop conditions + +- Stop and return `NEEDS_DECISION` if repairing the unit turns out to require + changing what selection means for kCPU or kTENSTORRENT. It does not: both + already report an absent capability and neither moves. +- Stop if any consumer of `Platform::get_device_capability()` for kMETAL or + kVULKAN is found beyond the selector. None exists at `df1ee2058`. diff --git a/.agents/specs/attn-validate-configuration.md b/.agents/specs/attn-validate-configuration.md index 81dcc45d5..f2b3cceee 100644 --- a/.agents/specs/attn-validate-configuration.md +++ b/.agents/specs/attn-validate-configuration.md @@ -321,6 +321,13 @@ platform that cannot answer, and Metal answers in a different unit; fixed in this flow because every candidate repair changes what backend selection means for kCPU/kMETAL/kVULKAN/kTENSTORRENT, which `AGENTS.md` routes to the normal row, spec and fresh-review path. Owner: `BACKEND-ATTN-REGISTRY`. +**REPAIRED** by [attn-capability-unit.md](attn-capability-unit.md), which took +that path. Measured, it moved two device types and not four: kCPU and +kTENSTORRENT already report an absent capability and did not move. Vulkan +carried the same defect and was worse — the Vulkan API version has `major == 1` +on every device that will ever exist, so FLASH_ATTN was refused there +unconditionally, and the lane's test asserted the priority list rather than the +selector, so nothing saw it. ## Stop conditions diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a9d50e139..e10c96643 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1007,11 +1007,19 @@ jobs: - name: Build # Bounded parallelism for the same reason as build-test-cpu: an unbounded # parallel link OOM-kills the runner. - run: cmake --build build-vulkan -j 2 --target test_vulkan_backend test_backend_cross_device + run: cmake --build build-vulkan -j 2 --target test_vulkan_backend test_backend_cross_device test_platform - name: Vulkan backend gate run: ./build-vulkan/tests/test_vulkan_backend - name: Cross-device numerics vs the CPU oracle run: ./build-vulkan/tests/test_backend_cross_device + - name: Platform seam gate, with a non-CUDA accelerator REGISTERED + # #1823. `test_platform` also runs on build-test-cpu, but that tier has + # only kCPU registered, so the capability-UNIT contract it asserts + # (`get_device_capability()` is an NVIDIA SM value or it is absent) walks + # exactly one platform there and cannot catch the defect it exists for. + # This lane registers kVULKAN, which is where that walk has teeth — and + # kVULKAN is one of the two platforms that carried the defect. + run: ./build-vulkan/tests/test_platform device-leakage: # The DSR RATCHET (work row `S1` of .agents/specs/accelerator-seam-audit.md). # Counts device-specific references in `src/vllm/` + `include/vllm/` — the @@ -1502,17 +1510,21 @@ jobs: # What that run could NOT cover is the #1584 repair, which landed in # 944d7d947, after it. # - # THIS JOB IS RED ON `main` RIGHT NOW, on #1823, and that is what a lane - # which had never executed anything is for. Its first run reported - # `test_metal_backend.cpp:170` THREW `"No valid attention backend for device - # type 2 from {FLASH_ATTN: [compute capability not supported]}"`, because + # THIS JOB'S FIRST RUN WAS RED, ON #1823, and that is what a lane which had + # never executed anything is for. It reported `test_metal_backend.cpp:170` + # THREW `"No valid attention backend for device type 2 from {FLASH_ATTN: + # [compute capability not supported]}"`, because # `FlashAttentionBackend::supports_compute_capability` is upstream's NVIDIA - # `>= (8,0)` while `MetalPlatform::get_device_capability` answers with the + # `>= (8,0)` while `MetalPlatform::get_device_capability` answered with the # Apple GPU FAMILY. Red since 369ea7fd4 (2026-08-19), which is not an - # ancestor of 7020de936, so nothing could see it for four days. It is + # ancestor of 7020de936, so nothing could see it for four days. It was # deliberately NOT hidden behind `continue-on-error`, a skip or a tuned - # floor, and the exactness step runs FIRST so this row's own gate is not - # hostage to it. + # floor, and the exactness step runs FIRST so that row's own gate was not + # hostage to it. FIXED by `.agents/specs/attn-capability-unit.md`: the Metal + # platform now reports an ABSENT capability, mirroring upstream's own answer + # for a foreign capability format (xpu.py:228-236), so the SM predicate is + # skipped rather than misapplied. The four days this went unseen are the + # exposure window this job exists to close, measured. concurrency: group: ci-macos-metal-mlx-${{ github.event_name }}-${{ (github.event_name == 'schedule' || github.event_name == 'workflow_dispatch') && github.run_id || github.ref }}-${{ github.repository }} cancel-in-progress: ${{ github.event_name != 'schedule' && github.event_name != 'workflow_dispatch' }} diff --git a/include/vllm/platforms/interface.h b/include/vllm/platforms/interface.h index 7df26e4e4..127c99270 100644 --- a/include/vllm/platforms/interface.h +++ b/include/vllm/platforms/interface.h @@ -216,7 +216,33 @@ class Platform { // Graph/command capture capability (backend.h:80 SupportsGraphCapture). bool supports_graph_capture() const { return backend().SupportsGraphCapture(); } - // interface.py:409-415 get_device_capability. `present() == false` on CPU. + // interface.py:420-431 get_device_capability. **THE UNIT IS AN NVIDIA SM + // VERSION**, and upstream's docstring says so outright: "Stateless version of + // torch.cuda.get_device_capability". Every predicate written against this value + // — FlashAttentionBackend::supports_compute_capability (flash_attn.py:200-202, + // `>= (8, 0)`), CudaPlatform::supports_fp8 (`has_device_capability(8, 9)`) — is + // a statement about SM versions and about nothing else. + // + // A PLATFORM WITH NO SM VERSION REPORTS ABSENT (`present() == false`). It must + // never answer in a unit of its own, however natural that unit is on the + // device. That is upstream's rule, applied by upstream to itself in + // xpu.py:228-236: "capacity format differs from cuda's and will cause + // unexpected failure, so use None directly". + // + // Absent is also what makes ONE shared selector correct. Upstream reaches + // `validate_configuration` only from CudaPlatform (cuda.py:381,410, guarded by + // `assert device_capability is not None` at :404) and RocmPlatform + // (rocm.py:531,558); every other platform has its own get_attn_backend_cls and + // never evaluates the SM predicate (cpu.py:75-87). Our + // SelectAttentionBackendName is shared across every DeviceType, so what + // upstream gets from its callers we get from this contract: absent skips the + // predicate exactly as upstream's CPU/XPU platforms skip it, present feeds it + // an SM value exactly as upstream's CUDA/ROCm platforms do. + // + // Answering in a foreign unit was #1823: Metal reported the Apple GPU family + // and Vulkan the Vulkan API version, and an SM-8.0 bar was applied to both. + // tests/vllm/platforms/test_platform.cpp gates the contract for every + // registered platform. virtual DeviceCapability get_device_capability() const = 0; // interface.py:417-439 has_device_capability — is this platform >= a required diff --git a/src/vllm/platforms/metal.cpp b/src/vllm/platforms/metal.cpp index 821c125d2..ec77486d5 100644 --- a/src/vllm/platforms/metal.cpp +++ b/src/vllm/platforms/metal.cpp @@ -27,15 +27,28 @@ class MetalPlatform final : public Platform { DeviceType device_type() const override { return DeviceType::kMETAL; } Backend& backend() const override { return vt::GetBackend(DeviceType::kMETAL); } - // interface.py:409-415 get_device_capability. CUDA answers with (sm_major, - // sm_minor); the Apple-silicon analogue is the MTLGPUFamilyApple GENERATION, - // which is what src/vt/metal/metal_context.mm probes and the backend exposes. - // {9, 0} on the M4 gate box. This makes has_device_capability(N, 0) mean - // "Apple family >= N", the same shape of question CUDA code already asks. - DeviceCapability get_device_capability() const override { - Backend& b = backend(); - return DeviceCapability{b.DeviceCapabilityMajor(), b.DeviceCapabilityMinor()}; - } + // interface.py:420-431 get_device_capability, whose docstring defines the UNIT: + // "Stateless version of torch.cuda.get_device_capability". It is an NVIDIA SM + // version, and every predicate written against it — most of all + // FlashAttentionBackend::supports_compute_capability (flash_attn.py:200-202, + // `capability >= (8, 0)`) — is a statement about SM versions. + // + // Apple silicon has no SM version, so the honest answer is ABSENT. This mirrors + // upstream's own answer for the same situation, xpu.py:228-236: "capacity + // format differs from cuda's and will cause unexpected failure, so use None + // directly". + // + // #1823 is what this used to be: it reported the MTLGPUFamilyApple GENERATION + // here, so an SM-8.0 bar was compared against an Apple family number. Family 9 + // on the M4 gate box cleared it by COINCIDENCE, a GitHub macos-15 runner + // reported lower, and FLASH_ATTN — the only entry in + // get_attn_backend_priority() — was refused on Metal's only attention path. + // + // The family number is not lost and was never this seam's to report: it stays + // on vt::Backend::DeviceCapabilityMajor/Minor + // (src/vt/metal/metal_backend.mm:127-130), which is where a Metal-unit question + // belongs and where tests/vt/test_metal_backend.cpp asks it. + DeviceCapability get_device_capability() const override { return DeviceCapability{}; } // interface.py:181-187 supported_dtypes order (bf16 default fallback). Metal 3 // on Apple family 9 handles all three; the kernels in src/vt/metal/metal_msl.h diff --git a/src/vllm/platforms/vulkan.cpp b/src/vllm/platforms/vulkan.cpp index 226972df1..0ca9d38e2 100644 --- a/src/vllm/platforms/vulkan.cpp +++ b/src/vllm/platforms/vulkan.cpp @@ -27,17 +27,28 @@ class VulkanPlatform final : public Platform { DeviceType device_type() const override { return DeviceType::kVULKAN; } Backend& backend() const override { return vt::GetBackend(DeviceType::kVULKAN); } - // interface.py:409-415 get_device_capability. CUDA answers with (sm_major, - // sm_minor) and the Metal skeleton with the Apple GPU family; the Vulkan - // analogue is the API VERSION the physical device reports — {1, 4} on GB10 - // (Vulkan 1.4.312). That makes has_device_capability(1, 1) mean "Vulkan >= 1.1", - // the same shape of question the CUDA code already asks, and it is the version - // the feature gates that matter here (16-bit storage, cooperative matrix, - // subgroup ops) are actually keyed to. - DeviceCapability get_device_capability() const override { - Backend& b = backend(); - return DeviceCapability{b.DeviceCapabilityMajor(), b.DeviceCapabilityMinor()}; - } + // interface.py:420-431 get_device_capability, whose docstring defines the UNIT: + // "Stateless version of torch.cuda.get_device_capability". It is an NVIDIA SM + // version. A Vulkan device has no SM version, so the honest answer is ABSENT — + // upstream's own answer for the same situation, xpu.py:228-236: "capacity + // format differs from cuda's and will cause unexpected failure, so use None + // directly". + // + // #1823, the Vulkan half. This used to report the Vulkan API VERSION ({1, 4} on + // GB10, and 1.x on every Vulkan device that will ever exist), which + // FlashAttentionBackend::supports_compute_capability (flash_attn.py:200-202) + // then compared against `>= (8, 0)`. FLASH_ATTN is the ONLY entry in + // get_attn_backend_priority(), so SelectAttentionBackendName threw on kVULKAN + // unconditionally. Metal at least had a coincidence; this had none, and it was + // invisible because the lane's test asserted that FLASH_ATTN is NAMED in the + // priority list rather than that the selector REACHES it. + // + // The API version is not lost and was never this seam's to report: it stays on + // vt::Backend::DeviceCapabilityMajor/Minor + // (src/vt/vulkan/vulkan_backend.cpp:144-145) and VulkanContext::api_major/minor, + // which is where the 16-bit-storage / cooperative-matrix / subgroup feature + // gates actually read it. + DeviceCapability get_device_capability() const override { return DeviceCapability{}; } // interface.py:181-187 supported_dtypes order (bf16 default fallback). All // three are implemented as STORAGE dtypes by the shaders in diff --git a/src/vllm/v1/attention/backend.cpp b/src/vllm/v1/attention/backend.cpp index 7b4eb0b70..427b38288 100644 --- a/src/vllm/v1/attention/backend.cpp +++ b/src/vllm/v1/attention/backend.cpp @@ -189,11 +189,22 @@ std::vector AttentionBackend::validate_configuration( // CudaPlatform.get_attn_backend_cls asserts `device_capability is not None` // before it calls this (cuda.py:403-404), and CpuPlatform has a separate // selector that never reaches it (cpu.py:75-87). Our selector is shared across - // every DeviceType, and DeviceCapability::present() is already false for every - // platform that cannot answer the question, so the predicate applies exactly - // where upstream applies it. Without this, FLASH_ATTN — which this tree also - // registers for kCPU/kMETAL/kVULKAN/kTENSTORRENT — would be refused on every - // one of them by a rule about NVIDIA compute capability. + // every DeviceType, so the guard is where upstream's caller-side assert lands. + // Without it, FLASH_ATTN — which this tree also registers for + // kCPU/kMETAL/kVULKAN/kTENSTORRENT — would be refused on every one of them by + // a rule about NVIDIA compute capability. + // + // #1823: this comment used to argue that `present()` is "already false for + // every platform that cannot answer the question". THAT WAS NOT TRUE, and it + // was not true of two platforms at once — Metal answered with the Apple GPU + // family and Vulkan with the Vulkan API version, so an SM-8.0 bar was compared + // against numbers that have nothing to do with SM versions. `present()` is a + // guard on WHETHER a platform answers, and it can say nothing about the UNIT. + // The unit is a contract on the value, stated on + // Platform::get_device_capability (include/vllm/platforms/interface.h) and + // gated for every registered platform by + // tests/vllm/platforms/test_platform.cpp. This line is correct only because + // that contract holds. if (capability.present() && !supports_compute_capability(capability)) { invalid_reasons.emplace_back("compute capability not supported"); } diff --git a/tests/vllm/platforms/test_platform.cpp b/tests/vllm/platforms/test_platform.cpp index 548a99ea7..82cdbd24a 100644 --- a/tests/vllm/platforms/test_platform.cpp +++ b/tests/vllm/platforms/test_platform.cpp @@ -153,6 +153,60 @@ TEST_CASE("CurrentPlatform resolves accelerator-first, else falls back to CPU") CHECK(GetPlatform(DeviceType::kCPU).is_cpu()); } +// #1823 — the UNIT contract on Platform::get_device_capability, gated as a CLASS +// rather than as a list of platforms, so a platform added later is covered with +// no edit here. +// +// The value is an NVIDIA SM version (interface.py:420-431, "Stateless version of +// torch.cuda.get_device_capability"), and every predicate written against it says +// something about SM versions — FlashAttentionBackend::supports_compute_capability +// is `>= (8, 0)` (flash_attn.py:200-202) and CudaPlatform::supports_fp8 is +// `has_device_capability(8, 9)`. A platform with no SM version must therefore +// report ABSENT, which is upstream's own answer in xpu.py:228-236 ("capacity +// format differs from cuda's ... so use None directly"). +// +// Metal reported the Apple GPU family here and Vulkan the Vulkan API version. +// Both were compared against an SM-8.0 bar: Metal cleared it by coincidence on an +// M4 and was refused on a lower Apple family, and Vulkan (major 1, forever) was +// refused unconditionally, taking FLASH_ATTN — the only entry in either +// platform's priority list — with it. +// +// This walks the platforms that are REGISTERED in this build, so it has teeth on +// exactly the lane that builds a given backend: the Vulkan lane sees kVULKAN, +// macos-metal-mlx sees kMETAL, a CUDA build sees kCUDA. On the CPU tier it checks +// kCPU alone, and that is stated rather than dressed up. +TEST_CASE("a registered platform answers get_device_capability in SM units or not at all") { + // The SM-unit platforms: their capability is a real compute capability, so it + // is present AND it is a plausible SM version. Everything else must be absent. + size_t checked = 0; + for (size_t i = 0; i < vt::kNumDeviceTypes; ++i) { + const DeviceType type = static_cast(i); + if (!HasPlatform(type)) continue; + ++checked; + const bool sm_unit = (type == DeviceType::kCUDA || type == DeviceType::kROCM); + const auto cap = GetPlatform(type).get_device_capability(); + // Printed unconditionally, not only on failure: the numbers ARE the finding. + MESSAGE("platform " << std::string(vt::DeviceTypeName(type)) + << " get_device_capability()" + << " present=" << cap.present() << " major=" << cap.major + << " minor=" << cap.minor << " sm_unit=" << sm_unit); + if (sm_unit) { + // A CUDA/ROCm platform is only registered when a device was probed, so it + // has an answer, and that answer is an SM (or gfx-derived) version. + CHECK(cap.present()); + CHECK(cap.major >= 1); + } else { + // No SM version exists for this device class. Reporting a number here is + // the #1823 defect, whatever unit that number is in. + CHECK_FALSE(cap.present()); + } + } + // A silent zero-platform walk would pass this case while measuring nothing. + // kCPU is registered on every tier this test builds on. + CHECK(checked >= 1); + CHECK(HasPlatform(DeviceType::kCPU)); +} + // The ratchet for the ONE place a new platform is not additive (BACKEND-ROCM W0, // found while adding kROCM: the platform registered correctly and would never // have been selected, because CurrentPlatform() walks a hardcoded array and no diff --git a/tests/vt/test_metal_backend.cpp b/tests/vt/test_metal_backend.cpp index 5fcdfd81b..e0107f24f 100644 --- a/tests/vt/test_metal_backend.cpp +++ b/tests/vt/test_metal_backend.cpp @@ -150,8 +150,23 @@ TEST_CASE("Metal platform is registered and reports unified/no-pool residency") CHECK(p.is_unified_memory()); CHECK_FALSE(p.supports_graph_capture()); - CHECK(p.get_device_capability().present()); - CHECK(p.get_device_capability().major >= 1); + // #1823. Platform::get_device_capability is an NVIDIA SM version + // (interface.py:420-431), and Apple silicon has no SM version, so the Metal + // platform reports ABSENT — upstream's own answer for a foreign capability + // format, xpu.py:228-236. This assertion used to be `present()`, and that is + // what let FlashAttentionBackend::supports_compute_capability's `>= (8, 0)` + // be applied to an Apple GPU FAMILY number: family 9 on an M4 cleared an + // SM-8.0 bar by coincidence, a lower family on a GitHub macos-15 runner did + // not, and the CHECK below at :170 threw. + CHECK_FALSE(p.get_device_capability().present()); + + // The Apple family is still probed and still reachable — on vt::Backend, which + // is where a Metal-unit question belongs. Asserting it HERE is what keeps the + // fix from being "delete the number": the number is real, it was in the wrong + // seam. ">= 1" rather than "== 9" because the gate must not name one Mac. + Backend& metal_backend = vt::GetBackend(DeviceType::kMETAL); + CHECK(metal_backend.DeviceCapabilityMajor() >= 1); + CHECK(metal_backend.DeviceCapabilityMinor() == 0); // interface.py:181-187 order — bf16 is the default fallback. REQUIRE(p.supported_dtypes().size() == 3); diff --git a/tests/vt/test_vulkan_backend.cpp b/tests/vt/test_vulkan_backend.cpp index 1cec70450..40c2d0e34 100644 --- a/tests/vt/test_vulkan_backend.cpp +++ b/tests/vt/test_vulkan_backend.cpp @@ -28,6 +28,7 @@ #include #include "vllm/platforms/interface.h" +#include "vllm/v1/attention/registry.h" #include "vt/backend.h" #include "vt/ops.h" #include "vt/vulkan/vulkan_context.h" @@ -446,8 +447,23 @@ TEST_CASE("Vulkan platform is registered and reports unified/no-pool residency") CHECK(p.is_unified_memory()); CHECK_FALSE(p.supports_graph_capture()); - CHECK(p.get_device_capability().present()); - CHECK(p.get_device_capability().major >= 1); + // #1823, the Vulkan half. Platform::get_device_capability is an NVIDIA SM + // version (interface.py:420-431); a Vulkan device has no SM version, so this + // platform reports ABSENT — upstream's own answer for a foreign capability + // format, xpu.py:228-236. This assertion used to be `present()`, and the value + // was the Vulkan API version, which + // FlashAttentionBackend::supports_compute_capability then compared against + // `>= (8, 0)`. `major` is 1 on every Vulkan device that will ever exist, so + // FLASH_ATTN was refused unconditionally on this backend. + CHECK_FALSE(p.get_device_capability().present()); + + // The API version is still probed and still reachable — on vt::Backend and + // VulkanContext, which is where the 16-bit-storage / coopmat / subgroup gates + // actually read it. The number was real; it was in the wrong seam. + Backend& vk_backend = vt::GetBackend(DeviceType::kVULKAN); + CHECK(vk_backend.DeviceCapabilityMajor() >= 1); + CHECK((vk_backend.DeviceCapabilityMajor() > 1 || + vk_backend.DeviceCapabilityMinor() >= 1)); // interface.py:181-187 order — bf16 is the default fallback. REQUIRE(p.supported_dtypes().size() == 3); @@ -477,6 +493,19 @@ TEST_CASE("Vulkan platform is registered and reports unified/no-pool residency") mla.use_mla = true; CHECK(p.get_attn_backend_priority(mla).empty()); } + // #1823. Naming FLASH_ATTN in the priority list is NOT the same as the selector + // REACHING it, and that gap is exactly why this defect was invisible on this + // lane: the case above asserted the name and stopped. This is the assertion + // that was missing, on this lane and on Metal's. It resolves only if no + // capability predicate refuses the candidate. + { + const auto cap = p.get_device_capability(); + MESSAGE("kVULKAN Platform::get_device_capability() present=" << cap.present() + << " major=" << cap.major << " minor=" << cap.minor + << "; vt::Backend API version " << vk_backend.DeviceCapabilityMajor() + << "." << vk_backend.DeviceCapabilityMinor()); + CHECK(vllm::v1::SelectAttentionBackendName(p) == "FLASH_ATTN"); + } } TEST_CASE("Vulkan registers the W0 op set and NOT the unimplemented rest") { From b3cc130472e607d16e3bb7ba363bf66c3e177ae9 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 24 Aug 2026 02:34:47 +0000 Subject: [PATCH 2/3] fix(BACKEND-ATTN-REGISTRY): the capability gate proves it saw a device, and two seam headers stop teaching the retired model (#1823) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Repairs the fresh review of the previous commit, which passed the fix on correctness and failed it on the reach the new gate claimed. A device-less Vulkan run passed everything while measuring nothing. `test_vulkan_backend.cpp:442` opens with a silent `if (!VulkanPresent()) return;`, so with the loader pointed at a missing ICD `test_platform` reports 15/15 and `Status: SUCCESS!` having walked kCPU alone, and the platform case reports `test cases: 1 | 1 passed` with `assertions: 0 | 0 passed` — a skip wearing a pass, under a green job. A test cannot tell "no device on this runner" from "CPU tier", but the lane knows which one it is, so the positive control belongs in the lane: both `build-test-vulkan` steps now grep the case's own MESSAGE line, and against those device-less logs the greps return rc 1 while the executables return rc 0. The class gate also did not run on the Metal tier at all, and `tests/vllm/platforms/test_platform.cpp` said it did. `macos-metal-mlx` built `vllm test_metal_backend` and ran only the suite, so "a platform added later is covered with no edit here" was false for the one accelerator tier that is not Vulkan. The lane now builds and runs `test_platform` with the same positive control for `platform metal`, which makes the claim true rather than deleting it. `src/vt/vulkan/vulkan_context.h` and `src/vt/metal/metal_context.h` still told the reader that the API version and the Apple family are "mirrored onto the Platform seam", and that `has_device_capability(1, 1)` reads as "Vulkan >= 1.1". That is the retired model, in the two headers that own the numbers, cross- referencing the two files this row corrected — the same defect class the row exists to repair. Four stale `interface.py` anchors are corrected against the pin. The spec gains the oracle anchor the review found and the implementer missed: `mla/prefill/selector.py:97-102 get_mla_prefill_backend` is upstream's own PLATFORM-AGNOSTIC selector, and on an absent capability it skips every capability predicate and returns FLASH_ATTN. That is the closest structural mirror of `SelectAttentionBackendName` upstream has, and it settles the design independently of the caller-guarantee argument. Its `## Evidence` mutation figure is corrected from 21 insertions / 7 deletions to 4 / 1: the original was measured against `origin/main` before the fix was committed, so it carried the whole change and could not show that the mutation applied. Re-verified on lavapipe: compile rc 0, test_vulkan_backend 35/35 and 2109 assertions SUCCESS! with 0 skipped, test_backend_cross_device 25/25 and 80140 assertions SUCCESS!, test_platform 15/15 and 118 assertions SUCCESS!, and each new ci.yml grep run verbatim at rc 0 with the device present and rc 1 without it. `src/vllm/platforms/metal.cpp` and `tests/vt/test_metal_backend.cpp` both pass `g++ -fsyntax-only -std=c++20` at rc 0. `scripts/agent-preflight.sh --staged` rc 0; the one earlier failure was `test_cpu_x86_llamacpp_floor` at load average 17, which passes 10/10 re-run and is #618. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code] --- .agents/specs/attn-capability-unit.md | 134 +++++++++++++++++++++++-- .github/workflows/ci.yml | 46 ++++++++- include/vllm/platforms/interface.h | 4 +- src/vllm/platforms/cpu.cpp | 3 +- src/vllm/platforms/tenstorrent.cpp | 8 +- src/vt/metal/metal_context.h | 16 ++- src/vt/vulkan/vulkan_context.h | 18 ++-- tests/vllm/platforms/test_platform.cpp | 21 +++- 8 files changed, 219 insertions(+), 31 deletions(-) diff --git a/.agents/specs/attn-capability-unit.md b/.agents/specs/attn-capability-unit.md index 1e853f254..582e65bdb 100644 --- a/.agents/specs/attn-capability-unit.md +++ b/.agents/specs/attn-capability-unit.md @@ -55,8 +55,16 @@ value.** Read at the parity pin `555967922`. 4. `vllm/v1/attention/backends/flash_attn.py:200-202 FlashAttentionBackend.supports_compute_capability` is therefore a statement about NVIDIA SM versions, by construction of who may ask it. -5. **Upstream met this exact defect and recorded the answer.** - `vllm/platforms/xpu.py:228-236 XpuPlatform.get_device_capability`: +5. **No non-CUDA/ROCm upstream platform reports a capability at all.** + `grep -rn "def get_device_capability" vllm/` returns `interface.py:420`, + `xpu.py:228`, `cuda.py:260,734,970` and `rocm.py:699`, and nothing else. + `cpu.py`, `tpu.py` and `zen_cpu.py` inherit the base `return None`. ROCm is + not a counterexample: `rocm.py:700` derives the pair through + `_capability_from_gcn_arch` (`rocm.py:223-238`), which documents itself as + mirroring `hipDeviceProp_t.major/.minor` — the CUDA-shaped API, not a foreign + unit. +6. **Upstream met this exact defect and recorded the answer.** + `vllm/platforms/xpu.py:228-234 XpuPlatform.get_device_capability`: ```python def get_device_capability(cls, device_id: int = 0) -> DeviceCapability | None: @@ -67,6 +75,28 @@ value.** Read at the parity pin `555967922`. A platform whose capability is not in CUDA's unit reports **None**. That is the mirror, in upstream's own words, for the situation this issue describes. + XPU is a real accelerator with a torch-queryable capability, and it declines + anyway, which is what makes it a precedent rather than a convenience. +7. **Upstream already runs a PLATFORM-AGNOSTIC selector on this contract, and it + resolves exactly the way this change makes ours resolve.** + `vllm/v1/attention/backends/mla/prefill/selector.py:97-102 + get_mla_prefill_backend` is reached from `mla_attention.py:519` rather than + from a platform class, so it faces the same problem our shared selector does: + + ```python + device_capability = current_platform.get_device_capability() + if device_capability is None: + logger.info_once( + "Device capability not available, using FlashAttention MLA prefill backend." + ) + return MLAPrefillBackendEnum.FLASH_ATTN.get_class() + ``` + + Absent capability, skip every capability predicate, take FLASH_ATTN. This is + the closest structural mirror of `SelectAttentionBackendName` that exists + upstream, and it settles the design independently of the caller-guarantee + argument above. (This anchor was found by the fresh review, not by the + implementer, and it is the stronger of the two.) **What this tree does differently, and why the rule still lands where upstream puts it.** `SelectAttentionBackendName` @@ -77,7 +107,7 @@ become a per-platform contract on the value: **absent means "no CUDA-SM answer", and the predicate is skipped — which is what upstream's CPU/XPU/TPU platforms get by never reaching `validate_configuration`; present means a CUDA-SM value — which is what upstream's CUDA/ROCm platforms get from `assert device_capability is not -None`.** With that contract the shared selector is behaviourally identical to +None`.** With that contract the shared selector is behaviorally identical to upstream's per-platform selectors. Without it the comparison is undefined, which is the bug. @@ -205,6 +235,25 @@ to make a test pass. | `test_vulkan_backend` | 0 | 35 / 35 passed / 0 failed / 0 skipped | 2109 / 2109 passed | SUCCESS! | 0 | | `test_backend_cross_device` | 0 | 25 / 25 passed / 0 failed / 0 skipped | 80140 / 80140 passed | SUCCESS! | 0 | | `test_platform` (Vulkan tier) | 0 | 15 / 15 passed / 0 failed / 0 skipped | 118 / 118 passed | SUCCESS! | 0 | +| `ctest` full CPU tier (601 tests) | 0 | 601 / 601 passed / 0 failed | — | `100% tests passed, 0 tests failed out of 601` | 3 ctest skips, all pre-existing checkpoint gates | + +**`macos-metal-mlx`, the deliverable, read at JOB level** on dispatched run +[`32681719071`](https://github.com/mudler/vllm.cpp/actions/runs/32681719071) at +`f7c41abdc`: conclusion `success`, and all twelve steps `success` including +"Execute the Metal suite on the runner's Metal device". `test_metal_backend` +reported **26 cases / 26 passed / 0 failed / 3 skipped, 112337 assertions / +112337 passed / 0 failed, `Status: SUCCESS!`, `SKIP lines: 0`**, against the red +run's 26 / 25 passed / **1 failed** / 3 skipped and 112336 assertions. The three +skips are the file's own `doctest::skip(true)` benchmarks, not a device guard. +The assertion count rose by exactly one, which is the arithmetic of replacing two +assertions with three, so the case ran rather than being skipped past. + +Job level was read on purpose. The FIRST dispatch, +[`32681413906`](https://github.com/mudler/vllm.cpp/actions/runs/32681413906), +reports `cancelled` for every job — it was cancelled when the pull request +opened, and a cancelled run renders as a failure in `gh pr checks`. Neither its +red nor its green would have meant anything. + `src/vllm/platforms/metal.cpp` and `tests/vt/test_metal_backend.cpp` are plain C++ and both pass `g++ -fsyntax-only -std=c++20` on this Linux host, rc 0. That @@ -212,9 +261,16 @@ is a syntax and type gate, not a build against the real Metal SDK; the executing verdict comes from `macos-metal-mlx`. **Mutation.** Restoring the defective `VulkanPlatform::get_device_capability` -body (1 hunk, `git diff --stat` 21 insertions / 7 deletions, compile rc 0) reds -BOTH new gates, and the tree was restored byte-identical afterwards -(`diff -q` IDENTICAL, both suites green again): +body (1 hunk, `git diff --stat` 4 insertions / 1 deletion against the committed +file, compile rc 0) reds BOTH new gates, and the tree was restored byte-identical +afterwards (`diff -q` IDENTICAL, both suites green again). + +The implementer first recorded this figure as 21 insertions / 7 deletions, which +does not reproduce: that run measured the mutation against `origin/main` because +the fix was not committed yet, so the diff carried the whole change and not the +mutation. The corrected figure is the fresh review's, measured against the +commit. A mutation figure that silently includes the change being mutated cannot +show that the mutation applied, which is the only reason to print it. ``` test_platform: 15 cases | 14 passed | 1 failed Status: FAILURE! @@ -222,6 +278,72 @@ test_vulkan_backend: tests/vt/test_vulkan_backend.cpp:458: ERROR: CHECK_FALSE( tests/vt/test_vulkan_backend.cpp:507: ERROR: ... THREW "compute capability not supported" ``` +## The gates the fresh review sent back, and their negative control + +The first review of this change returned FAIL. The fix was accepted on +correctness; what failed was the reach the new gate claimed. Both repairs are +recorded here because the measurement is the argument. + +**A device-less Vulkan run passed everything while measuring nothing.** +`tests/vt/test_vulkan_backend.cpp:442` opens with `if (!VulkanPresent()) return;` +— a silent early return, no `[SKIP]`, no counter. Measured on this host by +pointing the loader at a missing ICD: + +``` +$ VK_DRIVER_FILES=/nonexistent.json ./build-vulkan/tests/test_platform +[doctest] test cases: 15 | 15 passed | 0 failed | 0 skipped +[doctest] assertions: 117 | 117 passed | 0 failed | +[doctest] Status: SUCCESS! rc=0 + +$ VK_DRIVER_FILES=/nonexistent.json ./build-vulkan/tests/test_vulkan_backend -tc= -s +[doctest] test cases: 1 | 1 passed | 0 failed | 34 skipped +[doctest] assertions: 0 | 0 passed | 0 failed | +[doctest] Status: SUCCESS! rc=0 +``` + +`assertions: 0 | 0 passed` under `Status: SUCCESS!` is the whole failure mode in +one line, and the job would have been green. The repair is a POSITIVE CONTROL in +the lane rather than another assertion in the process: a test cannot tell "no +device on this runner" from "CPU tier", but the lane knows which one it is. Both +`build-test-vulkan` steps now grep this case's own MESSAGE line, and against the +logs above those greps return rc 1 while the executables return rc 0. With the +lavapipe ICD present they return rc 0 and the case reports 16 assertions. + +**The class gate did not run on the Metal tier at all**, and +`tests/vllm/platforms/test_platform.cpp` claimed it did. `macos-metal-mlx` built +`vllm test_metal_backend` and ran only the suite, so the forward-protection claim +("a platform added later is covered with no edit here") was false for the one +accelerator tier that is not Vulkan. Rather than delete the claim, the lane now +builds and runs `test_platform` with the same MESSAGE-line positive control for +`platform metal`. The Metal half of the contract no longer rests on two +hand-written assertions. + +Three smaller findings are repaired in place: `src/vt/vulkan/vulkan_context.h` +and `src/vt/metal/metal_context.h` still told the reader that the API version and +the Apple family are "mirrored onto the Platform seam" and that +`has_device_capability(1, 1)` reads as "Vulkan >= 1.1" — the retired model, in +the two headers that own the numbers, which is the same defect class this row +exists to repair. Four stale `interface.py` anchors were corrected against the +pin. + +## Reachability + +`SelectAttentionBackendName` is reached in production from +`src/vllm/v1/worker/gpu/runner.cpp:1048` and `:1059`, through +`src/vllm/v1/attention/registry.cpp:74`, which is the call to +`platform.get_device_capability()` this change corrects. The tests drive the REAL +registered platform (`GetPlatform(DeviceType::kVULKAN)` / +`GetPlatform(DeviceType::kMETAL)`) and the real selector, never a mock, so the +gate measures a capability rather than a class. + +Recorded honestly, per `.agents/reachability.md` step 4: deleting the +`runner.cpp:1059` call site in a scratch copy leaves both focused gates green +(compile rc 0, 1 hunk). The mutation does not move because the tests enter one +hop below that entry point, at the selector, and no runner-level gate runs on +lavapipe (`test_runner` is known-red on a CPU build, #1602/#1608). This is a +statement about where the existing gates sit, not a claim that the code is +unreached. + ## Outcome What the bug actually was, for the next reader: **not a wrong threshold, and not diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e10c96643..fcdcc324e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1019,7 +1019,31 @@ jobs: # exactly one platform there and cannot catch the defect it exists for. # This lane registers kVULKAN, which is where that walk has teeth — and # kVULKAN is one of the two platforms that carried the defect. - run: ./build-vulkan/tests/test_platform + # + # THE GREP IS THE POINT, not the exit status. `VulkanPresent()` is a + # silent early return in the suite, so on a runner where the ICD failed + # to install BOTH executables report `Status: SUCCESS!` with rc 0 while + # measuring nothing about Vulkan: test_vulkan_backend drops from 2109 + # assertions to 344, and test_platform walks kCPU alone. A lane whose + # only claim is "kVULKAN is registered here" must prove that, or the + # claim is the thing being tested and nothing else is. + run: | + set -euo pipefail + ./build-vulkan/tests/test_platform | tee platform-gate.log + grep -q 'platform vulkan get_device_capability()' platform-gate.log + grep -q 'Status: SUCCESS!' platform-gate.log + - name: The Vulkan suite MEASURED a device rather than skipping past one + # The same positive control for the suite itself: assert the platform + # case ran its #1823 assertions, which it does not do when + # `VulkanPresent()` is false. + run: | + set -euo pipefail + ./build-vulkan/tests/test_vulkan_backend \ + -tc="Vulkan platform is registered and reports unified/no-pool residency" -s \ + | tee vulkan-platform-case.log + grep -q 'kVULKAN Platform::get_device_capability() present=false' vulkan-platform-case.log + grep -q 'SelectAttentionBackendName(p) == "FLASH_ATTN" ) is correct' vulkan-platform-case.log + grep -qE 'test cases: *1 \| *1 passed' vulkan-platform-case.log device-leakage: # The DSR RATCHET (work row `S1` of .agents/specs/accelerator-seam-audit.md). # Counts device-specific references in `src/vllm/` + `include/vllm/` — the @@ -1565,7 +1589,7 @@ jobs: -DVLLM_CPP_HIP=OFF \ -DVLLM_CPP_TRITON=OFF \ -DVLLM_CPP_VULKAN=OFF - cmake --build build-metal --target vllm test_metal_backend -j 3 + cmake --build build-metal --target vllm test_metal_backend test_platform -j 3 - name: Every Metal translation unit produced an object # The postcondition, not the exit status. A green build proves nothing # if VLLM_CPP_METAL resolved OFF, if a TU left `target_sources`, or if @@ -1711,6 +1735,24 @@ jobs: EOF test "${rc}" -eq 0 grep -qE 'Status: SUCCESS!' metal-suite.log + - name: Platform seam gate, with kMETAL REGISTERED + # #1823. `test_platform` asserts the capability-UNIT contract + # (`Platform::get_device_capability()` is an NVIDIA SM value, or it is + # ABSENT) for every platform REGISTERED in the build it runs in. On + # build-test-cpu that is kCPU alone, and on build-test-vulkan it is kCPU + # plus kVULKAN. Without this step the contract had NO gate on the Metal + # tier at all, and the case's own comment claimed otherwise -- the + # forward-protection claim ("a platform added later is covered with no + # edit") was false for the one accelerator tier that is not Vulkan. + # + # The grep is the point, for the same reason as everywhere else in this + # job: a device-less runner registers no Metal platform, the walk covers + # kCPU only, and the executable still prints `Status: SUCCESS!` with rc 0. + run: | + set -euo pipefail + ./build-metal/tests/test_platform --no-colors=1 2>&1 | tee platform-gate.log + grep -q 'platform metal get_device_capability()' platform-gate.log + grep -q 'Status: SUCCESS!' platform-gate.log baseline-summary: # THE PUBLISHED VERDICT (issue #274, spec .agents/specs/main-verifiability.md). # diff --git a/include/vllm/platforms/interface.h b/include/vllm/platforms/interface.h index 127c99270..55c355b7f 100644 --- a/include/vllm/platforms/interface.h +++ b/include/vllm/platforms/interface.h @@ -245,11 +245,11 @@ class Platform { // registered platform. virtual DeviceCapability get_device_capability() const = 0; - // interface.py:417-439 has_device_capability — is this platform >= a required + // interface.py:433-454 has_device_capability — is this platform >= a required // (major, minor)? False when the platform has no queryable capability (CPU). bool has_device_capability(int major, int minor) const; - // interface.py:441-476 is_device_capability_family — is the device capability + // interface.py:481-495 is_device_capability_family — is the device capability // any .x (CUDA-13 "family" architecture semantics, e.g. 10.x, 11.x, // 12.x)? Argument is a full capability int (e.g. 120), mirroring upstream's // `(current_capability.to_int() // 10) == (capability // 10)`. False when the diff --git a/src/vllm/platforms/cpu.cpp b/src/vllm/platforms/cpu.cpp index 0c5b49cf1..c95d2f9ab 100644 --- a/src/vllm/platforms/cpu.cpp +++ b/src/vllm/platforms/cpu.cpp @@ -13,7 +13,8 @@ class CpuPlatform final : public Platform { DeviceType device_type() const override { return DeviceType::kCPU; } Backend& backend() const override { return vt::GetBackend(DeviceType::kCPU); } - // cpu.py get_device_capability -> None: a CPU has no queryable compute + // cpu.py inherits interface.py:420-431 get_device_capability -> None: a CPU + // has no queryable compute // capability, so has_device_capability(...) is always false. DeviceCapability get_device_capability() const override { return {}; } diff --git a/src/vllm/platforms/tenstorrent.cpp b/src/vllm/platforms/tenstorrent.cpp index 3f733e0bf..95403d900 100644 --- a/src/vllm/platforms/tenstorrent.cpp +++ b/src/vllm/platforms/tenstorrent.cpp @@ -27,10 +27,10 @@ class TenstorrentPlatform final : public Platform { DeviceType device_type() const override { return DeviceType::kTENSTORRENT; } Backend& backend() const override { return vt::GetBackend(DeviceType::kTENSTORRENT); } - // interface.py:409-415 get_device_capability. Tenstorrent's Tensix cores - // have no CUDA-SM-shaped "compute capability" to report; the base {0, 0} - // ("no meaningful compute capability", backend.h) is the honest answer, - // same as CPU. + // interface.py:420-431 get_device_capability, whose unit is an NVIDIA SM + // version. Tenstorrent's Tensix cores have no SM version to report, so ABSENT + // is the honest answer — same as CPU, and the same answer Metal and Vulkan + // give since #1823. DeviceCapability get_device_capability() const override { return DeviceCapability{}; } // OPT-125m runs BF16 weights/activations with F32 logits. The adapter diff --git a/src/vt/metal/metal_context.h b/src/vt/metal/metal_context.h index 012563d27..8ed59c8ad 100644 --- a/src/vt/metal/metal_context.h +++ b/src/vt/metal/metal_context.h @@ -82,11 +82,17 @@ class MetalContext { void* device() const { return device_; } // id void* command_queue() const { return queue_; } // id - // Capability data mirrored onto the Platform seam (see - // src/vllm/platforms/metal.cpp). `family` is the highest MTLGPUFamilyApple - // the device reports (9 on the M4 gate box), which is what we expose as the - // DeviceCapability major/minor pair {family, 0} — the Apple-silicon analogue - // of CUDA's sm_XY. + // The APPLE GPU FAMILY, exposed on vt::Backend and NOWHERE ELSE. `family` is + // the highest MTLGPUFamilyApple the device reports (9 on the M4 gate box), + // reached through vt::Backend::DeviceCapabilityMajor/Minor + // (src/vt/metal/metal_backend.mm). + // + // It is NOT mirrored onto vllm::platforms::Platform, and #1823 is why. That + // seam's get_device_capability() is an NVIDIA SM version by contract + // (interface.py:420-431), so the family is not "the Apple-silicon analogue of + // sm_XY" there — it is a different unit, and this header used to say it was. + // FlashAttentionBackend's `>= (8, 0)` was applied to it; family 9 cleared an + // SM-8.0 bar by coincidence and a lower family did not. int gpu_family_apple() const { return gpu_family_apple_; } size_t max_threads_per_threadgroup() const { return max_tg_threads_; } size_t threadgroup_memory_bytes() const { return tg_mem_bytes_; } diff --git a/src/vt/vulkan/vulkan_context.h b/src/vt/vulkan/vulkan_context.h index 5af8c490d..ff09d0611 100644 --- a/src/vt/vulkan/vulkan_context.h +++ b/src/vt/vulkan/vulkan_context.h @@ -299,13 +299,17 @@ class VulkanContext { void* ScratchData() const { return scratch_mapped_; } static constexpr size_t kScratchBytes = 1024; - // --- Capability data mirrored onto the Platform seam (src/vllm/platforms/ - // vulkan.cpp) and onto vt::Backend. - // The VULKAN API VERSION is what we expose as the DeviceCapability - // major/minor pair — {1, 4} on GB10 (API 1.4.312). CUDA answers this question - // with sm_XY and Metal with the Apple GPU family; the Vulkan analogue is the - // API level, so has_device_capability(1, 1) reads as "Vulkan >= 1.1", the same - // shape of question the CUDA code already asks. + // --- The VULKAN API VERSION, exposed on vt::Backend and NOWHERE ELSE. + // {1, 4} on GB10 (API 1.4.312), reached through + // vt::Backend::DeviceCapabilityMajor/Minor (src/vt/vulkan/vulkan_backend.cpp). + // + // It is NOT mirrored onto vllm::platforms::Platform, and #1823 is why. That + // seam's get_device_capability() is an NVIDIA SM version by contract + // (interface.py:420-431, "Stateless version of torch.cuda.get_device_capability"), + // so `has_device_capability(1, 1)` there does NOT read as "Vulkan >= 1.1" — on + // kVULKAN it is false for every argument, because the platform correctly + // reports ABSENT. This header used to claim the opposite, and the claim cost + // FLASH_ATTN on this backend: an SM-8.0 bar was applied to `major == 1`. // The shared VkQueue, as the opaque handle vt::Queue carries. void* queue_handle() const { return queue_; } diff --git a/tests/vllm/platforms/test_platform.cpp b/tests/vllm/platforms/test_platform.cpp index 82cdbd24a..dd315c0a4 100644 --- a/tests/vllm/platforms/test_platform.cpp +++ b/tests/vllm/platforms/test_platform.cpp @@ -172,9 +172,19 @@ TEST_CASE("CurrentPlatform resolves accelerator-first, else falls back to CPU") // platform's priority list — with it. // // This walks the platforms that are REGISTERED in this build, so it has teeth on -// exactly the lane that builds a given backend: the Vulkan lane sees kVULKAN, -// macos-metal-mlx sees kMETAL, a CUDA build sees kCUDA. On the CPU tier it checks -// kCPU alone, and that is stated rather than dressed up. +// exactly the lane that builds a given backend: `build-test-vulkan` sees kVULKAN, +// `macos-metal-mlx` sees kMETAL, a CUDA build sees kCUDA. On `build-test-cpu` it +// checks kCPU alone, and that is stated rather than dressed up. +// +// WHERE THE TEETH ACTUALLY ARE IS A CI FACT, NOT A C++ ONE, and this case cannot +// assert it. A platform registers only when its device was PROBED, so a runner +// whose Vulkan ICD failed to install, or a macOS runner with no Metal device, +// walks kCPU alone and prints `Status: SUCCESS!` — measuring nothing, loudly +// passing. `checked >= 1` below catches a zeroed walk and NOTHING WEAKER; it is +// deliberately a floor and not a count, because the honest count differs per +// lane. The positive control lives in `.github/workflows/ci.yml`, where both the +// Vulkan and the Metal step grep this case's own MESSAGE line for the platform +// that lane exists to cover. Read those two steps as part of this test. TEST_CASE("a registered platform answers get_device_capability in SM units or not at all") { // The SM-unit platforms: their capability is a real compute capability, so it // is present AND it is a plausible SM version. Everything else must be absent. @@ -202,7 +212,10 @@ TEST_CASE("a registered platform answers get_device_capability in SM units or no } } // A silent zero-platform walk would pass this case while measuring nothing. - // kCPU is registered on every tier this test builds on. + // kCPU is registered on every tier this test builds on. This is a floor on the + // WALK, not on the coverage: on an accelerator lane the honest count is 2, and + // proving THAT is the job of the ci.yml greps named above, because a missing + // device is indistinguishable from a CPU tier from inside this process. CHECK(checked >= 1); CHECK(HasPlatform(DeviceType::kCPU)); } From 4bc4152bb0a95eedc700e5b88bb54607cd5cfa79 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 24 Aug 2026 03:13:36 +0000 Subject: [PATCH 3/3] fix(BACKEND-ATTN-REGISTRY): close the re-review's four anchor and claim findings (#1823) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The re-review of the repair commit returned PASS with four LOW findings. None weakened a gate; all four were statements that do not hold. `src/vt/vulkan/vulkan_context.h:302` had replaced one false claim with a narrower one. "Exposed on vt::Backend and NOWHERE ELSE" is not true of Vulkan: `api_major()/api_minor()` are public on `VulkanContext`, the suite reads them directly, and `src/vllm/platforms/vulkan.cpp:48` says so in the same commit range. It now names both readers. The Metal twin at `metal_context.h:85` is left as written, because `gpu_family_apple()` genuinely has one consumer. The anchor sweep had stopped short. `src/vllm/platforms/platform.cpp:13,24` carried `interface.py:417-439` and `441-476` for the two functions whose DECLARATIONS were corrected in `interface.h`, and a third copy sat in a `TEST_CASE` name in a file this change already edits. The corrected `is_device_capability_family` anchor also overshot: the function is `interface.py:481-493`, and `481-495` runs into the next `@classmethod`. Six comments still wrote `xpu.py:228-236` while the spec had been corrected to `228-234`. The spec claimed the first dispatched run "reports `cancelled` for every job". It cancelled 12; 4 finished `success` and 3 `skipped`. The substantive point is unchanged — `macos-metal-mlx` is among the cancelled, so that run decides nothing — but a figure that does not reproduce is a figure that should not be in an evidence section. The spec also gains the repair head's own gate, read at job and step level on run 32683588981 at b3cc13047: `macos-metal-mlx` success with all ten steps success, the new step 10 printing `platform metal get_device_capability() present=false major=-1 minor=-1 sm_unit=false` beside `platform cpu`, 15 cases / 116 assertions / SUCCESS!. kMETAL registers only when a device is probed, so that line is the contract measured on Apple hardware rather than inferred from a Linux syntax check. Re-verified: compile rc 0, test_platform 15/15 and 118 assertions SUCCESS! with 0 skipped, test_vulkan_backend 35/35 and 2109 assertions SUCCESS! with 0 skipped, the renamed case reachable by `-tc` at 1/1, and both Metal files at `g++ -fsyntax-only` rc 0. `scripts/agent-preflight.sh --staged` rc 0. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code] --- .agents/specs/attn-capability-unit.md | 33 +++++++++++++++++++++++--- include/vllm/platforms/interface.h | 4 ++-- src/vllm/platforms/metal.cpp | 2 +- src/vllm/platforms/platform.cpp | 4 ++-- src/vllm/platforms/vulkan.cpp | 2 +- src/vt/vulkan/vulkan_context.h | 8 ++++--- tests/vllm/platforms/test_platform.cpp | 4 ++-- tests/vt/test_metal_backend.cpp | 2 +- tests/vt/test_vulkan_backend.cpp | 2 +- 9 files changed, 45 insertions(+), 16 deletions(-) diff --git a/.agents/specs/attn-capability-unit.md b/.agents/specs/attn-capability-unit.md index 582e65bdb..4a5a94e9c 100644 --- a/.agents/specs/attn-capability-unit.md +++ b/.agents/specs/attn-capability-unit.md @@ -250,9 +250,10 @@ assertions with three, so the case ran rather than being skipped past. Job level was read on purpose. The FIRST dispatch, [`32681413906`](https://github.com/mudler/vllm.cpp/actions/runs/32681413906), -reports `cancelled` for every job — it was cancelled when the pull request -opened, and a cancelled run renders as a failure in `gh pr checks`. Neither its -red nor its green would have meant anything. +was cancelled when the pull request opened, and `macos-metal-mlx` is one of the +12 jobs it cancelled (4 finished `success`, 3 `skipped`). A cancelled job renders +as a failure in `gh pr checks`, so neither its red nor its green would have meant +anything, and the verdict above is read from the second dispatch instead. `src/vllm/platforms/metal.cpp` and `tests/vt/test_metal_backend.cpp` are plain @@ -326,6 +327,32 @@ the two headers that own the numbers, which is the same defect class this row exists to repair. Four stale `interface.py` anchors were corrected against the pin. +## The repair head, gated on GitHub + +The lane repairs above are themselves gated, on dispatched run +[`32683588981`](https://github.com/mudler/vllm.cpp/actions/runs/32683588981) at +`b3cc13047`, read at JOB and STEP level. + +`macos-metal-mlx` conclusion `success`, all ten steps `success`, including the +new step 10 "Platform seam gate, with kMETAL REGISTERED". Its log carries the +positive control the step exists for: + +``` +tests/vllm/platforms/test_platform.cpp:202: MESSAGE: platform cpu get_device_capability() present=false major=-1 minor=-1 sm_unit=false +tests/vllm/platforms/test_platform.cpp:202: MESSAGE: platform metal get_device_capability() present=false major=-1 minor=-1 sm_unit=false +[doctest] test cases: 15 | 15 passed | 0 failed | 0 skipped +[doctest] assertions: 116 | 116 passed | 0 failed | +[doctest] Status: SUCCESS! +``` + +kMETAL is REGISTERED on that runner, so a Metal device was found, and it reports +`present=false`. That is the fix measured on Apple hardware rather than inferred +from a Linux syntax check, and it is the first time the capability-unit contract +has had any gate at all on the Metal tier. + +`test_metal_backend` on the same run: 26 cases / 26 passed / 0 failed / 3 +skipped, 112337 assertions, `Status: SUCCESS!`, `SKIP lines: 0`. + ## Reachability `SelectAttentionBackendName` is reached in production from diff --git a/include/vllm/platforms/interface.h b/include/vllm/platforms/interface.h index 55c355b7f..1955c2c4c 100644 --- a/include/vllm/platforms/interface.h +++ b/include/vllm/platforms/interface.h @@ -226,7 +226,7 @@ class Platform { // A PLATFORM WITH NO SM VERSION REPORTS ABSENT (`present() == false`). It must // never answer in a unit of its own, however natural that unit is on the // device. That is upstream's rule, applied by upstream to itself in - // xpu.py:228-236: "capacity format differs from cuda's and will cause + // xpu.py:228-234: "capacity format differs from cuda's and will cause // unexpected failure, so use None directly". // // Absent is also what makes ONE shared selector correct. Upstream reaches @@ -249,7 +249,7 @@ class Platform { // (major, minor)? False when the platform has no queryable capability (CPU). bool has_device_capability(int major, int minor) const; - // interface.py:481-495 is_device_capability_family — is the device capability + // interface.py:481-493 is_device_capability_family — is the device capability // any .x (CUDA-13 "family" architecture semantics, e.g. 10.x, 11.x, // 12.x)? Argument is a full capability int (e.g. 120), mirroring upstream's // `(current_capability.to_int() // 10) == (capability // 10)`. False when the diff --git a/src/vllm/platforms/metal.cpp b/src/vllm/platforms/metal.cpp index ec77486d5..0c9da605e 100644 --- a/src/vllm/platforms/metal.cpp +++ b/src/vllm/platforms/metal.cpp @@ -34,7 +34,7 @@ class MetalPlatform final : public Platform { // `capability >= (8, 0)`) — is a statement about SM versions. // // Apple silicon has no SM version, so the honest answer is ABSENT. This mirrors - // upstream's own answer for the same situation, xpu.py:228-236: "capacity + // upstream's own answer for the same situation, xpu.py:228-234: "capacity // format differs from cuda's and will cause unexpected failure, so use None // directly". // diff --git a/src/vllm/platforms/platform.cpp b/src/vllm/platforms/platform.cpp index 75234840a..75ce2e649 100644 --- a/src/vllm/platforms/platform.cpp +++ b/src/vllm/platforms/platform.cpp @@ -10,7 +10,7 @@ namespace vllm::platforms { -// interface.py:417-439 has_device_capability — is this platform's capability >= +// interface.py:433-454 has_device_capability — is this platform's capability >= // the required (major, minor)? Lexicographic on (major, minor), mirroring the // DeviceCapability tuple comparison; false when there is no queryable // capability (get_device_capability() -> None). @@ -21,7 +21,7 @@ bool Platform::has_device_capability(int major, int minor) const { return cap.minor >= minor; } -// interface.py:441-476 is_device_capability_family — is the device capability any +// interface.py:481-493 is_device_capability_family — is the device capability any // .x? Mirrors upstream exactly: `(to_int() // 10) == (capability // 10)`, // so sm_120 and sm_121 both map to the 12.x family. False when there is no // queryable capability (CPU / get_device_capability() -> None). diff --git a/src/vllm/platforms/vulkan.cpp b/src/vllm/platforms/vulkan.cpp index 0ca9d38e2..b7ada61b4 100644 --- a/src/vllm/platforms/vulkan.cpp +++ b/src/vllm/platforms/vulkan.cpp @@ -30,7 +30,7 @@ class VulkanPlatform final : public Platform { // interface.py:420-431 get_device_capability, whose docstring defines the UNIT: // "Stateless version of torch.cuda.get_device_capability". It is an NVIDIA SM // version. A Vulkan device has no SM version, so the honest answer is ABSENT — - // upstream's own answer for the same situation, xpu.py:228-236: "capacity + // upstream's own answer for the same situation, xpu.py:228-234: "capacity // format differs from cuda's and will cause unexpected failure, so use None // directly". // diff --git a/src/vt/vulkan/vulkan_context.h b/src/vt/vulkan/vulkan_context.h index ff09d0611..da21d3fb1 100644 --- a/src/vt/vulkan/vulkan_context.h +++ b/src/vt/vulkan/vulkan_context.h @@ -299,9 +299,11 @@ class VulkanContext { void* ScratchData() const { return scratch_mapped_; } static constexpr size_t kScratchBytes = 1024; - // --- The VULKAN API VERSION, exposed on vt::Backend and NOWHERE ELSE. - // {1, 4} on GB10 (API 1.4.312), reached through - // vt::Backend::DeviceCapabilityMajor/Minor (src/vt/vulkan/vulkan_backend.cpp). + // --- The VULKAN API VERSION. {1, 4} on GB10 (API 1.4.312). It is reached + // through vt::Backend::DeviceCapabilityMajor/Minor + // (src/vt/vulkan/vulkan_backend.cpp:144-145), which forwards to the + // api_major()/api_minor() accessors below; those are public and the suite + // reads them directly (tests/vt/test_vulkan_backend.cpp). // // It is NOT mirrored onto vllm::platforms::Platform, and #1823 is why. That // seam's get_device_capability() is an NVIDIA SM version by contract diff --git a/tests/vllm/platforms/test_platform.cpp b/tests/vllm/platforms/test_platform.cpp index dd315c0a4..90cc8ba4a 100644 --- a/tests/vllm/platforms/test_platform.cpp +++ b/tests/vllm/platforms/test_platform.cpp @@ -162,7 +162,7 @@ TEST_CASE("CurrentPlatform resolves accelerator-first, else falls back to CPU") // something about SM versions — FlashAttentionBackend::supports_compute_capability // is `>= (8, 0)` (flash_attn.py:200-202) and CudaPlatform::supports_fp8 is // `has_device_capability(8, 9)`. A platform with no SM version must therefore -// report ABSENT, which is upstream's own answer in xpu.py:228-236 ("capacity +// report ABSENT, which is upstream's own answer in xpu.py:228-234 ("capacity // format differs from cuda's ... so use None directly"). // // Metal reported the Apple GPU family here and Vulkan the Vulkan API version. @@ -296,7 +296,7 @@ TEST_CASE("has_device_capability tests platform capability >= required") { CHECK_FALSE(sm121.has_device_capability(13, 0)); } -TEST_CASE("is_device_capability_family matches any .x (interface.py:441-476)") { +TEST_CASE("is_device_capability_family matches any .x (interface.py:481-493)") { // sm_120 and sm_121 share the 12.x family; a different major does not. FakeCapabilityPlatform sm121(DeviceCapability{12, 1}); CHECK(sm121.is_device_capability_family(120)); // 121//10 == 120//10 == 12 diff --git a/tests/vt/test_metal_backend.cpp b/tests/vt/test_metal_backend.cpp index e0107f24f..0a283fbcb 100644 --- a/tests/vt/test_metal_backend.cpp +++ b/tests/vt/test_metal_backend.cpp @@ -153,7 +153,7 @@ TEST_CASE("Metal platform is registered and reports unified/no-pool residency") // #1823. Platform::get_device_capability is an NVIDIA SM version // (interface.py:420-431), and Apple silicon has no SM version, so the Metal // platform reports ABSENT — upstream's own answer for a foreign capability - // format, xpu.py:228-236. This assertion used to be `present()`, and that is + // format, xpu.py:228-234. This assertion used to be `present()`, and that is // what let FlashAttentionBackend::supports_compute_capability's `>= (8, 0)` // be applied to an Apple GPU FAMILY number: family 9 on an M4 cleared an // SM-8.0 bar by coincidence, a lower family on a GitHub macos-15 runner did diff --git a/tests/vt/test_vulkan_backend.cpp b/tests/vt/test_vulkan_backend.cpp index 40c2d0e34..c20ac4388 100644 --- a/tests/vt/test_vulkan_backend.cpp +++ b/tests/vt/test_vulkan_backend.cpp @@ -450,7 +450,7 @@ TEST_CASE("Vulkan platform is registered and reports unified/no-pool residency") // #1823, the Vulkan half. Platform::get_device_capability is an NVIDIA SM // version (interface.py:420-431); a Vulkan device has no SM version, so this // platform reports ABSENT — upstream's own answer for a foreign capability - // format, xpu.py:228-236. This assertion used to be `present()`, and the value + // format, xpu.py:228-234. This assertion used to be `present()`, and the value // was the Vulkan API version, which // FlashAttentionBackend::supports_compute_capability then compared against // `>= (8, 0)`. `major` is 1 on every Vulkan device that will ever exist, so