Open
Conversation
acardace
requested review from
dannyharnik,
elevran,
kfirtoledo and
vMaroon
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the GetPodScores method to accept pre-tokenized tokens rather than performing tokenization internally. This separation of concerns allows callers to handle tokenization errors explicitly and reuse tokens across multiple calls.
Changes:
- Modified
Tokenizemethod inpkg/tokenization/pool.goto return([]uint32, error)instead of just tokens - Updated
GetPodScoressignature to accepttokens []uint32and removedrenderReqandpromptparameters - Added new
Tokenizemethod toIndexerthat wraps the tokenization pool's Tokenize method - Updated all callers to tokenize prompts before calling
GetPodScoresand handle tokenization errors - Enhanced test coverage for error handling in tokenization failures
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/tokenization/pool.go | Modified Tokenize to return error, added error field to tokenizationResponse |
| pkg/tokenization/pool_test.go | Updated tests to verify error handling and fixed benchmark to ignore return values |
| pkg/kvcache/indexer.go | Refactored GetPodScores to accept tokens, added Tokenize wrapper method |
| tests/e2e/redis_mock/e2e_test.go | Updated all test cases to tokenize before calling GetPodScores |
| examples/valkey_example/main.go | Added tokenization step with error handling, reuses tokens across calls |
| examples/kv_events/online/main.go | Added tokenization with proper error handling in HTTP handlers |
| examples/kv_events/offline/main.go | Added tokenization step with error handling |
| examples/kv_cache_index_service/server/server.go | Added tokenization in GetPodScores RPC handler |
| examples/kv_cache_index_service/server/main.go | Added tokenization in main demo code |
| examples/kv_cache_index/main.go | Added tokenization with error handling, reuses tokens |
| examples/kv_cache_aware_scorer/kvcache_aware_scorer.go | Added tokenization in Score method with error handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
acardace
force-pushed
the
feat/getpodscores-tokens
branch
from
5226b29 to
c671aba
Compare
Change GetPodScores signature to accept tokens instead of handling tokenization internally. Signed-off-by: Antonio Cardace <acardace@redhat.com>
Signed-off-by: Antonio Cardace <acardace@redhat.com>
acardace
force-pushed
the
feat/getpodscores-tokens
branch
from
c671aba to
cc03a33
Compare
|
This PR is marked as stale after 21d of inactivity. After an additional 14d of inactivity (7d to become rotten, then 7d more), it will be closed. To prevent this PR from being closed, add a comment or remove the |
liu-cong
requested changes
Feb 20, 2026
| res := <-resultCh | ||
| tokens := res.Tokens | ||
| return tokens | ||
| if res.Err != nil { |
Collaborator
There was a problem hiding this comment.
nit: I think return res.Tokens, res.Err should be enough
| traceLogger.Info("Both chat/completions and completions present; defaulting to chat/completions") | ||
| } | ||
|
|
||
| renderReq := &preprocessing.ApplyChatTemplateRequest{ |
Collaborator
There was a problem hiding this comment.
It seems that the chat template is lost with this refactoring.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refactors
GetPodScoresto accepttokens []uint32instead ofrenderReqandpromptparameters.Tokenizemethod toIndexerthat returns([]uint32, error)GetPodScoressignature to accepttokens []uint32directlyFixes #244