feat: add shared failure-kind registry by ananthsub · Pull Request #2728 · NVIDIA-NeMo/Gym

ananthsub · 2026-08-24T20:56:34Z

Summary

add an optional failure_kind machine key to BaseVerifyResponse while allowing namespaced environment extensions
register active failure strings already produced on main and reserve vocabulary from the in-flight recovery PRs
deliberately keep retryability, delivery state, policy attribution, and masking out of the registry because those vary per failure event

Test plan

uv run --extra dev pre-commit run --files nemo_gym/failure_kinds.py nemo_gym/base_resources_server.py tests/unit_tests/test_failure_kinds.py tests/unit_tests/test_base_resources_server.py
uv run --extra dev pytest tests/unit_tests/test_failure_kinds.py tests/unit_tests/test_base_resources_server.py -q

Give rollout tooling stable machine-readable failure keys while keeping masking and retry policy on individual failure records. Reserve vocabulary for in-flight recovery changes so producers converge before the strings become public artifact contracts. Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>

copy-pr-bot · 2026-08-24T20:57:27Z

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.

ananthsub · 2026-09-09T05:46:14Z

#3179 now owns the shared failure-kind registry. Please rebase this draft onto current main first; it is the merge gate for #2608 and #2135 consumers. Keep the registry deliberately small—stable name, active/status state, and source—and do not encode retry policy that can diverge from #3159/#2845.

waple0820 · 2026-09-09T07:18:48Z

@ananthsub this gates #2611 per the merge order in #2750, and it's been in draft since 8/24 and conflicting with main.

We can take it over — rebase it and carry the conflicts, or implement the registry against #3179 if that's cleaner than handing it back and forth. #2611 then rebases onto whatever lands here instead of declaring failure_kind itself.

waple0820 · 2026-09-10T11:52:47Z

Opened #3213 as a replacement rather than a rebase of this one — this branch is on a fork we cannot push to, and #3179's last acceptance criterion is "PR #2728 is rebased or replaced under this issue".

Two differences worth naming. The vocabulary there is drawn from what main produces now: #2017 and #2883 introduced the agent_request_failed / agent_run_error split after this PR was drafted, and tests assert those constants stay registered so the set cannot drift from the code emitting it. The per-entry status/source fields are dropped, since reserved entries point at PRs that may not land and that is the "metadata that can become stale" the issue rules out.

#3213 also leaves BaseVerifyResponse alone — failure_kind reaching the verifier contract is #2608's follow-on and lands with #2611.

ananthsub · 2026-09-10T14:10:07Z

Closing as superseded by #3213, which implements the current #3179 contract against current main: a small failure-kind vocabulary, warn-on-unknown compatibility, and no stale per-kind policy metadata. The newer PR also includes the failure labels now emitted by the collector. There is no value in maintaining two competing registry implementations.

Closes #3179 Gym describes failures with free text and component-local labels, so the same failure reaches a collector, a log line and a metric under three different names and cannot be grouped. ## What a name is, and what it is not A name says what kind of thing went wrong and nothing else. Retryability, replay safety, whether a completed result should be masked, and what to tell a person are properties of the occurrence rather than of the name — they belong to the failure record, the verify response, and `failure_reason`. Keeping them out is the reason the registry carries no per-entry metadata: anything attached to a name goes stale the moment one caller wants to retry what another does not. That is also what the acceptance criterion "no retry-policy metadata that can become stale" asks for. Names are `<domain>_<condition>`, so a reader can tell an unreachable model server from an unreachable sandbox without a second field, and they are low cardinality, so they are safe as a metric label or a span dimension. `failure_reason` never is. ## Unknown names warn rather than raise `validate_failure_kind` returns the name unchanged and logs once. Rejecting an unregistered value would trade a visible wrong label for an invisible dropped failure, which is worse than the problem the vocabulary exists to fix, and it would break components mid-migration. An environment that needs something the shared set should not grow can use `<server>:<kind>`. ## The vocabulary describes the repo, not an intention `agent_run_error`, `agent_request_failed` and `judge_failed` are labels `main` emits today. Tests import those constants from `rollout_collection` and `rollout_reverification` and assert they are registered, so the set cannot drift from the code that produces it. ## Scope This adds the vocabulary only. `failure_kind` reaching `BaseVerifyResponse` is #2608's follow-on and lands with #2611, which rebases onto whatever merges here. Typed rollout outcomes referencing the same names are #2135. ## Relationship to #2728 This replaces rather than rebases #2728. That PR is on a fork branch we cannot push to, and #3179's last acceptance criterion is "PR #2728 is rebased or replaced under this issue". The vocabulary here is drawn from what `main` produces now, which has moved since #2728 was drafted — #2017 and #2883 introduced the `agent_request_failed` / `agent_run_error` split. The `status`/`source` fields are dropped for the reason above. ## Tests 16 cases: every name matches one convention and no two differ only by case or separator; every name states the layer that observed the failure; registered, unregistered and namespaced lookup; validation staying silent on known names and warning without raising on unknown ones; the set staying small enough to be a metric label and carrying no occurrence detail; the labels `main` already produces staying registered; and the module importing nothing from the server stack so data tooling can read it without installing a server's requirements. `uv run --extra dev pytest tests/unit_tests -q` → 4348 passed. `test_e2b_provider` and `test_enroot_provider` fail on a clean `upstream/main` too. --------- Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com> Co-authored-by: waple0820 <232305951+waple0820@users.noreply.github.com>

