fix: Make TokenizedPrompt.MultiModalFeatures per-prompt#1633
Open
albertoperdomo2 wants to merge 8 commits into
Open
fix: Make TokenizedPrompt.MultiModalFeatures per-prompt#1633albertoperdomo2 wants to merge 8 commits into
albertoperdomo2 wants to merge 8 commits into
Conversation
albertoperdomo2
requested review from
a team,
bongwoobak,
liu-cong,
sagearc,
vMaroon and
zetxqx
as code owners
June 12, 2026 18:51
Contributor
Author
ahg-g
reviewed
Jun 12, 2026
Comment on lines
+115
to
+119
| PerPromptTokens [][]uint32 | ||
| // MultiModalFeatures holds one entry per multimodal item in prompt order. | ||
| // Nil if the prompt contains no multimodal content. Offsets are relative | ||
| // to PerPromptTokens[0] (always single-prompt when multimodal content is | ||
| // present). | ||
| MultiModalFeatures []MultiModalFeature | ||
| // MultiModalFeatures holds multimodal items per prompt, indexed in | ||
| // lockstep with PerPromptTokens. Single-prompt requests use a length-1 | ||
| // outer slice. Nil if the prompt contains no multimodal content. | ||
| MultiModalFeatures [][]MultiModalFeature |
Collaborator
There was a problem hiding this comment.
it is a bigger change, but creating a PromptTokens struct that encapsulates the tokens and the mm features is more idiomatic
type TokenizedPrompt struct {
...
PromptTokens []PromptToken
...
}
type struct {
Tokens []uint32
MultiModalFeatures []MultiModalFeature
}
but we probably need better names, TokenizedPrompt and PromptToken are too close, perhaps ask Claude / Antigravity to suggest better names?
albertoperdomo2
requested review from
LukeAVanDrie,
kaushikmitr and
shmuelk
as code owners
June 15, 2026 09:03
Contributor
Author
|
@ahg-g anything blocking us on this one? |
abdallahsamabd
pushed a commit
to abdallahsamabd/llm-d-inference-scheduler
that referenced
this pull request
Jul 7, 2026
albertoperdomo2
force-pushed
the
fix/per-prompt-mm-features
branch
from
July 8, 2026 10:48
26dffe1 to
e781181
Compare
bongwoobak
reviewed
Jul 8, 2026
Member
|
burstprefix (not in this PR, pulled in by today's main-merge #1727) still uses the old TokenizedPrompt |
albertoperdomo2
force-pushed
the
fix/per-prompt-mm-features
branch
2 times, most recently
from
July 17, 2026 10:20
c6a1c9a to
4175bc7
Compare
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
The auto-merge left one test using the old TokenizedPrompt type and PerPromptTokens field in producer_test.go. Updated to use TokenizedRequest with Prompts/PromptTokens. Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
albertoperdomo2
force-pushed
the
fix/per-prompt-mm-features
branch
from
July 24, 2026 09:44
18f3cdc to
ee25953
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind bug
/kind feature
What this PR does / why we need it:
What this PR does
Introduces a
PromptTokensstruct that bundles token IDs and multimodal features per prompt, replacing the two parallel slices onTokenizedPrompt(PerPromptTokens [][]uint32andMultiModalFeatures []MultiModalFeature). RenamesTokenizedPrompttoTokenizedRequestto distinguish the request-level container from the new per-prompt type.Before:
After:
The association between tokens and their multimodal features is now structural, not positional. Consumers no longer need bounds checks or guards like
if len(tp.PerPromptTokens) == 1to pair features with the correct token array.The
InferenceRequestBodyfield is renamed fromTokenizedPrompttoTokenizedRequestto match the type. The data-key identifiers (TokenizedPromptDataKey,tokenizedPromptKeyID) are intentionally unchanged since they are plugin registry keys, not type names.Consumers updated:
tp.Promptsdirectly; thelen(tp.PerPromptTokens) == 1guard disappears because eachPromptTokenscarries its own features.ExtractMMItems): Iteratestp.Promptsaccessingp.MultiModalFeatures.hasMultimodalContent): Checkslen(p.MultiModalFeatures) > 0per prompt.tp.Promptsusingp.TokenIDs.TokenizedRequestwithPrompts: []PromptTokens{{...}}for pre-tokenized gRPC inputs.Why we need it
PerPromptTokenswas[][]uint32(per-prompt) butMultiModalFeatureswas[]MultiModalFeature(flat). MM feature offsets are only valid relative to one token array, but the flat shape forced consumers to assume which prompt the features belonged to. Theblockkeys.goconsumer had an explicitlen(tp.PerPromptTokens) == 1guard to avoid misapplying features to multi-prompt requests.Bundling tokens and features into
PromptTokenseliminates this assumption and the guard, and makes the per-prompt association structural rather than requiring lockstep indexing of parallel slices.Known follow-up
flattenMMFeaturesinscorer/preciseprefixcache/legacy_producer.goandconvertMMFeaturesToUpstreamintokenizer/tokenizer.goare now identical (same input type, same output type, same logic). Deduplicating them is out of scope here since it crosses package boundaries and the legacy producer is deprecated.Which issue(s) this PR fixes:
Fixes #1584
Release note (write
NONEif no user-facing change):