-
Notifications
You must be signed in to change notification settings - Fork 118
refactor: Replace prefix cache structure with golang-lru #928
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
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Welcome @kfirtoledo! |
Hi @kfirtoledo. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions 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. |
7dc0b4a
to
f3d636b
Compare
just to give more context on this PR - |
/assign |
/ok-to-test |
pkg/epp/scheduling/framework/plugins/multi/prefix/plugin_test.go
Outdated
Show resolved
Hide resolved
pkg/epp/scheduling/framework/plugins/multi/prefix/plugin_test.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Kfir Toledo <[email protected]> Co-authored-by: Maroon Ayoub <[email protected]>
Signed-off-by: Kfir Toledo <[email protected]>
Signed-off-by: Kfir Toledo <[email protected]>
FIXES #960 |
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.
This looks really nice! Just a few nits!
Signed-off-by: Kfir Toledo <[email protected]>
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.
/lgtm
/hold in case you want to handle a couple nits
maxCacheSize int | ||
table map[BlockHash]map[ServerID]*list.Element // from any prefix cache to the cache entry to find the server | ||
ll *list.List // LinkedList to keep track of the order of entries | ||
mu sync.RWMutex |
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.
nit: another optimization is to use different mutex for the hashToPods and podToLRU, but I don't think it's very important.
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.
Also, I used it only for the hashToPods operation, except in the ReportLRUSize, which we can remove if it hurts the performance
// Check if the LRU pod exist | ||
lruForPod, exists := i.podToLRU[pod] | ||
if !exists { | ||
newLRU, err := lru.NewWithEvict[BlockHash, struct{}](i.maxLRUSize, i.makeEvictionFn(pod)) |
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 read the lru code and the reason this could fail is only because if the LRU size is <= 0... Which IMO we can simply handle when initiating the indexer we set maxLRUSize = max(maxLRUSize, 1)
. We can then safely add a comment and ignore this error.
The way you handle the error is OK, but it adds some complexity to read, one might think: what if there is an error, do I end up with inaccurate score?
@@ -136,3 +141,56 @@ func TestPrefixPlugin(t *testing.T) { | |||
|
|||
plugin.PostCycle(context.Background(), cycleState5, &types.ProfileRunResult{TargetPod: pod1}) | |||
} | |||
|
|||
// TestPrefixPluginStress is a stress test for the prefix scoring plugin, using prompts of increasing length. | |||
func BenchmarkPrefixPluginStress(b *testing.B) { |
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.
do you mind sharing some results of running this test?
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.
Because it has become a benchmark test (not a unit test that runs every time), I make it longer- also add a check for prompts with length 1 to 1024:
So with the new implementation pprof :
CPU: Duration: 3.12s
Mem: 716.88MB total
The old one:
CPU: Duration: 29.02s
Mem: 9097.67MB total
/hold I'd like to run some benchmarks with this change. |
@@ -174,29 +165,33 @@ func (m *Plugin) PostCycle(ctx context.Context, cycleState *types.CycleState, re | |||
log.FromContext(ctx).Error(err, "failed to read prefix plugin cycle state") | |||
return | |||
} | |||
m.indexer.Add(state.PrefixHashes, ServerID(targetPod.NamespacedName)) | |||
err = m.indexer.Add(state.PrefixHashes, ServerID(targetPod.NamespacedName)) |
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.
have you considered replacing PostCycle with PostResponse? or is it planned for a follow up PR?
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.
Not in this PR, but we can do it in a following PR (cc @liu-cong)
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.
Both the PreRequest
and PostResponse
extension points will be needed: #971
Yeah that should be a follow up
Signed-off-by: Kfir Toledo <[email protected]>
/lgtm /hold cancel I ran benchmarks comparing this change with existing prefix plugin, using the same setup with #768. I ran the TLDR:
Great work ! @kfirtoledo |
/approve Thanks all for the effort here!
This is great! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kfirtoledo, kfswain 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 |
…sigs#928) * refactor: Replace prefix cache structure with golang-lru Signed-off-by: Kfir Toledo <[email protected]> Co-authored-by: Maroon Ayoub <[email protected]> * fix: rename prefix scorer parameters and convert test to benchmark test Signed-off-by: Kfir Toledo <[email protected]> * feat: Add per server LRU capacity Signed-off-by: Kfir Toledo <[email protected]> * fix: Fix typos and error handle Signed-off-by: Kfir Toledo <[email protected]> * fix: add safety check for LRUCapacityPerServer Signed-off-by: Kfir Toledo <[email protected]> --------- Signed-off-by: Kfir Toledo <[email protected]> Co-authored-by: Maroon Ayoub <[email protected]>
…e it easier to add plugins (#881) * configuration implementation (after rebase...) Signed-off-by: Shmuel Kallner <[email protected]> * Moved plugin registry back to pkg/epp/plugins Signed-off-by: Shmuel Kallner <[email protected]> * Removed unneeded 'forced imports' of scorers Signed-off-by: Shmuel Kallner <[email protected]> * Changed 'profilepicker' to 'profilehandler' in new and old code Signed-off-by: Shmuel Kallner <[email protected]> * Pass the configured SchedulingProfiles to LoadSchedulerConfig Signed-off-by: Shmuel Kallner <[email protected]> * Ensure that both the configText and configFile flags are not specified Signed-off-by: Shmuel Kallner <[email protected]> * Load RequestControl plugins from the configuration Signed-off-by: Shmuel Kallner <[email protected]> * Register all plugin factories Signed-off-by: Shmuel Kallner <[email protected]> * Review fixes Signed-off-by: Shmuel Kallner <[email protected]> * Reverted unneeded change Signed-off-by: Shmuel Kallner <[email protected]> * Updates from review comments Signed-off-by: Shmuel Kallner <[email protected]> * Added a stub interface for plugins to get data from the EPP Signed-off-by: Shmuel Kallner <[email protected]> * Added a temporary implementation of plugins.Handle Signed-off-by: Shmuel Kallner <[email protected]> * Added pluginName and plugins.Handle to plugin factory interface Signed-off-by: Shmuel Kallner <[email protected]> * Updated plugin factory signatures to reflect new API Signed-off-by: Shmuel Kallner <[email protected]> * Updated plugin instantiation to reflect new API Signed-off-by: Shmuel Kallner <[email protected]> * Updated plugin instantiation to reflect new API Signed-off-by: Shmuel Kallner <[email protected]> * Updated tests to reflect new API Signed-off-by: Shmuel Kallner <[email protected]> * Do not rename the imported package Signed-off-by: Shmuel Kallner <[email protected]> * Only upper layer of code should log errors Signed-off-by: Shmuel Kallner <[email protected]> * Only pass what is needed to instantiate the plugins Signed-off-by: Shmuel Kallner <[email protected]> * Review updates Signed-off-by: Shmuel Kallner <[email protected]> * Review update Signed-off-by: Shmuel Kallner <[email protected]> * Review update. Make more clear that the code only checks for already defined names Signed-off-by: Shmuel Kallner <[email protected]> * fixed e2e doc in makefile (does not require GPUs) (#976) Signed-off-by: Nir Rozenbaum <[email protected]> * API: Adds 5xx Status Code for Invalid ExtRef (#991) Signed-off-by: Daneyon Hansen <[email protected]> * feat(conformance): Add test for invalid EPP service reference (#959) * fix boilerplate header * add tests for InferencePoolInvalidEPPService * change to expect error on httproute refcond * moved the creation of the context to main.go. (#995) this is useful when writing a different main like llm-d, allowing to propogate the same context to the whole system. Signed-off-by: Nir Rozenbaum <[email protected]> * fix dead links (#989) * feat: add health check for epp cluster (#966) * feat: add health check for epp cluster Signed-off-by: zhengkezhou1 <[email protected]> * remove tls Signed-off-by: zhengkezhou1 <[email protected]> * don't use tls Signed-off-by: zhengkezhou1 <[email protected]> * health checking flag Signed-off-by: zhengkezhou1 <[email protected]> * fix import Signed-off-by: zhengkezhou1 <[email protected]> * add tls options Signed-off-by: zhengkezhou1 <[email protected]> --------- Signed-off-by: zhengkezhou1 <[email protected]> * Server unit test and utility to help with such tests (#820) Signed-off-by: Ira <[email protected]> * Update dynamic-lora-sidecar to expose metrics to track loaded adapters (#980) * Add a metrics to track loaded adapters * Update the sample manifests * Add explanation of metrics from dyanmic LoRA adapter sidecar * Add explanation of metrics from dyanmic LoRA adapter sidecar (take 2) * Update metrics.md based on feedback * refactor: Replace prefix cache structure with golang-lru (#928) * refactor: Replace prefix cache structure with golang-lru Signed-off-by: Kfir Toledo <[email protected]> Co-authored-by: Maroon Ayoub <[email protected]> * fix: rename prefix scorer parameters and convert test to benchmark test Signed-off-by: Kfir Toledo <[email protected]> * feat: Add per server LRU capacity Signed-off-by: Kfir Toledo <[email protected]> * fix: Fix typos and error handle Signed-off-by: Kfir Toledo <[email protected]> * fix: add safety check for LRUCapacityPerServer Signed-off-by: Kfir Toledo <[email protected]> --------- Signed-off-by: Kfir Toledo <[email protected]> Co-authored-by: Maroon Ayoub <[email protected]> * feat(conformance): Add HTTPRouteMultipleRulesDifferentPools test (#834) * copy of accepted inference pool test to start from. * add yaml file for the test * update time out * update the yaml file to add port 9002 * read timeout config from local repo * remove excess comments * correct spelling for scenarios * check route condition on RouteConditionResolvedRefs * remove empty lines in yaml * set optional/defaulted fields as unspecified * fix timeout * fix boilerplate header * change varialbe names to use primary secondary consistently. * remove extra comments * factor out common code * Add actual http traffic validation using echo-basic * remove extra comments from manifest * remove modifiedTimeoutConfig.HTTPRouteMustHaveCondition per review comment. * intermediate update * fix the test run * factor out common code * move epp def to shared manifest * remove extra comments * revert back to two epps * add to do for epp image * switch to GeneralMustHaveConditionTimeout * undo gateway version changes * remove unused HTTPRouteMustHaveConditions * update doc string for GetPod * update docstring * Remove resource type from names in manifests. * remove type from name * remove health check * add todo for combining getpod methods * configuration implementation (after rebase...) Signed-off-by: Shmuel Kallner <[email protected]> * After review, made code more obvious Signed-off-by: Shmuel Kallner <[email protected]> * Fixed merge issues Signed-off-by: Shmuel Kallner <[email protected]> --------- Signed-off-by: Shmuel Kallner <[email protected]> Signed-off-by: Nir Rozenbaum <[email protected]> Signed-off-by: Daneyon Hansen <[email protected]> Signed-off-by: zhengkezhou1 <[email protected]> Signed-off-by: Ira <[email protected]> Signed-off-by: Kfir Toledo <[email protected]> Co-authored-by: Nir Rozenbaum <[email protected]> Co-authored-by: Daneyon Hansen <[email protected]> Co-authored-by: sina chavoshi <[email protected]> Co-authored-by: Xudong Wang <[email protected]> Co-authored-by: Zhengke Zhou <[email protected]> Co-authored-by: Ira Rosen <[email protected]> Co-authored-by: Shotaro Kohama <[email protected]> Co-authored-by: Kfir Toledo <[email protected]> Co-authored-by: Maroon Ayoub <[email protected]>
Replace the prefix cache structure with
golang-lru
in the prefix indexer.Add
TestPrefixPluginStress
, a stress test for the prefix scoring plugin using prompts of increasing length.