[GDPVal] Enforce complete comparison trial panels - #2807
Merged
Kh4L merged 2 commits intoAug 27, 2026
Merged
Conversation
Add an opt-in strict comparison contract for reportable ELO runs. Reject missing artifacts, failed matchups, invalid verdicts, and incomplete vote panels before results enter the resume cache. Keep legacy behavior as the default and fingerprint strict runs separately. Signed-off-by: Serge Panev <spanev@nvidia.com>
…ng' into spanev/gdpval-strict-comparison-trials # Conflicts: # resources_servers/gdpval/app.py
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
1 participant
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.
Add an opt-in strict comparison contract for reportable ELO runs. Reject missing artifacts, failed matchups, invalid verdicts, and incomplete vote panels before results enter the resume cache. Keep legacy behavior as the default and fingerprint strict runs separately.
Checklist
pre-commit run --all-files) (so CI lint/format/copyright pass).git commit -s) (so the DCO check passes).