Remove KV cache scale boilerplate from model weight loading methods - #43167
Conversation
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request refactors the KV-cache scale loading process by replacing manual mapping logic in individual models with a centralized WeightsMapper provided by quantization configurations. It introduces a KVCacheScaleParameter for standardized scalar weight handling and updates AutoWeightsLoader to apply these mappers automatically. However, two significant issues were identified: first, the use of the | operator to merge WeightsMapper instances in vllm/model_executor/models/utils.py may cause a TypeError if the operator is not implemented; second, many models with custom weight loading loops will now fail to load KV-cache scales because the necessary mapper application was removed from their specific implementations without being replaced.
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
…llm-project#43167) Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…m-project#44484 - Add evaluation outputs showing comparison tables - Add CI evidence files with test run data - Add LLM selector replay outputs with inputs These serve as examples for running the ATS workflow.
…llm-project#43167) Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Signed-off-by: Ekagra Ranjan <3116519+ekagra-ranjan@users.noreply.github.com>
…llm-project#43167) Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Signed-off-by: Waqar Ahmed <waqar.ahmed@amd.com>
…llm-project#43167) Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Script fixes:
- compare_selector_vs_ci.py: use failed_test_list (individual tests with
identifier field) instead of jobs_failed (job-level) for classification.
The prior bug caused 0 true positives regardless of LLM selections.
- compare_selector_vs_ci.py: fix NameError on undefined `llm_selected`
(renamed to `selected_tests` consistently) and tautological status
expression in SUMMARY.md generation.
- fetch_buildkite_test_logs.py: add parse_pytest_summary_stats() to extract
pass/skip counts from the pytest summary line ("N failed, M passed, K
skipped"). These are now included per-job and aggregated in the result.
- collect_ci_evidence.py: fix double-append bug where `tests_run = all_tests_run`
created an alias and appending inside the for-loop modified the list being
iterated. Fixed by iterating over list(all_tests_run). Also wire summary_stats
through from log parser into the evidence output.
PR vllm-project#43167 evaluation correction:
- Actual failures were 2 (not 141). The earlier report misread the Buildkite
logs: the entrypoints job showed "1 failed, 269 passed, 6 skipped" and the
CPU job showed "1 failed, 657 passed, 253 skipped".
- Failing tests: test_chat.py::test_invocations (API response schema mismatch)
and test_mistral.py::test_apply_chat_template (token sequence mismatch).
- Updated evaluation_report.json, test_comparison_table.txt, SUMMARY.md,
README.md, evaluation_summary.md, and pr_43167_actual_failures.md.
Co-authored-by: Claude
Signed-off-by: Edward Arthur Quarm Jnr
Co-authored-by: Claude Signed-off-by: Edward Arthur Quarm Jnr
- Add report.template.md as the canonical reference for the report
structure, with {{PLACEHOLDER}} variables for each section.
- compare_selector_vs_ci.py: replace ASCII test_comparison_table.txt +
SUMMARY.md + evaluation_summary.md with a single report.md that
follows the template. Remove dead generate_text_table() function.
- skill_definition.md: update output file list to reference report.md
and report.template.md.
- evaluation/pr_43167/report.md: generated report for PR vllm-project#43167 in the
new format.
- evaluation/pr_43167/README.md: simplified to point to report.md.
Co-authored-by: Claude
Signed-off-by: Edward Arthur Quarm Jnr
- Patch ci_evidence.json with the 2 actual failures from build 70063 (the PR head CI run). The evidence was originally collected against the merge commit build (70204) where failures were already fixed. - Fix compare_selector_vs_ci.py: prefer vllm/ci pipeline build over intel-ci when selecting the build reference for the report header. - Fix compare_selector_vs_ci.py: show aggregate pass/skip stats once at the section level instead of duplicating them per job. - Regenerated report.md and evaluation_report.json from the script (TP:0, FN:2, FP:7, Recall:0%, Precision:0%). Co-authored-by: Claude Signed-off-by: Edward Arthur Quarm Jnr
…llm-project#43167) Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Signed-off-by: divineearthly <divineearthly@gmail.com>
…llm-project#43167) Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…llm-project#43167) Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…1053) ### What this PR does / why we need it? After vllm-project/vllm#43167 , we can remove our two patches about kvcache quant weight. Should be merged after our main2main at vllm-tag 0.23.0 and vllm-main-0605. ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? local tested model: Qwen3-32B-W8A8C8 DeepSeek-V3.1-W4A8C8 - vLLM version: v0.23.0 - vLLM main: vllm-project/vllm@1f486d9 --------- Signed-off-by: menogrey <1299267905@qq.com>
…lm-project#11053) ### What this PR does / why we need it? After vllm-project/vllm#43167 , we can remove our two patches about kvcache quant weight. Should be merged after our main2main at vllm-tag 0.23.0 and vllm-main-0605. ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? local tested model: Qwen3-32B-W8A8C8 DeepSeek-V3.1-W4A8C8 - vLLM version: v0.23.0 - vLLM main: vllm-project/vllm@1f486d9 --------- Signed-off-by: menogrey <1299267905@qq.com>
…lm-project#11053) ### What this PR does / why we need it? After vllm-project/vllm#43167 , we can remove our two patches about kvcache quant weight. Should be merged after our main2main at vllm-tag 0.23.0 and vllm-main-0605. ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? local tested model: Qwen3-32B-W8A8C8 DeepSeek-V3.1-W4A8C8 - vLLM version: v0.23.0 - vLLM main: vllm-project/vllm@1f486d9 --------- Signed-off-by: menogrey <1299267905@qq.com>
…lm-project#11053) ### What this PR does / why we need it? After vllm-project/vllm#43167 , we can remove our two patches about kvcache quant weight. Should be merged after our main2main at vllm-tag 0.23.0 and vllm-main-0605. ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? local tested model: Qwen3-32B-W8A8C8 DeepSeek-V3.1-W4A8C8 - vLLM version: v0.23.0 - vLLM main: vllm-project/vllm@1f486d9 --------- Signed-off-by: menogrey <1299267905@qq.com> Signed-off-by: Spicy-Stick <873805887@qq.com>
…1053) ### What this PR does / why we need it? After vllm-project/vllm#43167 , we can remove our two patches about kvcache quant weight. Should be merged after our main2main at vllm-tag 0.23.0 and vllm-main-0605. ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? local tested model: Qwen3-32B-W8A8C8 DeepSeek-V3.1-W4A8C8 - vLLM version: v0.23.0 - vLLM main: vllm-project/vllm@1f486d9 --------- Signed-off-by: menogrey <1299267905@qq.com> Signed-off-by: xqchen7 <chenxueqing7@huawei.com>
…llm-project#43167) Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…llm-project#43167) Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…llm-project#43167) Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…2463) ### What this PR does / why we need it? After vllm-project/vllm#43167 vllm-project/vllm#44589, we can remove our two patches about kvcache quant weight. ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? - vLLM version: v0.25.1 - vLLM main: vllm-project/vllm@54503ec Signed-off-by: menogrey <1299267905@qq.com>
…lm-project#11053) ### What this PR does / why we need it? After vllm-project/vllm#43167 , we can remove our two patches about kvcache quant weight. Should be merged after our main2main at vllm-tag 0.23.0 and vllm-main-0605. ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? local tested model: Qwen3-32B-W8A8C8 DeepSeek-V3.1-W4A8C8 - vLLM version: v0.23.0 - vLLM main: vllm-project/vllm@1f486d9 --------- Signed-off-by: menogrey <1299267905@qq.com>
The general changes in this PR are:
get_cache_scaleintoget_cache_scale_mapperAutoWeightsLoader.load_weightsKVCacheScaleParameterforBaseKVCacheMethodso that coersion to scalar happens automaticallyThis allows us to:
load_weightsmethod in modelling code