Repository navigation
qwen27: keep's gather and scatter stage the binding's KV heads a row, not 4 - #2013
Conversation
|
Thanks @cshintov. An independent review on current main (d8b80bd): Applying it: both commits apply cleanly on main d8b80bd. The lean check stays at 424 at each commit. Both author Main's bug
The fix is exact
Which shapes it matters on
The PR body names only the rank case. The 32-over-2 one-Mac shape had the same bug and gets the same fix. The body's Tests and the contract
Small addition (not blocking)Register the keep at the other two admitted shapes too: 32/2 (the one-Mac GQA16 case this also fixes) and 16/4. Today Device cells
|
|
Thanks @cshintov. The Metal kernel contract at this head ( |
|
Thanks @ashhart, and for catching the 32-over-2 one-Mac shape: the same overrun, the same fix. The keep registrations at 32/2 and 16/4 are ready as one test-only commit (16 of 16 kernels pass on an M4 Pro, both controls red; with main's kernels the 32/2 keep fails and 16/4 passes). I'll send it as a follow-up once this lands, so your M5 run on this head stands. |
…d a rank's 12 over 2 Keep checks the staging rows where Scratch sized them, and a device keep (one window, all rows dispatched, the map as tree_commit writes it, half the rows and one kept) runs beside it. On a rank both keeps fail: gather and scatter index 4 heads a staging row in staging sized for 2.
… not 4 Scratch sizes keep_k and keep_v at rows * kv_heads, but the kernels indexed row * 4 + head, so on a rank (12 over 2) a keep of more than half the frame's rows wrote past them. The 27B on one Mac is unchanged. The contract's keep control plants the 4 back in both kernels and must fail.
df7e488 to
a3efe3c
Compare
|
Thank you @cshintov. This is merged into main with your commits under your own name, so you're listed as a TensorFold contributor. It ships in the next release, credited to you in the changelog and the release notes. |
|
Thanks @cshintov! This is in TensorFold 1.2.2, out now: https://github.com/ashhart/TensorFold/releases/tag/v1.2.2 |
What this changes
On a tensor-parallel rank of the 27B (12 query heads over 2 KV heads, from #1433's rank slice), keep's gather and scatter indexed 4 heads a staging row, while Scratch sizes
keep_kandkeep_vat the binding's 2. A keep that accepts more than half the frame's rows wrote past both buffers. Now the two kernels index by the binding's KV heads; the 27B on one Mac computes the same indices as before.atc overlap --json: atc is not installed here, so I rangit merge-treeagainst every open pull request touching these five files (30). This adds a conflict with 4, none with the change itself:zig/tests/contract/metal_main.zig(adjacent appends).zig/src/families/qwen27/attention.zig, the args structs next toPromptArgs(adjacent lines).Commits:
contract: qwen27 attention_io's pack, append and keep at 24 over 4 and a rank's 12 over 2qwen27: keep's gather and scatter stage the binding's KV heads a row, not 4Where it reaches. The host keep and the device keep alike: both stage only the accepted rows (
tree_commit.metalmarks the rest0xffffffff, and gather and scatter return on them). With default frames (512 rows and up) a round accepts too few rows to reach it; a smallerTF_PROMPT_ROWSframe does. Gather and scatter used the same wrong index, so the caches came out right and only the staging overran, which is why no existing check saw it.Design.
Q27KeepArgs { kept, kv_heads }in place ofkept, and indexrow * kv_heads + head.Attention.keepfills it from the binding's geometry, and refuses a scratch whose KV heads differ from the binding's, since the staging belongs to the scratch.Alternatives rejected.
q27_attn_appendalready takeskv_headsinQ27PackArgs.Checks on this head
python3 tools/zig/lean_check.py: 424 problems on the base, 424 on this head, 0 new.zig build test-qwen27,test-qwen27-keep-host,test-qwen27-plans-hostandtest-kernel-contractpass;zig fmt --check build.zig zigpasses.tree_commit's metadata kernel (the contract writes its map in the same layout, read fromtree_commit.metal).Receipt
Kernel contract, only when a GPU kernel is new or changed
(i * kv_heads + h) * 256); the device keep keeps exactlyrows / 2 + 1rows of one chain, with the map as tree_commit writes it.SUMMARYlines (tf-kernel-contract --only qwen27):MTL_SHADER_VALIDATION=1The limits check covers 481 Metal launches against M1 to M5 with 0 problems. Exit status 0 on the fix commit.
Checklist
"draft": false, a resumed prompt equals a fresh one, and concurrent equals solo: not run with weights; on one Mac the indices are unchanged.