Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
245 changes: 245 additions & 0 deletions src/tests/test_kvaware_chat_tokenization.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,245 @@
"""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_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 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"]
86 changes: 73 additions & 13 deletions src/vllm_router/routers/routing_logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,71 @@ def _loadaware_beta(override: Optional[float]) -> float:
return DEFAULT_LOADAWARE_BETA


def _normalize_chat_messages(messages: List[Dict]) -> List[Dict]:
"""Text-only view of an OpenAI chat ``messages`` array for local
chat-template application.

Multimodal content parts (a list of ``{"type": ...}`` dicts) are
flattened to their text parts - mirroring ``PrefixAwareRouter`` - because
plain HF chat templates expect string content. ``None`` content (e.g.
assistant tool-call turns) becomes ``""``. Messages that already carry
string content pass through untouched, so template-relevant fields
(``role``, ``name``, ``tool_calls``, ...) are preserved. The input is
never mutated.
"""
normalized = []
for message in messages:
content = message.get("content")
if isinstance(content, list):
text_content = " ".join(
part.get("text", "")
for part in content
if isinstance(part, dict) and part.get("type") == "text"
)
message = {**message, "content": text_content}
Comment on lines +108 to +120

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If a multimodal message contains a part where "text" is explicitly set to null (e.g., {"type": "text", "text": null}), part.get("text", "") will return None instead of "". This will cause " ".join(...) to raise a TypeError: sequence item: expected str instance, NoneType found.

Using part.get("text") or "" safely defaults to an empty string if the value is None or missing.

Suggested change
if isinstance(content, list):
text_content = " ".join(
part.get("text", "")
for part in content
if isinstance(part, dict) and part.get("type") == "text"
)
message = {**message, "content": text_content}
if isinstance(content, list):
text_content = " ".join(
part.get("text") or ""
for part in content
if isinstance(part, dict) and part.get("type") == "text"
)
message = {**message, "content": text_content}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 2574697 — switched to part.get("text") or "" and added a regression test for an explicit {"type": "text", "text": null} part.

Comment on lines +108 to +120

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Joining empty or null text parts with " " can introduce unexpected leading, trailing, or double spaces (e.g., " hello" or "hello world"). In many tokenizers, leading or multiple spaces significantly alter the generated token IDs, which can lead to cache misses or routing mismatches.

Filtering out non-string, null, or empty text parts completely before joining ensures a clean, space-aligned text content.

Suggested change
if isinstance(content, list):
text_content = " ".join(
part.get("text") or ""
for part in content
if isinstance(part, dict) and part.get("type") == "text"
)
message = {**message, "content": text_content}
if isinstance(content, list):
text_content = " ".join(
part["text"]
for part in content
if isinstance(part, dict)
and part.get("type") == "text"
and isinstance(part.get("text"), str)
and part["text"]
)
message = {**message, "content": text_content}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied in 113973c, with one correction to the rationale: stray spaces here don't cause cache misses or routing mismatches — normalization is deterministic per message content, so the router's hashes stay self-consistent across requests, and for multimodal messages the router-side token ids can never exactly match the engine's anyway (image parts become vision tokens engine-side; this normalization is a best-effort prefix approximation for routing). Applying it regardless because filtering null/empty parts yields cleaner text, closer to what a text-only chat template would render, at zero cost.

elif content is None:
message = {**message, "content": ""}
normalized.append(message)
return normalized


def _extract_token_ids(tokenizer, request_json: Dict) -> List[int]:
"""Token ids as the serving engine would see them for this request body.

Chat-completion bodies (``messages``) are tokenized through the model's
chat template with ``add_generation_prompt=True``: vLLM engines cache KV
for the token ids *after* template application, so encoding the raw
message text would never line up with the engine-side prefix. Completion
bodies keep the plain ``encode`` path, unchanged. May raise (e.g. the
tokenizer defines no chat template) - callers fall back to the engine's
``/tokenize`` API.
"""
if "messages" in request_json:
return tokenizer.apply_chat_template(
_normalize_chat_messages(request_json["messages"]),
add_generation_prompt=True,
tokenize=True,
)
return tokenizer.encode(request_json.get("prompt", ""))


def _tokenize_request_payload(model: str, request_json: Dict) -> Dict:
"""Request body for the engine's ``/tokenize`` fallback.

