Repository navigation
[TRTLLM-16725][refactor] Move Kimi K3 weight loading into modeling_kimi_k3_loader.py - #20717
yijingl-nvidia wants to merge 1 commit into
Conversation
d68d727 to
743a753
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. WalkthroughKimi K3 checkpoint-loading logic now resides in a dedicated mixin module. The causal-LM wrapper inherits the mixin, and tests import the moved loading helpers and patch the new module’s ChangesKimi K3 Weight Loader Extraction
Estimated code review effort: 4 (Complex) | ~45 minutes Change: Refactor Merge Risk:
|
mikeiovine
left a comment
There was a problem hiding this comment.
Stamp on behalf of runtime devs, delegating proper review to @NVIDIA/trt-llm-models-devs
743a753 to
55e6806
Compare
55e6806 to
0843081
Compare
|
/bot run |
|
PR_Github #79725 [ run ] triggered by Bot. Commit: |
|
PR_Github #79725 [ run ] completed with state
|
0843081 to
1406184
Compare
|
/bot run |
|
PR_Github #79965 [ run ] triggered by Bot. Commit: |
|
PR_Github #79965 [ run ] completed with state
|
Mgluhovskoi
left a comment
There was a problem hiding this comment.
LGTM from telemetry POV.
chienchunhung
left a comment
There was a problem hiding this comment.
LGTM: This PR reorganizes Kimi K3 weight loading while preserving its behavior. The shared ModelLoader still calls model.load_weights(). Kimi now inherits that implementation from KimiK3WeightLoaderMixin in the new file. Kimi retains its specialized lazy checkpoint access, tensor sharding, FP8 handling, and expert loading.
PS: The extraction separates checkpoint loading from model execution and makes the later Model Streamer integration in #20782 easier to review.
1406184 to
c0c1044
Compare
|
/bot run |
|
PR_Github #81259 [ run ] triggered by Bot. Commit: |
|
PR_Github #81259 [ run ] completed with state Resource maintenance in LLM/main/L0_Test-x86_64-Single-GPU #9114: L40S-PyTorch-2 is temporarily paused due to Kubernetes agent provisioning and node issues.
|
|
/bot run |
|
PR_Github #81753 [ run ] triggered by Bot. Commit: |
|
PR_Github #81753 [ run ] completed with state |
c0c1044 to
f2bc560
Compare
|
/bot run |
|
PR_Github #82053 [ run ] triggered by Bot. Commit: |
|
PR_Github #82053 [ run ] completed with state |
Move the weight-loading methods of KimiLinearForCausalLM and the helpers that only they use into the new file modeling_kimi_k3_loader.py. modeling_kimi_linear.py had over 3,300 lines, and about 1,100 of them only turn checkpoint tensors into model parameters. The code moves unchanged: KimiLinearForCausalLM inherits the methods from KimiK3WeightLoaderMixin, and tests import the moved helpers from the new file. The inference module _Fp8BlockScaleWeightReadLinear stays in modeling_kimi_linear.py; the loader imports it inside the functions that use it, because modeling_kimi_linear.py imports the loader. No functional change. Signed-off-by: Yijing Li <257409031+yijingl-nvidia@users.noreply.github.com>
f2bc560 to
d7c81a0
Compare
sunnyqgg
left a comment
There was a problem hiding this comment.
Reviewed the move of the Kimi K3 weight loading into modeling_kimi_k3_loader.py. The moved functions and methods match the base exactly, apart from the two function-local imports of _Fp8BlockScaleWeightReadLinear. The MRO is unchanged (KimiK3WeightLoaderMixin ahead of SpecDecOneEngineForCausalLM). The external callers (runner.py, modeling_kimi_k25.py, the modeling_v2 targets) and the test monkeypatch targets still resolve, and ruff passes. Counts: 0 Blockers, 0 Possibly blocking, 2 Required, both inline: coordinate with the open PRs that edit the moved code, and correct the module docstring and the PR description about what stays in the model file.
| if device.type == "cuda": | ||
| torch.cuda.set_device(device) | ||
| path = os.path.join(ckpt_dir, file_name) | ||
| with safe_open(path, framework="pt", device="cpu") as fh: |
There was a problem hiding this comment.
[Required] Four open PRs edit the Kimi K3 load code that this PR moves. All of them are based on the same blob of modeling_kimi_linear.py (ea45af7c4f) that this PR deletes from:
- [None][perf] Read Kimi-Linear expert shards with pread into pinned host buffers #21588 replaces
safe_open(...)withPinnedSafetensorsFile(...)inload_expert_fileandload_split_file_expert(this line and L1073), and swaps thesafe_openimport in the model file. - [None][feat] Dense DWDP for Kimi-K3 context servers and a balanced DWDP expert partition #21012 adds a dense-DWDP guard at the top of
load_weights(L621). - [TRTLLM-14978][perf] Kimi-K3 decode attention-residual kernel: CuTe DSL PDL mix with the MoE residual folded in, MXFP8 hand-off to KDA, gated/sigmoid low-M GEMM variants #21399 and [TRTLLM-14978][perf] Kimi-K3 MoE decode front: routing + moe_sort in one launch, CuTe router logits, router-first order, NVFP4 latent_down epilogue #21400 add
attention.accept_prequantized_qkvg_input(fused.takes_prequantized_input)in_finalize_weight_load(around L1155-1157). - Draft [None][chore] Add a code-filtered mypy check for tensorrt_llm and fix the errors it found #20946 adds
# type: ignore[call-arg]onload_expert(*job, ...)(L1093).
Git will report conflicts in these regions. But resolving them by keeping this PR's copy of the loader as is (or, on their side, by dropping the hunk in the deleted region) silently loses the other PR's change. For #21588, nothing would catch that: its only test covers PinnedSafetensorsFile itself, not the Kimi load path.
Please link these PRs in the description and agree on the merge order. Whichever lands later has to port those hunks into modeling_kimi_k3_loader.py. When you rebase this PR over any of them, re-run the removed-vs-added comparison (e.g. git diff --color-moved) to make sure their edits end up in the loader.
| """Weight loading for the Kimi K3 text model, `KimiLinearForCausalLM`. | ||
|
|
||
| `KimiLinearForCausalLM` inherits the methods of `KimiK3WeightLoaderMixin`. | ||
| ``modeling_kimi_linear.py`` keeps the model structure and the forward pass, |
There was a problem hiding this comment.
[Required] This sentence doesn't match the code, and neither does the PR description's "the model file keeps only the model and its forward pass". The routed-expert checkpoint layouts stay in modeling_kimi_linear.py:967-1072: _load_packed_mxfp4_expert, _load_nvfp4_expert, _K3ExpertCkptSpec, _K3_EXPERT_CKPT_SPECS and _k3_expert_ckpt_spec. They read the per-expert checkpoint keys ({base}.{e}.w*.weight_packed, ...) and call quant_method.load_*_expert, and their fields are read only in this file (L595, L914-943, L960, L1006-1024). The section comment there still says "K3 streams routed experts one at a time (see load_weights)" (modeling_kimi_linear.py:970, and similarly L1136-1137), but load_weights is no longer in that file. So someone who adds a routed-expert layout, or the Model Streamer path from #20782, will look in the wrong file.
Please do one of the following:
- Move that section here. The model file already imports this module, so there is no cycle. Keep
_k3_expert_ckpt_specreachable frommodeling_kimi_linearfor the test that monkeypatches it, and update the tests that import the other names. - Or say in this docstring that the per-expert layouts stay with
KimiK3MoERuntimeand are reached throughmoe.expert_ckpt_spec.
In both cases, update the load_weights pointers at modeling_kimi_linear.py:970 and :1136, and the PR description.
sunnyqgg
left a comment
There was a problem hiding this comment.
Follow-up to my review of 2026-11-04: All 9 lower-priority points I left out of it still apply at d7c81a0. They are marked [Nice to have] and none blocks the merge on its own.
- [Nice to have] No CI has run on the current head d7c81a0. The last pipeline, PR_Github #82053 (SUCCESS), ran on f2bc560, which has the same five file contents but a base 59 commits older. GitHub also reports a merge conflict with main now, so please rebase and run
/bot runon the new head before merging.
| example FP8 block-scale pairs (`_checkpoint_fp8_pair()`), head-major | ||
| tensor-parallel shards (`_shard_head_major_param()`) and the fused gate/up | ||
| projection (`_gate_up_ckpt_keys()`). | ||
| - After loading, `_finalize_weight_load()` puts the FP8 block-scale GEMM |
There was a problem hiding this comment.
[Nice to have] The module docstring (modeling_kimi_k3_loader.py:19-22) presents _finalize_weight_load() as just the FP8 weight-read swap. It also runs finalize_decode_weights() and _build_mtp_conv_weights() on every KDA layer (:1147-1150) and repoints q/k/v/g to the fused qkvg_proj rows (:1156-1167). A load path that skips it, even for a BF16 checkpoint, leaves _packed_conv_weight as None, and the first KDA prefill asserts (kimi_kda_mixer.py:767). Please document that, and note in the load_weights() bullet that file-grouped expert streaming needs checkpoint_dir (and checkpoint_prefix for re-prefixed keys) on the weights mapping (:990-997); without checkpoint_dir it can drop to the shared-dict fallback (:1099-1110).
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Swap projections to the FP8 block-scale weight-read module after loading. | ||
| # The module itself, `_Fp8BlockScaleWeightReadLinear`, is in modeling_kimi_linear.py. |
There was a problem hiding this comment.
[Nice to have] _Fp8BlockScaleWeightReadLinear stays in modeling_kimi_linear.py:723, but its only production users are now in this file (modeling_kimi_k3_loader.py:176, :1157). Because modeling_kimi_linear.py:121 imports this module, both uses need function-level imports (:171, :1142), and hoisting either one later fails with a partially-initialized-module ImportError. Consider moving the class and its helpers (modeling_kimi_linear.py:699-941) into a module under _torch/modules/, importing it at the top here, and repointing the tests that import or monkeypatch them (e.g. test_linear_output_slice.py:240). A follow-up is fine; meanwhile the comments at :169 and :1140 should say "import the class", not "the module".
| return value[:] | ||
|
|
||
|
|
||
| class KimiK3WeightLoaderMixin: |
There was a problem hiding this comment.
[Nice to have] The mixin reads five host attributes, model, model_config, lm_head (modeling_kimi_k3_loader.py:785), _repurposed_tp_mapping (:713-714) and _fp8_weight_read_moe_mlp (:1171), but declares none, and the docstring names only two. The modeling_v2 K3 targets already set the private two by hand (kimi_k3_mxfp4__sm_100__tp16_moetp16ep1/modeling.py:3246-3248, kimi_k3_mxfp4__sm_100__tp16_moetp4ep4/modeling.py:3316-3318); a host that misses one hits an AttributeError mid-load. Please declare them as annotation-only class attributes, as MultimodalModelMixin does (modeling_multimodal_mixin.py:431), and note that the mixin must stay ahead of SpecDecOneEngineForCausalLM in the bases (modeling_kimi_linear.py:2350-2352) to override its load_weights.
| return count | ||
|
|
||
|
|
||
| _FP8_BLOCK_SCALE_SUFFIX = "_scale" |
There was a problem hiding this comment.
[Nice to have] Nit: the last section header before the class, "Swap projections to the FP8 block-scale weight-read module after loading" (modeling_kimi_k3_loader.py:150-153), now also covers the checkpoint FP8 helpers and _materialize from here to :311, which run during the load rather than after it. Please add a section header above this line, e.g. "Checkpoint FP8 block-scale pairs and lazy tensors", and consider moving _has_weights (:181), which the whole loader uses, out of the swap section.
| from .modeling_kimi_linear import KimiK3MoERuntime | ||
|
|
||
|
|
||
| _KIMI_K3_MLA_DERIVED_PARAM_SUFFIXES = ( |
There was a problem hiding this comment.
[Nice to have] Nit: the first definition in the new file has no comment. Please add one line, e.g. "MLA parameters derived from kv_b_proj during the load (_load_mla_fp8_kv_b() and the BF16 KV-B path); they have no checkpoint key, so _trunk_parameters() skips them." Today this is explained only in the _trunk_parameters() docstring (modeling_kimi_k3_loader.py:529-531).
|
|
||
| # --- _load_trunk_params tp_rank selection via production code --- | ||
| import tensorrt_llm._torch.models.modeling_kimi_k3_loader as loader_mod | ||
| import tensorrt_llm._torch.models.modeling_kimi_linear as prod_mod |
There was a problem hiding this comment.
[Nice to have] Nit: the test patches loader_mod.run_concurrently (:277, :303) but calls prod_mod.KimiLinearForCausalLM._load_trunk_params (:278, :304). The patch only takes effect because the inherited method looks up run_concurrently in the loader's globals (modeling_kimi_k3_loader.py:866), which a reader has to trace through the class bases. Call loader_mod.KimiK3WeightLoaderMixin._load_trunk_params(...) instead, or rename prod_mod to e.g. kimi_linear_mod.
| def checkpoint_key(key: str) -> str: | ||
| return f"{checkpoint_prefix}{key}" | ||
|
|
||
| if expert_jobs and ckpt_dir and os.path.isfile(index_path): |
There was a problem hiding this comment.
[Nice to have] No pre-merge test reaches this streaming path. The Kimi K3 load_weights tests build no MoE layers (num_experts unset in test_kimi_k3_checkpoint.py and test_kimi_k3_mla.py; is_moe = False at test_kimi_linear_checkpoint.py:273), so expert_jobs is empty, and no test calls _load_expert_slices. The gate/up row-concat (modeling_kimi_k3_loader.py:763-764) only sees zeros (test_kimi_k3_checkpoint.py:82, test_kimi_k3_mla.py:254). Optional: drive _load_expert_slices with a stub MoE and a tmp index plus two shards (one expert split across both), asserting each slot loads once and each layer finalizes once, and add a load_weights case with distinct gate/up values at tp=2, rank=1.
| ) | ||
| return len(param_jobs) | ||
|
|
||
| def _load_expert_slices( |
There was a problem hiding this comment.
[Nice to have] Follow-up idea: the per-expert protocol here (prepare a layer on its first expert, finalize it when the last slot lands, then check that every slot is filled and finalized; modeling_kimi_k3_loader.py:888-963, :1112-1135) lives in closures next to the two tensor sources. #20782 leaves this method as is and re-implements the same protocol in plan_model_streamer_load() (load_expert, finish). Extracting it into a small helper that takes get_tensor/get_optional_tensor would let the file-grouped, shared-dict and Model Streamer paths share one copy, so a change to the prepare/finalize contract lands once.
Description
This PR moves the Kimi K3 weight-loading code from
modeling_kimi_linear.pyinto the new filetensorrt_llm/_torch/models/modeling_kimi_k3_loader.py.No functional change.
modeling_kimi_linear.pyhad over 3,300 lines, and about 1,100 of them were weight loading. Now the model file keeps only the model and its forward pass. This improves code readability and maintenance. #20782 adds the Kimi K3 Model Streamer load to the new file.This PR does not depend on the other PRs in the series.
Test Coverage
0 tests added, 3 tests changed. The tests only import or patch the moved code from the new file:
tests/unittest/_torch/modeling/test_kimi_linear_checkpoint.pytests/unittest/_torch/moe/test_kimi_k3_situ_moe.pytests/unittest/_torch/modeling/test_kimi_linear_helix_mappings.pyPR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.PRs in this series
These PRs refactor checkpoint loading and add Model Streamer loading. They merge in this order, top to bottom.
checkpoint_io_policyfrom plain strings to the string enumCheckpointIOPolicy.CheckpointFormatfor the built-in checkpoint formats, make the checkpoint I/O helpers take onlyLoadFormat, and make the checkpoint I/O policy resolution easier to read.distributed/initialization.py.ModelLoader.load()shorter: move its MX, GMS, memory and model-hook helpers to new files inmodels/checkpoints/.BaseCheckpointLoader.modeling_kimi_linear.pytomodeling_kimi_k3_loader.py.checkpoint_io_policy="model_streamer", which loads a checkpoint with Run:ai Model Streamer.🤖 Generated with Claude Code
Dev Engineer Review
KimiK3WeightLoaderMixininmodeling_kimi_k3_loader.pyand moves Kimi K3 checkpoint-loading code out ofmodeling_kimi_linear.py.KimiLinearForCausalLMnow inherits the mixin. The stated intent is to keep model and forward-pass code inmodeling_kimi_linear.py; the PR description reports no functional change.QA Engineer Review
test_kimi_linear_checkpoint.py,test_kimi_k3_situ_moe.py, andtest_kimi_linear_helix_mappings.py. The changes update imports or the patched module path for moved loader helpers. The supplied change summary reports no test-logic changes.test-db/l0_b200.yml. The MoE test file is listed intest-db/l0_b200.ymland has selected tests listed intest-db/l0_b300.yml, including expert checkpoint selection, FP8 block-scaled dequantization, safetensors materialization, and streaming expert-load checks. No test-list entry was found for the Helix mapping test file. No manual-QA list entry was found.Per-File QA Perspective
tensorrt_llm/_torch/models/modeling_kimi_k3_loader.py: Adds the extracted loader mixin. Verify checkpoint mapping, sharding, FP8 handling, expert loading, finalization, and error paths.tensorrt_llm/_torch/models/modeling_kimi_linear.py: ChangesKimiLinearForCausalLMto inherit the loader mixin. Verify model construction and loader method resolution.tests/unittest/_torch/modeling/test_kimi_linear_checkpoint.py: Updates imports for moved checkpoint helpers. The file is listed intests/integration/test_lists/test-db/l0_b200.yml; its tests cover checkpoint planning, sharding, and KDA/MLA loading behavior.tests/unittest/_torch/moe/test_kimi_k3_situ_moe.py: Updates imports for moved loader helpers. The file is listed intest-db/l0_b200.yml; selected expert-loading and helper tests are also listed intest-db/l0_b300.yml.tests/unittest/_torch/modeling/test_kimi_linear_helix_mappings.py: Patchesrun_concurrentlyin the loader module for the mapping swap/restore test. No test-list entry was found.