Skip to content

feat(grpo): validation-only sampling params and grouped pass@k validation - #3401

Merged
terrykong merged 14 commits into
NVIDIA-NeMo:mainfrom
michal2409:split/validation-sampling
Aug 6, 2026
Merged

feat(grpo): validation-only sampling params and grouped pass@k validation#3401
terrykong merged 14 commits into
NVIDIA-NeMo:mainfrom
michal2409:split/validation-sampling

Conversation

@michal2409

@michal2409 michal2409 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Right now, validation measures the model exactly the way training explores it: one rollout per prompt, using the same sampling settings as training. For agentic RL that makes the validation accuracy a noisy number — a single sample at training temperature can pass or fail a prompt by luck.

This PR lets validation measure differently from how training explores:

  • policy.generation.val_temperature / val_top_p / val_top_k — validation-only sampling. The exemplar YAMLs default them to the train values via interpolation (val_temperature: ${.temperature}, ...), so validation samples exactly like training unless overridden (typically near-greedy).
  • grpo.val_num_generations_per_prompt — validation runs each prompt k times. accuracy stays the mean reward over all rollouts; when k > 1 a separate pass_k metric reports the share of prompts with at least one passing attempt.

How it works:

  • validate() builds an explicit SamplingParams dataclass from the val keys and passes it through the NeMo-Gym rollout entry points; training rollouts are untouched.
  • The vLLM async OpenAI server accepts exactly two sampling profiles: train and val (the on-policy guard for training is unchanged).
  • setup() rejects val sampling that differs from train sampling unless the backend is vLLM and the NeMo-Gym rollout path is used.

Together these turn validation accuracy into a stable, repeatable measurement — which matters whenever it drives a decision, such as accuracy-threshold stopping.

@michal2409
michal2409 requested review from a team as code owners July 29, 2026 11:06
@copy-pr-bot

copy-pr-bot Bot commented Jul 29, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@michal2409
michal2409 force-pushed the split/validation-sampling branch from b1104b4 to c068686 Compare July 29, 2026 11:28
…tion

Two opt-in validation-protocol additions for long-horizon agentic RL;
absent keys keep today's behavior byte-for-byte.

1) grpo.val_temperature / grpo.val_top_p: validation rollouts may use
   their own sampling (e.g. near-greedy) while training rollouts keep
   policy.generation untouched. validate() passes the overridden
   sampling to the NeMo-Gym rollout, which stamps it into every
   validation request. Because all requests get their sampling stamped
   from the caller's generation config, the params themselves identify
   validation traffic: the OpenAI-compatible server's on-policy
   sampling assert now accepts exactly two profiles — policy.generation
   and the resolved validation profile (published by setup() as the
   internal _validation_generation key, following the _pad_token_id
   internal-key pattern) — instead of one. Training requests are
   checked exactly as before, with a descriptive message on mismatch.

2) grpo.num_val_generations_per_prompt: validation generates k
   independent rollouts per prompt (repeat_interleave) and reports
   pass@k as accuracy. Absent or 1 keeps plain mean accuracy. Grouped
   near-greedy validation makes threshold metrics (e.g. target-accuracy
   stopping) far less noisy than one sample per prompt at training
   temperature.

Covered by a unit test pinning that grouped validation repeats prompts
contiguously and reports pass@k (0.5 where a plain mean would be
0.125).

Signed-off-by: Michal Futrega <mfutrega@nvidia.com>

@yuki-97 yuki-97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@michal2409 thanks for adding this valuable feature! left some comments.

Comment thread nemo_rl/algorithms/grpo.py Outdated
Comment thread nemo_rl/algorithms/grpo.py Outdated
Comment thread nemo_rl/models/generation/interfaces.py Outdated
Comment thread nemo_rl/algorithms/grpo.py Outdated
Comment thread nemo_rl/algorithms/grpo.py Outdated
Comment thread nemo_rl/algorithms/grpo.py Outdated
Comment thread nemo_rl/algorithms/grpo.py Outdated
Comment thread tests/unit/algorithms/test_grpo.py Outdated
Comment thread nemo_rl/algorithms/grpo.py Outdated
- Move val_temperature/val_top_p (plus new val_top_k) from GRPOConfig into
  GenerationConfig; exemplar YAMLs default them to the train values via
  ${.temperature}-style interpolation so validation samples like training
  unless overridden
- Replace the _validation_generation config side-channel with an explicit
  SamplingParams dataclass threaded through the NeMo-Gym rollout entry points
- Rename num_val_generations_per_prompt to val_num_generations_per_prompt and
  make it a required key
- Keep accuracy as the mean reward; report pass@k separately as pass_k when
  k > 1
- Reject validation sampling overrides outside the vLLM NeMo-Gym path at
  setup() time

Signed-off-by: Michal Futrega <mfutrega@nvidia.com>
michal2409 added a commit to michal2409/RL that referenced this pull request Jul 30, 2026
Adds two GRPOConfig keys, defaulted in the exemplar YAMLs:
- stop_at_validation_accuracy (float | null): end training once the chosen
  validation metric reaches this threshold; null (the default) disables
  early stopping.
