fix(async): do not let the vLLM prefix cache go stale across weight updates - #4018
Open
linj-glitch wants to merge 2 commits into
Open
fix(async): do not let the vLLM prefix cache go stale across weight updates#4018linj-glitch wants to merge 2 commits into
linj-glitch wants to merge 2 commits into
Conversation
…pdates With async rollouts and vLLM prefix caching enabled (the default on Ampere+), `recompute_kv_cache_after_weight_updates=false` did more than keep in-flight requests on their pre-update KV: it also left the prefix cache untouched across refits. Any later request sharing a prefix with an earlier one (same prompt, shared system prompt, groups spanning a refit) was served KV/state computed by whichever weights first filled the block, with no bound on how old. Since the trainer scores with current weights, the train-vs-rollout logprob mismatch grew step over step and reset only on engine restart. Observed on a Nemotron-H MoE async GRPO run with ~49k-token prompts that share ~85% of their context across same-instance rows recurring 3-20 steps apart: sequences masked by seq_logprob_error_threshold=2 went from 1-2/2048 on a fresh engine to 242/2048 after 17 steps, then back to ~8 on the next engine start. Changes: - Drained refits (no in-flight pause requested) now always invalidate the KV/ prefix cache after the weight update; nothing is using the cache at that point so this is free. The flag only governs what happens to requests that were in flight during an in-flight refit. - In-flight refits with the flag false and prefix caching on print a one-time warning explaining the unbounded staleness and how to fix it. - Default of recompute_kv_cache_after_weight_updates flipped to true (GRPO and PPO async configs). Recipes that set it explicitly are unchanged. - Docs: describe both effects of the flag; example snippets use true. - Tests: drained refit invalidates regardless of the flag; in-flight+false warns once and keeps the cache; no warning when prefix caching is off.
…from kv_cache_management_mode The new default (true) targets vLLM, where the flag also controls prefix-cache invalidation. Megatron generation already encodes the choice engine-side, and its consistency check would now fail for recipes that leave the flag unset with kv_cache_management_mode=persist. Follow the engine mode when the user did not set the flag explicitly; explicit disagreement still errors.
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.
What does this PR do ?
Stops the vLLM prefix cache from silently going stale across weight updates in async GRPO/PPO.
With async rollouts and vLLM prefix caching enabled (the default on Ampere+),
recompute_kv_cache_after_weight_updates=falsedid more than keep in-flight requests on their pre-update KV: it also left the prefix cache untouched across refits. Any later request that shares a prefix with an earlier one (same prompt, shared system prompt, a multi-sample group spanning a refit) was served KV/state computed by whichever weights first filled that block, with no bound on how old those weights were. The trainer scores with the current weights, so the train-vs-rollout logprob mismatch grew step over step within a job and reset only on engine restart. The docs described the flag as affecting in-flight requests only.Evidence
Nemotron-H MoE (Nano 3.5), async GRPO,
in_flight_weight_updates=true, flagfalse, prefix caching default-on, ~49k-token prompts where same-instance rows share ~85% of their context and recur 3–20 steps apart. Logs showSuccessfully reset prefix cacheexactly at engine start and at generation finish, never across the 17 refits in between, so every later request sharing a prefix with an earlier one was served KV/Mamba state computed by weights up to 17 steps old while the trainer scored it with current weights. A controlled A/B from the same checkpoint withenable_prefix_caching=falseconfirmed the mechanism is not what drove the growth of that run's masked-sequence count (that turned out to be a policy-side format collapse, unrelated to caching), so this PR is a correctness fix for silent staleness rather than the cause of a specific regression. The staleness itself is unbounded by construction and the docs described the flag as affecting in-flight requests only.Changes
AsyncTrajectoryCollector.resume_after_refit: refits that drained generation first (no in-flight pause requested) now always invalidate the KV/prefix cache after the update, regardless of the flag. Nothing is using the cache at that point, so this is free. The flag now only decides what happens to requests that were in flight during an in-flight refit (preempt and recompute vs. keep pre-update KV, Magistral-style).AsyncTrajectoryCollector.prepare_for_refit: in-flight refit with the flagfalseand prefix caching enabled prints a one-time warning explaining the unbounded staleness and how to fix it (recompute_kv_cache_after_weight_updates=trueorvllm_cfg.enable_prefix_caching=false).recompute_kv_cache_after_weight_updatesflipped totrueinAsyncGRPOConfigand the PPO async config. Recipes that set it explicitly are unchanged. Cost for in-flight users who relied on the old default: in-flight requests re-prefill once per refit (vLLMpause_generation(mode="keep", clear_cache=True)preempts them and resets the block pool).docs/guides/async-grpo.md: describe both effects of the flag; example snippets usetrue.tests/unit/algorithms/test_async_utils.py): drained refit invalidates even with the flagfalse; in-flight +falsewarns exactly once and keeps the cache; no warning when prefix caching is off. Existing keep-pause and legacy-fallback tests unchanged.If maintainers prefer not to change the default for the performance recipes, the first two changes plus the docs stand alone; the default flip is the last commit hunk in
grpo.py/ppo.py.Megatron generation backend
The second commit keeps the existing
kv_cache_management_modeconsistency check intact under the new default: when the flag is left unset andpolicy.generation.backend=megatron, it is derived from the engine-side mode (recompute→ true,persist/offload→ false) instead of erroring. Explicit disagreement still raises as before.Issues
Root-caused while investigating growing train/inference mismatch in a long-context async GRPO run; no existing issue.
Usage
Before your PR is "Ready for review"
Pre checks:
tests/unit/algorithms/test_async_utils.py)tests/unit/algorithms/test_async_utils.py: 118 passed;tests/unit/algorithms/test_grpo.py+test_ppo.py -k 'async or recompute or kv_cache or refit or config or setup': 107 passed;tests/unit/single_controller/test_refit_recovery.py+tests/unit/models/generation/test_swe1_dynamo_config.py: 37 passed (run inside the NeMo RL container).docs/guides/async-grpo.md)Additional Information