[None][feat] Enable KVCacheManagerV2 by default for Llama and Llama4 - #18342
[None][feat] Enable KVCacheManagerV2 by default for Llama and Llama4#18342erictsai-nv wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughLlama and Llama4 now prefer KV cache manager V2. Llama4 also explicitly prefers the Python transceiver runtime for disaggregated NIXL serving. Documentation and registered-model tests cover these preferences. ChangesLlama runtime preference integration
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Llama and Llama4 will select KVCacheManagerV2 automatically while explicit configuration remains honored. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the change, user-setting precedence, documentation updates, validation results, and relevant test coverage. The PR checklist is summarized by the final checked item, so the description is sufficiently complete. Full details: Docstring CoverageExplanation Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
Two things worth resolving before this lands.
The transceiver preference on Llama4ForConditionalGeneration isn't conditional on V2 being chosen. _resolve_transceiver_runtime_auto runs first and only looks at the backend and the model hook, so a user who sets kv_cache_config.use_kv_cache_manager_v2=False (or hits one of the automatic V1 demotions, e.g. two-model spec dec) on NIXL disagg still gets the Python transceiver with a V1 manager. That combination is permitted by create_kv_cache_transceiver, but the validation you describe covers V2 disagg. Did any Llama4 disagg run exercise V1 + Python transceiver? If not, it'd be good to say so explicitly, since it's the new default for that config.
Second, this is a default flip for the most widely used architecture in the repo (every checkpoint declaring architectures: ["LlamaForCausalLM"], not just Llama 3/4), and the title carries [None]. A JIRA would make it easier to trace and revert if a V2-attributable regression shows up post-merge.
Test and doc coverage otherwise matches the pattern used by the earlier Gemma/GPT-OSS entries.
Signed-off-by: Eric Tsai <ertsai@nvidia.com>
808627a to
e870239
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #70288 [ run ] triggered by Bot. Commit: |
|
PR_Github #70288 [ run ] completed with state
|
Dev Engineer Review
LlamaForCausalLMandLlama4ForConditionalGenerationpreferKVCacheManagerV2.QA Engineer Review
tests/unittest/llmapi/test_llm_args.py.LlamaForCausalLMandLlama4ForConditionalGeneration.Description
Enables
KVCacheManagerV2by default for Llama and Llama4.LlamaForCausalLMandLlama4ForConditionalGenerationdeclareget_preferred_kv_cache_manager_version() -> "V2", using the per-model preference hook introduced in #16060.LlamaForCausalLMalready prefers the Python transceiver through #16787; this change adds the same preference toLlama4ForConditionalGeneration, so disaggregated serving over NIXL retains V2 instead of falling back to V1. The preferences are adopted only when the user leaveskv_cache_config.use_kv_cache_manager_v2andcache_transceiver_config.transceiver_runtimeat"auto"; explicit user settings continue to win.The KV cache documentation now lists Llama and Llama4 among the model families that select V2 automatically.
Validation
Pre-PR GPU validation covered the existing Llama integration matrix across H100, H200, B200, B300, GB200, GB300, A10, A100, and L40S. Llama3 accuracy, end-to-end, speculative-decoding, disaggregated-serving, and performance cases passed across V1 and V2 arms without a V2-attributable correctness or performance regression. Llama4 V2 validation on B200 passed quickstart, TP2/PP2 multinode, 8-GPU chunked-prefill, and one-model Eagle3 cases.
Test Coverage
tests/unittest/llmapi/test_llm_args.py::TestKvCacheManagerV2AutoResolution::test_registered_models_prefer_v2now coversLlamaForCausalLMandLlama4ForConditionalGeneration.tests/unittest/llmapi/test_llm_args.py::TestKvCacheManagerV2AutoResolution::test_registered_models_keep_v2_on_nixlnow resolves both Llama entry points in production order and verifies that the fully-"auto"NIXL path retains V2 with the Python transceiver.TestKvCacheManagerV2AutoResolutioncontinue to cover explicit user overrides and automatic V1 demotion for incompatible routes.PR Checklist
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.