Repository navigation
[Fix] tokenizer: Isolate and offload regular fast tokenization - #30771
anencore94 wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request improves event loop responsiveness by offloading potentially blocking operations, such as request conversion and tokenization, to separate threads using asyncio.to_thread. It also introduces unit tests to verify that these operations do not block the event loop. The review feedback highlights a critical thread-safety concern: slow tokenizers implemented in pure Python are not thread-safe and running them concurrently in separate threads can lead to race conditions or crashes. It is recommended to serialize access to slow tokenizers using a lock.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def tokenize_sync(): | ||
| if not is_cross_encoder and ( | ||
| not getattr(self.tokenizer, "is_fast", False) | ||
| ): | ||
| return [self.tokenizer.encode(t) for t in tokenizer_input], None | ||
|
|
||
| encoded = self.tokenizer(tokenizer_input, **tokenizer_kwargs) | ||
| input_ids = encoded["input_ids"] | ||
| token_type_ids = ( | ||
| encoded.get("token_type_ids") if is_cross_encoder else None | ||
| return ( | ||
| encoded["input_ids"], | ||
| encoded.get("token_type_ids") if is_cross_encoder else None, | ||
| ) | ||
|
|
||
| input_ids, token_type_ids = await asyncio.to_thread(tokenize_sync) |
There was a problem hiding this comment.
Offloading tokenization to a separate thread via asyncio.to_thread is a great improvement for event loop responsiveness. However, if the tokenizer is a slow tokenizer (i.e., not getattr(self.tokenizer, "is_fast", False)), it is implemented in pure Python and is generally not thread-safe. Running it concurrently in multiple threads can lead to race conditions, corrupted tokenization outputs, or crashes.
Furthermore, since pure Python code does not release the GIL, running multiple slow tokenizations concurrently in separate threads does not provide any CPU parallelization benefits anyway.
To ensure thread safety for slow tokenizers while keeping the asyncio event loop responsive, we should serialize calls to slow tokenizers using a lock (e.g., a threading.Lock initialized on the manager).
if not hasattr(self, "_tokenizer_lock"):
self._tokenizer_lock = threading.Lock()
def tokenize_sync():
is_fast = getattr(self.tokenizer, "is_fast", False)
if not is_cross_encoder and not is_fast:
with self._tokenizer_lock:
return [self.tokenizer.encode(t) for t in tokenizer_input], None
if not is_fast:
with self._tokenizer_lock:
encoded = self.tokenizer(tokenizer_input, **tokenizer_kwargs)
else:
encoded = self.tokenizer(tokenizer_input, **tokenizer_kwargs)
return (
encoded["input_ids"],
encoded.get("token_type_ids") if is_cross_encoder else None,
)
input_ids, token_type_ids = await asyncio.to_thread(tokenize_sync)There was a problem hiding this comment.
Good catch. Fixed in edde3b7106adbc6080ffc94f391310f093039759. The follow-up replaces the default executor with a TokenizerManager-owned ThreadPoolExecutor(max_workers=1), shared by OpenAI request conversion and regular tokenization. This keeps preprocessing off the asyncio event loop while preserving the previous per-process serialization for slow tokenizers and shared template state. The helper also copies contextvars into the worker thread. I added regression coverage for serialization and context propagation; the related suite now passes 141 tests.
|
@merrymercy @slin1237 This PR is ready for review. Could an authorized maintainer add the Local validation: 142 related tests passed. The two focused responsiveness tests and the ASGI integration test fail with the expected starvation assertions on unmodified |
|
@JustinTong0323 @merrymercy Gentle ping when you have a chance. This PR is ready for review and touches the OpenAI request-preprocessing path and TokenizerManager ownership boundary. The related unit and ASGI regression tests pass locally; I would especially appreciate feedback on the offloading and serialization approach. Thank you! |
eb3e9a0 to
beebcba
Compare
|
Rebased this PR on the latest This is the one PR I would like to prioritize for review. Impact:
Local validation after rebase:
The ASGI liveness test was not rerun successfully in my local macOS environment because collection now requires optional local dependencies such as Could an authorized maintainer trigger CI with |
|
Hi @merrymercy @hnyls2002 — gentle ping on this one. Routing to you two as the most active recent committers on Quick summary for triage:
Happy to adjust scope or approach if you'd prefer a different design. Thanks! |
) Cherry-pick of sgl-project#30771 (squashed). Request preprocessing (tokenization, template rendering) ran inline on the asyncio event loop, starving the HTTP server -- health checks and other in-flight requests stalled behind a large tokenize. Move the blocking work off the loop via asyncio.to_thread and serialize it so concurrent requests don't oversubscribe the tokenizer. Co-Authored-By: Jaeyeon Kim <anencore94@gmail.com>
Co-authored-by: Yinghai Lu <yinghai@meta.com> (cherry picked from commit 7f6b4cb) Conflict resolution: - server_args.py: took upstream's mm_feature_transport declaration, which widens the Literal from {cpu, cuda_ipc} to include cuda_vmm and rewrites the help text. Strict superset of this base's version. - tokenizer_manager.py: pure adjacency conflict. This base carries init_request_preprocessor/run_in_request_preprocessor from sgl-project#30771 at the same insertion point as upstream's new _validate_cuda_vmm_feature_transport_support. The two are unrelated, so both were kept.
Cherry-pick of sgl-project#30771. Run OpenAIServingBase._convert_to_internal_request() and the regular TokenizerManager._tokenize_texts() fallback in a TokenizerManager-owned single-thread executor, so long or concurrent preprocessing no longer blocks the API server's asyncio loop (false /ping and /health failures). Request contextvars are propagated into the executor thread, and the dynamic-batch tokenizer path is unchanged. Co-Authored-By: Zhangmj <2270170683@qq.com>
9aa2980 to
08af556
Compare
|
Rebased onto the latest @ch-wan @hnyls2002 — bumping this one. Request preprocessing on the tokenizer-manager path runs inline on the event loop, so a burst of large requests starves it: On the red checks: all |
|
We are seeing the same false-health-check issue in production with DeepSeek-V4-Flash on SGLang v0.5.16: synchronous request preprocessing can starve We backported this PR onto our existing v0.5.16 image and ran the same 448-concurrent-client load test with ~64k-token chat inputs. Before the patch, the baseline produced 4 new readiness timeouts in the first minute. With this PR applied, the same load ran for 10+ minutes with zero readiness timeouts, zero liveness timeouts, and zero request errors. |
Cherry-pick of sgl-project#30771. Run OpenAIServingBase._convert_to_internal_request() and the regular TokenizerManager._tokenize_texts() fallback in a TokenizerManager-owned single-thread executor, so long or concurrent preprocessing no longer blocks the API server's asyncio loop (false /ping and /health failures). Request contextvars are propagated into the executor thread, and the dynamic-batch tokenizer path is unchanged. Co-Authored-By: Zhangmj <2270170683@qq.com>
|
@yongqinwang-cmd thank you — that's a much better validation than anything I could produce on my own, and the numbers line up exactly with what this patch is meant to fix. Independent reproduction on a different model and a different deployment, with a measured before/after (4 readiness timeouts in the first minute → zero over 10+ minutes at 448 concurrent clients with ~64k-token inputs), is the strongest evidence this PR has. Worth stating plainly why the failure mode is worse than it looks: the health endpoint going quiet does not mean the engine is unhealthy. Inference keeps making progress the whole time — it is only the event loop that cannot get a turn to answer the probe. So an orchestrator with a liveness probe restarts a server that was working, drops every in-flight request on it, and the restart lands the load on the remaining replicas, which then starve the same way. A transient burst turns into a rolling outage, and nothing in the logs points at preprocessing. @ch-wan @hnyls2002 @merrymercy — given a second party is now carrying this as a local patch in production, could we get this looked at? It is confined to the tokenizer-manager preprocessing path and ships three CPU-only regression tests. The blocker is still purely mechanical: the red checks are |
Stub `model_config` on the bare `TokenizerManager` the tokenization responsiveness test builds: `_tokenize_texts` now consults `model_config.is_embedding_gemma` for the EmbeddingGemma EOS fix-up, which landed after these tests were written. Move the ASGI liveness test under `test/registered/unit/entrypoints/` so it satisfies the registered-test taxonomy (`test/registered/<kind>/<subsystem>/`) introduced since this branch was opened. It is an in-process ASGI test on the same `base-a-test-cpu` suite as its siblings, so `unit` is the right kind. Signed-off-by: Jaeyeon Kim <anencore94@gmail.com>
08af556 to
60a56aa
Compare
|
Rebased onto current The bug still reproduces on today's
|
Cherry-pick of sgl-project#30771. Run OpenAIServingBase._convert_to_internal_request() and the regular TokenizerManager._tokenize_texts() fallback in a TokenizerManager-owned single-thread executor, so long or concurrent preprocessing no longer blocks the API server's asyncio loop (false /ping and /health failures). Request contextvars are propagated into the executor thread, and the dynamic-batch tokenizer path is unchanged. Co-Authored-By: Zhangmj <2270170683@qq.com>
Cherry-pick of sgl-project#30771. Run OpenAIServingBase._convert_to_internal_request() and the regular TokenizerManager._tokenize_texts() fallback in a TokenizerManager-owned single-thread executor, so long or concurrent preprocessing no longer blocks the API server's asyncio loop (false /ping and /health failures). Request contextvars are propagated into the executor thread, and the dynamic-batch tokenizer path is unchanged. Co-Authored-By: Zhangmj <2270170683@qq.com>
Cherry-pick of sgl-project#30771. Run OpenAIServingBase._convert_to_internal_request() and the regular TokenizerManager._tokenize_texts() fallback in a TokenizerManager-owned single-thread executor, so long or concurrent preprocessing no longer blocks the API server's asyncio loop (false /ping and /health failures). Request contextvars are propagated into the executor thread, and the dynamic-batch tokenizer path is unchanged. Co-Authored-By: Zhangmj <2270170683@qq.com>
Signed-off-by: Jaeyeon Kim <anencore94@gmail.com>
This comment has been minimized.
This comment has been minimized.
|
@hnyls2002 could you review the frontend preprocessing offload and authorize CI for 96a3df4? The current gate stops because This complements #30060: it removes synchronous HTTP/tokenizer preprocessing from the event loop, while that PR addresses scheduler health-signal IPC. The updated title check now passes. If another maintainer owns this path, a reviewer referral would help. |
Cherry-pick of sgl-project#30771. Run OpenAIServingBase._convert_to_internal_request() and the regular TokenizerManager._tokenize_texts() fallback in a TokenizerManager-owned single-thread executor, so long or concurrent preprocessing no longer blocks the API server's asyncio loop (false /ping and /health failures). Request contextvars are propagated into the executor thread, and the dynamic-batch tokenizer path is unchanged. Co-Authored-By: Zhangmj <2270170683@qq.com>
|
@hnyls2002 following up on |
|
@chromecast56 @JianWang97 Thanks for building on #30771 in #39716 and #44502 and preserving the attribution. I want to consolidate this work into one merge path; I am happy for either #30771 or #39716 to carry it, with the original implementation/tests and subsequent contributions credited. I updated #30771 to main and removed the OpenAI conversion hunk already covered by #39771. I also revisited the shared-tokenizer review on #39716. With the actual regular-tokenization method and pinned I am preparing a narrower revision that passes a worker-owned fast-tokenizer backend explicitly to regular tokenization, leaving unsupported tokenizer types on their existing synchronous path. This follows the isolation direction already developed in #39716 without taking over its broader endpoint/ordering work. Would you prefer to fold the focused fix/tests into #39716, or use #30771 as its small prerequisite and fold #44502 back here? Jian, validation of the final candidate on your workload would be especially helpful. I will keep this PR open until we agree on the target and attribution, rather than closing or replacing anyone's work prematurely. |
|
@hnyls2002 @merrymercy I pushed The 6 focused regressions pass (the unmodified base fails the 2 starvation cases); the related CPU suite passes 388 tests + 376 subtests. A synthetic probe with pinned tokenizers 0.23.2 also confirms that loop-side padding no longer waits behind this worker's backend. GPU short-prompt TTFT at low/high concurrency remains unvalidated. Could you review this isolation boundary and authorize |
|
@hnyls2002 @merrymercy could you help choose the merge path for the remaining regular fast-tokenizer fix and authorize CI on #39771 already covers OpenAI conversion; this PR is limited to regular HF fast tokenization with a separate worker-owned backend. The recorded CPU suite passes 388 tests plus 376 subtests, including starvation regressions that fail on the unmodified base. Slow/custom tokenizers keep their synchronous behavior. GPU serving/short-prompt TTFT validation remains outstanding. #39716 and #44502 are still open and overlap this work. Would you prefer this three-file fix as a prerequisite, or fold its implementation and regressions into one of those PRs with attribution preserved? I would like to avoid parallel merge paths. The current upstream tests are blocked on |
Problem
Long regular HF fast-tokenizer calls in
TokenizerManager._tokenize_texts()block the asyncio event loop and delay handlers such as/ping. Moving the call to a worker is insufficient if loop-side callers still share its native backend: a padding/truncation update can wait on the worker's encode.Addresses the regular HF fast-tokenizer case in #30770. OpenAI conversion is already offloaded by merged #39771; this PR leaves that implementation in place.
Changes
self.tokenizeror introducing context-dependent tokenizer properties./generate+/pingresponsiveness, backend separation, current wrapper settings, fallback behavior, serialization and context propagation. HTTP tests substitute model execution.The backend-isolation direction follows @chromecast56's work and @hnyls2002's shared-tokenizer review on #39716. #39716 and @JianWang97's #44502 both build on this PR; coordination to choose one merge path is open. This can remain a small prerequisite or be folded into #39716 with attribution preserved.
Validation
Base:
e2982985110fb432fd8ddba5f0147a5004fd03ab.git diff --checkpassed._tokenize_texts,tokenizers==0.23.2: a loop-side padding call during a 250k-token encode waited 296–307 ms with a deliberately shared-backend control, versus 0.20–0.33 ms with the candidate's separate backend (3 trials each; matching token outputs).Local checks used macOS arm64 / Python 3.11.13, Transformers 5.19.0 and the repository's CPU kernel-stubbing helper. The contention probe is a synthetic correctness investigation, not a GPU or model-serving benchmark. Short-prompt
bench_servingTTFT at low/high concurrency remains unvalidated. Upstream lint passed one0a728540. Upstream test execution remains blocked on the missingrun-cilabel (current gate); maintainer review remains pending.Scope and limits
This adds one native-backend copy per tokenizer manager. Runtime vocabulary mutation is outside this change; backend replacements retain the synchronous path. Slow/custom tokenizer offload, other endpoint preprocessing and global ordering/admission behavior remain separate work. There is no model, kernel, or KV-cache change.
Checklist
CI States
Latest PR Test (Base): ❌ Run #41331127112
Latest PR Test (Extra): ❌ Run #41331127183
Latest PR Test (AMD ROCm 10): ❌ Run #41331127133
Tip
Base CI only covers the core cases, and its workload may not exercise your change. Use
/rerun-testto run the tests your change touches, or copy this prompt for an agent: