[RL+VLM] Avoid retokenization drift for pre-tokenized (token-id) VLM requests - #26555
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces direct token-id space expansion for Kimi VLM prompts to resolve off-by-one length drift issues caused by decoding and re-tokenizing. It adds a dedicated processor module, integrates it into the Kimi K2.5 and VL processors, and includes both a reproduction script and unit tests. The reviewer suggested an optimization in kimi_token_ids.py to calculate token counts directly from existing offsets instead of redundantly calling the media token calculator.
| for image in images: | ||
| num_tokens = processor._processor.media_processor.media_tokens_calculator( | ||
| {"type": "image", "image": image} | ||
| ) | ||
| media_text_parts.append(processor.mm_tokens.image_token * num_tokens) | ||
| mediums.append({"type": "image", "image": image}) |
There was a problem hiding this comment.
The media_tokens_calculator is called again for each image here, which is redundant because the token counts have already been calculated and are encoded in image_offsets as (start, end) ranges. We can optimize this by calculating num_tokens directly from image_offsets using end - start + 1, avoiding redundant CPU overhead.
| for image in images: | |
| num_tokens = processor._processor.media_processor.media_tokens_calculator( | |
| {"type": "image", "image": image} | |
| ) | |
| media_text_parts.append(processor.mm_tokens.image_token * num_tokens) | |
| mediums.append({"type": "image", "image": image}) | |
| for image, (start, end) in zip(images, image_offsets): | |
| num_tokens = end - start + 1 | |
| media_text_parts.append(processor.mm_tokens.image_token * num_tokens) | |
| mediums.append({"type": "image", "image": image}) |
Move the Kimi-specific token-id expansion into the shared process_and_combine_mm_data path, so any VLM that receives a pre-tokenized (list[int]) prompt rebuilds the final input_ids from the user's original tokens instead of the HF re-tokenized prompt. This avoids decode/re-tokenize length drift (decode + re-tokenize is not identity) for all VLMs, not just Kimi. - resolve_image_token_counts: base method using the HF _get_num_multimodal_tokens convention; Kimi overrides it with media_tokens_calculator (its processor lacks that convention). - _expand_input_ids: splice per-image counts into the original ids; O(1) length validation against the HF output; on mismatch, warn and fall back to the HF re-tokenized ids (no worse than before). - Guard the whole path with SGLANG_MM_AVOID_RETOKENIZE (default on). - Remove the bespoke kimi_token_ids.py path and repro script; rewrite the unit test around the generic helpers. Co-authored-by: Cursor <cursoragent@cursor.com>
Simplify the avoid-retokenize branch in BaseMultimodalProcessor (assert the pre-tokenized input_ids is a list and drop the redundant tensor round-trip and length-only safety check), and add coverage: - Unit test (Qwen2.5-VL, Kimi-K2.5): a predefined non-canonical prompt is preserved verbatim with the flag ON and drifts with it OFF. - E2E tests launching real servers with SGLANG_MM_AVOID_RETOKENIZE off/on and comparing prompt_tokens (Qwen2.5-VL 1-GPU; Kimi-K2.5 8-GPU nightly). Co-authored-by: Cursor <cursoragent@cursor.com>
resolve_image_token_counts and _expand_input_ids now raise on genuine failures (missing count API, placeholder/image count mismatch) rather than returning None. The avoid-retokenize path catches the exception, logs the real cause, and falls back to decode+retokenize. Guard the path on raw_images and update the helper unit tests to the raising contract. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Drop the unit and Kimi e2e test files; the Qwen e2e (test_token_id_retokenize_e2e.py) is the single retained test for the avoid-retokenize behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid the abstract base TestCase being collected and run with no model set. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/tag-and-rerun-ci |
Co-authored-by: Cursor <cursoragent@cursor.com>
| and input_ids is not None | ||
| and raw_images | ||
| ): | ||
| assert isinstance( |
There was a problem hiding this comment.
do we need to take care of video/audio placeholder in this PR?
There was a problem hiding this comment.
we can do it later. We don't have use cases for video/audio RL for now.
|
/tag-and-rerun-ci |
…requests (sgl-project#26555) Co-authored-by: Byron Hsu <byron+per@periodiclabs.ai> Co-authored-by: root <root@slurm-h200-209-231.slurm-compute.tenant-slurm.svc.cluster.local> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Mick <mickjagger19@icloud.com>
…requests (#26555) Co-authored-by: Byron Hsu <byron+per@periodiclabs.ai> Co-authored-by: root <root@slurm-h200-209-231.slurm-compute.tenant-slurm.svc.cluster.local> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Mick <mickjagger19@icloud.com>
…requests (sgl-project#26555) Co-authored-by: Byron Hsu <byron+per@periodiclabs.ai> Co-authored-by: root <root@slurm-h200-209-231.slurm-compute.tenant-slurm.svc.cluster.local> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Mick <mickjagger19@icloud.com> (cherry picked from commit f6a5a1b)
…requests (sgl-project#26555) Co-authored-by: Byron Hsu <byron+per@periodiclabs.ai> Co-authored-by: root <root@slurm-h200-209-231.slurm-compute.tenant-slurm.svc.cluster.local> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Mick <mickjagger19@icloud.com>
TL;DR
For RL, we want token-in, token-out on both the inference and training sides so the two see the exact same token ids. Today the VLM path breaks this: when a request comes in as
input_ids, the server decodes them back to a string and then re-tokenizes that string. Decode → re-tokenize is not guaranteed to round-trip, so the prompt the model actually runs on can differ from what the client sent ("retokenization drift").This is harmless for plain text, but in RL it can cause a catastrophic trainer–inference mismatch — especially with router replay, where the recorded expert-routing decisions are indexed per token and must line up exactly.
Example:
Modification
BaseMultimodalProcessor.process_and_combine_mm_data: for a pre-tokenized (list[int]) prompt, rebuild the finalinput_idsfrom the user's original tokens and expand only the image placeholders, instead of adopting the HF re-tokenized prompt. Non-media tokens can no longer drift. The whole path is guarded bySGLANG_MM_AVOID_RETOKENIZE(default on).resolve_image_token_counts: per-image expanded token counts computed without re-tokenizing. The base implementation uses the transformers in-tree convention_get_num_multimodal_tokens(image_sizes=...)(Qwen-VL, Gemma3, GLM4V, ...); Kimi overrides it withmedia_tokens_calculator. On genuine failure it raises; the caller logs the cause and falls back to decode+retokenize._expand_input_ids: copies the original tokens verbatim and expands the i-th image placeholder intocounts[i]copies of the image token id. RaisesValueErrorwhen the number of placeholders does not match the number of images.Test
test/registered/vlm/test_token_id_retokenize_e2e.pylaunches a real Qwen2.5-VL server twice withSGLANG_MM_AVOID_RETOKENIZEoff then on, sending the same predefined non-canonical prompt (the word "Describe" split into "D"+"escribe", which decodes to the same text but re-encodes to the single merged token) plus one image:prompt_tokensshrinks by the drift delta.prompt_tokenskeeps the original length (image placeholder expanded).Asserts
prompt_tokens[on] - prompt_tokens[off] == drift_delta.CI States
Latest PR Test (Base): ⏳ Run #26699777289
Latest PR Test (Extra): ❌ Run #26699777237