[trainer] feat: pass non_tensor_batch to advantage estimators that can accept it - #7473
Open
waple0820 wants to merge 1 commit into
Open
[trainer] feat: pass non_tensor_batch to advantage estimators that can accept it#7473waple0820 wants to merge 1 commit into
waple0820 wants to merge 1 commit into
Conversation
…n accept it
compute_advantage() gives registered advantage estimators only tensors plus
`index`. Anything a rollout attached per sample - a validity flag, a task id,
benchmark metadata - lives in DataProto.non_tensor_batch and never reaches the
estimator, so a custom estimator cannot use it.
GDPO already needed exactly this and got a named special case:
if adv_estimator in (AdvantageEstimator.GDPO, "gdpo"):
adv_kwargs["non_tensor_batch"] = data.non_tensor_batch
adv_kwargs["batch"] = data.batch
That works, but it means an estimator has to be named in ray_trainer.py to see
its own data. Generalize it: pass both keys to any estimator whose signature can
accept them (declared parameter or **kwargs), which subsumes the GDPO case.
Nothing changes for existing estimators. Of the ones registered today only
grpo_vectorized has a fixed signature, and it now receives exactly what it did
before; every other estimator has **kwargs and ignores unused keys. GAE and GRPO
keep their own branches.
Motivation: a rollout that failed for infrastructure reasons scores reward=0 and
is indistinguishable from a policy that scored zero, so it depresses the GRPO
group baseline and inflates the advantage of whatever survived. Excluding those
samples needs a per-sample flag inside the estimator. The agent loop already
promotes AgentLoopOutput.extra_fields into non_tensor_batch, so the data is
there - it just is not forwarded.
Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com>
waple0820
requested review from
PeterSH6,
eric-haibin-lin,
tongyx361 and
vermouth1992
as code owners
August 19, 2026 09:54
waple0820
force-pushed
the
feat/adv-estimator-non-tensor-batch
branch
from
August 25, 2026 03:49
99f04ad to
c9e1e91
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
compute_advantage()hands a registered advantage estimator tensors plusindex. Everything a rollout attached per sample — a validity flag, a task id, benchmark metadata — lives inDataProto.non_tensor_batchand never reaches the estimator, so a custom estimator cannot use it.The data is already there.
AgentLoopOutput.extra_fieldsis promoted intonon_tensor_batchby the agent loop (verl/experimental/agent_loop/agent_loop.py), so a rollout can attach anything it wants per sample. It just is not forwarded one function further.GDPO hit this first and got a named special case in
verl/trainer/ppo/ray_trainer.py:That works, but it means an estimator has to be named in
ray_trainer.pyto see its own data — a custom estimator registered throughregister_adv_estcannot.Change
Pass both keys to any estimator whose signature can accept them (a declared parameter, or
**kwargs), which subsumes the GDPO case:Why this is safe for every estimator registered today
I audited all twelve
@register_adv_estfunctions incore_algos.py:gdponon_tensor_batch, has**kwargsgrpo_passk,reinforce_plus_plus_baseline,rloo,opo,reinforce_plus_plus,remax,gpg,optimal_token_baseline,multi_turn_optimal_token_baseline**kwargsgrpo_vectorized**kwargsgae,grpoPassing unconditionally would raise
TypeErrorongrpo_vectorized, which is the failure mode the signature check avoids.Motivation
A rollout that fails for infrastructure reasons — a lost environment session, an unreachable judge, an OOM-killed container — scores
reward=0, which is indistinguishable from a policy that genuinely scored zero. In GRPO that zero does not merely contribute no gradient: it lowers the group baseline and inflates the advantage of whatever survived, so the batch trains away from correct behaviour.We hit this training a browser agent: at 64-way rollout concurrency, orphaned environment sessions caused 1172 of 1280 rollouts across 20 steps (91.6%) to fail before the policy ever acted, and mean reward fell from 15.45% to 0.39%. Excluding those samples from the group statistics needs a per-sample flag inside the estimator, which is what this change makes possible.
Upstream context, for what produces such a flag: NeMo Gym is adding
mask_sampleto its verify contract so an environment can report an infrastructure failure rather than an implicit zero (NVIDIA-NeMo/Gym#2608, NVIDIA-NeMo/Gym#2611). With this change a recipe can consume it with a plain@register_adv_estestimator; without it, the only options are monkey-patchingcompute_advantageor vendoringcore_algos.py.Tests
tests/trainer/ppo/test_adv_estimator_kwargs_on_cpu.py:_accepts_kwargrecognises a declared parameter and a**kwargscatch-all, and rejects a fixed signature.non_tensor_batchand can read a per-sample field from it.Note on the V1 trainer
verl/trainer/ppo/v1/trainer_base.py::_compute_advantageselects a fixed field list from the TransferQueue (uid,response_mask,rm_scores,rollout_log_probs,old_log_probs,ref_log_prob,values) before building theDataProto, so under V1 a custom non-tensor field still does not reach the estimator even with this change. I left that out to keep this PR to one idea, but I am happy to follow up with a way to opt extra fields into that selection if you think that is the right direction.