-
Notifications
You must be signed in to change notification settings - Fork 485
[Router][Bugfix] KV-aware routing: tokenize chat-completions bodies through the chat template #1045
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
Open
tyler2cr
wants to merge
9
commits into
vllm-project:main
Choose a base branch
from
tyler2cr:router-kvaware-chat-completions
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+575
−48
Open
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
3f498ff
[Router][Bugfix] KV-aware routing: tokenize chat-completions bodies t…
tcr-enfuseio 2574697
[Router][Bugfix] Address review: non-blocking /tokenize fallback, tol…
tcr-enfuseio ec2becc
[Router][Bugfix] Tokenization failure degrades to session/QPS routing…
tcr-enfuseio 00fcf4d
[Router][Bugfix] Load the tokenizer in an executor - from_pretrained …
tcr-enfuseio 113973c
[Router] Review round 4: single-flight tokenizer init, drop empty tex…
tcr-enfuseio a7fd571
[Router] Extract shared _ensure_tokenizer helper (review round 5)
tcr-enfuseio d34db2c
[Router][Bugfix] Negative-cache failed tokenizer loads; fix helper bo…
tcr-enfuseio f6e243f
[Router][Bugfix] Render the chat template to text and encode it - tok…
tcr-enfuseio c0c4a42
[Router] Test: the chat path performs exactly one encode of the rende…
tcr-enfuseio File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,310 @@ | ||
| """Unit tests for chat-completion tokenization in the KV-aware routers. | ||
|
|
||
| `kvaware` and `loadaware` place a request by asking the LMCache controller | ||
| which engine already holds KV for the request's token-id prefix. vLLM | ||
| engines cache KV for the token ids *after* chat-template application, so a | ||
| chat-completions body (a "messages" array, no "prompt" key) must be | ||
| tokenized through `apply_chat_template` - the old | ||
| `encode(request_json.get("prompt", ""))` tokenized the empty string, the | ||
| lookup matched nothing, and every chat request silently degraded to the | ||
| session/QPS fallback. | ||
|
|
||
| As in `test_loadaware_router.py`, the routers are built with `__new__` and | ||
| only the attributes the tokenize path reads, so no LMCache controller (and | ||
| no network) is needed. | ||
| """ | ||
|
|
||
| from typing import Any, Dict | ||
|
|
||
| import pytest | ||
| from uhashring import HashRing | ||
|
|
||
| import vllm_router.routers.routing_logic as routing_logic | ||
| from vllm_router.routers.routing_logic import ( | ||
| KvawareRouter, | ||
| LoadAwareRouter, | ||
| _extract_token_ids, | ||
| _normalize_chat_messages, | ||
| _tokenize_request_payload, | ||
| ) | ||
|
|
||
|
|
||
| @pytest.fixture(autouse=True) | ||
| def lookup_msg_stub(monkeypatch): | ||
| """`LookupMsg`/`QueryInstMsg` come from the optional lmcache dependency; | ||
| stub them when absent so the routing tests run without the lmcache | ||
| extra.""" | ||
|
|
||
| class _Msg: | ||
| def __init__(self, **kwargs): | ||
| self.__dict__.update(kwargs) | ||
|
|
||
| for name in ("LookupMsg", "QueryInstMsg"): | ||
| if not hasattr(routing_logic, name): | ||
| monkeypatch.setattr(routing_logic, name, _Msg, raising=False) | ||
|
|
||
|
|
||
| URL_A = "http://10.0.0.1:8000" | ||
| URL_B = "http://10.0.0.2:8000" | ||
| INST_A = "instance-a" | ||
| LOCAL = "LocalCPUBackend" | ||
| MODEL = "test-model" | ||
| CHAT_IDS = [101, 102, 103, 104, 105] | ||
| MESSAGES = [ | ||
| {"role": "system", "content": "You are terse."}, | ||
| {"role": "user", "content": "Hello there"}, | ||
| ] | ||
|
|
||
|
|
||
| class EndpointInfo: | ||
| def __init__(self, url: str): | ||
| self.url = url | ||
| self.model_names = [MODEL] | ||
|
|
||
|
|
||
| class LookupRet: | ||
| def __init__(self, layout_info: Dict[str, Any]): | ||
| self.layout_info = layout_info | ||
|
|
||
|
|
||
| def endpoints(*urls): | ||
| return [EndpointInfo(url=url) for url in urls] | ||
|
|
||
|
|
||
| class ChatTokenizer: | ||
| """Records calls; template ids are disjoint from encode ids so a test | ||
| can tell which path produced them.""" | ||
|
|
||
| def __init__(self): | ||
| self.chat_template_calls = [] | ||
| self.encode_calls = [] | ||
|
|
||
| def apply_chat_template( | ||
| self, messages, add_generation_prompt=False, tokenize=False | ||
| ): | ||
| self.chat_template_calls.append( | ||
| { | ||
| "messages": messages, | ||
| "add_generation_prompt": add_generation_prompt, | ||
| "tokenize": tokenize, | ||
| } | ||
| ) | ||
| return list(CHAT_IDS) | ||
|
|
||
| def encode(self, prompt): | ||
| self.encode_calls.append(prompt) | ||
| return [1] * len(prompt) | ||
|
|
||
|
|
||
| class TemplatelessTokenizer(ChatTokenizer): | ||
| """A tokenizer with no chat template, as `apply_chat_template` raises on | ||
| base models.""" | ||
|
|
||
| def apply_chat_template(self, *args, **kwargs): | ||
| raise ValueError("no chat template defined") | ||
|
|
||
|
|
||
| # --- local tokenization ------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_messages_tokenize_through_the_chat_template(): | ||
| tokenizer = ChatTokenizer() | ||
| ids = _extract_token_ids(tokenizer, {"messages": MESSAGES}) | ||
| assert ids == CHAT_IDS | ||
| call = tokenizer.chat_template_calls[0] | ||
| assert call["add_generation_prompt"] is True | ||
| assert call["tokenize"] is True | ||
| assert tokenizer.encode_calls == [] | ||
|
|
||
|
|
||
| def test_prompt_requests_keep_the_plain_encode_path(): | ||
| tokenizer = ChatTokenizer() | ||
| ids = _extract_token_ids(tokenizer, {"prompt": "hello"}) | ||
| assert ids == [1] * len("hello") | ||
| assert tokenizer.chat_template_calls == [] | ||
|
|
||
|
|
||
| def test_multimodal_content_parts_are_flattened_to_their_text(): | ||
| messages = [ | ||
| { | ||
| "role": "user", | ||
| "content": [ | ||
| {"type": "text", "text": "describe"}, | ||
| {"type": "image_url", "image_url": {"url": "data:image/png;..."}}, | ||
| {"type": "text", "text": "this image"}, | ||
| ], | ||
| } | ||
| ] | ||
| normalized = _normalize_chat_messages(messages) | ||
| assert normalized == [{"role": "user", "content": "describe this image"}] | ||
| # The request body itself is never mutated. | ||
| assert isinstance(messages[0]["content"], list) | ||
|
|
||
|
|
||
| def test_null_text_part_becomes_an_empty_string(): | ||
| # {"type": "text", "text": null} is valid JSON a client can send; .get | ||
| # with a default only covers a MISSING key, so an explicit null must not | ||
| # reach " ".join as None. | ||
| messages = [ | ||
| { | ||
| "role": "user", | ||
| "content": [ | ||
| {"type": "text", "text": None}, | ||
| {"type": "text", "text": "hello"}, | ||
| ], | ||
| } | ||
| ] | ||
| normalized = _normalize_chat_messages(messages) | ||
| assert normalized == [{"role": "user", "content": " hello"}] | ||
|
|
||
|
|
||
| def test_none_content_becomes_an_empty_string(): | ||
| messages = [{"role": "assistant", "content": None, "tool_calls": [{"id": "1"}]}] | ||
| normalized = _normalize_chat_messages(messages) | ||
| assert normalized[0]["content"] == "" | ||
| assert normalized[0]["tool_calls"] == [{"id": "1"}] | ||
|
|
||
|
|
||
| def test_string_content_messages_pass_through_untouched(): | ||
| assert _normalize_chat_messages(MESSAGES) == MESSAGES | ||
|
|
||
|
|
||
| # --- the remote /tokenize fallback payload ------------------------------------ | ||
|
|
||
|
|
||
| def test_chat_bodies_use_the_tokenize_chat_request_form(): | ||
| payload = _tokenize_request_payload(MODEL, {"messages": MESSAGES}) | ||
| assert payload == { | ||
| "model": MODEL, | ||
| "messages": MESSAGES, | ||
| "add_generation_prompt": True, | ||
| } | ||
|
|
||
|
|
||
| def test_prompt_bodies_keep_the_completion_form(): | ||
| assert _tokenize_request_payload(MODEL, {"prompt": "hello"}) == { | ||
| "model": MODEL, | ||
| "prompt": "hello", | ||
| } | ||
|
|
||
|
|
||
| # --- through the routers ------------------------------------------------------ | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_kvaware_routes_chat_requests_via_the_kv_lookup_path(): | ||
| """The regression this file exists for: a messages-form body must reach | ||
| the controller as non-empty, template-aligned token ids and route to the | ||
| KV holder - not tokenize as "" and fall back to session/QPS.""" | ||
| router = KvawareRouter.__new__(KvawareRouter) | ||
| router.tokenizer = ChatTokenizer() | ||
| router.threshold = 2000 | ||
| router.instance_id_to_ip = {INST_A: URL_A} | ||
| router.session_key = None | ||
| router.hash_ring = HashRing() | ||
| seen = {} | ||
|
|
||
| async def query_manager(msg): | ||
| seen["tokens"] = msg.tokens | ||
| return LookupRet({INST_A: (LOCAL, len(CHAT_IDS))}) | ||
|
|
||
| router.query_manager = query_manager | ||
| url = await router.route_request( | ||
| endpoints(URL_A, URL_B), {}, {}, None, {"messages": MESSAGES} | ||
| ) | ||
| assert seen["tokens"] == CHAT_IDS | ||
| assert url == URL_A | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_loadaware_tokenizes_chat_requests_through_the_template(): | ||
| router = LoadAwareRouter.__new__(LoadAwareRouter) | ||
| router.tokenizer = ChatTokenizer() | ||
| ids = await router.tokenize_prompt(endpoints(URL_A), {"messages": MESSAGES}) | ||
| assert ids == CHAT_IDS | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_prompt_requests_are_unchanged_by_the_chat_support(): | ||
| router = LoadAwareRouter.__new__(LoadAwareRouter) | ||
| tokenizer = ChatTokenizer() | ||
| router.tokenizer = tokenizer | ||
| ids = await router.tokenize_prompt(endpoints(URL_A), {"prompt": "hello"}) | ||
| assert ids == [1] * len("hello") | ||
| assert tokenizer.chat_template_calls == [] | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_remote_tokenize_fallback_sends_the_messages_for_chat(monkeypatch): | ||
| """A tokenizer without a chat template falls back to the engine's | ||
| /tokenize with the original messages (vLLM's TokenizeChatRequest), not | ||
| {"prompt": ""}.""" | ||
| router = LoadAwareRouter.__new__(LoadAwareRouter) | ||
| router.tokenizer = TemplatelessTokenizer() | ||
| captured = {} | ||
|
|
||
| class Response: | ||
| @staticmethod | ||
| def raise_for_status(): | ||
| pass | ||
|
|
||
| @staticmethod | ||
| def json(): | ||
| return {"count": len(CHAT_IDS), "tokens": CHAT_IDS} | ||
|
|
||
| def fake_post(url, headers=None, json=None, timeout=None): | ||
| captured["url"] = url | ||
| captured["json"] = json | ||
| return Response() | ||
|
|
||
| monkeypatch.setattr(routing_logic.requests, "post", fake_post) | ||
| ids = await router.tokenize_prompt(endpoints(URL_A), {"messages": MESSAGES}) | ||
| assert ids == CHAT_IDS | ||
| assert captured["url"] == URL_A + "/tokenize" | ||
| assert captured["json"]["messages"] == MESSAGES | ||
| assert captured["json"]["add_generation_prompt"] is True | ||
| assert "prompt" not in captured["json"] | ||
|
|
||
|
|
||
| # --- tokenization failure degrades to fallback routing, never a 500 ----------- | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_kvaware_falls_back_to_qps_when_tokenization_fails(monkeypatch): | ||
| """A dead /tokenize endpoint (plus no usable local template) must not | ||
| fail the request - the router still has session/QPS routing.""" | ||
| router = KvawareRouter.__new__(KvawareRouter) | ||
| router.tokenizer = TemplatelessTokenizer() | ||
| router.threshold = 2000 | ||
| router.instance_id_to_ip = {} | ||
| router.session_key = None | ||
| router.hash_ring = HashRing() | ||
| lookups = [] | ||
|
|
||
| async def query_manager(msg): | ||
| lookups.append(msg) | ||
|
|
||
| router.query_manager = query_manager | ||
|
|
||
| def dead_post(url, headers=None, json=None, timeout=None): | ||
| raise ConnectionError("engine unreachable") | ||
|
|
||
| monkeypatch.setattr(routing_logic.requests, "post", dead_post) | ||
| url = await router.route_request( | ||
| endpoints(URL_A, URL_B), {}, {}, None, {"messages": MESSAGES} | ||
| ) | ||
| assert url == URL_A # QPS routing with no stats picks the first endpoint | ||
| assert lookups == [] # no KV lookup without token ids | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_loadaware_tokenize_returns_none_when_both_paths_fail(monkeypatch): | ||
| router = LoadAwareRouter.__new__(LoadAwareRouter) | ||
| router.tokenizer = TemplatelessTokenizer() | ||
|
|
||
| def dead_post(url, headers=None, json=None, timeout=None): | ||
| raise ConnectionError("engine unreachable") | ||
|
|
||
| monkeypatch.setattr(routing_logic.requests, "post", dead_post) | ||
| ids = await router.tokenize_prompt(endpoints(URL_A), {"messages": MESSAGES}) | ||
| assert ids is None | ||
Oops, something went wrong.
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.
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.
Updating the test assertion to expect
"hello"instead of" hello", aligning with the robust normalization logic that filters out null or empty text parts to prevent tokenization discrepancies.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.
Applied in 113973c together with the normalization change — the test now also covers an explicit empty-string part and asserts "hello" with no stray space.