feat(evaluator): add NeMo Gym runner for agent-eval - #986
Merged
Conversation
Contributor
|
SandyChapman
marked this pull request as ready for review
July 30, 2026 12:30
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:
📝 WalkthroughWalkthroughAdds a Gym-backed NeMo Evaluator runtime with task discovery, subprocess orchestration, rollout-to-trial conversion, reward metrics, tests, and a runnable CLI example with documentation. ChangesGym evaluation integration
Sequence Diagram(s)sequenceDiagram
participant User
participant run_gym_eval.py
participant GymAgentTaskRunner
participant NeMoGym
participant AgentEvaluator
User->>run_gym_eval.py: provide Gym checkout and runtime flags
run_gym_eval.py->>GymAgentTaskRunner: discover tasks and run evaluation
GymAgentTaskRunner->>NeMoGym: start environment and collect rollouts
NeMoGym-->>GymAgentTaskRunner: return rollout and failure records
GymAgentTaskRunner-->>AgentEvaluator: return evaluator trials
AgentEvaluator-->>User: report aggregate scores and output bundle
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nemo_evaluator_sdk/examples/gym/README.md (1)
1-67: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReorder sections and add a "Next Steps" section per doc guidelines.
Move "## Prerequisites" ahead of "## How it runs Gym" (prerequisites must lead the page), and add a closing "Next Steps" section with cross-links to related docs (e.g.,
AgentEvaluator, Harbor/Fabric runtime examples). Consider moving the "How it runs Gym" architecture explanation behind a cross-link rather than inline, to keep this page a single how-to.As per coding guidelines, "Always list prerequisites at the top of documentation pages before other content" and "Include 'Next Steps' section at the end with cross-links to related documentation content."
🤖 Prompt for 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. In `@packages/nemo_evaluator_sdk/examples/gym/README.md` around lines 1 - 67, Reorder the README so the existing “## Prerequisites” section appears before “## How it runs Gym,” keeping the introductory overview first. Add a closing “## Next Steps” section with links to related AgentEvaluator and Harbor/Fabric runtime documentation, and move the detailed Gym architecture explanation behind a cross-link if needed to keep this page focused as a how-to.Source: Coding guidelines
🤖 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/src/nemo_evaluator_sdk/agent_eval/runtimes/gym_runtime.py`:
- Around line 421-430: Update _dataset_path_from_tasks to inspect every task’s
non-empty gym_dataset_path, retaining the first path and validating that all
subsequent stamped paths match it. Raise ValueError when tasks contain
conflicting dataset paths, while preserving the existing missing-path error when
none are stamped.
- Around line 527-546: Update the success-path rollout handling in the loop that
builds AgentEvalTrial objects to validate NG_ROLLOUT_INDEX with the same type
check and synthesized suffix used by the failure branch. Ensure missing or null
indices receive unique fallback values rather than producing task IDs ending in
0 or None, while preserving valid rollout indices unchanged.
---
Outside diff comments:
In `@packages/nemo_evaluator_sdk/examples/gym/README.md`:
- Around line 1-67: Reorder the README so the existing “## Prerequisites”
section appears before “## How it runs Gym,” keeping the introductory overview
first. Add a closing “## Next Steps” section with links to related
AgentEvaluator and Harbor/Fabric runtime documentation, and move the detailed
Gym architecture explanation behind a cross-link if needed to keep this page
focused as a how-to.
🪄 Autofix (Beta)
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: e667e985-2206-4d60-b2eb-2c94966c2358
⛔ 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 (6)
packages/nemo_evaluator_sdk/examples/gym/README.mdpackages/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/fixtures/gym_mcqa_example.jsonlpackages/nemo_evaluator_sdk/tests/agent_eval/fixtures/gym_mcqa_rollouts.jsonlpackages/nemo_evaluator_sdk/tests/agent_eval/test_gym_runtime.py
ngoncharenko
approved these changes
Jul 31, 2026
SandyChapman
force-pushed
the
aalgo-432-walking-skeleton/schapman
branch
from
July 31, 2026 13:08
4156b6b to
18a7332
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/nemo_evaluator_sdk/tests/agent_eval/test_gym_runtime.py (1)
391-455: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
_wait_for_servers.The subprocess helpers are tested, but the readiness loop is not. It has three failure branches: early process exit, the uv required-version gate, and the startup timeout. All three are pure functions of a log file plus a fake process object, so they are cheap to test with a stub exposing
returncode.🤖 Prompt for 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. In `@packages/nemo_evaluator_sdk/tests/agent_eval/test_gym_runtime.py` around lines 391 - 455, Add focused async tests for _wait_for_servers using a temporary log file and a fake process exposing returncode, covering early process exit, failure of the required uv-version check, and startup timeout. Assert each branch raises or reports the expected outcome, and keep the tests isolated from real subprocesses or server startup.
🤖 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`:
- Line 45: Update the fenced code block in the README example to specify the
text language, using the existing output snippet unchanged and adding the
appropriate fence language identifier so it satisfies markdownlint MD040.
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/gym_runtime.py`:
- Around line 807-812: Update the Gym trial collection flow around the
missing-task handling in the function returning trials: instead of only warning
for task IDs in missing, append one FAILED trial record for each unrepresented
task, including the task ID and diagnostic context such as rollouts_path or the
relevant log files. Preserve the existing warning and empty-collection behavior
as appropriate, and ensure _score_trials receives these failure trials so
partial runs complete without raising for absent tasks.
---
Nitpick comments:
In `@packages/nemo_evaluator_sdk/tests/agent_eval/test_gym_runtime.py`:
- Around line 391-455: Add focused async tests for _wait_for_servers using a
temporary log file and a fake process exposing returncode, covering early
process exit, failure of the required uv-version check, and startup timeout.
Assert each branch raises or reports the expected outcome, and keep the tests
isolated from real subprocesses or server startup.
🪄 Autofix (Beta)
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: d29e61d2-d7c7-43ce-9b18-899d22e66b74
⛔ 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 (6)
packages/nemo_evaluator_sdk/examples/gym/README.mdpackages/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/fixtures/gym_mcqa_example.jsonlpackages/nemo_evaluator_sdk/tests/agent_eval/fixtures/gym_mcqa_rollouts.jsonlpackages/nemo_evaluator_sdk/tests/agent_eval/test_gym_runtime.py
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/nemo_evaluator_sdk/tests/agent_eval/fixtures/gym_mcqa_rollouts.jsonl
- packages/nemo_evaluator_sdk/tests/agent_eval/fixtures/gym_mcqa_example.jsonl
- packages/nemo_evaluator_sdk/examples/gym/run_gym_eval.py
SandyChapman
force-pushed
the
aalgo-432-walking-skeleton/schapman
branch
from
July 31, 2026 15:12
18a7332 to
6c1bbde
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/src/nemo_evaluator_sdk/agent_eval/runtimes/gym_runtime.py`:
- Around line 803-809: The _resolve_task_id call at line 805 raises ValueError
when an index is absent from the index_to_task_id map, which aborts the loop
despite tolerant=True protecting against truncated lines. Wrap the
_resolve_task_id call in a try-except block that catches ValueError, increments
unattributed_failures, and continues the loop instead of propagating the
exception. This preserves the "count, don't drop" behavior stated in the
comment.
🪄 Autofix (Beta)
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: 9c000293-8244-45f2-80f5-a5a8f2d5d4ec
⛔ 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 (6)
packages/nemo_evaluator_sdk/examples/gym/README.mdpackages/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/fixtures/gym_mcqa_example.jsonlpackages/nemo_evaluator_sdk/tests/agent_eval/fixtures/gym_mcqa_rollouts.jsonlpackages/nemo_evaluator_sdk/tests/agent_eval/test_gym_runtime.py
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/nemo_evaluator_sdk/tests/agent_eval/fixtures/gym_mcqa_rollouts.jsonl
- packages/nemo_evaluator_sdk/examples/gym/run_gym_eval.py
- packages/nemo_evaluator_sdk/examples/gym/README.md
- packages/nemo_evaluator_sdk/tests/agent_eval/fixtures/gym_mcqa_example.jsonl
SandyChapman
force-pushed
the
aalgo-432-walking-skeleton/schapman
branch
from
July 31, 2026 15:20
6c1bbde to
5483ada
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/nemo_evaluator_sdk/tests/agent_eval/test_gym_runtime.py (1)
271-281: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a pre-existing
rollouts.jsonl.The tests cover only the sidecar. Add a case where
rollouts.jsonlitself already exists, so the primary-artifact branch of_ensure_fresh_outputis locked in too.♻️ Suggested additional test
+def test_ensure_fresh_output_rejects_existing_rollouts(tmp_path: Path) -> None: + rollouts = tmp_path / "rollouts.jsonl" + rollouts.write_text("{}\n", encoding="utf-8") + with pytest.raises(FileExistsError): + _ensure_fresh_output(rollouts)🤖 Prompt for 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. In `@packages/nemo_evaluator_sdk/tests/agent_eval/test_gym_runtime.py` around lines 271 - 281, Add a test alongside test_ensure_fresh_output_rejects_reused_dir that creates a pre-existing rollouts.jsonl in tmp_path, calls _ensure_fresh_output with that path, and asserts FileExistsError is raised, covering the primary artifact branch while preserving the existing clean-directory case.
🤖 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/src/nemo_evaluator_sdk/agent_eval/runtimes/gym_runtime.py`:
- Around line 790-793: Update the success-record loop in the rollout evaluation
flow to count records where _resolve_task_id returns None instead of silently
continuing. Add an unattributed_successes counter and emit a warning alongside
the existing failures warning, including the count, rollouts_path, and
NG_TASK_INDEX, while preserving the current behavior of skipping those records.
---
Nitpick comments:
In `@packages/nemo_evaluator_sdk/tests/agent_eval/test_gym_runtime.py`:
- Around line 271-281: Add a test alongside
test_ensure_fresh_output_rejects_reused_dir that creates a pre-existing
rollouts.jsonl in tmp_path, calls _ensure_fresh_output with that path, and
asserts FileExistsError is raised, covering the primary artifact branch while
preserving the existing clean-directory case.
🪄 Autofix (Beta)
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: 007eed31-828b-45aa-9589-568ead82c78c
⛔ 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 (6)
packages/nemo_evaluator_sdk/examples/gym/README.mdpackages/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/fixtures/gym_mcqa_example.jsonlpackages/nemo_evaluator_sdk/tests/agent_eval/fixtures/gym_mcqa_rollouts.jsonlpackages/nemo_evaluator_sdk/tests/agent_eval/test_gym_runtime.py
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/nemo_evaluator_sdk/tests/agent_eval/fixtures/gym_mcqa_rollouts.jsonl
- packages/nemo_evaluator_sdk/tests/agent_eval/fixtures/gym_mcqa_example.jsonl
- packages/nemo_evaluator_sdk/examples/gym/README.md
- packages/nemo_evaluator_sdk/examples/gym/run_gym_eval.py
Adds `GymAgentTaskRunner`, a NeMo Gym-backed `AgentTaskRunner` that runs an existing NeMo Gym environment through the `gym` CLI and adapts its rollout bundle into `AgentEvalTrial` objects, so `AgentEvaluator` can score and report Gym runs through the same seam as Harbor/Fabric. Gym owns execution *and* scoring; this runner only surfaces its per-attempt reward. Mapping: one Gym dataset -> one run; each distinct row -> one `AgentEvalTask` (id = content hash of the row); each attempt (`num_repeats`) -> one `AgentEvalTrial`. Attribution is by `_ng_task_index`, which the runner *assigns* rather than infers. Gym only auto-assigns an index when a row lacks one, and its fallback dedup keys off the raw jsonl line text -- a rule we cannot reproduce from parsed rows, so inferring it risked silently swapping results between tasks. Instead the runner materializes a normalized dataset (one row per requested task, `_ng_task_index` stamped explicitly) and feeds that to Gym, giving a total, order-independent index -> task map. This also confines a run to the requested subset instead of rolling out the whole dataset. Other behaviour: - `GymRewardMetric` surfaces Gym's per-attempt reward; a missing or malformed reward is left unscored (None -> nan, excluded from the mean) rather than counted as a spurious 0.0. - The `*_failures.jsonl` sidecar is read and surfaced as FAILED trials (counted + diagnosed), not silently dropped. - One distinct row is one task: duplicate rows collapse and warn, since repeated attempts are `num_repeats`, not row duplication. - Trial fan-out rejects an index map that disagrees with the task list, so a future Gym change cannot quietly attribute results to the wrong tasks. - An incomplete collection fails the run rather than scoring a subset, and the error names the rollout bundle, the failures sidecar and the Gym logs instead of leaving the operator with the evaluator's generic task-id list. - Teardown signals the whole process group with SIGINT (mimicking Ctrl-C so Gym's own shutdown() stops Ray + uvicorn cleanly), escalating to SIGKILL after a grace window. - Subprocess output streams to log files under the run's work dir and is mirrored to the module logger at DEBUG, so callers control terminal visibility through ordinary logging config. Log drains are bounded: a grandchild that outlives teardown (e.g. Ray's detached gcs_server) can hold the inherited pipe open, which must not wedge the run. - Readiness keys off Gym's own "All N / N servers ready!" line; refuses to reuse a populated output_dir (Gym appends to its failures sidecar); sets RAY_ENABLE_UV_RUN_RUNTIME_ENV=0 so Gym's per-server venvs don't trip Ray's `uv run` hook. Includes a worked `mcqa` example and unit tests against captured fixtures. Verified end-to-end against a real Gym checkout (5 tasks -> 10 trials). Aggregations (native pass@k cross-check, bubbling Gym's rollouts_aggregate_metrics.json) and service-side execution are out of scope here and tracked separately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
aalgo-432-walking-skeleton/schapman
branch
from
July 31, 2026 15:26
5483ada to
e38fad2
Compare
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.
What
Adds
GymAgentTaskRunner, a NeMo Gym-backedAgentTaskRunnerthat runs an existing NeMo Gym environment through thegymCLI and adapts its rollout bundle intoAgentEvalTrialobjects, soAgentEvaluatorcan score and report Gym runs through the same seam as Harbor/Fabric. Gym owns execution and scoring; this runner only surfaces its per-attempt reward — it does not re-derive rewards.Includes a worked
mcqaexample (packages/nemo_evaluator_sdk/examples/gym/) and unit tests against captured fixtures. Verified end-to-end against a real Gym checkout (5 tasks → 10 trials).Mapping (Gym → Evaluator)
AgentEvalTask(id = content hash of the row); each attempt (--num-repeats) → oneAgentEvalTrial._ng_task_index(Gym mutatesresponses_create_paramsand drops injected fields, so a content round-trip is impossible), with a guard that fails loud if our row-dedup partition diverges from Gym's.GymRewardMetricsurfaces Gym's per-attemptreward; a missing/malformed reward is left unscored (None→ nan, excluded from the mean) rather than counted as a spurious0.0.*_failures.jsonlsidecar is read and surfaced asFAILEDtrials (counted + diagnosed), not silently dropped.Execution & robustness
gym env startthengym eval run --no-serve --input— which reads a dataset directly without triggering Gym's split-driven data-prep. Shells out to thegymexecutable in the caller-provided checkout; never importsnemo_gym, never handles secrets.shutdown()stops Ray + uvicorn cleanly), escalating to SIGKILL after a grace window.All N / N servers ready!line (no server-count guessing); bounded collection viacollection_timeout_s; refuses to reuse a populatedoutput_dir(Gym appends to its failures sidecar); setsRAY_ENABLE_UV_RUN_RUNTIME_ENV=0so Gym's per-server venvs don't trip Ray'suv runhook.Scope / follow-ups
rollouts_aggregate_metrics.jsonas run diagnostics) are intentionally out of scope here — tracked separately.Draft for early review.
Summary by CodeRabbit
New Features
Documentation
Tests