NVIDIA-NeMo / NVIDIA-NeMo/RL

[Dataplane Sync RL] cleanup tracking issue

Open
#3,336 1 comment 0 reactions 1 assignee Claimed by @ZhiyuLi-Nvidia View on GitHub
Feature
Dominant language
Python
Stars
2k
Forks
561
Avg merge
4d 5h
Merged PRs (30d)
145

Description

Tracking issue for loose ends in the Single Controller **sync dataplane** implementation (`grpo_sync.py`), companion to the async/SC tracking issue #2625.
Items will be checked off or split into separate issues as they land.

### Known missing items

- [ ] **Port `grpo.use_generation_logprobs_as_prev` (#3295) to `grpo_sync.py`.** #3295 lets the legacy sync trainer reuse the generation engine's processed logprobs as `prev_logprobs` ([grpo.py reuse branch](https://github.com/NVIDIA-NeMo/RL/blob/a2c85bae8f66f7e7b4fbc93e8e0bc43f8fd1ba55/nemo_rl/algorithms/grpo.py#L2998)), skipping one full-batch trainer forward per step (logprob time −51%, step time −20% on Qwen3-30B-A3B); only valid on-policy with `use_importance_sampling_correction=false` + `force_on_policy_ratio=false` (guarded by [`_validate_generation_logprobs_as_prev`](https://github.com/NVIDIA-NeMo/RL/blob/a2c85bae8f66f7e7b4fbc93e8e0bc43f8fd1ba55/nemo_rl/algorithms/grpo.py#L352); cf. verl [`bypass_mode`](https://github.com/volcengine/verl/blob/2b47a68b66d6fa21884990a5d4445ceba59c47e5/verl/trainer/ppo/core_algos.py#L2374), slime [`use_rollout_logprobs`](https://github.com/THUDM/slime/blob/aaf5c2092b01219fa0d5c2d323741d409086ca32/slime/backends/megatron_utils/loss.py#L912)). `grpo_sync.py` always pays the forward — its logprob phase fills `prev_logprobs` via [`policy.get_logprobs_from_meta`](https://github.com/NVIDIA-NeMo/RL/blob/a2c85bae8f66f7e7b4fbc93e8e0bc43f8fd1ba55/nemo_rl/algorithms/grpo_sync.py#L787-L830); with the flag it could copy `generation_logprobs` instead under the same guard conditions. Note this feature is sync-only by nature (async requires IS correction on, which the flag forbids — the IS weight π_current/π_stale *is* the staleness correction), so it does NOT apply to the async path tracked in #2625. Reuse also makes `seq_logprob_error` masking and the gen/train mismatch diagnostics (`gen_kl_error`, `token_mult_prob_error`) degenerate. (Flagged during #3295 review)

- [x] fix force_on_policy_ratio in TQ trainer: see https://github.com/NVIDIA-NeMo/RL/pull/3087

- [ ] **Port grouped validation (`grpo.val_num_generations_per_prompt` / `pass_k`, #3401) to `validate_sync`.** #3401 lets validation run each prompt k times — [`validate()` repeats the batch](https://github.com/NVIDIA-NeMo/RL/blob/d2089ab05490f1d0ff9bdc228e511cef8743641d/nemo_rl/algorithms/grpo.py#L3722) with `repeat_interleave(k)` and [reports a `pass_k` metric](https://github.com/NVIDIA-NeMo/RL/blob/d2089ab05490f1d0ff9bdc228e511cef8743641d/nemo_rl/algorithms/grpo.py#L3829) alongside the unchanged `accuracy`. `grpo_sync.py` validates through its own [`validate_sync`](https://github.com/NVIDIA-NeMo/RL/blob/414b9bb6b69539011e835ce369be8e53aad709bd/nemo_rl/algorithms/grpo_sync.py#L236) (called at [L518](https://github.com/NVIDIA-NeMo/RL/blob/414b9bb6b69539011e835ce369be8e53aad709bd/nemo_rl/algorithms/grpo_sync.py#L518) and [L1022](https://github.com/NVIDIA-NeMo/RL/blob/414b9bb6b69539011e835ce369be8e53aad709bd/nemo_rl/algorithms/grpo_sync.py#L1022)), which has neither, so with `data_plane.enabled: true` the key is accepted and **silently ignored**: validation still runs k=1 and `pass_k` never appears. If the user also sets `stop_at_validation_metric: pass_k`, they get the opaque [`'pass_k' is not a reported validation metric`](https://github.com/NVIDIA-NeMo/RL/blob/d2089ab05490f1d0ff9bdc228e511cef8743641d/nemo_rl/algorithms/grpo.py#L2584) at the first validation with nothing pointing at the cause. Note the *sampling* half of #3401 is already safe here — `examples/run_grpo.py` calls `grpo.setup()` before dispatching to `grpo_train_sync`, so the val-sampling guard does fire; only the grouped-k half is missing. Sync-dataplane-only: the async path calls the updated `grpo.validate()` directly and is unaffected, so this does not belong in #2625. Cheap interim mitigation if the port lags: assert `val_num_generations_per_prompt == 1` in `grpo.setup()` when `data_plane.enabled` is true, so the unsupported combination fails at startup instead of silently doing nothing. (Flagged during #3401 review)

Add anything else here as it comes up.

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.