Repository navigation
Command A+: image input - #838
Conversation
Automated review🟡 No findings on Description
What the review checked
Advisory, not a merge gate. Rules: |
0c7c789 to
2d41d8b
Compare
b675330 to
1ab6755
Compare
2d41d8b to
6503923
Compare
arun-elr
left a comment
There was a problem hiding this comment.
looks clean, the chunked-prefill cut and the TP layout follow the Qwen3.5 pattern. a few small things:
- CI only ran
labelhere since the base is #379's branch. I ran the PR's tests on CPU at 6503923:command_a_plus/tests+test_model_kv_layouts.py90 passed, 17 skipped (CUDA kernels, HF reference), ruff clean. worth a CI rerun once #379 lands. CommandAPlusVisionSubmodulekeeps the defaultcan_batch() -> False, so concurrent image prompts go through the encoder one at a time. the 40/60/83 ms TTFTs are warm single requests; what does image TTFT look like at c16? fine as a follow-up._expand_imagestiles every image before themax_seq_lencheck, so an over-long multi-image prompt does all the processor work before it's refused. could check the token count per image as it goes.- nit: tiles are always cast to bf16 (L299), so an fp32 run gets bf16 pixels. probably fine, just flagging for the reference checks.
1ab6755 to
fa6a0fe
Compare
6503923 to
7e8402c
Compare
Adds the SigLIP encoder and SwiGLU projector from the Cohere2Vision checkpoint as a vision_encoder node on rank 0. Image prompts take a prefill_image walk: the encoder's output replaces the <|IMG_PATCH|> embeddings in the LLM's prepare_inputs, so chunked prefill cuts both together. Text prompts are unchanged. Images are tiled by the checkpoint's own image processor, and each placeholder widens to the tiles' tokens. Token IDs and pixel values match Cohere2VisionProcessor, and validate_checkpoint.py --images matches the HF reference at every step.
Each image's tile count is known only once the processor has run, so the prompt's length is checked after each image instead of after all of them.
7e8402c to
e657b71
Compare
|
Thanks for taking a look @arun-elr! Good call on the encoder. I measured it on 4x GB200 (median TTFT in ms, 16 output tokens):
So images do queue up at c16. The tiles are all the same size, so batching them across requests should be pretty simple. I'll do that in a follow-up. Fixed the length check. It now refuses the prompt as soon as an image doesn't fit, instead of tiling all of them first, and there's a test for it. The bf16 cast is on purpose. Serving runs in bf16 anyway, and it halves what gets sent to the encoder. The HF parity check casts its pixels the same way, and the fp32 encoder test feeds the tower directly, so the reference checks aren't affected. CI should run now that this targets main. Locally, CPU Core passes (6782) and the GPU tests pass on GB200 (869). |
arun-elr
left a comment
There was a problem hiding this comment.
thanks for the numbers. the length check looks right, and cross-request batching as a follow-up sounds good. lgtm
NSagan271
left a comment
There was a problem hiding this comment.
LGTM, one nitpick + I have some live tests still running
Adds image input to Command A+ (#379).
components/vision.py), loaded strictly from the vision weights. It runs as avision_encodernode on rank 0, as Qwen3.5 does at TP4.prefill_imagewalk. The encoder runs first, then the LLM'sprepare_inputswrites its output over the<|IMG_PATCH|>embeddings, so chunked prefill splits both together. Text prompts are unchanged.max_seq_lenis refused at the first image that doesn't fit. Images come from/generatefiles, in the order sent, or from{"type": "image"}items inmessages.Validation (4x GB200, TP4, full checkpoint)
Cohere2VisionProcessorfor one image and for two images (one of them five tiles).validate_checkpoint.py --images(new flag) checks three text and three image prompts x 32 steps against the HF reference. The greedy token matches at 191 of 192 steps. The one miss is a near tie on the 1151-token image prompt, where HF's top two logits are 0.25 apart. The minimum cosine similarity is 0.996, and the maximum RMSE is 0.26 for images against 0.20 for text./generate, single-image, multi-image and mixed image/text concurrent requests all work. A 32-request soak at concurrency 16 returned no errors. Warm time to first token is 40 ms for text, 60 ms with a one-tile image and 83 ms with a five-tile image.Not included: an OpenAI adapter, CUDA graphs for the encoder, caching encoder output across turns, and batching images across requests. Without that, image TTFT at 16 concurrent requests is 400-520 ms vs 126 ms for text. Batching is the next follow-up.