kimi-k3 : the MoonViT-3d vision tower and full-size loading fixes - #70
kimi-k3 : the MoonViT-3d vision tower and full-size loading fixes#70danielhanchen wants to merge 4 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
Lines 1522 to 1525 in 6dd3474
Warm up with a smaller Kimi-K3 image
With the converter's default in_patch_limit of 16384 and a 14-pixel patch, max_pixels is 3,211,264, so this initializes the model by building a 1792x1792 graph with 16,384 pre-merge attention tokens. On CPU and other backends without flash attention, the quadratic attention scores alone require over 1 GiB, so loading can OOM even when the user will send only small images; preserve the advertised maximum but override warmup to a modest representative token grid as the other dynamic projectors do.
ℹ️ 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".
| @ModelBase.register("KimiK3ForConditionalGeneration") | ||
| class KimiK3VisionModel(MmprojModel): |
There was a problem hiding this comment.
When convert_hf_to_gguf.py --mmproj processes a Kimi-K3 config, get_model_class(..., mmproj=True) checks MMPROJ_MODEL_MAP before importing this decorated class, but that map has no KimiK3ForConditionalGeneration entry. The command therefore raises NotImplementedError: Architecture 'KimiK3ForConditionalGeneration' not supported before any vision tensors are converted; add the architecture-to-kimivl dispatch entry alongside Kimi-K2.5.
Useful? React with 👍 / 👎.
| elif "mm_projector.proj.2." in name: | ||
| name = name.replace(".proj.2.", ".proj.linear_2.") | ||
|
|
||
| yield from super().modify_tensors(data_torch, name, bid) |
There was a problem hiding this comment.
After the mmproj dispatch is fixed, the checkpoint's mm_projector.post_norm.weight reaches this call unchanged, but the MMPROJ tensor map's V_MM_POST_NORM aliases do not include that Kimi-K3 name. ModelBase.map_tensor_name() consequently raises ValueError during conversion, while clip_model_loader later requires mm.post_norm.weight; rename this tensor here or add the corresponding tensor-map alias.
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 | ||
| return super().filter_tensors(item) |
There was a problem hiding this comment.
Once dispatch reaches this converter, MoonViT's vision_tower.patch_embed.pos_emb.time_weight also passes this broad filter. The new graph explicitly supports only t == 1 and has no temporal-position input, while the MMPROJ tensor map recognizes only the spatial vision_tower.patch_embed.pos_emb; passing the temporal parameter to super().modify_tensors() therefore raises ValueError instead of producing an image-only mmproj. Filter this temporal-only parameter out.
Useful? React with 👍 / 👎.
The first pin for #95 was cut from fork master, whose merge base with the current base tag is from June 10, so merging it dragged in the whole fork/upstream divergence and conflicted across ~1600 files. Rebased the change onto b10359 and repinned to e2e842a. #95 now sits on penalties-upstream-base, matching how #70 and #91 are carried. Verified: all five pins merge onto b10359 in pr-set.json order, #70 via the additive resolver as before, everything else clean.
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
build_kda consumes ssm_a through a broadcast ggml_mul, never through ggml_ssm_scan, but it was registered as LLM_TENSOR_SSM_A, which llama-arch.cpp maps to GGML_OP_SSM_SCAN. create_tensor probes the buffer type with that op through weight_buft_supported, no backend offers SSM_SCAN for a [n_head] tensor, the probe fails, and ssm_a is placed on the CPU along with everything downstream of it. LLM_TENSOR_SSM_A_NOSCAN exists for exactly this case and maps to GGML_OP_MUL. Both spellings resolve to the same "blk.%d.ssm_a" name, so there is no GGUF change and no converter change. qwen3next.cpp and qwen35moe.cpp already declare their A this way. Measured on Kimi-K3-Mini across two GPUs, llama-bench pp2048: before 10705.91 +- 445.93 tok/s after 17009.96 +- 360.89 tok/s 1.59x The loader reports it directly. Before the change it prints "tensor 'token_embd.weight' (bf16) (and 4 others) cannot be used with preferred buffer type CUDA_Host, using CPU instead"; after it prints "(and 0 others)".
06d2326 to
edfd4c1
Compare
a614fab to
34af94c
Compare
Prebuilt: repin ggml-org#25731 and unslothai#70 onto post-squash heads
|
Tested 27fd568 on its own (cherry-picked with Setup: unsloth Kimi-K3-UD-Q4_K_XL + mmproj-F16.gguf; llama-server (router mode); 1× RTX 4090 Load log: Results:
One observation: Happy to test if this goes to ggml-org. |
# Conflicts: # src/models/kimi-k3.cpp # tools/mtmd/clip.cpp
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Refreshed this onto The pin had stopped merging in the nightly: Upstream landed the same change as ggml-org#28121 ( The two real fixes in that file are untouched: // required: a silent default here loads cleanly and produces garbage
ml.get_key(LLM_KV_EXPERT_LATENT_LENGTH, hparams.n_expert_latent);
...
GGML_ASSERT(hparams.attn_res_block_size > 0 && "Kimi-K3 requires attn_res.block_size");
GGML_ASSERT(hparams.n_expert_latent > 0 && "Kimi-K3 requires expert_latent_length");
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 883f2c9ba7
ℹ️ 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".
| hparams.image_min_pixels = min_pixels; | ||
| hparams.image_max_pixels = max_pixels; |
There was a problem hiding this comment.
When a Kimi-K3 mmproj contains the positive min/max pixel metadata emitted by this converter, these assignments always win, so --image-min-tokens and --image-max-tokens are ignored despite being copied into custom_image_*_tokens. In particular, users cannot lower the default 4096-token image budget to control memory use. Apply the custom values with the existing set_limit_image_tokens logic, including the patch_size * n_merge conversion, before computing the warmup size.
AGENTS.md reference: AGENTS.md:L80-L80
Useful? React with 👍 / 👎.
3 participants
ggml-org#26185has taken most of whatunslothai#44carried. Its history now has its own copies of the Kimi K3 chat format,message_delimiters, theLLAMA_MAX_EXPERTSbump to 1024, the converter fixes and the archs-test support, and one commit says so directly:fix: apply nits from @ngxson and text fixes from @danielhanchen.What it does not have is the vision half.
MoonViTappears there only inconversion/kimivl.py; there is notools/mtmd/models/kimik3.cpp, so the image path is absent, and the full-size loading fixes are missing too.This is only that remainder, on top of 26185's current head (
a614fab10b), so the diff is what we still add and nothing that is already upstream:kimi-k3 : fixes for loading and running the full-size model(conversion/base.py,src/models/kimi-k3.cpp)kimi-k3 : add the MoonViT-3d vision tower (image path)(the mtmd model, clip wiring, converter, constants)Ten files, +209/-2, against
unslothai#48's stack which duplicates the text-side work and has to be re-reconciled with 26185 every time either side moves.One resolution worth noting: cherry-picking the vision commit brought a
models/inkling.cppline intotools/mtmd/CMakeLists.txt, since our old branch had Inkling merged. That file does not exist here and would have broken the build, so onlymodels/kimik3.cppis added. I checked everymodels/*.cppthe CMakeLists references resolves to a real file.The base branch
kimi-k3-text-upstreamis 26185 ata614fab10b, so this PR reads as just our delta.