feat: declare failure_reason on BaseVerifyResponse by tdene · Pull Request #2552 · NVIDIA-NeMo/Gym

tdene · 2026-08-14T03:16:40Z

What does this PR do?

Declares failure_reason field on BaseVerifyResponse to standardize a way to report why a returned reward may not reflect the policy quality. failure_reason is meant to be human-readable; see #2750 for the whole design document.

Checklist

I have read the contributing guidelines.
The change is focused; unrelated "drive-by" edits are tracked as separate issues/PRs.
Tests added or updated and pass locally, or N/A for docs-only / non-code changes (so CI unit/server checks pass when applicable).
Pre-commit checks pass locally (pre-commit run --all-files) (so CI lint/format/copyright pass).
All commits have DCO sign-off (git commit -s) (so the DCO check passes).

copy-pr-bot · 2026-08-14T03:16:44Z

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

waple0820 · 2026-08-19T07:30:54Z

Hi — we hit exactly the gap this PR addresses, from the RL-training side, and we'd like to land our half with this PR rather than propose a competing one.

Context: we run browser-based RL on Gym (one isolated cloud browser per rollout, verified by an LLM judge). On a GB200 run at 64-way concurrency, orphaned environment sessions caused 1172 of 1280 rollouts across steps 61–80 (91.6%) to fail in session reset. Every one of those entered GRPO as a legitimate reward=0.0 sample. They didn't just contribute no gradient — they depressed the group baseline, inflating the advantage of the few survivors, so the run trained away from correct behavior. Mean reward went 15.45% → 0.39%.

failure_reason is exactly the right field for triage, and we'd adopt it verbatim. The one thing it can't do on its own is drive a training decision: a framework can't branch on free-form text. What we need alongside it is a machine-readable boolean.

The good news is that the boolean already exists in this repo — it's just not on the contract:

responses_api_agents/swe_agents/app.py:256 — mask_sample: bool = False, on SWEBenchWrapperInstanceConfig. Also in anyterminal_agent/app.py:106 and anyswe_agent/app.py:169, with semantics documented in swe_agents/README.md:653.
NeMo-RL already consumes it end-to-end: nemo_rl/experience/rollouts.py:229-241 _extract_mask_sample_flags() reads full_result["instance_config"]["mask_sample"], which reaches final_batch["mask_sample"] and zeroes loss_multiplier in nemo_rl/algorithms/grpo.py:2104.

So the semantics are settled and have a first-party consumer — but because the field hangs off one agent family's private config model, it only works for three SWE-shaped agents, and NeMo-RL has to hard-code that private schema path. Every other environment in the repo gets nothing, and verl gets nothing at all (verl-recipe nemo_gym/agent_loop.py:460 reads result["full_result"]["reward"]; _empty_result() at :368-380 hard-codes {"reward": 0.0}).

Would you be open to landing both fields together on BaseVerifyResponse?

class BaseVerifyResponse(BaseVerifyRequest):
    reward: float
    mask_sample: bool = False              # machine-readable: reward doesn't reflect policy quality
    failure_reason: Optional[str] = None   # human-readable cause (this PR)

Both default-inert, so existing environments are unchanged. Gym would state the fact and stop there — whether to mask the loss, resample, exclude from group statistics, or drop the group stays the training framework's decision, and the three frameworks we checked do genuinely differ there.

We're happy to do the work: contribute the mask_sample half as a PR on top of this one, migrate the three SWE agents to set the top-level field (keeping instance_config.mask_sample as a deprecated alias for a release), and add the round-trip tests. We're opening a tracking issue for the boolean shortly and will link it here — but we wanted to check with you first, since we'd rather this land once than twice.

tdene · 2026-08-19T14:40:52Z

@waple0820 thank you for the comment!

Could you please also take a look at #2383 #2384 #2385 #2386 #2387 #2388 #2410 as well? Those are all related PRs, in the spirit of this one, that handle resiliency issues I personally found to be critical. In fact this PR was opened to address reviewer comments on #2384.

It would be very useful to have your feedback on whether those fixes also look important to you.

ananthsub · 2026-08-24T20:26:05Z

+    # Elaborates on why `reward` may not be fully genuine, allowing the upstream
+    # training loop to decide on how to treat the response.
+    # For example, timeouts may result in reward=0, and we should be honest about the reason.
+    failure_reason: Optional[str] = None
+


echoing @waple0820 's comment, I believe it'd be useful to have these 3 fields:

mask_sample: signoff from the verifier/environment around using this reward downstream

failure_kind: a diagnosis/category of failures that we can use for grouping in telemetry

failure_reason: freeform error message

mask_sample=False but failure_kind+failure_reason being set can indicate a degraded but scored state as legitimate

An environment that knows it failed - lost session, unavailable judge, OOM-killed container, reset timeout - can only return reward=0.0 today, which is indistinguishable from a policy that genuinely scored zero. The field already exists in this repo, just not on the contract: `mask_sample` lives on SWEBenchWrapperInstanceConfig (responses_api_agents/swe_agents/app.py), anyterminal_agent and anyswe_agent, and NeMo-RL already consumes it end to end through full_result["instance_config"]["mask_sample"]. Because it hangs off one agent family's private config model it covers three SWE-shaped agents only, NeMo-RL has to hard-code that private path, and verl gets nothing. Promote it to BaseVerifyResponse, alongside the human-readable failure_reason proposed in NVIDIA-NeMo#2552: * mask_sample: bool = False - machine-readable, for training frameworks * failure_reason: Optional[str] = None - for triage and logs Gym states the fact and stops there. Whether to mask the loss, resample the episode, exclude the sample from group statistics or drop the group stays the training framework's decision; the frameworks we checked genuinely differ. Defaults are inert, so existing environments are unchanged. swe_agents and anyswe_agent now mirror their instance-config flag onto the contract field and keep the old location for one release; anyterminal_agent already surfaces it via TerminalBenchMetrics. Closes NVIDIA-NeMo#2608 Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com>

tdene · 2026-08-25T14:19:42Z

/ok to test 989be05

tdene · 2026-08-25T14:52:39Z

/ok to test 700bf92

copy-pr-bot · 2026-08-25T16:01:36Z

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.

Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>

An environment that knows it failed - lost session, unavailable judge, OOM-killed container, reset timeout - can only return reward=0.0 today, which is indistinguishable from a policy that genuinely scored zero. The field already exists in this repo, just not on the contract: `mask_sample` lives on SWEBenchWrapperInstanceConfig (responses_api_agents/swe_agents/app.py), anyterminal_agent and anyswe_agent, and NeMo-RL already consumes it end to end through full_result["instance_config"]["mask_sample"]. Because it hangs off one agent family's private config model it covers three SWE-shaped agents only, NeMo-RL has to hard-code that private path, and verl gets nothing. Promote it to BaseVerifyResponse, alongside the human-readable failure_reason proposed in NVIDIA-NeMo#2552: * mask_sample: bool = False - machine-readable, for training frameworks * failure_reason: Optional[str] = None - for triage and logs Gym states the fact and stops there. Whether to mask the loss, resample the episode, exclude the sample from group statistics or drop the group stays the training framework's decision; the frameworks we checked genuinely differ. Defaults are inert, so existing environments are unchanged. swe_agents and anyswe_agent now mirror their instance-config flag onto the contract field and keep the old location for one release; anyterminal_agent already surfaces it via TerminalBenchMetrics. Closes NVIDIA-NeMo#2608 Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com>

…coring it zero `verify` returned `reward=0.0` whenever the browser session was missing, which is the conflation this environment exists to demonstrate: a rollout whose session was lost is not a policy that solved nothing, and downstream the two are identical. It now reports `failure_reason` on that path, using the field NVIDIA-NeMo#2552 added to `BaseVerifyResponse`. A browser that died mid-episode was worse than mis-scored. `_score` reads the live page, so a dead browser raised out of `verify`; only `JudgeError` is converted to a routed row, so that exception ended the whole collection run rather than the one rollout it belonged to. Browser reads are now wrapped and reported the same way. An unsupported scoring key still raises, because a dataset typo is a configuration error rather than an infrastructure failure, and failing on the first rollout is the intended behaviour. Building the response by spreading the request and also passing `reward` as a keyword was a latent `TypeError: got multiple values`: `BrowserVerifyRequest` allows extra fields, so a caller putting `reward` or `failure_reason` in the body crashed verify. Response-owned fields are now dropped from the spread. The same shape was found and fixed in `anyswe_agent` on NVIDIA-NeMo#2611. Closing the previous browser on a re-seeded session swallowed every exception. A browser we could not close is a resource the run still holds, so it is logged. Tests build and serialize the real response, since the collision only appears at construction time. Reverting any one of the three fixes fails a test. Signed-off-by: waple0820 <feng.wang@lexmount.com>