- stop_at_validation_metric (str, default accuracy): which reported
  validation metric the early stop compares. Any key of the validation
  metrics dict is accepted; a metric that validation does not report fails
  loudly with the available keys (so e.g. pass_k composes with grouped
  validation once NVIDIA-NeMo#3401 lands, without a cross-PR dependency).

Motivation: train-to-accuracy runs (benchmarking, recipe qualification)
need to stop as soon as the quality bar is met rather than at a step count
they cannot know in advance; grouped-validation workloads measure that bar
on pass@k rather than mean accuracy, so the metric must be configurable.

Covered by test_training_stops_at_validation_accuracy_threshold (both
trainers), test_training_stops_on_configured_pass_k_metric, and
test_stop_metric_missing_from_validation_fails_loudly.

Signed-off-by: Michal Futrega <mfutrega@nvidia.com>
research/template_project/single_update.py validates the full pydantic
MasterConfig, so its config copy needs every required grpo key too
(same failure mode NVIDIA-NeMo#3400 hit in L1_Functional_Tests_Other_1).

Signed-off-by: Michal Futrega <mfutrega@nvidia.com>
@michal2409
michal2409 requested a review from terrykong as a code owner July 30, 2026 12:44
…ation-sampling

Signed-off-by: Michal Futrega <mfutrega@nvidia.com>

# Conflicts:
#	tests/unit/algorithms/test_grpo.py
@michal2409

Copy link
Copy Markdown
Contributor Author

/ok to test cd1e9e4

@yuki-97 yuki-97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thanks @michal2409 for the update, just one nit.

besides, maybe like the CI fail in #3404, will need to also update the configs under examples/nemo_gym/nemotron-3-ultra, main added some new recipes recently.

Comment thread nemo_rl/algorithms/grpo.py Outdated
@yuki-97

yuki-97 commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

@terrykong could you take a review as well?

…ation-sampling

Signed-off-by: Michal Futrega <mfutrega@nvidia.com>

# Conflicts:
#	nemo_rl/experience/rollouts.py
@michal2409 michal2409 added the CI:L1 Run doctests, unit tests, and functional tests label Jul 31, 2026
…ltra configs

Move the >= 1 assert from validate() to setup() so misconfiguration
fails at startup (review). Rename the pre-staged
num_val_generations_per_prompt key in the new nemotron-3-ultra configs
to the real key (keeping their value of 2) so their MasterConfig
contract test passes, and drop the mopd recipe line that now equals the
exemplar default (minimize-check).

Signed-off-by: Michal Futrega <mfutrega@nvidia.com>
@michal2409
michal2409 force-pushed the split/validation-sampling branch from 62de2ee to 38d141b Compare July 31, 2026 22:09
@michal2409

Copy link
Copy Markdown
Contributor Author

/ok to test 38d141b

max(train/critic/loss) lands at 6.68-7.00 in CI for unrelated PRs
(NVIDIA-NeMo#3401, NVIDIA-NeMo#3404, NVIDIA-NeMo#3423) since the vLLM 0.25.1 bump; the same critic-side
drift is already tracked in NVIDIA-NeMo#3412. Placeholder bump, like the grad_norm
bound raised in NVIDIA-NeMo#3280.

Signed-off-by: Michal Futrega <mfutrega@nvidia.com>
@michal2409

Copy link
Copy Markdown
Contributor Author

/ok to test 38d141b

michal2409 added a commit to michal2409/RL that referenced this pull request Aug 2, 2026
max(train/critic/loss) lands at 6.68-7.00 in CI for unrelated PRs
(NVIDIA-NeMo#3401, NVIDIA-NeMo#3404, NVIDIA-NeMo#3423) since the vLLM 0.25.1 bump; the same critic-side
drift is already tracked in NVIDIA-NeMo#3412. Placeholder bump, like the grad_norm
bound raised in NVIDIA-NeMo#3280.

Signed-off-by: Michal Futrega <mfutrega@nvidia.com>
@michal2409

Copy link
Copy Markdown
Contributor Author

/ok to test 061e8b7

…ation-sampling

# Conflicts:
#	examples/configs/grpo_math_1B.yaml
#	examples/nemo_gym/grpo_workplace_assistant_nemotron_nano_v2_9b.yaml
#	nemo_rl/algorithms/grpo.py
#	research/template_project/configs/grpo_math_1B.yaml
#	tests/unit/reference_configs/grpo_math_1B.yaml
@michal2409

Copy link
Copy Markdown
Contributor Author

/ok to test 47e3636

michal2409 added a commit to michal2409/RL that referenced this pull request Aug 3, 2026
…tion

Squash of NVIDIA-NeMo#3401 (head 47e3636) onto main @ 00d3fc2.

Signed-off-by: Michal Futrega <mfutrega@nvidia.com>
…ation-sampling

# Conflicts:
#	nemo_rl/algorithms/grpo.py
#	tests/unit/algorithms/test_grpo.py
@michal2409

Copy link
Copy Markdown
Contributor Author

/ok to test d2089ab

@michal2409

Copy link
Copy Markdown
Contributor Author

/ok to test 160b5be

@terrykong terrykong left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the structure for the val sampling params, per @yuki-97's request. Overall the shape looks right to me — threading an explicit SamplingParams through the rollout call instead of the _validation_generation side-channel, guarding at setup() rather than whitelisting per-request, and leaving accuracy alone while adding pass_k beside it are all the right calls. The flat val_ prefix also matches existing convention (val_period, val_batch_size, max_val_samples, val_at_start), so I wouldn't push for a nested block.

Two items I'd like resolved before merge, plus some smaller structural ones — all inline:

  1. The 7 standalone examples/nemo_gym/nemotron-3-ultra/*.yaml configs never got the new val_* generation keys, so they KeyError at startup. The gap @yuki-97 flagged on 7/31 is only half-closed — the grpo block was updated, the generation block wasn't.
  2. In vllm_worker_async.py, request_top_p = 1.0 if request.top_p is None else request.top_p normalizes the comparison but not the request, so a request omitting top_p now passes the on-policy guard and vLLM samples it from the model's generation_config.json. Previously that request was rejected.

Things I checked and found fine, so you don't need to re-verify them:

  • ${.temperature} interpolation resolves lazily after the defaults: deep-merge, so a child recipe overriding temperature gets the matching val_temperature.
  • Rollout ordering is preserved on both paths (_NemoGymStreamAccumulator restores by row_index; run_multi_turn_rollout scatters back via active_indices), so pass_k's view(-1, k) groups the right rollouts.
  • BatchedDataDict.repeat_interleave deep-copies list fields, so the k copies don't alias one message_log.
  • mock_grpo_components is function-scoped, so the new setup() test doesn't leak backend: megatron into its neighbours.

Filed #3497 separately for the engine-wide half of item 2 (vLLM resolving any unset sampling parameter from the model's generation_config.json) — that one predates this PR and isn't an ask here.

Reviewed at d2089ab; re-checked against 160b5be, which only touches model_utils.py, so every finding still applies unchanged. I couldn't run the test suite or linters locally (Linux-only lockfile), so nothing here is execution-verified.

Generated by Claude Code

Comment thread nemo_rl/algorithms/grpo.py
Comment thread nemo_rl/models/generation/interfaces.py Outdated
Comment thread nemo_rl/models/generation/interfaces.py Outdated
Comment thread nemo_rl/algorithms/grpo.py
Comment thread examples/configs/grpo_math_1B.yaml
Comment thread nemo_rl/algorithms/grpo.py
Comment thread nemo_rl/models/generation/vllm/vllm_worker_async.py Outdated
Comment thread nemo_rl/algorithms/grpo.py
Comment thread nemo_rl/models/generation/vllm/vllm_worker_async.py Outdated
michal2409 and others added 2 commits August 5, 2026 01:02
…-fast guards

- Make generation.val_temperature/val_top_p/val_top_k required in
  GenerationConfig (one access contract: bare subscripts everywhere), and
  add the interpolation defaults to every standalone config with a
  generation block: the 7 nemotron-3-ultra recipes, the research template,
  eval/mmau, distillation_math, ppo_math_1B, and their reference configs.
- vllm_worker_async: require top_p explicitly on requests instead of
  coalescing the comparison to 1.0 — vLLM resolves an unset top_p from the
  model's generation_config.json, so the old fallback silently re-admitted
  the off-policy request that main rejected. Document that grpo.validate()
  is the only caller constructing a non-train sampling profile.
- setup(): fail fast when val_top_k is set (the NeMo-Gym path can never
  honor it) and when stop_at_validation_metric='pass_k' is configured with
  val_num_generations_per_prompt <= 1.
- Rename the rollout SamplingParams dataclass to GenerationSamplingParams
  to avoid colliding with TrainingSamplingParams and vLLM's SamplingParams.
- Document that max_val_samples counts prompts (total rollouts = prompts * k).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Michal Futrega <mfutrega@nvidia.com>
@michal2409

Copy link
Copy Markdown
Contributor Author

/ok to test d07ef9f

GenerationConfig now requires val_temperature/val_top_p/val_top_k, so
test fixtures that build the config dict by hand must carry them too.
Mirror the train values, i.e. no validation override.

Signed-off-by: Michal Futrega <mfutrega@nvidia.com>
@michal2409

Copy link
Copy Markdown
Contributor Author

/ok to test 739a675

@yuki-97 yuki-97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thanks @michal2409 , LGTM.

one thing want to check that @terrykong do you think we should do #3497 in this PR, or leave it as is now and do in a next PR?

@terrykong
terrykong merged commit daf46ff into NVIDIA-NeMo:main Aug 6, 2026
175 of 178 checks passed
@terrykong

Copy link
Copy Markdown
Collaborator

@yuki-97 @michal2409 i merged. #3497 can be tackled in a follow up. it's sort of a preexisting issue

cspades pushed a commit to cspades/RL that referenced this pull request Aug 6, 2026
…tion (NVIDIA-NeMo#3401)

Signed-off-by: Michal Futrega <mfutrega@nvidia.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:L1 Run doctests, unit tests, and functional tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants