Repository navigation
fix(pi): record the context-overflow outcome in JSON mode - #4588
Conversation
ba0bb07 to
9a3e30b
Compare
|
/ok to test dad6526 |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP — LGTM — no reliability concerns.
Reviewed all three changed files and traced outcome delivery through the sandbox runner and terminal-status handling. The stdout write preserves line framing, and the no-body 400/413 guard retains provider-failure handling. Checked the new long-turn regression coverage and existing finish-tool cases; no actionable findings.
Static review only; tests 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.
yaoyu-33
left a comment
There was a problem hiding this comment.
Thanks for tracking this down. Reviewed at dad6526.
- The fix is right: Pi 0.80.2 calls
takeOverStdout()before it loads extensions, soconsole.logfromoutcome.mjswent to stderr, andWritable.prototype.writereaches the same stdout queue Pi uses for its own lines. We checked this against Pi 0.80.2's source and with a full-pipe probe; lines stay whole. - The no-body regex is byte-identical to Pi 0.80.2's Cerebras rule, so it removes only that rule. We ran 11 SDK-derived error bodies through
outcome.mjs, and each was classified as the PR describes. The new tests fail onmain, as they should.
One inline comment: now that the overflow flag reaches Gym, a run whose overflow recovery fails on a provider error is graded incomplete.
| type: "ng_pi_outcome", | ||
| context_overflow: isContextOverflow(event.message, ctx.model?.contextWindow), | ||
| })); | ||
| context_overflow: !noErrorBody && isContextOverflow(event.message, ctx.model?.contextWindow), |
There was a problem hiding this comment.
responses_api_agents/pi_agent/outcome.mjs:21
Now that this flag reaches Gym, one case changes from failed to scored. With auto_compaction on (the default, app.py:562), an overflow makes Pi call compact(). If that summarization call fails on a provider error (503, dropped connection), Pi 0.80.2 emits only compaction_end with Context overflow recovery failed: 503 …, and no further assistant message_end. So the last ng_pi_outcome still says overflow, and app.py:973-995 grades the run incomplete and scores its partial work. Before this PR it failed and was rerun. Could _sandbox_response grade the run as failed when a compaction_end for an overflow carries an error that is not itself a context limit? The one-attempt "recovery failed" case is a real limit and should stay incomplete. The rollouts in the description used auto_compaction: false, so they would not hit this path.
There was a problem hiding this comment.
Fixed in app.py#L940-L949 and app.py#L986-L989: if Pi's compaction call fails after an overflow, the run now fails with Pi's error and is rerun. Pi's verdict that the retry overflowed again comes with no compaction_start, so that run stays incomplete.
One difference: a compaction call that itself hits the context limit also fails the run. Pi reports that error only as text, and telling it apart would mean copying Pi's overflow patterns into Gym, which outcome.mjs exists to avoid. This may not be rare when a chat template drops earlier reasoning, because Pi's summary prompt keeps all of it; in our 220-task GDPVal Pi run, all 4 overflows recovered through compaction. The README now says so.
Tests: the grading test covers both compaction cases, and the real-Pi test now answers Pi's summary request and adds a 503 one, which fails the run with no retry.
There was a problem hiding this comment.
Updated in app.py#L1014-L1020 and app.py#L1120 after rebasing onto #4834 and #4739. Pi now reports provider errors with a cause instead of raising, and the default policy verifies a 400. So the run is now reported with the compaction call's error, and its cause is unknown, which the default policy retries. Merged as-is, it would have been reported with the overflow's 400 and graded. The check also now covers a no-body 400 that Pi compacts on but Gym does not count as an overflow.
There was a problem hiding this comment.
Verified at 55d87b1: a failed compaction call after an overflow now reports the run as failed (failure_kind: unknown), and the new rows catch a revert. Thanks.
|
/ok to test b063996 |
Signed-off-by: Abhishree <athittenaman@nvidia.com>
Also correct the outcome.mjs comment, and test the long turn through the real sandbox session helper instead of a hand-built Pi command. Signed-off-by: Abhishree <athittenaman@nvidia.com>
When the compaction call fails (a 503, a dropped connection), Pi stops without retrying the overflowed call. Report the run as failed with the compaction's error, which stays unclassified and is retried by default, rather than grading it as the model's context limit. Signed-off-by: Abhishree <athittenaman@nvidia.com>
b063996 to
55d87b1
Compare
|
/ok to test 55d87b1 |
yaoyu-33
left a comment
There was a problem hiding this comment.
Thanks for the quick fix. Re-reviewed at 55d87b1.
- The case from our earlier comment is fixed: when Pi's overflow compaction call fails (a 503, a dropped connection), the run is now reported as failed with
failure_kind: unknown, not gradedincomplete. We checked it against Pi 0.80.2's event order, in the PR's new test rows (two of which fail with the previousapp.py), and by driving the real Pi 0.80.2 CLI through ten recovery scenarios: compaction OK, retry overflowing again, a second recovery failing, and others. All ten grade as intended at this head. pytest responses_api_agents/pi_agent/tests: 313 passed, 31 skipped (Linux-only, including the new real-Pi 503 case, which we could not run here), and 1 failure from the OpenSandbox SDK missing locally, unrelated.
One small test suggestion inline.
Signed-off-by: Yu Yao <54727607+yaoyu-33@users.noreply.github.com>
|
/ok to test 3d9893b |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/39885640827 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/39902348627 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/39949340575 |
What does this PR do?
outcome.mjstells Gym whether a Pi run ended on a context-length error. After each model reply it writes anng_pi_outcomeline. The sandbox session uses that line to grade such a run as incomplete, with its partial work, instead of failed (#3626).The line never reached Gym. In JSON mode, Pi 0.80.2 points
process.stdout.write, whichconsole.loguses, at stderr before it loads extensions (takeOverStdout). Gym reads Pi's events only from stdout; the runner sends stderr to a log that Gym does not read. As a result:context_overflowwas always false;incomplete. Before feat(agent): report native causes for shared grading policy #4834, it failed with "Pi agent failed: …" and its partial work was never graded. Since feat(agent): report native causes for shared grading policy #4834, it is reported as a failed 400. The defaultagent_outcome_policyverifies that, but a policy that retries or fails 400s does not.Changes:
write, which still reaches stdout as one whole line. A directwriteSync(1, …)is not enough. After a long turn, Pi can still be writing a line larger than the 64 KiB pipe buffer, so a direct write splits that line or fails with EAGAIN.errorfield) no longer counts as an overflow in Gym. Pi 0.80.2 counts it as one, by a rule meant for Cerebras; Pi 0.86.0 applies that rule only to Cerebras. Gym's model servers can pass any provider's error through. Without this check, an ordinary 400 with such a body, like SGLang's flat error body, would count as a model limit, which is always graded, instead of a failed call that the outcome policy decides on.auto_compactionon (the default), Pi recovers from an overflow by compacting, then retrying once. If its compaction call fails, for example on a 503 or a dropped connection, Pi stops without the retry. Such a run is now reported as failed with Pi's error ("Context overflow recovery failed: …") and anunknowncause, which the default policy makes eligible for an episode retry. Without this change, it is reported with the overflow's 400 and graded. The same holds when Pi compacts after a no-body 400, which Gym does not count as an overflow. When the retry overflows again, Pi gives up with its own verdict, and the run is still gradedincomplete. A compaction call that itself hits the context limit is also made retryable: Pi wraps that call's error in its own message, which Pi'sclassify_outcomedoes not parse.bash, then returns a context-length 400 to every later call with tools. Pi compacts, retries and overflows again. The test checks that the run is gradedincompletewith no error. When Pi's compaction call gets a 503 instead, it checks that the run is reported as failed with Pi's error and anunknowncause. For this, the existing real-Pi test helper intest_sandbox.pynow accepts a custom model.test_outcome.pyreuses that file's pinned-version check, so its tests keep running after a Pi version bump. Its loader test now also covers the no-body 400 and 413.No issue: the bug was found while adding MCP tool grants to sandbox sessions in #4587 (merged). That PR's
emitEventingym_mcp.mjsuses the same write.outcome.mjskeeps its own copy because only sessions with MCP grants getgym_mcp.mjs. The wording ofgym_mcp.mjs's comment gets the same fix in a follow-up.Validation
main4e8f24d (which has feat(agent): report native causes for shared grading policy #4834 and fix(pi): report session errors and retain ownership #4739), with real Pi 0.80.2 on Node 22.19.0 (the sandbox's Node) and Node 24.21.0.pytest responses_api_agents/pi_agent/tests: 343 passed, 2 skipped on both Node versions (both skipped tests need Python 3.8).main: 338 passed, 2 skipped.tests/unit_tests/test_agent_outcome.py,tests/unit_tests/test_failure_kinds.pyandenvironment_servers/single_agent_turn/tests: 156 passed.main's code: both runs endfailedwith the overflow's 400, reported asmodel_request_error400, which the default policy verifies. In the 503 case, the compaction's 503 is not reported at all. With this PR, the first run isincompletewith no error. The second is failed with Pi's 503 error and anunknowncause, which the default policy makes retryable. The same holds for a no-body 400 (an empty body, or SGLang's flat body) whose compaction call gets a 503. Onmain, that run is reported as a failed 400 and verified.main'sconsole.log;process.stdout.write, called at write time or bound when the module loads;writeSync(1, …), with or without retrying on EAGAIN;compaction_start), when the check is not reset after each compaction, when the run is classified from the overflow's 400 instead of the compaction's error, and when the check covers only errors Gym counts as an overflow. Without the check, or with that classification, the long-turn test's 503 case fails too.maincbc7288 (before feat(agent): report native causes for shared grading policy #4834), with an SGLang-style flat error body, the run fails with "Pi agent failed: 400 status code (no body)". An empty body, a{"detail": …}body and an empty 413 fail the same way. Without the no-body check, all of these were gradedincomplete. A context-length 400 in OpenAI's nested form is gradedincompleteeither way.pre-commit run --all-files: passed.main4e8f24d. The codex, kilocode and Publish checks fail there, as they do onmain. Hermes passed on a re-run, after one execution error in a scenario this PR does not touch.Rollouts
Smoke on an internal cluster: one GDPVal task, run with
benchmarks/gdpval/pi.yamlfrom the open draft #4589, onmaincbc7288 + #4589 + this PR.auto_compaction: false.--max-model-len 49152. Pi asks for 32,768 output tokens per call, so the first call whose prompt passes 16,384 tokens gets vLLM's context-length 400.judge_deliverables: false, every row gets reward 0 andmask_sample: true.outcome.mjsvllm_modelwithpropagate_context_overflow_errors: trueincomplete, no error; no failure rowsopenai_modelwithpropagate_upstream_http_status_codes: [400]incomplete, no error; no failure rowsvllm_model, as abovemainopenai_model, as abovemain{"error": {"message": "This model's maximum context length is 49152 tokens. However, you requested 32768 output tokens and your prompt contains at least 16385 input tokens, …"}}.incompleterun asrollout_ended_on_failed_model_call.mainrows ran before feat(agent): report native causes for shared grading policy #4834. Onmain4e8f24d, such a run is reported as a failed 400 instead, which the default policy verifies (see the long-turn test above).vllm_modelrun used theoutcome.mjsof dad6526, which this PR still carries; the other four used an earlier revision with the same code and different comments.auto_compaction: false, these runs did not compact, so they do not cover the compaction change; the real-Pi test above does.Compatibility and benchmark impact
This affects Pi sandbox sessions whose model server passes a context-length 400 to Pi with its message:
inference_providerandswitchyard_model, by default;vllm_modeland the servers built on it (local_vllm_model,local_vllm_model_proxy,vllm_model_with_compaction) withpropagate_context_overflow_errors: true(the GDPVal README in feat(gdpval): run Pi in place of Stirrup #4589 sets it);openai_modelandlitellm_modelwith 400 inpropagate_upstream_http_status_codes.With their default settings,
vllm_model,openai_modeland the servers built on them see no change, and neither doesazure_openai_model.In those sessions, a run that ended on a context-length error that compaction could not fix is now
incomplete, and it is always verified. This includes runs whosecontext_windowis set above the served model's limit. Onmain, the run is reported as a failed 400. The defaultagent_outcome_policyverifies that too, so the default policy grades the run either way, but a policy that retries or fails 400s drops it. Rollout health flags these runs asrollout_ended_on_failed_model_call.A 400 or 413 that the SDK reports as "status code (no body)" (an empty body, or JSON with no
errorfield) still ends as a failed call, as onmain, even when it is a real overflow. The outcome policy decides whether to verify it. An example is SGLang's flat error body passed throughinference_provider.With
auto_compactionon, an overflow whose compaction call fails is now reported with anunknowncause, whatever that call's error, so the default policy makes it eligible for an episode retry. That includes a no-body 400 that Pi treats as an overflow. Onmain, such a run is reported with its 400 and verified.Checklist
unknowncause, which makes it eligible for an episode retry by default.pre-commit run --all-files) (so CI lint/format checks pass).git commit -s) (so the DCO check passes).