Chat bodies map to vLLM's ``TokenizeChatRequest`` (the engine applies its
own chat template, multimodal content included), completion bodies to
``TokenizeCompletionRequest``. ``add_generation_prompt=True`` is vLLM's
chat-completions default; sent explicitly to pin the alignment.
"""
if "messages" in request_json:
return {
"model": model,
"messages": request_json["messages"],
"add_generation_prompt": True,
}
return {"model": model, "prompt": request_json.get("prompt", "")}
Comment on lines +193 to +207

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To avoid duplicating the tokenizer loading and double-checked locking logic between KvawareRouter.route_request and LoadAwareRouter.tokenize_prompt, we can extract it into a shared helper function _ensure_tokenizer at the module level.

def _tokenize_request_payload(model: str, request_json: Dict) -> Dict:
    """Request body for the engine's ``/tokenize`` fallback.

    Chat bodies map to vLLM's ``TokenizeChatRequest`` (the engine applies its
    own chat template, multimodal content included), completion bodies to
    ``TokenizeCompletionRequest``. ``add_generation_prompt=True`` is vLLM's
    chat-completions default; sent explicitly to pin the alignment.
    """
    if "messages" in request_json:
        return {
            "model": model,
            "messages": request_json["messages"],
            "add_generation_prompt": True,
        }
    return {"model": model, "prompt": request_json.get("prompt", "")}


async def _ensure_tokenizer(router, model_name: str) -> None:
    """Ensure the tokenizer is loaded on the router instance, using a double-checked lock."""
    if router.tokenizer is None:
        if not hasattr(router, "_tokenizer_lock"):
            router._tokenizer_lock = asyncio.Lock()
        async with router._tokenizer_lock:
            if router.tokenizer is None:
                loop = asyncio.get_running_loop()
                router.tokenizer = await loop.run_in_executor(
                    None,
                    lambda: AutoTokenizer.from_pretrained(model_name),
                )

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied in a7fd571, with one signature change: the helper takes endpoints and resolves model_names[0] lazily inside the load branch. The suggested call form — _ensure_tokenizer(self, endpoints[0].model_names[0]) — evaluates model_names eagerly even when the tokenizer is already loaded, which broke an existing loadaware test whose endpoint stub doesn't answer model_names (the pre-refactor code only touched it when actually loading).

Comment on lines +193 to +207

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Extract the duplicated local-first tokenization and remote /tokenize fallback logic into a shared module-level helper function _tokenize_prompt. This avoids significant code duplication between KvawareRouter.route_request and LoadAwareRouter.tokenize_prompt.

def _tokenize_request_payload(model: str, request_json: Dict) -> Dict:
    """Request body for the engine's ``/tokenize`` fallback.

    Chat bodies map to vLLM's ``TokenizeChatRequest`` (the engine applies its
    own chat template, multimodal content included), completion bodies to
    ``TokenizeCompletionRequest``. ``add_generation_prompt=True`` is vLLM's
    chat-completions default; sent explicitly to pin the alignment.
    """
    if "messages" in request_json:
        return {
            "model": model,
            "messages": request_json["messages"],
            "add_generation_prompt": True,
        }
    return {"model": model, "prompt": request_json.get("prompt", "")}


async def _tokenize_prompt(
    router,
    endpoints: List[EndpointInfo],
    request_json: Dict
) -> Optional[List[int]]:
    """Local-first tokenization with the remote `/tokenize` fallback."""
    try:
        await _ensure_tokenizer(router, endpoints)
        return _extract_token_ids(router.tokenizer, request_json)
    except Exception:
        try:
            remote_url = endpoints[0].url + "/tokenize"
            headers = {"Content-Type": "application/json"}
            data = _tokenize_request_payload(
                endpoints[0].model_names[0], request_json
            )
            loop = asyncio.get_running_loop()
            response = await loop.run_in_executor(
                None,
                lambda: requests.post(
                    remote_url, headers=headers, json=data, timeout=10
                ),
            )
            response.raise_for_status()
            return response.json()["tokens"]
        except Exception as e:
            logger.warning(
                f"Tokenization failed locally and via remote /tokenize "
                f"({e}); falling back to session/QPS routing"
            )
            return None

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reasonable refactor — deliberately deferring it to keep this diff reviewable at its current size after six review rounds. The semantics of the two sites differ slightly (KvawareRouter degrades to session/QPS on total failure inside route_request; LoadAwareRouter returns None for its caller to handle), so the shared helper deserves its own focused change. Happy to follow up post-merge if maintainers agree.



class RoutingInterface(metaclass=SingletonABCMeta):
def _qps_routing(
self, endpoints: List[EndpointInfo], request_stats: Dict[str, RequestStats]
Expand Down Expand Up @@ -387,21 +452,17 @@ async def route_request(
"""
token_ids = None
# Local-first tokenization, fall back to remote "/tokenize" API on failure
# TODO (Yuhan): Handle chat completions
try:
Comment on lines 505 to 507

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To prevent a potential IndexError when endpoints is empty (e.g., during a temporary service discovery lag or when all backends are unhealthy), we should check if endpoints is empty at the beginning of route_request, similar to the check in LoadAwareRouter.route_request.

