Repository navigation
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@Ying1123 @merrymercy @hnyls2002 Could you please take a look at this PR when you have a chance? Thanks! |
mmangkad
left a comment
There was a problem hiding this comment.
- I think this needs a rebase onto #41790. After that the fix might be pretty small: the gather kernel already copies any dtype, so maybe just relax the bf16 assert in
gather_cast_kv_fp8_pad_pagedfor NoPE, and let_validate_flashmla_sparse_q8_backendaccept the unpacked fp8 pool when rope=0 and decode is tilelang. - Maybe update the GLM-5.3-Flash cookbook too, since it still says FP8 KV doesn't work with q8 prefill.
- Could you rerun GSM8K with
sgl-eval, with prefix caching on?
| if ( | ||
| dsa_impl == "flashmla_sparse_q8" | ||
| and topk_transform_method == TopkTransformMethod.PAGED | ||
| and not self.dsa_kv_cache_store_fp8 | ||
| and kv_cache.shape[-1] == 512 | ||
| ): |
There was a problem hiding this comment.
I think after #41790 this never runs, since q8 prefill is always RAGGED now, and the launch command actually fails at startup in _validate_flashmla_sparse_q8_backend. Maybe move this into the RAGGED path?
There was a problem hiding this comment.
I think after #41790 this never runs, since q8 prefill is always RAGGED now, and the launch command actually fails at startup in
_validate_flashmla_sparse_q8_backend. Maybe move this into the RAGGED path?
Done. Removed the PAGED fallback. Raw FP8 Q8 prefill now reuses the RAGGED path introduced in #41790.
Updated _validate_flashmla_sparse_q8_backend to accept only the supported raw FP8 configuration: NoPE, kv_lora_rank=512, TileLang decode, and SM90.
| return self._forward_flashmla_sparse_q8kv8( | ||
| q_nope=q_nope, | ||
| q_rope=q_rope, | ||
| kv_bf16=kv_cache.view(-1, 1, 512), |
There was a problem hiding this comment.
I guess this copies the whole layer pool into _q8kv8_kv_buf on every prefill layer? Maybe gather just the needed slots via page_table_1_flattened?
There was a problem hiding this comment.
I guess this copies the whole layer pool into
_q8kv8_kv_bufon every prefill layer? Maybe gather just the needed slots viapage_table_1_flattened?
Done. Removed the whole-pool copy.
Prefix requests now use page_table_1_flattened to gather only the referenced context slots through gather_cast_kv_fp8_pad_paged. For raw FP8 NoPE rows, this copies the FP8 values without scale conversion.
| # concat + .to(fp8)); it requires power-of-two head/dim counts (a | ||
| # tl.arange constraint), so non-power-of-two head counts fall back to | ||
| # the generic concat + cast. | ||
| elif q_rope.shape[-1] == 0: |
There was a problem hiding this comment.
I think concat_and_cast_q_fp8_pad already handles ROPE=0 on main, so maybe this isn't needed?
There was a problem hiding this comment.
I think
concat_and_cast_q_fp8_padalready handles ROPE=0 on main, so maybe this isn't needed?
Done. Removed the separate NoPE query-preparation branch and reused concat_and_cast_q_fp8_pad.
The ROPE=0 and ROPE>0 GPU regression tests both pass.
| topk_indices is None | ||
| or self.dsa_index_kpool <= 1 | ||
| or dsa_impl in ("fa3", "tilelang", "trtllm") | ||
| or (dsa_impl == "flashmla_sparse_q8" and phase == "prefill") |
There was a problem hiding this comment.
I think this opens up packed fp8 too, which main keeps closed on purpose, and it breaks test_kpool_tail_guard_still_rejects_q8_over_a_packed_fp8_pool. Maybe drop it?
There was a problem hiding this comment.
I think this opens up packed fp8 too, which main keeps closed on purpose, and it breaks
test_kpool_tail_guard_still_rejects_q8_over_a_packed_fp8_pool. Maybe drop it?
Done. Removed the unconditional Q8 prefill allowance and restored the packed FP8 KPool guard.
test_kpool_tail_guard_still_rejects_q8_over_a_packed_fp8_pool now passes.
| raw_fp8_nope=( | ||
| model_arch == "Glm5NextForConditionalGeneration" | ||
| and model_config_of(view).qk_rope_head_dim == 0 | ||
| and model_config_of(view).kv_lora_rank == 512 | ||
| ), |
There was a problem hiding this comment.
Maybe check the layout instead of the model name, and share one helper with calculate_mla_kv_cache_dim?
There was a problem hiding this comment.
Maybe check the layout instead of the model name, and share one helper with
calculate_mla_kv_cache_dim?
Done. Added is_q8_raw_fp8_nope_layout as a shared predicate for argument validation, backend validation, and KV pool allocation.
It checks the backend pairing and layout geometry rather than the model architecture name.
667c4af to
06404b9
Compare
This comment has been minimized.
This comment has been minimized.
Thanks for the detailed review! I’ve addressed the comments and rebased this branch onto main, including #41790. Changes:
Validation:
I also reran GSM8K using sgl-eval with prefix caching enabled on the server:
The average input length was 94 tokens, so this GSM8K run is an end-to-end accuracy sanity check rather than evidence of long-prefix Q8 sparse-prefill coverage. The GPU regression tests separately cover prefix gathering and numerical agreement with the reference. Could you please take another look when you have time? Thanks again for your feedback! |
|
@mmangkad could you help me take a look at this PR again?Thanks! |
1 similar comment
|
@mmangkad could you help me take a look at this PR again?Thanks! |
| # Generated by ACS2 SKILL - Ant International Security | ||
| """Shared layout checks for DSA backend validation and KV pool allocation.""" | ||
|
|
||
| from typing import Optional | ||
|
|
||
|
|
||
| def is_q8_raw_fp8_nope_layout( |
There was a problem hiding this comment.
Move this helper into configs/model_config.py next to is_deepseek_dsa. A whole new file for one small function is a bit much
There was a problem hiding this comment.
Move this helper into
configs/model_config.pynext tois_deepseek_dsa. A whole new file for one small function is a bit much
Done. Moved is_q8_raw_fp8_nope_layout next to is_deepseek_dsa in configs/model_config.py and removed kv_layout.py. Updated all imports; overrides.py uses a function-local import to avoid a circular dependency. Both import orders were checked successfully.
| kv_lora_rank=getattr(model_config_of(view), "kv_lora_rank", None), | ||
| qk_rope_head_dim=getattr(model_config_of(view), "qk_rope_head_dim", None), |
There was a problem hiding this comment.
Do not use getattr. I think DSA models always have these, so maybe just access them directly?
There was a problem hiding this comment.
Do not use
getattr. I think DSA models always have these, so maybe just access them directly?
Done. Replaced getattr with direct access to kv_lora_rank and qk_rope_head_dim in both the normal backend-resolution path and the HiSparse path.
| kv_lora_rank: Optional[int] = None, | ||
| qk_rope_head_dim: Optional[int] = None, |
There was a problem hiding this comment.
Same here, make these plain required ints (and in the validator at dsa_backend.py L423)
There was a problem hiding this comment.
Same here, make these plain required
ints (and in the validator atdsa_backend.pyL423)
Done. Made kv_lora_rank and qk_rope_head_dim required int parameters in the shared layout predicate and backend validators. Updated the call sites and test mocks to provide the geometry explicitly.
| (512, 64, "tilelang", False), | ||
| (512, 0, "trtllm", False), | ||
| (512, 0, "flashmla_kv", False), | ||
| (None, 0, "tilelang", False), |
There was a problem hiding this comment.
This (and the (None, None) case in test_dsa_tilelang_fp8_validation.py) can go once the args are required
There was a problem hiding this comment.
This (and the
(None, None)case intest_dsa_tilelang_fp8_validation.py) can go once the args are required
Done. Removed the None geometry cases from both test files. Kept the rejection coverage for unsupported dimensions, nonzero RoPE, incompatible decode backends, and unsupported serving combinations.
| ) -> None: | ||
| # q8 prefill consumes the tail columns as ragged ids, like the history | ||
| # picks. Only over a bfloat16 pool: gathering packed fp8 rows through | ||
| # picks over a bfloat16 or raw NoPE fp8 pool: gathering packed fp8 rows through |
There was a problem hiding this comment.
Nit: I think this lost the "Only", so it reads a bit off now. Maybe # picks. Only over a bf16 or raw NoPE fp8 pool: gathering packed?
There was a problem hiding this comment.
Nit: I think this lost the "Only", so it reads a bit off now. Maybe
# picks. Only over a bf16 or raw NoPE fp8 pool: gathering packed?
Fixed the wording to explicitly say “Only over a bf16 or raw NoPE fp8 pool.” The packed FP8 guard is unchanged.
| On Hopper, BF16 KV can also run the sparse prefill in FP8 with `--dsa-prefill-backend flashmla_sparse_q8` (decode stays on `tilelang` and the cache stays BF16): on H200 this pairing measured 5.8–12.1% higher throughput on prefill-only requests, with GSM8K accuracy within noise of BF16 + TileLang. It is SM90-only and requires a build that includes [PR #41790](https://github.com/sgl-project/sglang/pull/41790); FP8 KV is not available with it for this model. | ||
| On Hopper, BF16 KV can also run the sparse prefill in FP8 with `--dsa-prefill-backend flashmla_sparse_q8` (decode stays on `tilelang` and the cache stays BF16): on H200 this pairing measured 5.8–12.1% higher throughput on prefill-only requests, with GSM8K accuracy within noise of BF16 + TileLang. It is SM90-only and requires a build that includes [PR #41790](https://github.com/sgl-project/sglang/pull/41790). | ||
|
|
||
| With the raw FP8 KV support in [PR #42957](https://github.com/sgl-project/sglang/pull/42957) and [PR #44217](https://github.com/sgl-project/sglang/pull/44217), you can also pair Q8 prefill with `--kv-cache-dtype fp8_e4m3` and TileLang decode on CUDA H100/H200 (SM90). This path requires NoPE MLA (`qk_rope_head_dim=0`, `kv_lora_rank=512`) and gathers only the requested prefix rows from the raw FP8 pool. It does not use the scaled/packed FP8 layout. Keep prefix caching enabled; HiSparse, mixed chunks, and `--dcp-size > 1` are not supported with this pairing. The BF16 benchmark results above do not measure this FP8 KV configuration. |
There was a problem hiding this comment.
Why keep prefix caching on? I think it works fine without cache hits too, so maybe drop that part or say why
There was a problem hiding this comment.
Why keep prefix caching on? I think it works fine without cache hits too, so maybe drop that part or say why
Agreed. Removed the mandatory prefix-caching instruction from the cookbook. Prefix caching was enabled for validation, but that should not have been presented as a requirement for using this backend.
Preview approval requiredThis pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit |
This comment has been minimized.
This comment has been minimized.
|
Thanks for the follow-up review! All six comments have been addressed. Could you please take another look when you have a chance? Thanks!@mmangkad |
|
Required checks failed on 6b1ac36; this PR cannot be merged until they pass. PR title (run)
|
Motivation
Enable flashmla_sparse_q8 DSA backend for prefill on CUDA, import performents for models such as GLM-5.3-Flash.
Modifications
flashmla_sparse_q8prefill with TileLang decode for GLM-5.3-Flash’s raw FP8 KV layout on SM90, restricted to NoPE models withkv_lora_rank=512.-1sentinels, preserving all valid tail tokens.Depends on CUDA TileLang FP8 KV support from #42957.
Accuracy Tests
Validated GLM-5.3-Flash on 4 × H200 with flashmla_sparse_q8 and TileLang prefill .
GSM8K: 1,314 scored questions per run, 5-shot, temperature 0
Speed Tests and Profiling
4 × H200, TP4/EP4, TileLang DSA, triton MoE, EAGLE 5/1/6 and Prefill BCG,kv-cache-dtype fp8_e4m3. Only --dsa-prefill-backend changes between cases.
Checklist
Review and Merge Process
/tag-and-rerun-ci,/tag-run-ci-label,/rerun-failed-ciCI States
Latest PR Test (Base): ❌ Run #40813492287
Latest PR Test (Extra): ❌ Run #40813492239
Latest PR Test (AMD ROCm 10): ❌ Run #40813492251