feat(core): report infrastructure failure from verify, not just a reward - #2611
feat(core): report infrastructure failure from verify, not just a reward#2611waple0820 wants to merge 3 commits into
Conversation
1d8d648 to
d626da4
Compare
d626da4 to
f55c8a0
Compare
|
@ananthsub rebased onto main and reshaped around the three fields as you described them here: #2552 (comment)
Two things worth checking against your design:
In #2728 Ordering is yours: happy to land after #2728 and drop anything that overlaps, or before it if that is easier. |
| class BaseVerifyResponse(BaseVerifyRequest): | ||
| reward: float | ||
|
|
||
| # True when ``reward`` does not reflect policy quality because the environment or its |
There was a problem hiding this comment.
This description is too specific to RL training. Gym evaluation and other library callers can consume the same field, while loss masking, resampling, and group handling are only some possible policies. It is also an ordinary comment, so it will not appear in Pydantic's generated schema.
instead, lets add this with Field(description=...) that defines the result semantics: whether this completed sample should be excluded from evaluation scores and other downstream quality calculations because its reward is not a valid measurement of the evaluated system
There was a problem hiding this comment.
Done
One prerequisite this needed: AnySweVerifyResponse(SWEBenchMetrics, BaseVerifyResponse) inherits mask_sample from both, and SWEBenchMetrics wins the MRO, so on that response the field resolved to Optional[bool] = None with no description — the contract field was invisible in its schema whatever the base declared. Restated on the subclass so the type, default and description survive.
|
|
||
|
|
||
| def _masking_step_metrics(agent_name: str, total: int, unmasked: Counter) -> Dict[str, float]: | ||
| """Progress metrics for the rollouts an environment reported as masked. |
There was a problem hiding this comment.
This helper changes progress telemetry only; it does not make the final evaluation result mask-aware. Later, every persisted result is sent to /aggregate_metrics, where the generic profiler still averages masked rewards and treats mask_sample itself as a numeric metric. For example, a valid reward of 1 plus a masked reward of 0 still publishes mean/reward=0.5.
Please apply masking centrally in compute_aggregate_metrics(), including the input to custom compute_metrics() and key-metric selection. Masked rows should remain persisted for coverage, and fully masked tasks should publish coverage without inventing a zero quality score.
| metrics = agent_name_to_metrics[agent_name] | ||
| metrics.update({k: v for k, v in result.items() if isinstance(v, (int, float)) and not k.startswith("_")}) | ||
| agent_name_to_counts[agent_name] += 1 | ||
| if not result.get(MASK_SAMPLE_KEY): |
There was a problem hiding this comment.
reward_unmasked currently includes failure-sidecar and non-persisted results because these counters are updated for every returned dictionary. Please update quality metrics only for results eligible for the main rollout output. Track failed and omitted results separately.
| reward=1.0 if metrics.resolved else 0.0, | ||
| # Report it on the contract as well; `instance_config.mask_sample` stays | ||
| # for one release so existing consumers keep working. | ||
| mask_sample=bool(instance_config.mask_sample), |
There was a problem hiding this comment.
SWEBenchMetrics.model_dump() already contains mask_sample, so this explicit keyword followed by **metrics.model_dump() raises TypeError: got multiple values for keyword argument 'mask_sample' on every result. The value here also comes from instance_config, which remains at its default, while the computed decision is written to metrics.mask_sample.
Please resolve the value once from metrics.mask_sample, copy it into the existing compatibility instance_config field, exclude the key from the expanded metrics mapping, and pass it once at the top level.
An end-to-end /run test should construct and serialize the response so this path is covered.
There was a problem hiding this comment.
Fixed.
We added the end-to-end /run test. Reverting the fix makes it fail with exactly the TypeError you predicted.
… telemetry Addresses review on NVIDIA-NeMo#2611. `mask_sample` moves from a comment to `Field(description=...)` so it reaches the generated schema, and the description states the result semantics rather than one consumer's policy: whether a completed sample should be excluded from evaluation scores and other downstream quality calculations because its reward is not a valid measurement of the evaluated system. `compute_aggregate_metrics()` now applies that centrally. Quality metrics, the input to a custom `compute_metrics()`, and key-metric selection all come from the scored subset; masked rows stay in the input and are reported as coverage. A run where every sample is masked publishes coverage instead of inventing a zero, and `mask_sample` is no longer coerced into a numeric metric of its own. The keys appear only once something is masked, so a run that masks nothing is unchanged. Progress accounting is restricted to results that reach the main rollout output. A sidecar failure and a kill-shaped omission are counted as such rather than averaged in as rewards of zero. `anyswe_agent` raised `TypeError: got multiple values for keyword argument 'mask_sample'` on every result, because `SWEBenchMetrics` already carries the key that was also passed explicitly. It also read `instance_config.mask_sample`, which keeps its default, while `_should_mask_sample()` writes the decision to `metrics.mask_sample`. The value is now resolved once from the metrics, mirrored onto the compatibility field, and passed once. `SWEBenchMetrics.mask_sample` is declared `Optional[bool] = None` and wins the MRO on `AnySweVerifyResponse`, so the contract field is restated there to keep its type, default and description. An end-to-end `/run` test constructs and serializes the response, and fails with the original `TypeError` without the fix. Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com>
f95c306 to
ceb910e
Compare
… telemetry Addresses review on NVIDIA-NeMo#2611. `mask_sample` moves from a comment to `Field(description=...)` so it reaches the generated schema, and the description states the result semantics rather than one consumer's policy: whether a completed sample should be excluded from evaluation scores and other downstream quality calculations because its reward is not a valid measurement of the evaluated system. `compute_aggregate_metrics()` now applies that centrally. Quality metrics, the input to a custom `compute_metrics()`, and key-metric selection all come from the scored subset; masked rows stay in the input and are reported as coverage. A run where every sample is masked publishes coverage instead of inventing a zero, and `mask_sample` is no longer coerced into a numeric metric of its own. The keys appear only once something is masked, so a run that masks nothing is unchanged. Progress accounting is restricted to results that reach the main rollout output. A sidecar failure and a kill-shaped omission are counted as such rather than averaged in as rewards of zero. `anyswe_agent` raised `TypeError: got multiple values for keyword argument 'mask_sample'` on every result, because `SWEBenchMetrics` already carries the key that was also passed explicitly. It also read `instance_config.mask_sample`, which keeps its default, while `_should_mask_sample()` writes the decision to `metrics.mask_sample`. The value is now resolved once from the metrics, mirrored onto the compatibility field, and passed once. `SWEBenchMetrics.mask_sample` is declared `Optional[bool] = None` and wins the MRO on `AnySweVerifyResponse`, so the contract field is restated there to keep its type, default and description. An end-to-end `/run` test constructs and serializes the response, and fails with the original `TypeError` without the fix. 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>
ceb910e to
df1d43f
Compare
… telemetry Addresses review on NVIDIA-NeMo#2611. `mask_sample` moves from a comment to `Field(description=...)` so it reaches the generated schema, and the description states the result semantics rather than one consumer's policy: whether a completed sample should be excluded from evaluation scores and other downstream quality calculations because its reward is not a valid measurement of the evaluated system. `compute_aggregate_metrics()` now applies that centrally. Quality metrics, the input to a custom `compute_metrics()`, and key-metric selection all come from the scored subset; masked rows stay in the input and are reported as coverage. A run where every sample is masked publishes coverage instead of inventing a zero, and `mask_sample` is no longer coerced into a numeric metric of its own. The keys appear only once something is masked, so a run that masks nothing is unchanged. Progress accounting is restricted to results that reach the main rollout output. A sidecar failure and a kill-shaped omission are counted as such rather than averaged in as rewards of zero. `anyswe_agent` raised `TypeError: got multiple values for keyword argument 'mask_sample'` on every result, because `SWEBenchMetrics` already carries the key that was also passed explicitly. It also read `instance_config.mask_sample`, which keeps its default, while `_should_mask_sample()` writes the decision to `metrics.mask_sample`. The value is now resolved once from the metrics, mirrored onto the compatibility field, and passed once. `SWEBenchMetrics.mask_sample` is declared `Optional[bool] = None` and wins the MRO on `AnySweVerifyResponse`, so the contract field is restated there to keep its type, default and description. An end-to-end `/run` test constructs and serializes the response, and fails with the original `TypeError` without the fix. Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com>
… telemetry Addresses review on NVIDIA-NeMo#2611. `mask_sample` moves from a comment to `Field(description=...)` so it reaches the generated schema, and the description states the result semantics rather than one consumer's policy: whether a completed sample should be excluded from evaluation scores and other downstream quality calculations because its reward is not a valid measurement of the evaluated system. `compute_aggregate_metrics()` now applies that centrally. Quality metrics, the input to a custom `compute_metrics()`, and key-metric selection all come from the scored subset; masked rows stay in the input and are reported as coverage. A run where every sample is masked publishes coverage instead of inventing a zero, and `mask_sample` is no longer coerced into a numeric metric of its own. The keys appear only once something is masked, so a run that masks nothing is unchanged. Progress accounting is restricted to results that reach the main rollout output. A sidecar failure and a kill-shaped omission are counted as such rather than averaged in as rewards of zero. `anyswe_agent` raised `TypeError: got multiple values for keyword argument 'mask_sample'` on every result, because `SWEBenchMetrics` already carries the key that was also passed explicitly. It also read `instance_config.mask_sample`, which keeps its default, while `_should_mask_sample()` writes the decision to `metrics.mask_sample`. The value is now resolved once from the metrics, mirrored onto the compatibility field, and passed once. `SWEBenchMetrics.mask_sample` is declared `Optional[bool] = None` and wins the MRO on `AnySweVerifyResponse`, so the contract field is restated there to keep its type, default and description. An end-to-end `/run` test constructs and serializes the response, and fails with the original `TypeError` without the fix. Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com>
df1d43f to
b05103d
Compare
…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>
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>
`mask_sample` on the verify response was a declaration nothing read. Token capture already keeps a masked count for the rollouts it cannot rebuild, so this is the same accounting for the masking an environment reports itself. `progress/<agent>/reward` keeps averaging over every rollout, so an existing dashboard reads the number it read before. `reward_unmasked` is that average with the masked rollouts removed; the gap between the two is the score a run lost to its own infrastructure rather than to the policy. Both keys appear only once something is actually masked, so a run that reports no masking exports exactly what it exported before. Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com>
… telemetry Addresses review on NVIDIA-NeMo#2611. `mask_sample` moves from a comment to `Field(description=...)` so it reaches the generated schema, and the description states the result semantics rather than one consumer's policy: whether a completed sample should be excluded from evaluation scores and other downstream quality calculations because its reward is not a valid measurement of the evaluated system. `compute_aggregate_metrics()` now applies that centrally. Quality metrics, the input to a custom `compute_metrics()`, and key-metric selection all come from the scored subset; masked rows stay in the input and are reported as coverage. A run where every sample is masked publishes coverage instead of inventing a zero, and `mask_sample` is no longer coerced into a numeric metric of its own. The keys appear only once something is masked, so a run that masks nothing is unchanged. Progress accounting is restricted to results that reach the main rollout output. A sidecar failure and a kill-shaped omission are counted as such rather than averaged in as rewards of zero. `anyswe_agent` raised `TypeError: got multiple values for keyword argument 'mask_sample'` on every result, because `SWEBenchMetrics` already carries the key that was also passed explicitly. It also read `instance_config.mask_sample`, which keeps its default, while `_should_mask_sample()` writes the decision to `metrics.mask_sample`. The value is now resolved once from the metrics, mirrored onto the compatibility field, and passed once. `SWEBenchMetrics.mask_sample` is declared `Optional[bool] = None` and wins the MRO on `AnySweVerifyResponse`, so the contract field is restated there to keep its type, default and description. An end-to-end `/run` test constructs and serializes the response, and fails with the original `TypeError` without the fix. Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com>
b05103d to
2bbc5a3
Compare
|
Independent evaluation evidence supports the core distinction in this PR: one rollout with an agent/infrastructure failure and its fresh recovery with a valid-but-wrong result both produced scalar reward The strongest validation properties are already reflected in this PR's plan: valid zero versus masked zero, inert defaults for unmodified environments, fully masked groups producing coverage rather than an invented score, custom metrics receiving only scored samples, and observability retaining all attempts. I would additionally preserve an exact reconciliation invariant across scored, masked, failed, omitted, and unknown dispositions as #2750 evolves. Given that #2611 already owns the implementation, I will not open a duplicate PR for #2608. |
Closes #2608
BaseVerifyResponsecarries a single scalar for the outcome, so an environment that knows it failed — lost session, unavailable judge, OOM-killed container, reset timeout — can only return0.0, which is indistinguishable from a policy that genuinely scored zero.The concept is not new to this repo. It exists in four private locations:
mask_sample: bool = FalseonSWEBenchWrapperInstanceConfig(responses_api_agents/swe_agents/app.py:272), alsoanyterminal_agent/app.py:108andanyswe_agent/app.py:180, documented atswe_agents/README.md:670.conversational_tool_use/simulation/app.py:112keeps its own{"mask_sample": ...}dict.nemo_gym/token_id_capture/delivery.py:49already definesMASK_SAMPLE_KEY = "mask_sample"as a top-level rollout field, described there as what "a consumer reads to exclude a rollout from the loss".full_result["instance_config"]["mask_sample"](nemo_rl/experience/rollouts.py:229-241), which zeroesloss_multiplierinnemo_rl/algorithms/grpo.py:2104.So core already agrees what the key is called and what it means; it is reachable only if your environment happens to be SWE-shaped or if token capture produced it. verl gets nothing.
This is the same objection raised on #2384 — that a fix "should be applied uniformly, to other benchmarks as well" rather than to one harness. Putting the field on the contract is what makes it uniform.
What this changes
1. The contract (
nemo_gym/base_resources_server.py)Gym states the fact and stops there. Masking, resampling, excluding from group statistics or dropping the group stays the training framework's decision — the three frameworks we checked genuinely differ.
The three fields are orthogonal, per the review discussion on #2552: a rollout that degraded but was still scored legitimately keeps
mask_sample=Falsewhile naming afailure_kind/failure_reason.swe_agentsandanyswe_agentmirror their instance-config flag onto the contract field and keep the old location for one release.anyterminal_agentalready surfaces it throughTerminalBenchMetrics.2. Something that reads it (
nemo_gym/reward_profile.py,nemo_gym/rollout_collection.py)A declaration nothing consumes is not worth much, and the consumer that matters is the one that publishes the score.
compute_aggregate_metrics()computes quality from the scored subset: the profiler input, a customcompute_metrics(), and key-metric selection all skip masked samples. Masked rows stay in the input and are reported as coverage instead —coverage/rollouts_total|scored|maskedandcoverage/tasks_total|scored|fully_masked. A run where every sample is masked publishes coverage rather than inventing a zero, and a fully masked task is counted without contributing a group.mask_sampleis stripped before profiling, so it is no longer averaged as though the flag were a measurement.perf_summarydeliberately keeps using every response, masked included: a masked sample still executed and still reported its latency, so observability coverage should not shrink because a rollout was excluded from scoring.Progress telemetry gets the same split while a run is still going, over the results that reach the main rollout output:
progress/<agent>/rewardis unchanged — it still averages over every rollout, so an existing dashboard reads the number it read before.progress/<agent>/reward_unmaskedis that same average with the masked rollouts removed.progress/<agent>/masked_pctis how many were removed.progress/<agent>/failedand/omittedcount the attempts that never reach the main output at all, so a sidecar failure is never folded into a quality average as a reward of zero.The gap between the two reward series is the score a run lost to its own infrastructure rather than to the policy. On the run that motivated this, 1172 of 1280 rollouts across steps 61–80 failed in session reset and entered GRPO as legitimate zeros; nothing in the metrics said so.
Every new key appears only once something is actually masked, failed or omitted, so a healthy run publishes exactly what it published before.
Relationship to the other failure-handling work
failure_reasonand its comment already points here: "Machine-readable handling belongs tomask_sample/failure_kind." This PR is that half.failure_kindand the shared registry, and already reservessession_lostwithsource="#2611". No overlap — this PR does not touchfailure_kind.mask_sample=True, a stablefailure_kind, and a human-readablefailure_reason" — is exactly what the three fields together express once this lands.Compatibility
Defaults are inert; an unmodified environment behaves identically, and a run that masks nothing publishes exactly the keys it published before.
One inheritance detail worth naming:
AnySweVerifyResponseinheritsmask_samplefrom bothSWEBenchMetricsandBaseVerifyResponse, and the metrics model wins the MRO, so the contract field resolved toOptional[bool] = Nonewith no description on that response. It is restated on the subclass so the schema keeps the contract's type, default and description.Tests
tests/unit_tests/test_base_resources_server.py— defaults, round trip, that a plainreward=0.0is not implicitly masked, and that a degraded-but-scored rollout stays unmasked.tests/unit_tests/test_aggregate_metrics.py— a masked zero does not halve the score, the flag is not published as a quality metric, a run that masks nothing is byte-identical, a fully masked run reports coverage instead of a zero, a fully masked task is counted but not scored, customcompute_metrics()sees only scored samples, and a fully masked run still reports observability coverage.tests/unit_tests/test_rollout_collection.py— a healthy run adds no keys; a partially masked agent reports its share and the score without it; a fully masked agent publishes no average; failed and omitted attempts are counted, never averaged.responses_api_agents/anyswe_agent/tests/test_app.py— an end-to-end/runthat constructs and serializes the response. Reverting the fix makes it fail withTypeError: got multiple values for keyword argument 'mask_sample'.uv run --extra dev pytest tests/unit_tests -q→ 3383 passed. Two failures on this branch,test_e2b_providerandtest_enroot_provider, reproduce on a cleanupstream/mainand are unrelated.pre-commitclean.