feat[vLLM]: Support text replacement offsets in the remaining old-format processors - #47614
Conversation
|
I will come to merge this PR after we settle down vLLM updates |
zucchini-nlp
left a comment
There was a problem hiding this comment.
Niiice, looks good to me overall, just left comments about moving some things around if possible
| @@ -55,6 +51,7 @@ def __init__( | |||
| self.image_seq_length = image_seq_length | |||
| self.image_token_id = tokenizer.image_token_id | |||
There was a problem hiding this comment.
i think we also have to update image_token_id to boi_token_id here
There was a problem hiding this comment.
Done, thanks for catching this :)
Needed an image_token_ids override with it otherwise token_type_ids marks the boi instead of the 256 soft tokens. Also updated vLLM to read the soft id off the tokenizer like native gemma3_mm.py
| # Create empty text to be replaced with placeholders | ||
| if images is not None and not text: | ||
| images = self.image_processor.fetch_images(images) | ||
| text = [" ".join([self.boi_token] * len(images)) for images in make_nested_list_of_images(images)] |
There was a problem hiding this comment.
better places in self.prepare_inputs_layout
| if images is not None and text is not None: | ||
| batched_images = make_nested_list_of_images(images) |
There was a problem hiding this comment.
same, make nested list is beter placed in prepare_inputs which is usually called before valid. Here then we can expect inputs are "normalized for model"
| def _check_special_mm_tokens(self, text: list[str], text_inputs: "BatchFeature", modalities: list[str]): | ||
| """ | ||
| Checks that number of special tokens in text and processed text is same. The count can be different | ||
| if tokenized text was truncated, leading to issues in model code. | ||
| """ | ||
| token_str = self.tokenizer.image_token | ||
| token_id = self.tokenizer.image_token_id | ||
| if token_str is not None and token_id is not None: | ||
| ids_count = [list(ids).count(token_id) for ids in text_inputs["input_ids"]] | ||
| text_count = [sample.count(token_str) for sample in text] | ||
|
|
||
| if ids_count != text_count: | ||
| raise ValueError( | ||
| f"Mismatch in `image` token count between text and `input_ids`. Got ids={ids_count} and text={text_count}. " | ||
| "Likely due to `truncation='max_length'`. Please disable truncation or increase `max_length`." |
There was a problem hiding this comment.
can you add a small comment on why overriden - different tokens used for placeholder in input text and expanded text
| "Deprecated: `processor.image_token` will switch from returning " | ||
| "`tokenizer.image_token` to `tokenizer.boi_token` in v5.11." | ||
| def replace_image_token(self, image_inputs: dict, image_idx: int, **kwargs) -> str: | ||
| num_crops = to_py_obj(image_inputs["num_crops"])[image_idx] |
There was a problem hiding this comment.
Removed, num_crops works directly
| ) -> BatchFeature: | ||
| requires_backends(self, ["torch"]) | ||
|
|
||
| text = self._get_validated_text(text) |
There was a problem hiding this comment.
i'd rename this to common self._validate_inputs
| # Audio samples are already collated | ||
| if len(audio) == 1 and audio[0].ndim == 2: | ||
| audio = audio[0] |
There was a problem hiding this comment.
hmm, we didn't have to do that prev, any reason it's required now?
There was a problem hiding this comment.
Yeah it's because make_list_of_audio sees a [2, N] tensor as one sample, so we only ended up with one replacement for two inputs. Before the tensor went straight to the feature extractor which knew it was already collated and returned two audio_embed_sizes
There was a problem hiding this comment.
ahh oke, so iiuc the make_list_of_audio assumes mono-channel audio in all cases?
There was a problem hiding this comment.
Effectively yes you can say that, it never inspects ndim so [2, N] and [2, 1, N] are treated the same and the mono-channel audio assumptions are really's in the FEs, e.g. VibeVoice rejects ndim != 1, Granite takes the [2, N] and gives one embed size per sample etc.
| # Check only if passed explicitly as another value since by default we'll use `pt` | ||
| for group in (kwargs, kwargs.get("text_kwargs", {}), kwargs.get("common_kwargs", {})): | ||
| if group.get("return_tensors", "pt") != "pt": | ||
| raise ValueError(f"{self.__class__.__name__} only supports `return_tensors='pt'`.") |
There was a problem hiding this comment.
nit: lets merge kwargs first so we dont have to infer which possible dict to check for return_tensors. For ex in dia:
transformers/src/transformers/models/dia/processing_dia.py
Lines 159 to 165 in 1ce2865
There was a problem hiding this comment.
Done in the dia shape, thanks for the precedent :)
| if isinstance(text, str): | ||
| text = [text] | ||
| elif not isinstance(text, (list, tuple)): | ||
| raise ValueError("text input must be a string or list of strings") | ||
|
|
||
| if audio is not None: | ||
| audio = self.feature_extractor.fetch_audio(audio, sampling_rate=self.feature_extractor.sampling_rate) | ||
| audio = make_list_of_audio(audio) | ||
| data = self.feature_extractor(audio, **audio_kwargs) | ||
| audio_lengths = data["padding_mask"].sum(dim=-1).cpu().numpy() | ||
| for example in audio: | ||
| if example.ndim != 1: | ||
| raise ValueError(f"Audio should be mono, got shape: {example.shape}") | ||
|
|
||
| # Replace audio duration placeholders in text | ||
| audio_durations = audio_lengths / self.feature_extractor.sampling_rate | ||
| audio_durations_iter = iter(audio_durations) | ||
| audio_durations = iter([len(el) / self.feature_extractor.sampling_rate for el in audio]) | ||
| audio_duration_pattern = re.compile(re.escape(self.audio_duration_token)) | ||
| for i in range(len(text)): |
There was a problem hiding this comment.
self.prepare_inputs? and the valuerror in self.validate
| @property | ||
| def _audio_processor(self): | ||
| # TODO: To be replaced with `audio_processor` | ||
| return getattr(self, "audio_processor", getattr(self, "feature_extractor", None)) | ||
|
|
There was a problem hiding this comment.
huh, hopefully Eustache's upcoming refactor will make this better
|
[For maintainers] Suggested jobs to run (before merge) run-slow: gemma3, granite_speech, vibevoice_asr |
CI recapDashboard: View test results in Grafana |
zucchini-nlp
left a comment
There was a problem hiding this comment.
Great work, left one tiny comment. I will trigger slow CI (please dont push anything until it's done) and when it passes, we can merge :)
| if text is not None and not isinstance(text, str): | ||
| if not isinstance(text, list) or not isinstance(text[0], str): | ||
| raise TypeError("Invalid input text. Please provide a string, or a list of strings") |
There was a problem hiding this comment.
Because prepare_inputs_layout runs before validate_inputs so by then the dict has already become a list of keys so it'll pass this and integer would fail earlier in list(text).copy()
There was a problem hiding this comment.
ah oke, tbh this could be a general check in Mixin/ Not on you ofc, let's merge
| kwargs["padding"] = True | ||
| text_inputs = self.tokenizer(prompt_strings, **kwargs) | ||
| return BatchFeature(data={**text_inputs, **audio_inputs}) | ||
| kwargs.setdefault("audio_kwargs", {}).setdefault("device", device) |
There was a problem hiding this comment.
ah interesting, then it makes sense, I think Eustache's audio processor PR will bring device as a standard kwarg
| # Audio samples are already collated | ||
| if len(audio) == 1 and audio[0].ndim == 2: | ||
| audio = audio[0] |
There was a problem hiding this comment.
ahh oke, so iiuc the make_list_of_audio assumes mono-channel audio in all cases?
|
run-slow: gemma3, granite_speech, vibevoice_asr |
|
This comment contains models: ["models/gemma3", "models/granite_speech", "models/vibevoice_asr"] |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
|
Thanks a lot for your time @zucchini-nlp, looking forward to going back to the vLLM PR :) |
What does this PR do?
→ Change alluding to the discussion in vllm-project/vllm#50107 currently blocking the vLLM PR.
→ vllm-project/vllm#50107 (comment)
→ vllm-project/vllm#50107 (comment)
cc: @zucchini-nlp @eustlb
Code Agent Policy
Before submitting