Add weightMode toggle for multimodal cache affinity producer#1397
Add weightMode toggle for multimodal cache affinity producer#1397guygir wants to merge 5 commits into
Conversation
github-actions
Bot
added
the
size/L
3250359 to
6c9fad7
Compare
|
/assign @capri-xiyue |
Signed-off-by: Guy Girmonsky <guygir@gmail.com>
| if !p.useWeighted { | ||
| return nil | ||
| } | ||
| return map[plugin.DataKey]any{tokenproducer.TokenizedPromptDataKey: scheduling.TokenizedPrompt{}} |
There was a problem hiding this comment.
We can declare this as optional, when available we use the length as produced by the token-producer, when not we fall back to 1. No need to have an explicit WeightMode
There was a problem hiding this comment.
Agreed. I removed the explicit weightMode and implemented this as an optional data dependency instead.
I added the small framework primitive because using lengths "when available" needs ordering to be deterministic. Without an optional DAG edge, the MM producer can run before token-producer and silently fall back to 1 even though tokenization is configured and weights DO exist.
With this change, if token-producer is configured, mm-embeddings-cache-producer runs after it and uses TokenizedPrompt.MultiModalFeatures.Length. If token-producer is absent, nothing is auto-created and the producer falls back to unit weight 1.
| return map[plugin.DataKey]any{p.dk: attrmm.EncoderCacheMatchInfo{}} | ||
| } | ||
|
|
||
| // Consumes declares the token-producer dependency only for weighted mode so |
There was a problem hiding this comment.
What will happen is token-producer is declared but with estimate backend? I assume the weighted model will still work as expected?
There was a problem hiding this comment.
Yes, a configured token-producer backend, including estimate, runs before the MM producer through the optional DAG edge, which uses the populated MultiModalFeature.Length values - precise or estimated.
| metadata. | ||
| - `cacheSize` bounds the EPP routing LRU only; it is not a model-server encoder | ||
| cache capacity knob. | ||
| - Unweighted mode remains lightweight: structured chat media blocks are enough and |
There was a problem hiding this comment.
I think token producer with estimate backend is also lightweight. Should the default behavior be use weighted when tokenproducer is available?
There was a problem hiding this comment.
That makes sense, yep. Default behavior is automatic now: when token-producer is present, we use the tokenized multimodal lengths, whether estimated or precise. When it is absent, we keep the lightweight unit-weight fallback.
| if _, exists := itemsByHash[feature.Hash]; exists { | ||
| return | ||
| } | ||
| size := 1 |
There was a problem hiding this comment.
Is weight a better name?
There was a problem hiding this comment.
Indeed, changed the local variable name to weight where it represents scoring contribution.
|
@guygir are you able to address the comments? code freeze is approaching |
Yes, on it. Just made some changes - will push the latest version and address all comments by tomorrow morning. |
1a9e300 to
1fc2f73
Compare
|
Thanks @ahg-g and @capri-xiyue for the reviews, all addressed in the latest commit. The main change is that mm-embeddings-cache-producer now treats TokenizedPrompt as an optional data dependency: if token-producer is configured, the data graph orders the MM producer after it and the producer uses MultiModalFeature.Length. if token-producer is not configured, nothing is auto-created and the producer falls back to unit weight 1 per hash. I added a small OptionalConsumerPlugin framework hook because the alternative was not deterministic. Without a DAG edge, mm-embeddings-cache-producer can run before token-producer and silently fall back to unit weights even when tokenization is configured. A required Consumes() edge would fix ordering but would also require or auto-create token-producer, which is what we wanted to avoid in the first place... Since this commit included a small framework change, I ran additional validations: go test ./pkg/epp/framework/plugins/requestcontrol/dataproducer/multimodal/... all passed on my end. |
|
Thanks @guygir , please rebase. |
1fc2f73 to
b029b4a
Compare
|
Rebased onto current main and switched to upstream DataDependencies.Optional. kept the optional DAG ordering behavior and validated fallback,tokenized-weighted paths with tests |
| extractors: | ||
| - pluginRef: mm-embeddings-cache-producer | ||
| - pluginRef: precise-prefix-cache-scorer | ||
| schedulingProfiles: |
There was a problem hiding this comment.
How were the weights tuned? Why not extend the current defaults (optimized baseline) with the mm-embeddings-cache-scorer?
On a different note, tuning weights is very tough, we should migrate examples to the new approach.
This can be followed up in a different PR.
There was a problem hiding this comment.
They were tuned according to the mm benchmarks, but I agree it’s clearer for this sample to extend the current default/optimized baseline and add mm affinity (conservatively), at least for now.
b029b4a to
28d17d2
Compare
Use TokenizedPrompt as an optional data dependency so multimodal cache affinity can use tokenized placeholder lengths when token-producer is configured, while preserving the lightweight unit-weight fallback when it is absent. Signed-off-by: Guy Girmonsky <guygir@gmail.com>
28d17d2 to
09bdc45
Compare
|
This PR is marked as stale after 21d of inactivity. After an additional 14d of inactivity (7d to become rotten, then 7d more), it will be closed. To prevent this PR from being closed, add a comment or remove the |
|
Hi @guygir, could you fix the CI/CD including lint and test? |
Signed-off-by: guygir <guygir@gmail.com>
5 participants
Added weightMode toggle to the existing mm-embeddings-cache-producer, so multimodal encoder-cache affinity can run unweighted (default) or weighted by tokenized placeholder length. It builds on #901 without introducing separate weighted plugin types (the scorer stays size agnostic and continues to score matchedWeight / totalWeight).
1.1 unweighted mode keeps unit item size (1) and no token-producer dependency
1.2 weighted mode uses MultiModalFeature.Length and declares Consumes(token-producer) when configured
Validation:
Follow-up to #901.