fix(multimodal): give Kimi-K3 its own vision processor and fix alpha ordering - #1984
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
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:
📝 WalkthroughWalkthroughThe PR adds shared MoonViT preprocessing and transparent-image compositing, introduces a Kimi-K3 processor, routes K3 models separately from K2.5, and refactors K2.5 to use the shared pipeline. ChangesKimi MoonViT preprocessing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ModelRegistry
participant KimiK3Processor
participant moonvit_preprocess
participant transforms
participant EncoderInputs
ModelRegistry->>KimiK3Processor: resolve K3 model identifier
KimiK3Processor->>moonvit_preprocess: preprocess images with K3 parameters
moonvit_preprocess->>transforms: composite transparent backgrounds and resize
transforms-->>moonvit_preprocess: normalized image data
moonvit_preprocess->>EncoderInputs: patches and grid metadata
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-structured split. Reviewed the compositing formula, the before/after-resize stage dispatch, the straight-alpha resize path, config resolution with fallback defaults, and the zero-division guards. All paths trace correctly. Tests cover the key invariants thoroughly — no issues found.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![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: 33c23fd299
ℹ️ 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".
| height: u32, | ||
| filter: FilterType, | ||
| ) -> DynamicImage { | ||
| resize_inner(image, width, height, filter, false) |
There was a problem hiding this comment.
For RGBA images that are actually resized and contain partially transparent edges, this straight-alpha path does not match the referenced Pillow pipeline: Pillow converts RGBA/LA images to premultiplied modes for non-nearest resampling, resizes, and converts back. Disabling alpha handling here lets RGB from transparent pixels bleed into neighboring pixels before the chessboard composite, producing incorrect K3 tensors around common antialiased boundaries; use the alpha-aware resize path before compositing instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and I had this backwards — thanks. I checked against Pillow 11.2.1: im.resize(...) on an RGBA image is byte-identical to im.convert("RGBa").resize(...).convert("RGBA"), and differs from convolving the bands independently. So PIL does premultiply, and resize_straight_alpha was the bug rather than the fix.
Dropped the function; the compositing path now uses the default premultiplying resize. kimi_k3::test_after_resize_ignores_colour_hidden_under_alpha pins it — verified to fail when premultiplication is disabled.
There was a problem hiding this comment.
Clean, well-structured refactoring. The MoonViT split is the right call — the shared pipeline in moonvit.rs keeps the delta between K2.5 and K3 explicit, and the transparency compositing correctly matches the reference implementation. Test coverage is thorough. No issues found.
0 🔴 Important · 0 🟡 Nit · 0 🟣 Pre-existing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/multimodal/src/vision/processors/kimi_k25.rs`:
- Around line 301-321: The test test_k25_drops_alpha_rather_than_compositing
only validates fully transparent pixels, which cannot distinguish alpha dropping
from compositing. Extend it with a factor-aligned semi-transparent pixel case
such as Rgba([255, 255, 255, 128]) and assert the preprocessor output matches
convert("RGB") semantics by dropping alpha rather than blending it.
In `@crates/multimodal/src/vision/processors/moonvit.rs`:
- Around line 143-177: The no-background path in moonvit.rs lines 143-177 must
convert alpha-bearing images to RGB before passing them to the premultiplying
resize flow; update the source selection around transparent_bg and resize_fn so
transparent_bg=None with alpha uses image.to_rgb8(), while preserving the
existing fill and straight-alpha behavior for configured backgrounds. In
kimi_k25.rs lines 301-321, extend test_k25_drops_alpha_rather_than_compositing
with a semi-transparent input such as white RGBA alpha 128 so premultiplication
errors are detected.
In `@crates/multimodal/src/vision/transforms.rs`:
- Around line 1499-1610: Strengthen the test
straight_alpha_resize_keeps_rgba_and_ignores_premultiply by using non-uniform
source alpha, such as opaque and fully transparent rows, while keeping RGB
values under transparent pixels nonzero. Assert the resized output retains RGBA,
preserves straight RGB values, and produces the expected interpolated alpha
values rather than merely checking alpha remains zero.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4253a23e-32d8-49d8-a44c-6e3a135f4c6e
📒 Files selected for processing (8)
crates/multimodal/src/registry/kimi_k25.rscrates/multimodal/src/vision/preprocessor_config.rscrates/multimodal/src/vision/processor.rscrates/multimodal/src/vision/processors/kimi_k25.rscrates/multimodal/src/vision/processors/kimi_k3.rscrates/multimodal/src/vision/processors/mod.rscrates/multimodal/src/vision/processors/moonvit.rscrates/multimodal/src/vision/transforms.rs
key4ng
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/multimodal/src/vision/processors/kimi_k25.rs`:
- Around line 323-358: Correct the plane-size calculation in
test_k25_drops_alpha_before_resizing: derive the R-channel plane length from the
patch spatial dimensions in encoder_input.shape(), not from the fixed channel
dimension at shape()[1]. Keep the existing assertion checking flat[..planes] so
it validates the complete first channel plane of each patch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: acee7c23-6b54-41af-8f61-8514d48c9d3f
📒 Files selected for processing (4)
crates/multimodal/src/vision/processors/kimi_k25.rscrates/multimodal/src/vision/processors/kimi_k3.rscrates/multimodal/src/vision/processors/moonvit.rscrates/multimodal/src/vision/transforms.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26fa1c150e
ℹ️ 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".
| let resized = transforms::resize( | ||
| source, | ||
| cfg.new_width as u32, | ||
| cfg.new_height as u32, | ||
| image::imageops::FilterType::CatmullRom, | ||
| ); |
There was a problem hiding this comment.
When a K3 image decodes as ImageLumaA8, ImageLumaA16, or ImageRgba16 and requires the after_resize path, this passes that image directly to transforms::resize, but fir_image_to_dynamic only reconstructs Rgba8 among alpha formats and otherwise discards the premultiplied FIR result in favor of source.resize_exact. Fresh evidence beyond the earlier RGBA finding is the converter's missing LA/16-bit match arms at transforms.rs:416-428; these inputs therefore still use straight-alpha resizing and can bleed hidden color across transparent boundaries before chessboard compositing. Convert surviving-alpha inputs to RGBA8 first or add matching FIR output conversions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cf58df958
ℹ️ 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".
| #[serde(rename_all = "lowercase")] | ||
| pub enum TransparentBgPattern { | ||
| White, | ||
| Black, | ||
| Gray, |
There was a problem hiding this comment.
grey background spelling
When a checkpoint selects the supported solid-grey background with "pattern": "grey", rename_all = "lowercase" only allows this variant as "gray". Deserializing the entire TransparentBgConfig therefore fails, and resolved_transparent_bg silently falls back to K3's chessboard default, producing different pixels for transparent images. Add a serde rename or alias for the reference spelling.
Useful? React with 👍 / 👎.
Kimi-K3 was routed to `KimiK25Processor`, which drops the alpha channel instead of compositing over the chessboard background K3 ships in its `preprocessor_config.json`, and hardcodes K2.5's `in_patch_limit` of 16384 instead of K3's 65536. - Extract the shared MoonViT pipeline into `processors::moonvit` so the K2.5/K3 delta is explicit; K2.5 keeps its previous behavior (no transparency handling) bit-for-bit. - Add `KimiK3Processor` with K3's shipped defaults, resolving `in_patch_limit`, `patch_limit_on_one_side`, `transparent_bg_config` and `transparent_bg_fill_stage` from the model config at call time. - Add `transforms::fill_transparent_bg` (chessboard/black/white/grey patterns) plus `resize_straight_alpha`, so post-resize compositing convolves straight alpha the way the PIL reference does rather than premultiplying. - Lift the Kimi limits and K3 transparency keys out of nested `media_proc_cfg` in `PreProcessorConfig`. - Register `kimi-k3` / `kimi_k3` separately in the processor registry. Signed-off-by: key4ng <rukeyang@gmail.com>
…licit null bg config
The MoonViT resize path handled alpha two ways, both diverging from the
reference:
- With no `transparent_bg_config` (K2.5), the RGBA image went straight
into the resizer and alpha was dropped afterwards. The reference
converts at load time (`_to_pil` -> `.convert("RGB")`), so it
convolves the RGB stored under transparent pixels; premultiplying
first discards it. Pre-existing on main, not introduced by K3.
- On the `after_resize` path, `resize_straight_alpha` disabled
premultiplication on the theory that PIL convolves bands
independently. It does not: on Pillow 11.2.1 `Image.resize` for RGBA
is byte-identical to `convert("RGBa").resize(...).convert("RGBA")`.
The default premultiplying resize was already correct, so drop the
variant.
Also make K3's compiled-in board switchable. Defaults stay in the
processor because `preprocessor_config.json` is optional in this runtime,
but a checkpoint that wants the reference's alpha-dropping behaviour now
has an escape hatch via `"transparent_bg_config": null`.
Regression tests cover both orderings and were confirmed to fail with
each fix reverted.
Signed-off-by: key4ng <rukeyang@gmail.com>
… tests Signed-off-by: key4ng <rukeyang@gmail.com>
… test shape()[1] is the fixed channel dimension, so shape()[1] / 3 was always 1 and the assertion only covered a single pixel. Index the tensor directly and check every patch's full R plane, plus G/B, so a bug corrupting only part of a patch is caught too. Signed-off-by: key4ng <rukeyang@gmail.com>
0cf58df to
5bddbfb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/multimodal/src/vision/processor.rs (1)
331-346: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a deterministic matcher if overlapping patterns are possible.
VisionProcessorRegistry::find_in_candidatewalks aHashMap, so pattern precedence is not stable; the new Kimi K2/K3 entries are disjoint and fine, but any future overlap will route nondeterministically.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/multimodal/src/vision/processor.rs` around lines 331 - 346, Update VisionProcessorRegistry::find_in_candidate to use deterministic pattern matching rather than relying on HashMap iteration order, defining an explicit precedence for overlapping registered patterns while preserving the existing Kimi K2 and K3 registrations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/multimodal/src/vision/transforms.rs`:
- Around line 136-149: Guard the image conversion flow before the
`rgba.as_raw().chunks_exact(...)` loop so `width == 0` or another zero-sized
input returns an empty `RgbImage` without invoking `chunks_exact` with a zero
chunk size. Preserve the existing pixel conversion behavior for non-zero
dimensions.
---
Outside diff comments:
In `@crates/multimodal/src/vision/processor.rs`:
- Around line 331-346: Update VisionProcessorRegistry::find_in_candidate to use
deterministic pattern matching rather than relying on HashMap iteration order,
defining an explicit precedence for overlapping registered patterns while
preserving the existing Kimi K2 and K3 registrations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f1b8dc6-95cb-4c23-962f-fafd0949ebca
📒 Files selected for processing (8)
crates/multimodal/src/registry/kimi_k25.rscrates/multimodal/src/vision/preprocessor_config.rscrates/multimodal/src/vision/processor.rscrates/multimodal/src/vision/processors/kimi_k25.rscrates/multimodal/src/vision/processors/kimi_k3.rscrates/multimodal/src/vision/processors/mod.rscrates/multimodal/src/vision/processors/moonvit.rscrates/multimodal/src/vision/transforms.rs
End-to-end validation on a B300 node: main vs this branch vs pure vLLMI built four images that isolate each of the two defects, computed the expected numbers from the checkpoint's own vision processor, then served the same four images through three stacks: the pre-fix gateway, this branch, and a plain Results
This branch and pure vLLM agree case for case, including which rungs fail on C — both read R1–R7 and miss R8 (11px) and R9 (9px). Main disagrees with vLLM on A and C. On A, main's reasoning trace is explicit about what it received: "an image that appears to be completely black … entirely black with no discernible text". On C it does not report a blank image — it invents nine plausible-looking 4-character codes, none of them correct. Test images, expected numbers, token accounting, and setupTest imagesGenerated by a deterministic script (fixed text, fixed positions, DejaVu Sans Bold), so the files are reproducible.
A plain alpha-drop turns A and C into a uniformly black image; I verified this directly (RGB-plane luminance extrema Prompts: A/D ask for a transcription with an explicit "if you cannot see any text at all, reply NO TEXT VISIBLE" escape hatch, scored by substring hits on three known strings. B/C ask for exactly nine Expected numbers, from the checkpoint's own processor
The shipped config is Before touching any server I also ran this branch's processors over the same four files through Token accountingText-only
vLLM's totals run ~76 tokens above the gateway's for the same case because its chat template is more verbose. That is the separate SMG-vs-vLLM prompt-format difference, not something this PR touches. Setup
Two honest caveats, and a methodology trap worth flagging for anyone reproducing thisTwo honest caveats
A methodology trap worth flagging for anyone reproducing thisMy first attempt produced a nonsense result: the pre-fix gateway appeared to read C at 8/9, from a tensor that is provably a constant. The cause is that
A fresh, never-shared transparent image confirmed the pre-fix behaviour: no answer at all, the full 3000 tokens spent hallucinating "a wide black background with nine small white text labels scattered". The numbers above were then produced by giving each stack its own byte-distinct but pixel-identical copy of every image (differing PNG The operational consequence is worth noting independently of this PR: rolling out a gateway whose preprocessing has changed, against an engine that keeps caching by raw-byte hash, can either kill EngineCore or silently serve truncated embeddings for the same image. Restarting or draining the engine alongside such an upgrade avoids both. |
1 participant
Description
Problem
Kimi-K3 (#1968) was routed to
KimiK25Processor. The two models share the MoonViT encoder, but not the pixel pipeline, so K3 images were preprocessed in ways that diverge from the reference (kimi_k3_vision_processing.py):Transparency is dropped instead of composited. K3 ships a
transparent_bg_config(chessboard, 8px squares, 255/180, square on top-left) withtransparent_bg_fill_stage: "after_resize", and the reference paints RGBA input over that generated board. SMG calledto_rgb8(), which discards alpha without compositing — and a fully transparent pixel normally stores RGB(0,0,0), so the encoder saw solid black (normalized-1.0) where the reference sees a light checkerboard. Every image with an alpha channel (screenshots, logos, icons, charts exported with transparency) got a systematically wrong pixel tensor.The patch budget is a quarter of K3's. K3's
in_patch_limitis65536; K2.5's is16384. The registry builds processors with::new(), so K3 inherited the hardcoded K2.5 constant and images above the smaller budget were downscaled about 2x more per side than they should have been, losing resolution the model was trained to use.Alpha is dropped after the resize, not before — pre-existing on
mainand shared by K2.5. The reference converts at load time (_to_pil→.convert("RGB")), so it convolves the RGB stored underneath transparent pixels. SMG fed the RGBA image tofast_image_resize, which premultiplies by default, and only dropped alpha afterwards, discarding that colour instead.Solution
Split the two processors rather than widening
KimiK25Processor, so the delta between the models stays explicit and small:processors::moonvitholds the shared pipeline (patch-budget resize solve → resize → pad topatch_size * merge_size→ normalize → NaViT patchify), following the existingqwen_vl_base.rsfamily-base convention.KimiK3Processorcarries K3's shipped defaults and resolvesin_patch_limit,patch_limit_on_one_side,transparent_bg_configandtransparent_bg_fill_stagefrom the model'spreprocessor_config.jsonat call time, so a checkpoint shipping different values is honored instead of being overridden by compiled-in constants. An absent or malformed key keeps K3's board —preprocessor_config.jsonis optional in this runtime, and each processor is expected to supply its own model's defaults (grpc/multimodal/config.rs). A checkpoint that genuinely wants the reference's alpha-dropping path says so with an explicit"transparent_bg_config": null.KimiK25Processorbecomes a thin delegation that passestransparent_bg: None.transforms::fill_transparent_bgimplements the reference background formula (chessboard/black/white/grey) including partial-alpha blending, and both fill stages are honored.PIL.Image.resizedoes for RGBA — verified against Pillow 11.2.1, whereim.resize(...)is byte-identical toim.convert("RGBa").resize(...).convert("RGBA")and differs from convolving the bands independently.The prompt/placeholder spec in
registry/kimi_k25.rsstays shared (K3 uses the same<|media_pad|>and patch layout); a comment now records that the pixel pipelines are not.Behavior change for K2.5: opaque input is bit-identical to before, but alpha-bearing input now matches the reference instead of being premultiplied first. Fix (3) is a divergence that predates #1968; it is bundled here because the fix lives in the shared
moonvitpath this PR introduces.Two K3 differences are deliberately out of scope:
{width}x{height}alongside the media token. That belongs to the chat-template / prompt-encoding layer, not the vision processor.resize_bicubic_pilthatqwen_vl_base.rsuses.transforms.rsitself warns that this divergence "amplifies into a large embedding shift". It predates feat(kimi-k3): add K3 support #1968 and applies equally to K2.5, so it deserves its own PR.Changes
vision/processors/moonvit.rs(new) — shared MoonViT core:MoonVitParams, resize-config solve, resize/pad/normalize, patch extraction.vision/processors/kimi_k3.rs(new) —KimiK3Processorwith K3's defaults and call-time config resolution.vision/processors/kimi_k25.rs— reduced to a delegation overmoonvit.vision/transforms.rs—TransparentBgPattern,TransparentBgConfig,TransparentBgFillStage,TransparentBg,fill_transparent_bg.vision/preprocessor_config.rs— liftin_patch_limit,patch_limit_on_one_side,transparent_bg_configandtransparent_bg_fill_stageout of nestedmedia_proc_cfginto theextramap.vision/processor.rs— registerkimi-k3/kimi_k3as their own processor.vision/processors/mod.rs— export the new modules.registry/kimi_k25.rs— document that K3 shares the prompt spec, not the pixels.Test Plan
New tests, and what they pin:
kimi_k3::test_transparent_pixels_composite_over_chessboard0(black) →255/180chessboardkimi_k3::test_chessboard_phase_matches_reference(0,0)=255,(8,0)=180forsquare_on_top_left=true, matching the numpy referencekimi_k3::test_semi_transparent_blends_toward_backgroundkimi_k3::test_after_resize_ignores_colour_hidden_under_alphakimi_k3::test_larger_patch_budget_keeps_more_resolutionkimi_k3::test_from_preprocessor_config_reads_limits,test_transparent_bg_config_overridden_by_model_config,test_fill_stage_before_resize_changes_outputkimi_k3::test_explicit_null_config_disables_compositing"transparent_bg_config": nullreaches the alpha-dropping pathkimi_k3::test_rgb_input_matches_k25_pipeline,test_opaque_pixels_are_untouchedkimi_k25::test_k25_drops_alpha_before_resizingkimi_k25::test_in_patch_limit_resolved_from_configtransforms::chessboard_background_matches_referenceon_top_leftvaluespreprocessor_config::test_parse_kimi_k3_transparency_settingspreprocessor_config.jsonparses into the lifted fieldsprocessor::test_registry_separates_kimi_k25_and_k3moonshotai/Kimi-K3→kimi-k3,moonshotai/Kimi-K2.5→kimi-k2.5, both by name and bymodel_typeBoth alpha-ordering fixes were confirmed to be load-bearing by reverting each one and watching the matching test fail.
Note:
cargo clippy --all-features(and therefore theclippypre-commit hook) could not run locally — theopencvfeature's build script needs apkg-config/ OpenCV toolchain that isn't installed on this machine. Default-feature clippy is clean workspace-wide; CI covers the full feature set.Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit