diff --git a/.agents/specs/attn-capability-unit.md b/.agents/specs/attn-capability-unit.md new file mode 100644 index 000000000..4a5a94e9c --- /dev/null +++ b/.agents/specs/attn-capability-unit.md @@ -0,0 +1,401 @@ +# 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. **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: + # 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. + 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` +(`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 behaviorally 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 | +| `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), +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 +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` 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! +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" +``` + +## 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. + +## 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 +`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 +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 62fd997b9..77d8646e7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1071,11 +1071,43 @@ 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. + # + # 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 @@ -1566,17 +1598,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' }} @@ -1617,7 +1653,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 @@ -1763,6 +1799,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 7df26e4e4..1955c2c4c 100644 --- a/include/vllm/platforms/interface.h +++ b/include/vllm/platforms/interface.h @@ -216,14 +216,40 @@ 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-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 + // `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 + // 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-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/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/metal.cpp b/src/vllm/platforms/metal.cpp index 821c125d2..0c9da605e 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-234: "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/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/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/vllm/platforms/vulkan.cpp b/src/vllm/platforms/vulkan.cpp index 226972df1..b7ada61b4 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-234: "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/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..da21d3fb1 100644 --- a/src/vt/vulkan/vulkan_context.h +++ b/src/vt/vulkan/vulkan_context.h @@ -299,13 +299,19 @@ 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. {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 + // (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 548a99ea7..90cc8ba4a 100644 --- a/tests/vllm/platforms/test_platform.cpp +++ b/tests/vllm/platforms/test_platform.cpp @@ -153,6 +153,73 @@ 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-234 ("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: `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. + 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. 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)); +} + // 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 @@ -229,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 5fcdfd81b..0a283fbcb 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-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 + // 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..c20ac4388 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-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 + // 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") {