fix infinite loop in profile picker and switch predictor based routing to on by default with a header to disable #1929
Conversation
…redictor based scheduling off
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
the
cncf-cla: yes
BenjaminBraunDev
changed the title
k8s-ci-robot
added
the
size/XS
BenjaminBraunDev
changed the title
k8s-ci-robot
added
size/S
…nd set predictor scheduling to true instead of flase when no flag is present
k8s-ci-robot
added
size/M
BenjaminBraunDev
changed the title
BenjaminBraunDev
changed the title
| headerValue, ok := request.Headers[headerName] | ||
| if !ok { | ||
| return false, nil // Header not found, return 0 and false | ||
| return true, nil // Header not found, return true by default |
There was a problem hiding this comment.
If a Boolean header is not found, why should the default value be true?
Please revert this change.
There was a problem hiding this comment.
@kfswain @ahg-g @kaushikmitr and I discussed this and since this is all behind the helm deployment flag to begin with, it makes the most sense for this header to specifically be used to disable the predictive routing, it should be on by default.
There was a problem hiding this comment.
Updated header to a clear "off" flag and updated documentation.
|
In general, the header utility functions should be removed from both the profile modified in this PR and in SLO Aware plugin package. They should be added as functions on the LLMRequest struct. |
This was mainly done to keep all SLO routing change isolated to its associated plugins and modify as little non-plugin code as possible. |
…d make it on by default when deploying with latency based routing
k8s-ci-robot
added
size/L
BenjaminBraunDev
changed the title
|
@ahg-g Could you help get this PR approved? It's part of what's necessary for vertex to do AB testing with latency prediction. @kaushikmitr has a PR out for getting approval for the plugin package, but this affects a few files outside the package. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, BenjaminBraunDev The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
I don't understand why there is a header to disable the SLO based scheduling. Just don't use the plugin in the configuration. Also the header name is strange. x-prediction-based-scheduling-off seems strange. It should have been x-disable-prediction-based-scheduling |
4 participants
Small bugfix for latency routing profile picker (it wasn't returning an empty profile map when sending a request with predictor routing header set to false and the profiles all run).
This PR both corrects that logic and also turns predictor based routing to on by default (when deploying with latency based routing flag in helm charts) and replaces the old flag with a flag that turns it off when included instead.