build(sandboxed-gym): register the package and remove the NeMo-Gym dependency - #1400
Draft
SandyChapman wants to merge 3 commits into
Draft
build(sandboxed-gym): register the package and remove the NeMo-Gym dependency#1400SandyChapman wants to merge 3 commits into
SandyChapman wants to merge 3 commits into
Conversation
Signed-off-by: Sam Oluwalana <soluwalana@nvidia.com>
Copy the implementation of sandbox gym functionality into a package for nemo platform cd ~/work/nemo-platform-extract/packages/sandboxed_gym PYTHONPATH=src:/path/to/nemo-gym pytest tests/ -q PYTHONPATH=src sandboxed-gym serve --help # after pip install -e . Signed-off-by: Sam Oluwalana <soluwalana@nvidia.com>
…pendency Takes the extracted package from #1154 and makes it a workspace member, which requires removing its hard dependency on NeMo-Gym first. `nemo-gym[sandbox]` cannot be declared here. Gym floors at CPython 3.13.14 and pulls `mlflow-skinny>=3.15.1`, which this workspace cannot satisfy -- `services/unsloth` pins `<3.12.0`. uv does not fail on that: it resolves backwards to nemo-gym 0.2.1, a release predating the sandbox work, which satisfies neither the package's imports nor anything we plan against. Nor could the dependency be satisfied as written. The imports are from `nemo_gym.sandbox.broker`, which exists only on an unmerged fork (soluwalana/Gym@nmp/customizer, 15 ahead of upstream and 180 behind); upstream NVIDIA-NeMo/Gym has no such module at any release. So the broker contract and the sandbox provider types are vendored instead -- 290 and 191 lines of Pydantic models, constants and validators, with no Gym imports of their own. `BROKER_PROTOCOL_VERSION` is what detects drift between the copies. Gym remains a runtime import in exactly two places that genuinely drive it: the OpenSandbox provider (lazy, at backend construction) and `runtime/gym_host_runtime.py`, which runs inside the Gym image. Both are excluded from `ty` on the same grounds as the GPU training drivers. With that resolved, the package's 130 tests run for the first time: 128 pass, 2 skip where they construct the real OpenSandbox provider. Also fixes what the type checker found: the broker's serve closure dereferenced an optional attribute, a stale mypy-syntax ignore, a generator fixture annotated as its yield type, and an assertion that called a function twice and so never narrowed. Workspace impact is nil: no existing package changes version, none are removed. Co-authored-by: Sam Oluwalana <soluwalana@nvidia.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) | ||
| sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) | ||
| if self._config.port is not None: | ||
| sock.bind(("", self._config.port)) |
| for _ in range(max_retries): | ||
| port = random.randint(port_range_low, port_range_high - 1) | ||
| try: | ||
| sock.bind(("", port)) |
| ) | ||
| except Exception as exc: | ||
| LOGGER.exception("upstream rollout failed") | ||
| return JSONResponse(status_code=502, content={"error": str(exc)}) |
| with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock: | ||
| sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) | ||
| try: | ||
| sock.bind(("", port)) |
Contributor
|
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
3 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
Makes the extracted
sandboxed_gympackage from #1154 a workspace member, so it is installed, linted, type-checked and tested by CI. Doing that required removing its hard dependency on NeMo-Gym first — that dependency is unsatisfiable in this workspace, and silently degrades the resolution rather than failing.Sam's two commits are carried unchanged (cherry-picked onto current
main, authorship and sign-off intact); the third commit is the integration work.Related Issue
Supersedes the integration half of #1154. That PR remains the source of the package itself.
Changes
The NeMo-Gym dependency is removed. Two problems, either of which blocks registration:
nemo-gym[sandbox]cannot be resolved here. Gym floors at CPython 3.13.14 and pullsmlflow-skinny>=3.15.1;services/unslothpins<3.12.0. uv does not fail on this — it resolves backwards to nemo-gym 0.2.1, a release predating the sandbox work entirely.nemo_gym.sandbox.broker, which exists only on an unmerged fork (soluwalana/Gym@nmp/customizer, 15 commits ahead of upstream and 180 behind). UpstreamNVIDIA-NeMo/Gymhas no such module at any release.So the contract is vendored, not imported:
sandboxed_gym/wire.pynemo_gym/sandbox/broker/wire.py@f2a47392sandboxed_gym/sandbox_types.pySandboxSpec,SandboxHandle,SandboxExecResult,SandboxStatus,SandboxResourcesnemo_gym/sandbox/providers/base.py(0.5.0)Both are pure declarations with no Gym imports.
BROKER_PROTOCOL_VERSIONis the drift detector between the two copies.Gym remains a runtime import in exactly two places that genuinely drive it: the OpenSandbox provider (lazy, at backend construction) and
runtime/gym_host_runtime.py, which runs inside the Gym image. Both are excluded fromtyon the same grounds as the GPU training drivers inservices/unsloth.Fixes surfaced once the type checker could see the package:
broker.py— the serve closure dereferencedself._server, an optional attribute; it now captures the server directly.orchestrator.py— a# type: ignore[arg-type]in mypy syntax, whichtydoes not honour.test_episode_broker.py— a generator fixture annotated as its yield type.test_sandboxed_gym_host.py— an assertion that called_as_network()twice and so never narrowed theOptional.Three
ty: ignorecomments remain, each with a stated reason.Also: SPDX copyright headers on 52 files, and
ruff check/formatacross the package.Type of Change
Quality Gates
Verification
Signed-off-by:traileruv run pre-commit run -apasses, or any blocked checks are identified belowTargeted validation:
uv run --frozen pytest packages/sandboxed_gym/testsuv run --frozen ty check packages/sandboxed_gymuv run ruff check packages/sandboxed_gymuv run ruff format --check packages/sandboxed_gymmake check-copyright-headersuv locksandboxed-gym)uv run --frozen ty check(repo-wide)sandboxed_gymThe two skipped tests construct the real
OpenSandboxProvider, so they run only where Gym is installed — in the runtime image, not a workspace checkout.uv run pre-commit run -awas not run to completion: on this machine the UI hook fails on a missingpnpmshim, and the license hook rewrites files unrelated to this branch. The individual gates it wraps are listed above.Open questions for @soluwalana
NVIDIA-NeMo/Gymfirst? Vendoring unblocks this now; upstreaming is the better end state but is a cross-team ask against a fork that is 180 commits behind.opensandbox(the SDK) is imported bybackends/opensandbox.pybut declared nowhere. It is currently supplied by the environment. Should it be a declared optional dependency?