Repository navigation
Conversation
65e0f16 to
a3badb2
Compare
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP WITH CARE
I found four correctness/operability risks and one documentation NOTE. The inline comments include repair boundaries and proposed regression checks.
RISK — Please provide representative real smoke-rollout evidence for the final head. The PR leaves post-rebase rollout validation pending. Its pre-rebase timing tables don't establish that the final transport and routing changes preserve native tool execution, agent output, capture, and verification. AGENTS.md requires real model rollouts and inspection of agent/verifier behavior for behavior-changing agent work.
Please record representative delta-enabled multi-turn/tool runs for Codex, Hermes, OpenCode, and Pi against this head, including configuration, model/task identity, artifact locations, agent/tool behavior, and verifier outcome. Include a disabled control and check that Gym captures complete reconstructed requests with the expected model-call correlation. Preserve the existing native harness and default-disabled behavior; this transport-only change doesn't require a benchmark reward profile. Acceptance is recorded final-head evidence that each affected path reaches verification with intact conversation/capture evidence, rather than latency tables or mocked results alone.
I completed the source review across all 22 changed files and their directly affected paths. I couldn't execute tests or rollouts in this read-only review.
Review generated by codex. This was a full review. Request a full review with /review reset=true.
| # Reuse the same Gym connection even when the CLI opens a new local | ||
| # connection. Requests within this rollout must remain sequential. | ||
| # http.client preserves compressed/SSE data while handling framing. | ||
| connection = self._connection |
There was a problem hiding this comment.
RISK — Give each active HTTP exchange exclusive connection ownership. The helper uses a threaded listener, but every handler uses the same upstream connection without synchronization through response consumption. Hold the first upstream response open and POST from a second local connection: the handlers compete for one active HTTP/1.1 exchange, and the exception path closes that shared socket, so one request can receive a helper-generated 502 while the other's response is interrupted. Both should receive their own complete response. This isn't merely hypothetical unsupported traffic: the shipped OpenCode configuration enables subagent tasks, and the adapter doesn't enforce a single-outstanding-request contract.
Please change HistoryTransport.handle() and connection ownership so a connection belongs to one handler until its response is consumed or closed, and synchronize the corresponding history-state transition. A pool can retain parallel calls; serializing complete exchanges is narrower only if that restriction is acceptable for the supported harness behavior. The concurrency choice needs an explicit contract, so I haven't supplied an exact patch. Preserve sequential keepalive reuse, full/append/edit reconstruction, and the single full resend after explicit resync; don't retry uncertain model executions or interrupted streams.
Proposed acceptance: block the first upstream response, submit a second request through another local connection, then release both. Assert complete, unmixed responses without 502/socket reset, including streamed replies and distinct session headers. Retain the existing sequential test's exact reconstructed requests, suffix-only append, and one-upstream-connection assertions. These checks weren't executed.
| if harness == "opencode": | ||
| config = json.loads(params["env"]["OPENCODE_CONFIG_CONTENT"]) | ||
| config["provider"]["nemo_gym"]["options"]["baseURL"] = local_url | ||
| params["env"]["OPENCODE_CONFIG_CONTENT"] = json.dumps(config) |
There was a problem hiding this comment.
RISK — Preserve OpenCode's literal replacement-prompt serialization. When deltas are enabled, parsing and reserializing OPENCODE_CONFIG_CONTENT turns the deliberately escaped \u007b back into {. A replacement system prompt such as {env:G10_TEMPLATE_VALUE} or {file:missing-prompt.txt} is protected by _serialize_replacement_config() in the sandbox caller. This rewrite removes that protection before the CLI loads it, allowing substitution or a missing-file failure instead of literal instructions. The existing pinned-CLI regression defines this contract.
One bounded correction is to move the existing stdlib-only _serialize_replacement_config() and _dump_with_serialized_member() into the shared helper, keeping the agent's existing serializer name available through an import. Add literal_replacement_prompt: bool(system) and self.config.system_prompt_mode == "replace" to the staged payload, then replace this assignment with:
params["env"]["OPENCODE_CONFIG_CONTENT"] = (
_serialize_replacement_config(config)
if params.get("literal_replacement_prompt", False)
else json.dumps(config)
)The uploaded helper must contain the serializer; it can't import the full agent application. The invariant is selective protection of the generated agent.build.prompt only. Don't escape every brace: other fields, append-mode prompts, and configured prompts without a generated replacement must retain native expansion.
Proposed acceptance: extend the pinned-CLI test through delta-enabled staging and configure_transport(). Assert that build.prompt remains exactly equal to the input, including missing-file markers, Unicode, quotes, backslashes, and literal \u007b; retain ordinary-field expansion. Also retain the provider configuration test's unchanged model/API key and loopback URL assertions. This suggested correction needs verification; I couldn't run these checks.
|
|
||
| def __init__(self, socket_dir: str) -> None: | ||
| super().__init__(socket_dir) | ||
| self.claims = SessionClaims(socket_dir, uuid4().hex, holds=lambda _: True) |
There was a problem hiding this comment.
RISK — Bound the lifetime of model-rollout ownership records. With the global history flag enabled and multiple model workers, every distinct prefixed generation request creates a claim, including ordinary full-history requests. SessionClaims.claim() retains each ID in _mine and writes a claim file. Here, holds=lambda _: True prevents release, and install_rollout_routing() installs no cleanup. N completed one-call rollouts therefore leave N memory entries and N files for the server's entire lifetime, even after the bounded history buffer has evicted their histories. Long-running evaluation/training servers accumulate unbounded routing state.
Please give RolloutRouter/install_rollout_routing() a bounded ownership lifecycle coordinated with active requests and retained worker-local history. Reclaim inactive claims once the worker no longer needs that rollout's local state; don't cap _mine independently of the on-disk claims. Preserve ownership during inference and while usable histories remain. After ownership expires, an append reaching an empty worker must still get the explicit pre-inference resync response, not execute with partial history. The cleanup hook needs coordination with the history owner, so an exact eviction patch isn't clear from the current interfaces.
Proposed acceptance: submit more distinct one-call rollouts than the history retention limit, expire/evict their histories, and assert inactive claim files and in-memory IDs are reclaimed. Keep a blocked in-flight request and an actively appended conversation during cleanup; both must retain their owner. Retain the reconnect/delta tests' complete-history and no-double-append assertions. Source reasoning only; checks weren't executed.
| # A forwarded request must never bounce between workers. | ||
| await _conflict("The rollout's worker ownership changed while routing the request.", scope, receive, send) | ||
| else: | ||
| await self.forward(owner, scope, receive, send) |
There was a problem hiding this comment.
RISK — Recover model ownership after an owner worker exits. Establish rollout R on worker A, stop A while worker B remains alive, then send R's next request to B. The claim still names A, so this branch forwards to its dead socket and receives HTTP 410. Nothing transfers the claim, so even a complete-history resend keeps failing until the whole server restarts. The client's full-history fallback only handles 409 gym_history_resync. This newly strands otherwise recoverable model calls, including ordinary full-history requests; unlike resources-session state, model input history is available from the caller.
Please add model-specific dead-owner recovery in RolloutRouter, leaving stateful-session forwarding's 410 behavior unchanged. After a confirmed missing/refused owner socket before dispatch, atomically transfer the stale claim to a live worker. Its normal reconstruction can then reject an append with gym_history_resync or accept a full request. Preserve single ownership, the forwarding-loop guard, and checkpoint admission/fencing. Don't retry a request after uncertain execution, timeout, or an interrupted response. The claim-replacement fencing needs design verification; blindly unlinking the file would race competing recoveries.
Proposed acceptance: accept a full request on A, shut A down, and send the next append through B. Expect one explicit resync followed by successful full-history dispatch on the new owner. A direct full-history request should recover too, and simultaneous recovery attempts must select one owner. Retain the existing stateful-session worker-loss tests' 410 assertions. These checks weren't executed.
| For sandboxed OpenCode, Pi, Codex, and Hermes, set the agent's | ||
| `history_delta_enabled: true` and the Gym global flag | ||
| `history_delta_enabled: true`. Both default to `false`. Delta transport currently | ||
| requires exactly one model-proxy worker because histories are stored in process |
There was a problem hiding this comment.
NOTE — Suggestion: document the implemented multiworker behavior. The guide says exactly one model-proxy worker is required, but run_webserver() explicitly installs rollout-owner routing for multiple workers, with forwarded-delta coverage. The stale limit unnecessarily directs users to single-worker deployment.
Suggested replacement for the paragraph at lines 425–429:
For sandboxed OpenCode, Pi, Codex, and Hermes, set the agent's
`history_delta_enabled: true` and the Gym global flag
`history_delta_enabled: true`. Both default to `false`. With multiple model-proxy
workers, Gym routes each rollout to the worker holding its in-memory history.
OpenCode, Pi, and Codex also require Python 3 in the task sandbox.Preserve both opt-in flags and the sandbox Python requirement. Static acceptance: the documented worker behavior should match the enabled multiworker branch and forwarded-delta test. This wording doesn't claim recovery from worker loss. I haven't previewed the docs.
Signed-off-by: Stefan Niebler <82932102+stnie@users.noreply.github.com>
Signed-off-by: Stefan Niebler <82932102+stnie@users.noreply.github.com>
Signed-off-by: Stefan Niebler <82932102+stnie@users.noreply.github.com>
Signed-off-by: Stefan Niebler <82932102+stnie@users.noreply.github.com>
Signed-off-by: Stefan Niebler <82932102+stnie@users.noreply.github.com>
Signed-off-by: Stefan Niebler <82932102+stnie@users.noreply.github.com>
Signed-off-by: Stefan Niebler <82932102+stnie@users.noreply.github.com>
Signed-off-by: Stefan Niebler <82932102+stnie@users.noreply.github.com>
Signed-off-by: Stefan Niebler <82932102+stnie@users.noreply.github.com>
Signed-off-by: Stefan Niebler <82932102+stnie@users.noreply.github.com>
Signed-off-by: Stefan Niebler <82932102+stnie@users.noreply.github.com>
Signed-off-by: Stefan Niebler <82932102+stnie@users.noreply.github.com>
Signed-off-by: Stefan Niebler <82932102+stnie@users.noreply.github.com>
a3badb2 to
18bd7b4
Compare
What does this PR do?
Reduce sandbox-to-Gym HTTP traffic for long rollouts by sending newly appended conversation messages instead of resending the full history on every model call.
Adds optional history-delta transport for Codex, Hermes, OpenCode, and Pi. Gym reconstructs the complete request before validation, training capture, and inference.
The first request sends full history. History edits or compaction also trigger a full send. Missing or stale history triggers one full-history resynchronization before inference. Repeated delivery does not append the same delta twice.
Validation
Adds coverage for full and incremental requests, history edits, missing or stale bases, malformed deltas, repeated delivery, and dropped responses.
Focused test suite: 270 passed 3 skipped:
tests/unit_tests/test_model_history_transport.py
tests/unit_tests/test_history_delta.py
tests/unit_tests/models/test_base_responses_api_model.py
tests/unit_tests/server_runtime/test_session_routing.py
responses_api_agents/opencode_agent/tests/test_prompt_serialization.py
tests/e2e/session_routing/test_session_routing_e2e.py
I tested with this PRs changes on and off agains run_harness_conformance.py with real Pi, Codex, OpenCode, and Hermes harnesses against scripted model responses. We compared reconstructed model requests, tool-call sequences and results, final outputs, usage, and verification outcomes. These matched after normalizing generated IDs and paths, with only Hermes’s generated conversation-start timestamp differing
Rollouts
Pre-rebase: (Commit 65e0f16 )
Sandbox Shim path overhead (ms)
Gym: Entry -> full request ready time (ms)
Post Rebase: (Commit 18bd7b4)
Representative post-rebase rollout validation pending.
Compatibility and benchmark impact
Disabled by default. Enable
history_delta_enabled: truein both the agent configuration and Gym’s global configuration.Ordinary requests retain their existing behavior. Model processing and training capture receive reconstructed full histories. The transport does not add retries for model errors, timeouts, or interrupted streams.
No benchmark-result change is intended; the change reduces request payload size.
Checklist