fix: Use per-model tokenizer in KvawareRouter for multi-model setups - #1066
fix: Use per-model tokenizer in KvawareRouter for multi-model setups#1066Asthenia0412 wants to merge 2 commits into
Conversation
KvawareRouter kept a single self.tokenizer that was initialized from endpoints[0].model_names[0] on the first request and reused for every later request. When the router serves multiple base models, requests for model B were tokenized with model A's tokenizer, causing incorrect token IDs to be sent to the LMCache KV-aware lookup. Replace the single tokenizer with a per-model cache (self.tokenizers: Dict[str, AutoTokenizer]) so that each model gets its own tokenizer. The remote /tokenize fallback also uses the correct endpoint for the requested model. Closes vllm-project#1052 Signed-off-by: Asthenia <asthenia0412@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request updates the routing logic in KvawareRouter to support multiple tokenizers by replacing the single self.tokenizer with a dictionary self.tokenizers mapped by model name. Feedback on these changes highlights two main issues: first, removing self.tokenizer will cause an AttributeError in LoadAwareRouter which inherits from KvawareRouter and still references the old attribute; second, if the model key is missing from the request, the empty string default will cause AutoTokenizer.from_pretrained to fail, so a fallback to the first endpoint's model name should be implemented.
| self.session_key = session_key | ||
| self.hash_ring = HashRing() | ||
| self.tokenizer = None | ||
| self.tokenizers: Dict[str, AutoTokenizer] = {} |
There was a problem hiding this comment.
Replacing self.tokenizer with self.tokenizers in KvawareRouter.__init__ will break LoadAwareRouter (which inherits from KvawareRouter). LoadAwareRouter.tokenize_prompt still references self.tokenizer (e.g., if self.tokenizer is None:), which will now raise an AttributeError because self.tokenizer is no longer initialized in KvawareRouter.__init__.
Please update LoadAwareRouter.tokenize_prompt to use self.tokenizers as well, or unify the tokenization logic between the two routers.
| model_name = request_json.get("model", "") | ||
| # Find the endpoint serving this model | ||
| model_endpoint = next( | ||
| (ep for ep in endpoints if model_name in ep.model_names), | ||
| endpoints[0] if endpoints else None, | ||
| ) |
There was a problem hiding this comment.
If request_json does not contain the "model" key, model_name defaults to "". This will cause AutoTokenizer.from_pretrained("") to fail, and the remote fallback will send "model": "" to the remote /tokenize endpoint, which is likely to fail or be incorrect.
To preserve backward compatibility and ensure robustness, we should default model_name to endpoints[0].model_names[0] if it is not provided or is empty.
model_name = request_json.get("model", "")
if not model_name and endpoints and endpoints[0].model_names:
model_name = endpoints[0].model_names[0]
# Find the endpoint serving this model
model_endpoint = next(
(ep for ep in endpoints if model_name in ep.model_names),
endpoints[0] if endpoints else None,
)…dling - Fix tokenize_prompt() used by LoadAwareRouter to use per-model tokenizer cache instead of the removed self.tokenizer - Add validation for missing model name in request body - Refactor KvawareRouter.route_request() to reuse tokenize_prompt() instead of inline tokenization, avoiding code duplication Signed-off-by: Asthenia <asthenia0412@gmail.com>
Description
Fixes #1052
KvawareRouterkeeps a singleself.tokenizerthat is initialized fromendpoints[0].model_names[0]on the first request and reused for every later request. When the router serves multiple base models, requests for model B are tokenized with model A's tokenizer, causing incorrect token IDs to be sent to the LMCache KV-aware lookup.Changes
self.tokenizer(single) withself.tokenizers: Dict[str, AutoTokenizer](per-model cache)route_request(), extract the model name fromrequest_json["model"]and load the corresponding tokenizer on demand/tokenizefallback now uses the correct endpoint for the requested modelTesting
Behavior change verified by the reproducer in the issue:
loaded tokenizers: ["model-a"],lookup tokens: [[1], [1]]loaded tokenizers: ["model-a", "model-b"],lookup tokens: [[1], [2]]