Skip to content

feat(vllm_model): add extra_body_override_keys for server-authoritative sampling - #2253

Closed
GNroy wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
GNroy:fix/vllm-model-sampling-parity
Closed

feat(vllm_model): add extra_body_override_keys for server-authoritative sampling#2253
GNroy wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
GNroy:fix/vllm-model-sampling-parity

Conversation

@GNroy

@GNroy GNroy commented Jul 30, 2026

Copy link
Copy Markdown

Summary

VLLMModelConfig.extra_body supplies defaults only. Every merge site does

body_dict = extra_body | body_dict

so a value set explicitly on the request always wins. That is a sensible default, and the
/v1/completions builder documents it as deliberate ("extra_body fields do NOT override
request-level fields"). But it leaves an operator with no way to make the server authoritative
for a sampling parameter.

This PR adds an opt-in escape hatch. It does not change the default.

Why an operator would want this

When a model server generates rollouts for RL training, the sampling parameters actually used must
match the training config — otherwise the samples are off-policy but are attributed to the policy.

Agent harnesses routinely hardcode sampling parameters on internal or auxiliary calls: a title
generator or a context compressor that pins temperature low for a deterministic sub-task. Those
calls go through the same model server as the rollout itself, so the server silently emits
off-policy generations that are indistinguishable downstream.

With NeMo-RL this is not even silent. The async vLLM worker asserts that a request's temperature
and top_p match the generation config; a mismatched auxiliary call 500s, the client retries, and
once retries are exhausted the entire trajectory is discarded. In a multi-turn agentic run this
is enough to drop the large majority of rollouts in a step, so the async collector never fills one.

Worth noting top_p is already protected in practice: a harness that never sets it leaves it unset,
and model_dump(exclude_unset=True) drops unset fields, so the configured value survives the merge.
Only explicitly set values are affected.

Change

One new optional config field:

extra_body_override_keys: Optional[List[str]] = None

Keys listed here take precedence over an explicit request value:

policy_model:
  responses_api_models:
    vllm_model:
      extra_body:
        temperature: 1.0
        top_p: 1.0
      extra_body_override_keys: [temperature, top_p]

Applied at all three merge sites — the Responses-native path, the Chat-Completions path, and
_build_completion_body_from_chat_body (the /v1/completions backend). Covering only some of them
would give an operator the guarantee on one path and silently drop it on another, which is the exact
failure mode the field exists to prevent.

Blast radius

Defaults to None, so behaviour is unchanged unless the field is set. The merge is otherwise
untouched, and the configured extra_body dict is not mutated — it is shared across requests, and
there is a test for that specifically.

The VLLMModel subclasses (genrm_model, local_vllm_model, local_vllm_model_proxy) inherit the
field; it is optional, so none of them need changes.

Tests

Seven unit tests in TestMergeExtraBody, all of which fail against main and pass with the change:

  • request wins by default (the existing contract, pinned so this PR cannot regress it)
  • extra_body still supplies defaults for keys absent from the request
  • an override key wins over an explicit request value
  • override is scoped to listed keys only
  • an override key absent from extra_body injects nothing
  • the configured extra_body dict is not mutated
  • the /v1/completions path honours overrides too

Full vllm_model suite: 112 passed. ruff check / ruff format clean.

Alternatives considered

  1. Reverse the merge globally — breaks the documented "config supplies defaults" contract for
    every existing user.
  2. Hardcode ("temperature", "top_p") — works, but is arbitrary from the library's point of
    view and surprising for non-RL users.
  3. Fix the agent harnesses not to hardcode sampling params — the cleaner layering argument, and
    worth doing regardless. This is the defensive half: a server generating training data should be
    able to enforce its own sampling config without trusting every harness that calls it.

Questions for reviewers

  • Is extra_body_override_keys the right name and shape, or would a boolean
    extra_body_is_authoritative be preferable?
  • The override currently also applies to per-request metadata.extra_body overrides — whatever ends
    up in the merged extra_body wins for listed keys. Is that the behaviour you want?
  • CONTRIBUTING asks for an issue before features. Happy to move this to an issue first if you would
    rather discuss the shape before reviewing code.

…ve sampling

`extra_body` supplies defaults only: every merge site does `extra_body | body_dict`,
so a value set explicitly on the request always wins. That is a sensible default and
is documented as intentional, but it leaves an operator no way to make the server
authoritative for a sampling parameter.

That matters when the server exists to generate on-policy training data. Agent
harnesses routinely hardcode sampling parameters on internal or auxiliary calls --
a title generator or a context compressor pinning `temperature` for a deterministic
sub-task -- and those calls go through the same model server. The result is
off-policy samples attributed to the policy.

With NeMo-RL it is not even silent: the async vLLM worker asserts that a request's
`temperature`/`top_p` match the generation config, so the mismatched call 500s and,
once the client exhausts its retries, the whole trajectory is discarded.

Add `extra_body_override_keys`, an opt-in list of keys for which the configured value
wins even against an explicit request value. Defaults to `None`, so behaviour is
unchanged unless the field is set. Applied at all three merge sites -- the
Responses-native path, the Chat-Completions path, and the /v1/completions body builder
-- since covering only some of them would grant the guarantee on one path and silently
drop it on another.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com>
@github-actions github-actions Bot added the sla:triage-overdue Review assignment is over the one-business-day SLA label Jul 31, 2026
@ananthsub

Copy link
Copy Markdown
Contributor

Hi @GNroy , this PR has similarities with #2190 - could you confirm whether 2190 would meet your requirements?

@ananthsub ananthsub removed the sla:triage-overdue Review assignment is over the one-business-day SLA label Aug 3, 2026
@ananthsub
ananthsub self-requested a review August 3, 2026 15:24
@GNroy

GNroy commented Aug 3, 2026

Copy link
Copy Markdown
Author

Confirmed — #2190 meets our requirements. I'll close this in its favour.

One detail worth noting, since it differs from the case #2190 leads with: ours wasn't an absent parameter. The agent harness set temperature explicitly on auxiliary, non-rollout calls (context compression, title generation), so the outbound body carried the harness's value instead of the trainer's. sampling_overrides handles it because it replaces rather than only fills in — so the conflicting-parameter case is real in the wild, not just theoretical. top_k: -1 and top_p: 1.0 also match what we need.

Happy to close now, or when #2190 lands — whichever you prefer.

@sephmard sephmard added the community-request Issue reported or requested by someone from the community label Aug 3, 2026
@github-actions github-actions Bot added the sla:review-overdue Review response is over the one-business-day SLA label Aug 4, 2026
cmunley1 added a commit that referenced this pull request Aug 17, 2026
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.

1. Some harnesses built for interactive serving generally send no
sampling fields at all, so the outbound body carries no sampling keys
and the engine applies a default of its own rather than the configured
one.
- On a backend that validates the request, this fails outright.
- On one that does not, nothing errors and the run trains on samples
drawn from a distribution the policy is not being optimized under.

2. Alternatively, a harness could send its own value: a harness that
hardcodes `temperature` on an auxiliary call such as a title generator
or a context compressor. That value is not the configured one either.

`top_k` is a third case, since the Anthropic converter never maps it, so
whatever arrives is always a default.

## Where it hooks in

```mermaid
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_completions` branches to `_chat_completions_via_completions_api`
before 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_body` merged 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
through `extra_body` today would mean moving those values to
`sampling_overrides` to 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.

```yaml
policy_model:
  responses_api_models:
    vllm_model:
      sampling_overrides:
        temperature: 1.0
        top_p: 1.0
```

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_params` on 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::TestSamplingOverrides`
covers 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 over
`extra_body` on that path.

The pin runs after the `extra_body` merge 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:

- The `sampling_overrides` block is no longer added to
`vllm_model_for_training.yaml`. That file is overlaid by every training
run, including native-agent runs whose sampling is already correct.
- The per-parameter interpolation keys are gone. They resolved through
defaults that happened to equal the value every config already used, so
no run could tell a wired pin from an unwired one.
- A startup log states which profile the server will enforce, or that it
will not pin.

The diff is now two files: `vllm_model/app.py` and 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.

Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
@ananthsub

Copy link
Copy Markdown
Contributor

closing out this PR in favor of #2190 which merged

@ananthsub ananthsub closed this Aug 17, 2026
@nemo-automation-bot nemo-automation-bot Bot removed the community-request Issue reported or requested by someone from the community label Aug 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sla:review-overdue Review response is over the one-business-day SLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants