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. |
…nd set predictor scheduling to true instead of flase when no flag is present
| 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
|
@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 |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
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 |
|
We should remove the header when we graduate this plugin, for the most part it is to allow A/B testing and quickly turning it off if things are not working as expected. |
A better way would be to add a profile handler that picks profiles based on a header. It would solve your A/B testing issue here as well as any other similar tests in a much more general fashion. |
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.