Add layer-wise KV-cache AutoQuant with forward KL - #2211
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
Assisted-by: OpenAI Codex Signed-off-by: weimingc <17592131+meenchen@users.noreply.github.com>
Signed-off-by: weimingc <17592131+meenchen@users.noreply.github.com>
Signed-off-by: weimingc <17592131+meenchen@users.noreply.github.com>
Signed-off-by: weimingc <17592131+meenchen@users.noreply.github.com>
c55d772 to
d26d912
Compare
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2211 +/- ##
==========================================
- Coverage 78.94% 78.70% -0.25%
==========================================
Files 522 524 +2
Lines 60550 62255 +1705
==========================================
+ Hits 47803 48996 +1193
- Misses 12747 13259 +512
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: weimingc <17592131+meenchen@users.noreply.github.com>
Signed-off-by: weimingc <17592131+meenchen@users.noreply.github.com>
Signed-off-by: weimingc <17592131+meenchen@users.noreply.github.com>
Signed-off-by: weimingc <17592131+meenchen@users.noreply.github.com>
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (gpt-5.6-sol) — DM the bot to share feedback.
The feature solves a real gap: choosing one calibrated K/V format per attention layer under a KV-storage budget and exporting that heterogeneous mapping. However, the architectural choice is not fully justified. The repo already has AutoQuantizeKLDivSearcher for isolated forward-KL scoring, QuantRecipeHparam for swapping per-choice quantizers/grouped decisions, and BaseSearcher for checkpoint/signature lifecycle, with the existing PuLP-backed LPS already used here. The PR body explains why the current weight cost model cannot be used unchanged, but not why those abstractions cannot be generalized or subclassed with K/V quantizer attributes and a pluggable width-based cost model instead of adding a parallel 673-line search/checkpoint engine. Please document that tradeoff before approval.
I also found a storage-accounting bug for the supported asymmetric FP8-K/NVFP4-V candidate: its effective bits are averaged equally between K and V, while layers are weighted by the sum of potentially unequal K/V widths. This can violate kv_effective_bits; the existing width test even demonstrates unequal K/V projections, but no asymmetric test covers the case. The shipped recipe also imports cost_excluded_layers, while the KV recipe-to-API path silently drops that field, including *mtp*.
Test coverage is otherwise substantial, and the new source headers match LICENSE_HEADER. At +2,179/-61 across 20 files, this is also a high-risk review size; splitting the search/API work from export metadata where practical would make validation easier.
Signed-off-by: weimingc <17592131+meenchen@users.noreply.github.com>
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (gpt-5.6-sol) — DM the bot to share feedback.
Re-review: the feature addresses a real gap—selecting one calibrated K/V format per attention layer under a KV-storage budget and exporting the heterogeneous mapping—but all three prior blocking concerns remain.
- Design (approval-blocking): the updated PR body explains why the existing weight cost model cannot be used unchanged, but it still does not explain why the existing
AutoQuantizeKLDivSearcherisolated forward-KL flow,QuantRecipeHparamchoice swapping/grouping, andBaseSearchercheckpoint lifecycle cannot be generalized for K/V quantizer attributes and a pluggable width-based cost model. The already-imported PuLP alternative is exposed throughLPSand is appropriately reused. Before approval, please document why extending/subclassing those in-repo abstractions is not viable instead of adding a parallel 684-line search/checkpoint implementation. - Critical correctness: asymmetric FP8-K/NVFP4-V accounting still averages K/V bits and then multiplies by total width, so unequal K/V widths can violate the advertised
kv_effective_bitsconstraint. Removing this candidate from the shipped recipe limits exposure but does not fix the public API, which still accepts and advertises it; the new test still uses equal K/V widths. - Critical recipe/API mismatch: the shipped recipe still imports
base_cost_excluded_layers, but the KV recipe conversion returns before forwarding that field.*mtp*is not inbase_disabled_layers, so the recipe can search and budget MTP attention despite appearing to exclude it.
The new source/test headers match LICENSE_HEADER, and test coverage is otherwise substantial. At 22 files and +2,357/-67, the PR remains difficult to validate as one unit; splitting search/API from export metadata would reduce risk.
Signed-off-by: weimingc <17592131+meenchen@users.noreply.github.com>
|
Addressed the latest review in commit
Validation: 38 focused changed-area tests passed; all changed-file pre-commit hooks passed, including recipe validation. |
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (gpt-5.6-sol) — DM the bot to share feedback.
Re-review/design review: this solves a real gap—choosing one calibrated K/V format per attention boundary under a width-weighted cache budget and exporting the heterogeneous mapping. I checked the existing alternatives: AutoQuantizeKLDivSearcher/_AutoQuantizeBaseSearcher already provide isolated forward-KL search flow, QuantRecipeHparam provides candidate quantizer swapping/grouping, and BaseSearcher provides checkpoint lifecycle; the already-imported PuLP path is exposed through LPS and is reused here. Pydantic/OmegaConf cover schema/config composition but not the model-side search itself.
💬 Author replied in commit 34a249597 that the existing abstractions are tied to linear-module grouping, parameter-numel cost, and weight-search checkpoint contracts, and that generalizing them risks changing existing AutoQuant behavior. The PR body now records that tradeoff, so the deterministic design gate is substantially addressed; however, a parallel 714-line search/checkpoint implementation beside those existing abstractions still warrants owner/architect sign-off rather than automated approval.
💬 Addressed in 34a249597: asymmetric FP8-K/NVFP4-V now fails before mutation when any eligible layer has unequal K/V widths, with a focused unequal-width regression. This resolves the prior storage-accounting correctness issue.
💬 Addressed in 34a249597: the KV recipe removes unsupported cost_excluded_layers, moves *mtp* into disabled_layers, validation rejects the unsupported field, and recipe-to-public-API coverage verifies forwarding. This resolves the prior recipe/API mismatch.
Test coverage is substantial and the new-file headers match LICENSE_HEADER. The remaining concern is reviewability and architectural ownership: at 22 files and +2,426/-71, splitting the search/API work from export metadata would materially reduce risk if practical.
Signed-off-by: weimingc <17592131+meenchen@users.noreply.github.com>
|
Addressed the latest findings in signed+DCO commit
Evidence:
|
Signed-off-by: weimingc <17592131+meenchen@users.noreply.github.com>
What does this PR do?
Type of change: new feature.
Adds layer-wise KV-cache AutoQuantize with isolated forward-KL sensitivity:
mtq.auto_quantize_kv_cache, using BF16/no-KV-quant logits as the reference and quantizing one eligible attention layer at a time;constraints.kv_effective_bits, with caller-declared packed cost per K/V scalar;examples/hf_ptq;The existing weight-only
AutoQuantizeKLDivSearcherandQuantRecipeHparamoperate on linear-module weight/input/output quantizer recipes. Their grouping follows runtime-fusion rules and their cost is derived from parameternumel. KV search instead owns one joint K/V choice at an attention boundary, costs resident K/V scalar widths, and checkpoints calibrated K/V scale tensors together with per-layer search progress. Generalizing the existing hparam would therefore change its grouping, cost, and checkpoint contracts and risk existing weight AutoQuant behavior; subclassingBaseSearcherwould not remove those KV-specific responsibilities. The KV path remains separate while reusing the commonLPSsolver, calibration/quantizer utilities, andsafe_load/safe_saveprimitives, so the weight search API and checkpoints stay unchanged.Model-family support status
Support remains architecture-driven: there are no model-name checks, checkpoint paths, fixed layer lists, or campaign flags.
config.architectures = None, as observed with this family.Additional fail-closed behavior:
disabled_layerscontinue to preserve fixed or unsupported layers;fp8_k_nvfp4_v; FP8 and NVFP4 preset names are unchanged.cost_excluded_layers; non-KV-cache modules such as MTP must usedisabled_layers, which is passed to the public KV AutoQuant API and removes them from both scoring and budget accounting.Relationship to vLLM runtime support
This PR is the checkpoint producer: it searches the recipe and writes schema-v1
kv_cache_quantized_layersmetadata. The companion vllm-project/vllm#52813 is the checkpoint consumer: it reads that mapping and dispatches each attention layer.Together, the two PRs support layer-wise mixtures of full FP8 K/V and full NVFP4 K/V without any new kernel code; vLLM #52813 uses the existing uniform FP8 and NVFP4 implementations for each selected layer.
FP8-K/NVFP4-V within one layer is a separate capability. This PR can search and export that format, but vLLM #52813 deliberately rejects it because it requires the independent mixed-K/V attention-kernel implementation. Neither PR bundles that kernel work.
Usage
The shipped three-format recipe can also be run with:
Testing
examples/hf_ptqinvocation of the public KV AutoQuant API;main: 319 passed, 4 environment-gated failuresPrefixChange/scope_prefix) newer than the locally availabletransformers==5.4.0;Before your PR is "Ready for review"
CONTRIBUTING.md: N/AAdditional Information
disabled_layersare preserved in their existing KV format and excluded from the searched-layer bit budget.