Repository navigation
Conversation
…he fp32 master weights
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
…lict # Conflicts: # tests/experimental/test_async_grpo_trainer.py
… async_distillation - _init_inflight_cap and _control_loop now distinguish "no matching metric line" from a genuinely idle/empty server, instead of defaulting to 0 and silently relaxing or over-tightening the in-flight cap on a vLLM /metrics format change. - async_distillation's VLLMClient.pause() was missing the clear_cache=false change made to async_grpo's, leaving it on the old clear-every-sync behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| them makes vLLM preempt every running request and recompute it from scratch after the update.""" | ||
| response = requests.post( | ||
| f"{self.server_url}/pause", params={"mode": "keep", "clear_cache": "false"}, timeout=timeout | ||
| ) |
There was a problem hiding this comment.
Stale cache across weight updates
High Severity
pause now passes clear_cache=false on every weight sync, so vLLM keeps KV and prefix-cache blocks built with the previous weights. On the merged-weight path those blocks stay reusable, and later requests, especially repeated prompts, can decode against a mix of old and new weights. The LoRA path avoids this only because each sync uses a new adapter name.
Reviewed by Cursor Bugbot for commit 3494835. Configure here.
There was a problem hiding this comment.
I looked a bit deeper into how vLLM handles this, and I think there are a few important points.
First, in AsyncGRPO we already tag rollouts with the policy version before dispatching them, and drop samples that exceed max_staleness. So we already account for policy staleness during training. Recomputing the KV cache doesn't change the tokens or logprobs already sampled with the previous weights.
The real concern is the prefix cache for new requests. With clear_cache=false, a new request could reuse a prefix computed several policy versions ago. This isn't captured by our staleness check, so I agree there's a potential issue here.
But the performance cost of clearing the cache is huge, and I think this is the important part:
- TRL supports vLLM >=0.21. Until now, we called
pause(mode="keep")without specifyingclear_cache, so it defaulted totrue. - In practice, this preempts every running request at each weight sync, clears the prefix cache, and forces all requests to prefill again from scratch when resumed.
- With
clear_cache=false, running requests keep their KV cache and resume decoding directly after the sync, without any extra prefill.
In our sanity run, that's around 500 requests × 3k tokens = 1.5M tokens unnecessarily re-prefilled at every sync. This explains the bursty throughput we were seeing on main.
So clear_cache=false is actually the behavior our TRL docstring described IMO.
I'd prefer keeping false. We already handle rollout staleness, and avoiding this massive amount of recomputation is important for async. @qgallouedec @kashif
|
Follow-up on the review question about whether the gains come from the cap or from
The cap is what removes the queueing: 4× lower latency, half the staleness, no drops, and 15 minutes saved. Keeping the cache on its own saves 9 minutes but changes little else, because with 512 requests competing for a ~100-request cache, the server is still preempting for memory regardless. Keeping the cache really pays off once the cap holds the server within its cache capacity. The 35,638 preemptions left in the cap-only run are roughly one per running request per sync, corresponding to prefill recomputation. With Curves: https://huggingface.co/spaces/aminediroHF/trl-sanity (runs |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2ada061. Configure here.


AsyncGRPO: cap in-flight rollouts on the vLLM server's KV cache usage
What this does
The rollout worker currently admits up to
max_inflight_tasksrequests to vLLM (defaultmax_staleness * samples_per_step, 512 in the runs below). That number knows nothing about the server: when the KV cache cannot hold that many sequences, vLLM queues the rest and preempts running ones as they grow, so every request spends most of its life waiting inside the server before it decodes.This PR makes the worker follow the server instead. A small control loop polls
/metricsevery 5 s and keeps a live cap on in-flight requests:kv_cache_size_tokens / max_model_len, as many full-length sequences as the cache can hold, never abovemax_inflight_tasks.max_inflight_tasks.New rollout metrics:
rollout/inflight_cap,rollout/kv_cache_usage,rollout/vllm_running_requests,rollout/vllm_queued_requests,rollout/vllm_preemptions_total.It is the single-environment version of prime-rl's
ConcurrencyController(PrimeIntellect-ai/prime-rl#3285). No new config: everything is read from the server.Experiment
Sanity bed:
deepseek-ai/DeepSeek-R1-Distill-Qwen-1.5Bonsail/Sanity-Test-R1D-1.5B, vLLM 0.29 with data parallel 2 on two H200, the trainer on two more, 128 completions per step,max_completion_length=8192,max_staleness=4, 300 steps. To make the KV cache the bottleneck, as it is for any real model size, the server was started with--kv-cache-memory-bytes 8Gper replica: about 0.6 M tokens in total, or roughly 100 completions at the 6.1 k mean length this recipe produces. Everything else is identical between the two runs.kv8g-main:main@ 72cf37d, 512 in flightkv8g-cap: this branch, adaptive capCurves: https://huggingface.co/spaces/aminediroHF/trl-sanity
The cap tracks the server
The cap starts near 60 from the cache size, grows while usage is under 0.6 and backs off as it passes 0.8, settling between 70 and 130 requests, which is what the cache actually holds.
mainpushes 512 regardless: the server runs about 100 of them and queues the other 400.What over-admission costs
Completions are the same length on both runs, so the 4x latency gap is queue time inside vLLM. At 30 s per optimizer step a sample that takes 94 s comes back three policies late, which is why
mainsits at the staleness ceiling and drops 28 % of what it generates. The cap run never submits more than the cache holds, every request decodes immediately, and nothing is dropped.Raw generation throughput is about 15 % higher on
main: a fuller decode batch is more efficient per token. But 28 % of those tokens are thrown away as stale, so useful throughput is higher with the cap, and the trainer waits less per step. The learning curve is the same, as it should be: the cap changes when rollouts are admitted, not what is trained on.Why we want it
main.Not in this PR
Staleness on an oversized pool is a separate problem. With the full cache, 512 in flight still drops 42 % of samples as stale on this bed because the pool is sized by
max_stalenessrather than by what the trainer consumes. The cap only reacts to server load, so that needs its own change.Note
Medium Risk
Changes rollout admission and weight-sync pause semantics for all AsyncGRPO runs; mis-parsed metrics fall back to the ceiling, but wrong caps could still throttle or overload vLLM.
Overview
Adds adaptive concurrency for AsyncGRPO rollouts: the worker no longer always admits up to
max_inflight_tasksvLLM requests. A background_control_looppolls/metricsevery few seconds and adjusts a live_inflight_cap(still bounded bymax_inflight_tasks). Startup sets the cap fromkv_cache_size_tokens / max_model_lenwhen metrics are available; under load it cuts by 30% when KV usage exceeds 0.8 and grows by 25% when usage is below 0.6 with no queued requests. Admission in_generate_loopgates on this cap instead of a fixed slot pool.Documents the behavior under a new Concurrency section and logs
rollout/inflight_cap,rollout/kv_cache_usage, and related vLLM queue/running/preemption metrics.Weight sync changes: vLLM
pausenow usesclear_cache=false(async GRPO and async distillation clients) so cache clears do not preempt in-flight generations.WeightTransferClientacceptscompute_dtypefrom trainerbf16/fp16so dtype mismatch warnings compare against compute precision, not fp32 master weights.Includes an integration test
test_inflight_cap_follows_vllm_loadwith a fake metrics server.Reviewed by Cursor Bugbot for commit a5e14a6. Bugbot is set up for automated code reviews on this repo. Configure here.