[Router][Bugfix] Keep the KV-aware /tokenize fallback off the event loop - #1101
David-Wu1119 wants to merge 2 commits into
Conversation
KvawareRouter.route_request fell back to the engine's /tokenize endpoint with a synchronous requests.post inside the coroutine, so whenever the local tokenizer was unavailable every in-flight request on the router stalled for up to the 10 s timeout. LoadAwareRouter already had an async tokenize_prompt() that runs the call in an executor; move it to KvawareRouter so both routers use it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: David-Wu1119 <133224895+David-Wu1119@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request refactors the KV-cache-aware routing logic by moving the tokenize_prompt method and calling it within route_request to handle local-first tokenization with an asynchronous remote fallback. It also adds a unit test to verify that the remote fallback does not block the event loop. Feedback is provided regarding a potential IndexError if endpoints is empty, suggesting a defensive check to raise a 503 HTTPException instead.
| # TODO (Yuhan): Handle chat completions | ||
| try: | ||
| tokenizer = self._get_tokenizer(endpoints) | ||
| token_ids = tokenizer.encode(request_json.get("prompt", "")) | ||
| 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", ""), | ||
| } | ||
| body = requests.post( | ||
| remote_url, headers=headers, json=data, timeout=10 | ||
| ).json() | ||
| token_ids = body["tokens"] | ||
| token_ids = await self.tokenize_prompt(endpoints, request_json) |
There was a problem hiding this comment.
If endpoints is empty (e.g., when no backends are currently discovered or healthy), calling self.tokenize_prompt(endpoints, request_json) will raise an unhandled IndexError when attempting to access endpoints[0]. To prevent a 500 Internal Server Error, we should defensively check if endpoints is empty and raise a 503 HTTPException, consistent with how LoadAwareRouter handles this scenario.
| # TODO (Yuhan): Handle chat completions | |
| try: | |
| tokenizer = self._get_tokenizer(endpoints) | |
| token_ids = tokenizer.encode(request_json.get("prompt", "")) | |
| 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", ""), | |
| } | |
| body = requests.post( | |
| remote_url, headers=headers, json=data, timeout=10 | |
| ).json() | |
| token_ids = body["tokens"] | |
| token_ids = await self.tokenize_prompt(endpoints, request_json) | |
| if not endpoints: | |
| raise HTTPException( | |
| status_code=503, detail="No backend endpoints available" | |
| ) | |
| # TODO (Yuhan): Handle chat completions | |
| token_ids = await self.tokenize_prompt(endpoints, request_json) |
…scovered Without endpoints, the remote /tokenize fallback indexed endpoints[0] and the request failed with an IndexError. Raise the same 503 the other routers use, and add a test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: David-Wu1119 <133224895+David-Wu1119@users.noreply.github.com>
|
@ruizhang0101 thanks for the review. Fixed the gemini issue in 53d1045: with no discovered endpoints, the KV-aware router now raises the same 503 ("No backend endpoints available") the other routers use, instead of an IndexError from |
KvawareRouter.route_requesttokenizes the prompt locally and falls back to the engine's/tokenizeendpoint when the local tokenizer can't be loaded. That fallback was a synchronousrequests.post(..., timeout=10)called directly inside the coroutine. So whenever the fallback is taken, the router's event loop is blocked for the whole HTTP round trip, up to the 10 s timeout, and every other in-flight request on the router stalls with it.LoadAwareRouter, which subclassesKvawareRouter, already solved this in #1035. It has an asynctokenize_prompt()that does the same local-first tokenization but runs the remote call in an executor. This PR movestokenize_prompt()up toKvawareRouterand hasKvawareRouter.route_requestuse it. Both routers now share one implementation, andLoadAwareRouter's behavior is unchanged because it inherits the method.Test:
test_kvaware_remote_tokenize_fallback_does_not_block_event_loopmakes local tokenizer loading fail and replacesrequests.postwith a 0.3 s blocking call. It then runsroute_requestnext to a coroutine that ticks every 10 ms. Onmainthe ticker never runs during the fallback (ticks == 0); with this change it keeps ticking, and routing still uses the tokens returned by/tokenize.src/tests/test_kvaware_router.py+src/tests/test_loadaware_router.py: 38 passed.-swhen doinggit commit[Bugfix],[Feat], and[CI].Found with ruff's
ASYNC210(blocking HTTP call in an async function); written with AI assistance.🤖 Generated with Claude Code