Enable fast ROCm Voxtral decode - #22005
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22005
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 Unclassified FailuresAs of commit f7a058c with merge base 9a2d135 ( UNCLASSIFIED FAILURES - DrCI could not classify the following jobs because the workflow did not run on the merge base. The failures may be pre-existing on trunk or introduced by this PR:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
f2f68c8 to
77f1bf2
Compare
There was a problem hiding this comment.
Pull request overview
Enables automatic packed INT4 matvec decoding for ROCm W4 Voxtral exports while keeping ROCm on standard SDPA after split-K.
Changes:
- Removes the experimental matvec opt-in.
- Updates ROCm export logic, documentation, tests, and backend behavior.
- Disables the gfx1100 ROCm workflow job.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Review findings |
|---|---|
examples/models/voxtral_realtime/run_rocm_e2e.sh |
No final comments supplied. |
examples/models/voxtral_realtime/README.md |
No final comments supplied. |
examples/models/voxtral_realtime/export_voxtral_rt.py |
Critical (1 vote): Add a quality opt-out or validation gate before enabling matvec for all ROCm W4 exports. |
backends/cuda/triton/replacement_pass.py |
No final comments supplied. |
backends/cuda/tests/test_sdpa_splitk_replacement.py |
Moderate (3 votes): Verify Replaced 1 nodes for ROCm standard SDPA, including the assertion around line 165. |
backends/cuda/aoti_packed_int4_tensor.py |
No final comments supplied. |
.github/workflows/rocm.yml |
Moderate (3 votes): Retain equivalent gfx1100 coverage or document and track the missing architecture validation. |
Suppressed comments (1)
backends/cuda/tests/test_sdpa_splitk_replacement.py:170
- As in the threshold case above, the ROCm branch only asserts absence of the split-K log and never confirms that this eligible shape uses standard
triton.sdpa. The test would still pass if the replacement pass stopped replacing this attention node altogether. Add a replacement-count assertion for theexpected == 0case, matchingtest_below_threshold_uses_standard.
expected = 0 if torch.version.hip is not None else 1
self.assertEqual(
len(splitk),
expected,
f"Expected {expected} split-K selections. Log: {msgs}",
)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| needs: [voxtral-run-decision] | ||
| if: | | ||
| needs.voxtral-run-decision.outputs.run-gfx1100 == 'true' && | ||
| false && needs.voxtral-run-decision.outputs.run-gfx1100 == 'true' && |
| expected = 0 if torch.version.hip is not None else 1 | ||
| self.assertEqual( | ||
| len(splitk), | ||
| expected, | ||
| f"Expected {expected} split-K selections. Log: {msgs}", | ||
| ) |
| text_decoder.eval() | ||
|
|
||
| packed_linear_count = 0 | ||
| use_packed_matvec = use_aoti_packed_int4 and qlinear == "4w" |
77f1bf2 to
62f5a84
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (4)
.github/workflows/rocm.yml:230
- Because of
false &&, this job can never run, even when the decision job setsrun-gfx1100forworkflow_dispatchand direct Voxtral changes. That removes the gfx1100 canary while this PR changes the ROCm export path, and the documentation still says gfx1100 is covered; please re-enable the condition before relying on this job, or update the coverage documentation and track the disablement separately.
false && needs.voxtral-run-decision.outputs.run-gfx1100 == 'true' &&
backends/cuda/tests/test_sdpa_splitk_replacement.py:147
- On ROCm,
expectedis 0, so this assertion only proves that no split-K log was emitted; it still passes if the replacement pass failed and left the original SDPA node untouched. The test docstring promises standard SDPA on ROCm, so also assert that one node was replaced (and that the resulting path is standard).
expected = 0 if torch.version.hip is not None else 1
self.assertEqual(
len(splitk),
expected,
f"Expected {expected} split-K selections. Log: {msgs}",
backends/cuda/tests/test_sdpa_splitk_replacement.py:169
- The ROCm branch again sets
expectedto 0 and only checks for absence of split-K, so this large-cache case also passes when no SDPA replacement occurred at all. Add the same replacement assertion here so the test actually covers the promised standard-SDPA path.
expected = 0 if torch.version.hip is not None else 1
self.assertEqual(
len(splitk),
expected,
f"Expected {expected} split-K selections. Log: {msgs}",
examples/models/voxtral_realtime/export_voxtral_rt.py:157
- This makes the previously opt-in BF16-rounded matvec path the default for every ROCm W4 decoder. The removed TODO/README documented greedy-output drift for this fixed-shape path, while the new end-to-end check only verifies that one token appears, so a transcript regression can still pass silently. Please keep this opt-in until the SDPA change is validated against a full expected transcript/quality check, or add that regression check before changing the default.
use_packed_matvec = use_aoti_packed_int4 and qlinear == "4w"
Use the packed INT4 matvec automatically for ROCm W4 decoder exports. Keep ROCm fixed-shape decode on standard SDPA after split-K produced non-finite logits. CUDA and generic packed tensor defaults are unchanged. With assistance from Claude Code and Codex.
Keep the job definition and trigger conditions in place, but force it off while the self-hosted runner teardown remains unstable. With assistance from Claude Code and Codex.
62f5a84 to
f7a058c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.github/workflows/rocm.yml:230
false &&unconditionally prevents this job from running, including onworkflow_dispatch, so the gfx1100 end-to-end path no longer receives any validation for this ROCm W4 change. If runner teardown instability requires a temporary pause, please use an explicitly temporary gate and restore the actual decision condition before landing rather than committing an unconditional skip.
false && needs.voxtral-run-decision.outputs.run-gfx1100 == 'true' &&
examples/models/voxtral_realtime/export_voxtral_rt.py:170
- This makes the previously opt-in
int4_matvec_bf16path the default for every ROCm W4 decoder. The removed README/TODO documented greedy transcript drift, and the kernel still uses BF16-rounded weights and a different reduction order, so new ROCm W4 exports can silently change transcription quality. Please keep this opt-in until parity/quality is demonstrated, or add an explicit accuracy gate/fallback before enabling it globally.
if use_packed_matvec:
packed_linear_count = _pack_aoti_int4_weights(
text_decoder,
use_matvec=True,
)
|
The speedup here is real and the description undersells it. Comparing the gfx950 My concern is that four separate changes are bundled here and three of them are 1. The split-K guard is not a ROCm issue.
The guard is also narrower than it looks.
So on ROCm it stops one of five paths. If it lands ahead of #22133, could the Related: 2. The greedy-output drift. This removes the TODO in The gfx950 logs suggest the drift is still there. Same clip, same model, same Some of that is better (punctuation, casing) and some is worse ("Leighton" to Was the drift resolved, or is it being accepted? Either answer is fine, it just 3. Disabling the gfx1100 job.
Could this go in its own PR with a tracking issue and an owner? "Temporarily" plus 4. Flag removal.
Small one: the README diff removes the On CI: the |
1 - yes, this is something needs fixing on cuda side first see #22133 Thanks for the feedback :) |
Use the packed INT4 matvec automatically for ROCm W4 decoder exports. Keep ROCm fixed-shape decode on standard SDPA after split-K. CUDA and generic packed tensor defaults are unchanged.