fix: detect rollouts truncated by the per-turn generation cap - #14
Open
albcui wants to merge 2 commits into
Open
fix: detect rollouts truncated by the per-turn generation cap#14albcui wants to merge 2 commits into
albcui wants to merge 2 commits into
Conversation
hit_max_tokens feeds final_batch["truncated"], which grpo.overlong_filtering uses to drop a sample from the loss, but it only tested whether the conversation had filled the context (vllm_cfg.max_model_len). A rollout can also be stopped by policy.generation.max_new_tokens, which NeMo-Gym agents apply per turn, and a run with a small max_new_tokens never fills the context at all -- so every one of its truncated samples kept the near-zero reward that overlong filtering exists to discard. Check both budgets, and compare with >= rather than ==: the message log is post-processed before it reaches here (reasoning content is re-wrapped in thinking tags, for one), so an exact match can be thrown off by a single token. truncation_rate and natural_termination_rate derive from the same flag, so runs whose generation cap bound below the context will now report higher truncation than before. The earlier numbers were under-counting. Bump the Gym submodule for the matching change: verifiers_agent now honors responses_create_params.max_output_tokens, so max_new_tokens actually reaches the agent. The two belong together -- without the Gym half nothing is capped below the context, and without this half the newly honored cap goes undetected. Signed-off-by: Albert Cui <albcui@nvidia.com>
The previous commit inferred truncation from token counts because the Gym path appeared to have no better signal. It does: verifiers_agent now reports is_truncated, derived from the model server's finish_reason == "length" -- the same ground truth the native generation path already uses (vllm_worker_async.py, "is_truncated = generation_details.finish_reason == 'length'"), and the semantic docs/guides/grpo.md describes for overlong filtering. Prefer it. It needs no knowledge of which budget was binding, so it covers context exhaustion, an environment's own per-turn cap and max_new_tokens alike, and it does not mistake a turn that emitted EOS on its final allowed token for one that ran out of budget. This also converges the NeMo-Gym path with the native path rather than maintaining a second, weaker notion of truncation. Keep the length comparison as a fallback: agents other than verifiers_agent report no flag, and omitting the field selects the previous behaviour. Bumps the Gym submodule for the reporting change. Signed-off-by: Albert Cui <albcui@nvidia.com>
anubhutivyas
approved these changes
Aug 21, 2026
|
This PR is stale because it has been open for 14 days with no activity. Remove stale label or comment or update or this will be closed in 7 days. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
hit_max_tokens feeds final_batch["truncated"], which grpo.overlong_filtering uses to drop a sample from the loss, but it only tested whether the conversation had filled the context (vllm_cfg.max_model_len). A rollout can also be stopped by policy.generation.max_new_tokens, which NeMo-Gym agents apply per turn, and a run with a small max_new_tokens never fills the context at all -- so every one of its truncated samples kept the near-zero reward that overlong filtering exists to discard.
Check both budgets, and compare with >= rather than ==: the message log is post-processed before it reaches here (reasoning content is re-wrapped in thinking tags, for one), so an exact match can be thrown off by a single token.
truncation_rate and natural_termination_rate derive from the same flag, so runs whose generation cap bound below the context will now report higher truncation than before. The earlier numbers were under-counting.
Bump the Gym submodule for the matching change: verifiers_agent now honors responses_create_params.max_output_tokens, so max_new_tokens actually reaches the agent. The two belong together -- without the Gym half nothing is capped below the context, and without this half the newly honored cap goes undetected.
What does this PR do ?
Add a one line overview of what this PR aims to accomplish.
Issues
List issues that this PR closes (syntax):
Usage
# Add a code snippet demonstrating how to use thisBefore your PR is "Ready for review"
Pre checks:
Additional Information