Repository navigation
Use vLLM's own server instead of TRL's - #6765
Conversation
|
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 659619393f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
827c4df to
73a0661
Compare
73a0661 to
4284a70
Compare
`trl vllm-serve` ran a custom FastAPI app around `LLM`, with its own data-parallel fan-out and a worker extension for weight sync. vLLM's server covers all of it, so the app is deleted and the command becomes a deprecated wrapper that translates its flags into `vllm serve`, prints the equivalent command, and forwards anything it does not know about. - generation goes to `/v1/completions` for token IDs. Multimodal prompts have no OpenAI-compatible endpoint that takes token IDs and images at once, so the images are processed on their own and the resulting features are paired with the trainer's token IDs on `/inference/v1/generate`, which keeps multi-turn generation working with images - weight sync uses vLLM's NCCL weight-transfer engine: the tensors are announced once and streamed in a single packed broadcast, rather than one HTTP request and one broadcast per tensor - teacher logprobs for distillation come from `prompt_logprobs` - the docs teach `vllm serve` directly, and the web-server dependencies go The request sets `output_kind` explicitly, as the server would otherwise drop completions when `n > 1` (vllm-project/vllm#52399).
4284a70 to
92abffe
Compare
The sections explaining vLLM's own workers, tensor parallelism and PagedAttention belong to vLLM's documentation, more so now that TRL runs its server rather than its own. The throughput plots went with them: they were measured on older vLLM versions and the warning underneath already told readers not to follow the data parallel advice they show.
The required settings are listed in the table below, the reward and loss steps belong to the trainer docs, and the separate-GPUs example appears verbatim right after the quickstart. What is left is the part specific to TRL: which endpoints the trainer calls, and which way the weights travel.
`trl vllm-serve`'s custom app, and with it `/get_sequence_logprobs/`, goes away in huggingface#6765, so the rollout worker teacher-forces the student's completion through vLLM's own endpoint instead: `max_tokens=1` with `prompt_logprobs=teacher_top_k`, sliced from `len(prompt_ids)`. Same request `VLLMClient.get_sequence_logprobs` issues for the synchronous server-teacher trainers. The response is parsed here rather than through that client: it truncates each position to exactly `top_logprobs` after rank-sorting and reports the realized token in separate fields, which would drop that token wherever it ranks below k, zeroing the teacher signal there at `beta=1.0`. Parsing the raw `prompt_logprobs` mapping keeps it at any rank, so the candidate rows stay `teacher_top_k + 1` wide and `_narrow_top1_actual_support` keeps finding what it indexes. `/v1/completions` names the model it addresses and, under MOPD, every teacher serves a different one, so each teacher's served id is resolved from its `/v1/models` at worker startup. That doubles as the teachers' readiness wait. Teacher servers are now plain `vllm serve` and need `--logprobs-mode processed_logprobs` (or `teacher_temperature` never reaches their logprobs) and `--max-logprobs -1` (or `teacher_top_k > 20` is rejected). Documented, along with the pre-existing requirement that every teacher share the student's tokenizer, since teacher candidate ids index the student's vocabulary directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Upstream huggingface#6715 reworked what AsyncGRPO measures; huggingface#6674 reworked how the sync distillation trainer reads a model's logit post-processing. Both apply here. Metrics, following huggingface#6715's namespaces (`rollout/`, `completions/`, `sample/`, `batch/`, `perf/`) and its reduction rule — the value's shape and the key's suffix pick the reduction, so a rate is stored as a `(num, den)` pair and reduced as Σnum/Σden instead of as a mean of ratios: - the worker gets a `metrics_queue` the trainer drains in `log()`, so it can report what only it can see: `rollout/duration_s`, `rollout/score_s`, `rollout/generated_tok_s` (windowed), `rollout/inflight`, `rollout/backpressure_s`, `rollout/vllm_retry_total` and the `completions/*` family. - per-sample gauges no longer ride on `RolloutSample.metrics` and are no longer NaN-padded into the batch, broadcast to every rank and reduced back to compute a number rank 0 already had. The collator logs the sample and packing metrics directly; `sample.enqueued_at` gives the trainer `sample/time_in_queue_s`. - new per-step accounting (`perf/step_s`, `fwd_bwd_s`, `fwd_s`, `optimizer_s`, `rollout_wait_s`, the three weight-sync phases) plus throughput and MFU on both a compute and a wall-clock basis, which is what separates a slow trainer from a starved one. - trainer-side queue metrics (`sample/rollout_queue_size`, `staleness_*`, `dropped_stale_total`, `batch/dropped_oversize_total`). - traces are sampled per policy version rather than per N samples, which at a few hundred samples/step buried the real metrics under empty rows. Two metrics are distillation's own, since the teacher call sits on the critical path of every rollout rather than in a separate scoring loop: `rollout/score_s`, and `teacher_score_s/{teacher_id}` under MOPD — one slow expert throttles only the rollouts routed to it, which the blended mean hides. `docs/source/async_distillation_trainer.md` documents every metric. Also in this commit: - read `logit_scale` / `final_logit_softcapping` through `get_text_config()` and fall back to Muse Glimmer's `output_multiplier`, matching `DistillationTrainer`. - drop `# docstyle-ignore` from `AsyncDistillationConfig` (huggingface#6785) and let the styler reflow it. Two pre-existing line-break artifacts it exposed — a broken markdown link and a broken class reference — are fixed rather than reflowed into literal spaces. - raise `AsyncRolloutWorker`'s vLLM gate from 0.17.1 to 0.22.0, which is what `weight_transfer.py` and the docs already require and which is above TRL's own supported floor. - drop a stale `trl vllm-serve` mention left over from the huggingface#6765 migration. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e48e5a6. Configure here.
| messages = [ | ||
| prepare_multimodal_messages(prompt, images=[image] if image is not None else None) | ||
| for prompt, image in zip(prompts, images, strict=True) | ||
| ] |
There was a problem hiding this comment.
None-image rows break multimodal chat
Medium Severity
Passing images=None into prepare_multimodal_messages for None-image rows does not work with the earlier training_step rewrite that inserts {"type": "image"} placeholders on every conversational prompt whenever an image column is present. prepare_multimodal_messages then sees one unfilled placeholder and zero images and raises, so Online DPO server-mode VLM batches that mix real images with None fail. The old generate(..., images=...) path tolerated those rows.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e48e5a6. Configure here.
|
Merging without review. This one is quite big and important so please feel free to give late review share if anything is missing. |


