Skip to content

Conversation

zhengkezhou1
Copy link

fix #218

@elevran
Copy link
Collaborator

elevran commented Aug 25, 2025

/assign @vMaroon

@elevran
Copy link
Collaborator

elevran commented Aug 25, 2025

@zhengkezhou1 kindly please rebase and sign your commits

@elevran
Copy link
Collaborator

elevran commented Aug 25, 2025

/cc @vMaroon

@github-actions github-actions bot requested a review from vMaroon August 25, 2025 15:38
@zhengkezhou1 zhengkezhou1 force-pushed the prefix-cache-scorer-unit-test branch from 9aa3959 to e6b8203 Compare August 25, 2025 15:46
@zhengkezhou1
Copy link
Author

all commits are signed now.

@elevran
Copy link
Collaborator

elevran commented Aug 25, 2025

the commits do not appear to be signed/verified yet.
We're looking for the verified marking which is is still missing. It should look something like the below.
image

See https://docs.github.com/en/authentication/managing-commit-signature-verification

@zhengkezhou1 zhengkezhou1 force-pushed the prefix-cache-scorer-unit-test branch from e6b8203 to c8e1e2a Compare August 25, 2025 16:24
@zhengkezhou1
Copy link
Author

ok, its works now right?


for _, tt := range testcases {
t.Run(tt.name, func(t *testing.T) {
mockScorer := &mockPodScorer{
Copy link
Member

@vMaroon vMaroon Aug 25, 2025

Choose a reason for hiding this comment

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

The precise prefix-cache scorer is a wrapper of the kvcache.Indexer. The wrapping logic is mostly normalization.

What this test ends up doing is test the latter - which is great, but can be achieved by testing the indexedScoresToNormalizedScoredPods function directly instead of using a mock PodScorer interface.

I think this test should verify correctness of the scorer as-is. This would require:

  1. Access to the indexer's backing kvblock.Index
  2. Having the test populate the kvblock.Index with kv-block information
  3. Testing correctness of the scorer as-is

Copy link
Author

Choose a reason for hiding this comment

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

Adding data to the kvblock.Index alone isn't sufficient; we also need to add data to the tokensIndexer. However, the kv-cache-manager currently only allows access to the KVBlockIndex. Can we add a method to the kv-cache-manager to expose the tokensIndexer?

Copy link
Member

@vMaroon vMaroon Aug 29, 2025

Choose a reason for hiding this comment

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

You can add data to the tokenization cache by making the calls in a "warmup" step (call once, get no scores). Note that this behavior will change soon once tokenization is synchronous, after-which there would be no need for such a step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add unit tests for kvcache aware scorer
3 participants