fix(sandboxed-gym): frame rollout responses and honour an offline environment - #1929
Merged
Merged
Conversation
Contributor
|
…ironment Two fixes landed on NeMo-RL's fork of this code while the branch that deletes that fork sat unmerged. Neither exists here, so completing that consolidation would drop both -- the exact drift consolidating is meant to end. Response framing. `/rollouts/run` emits a whitespace heartbeat while a batch runs, because the body can take minutes and the status line has to leave first. The body went out under HTTP/1.0 with neither a length nor chunking, delimited only by the connection closing -- and the OpenSandbox proxy does not wait for that close. It returned 200 with the lone " " heartbeat as the finished response, and the caller failed with `JSONDecodeError: Expecting value: line 1 column 1 (char 0)`. Chunked framing is the delimiter and it is 1.1-only, so the handler moves to 1.1 and takes only that half: `parse_request` declines connection reuse, which this server gains nothing from -- a rollout runs for minutes, so a handshake per request rounds to zero -- and which would cost a real hazard, because an unread request body (the 413 path declines to read one on purpose) is what the server would otherwise parse as the next request line. Every response is now explicitly framed through one of `_send_empty`, `_send_body`, `_send_json` or the chunked path. A 1.0 caller cannot parse chunks, so it gets the batch buffered behind a Content-Length instead: a late answer is a loud failure where a truncated one is a wrong reward. Offline environments. `nmp/rl`'s grpo_config already sets `environment_offline` on a manifest whose wheelhouse is a complete closure, and this package had no such field. `NemoGymSandboxedConfig` is `extra="forbid"`, so that key does not pass through unnoticed -- it fails validation, and a sandboxed run with an offline environment could not start once NeMo-RL consumes this package. The field now exists on both the caller dialect and the serve config, reaches the host as `NMP_ENVIRONMENT_OFFLINE`, and sets `UV_OFFLINE` alongside the wheelhouse's `UV_FIND_LINKS`. Index fallback is otherwise retained, which is a liability only when an index is configured but unreachable: uv then fails to resolve `uv venv --seed` rather than falling back to `--find-links`. Not derivable from the package format -- a wheels-v1 package can ship wheels and still need an index for its agent -- so the caller decides, and an operator's own `UV_OFFLINE` wins. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
aalgo-583-backport-rl-host-fixes/schapman
branch
from
September 10, 2026 13:18
2763066 to
cf0ac5d
Compare
SandyChapman
marked this pull request as ready for review
September 10, 2026 13:26
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughChangesSandboxed Gym now supports Sandboxed Gym runtime
Sequence Diagram(s)sequenceDiagram
participant NemoGymSandboxedConfig
participant orchestrator
participant gym_host_runtime
participant uv
NemoGymSandboxedConfig->>orchestrator: environment_offline=True
orchestrator->>gym_host_runtime: NMP_GYM_OFFLINE=1
gym_host_runtime->>uv: UV_OFFLINE=1
Merge Risk: ⚪ Minimal · up to This change adds offline sandbox configuration and explicit rollout response framing while removing the obsolete runtime fallback. Focused tests pass, and no concrete merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
4 tasks
`gym_host.sh` fell back to `$root/nemo_rl/environments/sandbox/gym_host_runtime.py` when no runtime was given. That path is NeMo-RL's fork of this file, which is being deleted as RL moves onto this package, so the fallback would name a file that cannot exist. Unreachable through the package's own API -- `default_gym_host_entrypoint` always passes the runtime as argv[4] -- but reachable by anyone invoking the script with three arguments, and stale either way: `gym_host_runtime_path` in entrypoint.py already falls back to the `packages/` path alone, with no RL branch. The shell now matches it. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
JashG
approved these changes
Sep 11, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Sep 11, 2026
5 tasks
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.
Summary
Two fixes landed on NeMo-RL's fork of this code while the branch that deletes that fork sat unmerged. Neither exists here, so completing that consolidation would drop both — the exact drift consolidating is meant to end. This ports them.
Unblocks soluwalana/RL#25, which currently cannot rebase: three of its five conflicts are files carrying these fixes.
Upstream, for side-by-side review
These are ports, not cherry-picks —
runtime/gym_host_runtime.pyhere has diverged from the fork (it carriesSG_EXAMPLE_IDand the model-call capture, which the fork does not). Compare against:eab835b1environment_offline→UV_OFFLINE5e4346e9,19720f30environment_offlinethrough to the actors9932dc8aTwo differences a reviewer diffing against the fork will notice, both deliberate:
configure_environment_wheelhouse(env_root, *, offline=...)takes the flag as an argument; here the runtime readsNMP_ENVIRONMENT_OFFLINEinside_install_wheels_v1_dependencies, which is where this package already stages the wheelhouse and setsUV_FIND_LINKS.test_connections_are_never_reused,test_rollouts_run_sends_a_length_to_an_http_10_caller,test_an_offline_environment_takes_uv_off_the_indexand the rest name and enforce it.Changes
Response framing
/rollouts/runemits a whitespace heartbeat while a batch runs, because the body can take minutes and the status line has to leave first. The body went out under HTTP/1.0 with neither a length nor chunking, delimited only by the connection closing — and the OpenSandbox proxy does not wait for that close. It returned 200 with the lone" "heartbeat as the finished response, and the caller failed withJSONDecodeError: Expecting value: line 1 column 1 (char 0).Chunked framing is the delimiter and it is 1.1-only, so
Handlermoves to HTTP/1.1 and takes only that half:parse_requestsetsclose_connection = True. Reuse gains this server nothing — a rollout runs for minutes, so a handshake per request rounds to zero — and costs a real hazard: an unread request body (the 413 path declines to read one on purpose) is what the server would otherwise parse as the next request line._send_empty,_send_body,_send_json, or the chunked path terminated by the zero-length chunk.Content-Length. A late answer is a loud failure where a truncated one is a wrong reward.Offline environments
nmp/rl'sgrpo_configalready setsenvironment_offlineon a manifest whose wheelhouse is a complete closure (grpo_config.py:299), and this package had no such field.NemoGymSandboxedConfigisextra="forbid", so that key does not pass through unnoticed — it fails validation, and a sandboxed run with an offline environment could not start at all once NeMo-RL consumes this package.The field now exists on the caller dialect and the serve config, reaches the host as
NMP_ENVIRONMENT_OFFLINE, and setsUV_OFFLINEalongside the wheelhouse'sUV_FIND_LINKS. Index fallback is otherwise retained, which is a liability only when an index is configured but unreachable: uv then fails to resolveuv venv --seedrather than falling back to--find-links.Deliberately a caller decision, not derived: a wheels-v1 package can ship wheels and still need an index for its agent. An operator's own
UV_OFFLINEwins.A stale path the deletion exposes
gym_host.shfell back to$root/nemo_rl/environments/sandbox/gym_host_runtime.pywhen no runtime was given — NeMo-RL's copy of this file, which soluwalana/RL#25 deletes. Unreachable through the package's own API, sincedefault_gym_host_entrypointalways passes the runtime as argv[4], but reachable by anyone invoking the script with three arguments.Stale regardless:
gym_host_runtime_pathinentrypoint.pyalready falls back to thepackages/path alone, with no RL branch. The shell now matches it.Found by sweeping nemo-platform for the old import path while reviewing RL#25 — it is the only place in this repo that still named it.
Type of Change
Quality Gates
Handler, the offline trade-off on the config field and at theUV_OFFLINEassignment.Verification
Signed-off-by:traileruv run pre-commit run -apasses, or any blocked checks are identified belowTargeted validation:
uv run pytest packages/sandboxed_gym/tests -q→ 339 passed, 5 skipped (329 before)pre-commiton the changed files →ruff,ruff format,ty, copyright, merge-conflict all passprotocol_versionto 1.0 failstest_connections_are_never_reusedContent-Length: 0from_send_emptyfailstest_bodiless_responses_carry_a_lengthenvironment_offlinefails one offline test eachReview notes
An independent review (Codex, read-only) cleared the things most likely to bite and returned two findings:
_chunkedleaking across requests or threads — cleared.ThreadingHTTPServercreates one handler instance per connection andparse_requestcloses every connection, so it cannot leak.environment_offline(major) — accurate, and deliberately left alone. The field defaults toFalse, so Evaluator's behaviour is unchanged; it simply cannot opt in yet. Wiring it means adding a knob to Evaluator's plan and job API, which is a product decision, not part of restoring parity with the RL fork. Worth a follow-up.parse_requestcloses the connection. Pre-existing, and no in-repo caller reaches it:urllib/http.clientsend 1.1.Every in-repo caller therefore takes the chunked path; the buffered 1.0 path is a compatibility fallback. One consequence worth naming: on that fallback a disconnected caller is no longer detected mid-rollout, because there is nothing on the wire to fail a write against. It surfaces at the deadline instead.
Not verified
No sandboxed run against a live OpenSandbox host — the original defect was observed through the real proxy, and the regression guard here reproduces its framing at the socket level rather than through that proxy.
Summary by CodeRabbit
New Features
Tests