Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 119 additions & 21 deletions responses_api_models/vllm_model/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,11 @@ def model_post_init(self, context):
class VLLMModel(SimpleResponsesAPIModel):
config: VLLMModelConfig

_DYNAMO_ENGINE_DATA_FIELD_MAPPING: ClassVar[Dict[str, str]] = {
"prompt_token_ids": "prompt_token_ids",
"completion_token_ids": "generation_token_ids",
"completion_logprobs": "generation_log_probs",
}
_TOKENIZE_CHAT_FIELDS: ClassVar[tuple[str, ...]] = (
"model",
"messages",
Expand Down Expand Up @@ -697,6 +702,8 @@ def _preprocess_chat_completion_create_params(self, request: Request, body_dict:
# No user message found — create one with just the audio blocks.
body_dict.setdefault("messages", []).append({"role": "user", "content": list(audio_blocks)})

if self.config.return_token_id_information and self._requests_dynamo_engine_data(body_dict):
self._derive_required_prefix_token_ids(body_dict)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3. Blocking — required_prefix_token_ids is now auto-derived and forwarded for every server with return_token_id_information=true, with no config gate.

if self.config.return_token_id_information:
self._derive_required_prefix_token_ids(body_dict)
self._apply_sampling_overrides(body_dict)
self._validate_single_choice_token_request(body_dict)

Compare the sibling vLLM-specific token flag, which is gated:

if self.config.request_prompt_and_generation_token_ids:
    body_dict["return_token_ids"] = True

(app.py#L487-L488)

The derived value flows into create_chat_completion(**body_dict) and, via _TOKENIZE_CHAT_FIELDS (app.py#L237-L244), into /tokenize as well. Before this PR the field was opt-in through extra_body only — see #1558, which added it purely to "keep fallback tokenization aligned with the generation prompt".

The derivation assumes the previous turn's prompt_token_ids + generation_token_ids is a byte-prefix of the newly rendered prompt. Two common cases where it is not:

  • Reasoning models. With uses_reasoning_parser=true, turn 1's generation_token_ids include the <think>...</think> tokens, but on turn 2 app.py#L491-L523 moves reasoning into reasoning_content and most templates drop historical reasoning entirely.
  • Tool calls. vLLM re-serializes tool_calls from parsed JSON, not the original generated bytes, so whitespace and key order drift.

In both cases an engine that enforces the prefix 400s the request, and the previously-working /tokenize fallback breaks too. Recommend gating this behind a config flag rather than applying it to all capture-mode servers.

self._apply_sampling_overrides(body_dict)
self._validate_single_choice_token_request(body_dict)
if self._external_capture_enabled:
Expand Down Expand Up @@ -978,25 +985,38 @@ async def chat_completions(

# Token metadata uses this source order:
# 1. A complete bundle on the assistant message.
# 2. Prompt IDs at the response top level and generation IDs on the choice.
# 3. Generation data from choice logprobs and prompt IDs from `/tokenize`.
# 2. A complete bundle in Dynamo engine data.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

14. CLAUDE.md — validation is unit-tests-only, with the real Dynamo smoke deferred until after merge.

CLAUDE.md#L14: "For environment or agent changes: run real rollouts with a model and inspect agent and verifier behavior. Green unit tests alone are not enough."

The PR's Validation section lists only pytest responses_api_models/vllm_model/tests/test_app.py and pre-commit, and states: "The final managed Dynamo smoke will run from NeMo-RL after this PR merges and NeMo-RL pins the exact containing Gym main commit."

Every nvext.engine_data payload shape in the tests is hand-authored, so nothing confirms the real field names, the nesting level (top-level vs. per-choice — this code only reads top-level chat_completion_dict["nvext"]), or the element type of completion_logprobs. _require_log_prob_list only checks that the value is a list, so a list of per-token logprob objects would pass validation here and surface later as a pydantic 500 rather than the clean rejection the PR promises.

# 3. Prompt IDs at the response top level and generation IDs on the choice.
# 4. Generation data from choice logprobs and prompt IDs from `/tokenize`.
#
# An earlier source supplies the normalized bundle.
# Later inline sources are still checked when present.
# A partially present source is invalid.
# Duplicate token IDs must agree.
message_bundle = self._extract_message_token_bundle(message_dict)
response_token_ids = self._extract_vllm_response_token_ids(chat_completion_dict, choice_dict)

if message_bundle is not None:
dynamo_bundle = self._extract_dynamo_engine_data(chat_completion_dict)

if message_bundle is not None and dynamo_bundle is not None:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2. Blocking — the message-vs-Dynamo cross-check ignores generation_log_probs, and the message bundle silently wins.

if message_bundle is not None and dynamo_bundle is not None:
if (
message_bundle["prompt_token_ids"] != dynamo_bundle["prompt_token_ids"]
or message_bundle["generation_token_ids"] != dynamo_bundle["generation_token_ids"]
):
raise RuntimeError("Message-level token metadata disagrees with Dynamo engine data.")
native_bundle = message_bundle if message_bundle is not None else dynamo_bundle
if native_bundle is not None:

The equality check covers only prompt_token_ids and generation_token_ids. If the two sources agree on token IDs but disagree on log probabilities, no error is raised and line 771 picks message_bundle, discarding Dynamo's values.

Failure case: a NeMo-RL wrapper copies correct token IDs onto choice.message but stale or recomputed generation_log_probs (previous engine step, different sampling config), while nvext.engine_data.completion_logprobs holds the true values. Both bundles pass the ID check, the message bundle wins, and Gym emits the wrong per-token logprobs. Those drive the importance-sampling ratio in RL, so training silently consumes a corrupted signal with no error anywhere.

This also contradicts the PR description's own statement that native Dynamo data "remains authoritative". Either extend the comparison to generation_log_probs, or make dynamo_bundle win for that field.

if any(message_bundle[field] != dynamo_bundle[field] for field in REQUIRED_TOKEN_METADATA_FIELDS):
raise RuntimeError("Message-level token metadata disagrees with Dynamo engine data.")

native_bundle = message_bundle if message_bundle is not None else dynamo_bundle

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

responses_api_models/vllm_model/app.py:772

1 action item.

TL;DR — this PR asks Dynamo for nvext.engine_data but never checks it came back; when it's absent the code silently falls through to a /tokenize call that the NeMo-RL wrapper does not serve, turning a one-line diagnostic into a bare 404.

Raised here rather than on NeMo-RL #3763 because the fix lands in this file. #3763 deletes _validate_engine_data, which raised "Dynamo response did not include nvext.engine_data." — a clean 502 naming the field. _extract_dynamo_engine_data returns None in that case instead, so the check disappears rather than moving.

Why the fallthrough terminates at a 404 rather than at an earlier, friendlier error — I checked this against the exact pinned Dynamo (NeMo-RL pins ai-dynamo[vllm]==1.3.0.post1, asserted at grpo_dynamo.sh:32; the 1.3.0 workspace tree is what's cited below):

  1. native_bundle is None, so the else at line 787 runs.
  2. _extract_vllm_response_token_ids already returned None — Dynamo's chat path emits token IDs only under nvext (chat_completions/delta.rs:341-343) and there is no top-level prompt_token_ids anywhere in its chat_completions module (grep → 0 hits). choice.token_ids would need vLLM's return_token_ids: true, which this app only sends when request_prompt_and_generation_token_ids is set — and vllm_model_for_training.yaml sets it false.
  3. So _extract_choice_logprobs does not save us either: Dynamo honors return_tokens_as_token_ids (chat_completions.rs:123) and emits format!("token_id:{}", tid) (delta.rs:122), and this app pins return_tokens_as_token_ids=True — so the logprobs parse cleanly and execution reaches...
  4. ...await client.create_tokenize(...) at line 796, which POSTs /tokenize. The NeMo-RL wrapper registers only /health and /v1/chat/completions. 404.

Both conditions are structural properties of Dynamo's chat protocol, not a coincidence — _extract_vllm_response_token_ids returns None for every Dynamo chat response by construction.

Not hypothetical: docs/guides/dynamo-generation.md:65-67 in NeMo-RL already documents that "Dynamo 1.3.0's legacy tool jail removes nvext.engine_data" for tool_choice=auto.

Action: assert the contract where you already know it was requested. body_dict is in scope here (used at line 796), and _requests_dynamo_engine_data already encodes "the caller asked for this field". Putting it here rather than inside _extract_dynamo_engine_data keeps that helper a pure extractor. This blocks nothing legitimate — it fires only when the request itself put "engine_data" in nvext.extra_fields, i.e. exactly when the response is contractually required to carry it.

Suggested change
native_bundle = message_bundle if message_bundle is not None else dynamo_bundle
native_bundle = message_bundle if message_bundle is not None else dynamo_bundle
if native_bundle is None and self._requests_dynamo_engine_data(body_dict):
raise RuntimeError(
"Request asked for `nvext.engine_data` but the response did not include it."
)

if native_bundle is None and self._requests_dynamo_engine_data(body_dict):
raise RuntimeError("Request asked for `nvext.engine_data` but the response did not include it.")
response_token_ids = self._extract_vllm_response_token_ids(
chat_completion_dict,
choice_dict,
ignore_partial=native_bundle is not None,
)
if native_bundle is not None:
if response_token_ids is not None:
response_prompt_token_ids, response_generation_token_ids = response_token_ids
if (
message_bundle["prompt_token_ids"] != response_prompt_token_ids
or message_bundle["generation_token_ids"] != response_generation_token_ids
native_bundle["prompt_token_ids"] != response_prompt_token_ids
or native_bundle["generation_token_ids"] != response_generation_token_ids
):
raise RuntimeError("Message-level token metadata disagrees with vLLM response token IDs.")
message_dict.update(message_bundle)
raise RuntimeError("Native token metadata disagrees with vLLM response token IDs.")
message_dict.update(native_bundle)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1. Blocking — nvext is never stripped from the response, so the full Dynamo token payload is echoed back twice.

This comment is about line 817, which falls outside the diff hunks; anchoring to the nearest changed line in the same function.

# Top-level and choice-level token-ID fields are transport details.
chat_completion_dict.pop("prompt_token_ids", None)
choice_dict.pop("token_ids", None)
choice_dict["message"] = NeMoGymChatCompletionMessageForTraining.model_validate(message_dict)

Lines 817-818 deliberately strip the transport-only token fields:

chat_completion_dict.pop("prompt_token_ids", None)
choice_dict.pop("token_ids", None)

but nvext — read at line 879 and carrying exactly the same data — is left in place. NeMoGymChatCompletion extends openai's ChatCompletion, whose base model is extra="allow", so extras survive validation and FastAPI serialization. The existing assertion at test_app.py#L4825 (assert "prompt_token_ids" not in data) only makes sense because extras survive — which is precisely why those two pop calls exist.

Concretely, for a response with nvext.engine_data = {"prompt_token_ids": [...8k ints...], "completion_token_ids": [...2k ints...], "completion_logprobs": [...2k floats...]}, the returned JSON now carries the whole bundle twice: once normalized onto choices[0].message, once raw under nvext.engine_data. Every stored rollout record roughly doubles in token-data size, and transport internals the code hides everywhere else are re-exposed to downstream consumers.

Suggested fix: chat_completion_dict.pop("nvext", None) alongside the existing pops (or strip just engine_data if other nvext fields are meant to survive).

else:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5. On the native path Gym returns its own injected token_id:<int> pseudo-tokens to callers, and a new test locks that in.

This comment is about line 814, which falls outside the diff hunks; anchoring to the nearest changed line (the else that guards it).

# The adapter consumed this compatibility payload.
choice_dict.pop("logprobs", None)
# Top-level and choice-level token-ID fields are transport details.

choice_dict.pop("logprobs", None) lives only in the else branch. The capture path force-sets logprobs=True, top_logprobs=0, return_tokens_as_token_ids=True (app.py#L478-L486), so with a Dynamo bundle present native_bundle is not None, the pop is skipped, and the response ships:

"logprobs": {"content": [{"token": "token_id:123", "logprob": -9.1, ...}]}

Any consumer reading logprobs.content[*].token expecting decoded text (log viewers, per-token attribution, external eval tooling) gets a Gym-internal debug encoding. This was already true for the message-bundle path, but the new test_capture_path_uses_dynamo_logprobs_without_consuming_choice_logprobs asserts the leaked payload is preserved verbatim, cementing it as intended behaviour rather than fixing it.

logprob_token_ids, generation_log_probs = self._extract_choice_logprobs(choice_dict)
if response_token_ids is not None:
Expand Down Expand Up @@ -1029,12 +1049,15 @@ async def chat_completions(
)
)

# The adapter consumed this compatibility payload.
choice_dict.pop("logprobs", None)

# Top-level and choice-level token-ID fields are transport details.
# These fields only transport the token bundle to Gym.
choice_dict.pop("logprobs", None)
chat_completion_dict.pop("prompt_token_ids", None)
choice_dict.pop("token_ids", None)
nvext = chat_completion_dict.get("nvext")
if isinstance(nvext, dict):
nvext.pop("engine_data", None)
if not nvext:
chat_completion_dict.pop("nvext", None)
choice_dict["message"] = NeMoGymChatCompletionMessageForTraining.model_validate(message_dict)

return NeMoGymChatCompletion.model_validate(chat_completion_dict)
Expand Down Expand Up @@ -1177,41 +1200,63 @@ def _strip_capture_transport_fields(payload: Dict[str, Any]) -> None:

@staticmethod
def _require_token_id_list(value: Any, field_name: str) -> List[Any]:
"""Check the container without scanning or copying token IDs."""
"""Validate a JSON token-ID list without copying it."""
if not isinstance(value, list):
raise RuntimeError(f"`{field_name}` must be a list of integer token IDs.")
for index, token_id in enumerate(value):
if not isinstance(token_id, int) or isinstance(token_id, bool):
raise RuntimeError(f"`{field_name}[{index}]` must be an integer token ID.")
return value

@staticmethod
def _require_log_prob_list(value: Any, field_name: str) -> List[Any]:
"""Check the container without scanning or copying log probabilities."""
"""Validate a JSON log-probability list without copying it."""
if not isinstance(value, list):
raise RuntimeError(f"`{field_name}` must be a list of numeric log probabilities.")
for index, log_prob in enumerate(value):
if not isinstance(log_prob, (int, float)) or isinstance(log_prob, bool):
raise RuntimeError(f"`{field_name}[{index}]` must be a numeric log probability.")
return value

@classmethod
def _validate_token_bundle(cls, bundle: Dict[str, Any], source: str) -> Dict[str, Any]:
def _validate_token_bundle(
cls,
bundle: Dict[str, Any],
source: str,
source_field_names: Optional[Dict[str, str]] = None,
) -> Dict[str, Any]:
source_field_names = source_field_names or {}
present_fields = TOKEN_METADATA_FIELDS.intersection(bundle)
missing_fields = REQUIRED_TOKEN_METADATA_FIELDS.difference(present_fields)
if missing_fields:
missing = ", ".join(sorted(missing_fields))
missing = ", ".join(sorted(source_field_names.get(field, field) for field in missing_fields))
raise RuntimeError(f"{source} returned partial token metadata; missing: {missing}.")

def source_field(field: str) -> str:
return f"{source}.{source_field_names.get(field, field)}"

normalized = {
"prompt_token_ids": cls._require_token_id_list(bundle["prompt_token_ids"], f"{source}.prompt_token_ids"),
"prompt_token_ids": cls._require_token_id_list(
bundle["prompt_token_ids"], source_field("prompt_token_ids")
),
"generation_token_ids": cls._require_token_id_list(
bundle["generation_token_ids"], f"{source}.generation_token_ids"
bundle["generation_token_ids"], source_field("generation_token_ids")
),
"generation_log_probs": cls._require_log_prob_list(
bundle["generation_log_probs"], f"{source}.generation_log_probs"
bundle["generation_log_probs"], source_field("generation_log_probs")
),
}
if "routed_experts" in bundle:
normalized["routed_experts"] = bundle["routed_experts"]

if len(normalized["generation_token_ids"]) != len(normalized["generation_log_probs"]):
mismatched_fields = (
f"{source_field_names['generation_token_ids']} and {source_field_names['generation_log_probs']}"
if source_field_names
else "generation token IDs and log probabilities"
)
raise RuntimeError(
f"{source} returned mismatched generation token IDs and log probabilities: "
f"{source} returned mismatched {mismatched_fields}: "
f"{len(normalized['generation_token_ids'])} token IDs and "
f"{len(normalized['generation_log_probs'])} log probabilities."
)
Expand All @@ -1229,18 +1274,46 @@ def _extract_message_token_bundle(cls, message_dict: Dict[str, Any]) -> Optional
"choice.message",
)

@classmethod
def _extract_dynamo_engine_data(cls, chat_completion_dict: Dict[str, Any]) -> Optional[Dict[str, Any]]:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9. Altitude — Gym never requests engine_data, so this path is dead code in any Gym-only deployment.

@classmethod
def _extract_dynamo_engine_data(cls, chat_completion_dict: Dict[str, Any]) -> Optional[Dict[str, Any]]:
nvext = chat_completion_dict.get("nvext")
if nvext is None:

Point vllm_model (with return_token_id_information: true) directly at a Dynamo endpoint: Gym sends logprobs / top_logprobs / return_tokens_as_token_ids and, when configured, return_token_ids — but never nvext: {"engine_data": ...}. Dynamo returns no engine_data, this returns None, and the code falls back to the /tokenize path the PR describes as "unsupported".

Combined with "No public schema or configuration flag is added" from the PR description, there is no supported way for a Gym user to turn this on; it only activates behind the NeMo-RL wrapper whose duplication the PR aims to remove. The fix looks like it sits one layer too shallow — consuming the bundle without the matching request-side knob.

nvext = chat_completion_dict.get("nvext")
if nvext is None:
return None
if not isinstance(nvext, dict):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

11. Three new branches added by this PR have no test coverage.

if nvext is None:
return None
if not isinstance(nvext, dict):
raise RuntimeError("`nvext` must be an object when present.")
if "engine_data" not in nvext:
return None

  • Line 883, RuntimeError("nvext must be an object when present.")test_capture_path_rejects_malformed_dynamo_engine_data only parametrizes engine_data=[], never a non-dict nvext.
  • Lines 884-885, nvext present without an engine_data key returning None. This is the common case for any Dynamo response carrying other nvext fields, and a regression here would break every request.
  • Lines 972-975, the ValueError in _derive_required_prefix_token_ids.

CLAUDE.md#L140: "Test coverage must be >= 96%".

raise RuntimeError("`nvext` must be an object when present.")
if "engine_data" not in nvext:
return None

engine_data = nvext["engine_data"]
if not isinstance(engine_data, dict):
raise RuntimeError("`nvext.engine_data` must be an object when present.")

bundle = {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8. Malformed-engine_data errors name Gym destination fields that do not exist in a Dynamo payload.

field_mapping = {
"prompt_token_ids": "prompt_token_ids",
"completion_token_ids": "generation_token_ids",
"completion_logprobs": "generation_log_probs",
}
bundle = {
destination: engine_data[source] for source, destination in field_mapping.items() if source in engine_data
}
return cls._validate_token_bundle(bundle, "nvext.engine_data")

The bundle is renamed to Gym's field names before _validate_token_bundle sees it, and the source label is "nvext.engine_data". So engine_data = {"prompt_token_ids": [...], "completion_logprobs": [...]} produces:

nvext.engine_data returned partial token metadata; missing: generation_token_ids.

But generation_token_ids is not a Dynamo field name — the actually-missing key is completion_token_ids. Same for the mismatch error, which reports generation_token_ids and generation_log_probs counts. An operator grepping the Dynamo response or its schema for the named field finds nothing. Worth mapping back to source names in the error text.

destination: engine_data[source]
for source, destination in cls._DYNAMO_ENGINE_DATA_FIELD_MAPPING.items()
if source in engine_data
}
source_field_names = {
destination: source for source, destination in cls._DYNAMO_ENGINE_DATA_FIELD_MAPPING.items()
}
return cls._validate_token_bundle(bundle, "nvext.engine_data", source_field_names)

@classmethod
def _extract_vllm_response_token_ids(
cls,
chat_completion_dict: Dict[str, Any],
choice_dict: Dict[str, Any],
*,
ignore_partial: bool = False,
) -> Optional[tuple[List[Any], List[Any]]]:
prompt_value = chat_completion_dict.get("prompt_token_ids")
generation_value = choice_dict.get("token_ids")
prompt_present = prompt_value is not None
generation_present = generation_value is not None

if prompt_present != generation_present:
if ignore_partial:
return None
missing = "choice.token_ids" if prompt_present else "prompt_token_ids"
raise RuntimeError(f"vLLM response returned partial token metadata; missing: {missing}.")
if not prompt_present:
Expand Down Expand Up @@ -1288,6 +1361,31 @@ def _get_tokenize_chat_body(cls, body_dict: Dict[str, Any]) -> Dict[str, Any]:
"""Keep every known prompt-affecting field aligned with generation."""
return {field: body_dict[field] for field in cls._TOKENIZE_CHAT_FIELDS if field in body_dict}

@staticmethod
def _requests_dynamo_engine_data(body_dict: Dict[str, Any]) -> bool:
nvext = body_dict.get("nvext")
if not isinstance(nvext, dict):
return False
extra_fields = nvext.get("extra_fields")
return isinstance(extra_fields, list) and "engine_data" in extra_fields

@classmethod
def _derive_required_prefix_token_ids(cls, body_dict: Dict[str, Any]) -> None:
if body_dict.get("required_prefix_token_ids") is not None:
return

for message in reversed(body_dict.get("messages", [])):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7. When the newest assistant message has no token metadata, the loop silently falls back to an older turn's prefix.

for message in reversed(body_dict.get("messages", [])):
if not isinstance(message, dict) or message.get("role") != "assistant":
continue
prompt_token_ids = message.get("prompt_token_ids")
generation_token_ids = message.get("generation_token_ids")
if prompt_token_ids is None and generation_token_ids is None:
continue
if not isinstance(prompt_token_ids, list) or not isinstance(generation_token_ids, list):

The continue at line 971 skips assistant messages without token metadata and keeps walking backwards, so the derived prefix can come from a turn several exchanges back.

That is fine when history is strictly append-only, but not otherwise: an agent harness that injects a synthetic assistant turn, or any flow that edits, truncates, or regenerates an intervening message (best-of-n branching, tool-output rewrites), leaves those older IDs no longer a prefix of the current prompt. The mismatch is then asserted by the engine, far from its cause here. Consider bailing out (returning without setting the field) as soon as the most recent assistant message lacks metadata, rather than reaching further back.

if not isinstance(message, dict) or message.get("role") != "assistant":
continue
token_bundle = cls._extract_message_token_bundle(message)
if token_bundle is None:
continue
body_dict["required_prefix_token_ids"] = [
*token_bundle["prompt_token_ids"],
*token_bundle["generation_token_ids"],
]
return

def _validate_single_choice_token_request(self, body_dict: Dict[str, Any]) -> None:
context = current_capture_context()
external_capture = context is not None and context.external_staging
Expand Down
Loading
Loading