Suggested change
token_ids = None
# Local-first tokenization, fall back to remote "/tokenize" API on failure
# TODO (Yuhan): Handle chat completions
try:
if not endpoints:
raise HTTPException(
status_code=503, detail="No backend endpoints available"
)
token_ids = None
# Local-first tokenization, fall back to remote "/tokenize" API on failure
try:

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid catch, but pre-existing behavior rather than something this PR introduces — the unpatched KvawareRouter.route_request performs the same unguarded endpoints[0] access, so the IndexError exists on main today. Keeping this PR scoped to the chat-tokenization fix; happy to bring the empty-endpoints guard (mirroring LoadAwareRouter's 503) as a small follow-up PR if maintainers want it.

if self.tokenizer is None:
self.tokenizer = AutoTokenizer.from_pretrained(
endpoints[0].model_names[0]
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Loading the tokenizer using AutoTokenizer.from_pretrained is a synchronous, blocking I/O operation (which may also involve network requests if the model is not cached locally). Calling it directly in an async def function blocks the event loop, preventing the router from processing other concurrent requests. It should be run in an executor to keep the event loop responsive.

Suggested change
if self.tokenizer is None:
self.tokenizer = AutoTokenizer.from_pretrained(
endpoints[0].model_names[0]
)
if self.tokenizer is None:
loop = asyncio.get_running_loop()
self.tokenizer = await loop.run_in_executor(
None,
lambda: AutoTokenizer.from_pretrained(endpoints[0].model_names[0])
)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 00fcf4d — the tokenizer load now runs via loop.run_in_executor in both routers.

token_ids = self.tokenizer.encode(request_json.get("prompt", ""))
token_ids = _extract_token_ids(self.tokenizer, request_json)
except Exception:
# 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", ""),
}
data = _tokenize_request_payload(endpoints[0].model_names[0], request_json)
body = requests.post(
remote_url, headers=headers, json=data, timeout=10
).json()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The synchronous requests.post call inside the async def route_request method blocks the event loop during the remote /tokenize fallback. This can severely degrade router throughput and latency under load.

To prevent blocking the event loop, run the synchronous HTTP request in an executor using asyncio.get_running_loop().run_in_executor, similar to how it is done in LoadAwareRouter.tokenize_prompt.

Suggested change
data = _tokenize_request_payload(endpoints[0].model_names[0], request_json)
body = requests.post(
remote_url, headers=headers, json=data, timeout=10
).json()
data = _tokenize_request_payload(endpoints[0].model_names[0], request_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()

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 2574697 — the /tokenize fallback now runs via loop.run_in_executor, matching the pattern in LoadAwareRouter.tokenize_prompt.

Expand Down Expand Up @@ -649,22 +710,21 @@ async def tokenize_prompt(
) -> List[int]:
"""Local-first tokenization with the remote `/tokenize` fallback.

The remote fallback is a blocking HTTP call, so it runs in an
executor rather than on the event loop.
Chat-completion bodies go through the chat template so the token ids
match what the engine caches KV for (see `_extract_token_ids`). The
remote fallback is a blocking HTTP call, so it runs in an executor
rather than on the event loop.
"""
try:
if self.tokenizer is None:
self.tokenizer = AutoTokenizer.from_pretrained(
endpoints[0].model_names[0]
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Loading the tokenizer using AutoTokenizer.from_pretrained is a synchronous, blocking I/O operation. Calling it directly in an async def function blocks the event loop, preventing the router from processing other concurrent requests. It should be run in an executor to keep the event loop responsive.

Suggested change
if self.tokenizer is None:
self.tokenizer = AutoTokenizer.from_pretrained(
endpoints[0].model_names[0]
)
if self.tokenizer is None:
loop = asyncio.get_running_loop()
self.tokenizer = await loop.run_in_executor(
None,
lambda: AutoTokenizer.from_pretrained(endpoints[0].model_names[0])
)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 00fcf4d — the tokenizer load now runs via loop.run_in_executor in both routers.

return self.tokenizer.encode(request_json.get("prompt", ""))
return _extract_token_ids(self.tokenizer, request_json)
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", ""),
}
data = _tokenize_request_payload(endpoints[0].model_names[0], request_json)
loop = asyncio.get_running_loop()
response = await loop.run_in_executor(
None,
Expand Down