-
Notifications
You must be signed in to change notification settings - Fork 487
[Router] kvaware/loadaware remote /tokenize: send messages for chat-c… #1050
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -398,10 +398,23 @@ async def route_request( | |||||||||||||||||||||||
| # Remote /tokenize fallback (let errors bubble up to keep behavior simple) | ||||||||||||||||||||||||
| remote_url = endpoints[0].url + "/tokenize" | ||||||||||||||||||||||||
| headers = {"Content-Type": "application/json"} | ||||||||||||||||||||||||
| data = { | ||||||||||||||||||||||||
| "model": endpoints[0].model_names[0], | ||||||||||||||||||||||||
| "prompt": request_json.get("prompt", ""), | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| # Chat-completions bodies carry `messages`, not `prompt` - send | ||||||||||||||||||||||||
| # them as vLLM's TokenizeChatRequest so the engine applies its | ||||||||||||||||||||||||
| # own chat template (`add_generation_prompt=True` is the | ||||||||||||||||||||||||
| # chat-completions default, pinned explicitly). Sending the old | ||||||||||||||||||||||||
| # prompt-form payload for a chat body tokenizes "" and the KV | ||||||||||||||||||||||||
| # lookup keys on garbage. | ||||||||||||||||||||||||
| 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", ""), | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| body = requests.post( | ||||||||||||||||||||||||
| remote_url, headers=headers, json=data, timeout=10 | ||||||||||||||||||||||||
| ).json() | ||||||||||||||||||||||||
|
Comment on lines
418
to
420
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Performance Issue: Blocking HTTP Call on the Event LoopCalling We should run this blocking HTTP call in an executor, just like it is done in
Suggested change
|
||||||||||||||||||||||||
|
|
@@ -661,10 +674,23 @@ async def tokenize_prompt( | |||||||||||||||||||||||
| except Exception: | ||||||||||||||||||||||||
| remote_url = endpoints[0].url + "/tokenize" | ||||||||||||||||||||||||
| headers = {"Content-Type": "application/json"} | ||||||||||||||||||||||||
| data = { | ||||||||||||||||||||||||
| "model": endpoints[0].model_names[0], | ||||||||||||||||||||||||
| "prompt": request_json.get("prompt", ""), | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| # Chat-completions bodies carry `messages`, not `prompt` - send | ||||||||||||||||||||||||
| # them as vLLM's TokenizeChatRequest so the engine applies its | ||||||||||||||||||||||||
| # own chat template (`add_generation_prompt=True` is the | ||||||||||||||||||||||||
| # chat-completions default, pinned explicitly). Sending the old | ||||||||||||||||||||||||
| # prompt-form payload for a chat body tokenizes "" and the KV | ||||||||||||||||||||||||
| # lookup keys on garbage. | ||||||||||||||||||||||||
| 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", ""), | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+683
to
+693
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correctness Issue: Local Tokenizer Bypasses Chat Template / MessagesSimilar to the issue in This means To fix this, we should explicitly prevent the local tokenizer from encoding an empty prompt when For example, update the 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", "")) |
||||||||||||||||||||||||
| loop = asyncio.get_running_loop() | ||||||||||||||||||||||||
| response = await loop.run_in_executor( | ||||||||||||||||||||||||
| None, | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
tryblock at line 391 will succeed becauserequest_json.get("prompt", "")will return""(sincepromptis not in a chat-completions request).This means
self.tokenizer.encode("")will execute successfully without raising an exception, and the router will completely bypass thisexcept 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
messagesis present in the request. Since local chat template tokenization is not yet supported in this interim patch, we can raise an exception in thetryblock ifmessagesis inrequest_jsonto force the remote/tokenizefallback.For example, update the
tryblock above (around line 391) to: