fix(sc): preserve DatumSpec loss_multiplier through rollouts - #3786
fix(sc): preserve DatumSpec loss_multiplier through rollouts#3786tianyi-zhang-02 wants to merge 1 commit into
Conversation
|
Closed a gap in my own description: Besides the env flag and truncated completions,
It now rides on The updated table:
Tests go red under either mutation — starting from |
…ity PRs NVIDIA-NeMo#3786 and NVIDIA-NeMo#3787 each rewrote this comment to name the knob they removed, so they conflicted on nothing but the prose -- the tuple entries merged fine. Stating the rule once, without naming a knob, lets the two auto-merge in either order. Signed-off-by: Tianyi Zhang <zhangtianyi975@gmail.com> Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
…ity PRs NVIDIA-NeMo#3786 and NVIDIA-NeMo#3787 each rewrote this comment to name the knob they removed, so they conflicted on nothing but the prose -- the tuple entries merged fine. Stating the rule once, without naming a knob, lets the two auto-merge in either order. Signed-off-by: Tianyi Zhang <zhangtianyi975@gmail.com> Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
…on point Both parity PRs added a test right before the same anchor, so they conflicted on nothing but placement. Moving this one to the end of the file lets the two auto-merge in either order. Signed-off-by: Tianyi Zhang <zhangtianyi975@gmail.com> Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
|
Heads-up on merge order, with the resolution written out. Four of my open PRs edit the same twelve-line block — the
I checked every pair by actually merging them. #3786 × #3787 is clean now — they only ever conflicted on the comment wording and on both adding a test before the same anchor, and both are fixed. The remaining four pairs are a genuine textual conflict on that block, and no restructuring avoids it: extracting or reordering the list conflicts just as hard. So rather than force a stack, here is the resolution. Whichever order they land in, this is the merged form: # An enabled one here describes shaping this run does not do. An entry
# leaves this list when the SC path starts implementing it -- rejecting a
# knob is only right while nobody honours it.
#
# DistillationConfig defines none of them: there is no reward to shape or
# filter on, so the list cannot even be evaluated on that path.
unsupported = (
[]
if is_distillation_run(master_config)
else [
name
for name, enabled in (
("use_dynamic_sampling", algo_cfg.use_dynamic_sampling),
("reward_scaling", algo_cfg.reward_scaling.enabled),
("reward_shaping", algo_cfg.reward_shaping.enabled),
)
if enabled
]
)I built that merge locally and ran the SC and config suites against it — 1084 passed. Happy to rebase whichever ones are left once the first lands; just say which order you want. |
The guard used to wrap that list in a conditional, which put this PR on the same twelve lines as NVIDIA-NeMo#3786 and NVIDIA-NeMo#3787 -- each of those removes an entry from it. Four pairwise conflicts on nothing but placement. Returns early instead. DistillationConfig defines none of the knobs the list names, so the check does not apply and the comprehension could not be evaluated anyway; everything after it is PPO-specific and returns early on this path already. The colocated requirement is the one thing that does apply, so it moves into a helper and the guard calls it directly rather than falling through. The list itself is now untouched by this PR, so all four conflicts go away and none of the three needs to land before the others. Signed-off-by: Tianyi Zhang <zhangtianyi975@gmail.com> Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
The guard used to wrap that list in a conditional, which put this PR on the same twelve lines as NVIDIA-NeMo#3786 and NVIDIA-NeMo#3787 -- each of those removes an entry from it. Four pairwise conflicts on nothing but placement. Returns early instead. DistillationConfig defines none of the knobs the list names, so the check does not apply and the comprehension could not be evaluated anyway; everything after it is PPO-specific and returns early on this path already. The colocated requirement is the one thing that does apply, so it moves into a helper and the guard calls it directly rather than falling through. The list itself is now untouched by this PR, so all four conflicts go away and none of the three needs to land before the others. Signed-off-by: Tianyi Zhang <zhangtianyi975@gmail.com> Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
The guard used to wrap that list in a conditional, which put this PR on the same twelve lines as NVIDIA-NeMo#3786 and NVIDIA-NeMo#3787 -- each of those removes an entry from it. Four pairwise conflicts on nothing but placement. Returns early instead. DistillationConfig defines none of the knobs the list names, so the check does not apply and the comprehension could not be evaluated anyway; everything after it is PPO-specific and returns early on this path already. The colocated requirement is the one thing that does apply, so it moves into a helper and the guard calls it directly rather than falling through. The list itself is now untouched by this PR, so all four conflicts go away and none of the three needs to land before the others. Signed-off-by: Tianyi Zhang <zhangtianyi975@gmail.com> Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
The guard used to wrap that list in a conditional, which put this PR on the same twelve lines as NVIDIA-NeMo#3786 and NVIDIA-NeMo#3787 -- each of those removes an entry from it. Four pairwise conflicts on nothing but placement. Returns early instead. DistillationConfig defines none of the knobs the list names, so the check does not apply and the comprehension could not be evaluated anyway; everything after it is PPO-specific and returns early on this path already. The colocated requirement is the one thing that does apply, so it moves into a helper and the guard calls it directly rather than falling through. The list itself is now untouched by this PR, so all four conflicts go away and none of the three needs to land before the others. Signed-off-by: Tianyi Zhang <zhangtianyi975@gmail.com> Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
|
Stepping back from most of this — #3766 got here first and covers more of it. I went looking for duplicate work across the open PRs and found @macandro96's #3766 ("fix(grpo): support environment sample masking in single controller"), opened two days before this one. It solves the same problem and its coverage is a superset of mine:
The one design difference I did check rather than assume: #3766 keeps a separate One thing #3766 does not cover, and it is why I am not just closing this: the prompt-level So: I'll cut this down to just the |
|
Correction to what I wrote above — I said #3766's coverage is a superset of this PR's. It isn't, and the difference matters in your favour, @macandro96. I checked #3766's
So the accurate split is:
Which doesn't change my conclusion — #3766 came first, its column-based shape is the better one, and I'd rather not have two mechanisms for one rule. But the right outcome isn't "close this"; it's that #3766 needs the Happy to send those two as a small PR on top of #3766 once it lands, and close this. Or, if it's easier, take the two lines from here directly — no attribution needed. :) |
Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
03d262d to
d85b269
Compare
What does this PR do?
Preserves
DatumSpec.loss_multiplieracross the SingleController rollout boundary.The legacy GRPO path turns that field into
sample_mask, including fractional and zero weights. SingleController previously dropped it while buildingPromptGroupRecord, sorecord_to_train_batchalways started from ones.This PR only adds the missing producer-to-consumer plumbing:
PromptGroupRecordstores the prompt-level loss multiplier;DatumSpec;record_to_train_batchfillssample_maskfrom that value.Environment masking, completion truncation, filtering semantics, and related metrics are deliberately left to the earlier #3766. This branch no longer overlaps that implementation.
Validation
Final SHA:
d85b2696593ccb3ba6fe69e5f02baf20e623cb27, based on upstreammainatccbcd4cc5.Runpod Secure Cloud,
nvcr.io/nvidia/nemo-rl:v0.7.0, Python 3.13.14, PyTorch 2.11.0+cu130, vLLM 0.25.1:The vLLM caller tests used GPUs 0/1 and verified that both rollout callers preserve
loss_multiplier=0.25in their produced records.The NeMo Gym supplement used 2×H100 80 GB. Each node ran in a fresh pytest/Ray process with separate locked vLLM and NeMo Gym actor environments. The direct manager test passed in 101.92 s and preserved
loss_multiplier=0.25through two real Gym rollouts; the legacy-vs-manager parity test passed in 75.21 s after both paths completed two rollouts against the same resource, agent, and model servers.