trl vllm-serveran a custom FastAPI app aroundLLM, with its own data-parallel fan-out and a worker extension forweight sync. vLLM's server covers all of it, so the app is gone and the command is now a deprecated wrapper that
translates its flags into
vllm serveand prints the equivalent command.Changes
trl/scripts/vllm_serve.pygoes from 1218 to ~130 lines! It adds the three settings TRL needs(
--weight-transfer-config,--logprobs-mode processed_logprobs,--max-logprobs -1), setsVLLM_SERVER_DEV_MODE=1, and forwards any other argument tovllm serve.fastapi,uvicornandpydanticleave thevllmextra./v1/completionsfor token IDs. No OpenAI-compatible endpoint takes token IDs and images, somultimodal prompts have their images processed on their own and the resulting features paired with the trainer's
token IDs on
/inference/v1/generate, which keeps multi-turn generation working with images.prompt_logprobs.vllm serveis the documented way to start the server.No change to training code:
vllm_mode,vllm_server_base_url,vllm_group_portand the rest behave as before,trl vllm-servestill accepts the same flags, and GRPO and RLOO are untouched.Benchmark
GRPO in server mode on H100s, Qwen2.5-1.5B (text) and Qwen2.5-VL-3B (VLM), same seed and same data, server on its own GPUs and a two-process trainer.
Same behaviour: with a fixed generation seed the two implementations produce bit-identical completions (16/16 over 4 prompts x n=4, logprobs identical to 0.0 over 499 tokens). The training curves still do not lie on top of each
other, for reasons that have nothing to do with this PR.
Why the curves do not overlap exactly
The text curves diverge because each run talks to its own server process. Against the same server, two runs of
the same code are bit-identical, so the trainer itself is deterministic:
Give each run its own server — which comparing two implementations necessarily does — and they drift apart, because a
fresh engine returns slightly different logits and one flipped token changes every later step. That drift is as large
between two runs of
mainas it is betweenmainand this PR:The VLM curves sit apart for a real reason. The server returns the processed image already cast to the model
dtype, while handing the engine the image directly preprocesses in float32. The values are otherwise the same:
So the image is the same image, rounded to the dtype the model runs in. That moves logprobs by ~0.04 and occasionally flips a greedy argmax: over three greedy generations the two implementations agreed on two and differed by one word on the third, both naming the colour correctly.
Validation
On H100s against vLLM 0.27.1:
tests/test_vllm_client_server.py -m slow: 34 passedTwo fixes came out of this: vllm-project/vllm#52399, where
/inference/v1/generatedrops completions whenn > 1(the client setsoutput_kindexplicitly until it lands), andthe client here no longer builds its NCCL communicator on an unindexed device, which on
mainbreaks asingle-process trainer with
this nccl communicator is created to work on cuda, but the input tensor is on cuda:0.Note
High Risk
Server-mode training depends on new vLLM HTTP/NCCL paths and required serve flags; regressions would affect generation, weight sync, and distillation across GRPO/RLOO/Online DPO, though trainer config surface is unchanged.
Overview
Replaces TRL’s custom FastAPI vLLM server with upstream
vllm serve.trl vllm-serveis deprecated: it only maps TRL flags tovllm serve, setsVLLM_SERVER_DEV_MODE=1, injects TRL-required options (NCCL weight transfer,processed_logprobs, unlimited logprobs), and forwards extra CLI args. Thevllmextra drops FastAPI/uvicorn/pydantic.VLLMClientnow talks to vLLM’s HTTP API: text/token prompts via/v1/completions, chat/multimodal via/v1/chat/completions, and token-ID + image features via/inference/v1/generateafterimage_features(/v1/chat/completions/render). Teacher logprobs for distillation useprompt_logprobson completions instead of a custom endpoint. Weight sync uses vLLM’s NCCL weight-transfer engine (init_weight_transfer_engine, batchedupdate_weights, optionalstart_weight_update/finish_weight_update) with aweight_update()context for one sync per optimizer step.Trainers and
VLLMGenerationare updated to match (e.g. Online DPO multimodal →chat, packed param streaming, prefix-cache reset). Docs, examples, and config help text now documentvllm serveinstead oftrl vllm-serve.Reviewed by Cursor Bugbot for commit e48e5a6. Bugbot is set up for automated code reviews on this repo. Configure here.