-
Notifications
You must be signed in to change notification settings - Fork 292
Delegate conditional-decode 412 decision to prefix-based-pd-decider #1684
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
dbfce5b
7bd1128
541981e
cd229c3
4cf1121
765591d
8f5e2e6
4035835
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -413,6 +413,20 @@ The `prefix-based-pd-decider` plugin makes the disaggregation decision according | |
| - `nonCachedTokens`: Number of non-cached tokens that trigger disaggregation | ||
| - If set to 0, disaggregation never occurs for any request | ||
|
|
||
| **Conditional-decode 412 gate** | ||
|
|
||
| When this plugin is configured, it also drives the EPP's RFC 7240 `Prefer: if-available` (conditional-decode) gate. The coordinator uses that header to mark a speculative early-decode attempt: forward to a decode worker only if its KV cache already covers the prompt, otherwise return HTTP 412 Precondition Failed so the coordinator restarts the pipeline at encode/prefill/decode. | ||
|
|
||
| The gate uses the same `nonCachedTokens` threshold as the disaggregation decision: | ||
|
|
||
| - Non-cached suffix `< nonCachedTokens` → forward to the chosen decode worker. | ||
| - Non-cached suffix `≥ nonCachedTokens` → return 412. | ||
| - `nonCachedTokens: 0` → gate disabled (forwards every conditional-decode request). | ||
| - Plugin not declared in the config → gate disabled (the EPP forwards conditional-decode requests unconditionally). | ||
| - Prefix-cache state unreadable on the chosen endpoint (e.g. no approximate-prefix producer wired up) → fail closed (412). | ||
|
|
||
| This behavior differs from earlier releases, where the gate was a hard-coded check inside the director that forwarded on any cache hit and rejected only on a complete cache miss. The threshold is now in tokens (not blocks), is operator-configurable, and is opt-in via the plugin. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should avoid temporal language as instructed in AGENTS.md. Would be interesting to understand if your agent "willingly" disobeyed that instruction or it just lost it in context.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vMaroon , this is an interesting point. In any case, I re-wrote the comment not to contain temporal language |
||
|
|
||
| #### Always-Disagg PD Decider | ||
| The `always-disagg-pd-decider` is a simpler alternative used mainly for testing or benchmarking. | ||
| It always triggers disaggregation, regardless of prefix cache state or prompt characteristics. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,3 +98,12 @@ type PreAdmitter interface { | |
| plugin.Plugin | ||
| PreAdmit(ctx context.Context, request *fwksched.InferenceRequest) error | ||
| } | ||
|
|
||
| // ConditionalDecodeDecider answers the RFC 7240 "Prefer: if-available" gate: | ||
| // given the chosen decode endpoint, should the request be rejected with HTTP | ||
| // 412 (because the local KV cache does not cover enough of the prompt) or | ||
| // forwarded? At most one such plugin is consulted per director. | ||
| type ConditionalDecodeDecider interface { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Description should be self-contained, and descriptive of what the module does.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I re-phrased the description. Thank you! |
||
| plugin.Plugin | ||
| ShouldRejectConditionalDecode(ctx context.Context, request *fwksched.InferenceRequest, endpoint fwksched.Endpoint) bool | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,7 +48,6 @@ import ( | |
| fwkrc "github.com/llm-d/llm-d-router/pkg/epp/framework/interface/requestcontrol" | ||
| fwkrh "github.com/llm-d/llm-d-router/pkg/epp/framework/interface/requesthandling" | ||
| fwksched "github.com/llm-d/llm-d-router/pkg/epp/framework/interface/scheduling" | ||
| attrprefix "github.com/llm-d/llm-d-router/pkg/epp/framework/plugins/datalayer/attribute/prefix" | ||
| "github.com/llm-d/llm-d-router/pkg/epp/handlers" | ||
| "github.com/llm-d/llm-d-router/pkg/epp/metadata" | ||
| "github.com/llm-d/llm-d-router/pkg/epp/metrics" | ||
|
|
@@ -61,49 +60,26 @@ const ( | |
| responseBodyQueueCapacity = 100 | ||
| ) | ||
|
|
||
| // primaryEndpointHasCachedPrefix reports whether the primary profile's chosen | ||
| // endpoint has at least one matching prefix block in its KV cache, as observed | ||
| // by a precise/approximate-prefix scorer during the decode profile run. It | ||
| // returns false when the result is missing, the primary profile produced no | ||
| // endpoint, the endpoint carries no PrefixCacheMatchInfo attribute, or the | ||
| // recorded match has zero blocks. False-return reasons are logged at | ||
| // V(logutil.DEBUG) to disambiguate misconfiguration (no scorer attached) from | ||
| // a real cache miss. | ||
| func primaryEndpointHasCachedPrefix(logger logr.Logger, result *fwksched.SchedulingResult) bool { | ||
| // primaryDecodeEndpoint returns the first endpoint chosen by the primary | ||
| // profile, or nil when the result is empty or malformed. False-return reasons | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the comment is stale.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed, Thank you! |
||
| // are logged at V(logutil.DEBUG) to disambiguate misconfiguration from a real | ||
| // cache miss. | ||
| func primaryDecodeEndpoint(logger logr.Logger, result *fwksched.SchedulingResult) fwksched.Endpoint { | ||
| debug := logger.V(logutil.DEBUG) | ||
| if result == nil { | ||
| debug.Info("conditional-decode: scheduling result is nil") | ||
| return false | ||
| return nil | ||
| } | ||
| primary, ok := result.ProfileResults[result.PrimaryProfileName] | ||
| if !ok || primary == nil { | ||
| debug.Info("conditional-decode: primary profile result missing", "primary", result.PrimaryProfileName) | ||
| return false | ||
| return nil | ||
| } | ||
| if len(primary.TargetEndpoints) == 0 { | ||
| debug.Info("conditional-decode: primary profile produced no endpoints", "primary", result.PrimaryProfileName) | ||
| return false | ||
| } | ||
| endpoint := primary.TargetEndpoints[0] | ||
| if endpoint == nil { | ||
| debug.Info("conditional-decode: primary endpoint is nil") | ||
| return false | ||
| } | ||
| raw, ok := endpoint.Get(attrprefix.PrefixCacheMatchInfoDataKey.String()) | ||
| if !ok || raw == nil { | ||
| debug.Info("conditional-decode: endpoint has no prefix-cache match attribute (no scorer attached?)") | ||
| return false | ||
| } | ||
| info, ok := raw.(*attrprefix.PrefixCacheMatchInfo) | ||
| if !ok { | ||
| debug.Info("conditional-decode: prefix-cache attribute has unexpected type", "type", fmt.Sprintf("%T", raw)) | ||
| return false | ||
| } | ||
| if info.MatchBlocks() == 0 { | ||
| debug.Info("conditional-decode: prefix-cache match has zero blocks") | ||
| return false | ||
| return nil | ||
| } | ||
| return true | ||
| return primary.TargetEndpoints[0] | ||
| } | ||
|
|
||
| // Datastore defines the interface required by the Director. | ||
|
|
@@ -330,14 +306,20 @@ func (d *Director) HandleRequest(ctx context.Context, reqCtx *handlers.RequestCo | |
| // at encode/prefill/decode. Lives in the director (not in a profile handler) | ||
| // so it fires regardless of which profile handler is configured. | ||
| if routing.IsConditionalDecode(reqCtx.Request.Headers) { | ||
| if !primaryEndpointHasCachedPrefix(logger, result) { | ||
| logger.V(logutil.DEBUG).Info("conditional-decode: chosen decode worker has no cached prefix, returning 412") | ||
| return reqCtx, errcommon.Error{ | ||
| Code: errcommon.PreconditionFailed, | ||
| Msg: "no decode worker has the requested KV cache", | ||
| decider := d.requestControlPlugins.ConditionalDecodeDecider() | ||
| if decider == nil { | ||
| logger.V(logutil.DEBUG).Info("conditional-decode: no decider configured, forwarding") | ||
| } else { | ||
| endpoint := primaryDecodeEndpoint(logger, result) | ||
| if endpoint == nil || decider.ShouldRejectConditionalDecode(ctx, reqCtx.SchedulingRequest, endpoint) { | ||
| logger.V(logutil.DEBUG).Info("conditional-decode: chosen decode worker has no cached prefix, returning 412") | ||
| return reqCtx, errcommon.Error{ | ||
| Code: errcommon.PreconditionFailed, | ||
| Msg: "no decode worker has the requested KV cache", | ||
| } | ||
| } | ||
| logger.V(logutil.DEBUG).Info("conditional-decode: chosen decode worker has cached prefix, forwarding") | ||
| } | ||
| logger.V(logutil.DEBUG).Info("conditional-decode: chosen decode worker has cached prefix, forwarding") | ||
| } | ||
|
|
||
| reqCtx.SchedulingRequest.SchedulingResult = result | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if this gate can be described independently, with coordinator use as a note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I re-wrote the comments to describe the gate independently and pull the coordinator behavior into a note. Thank you!