[fix] /pause_generation and /continue_generation wrong for --tokenizer-worker-num > 1 - #24462
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
/tag-and-rerun-ci |
…r-worker-num > 1 Fixes sgl-project#21235. (Resubmit of sgl-project#24445 which was reverted in sgl-project#24461 due to a _check_all_req_types import failure on the helper class. This version ships the rename inline so io_struct loads cleanly.) With --tokenizer-worker-num=N, there are N independent TokenizerWorker processes, each holding its own is_pause flag. The HTTP load balancer dispatches each /pause_generation or /continue_generation to a single worker, so previously only 1 of N workers had its flag toggled. After a pause+continue cycle, with probability (N-1)/N one worker remained stuck is_pause=True; any /generate request the load balancer routed to that worker hung forever (and in PD disaggregation, with 8 workers, 15/64 of requests would wait forever for KV transfer). Fix: route pause/continue through MultiTokenizerRouter, which fans out to all registered tokenizer manager IPCs. - TokenizerWorker registers its IPC with the router on startup via TokenizerWorkerRegistration (control message; deliberately NOT named with a Req/Input/Output suffix since it is not a scheduler IO type and would otherwise trip io_struct._check_all_req_types). - Router intercepts PauseGenerationReqInput / ContinueGenerationReqInput in router_worker_obj, emits PauseContinueBroadcast to every registered worker IPC, then forwards once to scheduler rank 0 (which broadcasts to all TP/PP/DP ranks internally). Skipped for abort mode, which drains via the originator's existing abort polling. - Workers apply the broadcast under the existing is_pause_cond and call notify_all() on continue. - Originator awaits a Future resolved when its own broadcast lands, so the HTTP response only returns after the local flag is flipped. Added latency: one IPC round-trip (worker -> router -> worker), sub-ms on a single host. Co-Authored-By: lawrence-harmonic <185285563+lawrence-harmonic@users.noreply.github.com>
e038ecd to
a810613
Compare
|
/tag-and-rerun-ci |
|
Does this prevent /pause_generation from being reordered with /update_weights_from_distributed when they are sent to different workers? This was an issue I had to account for in #21237 by introducing _ensure_paused_or_model_locked Right now the updates weights only checks is_paused once, if it is False at the beginning, then it will wait on model_update_lock forever.
|
…r-worker-num > 1 (sgl-project#24462) Co-authored-by: lawrence-harmonic <185285563+lawrence-harmonic@users.noreply.github.com>
…zer-worker-num > 1 (#24769) Co-authored-by: maocheng23 <35615230+maocheng23@users.noreply.github.com> Co-authored-by: lawrence-harmonic <185285563+lawrence-harmonic@users.noreply.github.com>
…r-worker-num > 1 (sgl-project#24462) Co-authored-by: lawrence-harmonic <185285563+lawrence-harmonic@users.noreply.github.com>
Summary
Resubmit of #24445 (which was reverted in #24461 due to a
_check_all_req_typesimport-time failure). This version ships the rename inline soio_structloads cleanly.Fix
/pause_generationand/continue_generationnot propagating to all tokenizer workers when--tokenizer-worker-num > 1. Previously each HTTP request only flippedis_pauseon the single worker that handled the request, leaving other workers in an inconsistent state and causing/generaterequests routed to the stuck worker(s) to hang forever.Fixes #21235.
Difference from #24445
TokenizerWorkerRegisterReq→TokenizerWorkerRegistration.This is a worker→router control message, not a scheduler IO type.
io_struct._check_all_req_typesenforces (at import time) that any class ending inReq/Input/Outputmust subclassBaseReq/BaseBatchReq. Renaming away from that suffix is more semantically appropriate than retrofittingBaseReqinheritance. This was the cause of the post-merge CI failure that prompted the revert.Root cause
With
--tokenizer-worker-num=N, there are N independentTokenizerWorkerprocesses (subclasses ofTokenizerManager), each holding its ownis_pauseflag (tokenizer_manager.py:416). The HTTP load balancer dispatches each request to one worker, so:/pause_generationonly setsis_pause = Trueon 1 of N workers (tokenizer_manager.py:1486-1490)./continue_generationonly setsis_pause = Falseon 1 of N workers — likely a different one (tokenizer_manager.py:1501-1505).After a pause+continue cycle, with probability
(N-1)/None worker remains stuckis_pause = True. The per-worker flag gates incoming requests attokenizer_manager.py:542-543(await self.is_pause_cond.wait_for(lambda: not self.is_pause)), so any subsequent request the load balancer routes to the stuck worker hangs forever. With PD disaggregation and 8 workers, 15/64 of requests need to traverse the stuck worker on either prefill or decode and hang waiting for KV transfer.The shared scheduler's
_engine_pausedflag was toggled correctly (it's a single process), so generation resumed globally — but per-worker request gating was broken.Fix
Route pause/continue through
MultiTokenizerRouter, which fans out to every registered tokenizer manager IPC.TokenizerWorkerRegistration— eachTokenizerWorkerregisters its IPC name with the router on startup.PauseGenerationReqInput/ContinueGenerationReqInputinrouter_worker_obj, emitsPauseContinueBroadcastto every registered worker IPC, then forwards to the scheduler (skipping formode="abort", which drains via the originator's existing abort polling). We only need to send to one scheduler and it will broadcast to others — the router forwards once to scheduler rank 0, which fans out to all TP/PP/DP ranks internally via the existing scheduler-side broadcast path.is_pause_condand callnotify_all()on continue.Futureresolved when its own broadcast lands, so the HTTP response only returns after the local flag is flipped (other workers are flipped in parallel).Total added latency: one IPC round-trip (
worker → router → worker), sub-ms on a single host.Test plan
/generatewhile paused, assert none complete; resume; assert all complete).test/registered/tokenizer/test_multi_tokenizer.pypasses with--tokenizer-worker-num > 1.test/registered/unit/managers/test_scheduler_pause_generation.pypasses.test/registered/language/test_srt_backend.pypasses (this was the regression on [fix] /pause_generation and /continue_generation wrong for --tokenizer-worker-num > 1 #24445 caused by the naming check; verifies the rename fixes it).test/registered/rl/test_pause_generation_tensor_consistency.pypasses.Notes / follow-ups
_pause_continue_future; concurrent ops landing on the same worker process can overwrite it. Sequential usage (the bug repro) is fine; anop_id-keyed future map would harden this.model_update_lock. Scheduler-sideabort_requestclears global scheduler state, but per-worker tokenizer-side in-flight state on non-originator workers isn't drained by the originator's poll. Consider broadcasting the abort poll if that becomes load-bearing.TokenizerWorkers sendingTokenizerWorkerRegistrationbefore HTTP traffic begins. Standard startup ordering handles this.