Closes #3179 Gym describes failures with free text and component-local labels, so the same failure reaches a collector, a log line and a metric under three different names and cannot be grouped. ## What a name is, and what it is not A name says what kind of thing went wrong and nothing else. Retryability, replay safety, whether a completed result should be masked, and what to tell a person are properties of the occurrence rather than of the name — they belong to the failure record, the verify response, and `failure_reason`. Keeping them out is the reason the registry carries no per-entry metadata: anything attached to a name goes stale the moment one caller wants to retry what another does not. That is also what the acceptance criterion "no retry-policy metadata that can become stale" asks for. Names are `<domain>_<condition>`, so a reader can tell an unreachable model server from an unreachable sandbox without a second field, and they are low cardinality, so they are safe as a metric label or a span dimension. `failure_reason` never is. ## Unknown names warn rather than raise `validate_failure_kind` returns the name unchanged and logs once. Rejecting an unregistered value would trade a visible wrong label for an invisible dropped failure, which is worse than the problem the vocabulary exists to fix, and it would break components mid-migration. An environment that needs something the shared set should not grow can use `<server>:<kind>`. ## The vocabulary describes the repo, not an intention `agent_run_error`, `agent_request_failed` and `judge_failed` are labels `main` emits today. Tests import those constants from `rollout_collection` and `rollout_reverification` and assert they are registered, so the set cannot drift from the code that produces it. ## Scope This adds the vocabulary only. `failure_kind` reaching `BaseVerifyResponse` is #2608's follow-on and lands with #2611, which rebases onto whatever merges here. Typed rollout outcomes referencing the same names are #2135. ## Relationship to #2728 This replaces rather than rebases #2728. That PR is on a fork branch we cannot push to, and #3179's last acceptance criterion is "PR #2728 is rebased or replaced under this issue". The vocabulary here is drawn from what `main` produces now, which has moved since #2728 was drafted — #2017 and #2883 introduced the `agent_request_failed` / `agent_run_error` split. The `status`/`source` fields are dropped for the reason above. ## Tests 16 cases: every name matches one convention and no two differ only by case or separator; every name states the layer that observed the failure; registered, unregistered and namespaced lookup; validation staying silent on known names and warning without raising on unknown ones; the set staying small enough to be a metric label and carrying no occurrence detail; the labels `main` already produces staying registered; and the module importing nothing from the server stack so data tooling can read it without installing a server's requirements. `uv run --extra dev pytest tests/unit_tests -q` → 4348 passed. `test_e2b_provider` and `test_enroot_provider` fail on a clean `upstream/main` too. --------- Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com> Co-authored-by: waple0820 <232305951+waple0820@users.noreply.github.com> Signed-off-by: Junkeun Yi <jkyi@nvidia.com>

