feat(vllm-model): pin sampling params for on-policy training - #2190
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
7cd8c48 to
1673d99
Compare
|
Hey @ananthsub, just checking the usecase and pain points here precisely. So the entrypoint where the top_p and temperature is being set is from the Agent Harness itself? I don't remember claude_code or code allowing for that. Is that only for specific open source agents that are allowing it? |
fbca0b7 to
4afa87e
Compare
@ffrujeri I've updated the PR description to clarify this:
|
fc22992 to
c2d3f1d
Compare
c2d3f1d to
3a54871
Compare
3dfab71 to
41aa740
Compare
41aa740 to
c4c8602
Compare
c4c8602 to
3ba17ec
Compare
3ba17ec to
1da3f6c
Compare
…e engine A caller outside the training loop has no way to know the sampling distribution the policy is optimized under, and need not send sampling params at all. The converters forward a field only when it was set, so the body can reach the engine with no temperature or top_p and the engine applies a default of its own. A caller that does send its own values is the other half of the same problem. Add sampling_overrides, applied at every site that builds a request for the engine, since a pin covering some endpoints and not others is off-policy while reporting that sampling is pinned. Unset by default and not set by any shipped config, so this adds a capability and changes no existing run. A deployment that wants it configures it on the model server whose callers cannot supply their own params, and leaves the servers used by callers that already send the right values alone. Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
1da3f6c to
b608524
Compare
Lets a model server put a fixed set of sampling parameters on every request it sends to the engine.
Why an absent parameter is the problem
Gym's converters forward only what the caller set.
temperatureon an auxiliary call such as a title generator or a context compressor. That value is not the configured one either.top_kis a third case, since the Anthropic converter never maps it, so whatever arrives is always a default.Where it hooks in
flowchart LR C["caller request"] --> R{"which API?"} R -->|/v1/responses| P["_apply_sampling_overrides<br/>\napplied last"] R -->|/v1/chat/completions| P R -->|/v1/completions| P P --> E["engine request"] R -->|/tokenize| T["unchanged<br/>\ntakes no sampling params"]All three generation paths. The completions path matters specifically:
chat_completionsbranches to_chat_completions_via_completions_apibefore the usual preprocessing, so an override applied only in preprocessing would be silently inert there. A pin covering some endpoints and not others is off-policy while reporting that sampling is pinned.Precedence
The pin is applied last, so it wins over both what the caller sent and anything
extra_bodymerged in. Values replace rather than fill in, because both failure shapes above are real: an absent parameter and a caller-chosen one.This makes the server authoritative for sampling, which is what #2253 sets out to do through
extra_body_override_keys. Configuring sampling throughextra_bodytoday would mean moving those values tosampling_overridesto get the same authority.Configuration
Unset by default, and no config in this repo sets it, so merging this adds a capability and changes no existing run.
Configure it on the model server whose callers cannot supply their own sampling params, and leave alone the servers used by callers that already do. A Gym agent builds its request from
responses_create_paramson the row, which the integrating framework has already stamped with the right values, including a per-rollout validation profile. Pinning that server would replace those values with themselves at best, and discard a deliberate validation profile at worst. Two kinds of caller therefore want two model server instances, which is an existing pattern.Gym holds no knowledge of any particular framework here. It enforces whatever profile it is given.
What this does not do
A caller that never reads the row cannot receive a per-rollout profile by any mechanism, so a pinned server serves validation rollouts at the profile it was configured with. That is invisible while a run's validation and training profiles are equal, and it is a limitation of the caller rather than of the pin: without the pin, the same run does not work at all.
Testing
responses_api_models/vllm_model/tests/test_app.py::TestSamplingOverridescovers five cases: replacing a value the caller sent, applying when the caller sent nothing, staying a no-op when unset, reaching the completions-API path that skips preprocessing, and winning overextra_bodyon that path.The pin runs after the
extra_bodymerge on each of the three paths, so its precedence does not depend on which endpoint a caller picks.Changed since the approving review
Three things, all narrowing:
sampling_overridesblock is no longer added tovllm_model_for_training.yaml. That file is overlaid by every training run, including native-agent runs whose sampling is already correct.The diff is now two files:
vllm_model/app.pyand its tests.Why it is first in the stack
Without it, token capture records ids sampled at the wrong distribution, so everything above this is capturing the wrong thing correctly. It also stands alone and is useful without the rest.
Followed by #2124.