[Router][Bugfix] Cache KV-aware tokenizers per model - #1077
Conversation
Fixes vllm-project#1052 Signed-off-by: 姚远 <asmile.yy@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request updates the router to support multiple tokenizers by replacing the single self.tokenizer with a dictionary mapping model names to their respective tokenizers, and introduces a helper method _get_tokenizer to dynamically load them. The feedback highlights a critical issue where _get_tokenizer is synchronous and performs blocking I/O, which would freeze the asyncio event loop. The reviewer suggests making _get_tokenizer asynchronous, executing the blocking tokenizer loading in a thread pool executor, adding error handling for empty endpoints, and awaiting the method calls in route_request and tokenize_prompt.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: baf2537d09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: 姚远 <asmile.yy@gmail.com>
|
@ruizhang0101 Hi, this PR has been open for 5 days without a human review. All current checks are green, and the automated review feedback has been addressed. When you have a chance, could you take a look? Thanks! |
Summary
KvawareRouterandLoadAwareRouterFixes #1052.
Problem
route_general_request()filters endpoints for the requested model, butKvawareRoutercurrently initializes oneself.tokenizerfrom the first model it sees and reuses it for later requests.With model A ->
[1]and model B ->[2], currentmainloads only model A and sends:The expected lookup tokens are:
LoadAwareRouterhas the same singleton-tokenizer path.Relation to #1045
#1045 improves chat tokenization and tokenizer initialization, but its current
_ensure_tokenizer()still stores one successfully loadedrouter.tokenizer. This fix is independent: it makes the cache model-specific. If #1045 lands first, this can be rebased onto that helper.Testing
[[1], [2]]git commit -s.