perf(turboquant): raise KV activation threshold 2048 → 100K - #210
perf(turboquant): raise KV activation threshold 2048 → 100K#210dusterbloom wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe default TurboQuant decode activation threshold constant was raised from 2048 to 100,000 with expanded documentation explaining the rationale. Additionally, the model doctor now emits a warning when a model's KV-cache config is set to turboquant, describing performance characteristics and the activation threshold override. ChangesTurboQuant threshold and diagnostics
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: panbanda Suggested labels: enhancement, documentation Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/higgs-models/src/cache.rs (1)
21-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider making the constant visible to
higgsto avoid literal drift.
doctor.rshardcodes "100K" in its new warning text sinceDEFAULT_TURBOQUANT_ACTIVATE_ATis private to this crate. If this default changes again, the doctor message will silently go stale.♻️ Suggested fix
-const DEFAULT_TURBOQUANT_ACTIVATE_AT: i32 = 100_000; +pub const DEFAULT_TURBOQUANT_ACTIVATE_AT: i32 = 100_000;Then reference it from
doctor.rsinstead of the literal100Kin the warning string.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/higgs-models/src/cache.rs` around lines 21 - 41, The warning text in the doctor path hardcodes the TurboQuant threshold as “100K”, which can drift from the real default. Expose the existing DEFAULT_TURBOQUANT_ACTIVATE_AT constant from cache.rs to higgs so doctor.rs can reference it when building the warning message instead of embedding a literal. Update the warning construction in the doctor code to use that shared constant (or a shared formatter derived from it) so future default changes stay in sync automatically.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/higgs-models/src/cache.rs`:
- Around line 21-41: The warning text in the doctor path hardcodes the
TurboQuant threshold as “100K”, which can drift from the real default. Expose
the existing DEFAULT_TURBOQUANT_ACTIVATE_AT constant from cache.rs to higgs so
doctor.rs can reference it when building the warning message instead of
embedding a literal. Update the warning construction in the doctor code to use
that shared constant (or a shared formatter derived from it) so future default
changes stay in sync automatically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 64779fb2-7e6a-4878-ace9-8ca1c0313b17
📒 Files selected for processing (2)
crates/higgs-models/src/cache.rscrates/higgs/src/doctor.rs
TurboQuant's custom Metal decode kernels are slower than MLX's dense SDPA, so activating at 2048 tokens silently degrades decode for any model with kv_cache="turboquant". Benchmarked on Qwen3.5-9B-4bit (M4 32GB), forcing activation via HIGGS_TURBOQUANT_MIN_TOKENS=0: context dense turboquant d decode ~15 tok 15.6 tok/s 11.7 tok/s -25% ~7K tok 12.5 tok/s 10.1 tok/s -19% Plus a first-token stall at 7K when the prefilled KV is bulk-quantized (TTFT 0.4s -> 6.4s). TurboQuant never wins in the measured range. Dense KV is only ~10 KB/token (~1 GB at 100K), so quantization's memory saving only pays for its decode tax near that scale. - cache.rs: DEFAULT_TURBOQUANT_ACTIVATE_AT 2048 -> 100_000 (overridable via HIGGS_TURBOQUANT_MIN_TOKENS) - doctor.rs: warn when a model sets kv_cache=turboquant Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
c9f85b1 to
4a35164
Compare
|
Addressed the CodeRabbit nitpick on
Verified locally: On the red
The identical failure appears on unrelated dependency-bump PRs since ~2026-06-24 (e.g. the renovate lockfile PRs). This change touches only a constant and a 🤖 Addressed by Claude Code |
|
The failing Build / Lint / Coverage / MSRV checks here are the known macOS-runner infra breakage, not anything in this PR (a constant + a doctor string can't affect Metal shader linking). This run hit both variants of it:
Both are fixed in #211 (pin |
What
Raises the default TurboQuant KV activation threshold from 2048 → 100K tokens, and adds a
doctorwarning forkv_cache = "turboquant".Why
TurboQuant's custom Metal decode kernels (
decode_scores/decode_weighted_values) are measurably slower than MLX's built-in dense SDPA. At the current2048default, any model configured withkv_cache = "turboquant"silently drops onto the slow path from ~2K tokens of context onward — exactly where you'd enable it.Benchmarked with
bench_decodeon Qwen3.5-9B-4bit (M4 32 GB), same server,--kv-cache offvsturboquant(+HIGGS_TURBOQUANT_MIN_TOKENS=0to force activation):Plus a multi-second first-token stall at 7K when the prefilled KV is bulk-quantized (TTFT 0.4 s → 6.4 s). TurboQuant never wins in the tested range.
Dense KV costs only ~10 KB/token — ~1 GB at 100K context — so TurboQuant's memory saving only pays for its decode tax near that scale. The new default keeps decode on the fast dense path; set
HIGGS_TURBOQUANT_MIN_TOKENSto opt back in when context length genuinely threatens memory.Changes
cache.rs:DEFAULT_TURBOQUANT_ACTIVATE_AT2048 → 100_000, with the bench data in the doc comment.doctor.rs:warn()when a model setskv_cache = turboquant.Follow-up
The real fix is a faster fused decode kernel — the values kernel is O(T)/thread with no SIMD-group reduction (flagged with a
ponytail:marker incache.rs). This PR just closes the footgun until then.Test
cargo fmt+cargo clippy -p higgs -p higgs-modelscleancargo test -p higgs doctor -- --test-threads=1→ 42 passed🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes