WARNING: THIS SITE IS A MIRROR OF GITHUB.COM / IT CANNOT LOGIN OR REGISTER ACCOUNTS / THE CONTENTS ARE PROVIDED AS-IS / THIS SITE ASSUMES NO RESPONSIBILITY FOR ANY DISPLAYED CONTENT OR LINKS / IF YOU FOUND SOMETHING MAY NOT GOOD FOR EVERYONE, CONTACT ADMIN AT ilovescratch@foxmail.com
Skip to content

Conversation

@evacchi
Copy link
Contributor

@evacchi evacchi commented Dec 4, 2025

Fixes #218. Partially based on initial work in #311 and @vMaroon's advice at #311 (review) (if I have understood it correctly).

Opening as draft to see if I am on the right track, early feedback is welcome.

At this time, this is successfully testing a couple of edge cases (nil request, empty request body), just to make sure that the test case run correctly; then there is one proper test case ("test normalized scores with different kv-block hits"), failing at this time

  1. I have lifted pkg/plugins/scorer/testdata/test-model from https://github.com/llm-d/llm-d-kv-cache-manager/tree/98db134628e59dfe527c40000f8d3829c002d99b/pkg/tokenization/testdata/test-model
  2. applying the templates requires Python with the right packages, so we have to setup PYTHONPATH correctly; thus I have also lifted a few sections from https://github.com/llm-d/llm-d-kv-cache-manager/blob/98db134628e59dfe527c40000f8d3829c002d99b/Makefile as well

The reason the test fails is all scores are zero, indicating there might be still some issues initializing the tokenizer. I have to investigate.

Notes:

  • Because of (2) this is sort-of an integration test. I can mark this as a "long" test (if testing.Short() { t.Skip() }) in the same fashion of other tokenization tests in the cache manager.
  • Also notice that there is apparently ongoing work to refactor kvcache.Indexer into an interface, and that would probably simplify this; e.g. by allowing us to inject a mock tokenizer
  • Finally, a misconfigured tokenizer can easily hang; from a cursory glance, I'd say it's because at least some errors fail to bubble up from the task queue; maybe I'll follow up with some fixes around that

@github-actions
Copy link

github-actions bot commented Dec 5, 2025

🚨 Unsigned commits detected! Please sign your commits.

For instructions on how to set up GPG/SSH signing and verify your commits,
please see GitHub Documentation.

@evacchi evacchi force-pushed the precise_prefix_cache_test branch 3 times, most recently from 8fd3821 to c666f2e Compare December 5, 2025 12:58
@evacchi
Copy link
Contributor Author

evacchi commented Dec 5, 2025

notes:

  • the tokenizer contains "typos" and CI complains about it; I could either add excludes to CI, or use an alternative approach
  • the local tokenizer is now working but db.TokensToKVBlockKeys() returns nil 👀

@evacchi evacchi force-pushed the precise_prefix_cache_test branch from 6ff41cd to 39b2a03 Compare December 5, 2025 16:07
@evacchi
Copy link
Contributor Author

evacchi commented Dec 5, 2025

this is now working e2e for Completions. It still hangs for ChatCompletions, I will investigate later. This is still open for comments.

again, hangs are 90% due to errors not bubbling up from the tasks queues of the tokenizer; the response queue should receive errors as well as completed tasks. I'll follow up with an issue and/or PR there once this is done.

@elevran
Copy link
Collaborator

elevran commented Dec 5, 2025

  • the tokenizer contains "typos" and CI complains about it; I could either add excludes to CI, or use an alternative approach

The typos CI check is not required for merge, it's informational.

@evacchi
Copy link
Contributor Author

evacchi commented Dec 8, 2025

added ChatCompletion example

@evacchi evacchi force-pushed the precise_prefix_cache_test branch 2 times, most recently from 9dbab1a to 97da662 Compare December 9, 2025 09:23

// KVBlockIndex returns the underlying kvblock.Index for testing purposes.
func (s *PrecisePrefixCacheScorer) KVBlockIndex() kvblock.Index {
return s.kvCacheIndexer.KVBlockIndex()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: this is only necessary because the test follows the pattern package score_test. If the test were under package score then we could just access s.kvCacheIndexer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think going for package score and adding a nolint directive wherever necessary is cleaner. @elevran what do you think?

In general this scorer is not expected to understand the internals of the kvcache library, it is only concerned with the API exposed by the indexer.

@evacchi evacchi force-pushed the precise_prefix_cache_test branch from 97da662 to 0efed84 Compare December 9, 2025 11:00
@evacchi evacchi marked this pull request as ready for review December 9, 2025 11:01
@evacchi evacchi changed the title [WIP] test: add precise_prefix_cache_test test: add precise_prefix_cache_test Dec 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

add unit tests for kvcache aware scorer

3 participants