Repository navigation
feat(nooa): add the sandbox process entrypoint - #3850
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
7217629 to
bb250a0
Compare
bb250a0 to
0b083f2
Compare
0b083f2 to
a25cb1c
Compare
a25cb1c to
ce7fe5e
Compare
ce7fe5e to
7717f40
Compare
d711953 to
cffae80
Compare
745272e to
d159448
Compare
d159448 to
c172798
Compare
c172798 to
b4113a6
Compare
yaoyu-33
left a comment
There was a problem hiding this comment.
Rechecked at 4bac379. The entrypoint layer is unchanged, and the inherited exception-handling and musl findings are fixed. The exact-head NOOA suite passed: 253 tests, with 8 Linux-only installer tests skipped on macOS. No blocking issue found in this layer; full CI is still outstanding.
Signed-off-by: Nikhil Deshmudre <nikhil.deshmudre@gmail.com> (cherry picked from commit 0b083f2) Signed-off-by: Nikhil Deshmudre <nikhil.deshmudre@gmail.com> NOOA-only rebuild of PR #3850 from a25cb1c. NOOA-only scope: shared infrastructure remains identical to main. Signed-off-by: Nikhil Deshmudre <nikhil.deshmudre@gmail.com>
Signed-off-by: Nikhil Deshmudre <nikhil.deshmudre@gmail.com>
|
🌿 Preview your docs: https://nvidia-preview-pr-3850-e10a4ac593dc.docs.buildwithfern.com/nemo/gym |
init-nikhil
left a comment
There was a problem hiding this comment.
CODEX SELF REVIEW
Reviewed head e10a4ac593dc74b8f26a6707c6333eabd7654228 against actual PR base 7ada5311b05fb6d2f74451592660ed2fc4055c02.
Engine: official OpenAI Codex CLI 0.154.0, fresh codex exec --sandbox read-only --ephemeral --json ... review --base process, with inherited model/auth configuration and no overrides.
No actionable findings were returned by the Codex review engine. This does not guarantee correctness.
Review probes: 93 selected tests passed (79 + 14), plus checkpoint integration assertions using in-memory persistence. These used the existing local dependency environment with explicit import setup. Disk-writing tests and live-model smoke validation were not run by this review process.
Evidence retained for run gymcheck-3850-20261019T230215Z: exact invocation and source snapshot, engine stdout/stderr, final response, command outcomes, and exit status 0.
|
Verification checks PASS — checked head
Environment: Python 3.13.14 on macOS arm64; NOOA Scope: worker protocol and checkpoint evidence. Downstream controller/collector recovery, grading, Linux supervisor behavior and provider isolation are not applicable to this PR's changes and were not claimed. The normal return_result fallback remains marked non_trainable_terminal_output. |
yaoyu-33
left a comment
There was a problem hiding this comment.
Thanks for the work here. This adds the standalone sandbox worker (input.json in, result.json / partial checkpoint.json out) that #3851 consumes. The atomic-write approach and keeping partial checkpoints out of the terminal result schema both look right to me. I ran the nooa suite locally (259 passed, 8 skipped: the Linux-only installer tests).
The main things to look at are in the inline comments: the vacuous temp-file assertion in test_sandbox_entrypoint.py, early failures that leave no result.json, and what bounds the model client's retries when the endpoint is unreachable from the sandbox. The rest are smaller notes on test coverage and file modes.
PR hygiene: could you link the relevant issue, or say why none is needed (AGENTS.md "Pull Requests")? Since nothing imports sandbox_entrypoint.py yet, a pointer to #3851 as the consumer would help reviewers.
| else: | ||
| await entrypoint._main(source, destination) | ||
| assert entrypoint.SandboxResult.model_validate_json(destination.read_text()).observations.source == "nooa" | ||
| assert not destination.with_suffix(".tmp").exists() |
There was a problem hiding this comment.
test_sandbox_entrypoint.py:230
destination.with_suffix(".tmp") is result.tmp, but _write_json names its temp files .result.json.<random>.tmp, so this assertion can never fail and a leaked temp file would go unnoticed. Could this check the directory listing instead, e.g. sorted(p.name for p in tmp_path.iterdir()) == ["checkpoint.json", "input.json", "result.json"] (the same pattern is already used correctly a few tests below)?
| ) | ||
|
|
||
|
|
||
| async def _main(input_path: Path, output_path: Path) -> None: |
There was a problem hiding this comment.
result.json is only written when execute() returns. A bad input.json, a failure in finalize_run_result / SandboxResult(...) construction, or in setting up the aiohttp client exits non-zero with no failure artifact, which a controller can't tell apart from a SIGKILL (the test for this case asserts not destination.exists()). Would a top-level handler that writes RunnerError(kind="fatal") fit here, or is "no result.json plus non-zero exit means infrastructure failure" the intended contract? If so, it would be good to document it in the module docstring.
| headers: dict[str, str] | None = None, | ||
| ) -> ClientResponse: | ||
| # The supplied path already includes rollout and token-capture routing. | ||
| return await request( |
There was a problem hiding this comment.
Could you confirm what bounds this call when the model endpoint is unreachable from the sandbox? request(..., _internal=True) retries connection errors indefinitely and sets no timeout, and unlike the head-server case there is no supervisor here to shut things down, so the worker would hang silently and never reach the transient mapping. If #3851 enforces a wall-clock limit that is fine, but it would help to say so here or pass a bounded _max_connection_retries.
| partial: Literal[True] | ||
|
|
||
|
|
||
| def _write_json(path: Path, value: BaseModel) -> None: |
There was a problem hiding this comment.
Each checkpoint rebuilds the whole trajectory and does a synchronous serialize + fsync + rename on the event loop (via checkpoint() in runner.py), after every model response and tool completion. Cost grows with transcript length and it stalls this rollout's other coroutines while it runs. Not blocking, but consider handing the snapshot bytes to a single writer thread (asyncio.to_thread, latest-wins) if long runs are expected.
|
|
||
| monkeypatch.setattr(entrypoint, "set_global_aiohttp_client", initialize) | ||
| monkeypatch.setattr(entrypoint, "execute", execute) | ||
| monkeypatch.setattr(asyncio.get_running_loop(), "add_signal_handler", MagicMock()) |
There was a problem hiding this comment.
test_sandbox_entrypoint.py:220
Nothing exercises the real SIGTERM path through _main: add_signal_handler is mocked here, so "SIGTERM produces a cancelled result.json" is covered only by the manual smoke. Could you add a test that starts _main with a runner blocked on an event, sends SIGTERM to the process, and asserts result.json has error.kind == "cancelled" and response is None?
| write_checkpoint(tmp_path / "checkpoint.json", result) | ||
| checkpoints.append(SandboxCheckpoint.model_validate_json((tmp_path / "checkpoint.json").read_text())) | ||
|
|
||
| runner._on_progress = checkpoint |
There was a problem hiding this comment.
The best-effort contract in checkpoint() (except Exception + logger.exception) has no test. Could you add one where on_progress raises (e.g. OSError("disk full")) and the run still returns its normal result and logs the failure? As written, removing that try/except would not fail anything.
| ) | ||
| invocation.conversation.append(call_item) | ||
| output.append(call_item) | ||
| if record.completed_at is None: |
There was a problem hiding this comment.
This completed_at is None skip also applies to the final projection, not just checkpoints. If a tool's after-hook never fires, the final response would contain a function_call with no matching function_call_output, where before it got an incomplete output. Hooks do fire on exceptions and cancellation so this looks hard to reach; would it be worth limiting the skip to checkpoint projection (a partial flag) to keep the final response well-formed?
| serialized = value.model_dump_json() | ||
| temporary = None | ||
| try: | ||
| with tempfile.NamedTemporaryFile( |
There was a problem hiding this comment.
NamedTemporaryFile creates the file with mode 0600 and replace() carries that over to result.json / checkpoint.json. If the supervisor or collector reads these as a different user than the worker, the read will fail. Could you chmod to the intended mode before the replace, or confirm the same-uid assumption?
Signed-off-by: Nikhil Deshmudre <nikhil.deshmudre@gmail.com>
Signed-off-by: Nikhil Deshmudre <nikhil.deshmudre@gmail.com>
yaoyu-33
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. I re-reviewed the new commit and the earlier comments are addressed: the temp-file assertion now checks the whole directory listing, there is a real-subprocess SIGTERM test, a test for a failing on_progress, and the dangling function_call_output is fixed by limiting the skip to checkpoint projections (partial). The remaining three (no result.json on early failures, unbounded model-call retries, and 0600 artifacts) are now documented as the worker's contract, and the #3851 supervisor backs it: it kills the worker at the deadline, treats a missing result plus a non-zero exit as an infrastructure failure, and never grades a checkpoint. I ran the nooa suite locally on this head (263 passed, 8 skipped: the Linux-only installer tests).
The synchronous serialize and fsync per checkpoint is still there, with the durability rationale added in runner.py. I'm fine leaving it as an accepted tradeoff; if long transcripts turn out to stall the loop, offloading the write is the follow-up. I have no remaining blocking comments.
yaoyu-33
left a comment
There was a problem hiding this comment.
Approving. The earlier comments are addressed, and I have no remaining blocking comments.
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/39878585907 |
Tests and validation
Head
3c332f419f0073dd9fc360e0056a47f254cc3d98; base7128f824fdebf6ace8d2f5e6fb3a7eda91255967.Consumed by #3851; the existing stack tracks this focused implementation, so no separate issue is needed.
3c332f419: 263 passed, 8 Linux-only skips on macOS with NOOA0514723; 78 focused checks passed.pre-commit run --all-files: passed.Assembled-stack tests, Docker smoke and their exact-revision limits are recorded in #4813; they supplement these layer-specific checks.
Missing/malformed results with unsuccessful termination are infrastructure failures; recovered checkpoints never authorize grading. Callers must enforce a wall-clock deadline. Mode 0600 artifacts require the worker UID or privileged reads. Checkpoint projection/serialization/fsync remain synchronous: queued writes could lose the latest execution boundary on abrupt termination. The nonblocking async-writer suggestion is deferred; long transcripts still incur this event-loop cost.
CI: on the new head (2026-10-21 UTC), DCO passed; OpenCode, Pi and Codex conformance and their aggregate report are failing. Other checks are still running where shown in the Checks tab. GitHub is not all green.