refactor(evaluator)!: resolve the Gym CLI from PATH instead of a configured checkout - #1196
Merged
Conversation
…igured checkout `GymRuntimeConfig` no longer carries `gym_root` or a `gym_bin` path. The runner resolves `gym` from PATH, so NeMo Gym must be installed in the same environment as the SDK. Two reasons the old shape does not survive: * These runner configs become serialized job specs when Gym runs as a governed platform job. A path into somebody's checkout or venv means nothing on the other side of that boundary, so the fields would only ever be stripped or ignored server-side. * A checkout is no longer required. NeMo-Gym now ships its component trees in the wheel — `resources_servers` and friends install beside `nemo_gym`, configs and `data/example.jsonl` included — so a plain `pip install nemo-gym` resolves environments and their example data with nothing on disk. `gym_root` had exactly two jobs: defaulting `gym_bin` to `<gym_root>/.venv/bin/gym`, and setting the subprocess cwd. The first is replaced by `shutil.which`, matching how `CodexCliAgentRuntime` already resolves its own CLI. The second is dropped: the subprocesses inherit this process's working directory, which is where Gym looks for the gitignored `env.yaml` holding the collector's credentials before falling back to its install root. Running from a Gym checkout still makes its components take precedence, so the capability survives without a field to configure it. A missing CLI now fails with an actionable message before the run starts, instead of an ENOENT out of `create_subprocess_exec` partway through. The mcqa example drops `--gym-root`, defaults its dataset to the packaged `resources_servers/<name>/data/example.jsonl`, and its README leads with `pip install nemo-gym` rather than a checkout. BREAKING CHANGE: `GymRuntimeConfig.gym_root` and `GymRuntimeConfig.gym_bin` are removed. Install NeMo Gym in the same environment as the SDK; run from the directory holding `env.yaml`, or from a Gym checkout when you need components the wheel does not carry. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Gym integration now uses an installed ChangesGym CLI integration
Sequence Diagram(s)sequenceDiagram
participant User
participant run_gym_eval
participant GymRuntime
participant GymCLI
User->>run_gym_eval: start evaluation
run_gym_eval->>GymRuntime: configure dataset and runtime
GymRuntime->>GymCLI: resolve gym from PATH
GymRuntime->>GymCLI: run evaluation from caller directory
GymCLI-->>User: write evaluation results
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nemo_evaluator_sdk/examples/gym/README.md`:
- Around line 9-16: Update the setup section in the NeMo Gym README to use a
tab-set containing both the CLI workflow and a tested Python SDK workflow. Keep
installation commands, env.yaml configuration, dataset selection, and output
behavior equivalent across both examples, and ensure the Python example reflects
the current SDK API.
- Around line 18-40: Update the README’s later “How it runs Gym” setup
description to match the runtime contract: state that the executable is resolved
from PATH and env.yaml is loaded from the caller’s working directory. Remove
wording that implies both come from a Gym checkout, while preserving the
existing explanation of checkout precedence where applicable.
In `@packages/nemo_evaluator_sdk/examples/gym/run_gym_eval.py`:
- Around line 81-100: Update _packaged_dataset to assign the bundled
example.jsonl resource to a variable, verify it with is_file(), and raise the
existing SystemExit guidance with the --dataset fallback when it is missing;
return the resource path only after validation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bcc8952b-8f74-4c78-8b68-53f36a86f0fe
⛔ Files ignored due to path filters (1)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/gym_runtime.pyis excluded by!sdk/**
📒 Files selected for processing (5)
packages/nemo_evaluator_sdk/examples/gym/README.mdpackages/nemo_evaluator_sdk/examples/gym/inspect_results.pypackages/nemo_evaluator_sdk/examples/gym/run_gym_eval.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/gym_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_run_metadata.py
Contributor
|
Merged
15 tasks
arpitsardhana
approved these changes
Aug 10, 2026
Contributor
|
I also think we should bubble up the requirement of thin python client to gym so that SDK can use without relying on commandline execution |
SandyChapman
added a commit
that referenced
this pull request
Aug 10, 2026
Three fixes from review of #1196. **README contradicted itself.** The "How it runs Gym" section still said the executable comes from "your checkout" and credentials from "that checkout's env.yaml", which the rewritten prerequisites had already replaced. It now says `gym` is resolved from PATH and `env.yaml` is read from the directory you run from. **`_packaged_dataset` returned paths it never checked.** It caught `ModuleNotFoundError` but not a missing file, so an importable environment without bundled data returned a path that does not exist and left `discover_gym_tasks` to raise a bare `FileNotFoundError` about a path the caller never chose. Only git-tracked files ship in the wheel, so an environment whose splits are downloaded at runtime has no `example.jsonl` — this is reachable, not theoretical. It now fails with the same `--dataset` guidance as the import case. **Clarified that `_GYM_CLI` is resolved, never executed.** Review asked whether the resolving and executing processes could disagree about PATH. They cannot: `shutil.which` returns an absolute path and that is what the subprocesses run. The constant reads like a bare name, so the reasoning is now written down next to it. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
Three fixes from review of #1196. **README contradicted itself.** The "How it runs Gym" section still said the executable comes from "your checkout" and credentials from "that checkout's env.yaml", which the rewritten prerequisites had already replaced. It now says `gym` is resolved from PATH and `env.yaml` is read from the directory you run from. **`_packaged_dataset` returned paths it never checked.** It caught `ModuleNotFoundError` but not a missing file, so an importable environment without bundled data returned a path that does not exist and left `discover_gym_tasks` to raise a bare `FileNotFoundError` about a path the caller never chose. Only git-tracked files ship in the wheel, so an environment whose splits are downloaded at runtime has no `example.jsonl` — this is reachable, not theoretical. It now fails with the same `--dataset` guidance as the import case. **Clarified that `_GYM_CLI` is resolved, never executed.** Review asked whether the resolving and executing processes could disagree about PATH. They cannot: `shutil.which` returns an absolute path and that is what the subprocesses run. The constant reads like a bare name, so the reasoning is now written down next to it. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
gym-bin-resolution/schapman
branch
from
August 10, 2026 16:26
1fc6cf5 to
e636cf6
Compare
ngoncharenko
approved these changes
Aug 10, 2026
Co-authored-by: Nick Goncharenko <8766167+ngoncharenko@users.noreply.github.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
gym-bin-resolution/schapman
branch
from
August 10, 2026 17:07
4b37302 to
d01b12e
Compare
8 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
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
GymRuntimeConfigno longer carriesgym_rootor agym_binpath — the runner resolvesgymfrom PATH, so NeMo Gym must be installed in the same environment as the SDK. Before, a caller had to name a Gym checkout and the runner derived the executable from<gym_root>/.venv/bin/gym; now there is no path configuration at all, and a missing CLI fails with an actionable message before the run starts instead of anENOENTpartway through.Two reasons the old shape does not survive. These runner configs become serialized job specs when Gym runs as a governed platform job, and a path into somebody's checkout or venv means nothing on the other side of that boundary. And a checkout is no longer required: NeMo-Gym now ships its component trees in the wheel, so
pip install nemo-gymresolves environments and their example data with nothing on disk.Related Issue
Tracked in Linear as AALGO-485 (validating the Gym runner across a representative sample of built-in environments); this is the configuration cleanup that came out of it. No GitHub issue.
Changes
gym_runtime.py— removegym_rootandgym_binfromGymRuntimeConfig. Add a module-level_gym_executable()that resolves_GYM_CLIviashutil.which, mirroring howCodexCliAgentRuntimealready resolves its own CLI, and raises with install guidance when absent.gym env startandgym eval runsubprocesses inherit this process's working directory, which is where Gym looks for the gitignoredenv.yamlholding the collector's credentials before falling back to its install root. Running from a Gym checkout still makes its components take precedence, so that capability survives without a field.examples/gym/run_gym_eval.py— drop--gym-root; default the dataset to the packagedresources_servers/<name>/data/example.jsonlviaimportlib.resources, with an actionable error when Gym is not importable.examples/gym/README.md— lead withpip install nemo-gym. The previous text asserted "a working NeMo Gym checkout is required — Gym resolves its environments from the repo, not from a package install", which is no longer true.examples/gym/inspect_results.py— update the usage example.tests/agent_eval/test_run_metadata.py— dropgym_rootfrom the twoGymRuntimeConfigconstructions.runner_info()output is unchanged, so persistedmetadata.jsonprovenance keeps its shape.make vendor; not hand-edited.Type of Change
Quality Gates
test_run_metadata.pycovers therunner_info()surface and was updated for the removed field; the 542-testtests/agent_eval/suite covers the runner. The new failure path needs a missinggymbinary, which the existing suite cannot assert against without mocking PATH — it was verified by hand instead (below), and live coverage across real environments is the subject of AALGO-485.Verification
Signed-off-by:traileruv run pre-commit run -apasses, or any blocked checks are identified belowTargeted validation:
Failure path exercised by hand, since it requires
gymto be absent from PATH:That first run also confirms the packaged-dataset claim end to end:
discover_gym_tasksreadexample.jsonlstraight out of site-packages with no checkout present.Two
pre-commithooks fail in my environment, neither exercising this change:Run uv lock with platform uv— requires uv 0.9.14; I have 0.9.30. This diff touches nopyproject.tomloruv.lock, and the separateCheck for uv.lock drifthook passes, so the lock is correct. Worth noting the tension this PR walks into: nemo-platform pins that hook at 0.9.14 while NeMo-Gym's ownpyproject.tomlsetsrequired-version = ">=0.9.30", so one uv install cannot satisfy both — and this change asks contributors to install Gym alongside the SDK.Run UI lint-staged—mise ERROR No version is set for shim: pnpm. A local Node toolchain gap; this diff touches noweb/files.Every other hook passes:
ruff,ruff format,ty, config reference docs, Helm docs, uv.lock drift, copyright headers, plugin import boundary, merge-conflict check.Summary by CodeRabbit
New Features
gymcommand available on yourPATH.env.yamlfile.Bug Fixes
Documentation