NVIDIA-NeMo / NVIDIA-NeMo/RL

Multi-turn rollout: a max_tokens-truncated response ends the trajectory on the async path but not the sync one

Open
#3,851 0 comments 0 reactions 1 assignee Claimed by @terrykong View on GitHub
bug community-request waiting-on-maintainers
Dominant language
Python
Stars
2k
Forks
561
Avg merge
4d 5h
Merged PRs (30d)
145

Description

## What

`policy.generation.vllm_cfg.async_engine` is meant to be a performance switch, but on the multi-turn path it changes the trajectory: a response that hits `max_tokens` without a stop token ends the rollout on the async path and does not on the sync-batched one.

## Where

Both live in `nemo_rl/experience/rollouts.py`, and both compute the same signal.

**Async — `run_sample_multi_turn_rollout`.** The generation metric sets a flag:

```python
# rollouts.py:1297
if response_truncated is not None and response_truncated[0]:
truncated = True
```

and the turn loop guards on it:

```python
# rollouts.py:1266
if terminated or truncated:
break
```

**Sync batched — `run_multi_turn_rollout`.** The same signal is recorded:

```python
# rollouts.py:979
if response_truncated[i]:
sample_truncated[global_idx] = True
```

but `sample_truncated` is only ever read for metrics. What ends the loop is:

```python
# rollouts.py:1062
done = truncation_mask | terminateds
```

and `truncation_mask` is freshly zeroed each turn and set in exactly one place — the branch that trims an environment observation because it would overflow `max_seq_len`. A response truncated by `max_tokens` never sets it, so the sample stays active and takes another turn.

## Why it matters

Same config, same seed, different `async_engine` — different trajectory length, different `turn_count`, different `max_turns_reached`, and a different set of tokens in the training batch. `should_use_async_rollouts` picks the path from the generation config alone (`grpo.py:3109`, `grpo.py:4027`, `distillation.py:827`, and the PPO equivalents), so nothing about the algorithm config says which semantics you get.

It also means "hit max_tokens" is load-bearing on one path and metrics-only on the other, which makes `sample_truncated` harder to reason about than it looks.

## What I'd like to know before proposing a fix

Which one is intended? I can see the argument both ways:

- **Truncation ends the trajectory** (async today). A response cut mid-thought has no parseable action, so the next environment step is being fed a fragment.
- **Truncation continues** (sync today). The environment may still return something useful, and ending early loses a turn the budget allowed.

Happy to send the patch plus a test either way once someone says which. I'd rather not pick the semantics unilaterally — it changes what gets trained on.

Found while reading both paths for unrelated work; verified on `main` at `cfd90812`.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.