feat: usage limit policy plugin#2530
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi @evacchi. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
c0addfa to
5f1a9fc
Compare
LukeAVanDrie
left a comment
There was a problem hiding this comment.
The new interface and its integration LGTM. Thanks for picking this up! I just have some feedback on the config loading path so we are consistent with other plugins.
| registry contracts.FlowRegistry, | ||
| sd contracts.SaturationDetector, | ||
| podLocator contracts.PodLocator, | ||
| usageLimitPolicy flowcontrol.UsageLimitPolicy, |
There was a problem hiding this comment.
nit: We may be getting to a point where we want to group dependencies into a config struct. Not blocking this PR on this refactoring effort though.
E.g.,
type Dependencies struct {
Registry contracts.FlowRegistry
SaturationDetector contracts.SaturationDetector
PodLocator contracts.PodLocator
UsageLimitPolicy flowcontrol.UsageLimitPolicy
Clock clock.WithTicker
}
...
func NewFlowController(ctx context.Context, poolName string, config *Config, deps Dependencies) { ... }
There was a problem hiding this comment.
very good point, been thinking the same. Let's schedule this and the other refactoring. I can take this one if you'd like
There was a problem hiding this comment.
do we have an issue tracking all followups?
There was a problem hiding this comment.
created issue for this #2764 will self-assign (the other one, i.e. unwrap the config value, I am doing in this PR)
|
thanks for suggestions, will address this tomorrow! |
1d91727 to
0b60d58
Compare
d29a574 to
8690c86
Compare
|
rebasing |
8690c86 to
fb7e8da
Compare
|
double-checked integration into llm-d-inference-scheduler with a plugin impl there, and it should be fine |
c81252e to
18e6590
Compare
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
|
/lgtm @ahg-g; mind taking a look at this when you get a moment? |
|
/ok-to-test |
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
|
@ahg-g good to go now |
| registry contracts.FlowRegistry, | ||
| sd contracts.SaturationDetector, | ||
| podLocator contracts.PodLocator, | ||
| usageLimitPolicy flowcontrol.UsageLimitPolicy, |
There was a problem hiding this comment.
do we have an issue tracking all followups?
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, evacchi, LukeAVanDrie 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 |
…e-extension#2530) * feat: usage limit policy plugin Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com> * formatting Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com> * remove leftover runner_test.go Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com> * missing boilerplate header Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com> * replace Noop policy with StaticUsageLimitPolicy Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com> * hoist check to the top of dispatchCycle() Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com> * drop UsageLimitConfig wrapper Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com> * formatting Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com> * linting Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com> * apply suggestions Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com> --------- Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR introduces pluggable
UsageLimitPolicyinterface. The goal is to complement "Improved Flow Control Request Management". The PR is a follow up to the initial discussion in #2268The interfaces may be used to implement the proposal in the doc, as well as dynamically-adaptable behavior (as demonstrated here).
a
UsageLimitPolicyreturns a list of "ceilings" for a given list of priorities:We provide a few example/utility implementations:
usagelimits.DefaultPolicy()that always returns 1.0 for all given prioritiesusagelimits.NewConstPolicy(usageLimitName string, threshold float64)constructor for fixed thresholds for all given prioritiesusagelimits.NewPolicyFunc(...)constructor to define a policy by afuncNote
This PR implicitly requires pool-wide saturation addressed in #2343; thus, this PR should ideally be merged after #2343, and it's likely to require a rebase then.
Which issue(s) this PR fixes:
Related #1861, #2268, #2343, Improved Flow Control Request Management
Does this PR introduce a user-facing change?: