Skip to content

fix: /pause_generation with --tokenizer-worker-num > 1 - #21237

Closed
lawrence-harmonic wants to merge 4 commits into
sgl-project:mainfrom
lawrence-harmonic:fix/pause_multi_tokenizer
Closed

lawrence-harmonic wants to merge 4 commits into
sgl-project:mainfrom
lawrence-harmonic:fix/pause_multi_tokenizer

Conversation

@lawrence-harmonic

@lawrence-harmonic lawrence-harmonic commented Mar 23, 2026

Copy link
Copy Markdown
Contributor

Motivation

Fix #21235

Modifications

We set the pause state in shared memory and poll it to update the local is_pause / is_pause_cond. Note that the consistency of is_pause will be reduced to eventual consistency in the multi-worker case. e.g. a pause immediately followed by a weight update may result in the weight update not seeing is_pause set immediately. Thus, we change the logic to continue checking for the pause flag while waiting for the writer lock.

The introduction of RWCondition is to achieve the following: we want to block others from changing is_pause during weight update, but not from polling is_pause.

NOTE: We do not fix the following existing issues:

  • self.model_update_lock.writer_lock is process-local, thus does not actually ensure global exclusivity
  • Similarly, /continue_generation can go to a different tokenizer worker and thus bypass the local condition variable lock.

In my opinion, the most correct way to do synchronization would be to have a system-wide model rwlock. Then, take reader locks in the schedulers instead of the tokenizers (maybe also take the writer locks in the schedulers instead of the tokenizers). The schedulers can release lock when paused. Then update weights could then just always take the model lock, simplifying the pause vs lock casework (skipping the lock, as we do now, technically works now but is a bit strange).

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@lawrence-harmonic
lawrence-harmonic force-pushed the fix/pause_multi_tokenizer branch from 90ecf63 to 8c2c6cd Compare March 24, 2026 05:50
@lawrence-harmonic

Copy link
Copy Markdown
Contributor Author

/tag-and-rerun-ci

/pause_generation sets `is_pause` only on one worker.
/continue_generation unsets `is_pause` on a potentially different
worker.

e.g. with 8 workers, with 7/8 probability we will leave 1/8 of workers
paused, thus 1/8 of all requests will hang. With PD disaggregation, with
15/64 probability a request will get routed to one worker which is
paused and one worker which is not paused, and these requests will wait
forever for KV transfer.

We fix this by setting the pause state in shared memory and polling it
to update the local `is_pause` / `is_pause_cond`. Note that the
consistency of `is_pause` will be reduced to eventual consistency in the
multi-worker case. e.g. a pause immediately followed by a weight update
may result in the weight update not seeing `is_pause` set immediately.
Thus, we change the logic to continue checking for the pause flag while
waiting for the writer lock.

NOTE: We do not fix the following existing issues:
* /continue_generation is not safe during a weight update which started
  while paused
* weight update with multi-worker without pausing is not safe, i.e.
  `self.model_update_lock.writer_lock` is local, but the lock needs to
  be acquired across all workers
@lawrence-harmonic
lawrence-harmonic force-pushed the fix/pause_multi_tokenizer branch from a3893a4 to 37a3c7d Compare April 17, 2026 18:39
@lawrence-harmonic

Copy link
Copy Markdown
Contributor Author

/tag-and-rerun-ci

@Qiaolin-Yu

Copy link
Copy Markdown
Collaborator

Thanks for the contribution! We will merge this in #24462 and have added you as a co-author.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] /pause_generation and /continue_generation wrong for --tokenizer-worker-num > 1

2 participants