kimi-k3 : full-size model fixes and the MoonViT-3d vision tower - #48
kimi-k3 : full-size model fixes and the MoonViT-3d vision tower#48danielhanchen wants to merge 2 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efc8bc38f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| assert self.hparams_vision is not None | ||
| n_head = self.hparams_vision["vt_num_attention_heads"] | ||
|
|
||
| if "wqkv" in name and "weight" in name: |
There was a problem hiding this comment.
For Kimi-K3 checkpoints, vision_tower.patch_embed.proj.weight is a rank-5 Conv3D kernel. This method lets it fall through to MmprojModel.modify_tensors(), which serializes it as a five-dimensional v.patch_embd.weight; the GGUF loader rejects tensors exceeding GGML_MAX_DIMS == 4, and clip_graph::build_inp() only invokes ggml_conv_2d. Fold or squeeze the temporal dimension for the supported single-image path before yielding this tensor.
Useful? React with 👍 / 👎.
| if "mm_projector.proj.0." in name: | ||
| name = name.replace(".proj.0.", ".proj.linear_1.") | ||
| elif "mm_projector.proj.2." in name: | ||
| name = name.replace(".proj.2.", ".proj.linear_2.") |
There was a problem hiding this comment.
When converting the published PatchMergerV2 layout, the required mm_projector.post_norm.weight passes filter_tensors() but is neither renamed here nor covered by the MMPROJ tensor aliases, so super().modify_tensors() raises Can not map tensor and conversion cannot finish. This tensor must be mapped to mm.post_norm.weight, which the new PROJECTOR_TYPE_KIMIK3 loader requires.
Useful? React with 👍 / 👎.
| def filter_tensors(cls, item: tuple[str, Callable[[], Tensor]]) -> tuple[str, Callable[[], Tensor]] | None: | ||
| name, _ = item | ||
| if not name.startswith(("vision_tower.", "mm_projector.")): | ||
| return None |
There was a problem hiding this comment.
The MoonViT-3d state dict also contains vision_tower.patch_embed.pos_emb.time_weight, and this broad prefix filter retains it even though the image-only graph deliberately omits the temporal position term. There is no MMPROJ tensor-map entry or runtime tensor for this parameter, so it reaches super().modify_tensors() and aborts conversion with Can not map tensor; explicitly discard it for the supported t == 1 path.
Useful? React with 👍 / 👎.
ngxson
left a comment
ngxson
left a comment
There was a problem hiding this comment.
vision model impl looks clean, thanks!
There was a problem hiding this comment.
would you mind pushing the n_embd_head change to a dedicated PR, so that I can run a test & make sure it doesn't break existing models?
There was a problem hiding this comment.
@ngxson will check!
I was planning to make this as a PR to pwilkin's fork as well haha
Or if you wanna take over that also works - but I shall re-check! I also did multiple images and Kimi works - https://unsloth.ai/docs/models/kimi-k3#run-kimi-k3-in-llama.cpp
I'll double check other models
unsloth published the sub-4-bit tiers on 2026-07-29 and all four fit: UD-IQ1_S 594.0, UD-IQ1_M 648.9, UD-IQ2_XXS 711.1, UD-Q2_K_XL 861.3 GB against 1133 GB of RAM. The pre-staged row needed no correction - the subdir and prefix it guessed (UD-Q2_K_XL / Kimi-K3-UD-Q2_K_XL) are exactly what shipped, 19 shards - so its `?` becomes a real count and only kimi-k3-ik stays pending, ubergarm having published nothing. That closes the quant gate and leaves the engine as the sole blocker, now characterised properly rather than as "no arch": - ik has no kimi-k3 arch; mainline's #26185 is open and conflicted; unsloth built these against their own fork (unslothai/llama.cpp#48), which their README tells you to use. - Before any of that matters, LLAMA_MAX_EXPERTS is 512 and K3 has 896, so it trips an arch-generic assert in load_hparams before a kimi-k3 hook could run - ik src/llama-hparams.cpp:9, asserted at :165. Mainline's PR to raise it (#26192) was closed unmerged. Recorded as step 0 in porting/k3, with the caveat that the constant sizes stack arrays, so "one line" needs checking rather than assuming. Two findings from Moonshot's README invalidate opts that are right for K2.x, so the K3 rows change from --reasoning off to --reasoning-format deepseek: - K3 always thinks. There is no enable_thinking equivalent; effort is a top-level reasoning_effort field llama.cpp has no flag for, defaulting to max. So the K2-correct flag is simply meaningless here. - K3 was trained in preserved-thinking-history mode: multi-turn and tool calls require the complete assistant message replayed, including reasoning_content AND tool_calls. That is precisely the shape ik #1605 400s on. For K2 you can avoid emitting it; for K3 it is mandatory, so #1605 is promoted from caveat to blocking for agentic K3. Sampling: Moonshot's generation_config sets only max_length and eos, so the rows assert no temperature or top_p rather than inventing them. The registry now warns that downloading is safe but serving is not: download and verify will succeed, then the load will fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Graph node budget, LLM_TYPE for the 93-layer config, four hparams promoted to required, and an E8M0 NaN check during the MXFP4 repack. Details in the PR description. Assisted-by: Claude Code
New kimik3 projector type, its graph builder and the mmproj converter. Also adds an optional clip.%s.attention.head_dim so build_vit stops deriving d_head from n_embd, which is wrong whenever a tower's qkv width differs from n_embd. Assisted-by: Claude Code
cf67f0d to
12e01bd
Compare
efc8bc3 to
daef2b3
Compare
There was a problem hiding this comment.
💡 Codex Review
Lines 1490 to 1491 in daef2b3
Honor custom token limits when loading Kimi-K3
When a Kimi-K3 user supplies --image-min-tokens or --image-max-tokens, clip_ctx stores the override in custom_image_*_tokens, but this branch directly replaces the effective limits with the GGUF metadata and never applies those custom values. The converter always emits both metadata keys, so the set_limit_image_tokens() fallback that honors the overrides is unreachable; a requested memory cap can therefore be ignored and images can still expand to the checkpoint's 4096-token default. Apply the custom limits after loading the metadata or reuse the existing limit helper.
AGENTS.md reference: AGENTS.md:L80-L80
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…head (#49) The nightly has been red since 2026-07-31. pwilkin force-pushed ggml-org#26185 at 21:16Z that day, eleven minutes before the schedule fired, and the pin in this file still named cf67f0d -- a merge commit that the rebase wrote out of the branch. The membership gate in the resolve step caught it and refused, so every build job was skipped. Runs 30666643530 and 30718507279 both died there. Two more things were wrong once that was unblocked: - Inkling was pinned to d69b7e6, which is the commit three separate reporters got garbage output from (CPU, Mac, and ROCm with -fa on). ggml-org#25731 fixed all of those in 02142bb; the nightly was shipping the broken one. - The rebase left ggml-org#26185 unmergeable against current master, and Kimi-K3 and Inkling register in the same arch, model and mtmd tables. Two independent heads cannot both land, so one of them has to know about the other. #48 now carries that. Its base branch was advanced to the rebased ggml-org#26185 head, then merged with b10223 and with Inkling, resolving four collisions: src/llama-arch.cpp KIMI_K3 and INKLING share a fallthrough src/llama-model.cpp same, in the ROPE_TYPE_NONE group tests/test-llama-archs.cpp master's MINIMAX_M3 vs KIMI_K3 tools/mtmd/ CMake source list and the graph registry All four are additive; both arches belong in every group. The two Kimi-K3 commits were rebased on top, so #48 is still a 10-file stacked diff against its base. ggml-org#26185 stays listed, last. Its head is now an ancestor of #48, so the resolver merges it as a no-op and the release manifest still names it. Verified against b10223: the four merge in this order with zero unmerged paths, and the merged tree builds with -DGGML_CUDA=ON. On the merged tree test-chat passes and test-llama-archs -a kimi-k3 is OK on both CUDA and CPU (NMSE 8.77e-08); it fails to even load the model without this repin.
The 2026-08-02 schedule (run 30767267935) failed in resolve: ggml-org#25731 (02142bb) does not merge cleanly onto b10229 + the PRs listed before it Not the pin rot that broke the previous two nights -- all four pins passed the membership gate this time. Upstream added LLM_ARCH_DEEPSEEK4 to the same fallthrough group in src/llama-arch.cpp that Inkling registers in, between b10226 and b10229, and the resolver has no conflict resolution. Fixed on the branch rather than here: b10229 is merged into add-inkling and both cases are kept, since both arches belong in that group. That also clears ggml-org#25731's own mergeable state against master, which had gone red for the same reason. No other pin moves. #48 needed nothing -- it already carries b10223 and the new arch does not collide with Kimi-K3. Verified against b10229: the four merge in order with zero unmerged paths, and the merged tree builds with -DGGML_CUDA=ON. test-chat passes and test-llama-archs -a kimi-k3 is OK on CUDA and CPU (NMSE 8.77e-08). The new Inkling head also merge-tree's clean against upstream master, which is six commits past b10229, so the next tag should resolve too.
3 participants
Stacked on top of ggml-org#26185 (Kimi-K3 text support), which is the base branch
here. Only the two Kimi-K3 commits are included, so the diff is 14 files rather than the
merged nightly tree.
1. Fixes for loading and running the full-size model
The 1T-parameter checkpoint exposes issues the small test model does not:
n_expert_usedis read per layer instead of once, so the dense prefix and the MoE layersno longer have to agree.
count, which was over-allocating on a 92-layer model.
conversion/base.pykeeps the expert tensors in their source order, avoiding a repackthat needed the whole layer resident.
2. MoonViT-3d vision tower (image path)
Adds the vision encoder as a new mtmd model:
tools/mtmd/models/kimik3.cppbuilds the graph. Patch embedding is 3d, so the temporalaxis is folded into the batch for the image case.
clip_hparams::n_embd_headis added because MoonViT's head dimension is notn_embd / n_head. It defaults to 0 and the existingn_embd / n_headpath is usedwhenever it is unset, so every other mmproj model is unaffected. I verified this against
the mmproj files for the other supported architectures.
conversion/kimivl.pygainsKimiK3VisionModelto write the mmproj.Audio is not included. Only the image path is implemented.
Testing
n_embd_headchangedoes not alter their behaviour.