…ard (NVIDIA-NeMo#2611) Closes NVIDIA-NeMo#2608 `BaseVerifyResponse` carries a single scalar for the outcome, so an environment that *knows* it failed — lost session, unavailable judge, OOM-killed container, reset timeout — can only return `0.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 = False` on `SWEBenchWrapperInstanceConfig` (`responses_api_agents/swe_agents/app.py:272`), also `anyterminal_agent/app.py:108` and `anyswe_agent/app.py:180`, documented at `swe_agents/README.md:670`. - `conversational_tool_use/simulation/app.py:112` keeps its own `{"mask_sample": ...}` dict. - `nemo_gym/token_id_capture/delivery.py:49` already defines `MASK_SAMPLE_KEY = "mask_sample"` as a **top-level** rollout field, described there as what "a consumer reads to exclude a rollout from the loss". - NeMo-RL consumes only the first one, through `full_result["instance_config"]["mask_sample"]` (`nemo_rl/experience/rollouts.py:229-241`), which zeroes `loss_multiplier` in `nemo_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 NVIDIA-NeMo#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`) ```python class BaseVerifyResponse(BaseVerifyRequest): reward: float mask_sample: bool = False # is this reward a valid measurement failure_kind: Optional[str] = None # which kind of failure, from NVIDIA-NeMo#3179's vocabulary failure_reason: Optional[str] = None # what happened, for a person ``` The three are orthogonal. `mask_sample` decides usability, `failure_kind` groups in logs and metrics, `failure_reason` explains one occurrence. A sample that degraded but was still measured validly names a kind and stays unmasked. `failure_kind` validates against `nemo_gym.failure_kinds` and warns rather than rejects — an unregistered name is a migration signal, and failing the response over it would replace a visible wrong label with an invisible lost failure. An environment that needs something the shared set should not grow uses `<server>:<kind>`. 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. `swe_agents` and `anyswe_agent` mirror their instance-config flag onto the contract field and keep the old location for one release. `anyterminal_agent` already surfaces it through `TerminalBenchMetrics`. **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 custom `compute_metrics()`, and key-metric selection all skip masked samples. Masked rows stay in the input and are reported as coverage instead — `coverage/measured_rollouts|masked_rollouts` and `coverage/measured_tasks|fully_masked_tasks`. 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_sample` is stripped before profiling, so it is no longer averaged as though the flag were a measurement. `perf_summary` deliberately 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>/reward` is unchanged — it still averages over every rollout, so an existing dashboard reads the number it read before. - `progress/<agent>/reward_unmasked` is that same average with the masked rollouts removed. - `progress/<agent>/masked_pct` is how many were removed. - `progress/<agent>/failed` and `/omitted` count 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. ## Scope Run-level reconciliation is no longer in this PR. NVIDIA-NeMo#3302 implements it for NVIDIA-NeMo#2135, using the materialized inventory and attempt journal to account for omitted and unknown work offline — neither of which this contract can see. What remains here is the per-sample half. ## Relationship to the other failure-handling work - **NVIDIA-NeMo#2552** (merged) landed `failure_reason` and its comment already points here: *"Machine-readable handling belongs to `mask_sample`/`failure_kind`."* This PR is that half. - **NVIDIA-NeMo#2728** owns `failure_kind` and the shared registry, and already reserves `session_lost` with `source="NVIDIA-NeMo#2611"`. No overlap — this PR does not touch `failure_kind`. - **NVIDIA-NeMo#2750** lists this under *Failure fields and artifact integrity*. The design document's rule — *"the producer returns the real result with `mask_sample=True`, a stable `failure_kind`, and a human-readable `failure_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: `AnySweVerifyResponse` inherits `mask_sample` from both `SWEBenchMetrics` and `BaseVerifyResponse`, and the metrics model wins the MRO, so the contract field resolved to `Optional[bool] = None` with 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 plain `reward=0.0` is *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, custom `compute_metrics()` sees only scored samples, and a fully masked run still reports observability coverage. - `tests/unit_tests/test_rollout_collection.py` — the disposition account closes for every combination and the residual never goes negative; 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 `/run` that constructs and serializes the response. Reverting the fix makes it fail with `TypeError: 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_provider` and `test_enroot_provider`, reproduce on a clean `upstream/main` and are unrelated. `pre-commit` clean. --------- Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com> Co-authored-by: waple0820 <232305951+waple0820@users.noreply.github.com>

Closes #3179 Gym describes failures with free text and component-local labels, so the same failure reaches a collector, a log line and a metric under three different names and cannot be grouped. ## What a name is, and what it is not A name says what kind of thing went wrong and nothing else. Retryability, replay safety, whether a completed result should be masked, and what to tell a person are properties of the occurrence rather than of the name — they belong to the failure record, the verify response, and `failure_reason`. Keeping them out is the reason the registry carries no per-entry metadata: anything attached to a name goes stale the moment one caller wants to retry what another does not. That is also what the acceptance criterion "no retry-policy metadata that can become stale" asks for. Names are `<domain>_<condition>`, so a reader can tell an unreachable model server from an unreachable sandbox without a second field, and they are low cardinality, so they are safe as a metric label or a span dimension. `failure_reason` never is. ## Unknown names warn rather than raise `validate_failure_kind` returns the name unchanged and logs once. Rejecting an unregistered value would trade a visible wrong label for an invisible dropped failure, which is worse than the problem the vocabulary exists to fix, and it would break components mid-migration. An environment that needs something the shared set should not grow can use `<server>:<kind>`. ## The vocabulary describes the repo, not an intention `agent_run_error`, `agent_request_failed` and `judge_failed` are labels `main` emits today. Tests import those constants from `rollout_collection` and `rollout_reverification` and assert they are registered, so the set cannot drift from the code that produces it. ## Scope This adds the vocabulary only. `failure_kind` reaching `BaseVerifyResponse` is #2608's follow-on and lands with #2611, which rebases onto whatever merges here. Typed rollout outcomes referencing the same names are #2135. ## Relationship to #2728 This replaces rather than rebases #2728. That PR is on a fork branch we cannot push to, and #3179's last acceptance criterion is "PR #2728 is rebased or replaced under this issue". The vocabulary here is drawn from what `main` produces now, which has moved since #2728 was drafted — #2017 and #2883 introduced the `agent_request_failed` / `agent_run_error` split. The `status`/`source` fields are dropped for the reason above. ## Tests 16 cases: every name matches one convention and no two differ only by case or separator; every name states the layer that observed the failure; registered, unregistered and namespaced lookup; validation staying silent on known names and warning without raising on unknown ones; the set staying small enough to be a metric label and carrying no occurrence detail; the labels `main` already produces staying registered; and the module importing nothing from the server stack so data tooling can read it without installing a server's requirements. `uv run --extra dev pytest tests/unit_tests -q` → 4348 passed. `test_e2b_provider` and `test_enroot_provider` fail on a clean `upstream/main` too. --------- Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com> Co-authored-by: waple0820 <232305951+waple0820@users.noreply.github.com> (cherry picked from commit 4402098) Signed-off-by: Peter Jin <pjin@nvidia.com>

copy-pr-bot Bot temporarily deployed to public August 24, 2026 20:57 Inactive

ananthsub marked this pull request as draft August 24, 2026 20:57

copy-pr-bot Bot temporarily deployed to public August 24, 2026 20:59 Inactive

ananthsub mentioned this pull request Aug 25, 2026

Make rollout failures bounded, visible, and recoverable #2750

Open

8 tasks

waple0820 mentioned this pull request Aug 27, 2026

feat(core): report infrastructure failure from verify, not just a reward #2611

Merged

ananthsub mentioned this pull request Sep 9, 2026

feat(core): define a shared failure-kind registry #3179

Closed

5 tasks

linear-code Bot mentioned this pull request Sep 10, 2026

Oncall queue report - 2026-09-15 PDT #3208

Closed

waple0820 mentioned this pull request Sep 10, 2026

feat(core): define the shared failure-kind vocabulary #3213

Merged

ananthsub closed this Sep 10, 2026

Navigation Menu

Repository navigation

feat: add shared failure-kind registry - #2728

feat: add shared failure-kind registry#2728
ananthsub wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
ananthsub:ansubramania/failure-kind-registry

ananthsub commented Aug 24, 2026 •

edited

Loading

Uh oh!

copy-pr-bot Bot commented Aug 24, 2026

Uh oh!

ananthsub commented Sep 9, 2026

Uh oh!

waple0820 commented Sep 9, 2026

Uh oh!

waple0820 commented Sep 10, 2026

Uh oh!

ananthsub commented Sep 10, 2026

Uh oh!

This branch was previously deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ananthsub commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

copy-pr-bot Bot commented Aug 24, 2026

Uh oh!

ananthsub commented Sep 9, 2026

Uh oh!

waple0820 commented Sep 9, 2026

Uh oh!

waple0820 commented Sep 10, 2026

Uh oh!

ananthsub commented Sep 10, 2026

Uh oh!

This branch was previously deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ananthsub commented Aug 24, 2026 •

edited

Loading