Repository navigation
[Fix] scheduler: Publish KV events while the engine is paused - #46334
Conversation
The paused branch of every scheduler event loop skips on_idle, so events recorded while paused were only published after continue_generation. In the RL weight-update sequence (pause in retract mode, update weights with flush_cache, continue), the AllBlocksCleared from the flush therefore reached KV-event routers only after resume, and they kept routing against the old tree for the whole update. Publish from _record_scheduler_state_for_paused_engine, which every pausable loop calls on each paused iteration. Signed-off-by: Sohom Chakraborty <16609933+sohom-cs@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
tejachavali
left a comment
There was a problem hiding this comment.
This fixes a real gap. A paused scheduler loop never reaches on_idle, so KV events recorded during the pause, like the AllBlocksCleared from a weight update flush, were held until resume while routers kept scoring against stale blocks. Adding the publish to _record_scheduler_state_for_paused_engine is the right place, since every paused loop already calls that helper. The new test is a genuine one: without the fix nothing drains the flushed event before the assertion.
One thing to confirm before merge: the PR test runs currently show as failed. Are those failures unrelated to this change? Otherwise I have no objections.
|
Thanks for the review. Yes, they're unrelated: the PR has no run-ci label yet, so pr-gate fails by design and the *-finish jobs only report that; none of the test suites has run. Locally the new test fails on main and passes with the fix. |
BBuf
left a comment
There was a problem hiding this comment.
[BBuf Review Agent] LGTM. Let's wait for the related CPU CI.
|
/tag-and-rerun-ci |
|
[BBuf Review Agent] Could you rebase this onto the latest main? It conflicts with main now. |
|
/tag-and-rerun-ci |
Signed-off-by: Sohom Chakraborty <16609933+sohom-cs@users.noreply.github.com>
|
Done: merged current The CI run for this push may stop at pr-gate on my 120-minute author cooldown; I'll rerun it once that clears. |
|
/rerun-failed-ci |
|
/rerun-failed-ci |
…oject#46334) Signed-off-by: Sohom Chakraborty <16609933+sohom-cs@users.noreply.github.com>
Motivation
An RL weight sync runs
pause_generationin retract mode, thenupdate_weights_*withflush_cache=True, thencontinue_generation. The flush queues AllBlocksCleared, but a paused scheduler never publishes KV events, so a KV-event router (sgl-router, or Dynamo) keeps that worker's whole pre-flush cache view for the entire sync and scores requests against blocks that no longer exist. The clear reaches the router only after resume.#24003 added a publish at pause entry. It covers what was queued up to the pause, not what happens during it, such as the weight update's flush.
Found while following #24003's pause-time flush through a weight update.
Modifications
Every pausable event loop (normal, overlap, PD prefill, PD decode, MLX) calls
_record_scheduler_state_for_paused_engine()on each paused iteration and skipson_idle. This adds onepublish_kv_events()call to that helper, which covers the flush and anything else recorded while paused. The call does nothing on ranks without a publisher, and on an idle paused loop it only drains an empty queue.Accuracy Tests
In
test/registered/unit/managers/test_scheduler_pause_generation.py:Results:
Lists differ: [] != ['AllBlocksCleared']CPU only, stub scheduler, Python tree core. I have not run it against a live router or an RL job.
Speed Tests and Profiling
N/A
Checklist
Related: #46258 (every decode step publishes; this PR covers the paused loop). #46307 edits the same paused-loop branch for a different reason; the two changes are independent.
Developed with AI assistance (Claude); every changed line was reviewed and the tests were run locally by me.
CI States
Latest PR Test (Base): ✅ Run #41272361376
Latest PR Test (Extra): ❌ Run #41272361479
Latest PR Test (AMD ROCm 10): ✅ Run #41272361313
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: