Add support for length-based scoring in prefix cache plugin#1459
Conversation
|
🚨 Unsigned commits detected! Please sign your commits. For instructions on how to set up GPG/SSH signing and verify your commits, please see GitHub Documentation. |
da55286 to
33f74a1
Compare
c6b69f5 to
c528524
Compare
liu-cong
left a comment
There was a problem hiding this comment.
can you also add your benchmark results?
| // The name of the data producer that produces PrefixCacheMatchInfo. | ||
| PrefixMatchInfoProducerName string `json:"prefixMatchInfoProducerName,omitempty"` | ||
| PrefixMatchInfoProducerName string `json:"prefixMatchInfoProducerName,omitempty"` | ||
| PrefixLengthWeight *float64 `json:"prefixLengthWeight,omitempty"` |
There was a problem hiding this comment.
can we use float and int instead of the pointers?
There was a problem hiding this comment.
Pointers are useful for differentiating between fields not in the JSON and fields in the JSON with the default value (0 or 0.0 in this case).
There was a problem hiding this comment.
I decided to go with float and int to keep things simple. Let me know what you think.
|
|
||
| - higher score: more of the request prefix is expected to be reusable from cache | ||
| - lower score: less prefix cache reuse is expected | ||
| If `prefixLengthWeight` is `0.0` (the default), the score simplifies to just the `matchRatio`. |
There was a problem hiding this comment.
This comment should be included in the initial description of how the plugin computes the score.
| ## Configuration | ||
|
|
||
| This plugin does not define any plugin-specific parameters. | ||
| This plugin supports the following configuration parameters: |
There was a problem hiding this comment.
| This plugin supports the following configuration parameters: | |
| This plugin supports the following optional configuration parameters: |
|
/hold until we get good benchmarking results |
|
@azamikram Please rebase with main to solve the CI issues |
ahg-g
left a comment
There was a problem hiding this comment.
I think another approach is to set a minimum for the denominator (8k tokens for example, but we need to do some testing), this will make it simpler since it is just one parameter, not two.
| // The name of the data producer that produces PrefixCacheMatchInfo. | ||
| PrefixMatchInfoProducerName string `json:"prefixMatchInfoProducerName,omitempty"` | ||
| PrefixMatchInfoProducerName string `json:"prefixMatchInfoProducerName,omitempty"` | ||
| PrefixLengthWeight *float64 `json:"prefixLengthWeight,omitempty"` |
There was a problem hiding this comment.
pls clearly document the parameters
| PrefixMatchInfoProducerName string `json:"prefixMatchInfoProducerName,omitempty"` | ||
| PrefixMatchInfoProducerName string `json:"prefixMatchInfoProducerName,omitempty"` | ||
| PrefixLengthWeight *float64 `json:"prefixLengthWeight,omitempty"` | ||
| MaxModelLen *int `json:"maxModelLen,omitempty"` |
There was a problem hiding this comment.
MaxModelLen is confusing (I know vllm uses this name, but it is not accurate), probably MaxContextLength? also, what is the unit? please prefix it with the unit, if tokens, it would be MaxContextLengthTokens
There was a problem hiding this comment.
I agree that MaxModelLen is a bit confusing and its purpose isn't set to a limit on the maximum number of tokens. The goal is to configure the point at which the requests become most important for KV cache usage. I ended up renaming it to prefillSaturationTokens, let me know what you think.
| This produces a normalized score in the range `[0, 1]`: | ||
| Where: | ||
| - `matchRatio = matchBlocks / totalBlocks` (the fraction of the request prefix matched) | ||
| - `matchLengthRatio = min(1.0, matchedBlocks * blockSize / maxModelLen) ^ 2` (square of normalized matched blocks relative to `maxModelLen`) |
There was a problem hiding this comment.
explain why this is applying a square
| | Parameter | Type | Description | Default | | ||
| | :--- | :--- | :--- | :--- | | ||
| | `prefixLengthWeight` | float | Weight of the absolute prefix length in the score. Must be between `0.0` and `1.0`. | `0.0` | | ||
| | `maxModelLen` | integer | The maximum context length (in tokens) supported by the model. Required if `prefixLengthWeight` > `0.0`. | `8192` | |
There was a problem hiding this comment.
where is this default being applied?
| @@ -115,8 +138,19 @@ func (p *Plugin) Score(ctx context.Context, _ *fwksched.InferenceRequest, endpoi | |||
| } | |||
|
|
|||
| if prefixMatchInfo, ok := info.(*attrprefix.PrefixCacheMatchInfo); ok { | |||
There was a problem hiding this comment.
to reduce indentation, pls do the following:
if prefixMatchInfo, ok := info.(*attrprefix.PrefixCacheMatchInfo); !ok {
logger.V(logutil.DEFAULT).Error(nil, "PrefixCacheMatchInfo has unexpected type, assigning score 0", "endpoint", endpoint)
return scores
}
// reset of the scores calculations
return scores
|
@azamikram we would like to get this into the release do you have cycles to address feedback? |
| ```text | ||
| score = matchBlocks / totalBlocks | ||
| score = 0 | ||
| score += matchLengthWeight * matchLengthRatio |
There was a problem hiding this comment.
pls also update the doc according with the renamed matchLengthScore and matchRatioScore.
Let's also have the formula in one line, score += add unnecessary cognitive overhead IMO.
There was a problem hiding this comment.
I updated the doc and the PR description as well. Also add main results in the PR from the doc.
bf0865c to
0752452
Compare
Signed-off-by: Azam Ikram <azamikram@google.com>
What type of PR is this?
/kind feature
What this PR does / why we need it:
The current prefix scorer only considers the match ratio when scoring endpoints, treating all matched prefixes equally regardless of their absolute length.
This PR introduces
matchLengthWeightandmatchLengthScaleTokensparameters to the prefix scorer. The updated scoring function computes a weighted sum of the match ratio and the match length ratio. The match length ratio is non-linear to prioritize KV cache reuse for longer requests relative to thematchLengthScaleTokens.To maintain backward compatibility,
matchLengthWeightdefaults to0.0, preserving the current scoring behavior.Benchmark results and detailed report
Macro Results (RatioOnlyScorer is the baseline and CostAwareScorer is the proposed function)
Use case 1: RatioOnlyScorer never migrates to a cold pod even when the load increases. CostAwareScorer migrates the short requests to a cold pod.

Use case 2: Even when RatioOnlyScorer migrates to a cold pod, it places both short and long request on the cold pod.

Use case 3: When the load increases so much that long requests also need to be migrates to a cold pod, both RatioOnlyScorer and CostAwareScorer converge.

Which issue(s) this PR fixes:
Fixes #1095
Release note (write
NONEif no user-facing change):