fix(sc): apply advantage clipping and overlong filtering - #3927
Conversation
Apply configured GRPO advantage bounds after estimation while preserving pre-clipping metrics. Propagate overlong filtering into SingleController payload conversion so truncated completions receive a zero sample mask. Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
github-actions
Bot
added
the
Documentation
yfw
left a comment
yfw
left a comment
There was a problem hiding this comment.
The parity port checks out. I traced the filter/clip ordering against all three legacy paths and SC matches grpo_sync step for step — overlong filtering lands on the same side of compute_advantage as legacy (grpo.py:3372 -> :3403 -> :3540; ppo.py:1507-1514 -> :1546), so no estimator sees a mask it would not have seen on the legacy path. 975 passed across tests/unit/single_controller/ and tests/unit/experience/; pre-commit run --all-files is clean. Both new tests are strong under mutation — deleting the clip block reddens only test_advantage_stage_clips_training_values_after_recording_metrics, and the False leg of the payload parametrize is what catches filtering applied unconditionally.
Worth adding to the PR description: this also repairs a nightly that cannot currently start. examples/configs/recipes/llm/mopd-qwen3-1.7b-3n8g-megatron-pack-single-controller.yaml inherits grpo.overlong_filtering: true from its parent (mopd-qwen3-1.7b-3n8g-megatron-pack.yaml:7) without overriding it, and is listed in tests/test_suites/nightly.txt:277. Resolving that recipe and running the real validator: it passes at this head, and raises NotImplementedError on main (the list entry at config.py:783 as of ccbcd4cc5). It is the only shipped SC recipe whose behaviour this PR changes.
That points at one coverage gap worth closing here: no test runs validate_single_controller_config against a shipped recipe. test_setup.py:398 test_single_controller_mopd_recipe_resolves_to_runtime_contract loads that exact recipe but only MasterConfig.model_validates it and asserts fields, so it stayed green throughout. Adding validate_single_controller_config(config) there — the symbol is not currently imported in that file — would have caught this, and would catch the next one.
Three inline comments follow. All are low severity; none blocks.
Generated by Claude Code
Resolve the SingleController payload conflicts while preserving message-violation fields and overlong filtering. Address review feedback for clipped advantage metrics, pre-clip OPD statistics, empty-step diagnostics, and shipped recipe validation. Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
yfw
added
the
CI:Lfast
|
/ok to test bfbb5fe |
yuki-97
left a comment
yuki-97
left a comment
There was a problem hiding this comment.
Scope: the SC advantage-clipping + overlong-filtering port (16 files), checked against the three legacy paths — grpo.py, grpo_sync.py, ppo.py.
Checked:
- Clip ordering — relative to the message-level advantage penalties, and pre- vs post-clip metric emission.
- OPD statistics — that they stay on the estimator output rather than the clipped tensor.
token_mask * sample_mask— the mask the estimator actually receives once filtering is on.truncatedpopulation — on both the env-rollout and the Gym rollout path.- Recipe inheritance — for the two SC recipes that resolve
overlong_filtering: true. - The new unit coverage.
Parity holds throughout.
Remaining comments are four nits — one test-coverage gap, one docstring wording, and two on the metric/select block in _advantage_stage. None blocking.
Co-authored-by: Yuki Huang <yukih@nvidia.com> Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
Reuse the precomputed response advantages on PPO while recomputing them after GRPO clipping. Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
|
/ok to test fdb049f |
Resolve SingleController sample-filtering conflicts by adopting the upstream raw-mask pipeline and retaining GRPO advantage clipping. Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
|
/ok to test 796e46e |
…#3927) Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com> Co-authored-by: Yuki Huang <yukih@nvidia.com>
Main's environment sample masking (#3766) and overlong filtering (#3927) made the SingleController advantage stage read the per-sample ``mask_sample`` and ``truncated`` fields unconditionally. The native ``pack_payload`` path emits both from each Completion; the capture-mode ``BlackboxFinalizer`` assembles its own train batch from receipts and never did, so the first streamed batch of a token-capture run died with ``KeyError: 'mask_sample'`` (job 6899709, 64-node rlvr_dolphin_sc). Thread the two flags through the metadata-only ``FinalizationRequest``: the rollout manager reads the gym ``instance_config.mask_sample`` flag and the completion's ``truncated`` flag at dispatch time, the finalizer actor checks they are parallel to the receipts, and ``finalize_group`` publishes them as bool tensors next to the existing seven fields. Both new parameters are required so every caller states them. The ``mask_env_flagged_samples`` gate now pops the flag from receipt results as well as token results inside ``AsyncNemoGymRolloutImpl``, so the request builder reads env_extras without reaching into the impl (job 6903474 died on exactly that reach). Receipt completions set ``truncated=False`` by existing convention, so overlong filtering remains inert in capture mode; that is unchanged behaviour, now visible in the row schema instead of a KeyError. Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
2 participants
Summary
Ports the remaining SingleController parity from #2686:
sample_maskwhen overlong filtering is enabledThe change composes with the message-violation payload fields now on
main, reuses the existing TransferQueue schema, and does not alter rollout topology, batching, lifecycle, or retry behavior. It also unblocks validation of the shipped MOPD SingleController nightly and lets the PPO SingleController nightly inherit enabled overlong filtering from its parent recipe.Validation
pytest -q tests/unit/single_controller tests/unit/experience/test_payload.py— 733 passedgit diff --checkKnown limitation
The existing SingleController empty-step guard still rejects a step when every completion across that step is filtered; mixed valid/truncated batches follow the intended masked-loss behavior.