Conversation
fzyzcjy
requested review from
Shi-Dong,
Zhichenzzz,
guapisolo,
jybsuper,
maocheng23 and
yueming-yuan
as code owners
August 27, 2026 07:47
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
pause-generation-mode=in_place freezes in-flight requests across a weight update so their KV survives, which also means the engine-side prefix-cache flush cannot run (flush_cache refuses while requests hold KV, and the engine asserts on it). The update path therefore skips the flush in this mode -- but nothing ever invalidates the radix tree, so prefixes cached under pre-update weights keep serving every later request that shares them. Rollouts silently mix old-policy KV into new-policy generations, the recorded rollout log probs describe that hybrid, and the train-vs-rollout log-prob gap grows with every update. The per-request weight_version bookkeeping (mixed_version_ratio) only tracks requests alive across an update, so it stays near zero while the prefix reuse poisons all subsequent traffic. retract does not have this problem: every retracted request recomputes its KV under the new weights and the update path flushes the prefix cache, which actually succeeds under load since #1750 and sgl-project/sglang#31962. Flip every recipe that defaulted or hardcoded in_place over to retract, and document why in the fully-async guide. examples/multi_lora keeps in_place deliberately: adapter upsert relies on it (an unload would deadlock behind paused requests) and its --use-tis corrects the training-side bias.
fzyzcjy
force-pushed
the
tom/fix-inplace-stale-radix-cache
branch
from
August 27, 2026 07:58
bef6eef to
80f79db
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TODO: show cache weight version, fix the unbounded issue
Summary
--pause-generation-mode in_placeleaves the SGLang radix (prefix) cache poisoned with pre-update KV after every weight update, so later rollouts silently generate on a mixture of new weights and old-policy KV. Every fully-async recipe on main currently defaults to or hardcodesin_place. This PR flips them toretract, which recomputes retracted requests' KV under the new weights and flushes the prefix cache on every update.Mechanism
pause_generation(mode="in_place")freezes in-flight requests so their KV survives the update — that is the mode's purpose. Because those frozen requests hold KV, the engine-side cache flush cannot run:scheduler.flush_cacherefuses while requests hold pages, and the engine's update pathsasserton flush success. sglang's ownio_struct.pydocuments it: "In 'inplace' mode, flush_cache will fail if there are any requests".in_place(update_weight_from_distributed/mixin.py:315-316, same guard indelta.pyandupdate_weight_from_tensor.py).--n-samples-per-prompt, earlier turns of a multi-turn conversation) alive indefinitely — so the poisoning is unbounded in time and covers all subsequent traffic, not just the requests frozen across one update.Symptom
rollout_log_probsdescribe that hybrid, while the trainer recomputes log probs under the current weights, sotrain/train_rollout_logprob_abs_diffgrows with every update instead of sitting at cross-implementation numeric noise, and the effective behavior policy lags the trainer roughly in proportion to the cached-prefix token share.rollout/weight_version/mixed_version_ratiometric stays near zero throughout: per-request weight-version events only track requests alive across an update, and radix reuse by new requests is invisible to them. The corruption is therefore silent on the dashboard.--update-weights-interval 1.Why retract is the right default now
retractreturns in-flight requests to the waiting queue, recomputes their KV under the new weights, and the update path flushes the prefix cache — both halves of the staleness problem disappear.retractworks under high concurrency.in_placestays available for setups that need it, but until the engine invalidates version-stale prefixes on its own, it should be an explicit opt-in with known cost, not the default.Changes
examples/infra_features/fully_async/run_qwen3_30b_a3b_fully_async.pyin_place→retractexamples/infra_features/fully_async/run_qwen3_5_4b_fully_async_eval.pyretractexamples/infra_features/random_async/run_random_async_3node.pyretractexamples/swe-agent-harbor-docker/run-glm47-flash-agentic-async.pyretractexamples/experimental/openenv/glm52_tbench2/run_glm5_2_744b_a40b_daytona.pyretractscripts/run_inkling.pyretractdocs/user-guide/fully-async.md,docs/examples/infra-features/random-async.md,examples/infra_features/random_async/README.mdDeliberate exception:
examples/multi_lora/run_multi_lora.pykeepsin_place— its adapter upsert relies on it (an unload would deadlock behind paused in-flight requests), and its--use-tiscorrects the training-side bias since the recorded log probs faithfully describe the (lagged) behavior policy.Follow-up
in_placesafe again (including for multi-turn workloads, which lose cross-turn prefix reuse under any flush-based scheme).Tests
pre-commit run --all-filesclean. The change is recipe defaults plus docs; behavior is covered by the existing fully-async paths that already runretract.