Repository navigation
Conversation
719f8ce to
66bb6a5
Compare
|
/ok to test 66bb6a5 |
github-actions
Bot
added
the
sla:triage-overdue
|
/claude review |
|
SHIP — clean, well-scoped, well-tested. This PR decomposes proof_judge's zero-rewards into a named Reward math is unchanged. The refactor Request contract — Response schema — Tests — good coverage: parametrized cases assert reward + One NOTE inline re: the "422 if empty" comment being slightly inaccurate ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| class ProofWithJudgeVerifyRequest(BaseVerifyRequest): | ||
| problem: str = "" | ||
| # Force a 422 error if the problem is empty. |
There was a problem hiding this comment.
NOTE: The comment says "Force a 422 error if the problem is empty," but problem: str only rejects a missing field — an empty string "" still validates. If you actually want to reject empty problems, use problem: str = Field(min_length=1). Otherwise reword the comment to "if the problem is missing." Not a correctness issue given all in-repo datasets supply a non-empty problem.
github-actions
Bot
removed
the
sla:triage-overdue
|
/ok to test 7307b03 |
|
/claude review |
|
SHIP — no reliability concerns. Reviewed
No async/HTTP-client, public-API, dependency, or config-convention issues. Reward aggregation math is untouched. |
There was a problem hiding this comment.
I think this is not needed anymore, now that we set problem: str = Field(min_length=1)
There was a problem hiding this comment.
Oops. I knee-jerk addressed the claude review without thinking. Fixed!
|
This PR looks correct in a sense that it will do its job without breaking anything else. However, the previous PR #2113 introduced a unification on how we handle judge failures, so making changes just to one harness does not seem right. As for now, the unified approach assumes that unparsable responses are not judge failures. This means that the judge had a fair chance of giving a verdict (i.e. we received a response from the API) and it just happened to be not what we expected. It was not considered a judge failure because judge failures currently mean the call itself failed (eg. due to rate limiting, auth issues, timeouts, HTTP errors) so we never got a verdict at all. We could change this approach to include unparsable responses as judge failures but this change should be applied uniformly, to other benchmarks as well. |
4c2677d to
569a6cf
Compare
Done in #2552; merged. |
| # A format reason means the policy failed to follow the format and received an automatic 0. | ||
| # "judge_unparseable" means that the judge itself failed to produce a parseable score. | ||
| # Distinguishing between these cases is needed for judge error-proofing. | ||
| failure_reason: Optional[str] = None |
|
This proof-judge-specific work now belongs under #3180. The common contract distinguishes a valid wrong answer, a completed result whose reward should be masked (#2608), and a judge call that produced no usable result (#2135). Please keep proof-judge metrics and tests, but avoid introducing a second failure shape or reward-zero sentinel; also resolve the current blocked check before landing. |
…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>
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
569a6cf to
3806d0f
Compare
|
/ok to test 3806d0f |
@ananthsub this has been updated as requested |
svcnvidia-nemo-ci
added
area:environment
A zero reward from a judge model has three distinct causes that are currently indistinguishable:
The middle case leads to severe training run degradation, should the judge model become corrupted - which is not uncommon.
This PR merely surfaces the failure reason, emitting
judge_unparseableif the problem is with the judge, allowing upstream training infrastructure to decide how to best handle such judge degradation.