-
Notifications
You must be signed in to change notification settings - Fork 55
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
Enhancements to LLM Instance Gateway: Scheduling Logic, and Documentation Updates #78
Changes from all commits
dcd4109
bf33d74
235cfca
5d3bcae
0e27908
adb9f8b
fb9aebe
481ec1d
9af96d4
41d18fd
474a95f
bbb343b
b858dd2
6cd2752
93dfe94
5dcf1a8
b8e18cb
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 |
---|---|---|
|
@@ -121,4 +121,4 @@ spec: | |
emptyDir: | ||
medium: Memory | ||
- name: adapters | ||
emptyDir: {} | ||
emptyDir: {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,11 @@ const ( | |
// TODO(https://github.com/kubernetes-sigs/llm-instance-gateway/issues/16) Make this configurable. | ||
kvCacheThreshold = 0.8 | ||
// TODO(https://github.com/kubernetes-sigs/llm-instance-gateway/issues/16) Make this configurable. | ||
queueThreshold = 5 | ||
queueThresholdCritical = 5 | ||
// TODO(https://github.com/kubernetes-sigs/llm-instance-gateway/issues/16) Make this configurable. | ||
// the threshold for queued requests to be considered low below which we can prioritize LoRA affinity. | ||
// The value of 50 is arrived heuristicically based on experiments. | ||
queueingThresholdLoRA = 50 | ||
kaushikmitr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
|
||
var ( | ||
|
@@ -27,9 +31,8 @@ var ( | |
nextOnFailure: sheddableRequestFilter, | ||
} | ||
|
||
// lowLatencyFilter tries to minimize the latency. The heuristic is to pick a server with lower | ||
// cost to load an adapter and has low KV cache, which typically yields lower latency. | ||
lowLatencyFilter = &filter{ | ||
// queueLoRAAndKVCacheFilter applied least queue -> low cost lora -> least KV Cache filter | ||
queueLoRAAndKVCacheFilter = &filter{ | ||
name: "least queuing", | ||
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. update the name? 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 changed the filter names to be more descriptive. |
||
filter: leastQueuingFilterFunc, | ||
nextOnSuccessOrFailure: &filter{ | ||
|
@@ -42,13 +45,39 @@ var ( | |
}, | ||
} | ||
|
||
// queueAndKVCacheFilter applies least queue followed by least KV Cache filter | ||
queueAndKVCacheFilter = &filter{ | ||
name: "least queuing", | ||
filter: leastQueuingFilterFunc, | ||
nextOnSuccessOrFailure: &filter{ | ||
name: "least KV cache percent", | ||
filter: leastKVCacheFilterFunc, | ||
}, | ||
} | ||
|
||
lowLatencyFilter = &filter{ | ||
name: "low queueing filter", | ||
filter: toFilterFunc((lowQueueingPodPredicate)), | ||
nextOnSuccess: &filter{ | ||
name: "affinity LoRA", | ||
filter: toFilterFunc(loRAAffinityPredicate), | ||
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. why not use 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. lowLoRACostPredicate picks both pods with canAcceptNewLoraPredicate and loRAAffinityPredicate, For stronger affinity we want to pick only pods with loRAAffinityPredicate and if no such pod is present only then pick canAcceptNewLoraPredicate 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. Why not do that for the other branch too then? 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. The lowLoRACostPredicate ensures weak affinity by spreading the load of a LoRA adapter across multiple pods, avoiding "pinning" all requests to a single pod. This gave good performance in our initial benchmarking results in the scenario where # of lora slots > # of lora adapters. loRAAffinityPredicate on the other hand ensures strong affinity i.e it pins requests to a single pod with that adapter. Depending on the scenario one or the other might be better. 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. Can we document this reasoning please? 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 added a comment to lowLoRACostPredicate with the reasoning, like we have in leastKVCacheFilterFunc. |
||
nextOnSuccess: queueAndKVCacheFilter, | ||
nextOnFailure: &filter{ | ||
name: "can accept LoRA Adapter", | ||
filter: toFilterFunc(canAcceptNewLoraPredicate), | ||
nextOnSuccessOrFailure: queueAndKVCacheFilter, | ||
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 if we replace This will simplify the code, however at the cost of potentially more confusion with the noop step. It's up to you. 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 agree but also think this will make it more confusing, also i think queueAndKVCacheFilter is something we might need in future. For example, when we the request does not have need a lora adapter we can directly apply queueAndKVCacheFilter instead of checking for lora affinity. |
||
}, | ||
}, | ||
nextOnFailure: queueLoRAAndKVCacheFilter, | ||
} | ||
|
||
sheddableRequestFilter = &filter{ | ||
// When there is at least one model server that's not queuing requests, and still has KV | ||
// cache below a certain threshold, we consider this model server has capacity to handle | ||
// a sheddable request without impacting critical requests. | ||
name: "has capacity for sheddable requests", | ||
filter: toFilterFunc(noQueueAndLessThanKVCacheThresholdPredicate(queueThreshold, kvCacheThreshold)), | ||
nextOnSuccess: lowLatencyFilter, | ||
filter: toFilterFunc(noQueueAndLessThanKVCacheThresholdPredicate(queueThresholdCritical, kvCacheThreshold)), | ||
nextOnSuccess: queueLoRAAndKVCacheFilter, | ||
// If all pods are queuing or running above the KVCache threshold, we drop the sheddable | ||
// request to make room for critical requests. | ||
nextOnFailure: &filter{ | ||
|
@@ -62,6 +91,7 @@ var ( | |
) | ||
|
||
func NewScheduler(pmp PodMetricsProvider) *Scheduler { | ||
|
||
return &Scheduler{ | ||
podMetricsProvider: pmp, | ||
filter: defaultFilter, | ||
|
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.
Leave this comment here but this doesn't need to be addressed in this PR.
We can potentially refactor this predicate to prefer the affinity first, then fall back to canAcceptNewLoRA if no affinity is found. In this case we should be able to consolidate much of the different decision tree branches. Will of course need some benchmark to see the impact.