[GDPVal] Classify a missing reference deliverable as a terminal failure - #2796
Merged
Kh4L merged 4 commits intoAug 27, 2026
Conversation
A comparison verify against a reference model that has no deliverable for the
task returned reward 0.0 with judge_response {"error": "reference_missing"} and
no failure class, so the harness persisted it as an ordinary SUCCESS row.
That row carries no battle evidence, and a success row without a usable battle is
rejected outright by the non-final-stage coverage gate:
if set(successful_by_key) - fit_eligible_keys:
return None
which runs before any threshold is consulted. The waiver machinery never sees the
row either, because waivable_failure_classes only applies to rows that failed. So
one missing reference file killed an entire multi-stage run, with no
partial_completion setting able to relax it, and the run reported a stage as
incomplete while every configured gate had passed.
Observed three times on GDPVal-AA-v2, always mapping onto a genuine gap in the
mounted reference sets rather than anything the eval model did.
Stamp the response with _ng_failure_class=reference_missing and mark it terminal.
The row then leaves the success set, lands in the failures sidecar where it is
visible, and is governed by the stage's partial_completion fractions like any
other unusable row. Terminal because retrying cannot make the file appear --
the same reasoning already applied to skipped samples.
Also set extra="allow" on GDPValVerifyResponse. Underscore-prefixed harness keys
cannot be declared as pydantic fields and the default extra="ignore" dropped them
silently, so a verify response that tried to stamp a failure class was serialised
without one. Verified: the keys now survive model_dump.
Note this is a mitigation, not a cure. The underlying defect is incomplete
reference deliverable sets; backfilling them removes the cause. This change stops
a data gap from being mistaken for a model result and from destroying an
otherwise complete run.
Signed-off-by: Alex Gronskiy <agronskiy@nvidia.com>
Three cases the change rests on, none previously covered: - a terminal omission is accepted by _partial_stage_outcome WITHOUT appearing in waivable_failure_classes, because terminal rows are already resolved and so never reach the unresolved-key loop that consults that list. This is why no leaf policy needs to add reference_missing. - forgiveness stays bounded: below min_success_fraction the stage is still rejected. - the hard gate is deliberately left intact -- a row that IS in the success set while carrying no battle evidence remains fatal. The change routes the infrastructure case out of that gate rather than weakening it. Signed-off-by: Alex Gronskiy <agronskiy@nvidia.com>
The nemo_gym edit was redundant. The verify response already stamps the generic
_ng_failure_terminal flag, and _is_terminal_failure falls through to
return bool(record.get(NG_TERMINAL_KEY))
so teaching it a GDPVal-specific class name changed nothing. Reverted
nemo_gym/rollout_collection.py and its test, moved
REFERENCE_MISSING_FAILURE_CLASS into resources_servers/gdpval/app.py, and
relocated the terminal/success tests alongside the rest of the gdpval suite.
Better layering on its own terms -- reference_missing is GDPVal vocabulary and
the harness only needs the generic flag -- and it also fixes CI. Touching
nemo_gym/ flipped unit-tests.yml from the server-only path onto a full run, whose
Test job enforces fail_under=96 while measuring the whole repo. The gdpval
package is ~2300 uncovered statements there because its tests run in the separate
sharded server-suite job, whose coverage is never merged in, so the gate reported
81.55% and failed with 356 passed and 0 failures. Confining the diff to
resources_servers/gdpval keeps the run on the server path, which is how PR #2704
passed.
Signed-off-by: Alex Gronskiy <agronskiy@nvidia.com>
Signed-off-by: Alex Gronskiy <agronskiy@nvidia.com>
agronskiy
added a commit
that referenced
this pull request
Sep 1, 2026
Merges `main` at b793621 ("Super 3.5 vllm model tuning (Qwen3.5-122B-A10B, Inkling-Small)", #2599) rather than the current `main` tip. WHY THIS TARGET b793621 is the commit immediately before 65129dd ("chore(deps): pin openai to 2.44.0", #2456). That PR added class NeMoGymChatCompletionCreateParamsNonStreaming(BaseModel): model_config = ConfigDict(extra="forbid") which makes the proxy reject every agentic chat-completions request that carries `chat_template_kwargs` — the field agentic GDPVal runs use to drive thinking budgets on self-hosted models. Each such request comes back as 422 Unprocessable Entity {"detail":[{"type":"extra_forbidden","loc":["body","chat_template_kwargs"], "msg":"Extra inputs are not permitted","input":{"thinking":true}}]} so the rollout fails before it reaches the model. Merging `main` past 65129dd would take that regression into the GDPVal branch and break the production runs this branch exists to serve. b793621 was chosen because it is the last commit that is regression-free while still giving the branch everything else it needs: * `requires-python = ">=3.13.14"`, so the py313 runtime container stays correct; * `extra="forbid"` appears exactly once in nemo_gym/openai_utils.py, on `NeMoGymResponseCreateParamsNonStreaming` (the pre-existing 2025 Responses-class validation) and NOT on the ChatCompletion class; * the rollout-observability work (bc521f7) is an ancestor. CONFLICTS AND RESOLUTIONS Eight files conflicted; all GDPVal files auto-merged. Every conflict was resolved to keep both sides' behaviour rather than pick a winner. * nemo_gym/cli/env.py, cli/setup_command.py and their tests: `get_venv_path` and `resolve_server_venv_path` are the same function under two names (byte-identical bodies). Canonicalised on devel's name and docstring, kept main's `Path(...)` normalisation of `root_venv_path`, renamed main's call sites and the `monkeypatch.setattr` in test_cli.py, and dropped the now-unused `ROOT_DIR` import. * nemo_gym/openai_utils.py: took main's content-part types — its `video_url` part accepts `Union[str, Dict[str, Any]]` where devel's was dict-only, and it adds `NeMoGymChatCompletionContentPartFileParam` — while keeping devel's comment recording the 422 that motivated adding a video part at all. * responses_api_models/local_vllm_model/setup.py: both sides pin vllm==0.24.0 at this merge target, so the resolution is main's file (which additionally pins flashinfer-python==0.6.12) plus devel's note that 0.24.0 is the first release with MiniMax-M3 support. * nemo_gym/rollout_reverification.py: plain union of both import lists from nemo_gym.rollout_collection; dropped devel's `NG_TERMINAL_KEY as NG_TERMINAL_KEY` re-export idiom, nothing needs it. * nemo_gym/rollout_collection.py: kept both sides' features — main's exporters (`upload_rollouts`/`export_rollouts`/`get_exporters`, replacing the W&B-specific path) and token-capture retirement, devel's dispatch budget, drain margin and `DispatchLatencyTracker`, `kill_shaped` no-persist rows, `_validate_dispatch_concurrency` and `ordered_tasks`. Three resolutions here are semantic rather than textual: - main's `from time import time` SHADOWS the `time` module that devel's `time.monotonic()` calls need, so the module import was kept and main's two bare `time()` calls rewritten as `time.time()`; taking main's side verbatim compiles and then fails at runtime; - the persistence branch takes main's flat `if no_persist / elif failure_class is not None / else` chain with its token-capture retirement, dropping devel's `result_strs.append` (it fed the removed W&B table) and the stale "not the W&B table either" wording in the kill_shaped comment; - devel's `ordered_tasks = [asyncio.ensure_future(...)]` scheduling was kept, because main still passes bare coroutines to `as_completed`, which makes `dispatch_longest_first` a no-op, and the shared code after the conflict references `ordered_tasks`. * tests/unit_tests/test_rollout_collection.py: import union, and devel's version of the fresh-run cleanup test, which is a superset of main's and additionally asserts the aggregate-metrics file is cleared. The fresh-run cleanup in `run_from_config` now unlinks the failures sidecar and the aggregate-metrics file so a re-run cannot inherit retry attempts or published metrics from an older run at the same output path. VERIFICATION `ruff check` and `ruff format --check` with the CI-pinned ruff 0.9.9 are clean across nemo_gym, tests, responses_api_models and resources_servers. resources_servers/gdpval/tests: 386 passed, 11 skipped — the `reference_missing` path (#2796) and `strict_comparison_trials` (#2807) are both intact. tests/unit_tests against an `origin/main` baseline built in a scratch worktree on the same venv: 79 failures on the baseline, 101 here. The 44 new failures are all in test_openai_utils.py (43) plus test_responses_api_model_streaming.py::test_prunes_nested_extra_fields, and are an environment artefact, not a merge defect: b793621 predates #2456 and still declares `openai<=2.7.2`, while the venv has openai 2.44.0 installed. Those tests enumerate the *installed* SDK's item tags (`shell_call`, `apply_patch_call`, `compaction`, `tool_search_call`, ...) and require a Gym union member for each; the pre-2.44 schemas in this tree have none. Re-running the same suite with an openai inside the declared pin (2.7.2 shadowed onto PYTHONPATH, venv untouched) gives 1 failed / 2976 passed — the single failure, test_opensandbox_cleanup.py::test_script_help_runs_by_ direct_path, is also present in the origin/main baseline. So the new-failure set against baseline is EMPTY once the SDK matches the pin, and no failure touches rollout collection, reverification, the CLI or GDPVal. Signed-off-by: Alex Gronskiy <agronskiy@nvidia.com>
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
2 participants
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.
A comparison verify against a reference model that has no deliverable for the task returned
reward: 0.0withjudge_response: {"error": "reference_missing"}and no failure class, so the harness persisted it as an ordinary success row. A success row carrying no battle evidence is rejected outright by the non-final-stage coverage gate in_partial_stage_outcome(if set(successful_by_key) - fit_eligible_keys: return None), which runs before any threshold is consulted; the waiver machinery never sees the row either, sincewaivable_failure_classesonly applies to rows that failed. Net effect: one missing reference file killed an entire multi-stage run, nopartial_completionsetting could relax it, and the run reported a stage incomplete while every configured gate had passed. This PR stamps the response with_ng_failure_class=reference_missingand marks it terminal — retrying cannot make the file appear, the same reasoning already applied toskipped. The row then leaves the success set, becomes visible in the failures sidecar, and is governed by the stage'spartial_completionfractions like any other unusable row. It also setsextra="allow"onGDPValVerifyResponse: the underscore-prefixed harness keys cannot be declared as pydantic fields, and the defaultextra="ignore"dropped them silently, so a response that tried to stamp a failure class was serialised without one.Observed three times on GDPVal-AA-v2 in two days — DeepSeek V4 Flash (
9e39df84/qwen35_397b), Inkling-Small (1752cb53/qwen35_397b), Nemotron 3 Ultra (e222075d/qwen36_35b) — where the first two lost stage 2 entirely at 97.8% and 95.6% coverage and the third survived only because it landed in the final stage, which is never coverage-gated. This is a mitigation, not a cure: the underlying defect is incomplete reference deliverable sets (qwen35_397b212/220,qwen36_35b216/220,gptoss_120b218/220 — 14 missing pairs), and backfilling those removes the cause. A reviewer may reasonably prefer the stricter structural fix of refusing to plan a (task, reference) pair whose reference deliverable is absent, so a run never draws one; that would live in the task-distribution builder and is not attempted here. Tests:test_verify_comparison_missing_referenceandtest_reference_ids_empty_yields_no_referencesextended to assert both keys survivemodel_dump(); newTestReferenceMissingCoverage(terminal omission accepted without being waivable, still rejected belowmin_success_fraction, and an evidence-less success row still hard-rejects — the gate is not weakened) andTestReferenceMissingIsTerminal. Diff is confined toresources_servers/gdpval/; CI green.🤖 Generated with Claude Code