Align quantized_kv_start defaults with the CLI - #1652
Open
Vlor999 wants to merge 1 commit into
Open
Conversation
generate_step() and speculative_generate_step() defaulted quantized_kv_start to 0, so a caller passing kv_bits= without it quantized the KV cache from the first token. The CLI already ships DEFAULT_QUANTIZED_KV_START = 5000, so the same arguments behaved differently depending on the entry point used. Use the existing constant as the default in both functions and document the trade-off it encodes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Fixes #1651.
generate_step()andspeculative_generate_step()defaultedquantized_kv_startto
0, while the CLI already shipsDEFAULT_QUANTIZED_KV_START = 5000. A callerpassing
kv_bits=withoutquantized_kv_starttherefore got different behaviourdepending on whether they came through the library or the CLI. This uses the
existing constant as the default in both functions and documents the trade-off
it encodes.
Why
mlx-community/Qwen3-0.6B-4bit,kv_bits=8, 8-token prompt, median of 3,Apple M4 Pro (14-core), macOS 26.6:
start=0(before)start=5000(after)The trade-off, stated plainly
This is not a free win. The new default is 6-22% faster to decode, but it
uses more memory once generations get long — 169 MB more at 1024 tokens. Below
roughly 256 generated tokens the memory is a wash (367 MB either way) and the
speed difference is the whole story; past that, the old default genuinely buys
memory back.
So the argument here is consistency, not superiority: the CLI already
encodes 5000 as the intended default via a shared constant, and the library
silently disagreeing with it is the part that looks like a bug. If you would
rather keep
0and document the difference instead, say so and I will closethis in favour of a docs-only change — I raised both options in #1651.
This changes behaviour for existing callers who pass
kv_bits=and rely onthe current
0. They can restore it explicitly withquantized_kv_start=0,which the updated docstring now calls out.
Validation
Three tests added, pinning both function defaults to
DEFAULT_QUANTIZED_KV_STARTand asserting
cache_prompt.pystill shares the same value — so the two cannotdrift apart again silently.
I checked the tests are not vacuous by reverting
generate_step's default to0: the suite fails. With the change in place, all 60 pass.Supersedes #1567, which was opened against an older
main. Same change, rebuilton current
main, with the measurements and tests added.