[Router] kvaware/loadaware remote /tokenize: send messages for chat-c… - #1050
[Router] kvaware/loadaware remote /tokenize: send messages for chat-c…#1050tyler2cr wants to merge 1 commit into
Conversation
…ompletions bodies (minimal interim patch) MINIMAL carry-patch for deployments that always take the remote /tokenize path (served-model-name != hub id, so the local tokenizer never loads): the remote payload maps to vLLM's TokenizeChatRequest when the body has messages, instead of tokenizing prompt='' for every chat request. This is the interim, easy-to-rebase subset of PR vllm-project#1045 for downstream image builds while the full PR (local chat-template tokenization, executors, failure caching, tests) is in upstream review. Superseded by Signed-off-by: tyler <tcr@enfuse.io> vllm-project#1045 when it lands.
There was a problem hiding this comment.
Code Review
This pull request updates the remote /tokenize fallback logic in route_request and tokenize_prompt to support chat-completions requests by sending messages and setting add_generation_prompt to True instead of using the prompt field. The review feedback highlights two critical correctness issues where the local tokenizer could bypass the fallback logic by successfully encoding an empty prompt when messages is present, and a performance issue where a synchronous HTTP request blocks the event loop in route_request.
| if "messages" in request_json: | ||
| data = { | ||
| "model": endpoints[0].model_names[0], | ||
| "messages": request_json["messages"], | ||
| "add_generation_prompt": True, | ||
| } | ||
| else: | ||
| data = { | ||
| "model": endpoints[0].model_names[0], | ||
| "prompt": request_json.get("prompt", ""), | ||
| } |
There was a problem hiding this comment.
Correctness Issue: Local Tokenizer Bypasses Chat Template / Messages
If the local tokenizer successfully loads (e.g., if the model is cached locally or the model name matches), the try block at line 391 will succeed because request_json.get("prompt", "") will return "" (since prompt is not in a chat-completions request).
This means self.tokenizer.encode("") will execute successfully without raising an exception, and the router will completely bypass this except Exception: block. As a result, it will perform a KV lookup on the empty prompt "" instead of the actual chat messages.
To fix this, we should explicitly prevent the local tokenizer from encoding an empty prompt when messages is present in the request. Since local chat template tokenization is not yet supported in this interim patch, we can raise an exception in the try block if messages is in request_json to force the remote /tokenize fallback.
For example, update the try block above (around line 391) to:
try:
if "messages" in request_json:
raise NotImplementedError("Local chat template tokenization not supported yet")
if self.tokenizer is None:
self.tokenizer = AutoTokenizer.from_pretrained(
endpoints[0].model_names[0]
)
token_ids = self.tokenizer.encode(request_json.get("prompt", ""))| if "messages" in request_json: | ||
| data = { | ||
| "model": endpoints[0].model_names[0], | ||
| "messages": request_json["messages"], | ||
| "add_generation_prompt": True, | ||
| } | ||
| else: | ||
| data = { | ||
| "model": endpoints[0].model_names[0], | ||
| "prompt": request_json.get("prompt", ""), | ||
| } |
There was a problem hiding this comment.
Correctness Issue: Local Tokenizer Bypasses Chat Template / Messages
Similar to the issue in KvawareRouter.route_request, if the local tokenizer successfully loads, the try block at line 668 will succeed because request_json.get("prompt", "") will return "" (since prompt is not in a chat-completions request).
This means self.tokenizer.encode("") will execute successfully without raising an exception, and the router will completely bypass this except Exception: block. As a result, it will perform a KV lookup on the empty prompt "" instead of the actual chat messages.
To fix this, we should explicitly prevent the local tokenizer from encoding an empty prompt when messages is present in the request. Since local chat template tokenization is not yet supported in this interim patch, we can raise an exception in the try block if messages is in request_json to force the remote /tokenize fallback.
For example, update the try block above (around line 668) to:
try:
if "messages" in request_json:
raise NotImplementedError("Local chat template tokenization not supported yet")
if self.tokenizer is None:
self.tokenizer = AutoTokenizer.from_pretrained(
endpoints[0].model_names[0]
)
return self.tokenizer.encode(request_json.get("prompt", ""))| body = requests.post( | ||
| remote_url, headers=headers, json=data, timeout=10 | ||
| ).json() |
There was a problem hiding this comment.
Performance Issue: Blocking HTTP Call on the Event Loop
Calling requests.post synchronously inside an async def function blocks the entire FastAPI event loop, which can severely degrade performance and increase latency for all concurrent requests under load.
We should run this blocking HTTP call in an executor, just like it is done in LoadAwareRouter.tokenize_prompt.
| body = requests.post( | |
| remote_url, headers=headers, json=data, timeout=10 | |
| ).json() | |
| loop = asyncio.get_running_loop() | |
| response = await loop.run_in_executor( | |
| None, | |
| lambda: requests.post( | |
| remote_url, headers=headers, json=data, timeout=10 | |
| ) | |
| ) | |
| body = response.json() |
…ompletions bodies (minimal interim patch)
MINIMAL carry-patch for deployments that always take the remote /tokenize path (served-model-name != hub id, so the local tokenizer never loads): the remote payload maps to vLLM's TokenizeChatRequest when the body has messages, instead of tokenizing prompt='' for every chat request.
This is the interim, easy-to-rebase subset of PR #1045 for downstream image builds while the full PR (local chat-template tokenization, executors, failure caching, tests) is in upstream review. Superseded by
#1045 when it lands.
FILL IN THE PR DESCRIPTION HERE
FIX #xxxx (link existing issues this PR will resolve)
BEFORE SUBMITTING, PLEASE READ THE CHECKLIST BELOW AND FILL IN THE DESCRIPTION ABOVE
-swhen doinggit commit[Bugfix],[Feat], and[CI].Detailed Checklist (Click to Expand)
Thank you for your contribution to production-stack! Before submitting the pull request, please ensure the PR meets the following criteria. This helps us maintain the code quality and improve the efficiency of the review process.
PR Title and Classification
Please try to classify PRs for easy understanding of the type of changes. The PR title is prefixed appropriately to indicate the type of change. Please use one of the following:
[Bugfix]for bug fixes.[CI/Build]for build or continuous integration improvements.[Doc]for documentation fixes and improvements.[Feat]for new features in the cluster (e.g., autoscaling, disaggregated prefill, etc.).[Router]for changes to thevllm_router(e.g., routing algorithm, router observability, etc.).[Misc]for PRs that do not fit the above categories. Please use this sparingly.Note: If the PR spans more than one category, please include all relevant prefixes.
Code Quality
The PR need to meet the following code quality standards:
pre-committo format your code. SeeREADME.mdfor installation.DCO and Signed-off-by
When contributing changes to this project, you must agree to the DCO. Commits must include a
Signed-off-by:header which certifies agreement with the terms of the DCO.Using
-swithgit commitwill automatically add this header.What to Expect for the Reviews
We aim to address all PRs in a timely manner. If no one reviews your PR within 5 days, please @-mention one of YuhanLiu11
, Shaoting-Feng or ApostaC.