Repository navigation
feat(gdpval): run Codex in place of Stirrup - #4591
Conversation
fd1cb9e to
53c4a64
Compare
|
Just curious while scanning through some new PRs: I noticed #4587 has Pi stop after a successful finish, while Codex here can keep working afterward. Is that intentional, or will be unified later? |
|
@yuhezhang-ai Intentional for now. Pi can end a run when a tool result sets |
9658fe6 to
37443f5
Compare
37443f5 to
2eb11d4
Compare
|
/ok to test 2eb11d4 |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
HOLD — 1 RISK: the new Codex preset cannot run against the reviewed head because its declared session-MCP prerequisite is absent.
Reviewed all seven changed files and their directly affected preparation, agent-session, MCP, and verification paths. The harness-neutral preparation preserves the Stirrup row contract, and Pi’s configured finish behavior matches its adapter. Validation reported against a tree merged with #4590 does not establish compatibility of this immutable head.
Static review only; tests and rollouts were not executed.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex. This was a full review. Request a full review with /review reset=true.
| agent_server: | ||
| type: responses_api_agents | ||
| name: gdpval_codex_agent | ||
| resources_tool_transports: [mcp] |
There was a problem hiding this comment.
RISK — Land the native Codex MCP prerequisite before enabling this preset.
Failure / blast radius: With otherwise valid sandbox/model settings, every gdpval/codex episode fails during agent setup. This transport makes the environment construct a required MCPToolAccess, but CodexAgent._seed_agent_session_state rejects every required grant with HTTP 422. Thus none of the 220 tasks reaches policy execution or verification. The description acknowledges #4590, but that implementation is absent here.
Correction: Rebase onto the complete prerequisite before merging, or include that implementation. Do not simply remove the rejection or make the grant optional: the native activation also calls _build_config without MCP servers, so that alone would run without finish and the web tools. The required invariant is that supported required MCP grants are accepted and configured with their authenticated URLs/headers before inference, while unsupported required HTTP grants still fail closed. The prerequisite must also declare and apply this preset’s mcp_tool_timeout_seconds; CodexAgentConfig currently has no such field. This needs the complete adapter change, not a YAML-only patch.
Preservation / acceptance — proposed checks, not executed: Add a test using this resolved preset and the environment-generated GDPVal grant: with sandbox installation mocked, session seeding must return 200, and generated TOML must contain the GDPVal MCP URL, session-token headers, required status, and 900-second tool timeout. A scripted episode must discover all four tools, call finish, close normally, and persist its submitted file through /verify; preserve the documented Codex behavior that finish itself does not terminate the run. Retain the existing no-grant borrowed-sandbox case’s 200 response and disconnect-without-stop assertions, plus the required-direct-HTTP case’s 422-before-connection assertion in the Codex session tests. Keep Pi’s terminal-finish behavior and Stirrup’s request tool list unchanged.
There was a problem hiding this comment.
Fixed in 79076a4e5: this branch now carries #4590 at cd4ace0 as one commit, so seeding accepts the required GDPVal MCP grant and writes it to Codex's config with its URL, session-token header, required = true and a 900 s tool timeout. Checked end to end with Codex 0.144.4, and on 3 GDPVal tasks with a real model. I'll drop that commit by rebasing onto main after #4590 merges.
2eb11d4 to
ff00cd9
Compare
|
/ok to test ff00cd9 |
|
/review reset=true |
|
🌿 Preview your docs: https://nvidia-preview-pr-4591-cc82ea22bf03.docs.buildwithfern.com/nemo/gym |
75a890e to
cc82ea2
Compare
|
/ok to test cc82ea2 |
yaoyu-33
left a comment
There was a problem hiding this comment.
Re-reviewed at cc82ea2, the Codex commit on top of #4589 (f0c065e) and #4590 (452cfc2).
- The config test now maps
codextoCodexAgentConfigand builds each config, so the validators run. Withmcp_tool_timeout_seconds: 20000incodex.yaml, thecodexcase fails as it should.test_harness_configs.pyandtest_prepare.py: 11 passed. - The branch merges cleanly now. The plan to rebase to the single Codex commit after #4589 and #4590 merge sounds right.
No further comments from us.
There was a problem hiding this comment.
Approving at cc82ea2, the Codex commit. codex.yaml resolves, validates against CodexAgentConfig, and fits the episode timeout. The README's Codex claims we checked against Codex rust-v0.144.4 hold, and the config test now covers codex. The branch still carries #4589 and #4590, which are approved separately; please merge after them and rebase to this commit as planned.
Two non-blocking README corrections are inline, both about how Codex runs are scored, since readers will compare these numbers with Pi's and Stirrup's.
| request 30 times before failing with a "high demand" error. | ||
| - `finish` does not end a Codex run: Codex continues until the model replies without a tool call, and a later | ||
| `finish` or `abandon_task_finish` replaces the recorded call. The files are collected when the run ends, so a run | ||
| that fails after `finish`, for example on a context-length error, is not judged. |
There was a problem hiding this comment.
benchmarks/gdpval/README.md:489
Non-blocking: with propagate_upstream_http_status_codes: [400], a context-length failure after finish is judged, not skipped. Codex classifies the upstream 400 as model_request_error with status_code: 400 (codex_agent/app.py:567-575), and the environment server's default outcome policy sends 400 to verify (single_agent_turn/app.py:121-125). So the run is judged on the recorded finish, or as an empty submission without one. Could this sentence say that? Or, if skipping these runs is the intent, codex.yaml would need an agent_outcome_policy override.
There was a problem hiding this comment.
Thanks @yaoyu-33, will add in the follow-up PR along with others
There was a problem hiding this comment.
Addressed in #5076: README.md:490-495. With [400], the run is now described as judged on its last finish or abandon_task_finish, or as an empty submission, and the default HTTP 500 case as a failure that --resume reruns.
| '++policy_model.responses_api_models.openai_model.propagate_upstream_http_status_codes=[400]' | ||
| ``` | ||
|
|
||
| - The rows, the task tools and the deliverables directory work as for Pi, and these are not AA v2 scores either. |
There was a problem hiding this comment.
benchmarks/gdpval/README.md:473
Non-blocking: could the Codex bullets say how a timeout is scored, as the Pi section now does? Codex sets no stop_reason: wall_time, so GDPVal's agent_timeout mask never applies. A Codex run that reaches timeout is judged on its last finish, with the files as they are when it stops, or as an empty submission without one, and it is not retried. The PR body says this, but the README doesn't.
There was a problem hiding this comment.
Will add in the follow-up PR along with others
There was a problem hiding this comment.
Addressed in #5076: README.md:499-504. The Codex bullets now say how a timeout is scored. One change from your note: a timed-out run is rerun when Codex also reported an error during the run. A Pi run that times out while still working is now masked, as Stirrup's is, so the bullet ends with that difference.
Add benchmarks/gdpval/codex.yaml and README steps for running GDPVal with Codex, including the model server settings Codex needs. Signed-off-by: Abhishree <athittenaman@nvidia.com>
cc82ea2 to
5916dea
Compare
|
/ok to test 5916dea |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/39997461092 |
What does this PR do?
Adds
benchmarks/gdpval/codex.yaml, which runs GDPVal with Codex in place of Stirrup, using the same resources server, judges and multi-stage driver, so Codex can be compared with Stirrup and Pi on the same tasks and scoring:/rootas its working directory.finish,abandon_task_finish,web_searchandfetch_web_pageover MCP:expose_tools_over_mcp: trueon the resources server andresources_tool_transports: [mcp]on the environment server. The model sees them asmcp__gdpval_resources_server__<tool>.codex.yamlincludesconfig.yaml, removes the Stirrup agent withgdpval_stirrup_agent: nulland pointsgdpval_environment_serverat Codex, aspi.yamldoes.concurrency: 32, aspi.yamlsets it: at least the environment server'smax_concurrent_episodes;timeout: 10800for the Codex process, inside the environment server's 18,000 s episode timeout;mcp_tool_timeout_seconds: 900. A Codex MCP call that times out keeps running on the server and holds back the next call to it (such asfinish), so the timeout stays well above a slow web call.openai_model'sextra_body, as in the run below. It also passes vLLM's context-length error to Codex as an HTTP 400, so such a run fails within seconds with vLLM's message; Codex does not compact on it.finishdoes not end a Codex run. Codex continues until the model replies without a tool call; a laterfinishorabandon_task_finishreplaces the recorded call, and/verifycollects the files at the end.It builds on #4589 and #4590, both now on
main: #4589 adds the harness-neutral rows, the harness config test and the README section, and #4590 lets Codex sandbox sessions use MCP tool grants (without it, everygdpval/codexepisode would fail at agent setup with HTTP 422). This PR's commit (5916dea77) also addscodexto that config test, which builds each agent's config class, so a misspelled key or a value that breaks a validator (for examplemcp_tool_timeout_secondsabovetimeout) fails the test.It also relies on #3902, now on
main, which keeps the task's HOME, so a file the model saves under~is still in the sandbox at/verify.No issue: this ports athitten#1 (Codex × GDPval on the yaoyu-33#8 prototype) to the merged #4178 design.
Validation
At 5916dea, rebased onto
mainab25f45 after #4589 and #4590 merged.git range-diffshows the commit unchanged from cc82ea2, the approved head.pytest resources_servers/gdpval/testsin a fresh venv from the server'srequirements.txt, withcryptographyinstalled: 864 passed, 1 skipped (it needs Stirrup installed). A misspelled key or amcp_tool_timeout_secondsabovetimeoutincodex.yamlfails the harness config test.pytest tests/unit_tests/test_benchmarks.py tests/unit_tests/test_discovery.py tests/unit_tests/test_registry.py tests/unit_tests/test_cli.py: 287 passed, 1 failed. The failure is a workstation issue: its hostname resolves to 127.0.1.1, so four configs this PR does not touch (apex_shortlist and hle) are skipped bymain's sandbox-host check.pytest responses_api_agents/codex_agent/tests tests/unit_tests/test_agent_outcome.pywithCODEX_TEST_BINset to Codex 0.144.4: 409 passed, 1 skipped (it needs Python 3.8).gym eval run's argument handling, givesopenai_modelthe model flags, theextra_bodyand the 400 setting, and resolves with no warnings.gym eval prepare --benchmark gdpval/codexwrites the 220 harness-neutral rows.main's GDPVal resources server over MCP with Gym'slocalsandbox provider, and the realopenai_modelserver configured by the README command, in front of a scripted Responses engine. The host's Node and Codex stood in for the in-sandbox install.mcp__gdpval_resources_server__<tool>.finish,abandon_task_finish, andfinishwithweb_searchin one turn each ended after one closing model call, and/verifysavedfinish_params.jsonand the files. A laterfinishorabandon_task_finishreplaced the earlier one./verifywas not called; with the default HTTP 500, Codex retried the request. A run that timed out afterfinishreached/verify, which saved its submission.pre-commit run --all-files: passed, with no changes.Rollouts
Smoke of the previous head ff00cd9 (
main4c66403 with #4589, #4590 at cd4ace0 and this PR's commit), on a GPU cluster with the same model as the runs below. It used the README'sgym eval prepareandgym eval runcommands with the placeholders filled in,--limit 3, and its own output, log and deliverables paths. The cluster has no OpenSandbox, LibreOffice or Tavily key, so a localenv.yamlran the task sandboxes under Apptainer with the GDPVal image and turned judging off, andTAVILY_API_KEYwas a placeholder (no run calledweb_search).finishonce over MCP, and/verifysaved the files andfinish_params.jsonundertask_<id>/repeat_0/.finish, Codex made one more model call in each run: a closing message with no tool call.The earlier smoke and the 220-task run below used #4178 before it merged. Smoke on #4178 at bf9f8db, merged with
main, an earlier revision of #4590 and #4586 (LibreOffice command check), 3 tasks with the same model and sandbox as the full run below and this config's defaults:finishonce over MCP, and/verifysaved the files,finish_params.jsonand the reference files undertask_<id>/repeat_0/.finish, Codex made one more model call in each run: a closing message with no tool call.220 GDPVal tasks with Codex 0.144.4, from #4178 at fd1cb9e merged with
mainplus an earlier revision of #4590, the HOME change from #3902, and #4586:openai_modelwith the README command'sextra_body(temperature 1.0, top_p 0.95, 32,768 output tokens per call) andpropagate_upstream_http_status_codes: [400].pi.yaml.openai_model's 1,800 s limit per model call (fix(openai): jittered backoff, Retry-After and a default provider timeout #4786); its slowest call took 335 s.judge_deliverables: false, then the GPT-5.5 / Gemini 3.1 Pro / Opus 4.8 panel, with:gym eval reverify.Scores, compared with the earlier Codex run (athitten#1) counted the same way:
comparison/eval_elo)/verify. The 2 runs that did not callfinishwere judged as empty submissions.finish(one of them twice).finish, and one hit the 3,600 s timeout.pkill -f python3, which killed the Codex runner.web_searchcall worked, because the run used a placeholder Tavily key.fetch_web_pageworked.The setup is not identical to the earlier run: harness-neutral prompt,
finishover MCP, a page-fetch tool, compaction at 196,608 tokens. So part of the difference comes from these setup changes.Compatibility and benchmark impact
config.yamland the Stirrup path are unchanged.timeoutis judged on what it submitted before the timeout (nothing if it had not calledfinish) and is not retried. Stirrup reports the same event withstop_reason: wall_time, which GDPVal masks and retries. One run above timed out.web_searchneeds a validTAVILY_API_KEY:gym eval runstops if it is unset, and with an invalid key each search returns an error to the model, as in the run above.Checklist
pre-commit run --all-files) (so CI lint/format checks pass).git commit -s) (so the DCO check passes).