feat: add Kimi K3 model support - #3259
Conversation
Signed-off-by: HuiyingLi <willwin.lee@gmail.com>
Signed-off-by: HuiyingLi <willwin.lee@gmail.com>
Signed-off-by: HuiyingLi <willwin.lee@gmail.com>
Signed-off-by: HuiyingLi <willwin.lee@gmail.com>
Signed-off-by: HuiyingLi <willwin.lee@gmail.com>
|
/claude review |
Review summaryLight review of the Kimi K3 model-onboarding PR (17 files). Registry wiring ( A few points worth addressing (all posted inline):
The |
| except ImportError: | ||
| from transformers.activations import GELUTanh | ||
|
|
||
| activations.PytorchGELUTanh = GELUTanh |
There was a problem hiding this comment.
Import-time monkeypatching of a third-party (transformers) global. activations.PytorchGELUTanh = GELUTanh mutates transformers.activations for the whole process the moment this module is imported (it is imported transitively by multimodal.py, the registered VLM). Even though it only fires on older transformers where the symbol is missing, a process-global side effect at import time is fragile — the alias leaks into any other code that inspects transformers.activations. Prefer binding a module-local name only (PytorchGELUTanh = GELUTanh) without writing back onto the activations module.
| try: | ||
| from .encoding import build_chat_segments, is_batched_conversation | ||
| except ImportError: # pragma: no cover - supports direct file execution/import. | ||
| from encoding_k3 import build_chat_segments, is_batched_conversation |
There was a problem hiding this comment.
The except ImportError fallback imports from encoding_k3, but no such module exists anywhere in the package (the sibling module is encoding.py). If the relative import ever fails, this raises ModuleNotFoundError rather than a working fallback, so the branch is dead/misleading. Either drop the fallback (the module is only ever imported as part of the package) or point it at the correct name.
| num_experts=2, | ||
| num_experts_per_token=1, | ||
| num_shared_experts=0, | ||
| first_k_dense_replace=num_hidden_layers + 1, |
There was a problem hiding this comment.
first_k_dense_replace=num_hidden_layers + 1 forces every layer to be dense, so no KimiK3MoE layer is ever instantiated in the test suite. Combined with test_state_dict_adapter.py (which only exercises key mapping / MXFP4 dequant), the routed-expert forward path has no numerical coverage: KimiK3MoE.forward, KimiK3Gate (fp32 sigmoid + group top-k), _forward_reference_order, and — most importantly — the new apply_router_weight_after_down grouped-experts logic added to the shared moe/experts.py are all untested.
The experts.py change reshapes the scatter output to [tokens, topk, hidden], applies the fp32 router weight after the down projection, then sums over topk, with new slot_id scatter indexing in both _forward_loop and _forward_grouped_mm. This is subtle index math on a shared component consumed by every MoE model. Please add a small CPU unit test that builds a tiny K3 model with first_k_dense_replace low enough to create at least one MoE layer and asserts a finite forward (and ideally parity of apply_router_weight_after_down=True vs. the equivalent before-down weighting on the reference loop path).
Avoid import-time transformer mutation, remove the dead tokenizer fallback, and cover K3 MoE router-weight placement against reference inference for both loop and grouped-mm expert backends. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: HuiyingLi <willwin.lee@gmail.com>
|
/claude review |
|
Light review — Kimi K3 (KDA/MLA hybrid MoE) onboarding. Read the full diff for all 17 changed files (excluding The model package is well-structured: registry wiring ( Two findings posted inline:
|
| assert thinking_effort in _VALID_THINKING_EFFORTS, ( | ||
| f"Unsupported thinking_effort={thinking_effort!r}; supported values are {sorted(_VALID_THINKING_EFFORTS)}." | ||
| ) | ||
| if thinking and thinking_effort in _VALID_THINKING_EFFORTS: |
There was a problem hiding this comment.
"medium" is advertised as a supported thinking_effort in the emitted system message ("supported values include low, medium, high, and max" at line 511), but _VALID_THINKING_EFFORTS = {"low", "high", "max"} omits it. Passing thinking_effort="medium" therefore trips this assert and raises AssertionError, and even if it passed the assert the elif thinking and thinking_effort in _VALID_THINKING_EFFORTS guard at line 505 would silently drop the effort message. Either add "medium" to _VALID_THINKING_EFFORTS or remove it from the advertised list so the two agree.
Separately, this is user-supplied input validated with assert, which is stripped under python -O; prefer raising a ValueError that names the invalid value.
| "kimi_k2": ("nemo_automodel.components.models.kimi_k2.config", "KimiK2Config"), | ||
| "kimi_k25": ("nemo_automodel.components.models.kimi_k25_vl.model", "KimiK25VLConfig"), | ||
| "kimi_k3": ("nemo_automodel.components.models.kimi_k3.config", "KimiK3Config"), | ||
| "kimi_linear": ("nemo_automodel.components.models.kimi_k3.config", "KimiK3TextConfig"), |
There was a problem hiding this comment.
New _CUSTOM_CONFIG_REGISTRATIONS entries kimi_k3 and kimi_linear are added, but there is no focused test proving that AutoConfig/resolve_custom_config_cls resolves KimiK3Config/KimiK3TextConfig from a checkpoint-style config.json (model_type: "kimi_linear" / "kimi_k3"). Since Automodel owns these config classes and the K3 checkpoint advertises kimi_linear (a model_type not guaranteed present in the installed Transformers CONFIG_MAPPING), the exact failure mode the registry guards against — a stale Transformers release where AutoConfig.from_pretrained cannot find the type — is untested. Please add a test that writes a config.json with these model_types and asserts the local config class is resolved (covering the builtin-absent case).
Accept the documented medium thinking effort with durable input validation, and prove both Kimi K3 model types resolve through local configs when the installed Transformers mapping has no builtin entry. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: HuiyingLi <willwin.lee@gmail.com>
|
/claude review |
|
Reviewed all 18 changed files (excluding One inline finding (medium severity): Nothing else rose to a high-confidence blocking finding. |
| if self.output_attn_res_norm is not None: | ||
| self.output_attn_res_norm.reset_parameters() | ||
| if self.output_attn_res_proj is not None: | ||
| nn.init.normal_(self.output_attn_res_proj.weight, mean=0.0, std=init_std) |
There was a problem hiding this comment.
init_weights reads self.output_attn_res_norm / self.output_attn_res_proj unconditionally, but __init__ only assigns those attributes when self.use_attn_residuals is true (attn_res_block_size is not None, line 1402-1409). Every other consumer of these attributes guards the None-block-size mode — forward branches on use_attn_residuals, and customize_pipeline_stage_modules / get_pipeline_stage_metas use getattr(..., None) — so a model configured with attn_res_block_size=None is a supported configuration, yet the checkpoint-free init path here raises AttributeError before the first forward. Guard the access, e.g.:
if self.use_attn_residuals:
self.output_attn_res_norm.reset_parameters()
nn.init.normal_(self.output_attn_res_proj.weight, mean=0.0, std=init_std)(matching the getattr pattern used elsewhere). The existing tests always set a non-None block size, so this path is uncovered.
Keep checkpoint-free initialization valid when attention residual mixing is disabled while preserving pruned pipeline-stage handling, with a CPU forward regression test for the no-residual configuration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: HuiyingLi <willwin.lee@gmail.com>
|
/claude review |
|
Reviewed the full diff (all 19 non- One non-blocking coverage observation: The KDA linear-attention path — the defining "Kimi Linear" component — is not exercised by any test. Every model test in This is understandable — |
|
/ok to test 181e86a |
|
/ok to test fd2ea50 |
Signed-off-by: HuiyingLi <willwin.lee@gmail.com>
…del into huiyingl/feat/kimi-k3 Signed-off-by: HuiyingLi <willwin.lee@gmail.com>
Signed-off-by: HuiyingLi <willwin.lee@gmail.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: HuiyingLi <willwin.lee@gmail.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: HuiyingLi <willwin.lee@gmail.com>
|
/ok to test c6f144e |
Signed-off-by: HuiyingLi <willwin.lee@gmail.com>
…del into huiyingl/feat/kimi-k3 Signed-off-by: HuiyingLi <willwin.lee@gmail.com>
|
/ok to test 7c08b9b |
Signed-off-by: HuiyingLi <willwin.lee@gmail.com>
|
/ok to test 7439737 |
Signed-off-by: HuiyingLi <willwin.lee@gmail.com>
|
/ok to test a4d357e |
|
/ok to test b4b19d1 |
1 similar comment
|
/ok to test b4b19d1 |
Signed-off-by: HuiyingLi <willwin.lee@gmail.com>
…-k3' into huiyingl/feat/kimi-k3 Signed-off-by: HuiyingLi <willwin.lee@gmail.com>
|
/ok to test c9fac9d |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: HuiyingLi <willwin.lee@gmail.com>
|
/ok to test 29de6d4997022544647c13356f9c8e0466628214 |
@HuiyingLi, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test 29de6d4 |
|
🌿 Preview your docs: https://nvidia-preview-huiyingl-feat-kimi-k3.docs.buildwithfern.com/nemo/automodel |
Summary
trust_remote_codeexamples/llm_finetune/kimi/k3_hellaswag.yamlEP32/PP8 recipeRuntime Changes
Validation
ruff check: passedruff format --check: passed91 passed5639240: completed 100 steps, loss2.0011 -> 1.4307cp1: https://wandb.ai/Nemo-automodel/huiyingl_workspace/runs/fcguk1ys
cp2: https://wandb.ai/Nemo-automodel/huiyingl_workspace/runs/n7n3dtc0