refactor(evaluator)!: unify the backend contract on evaluate/evaluate_dataset - #1173
Closed
SandyChapman wants to merge 4 commits into
Closed
refactor(evaluator)!: unify the backend contract on evaluate/evaluate_dataset#1173SandyChapman wants to merge 4 commits into
SandyChapman wants to merge 4 commits into
Conversation
Contributor
|
SandyChapman
force-pushed
the
evaluator-agent-eval-submit/schapman
branch
from
August 9, 2026 22:53
d2ccb45 to
6e36303
Compare
SandyChapman
marked this pull request as ready for review
August 9, 2026 23:14
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:
📝 WalkthroughWalkthroughThe evaluator SDK now uses metric sequences and ChangesEvaluator API migration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: 7
🧹 Nitpick comments (1)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/evaluator.py (1)
200-242: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winExpose the polling and timeout knobs on
submit.
submitcallswait_until_done()with defaults only, so every run is capped atDEFAULT_JOB_TIMEOUT_SECONDS(3600s). A caller with a longer agent run has no way to raise the ceiling without dropping tobackend.evaluate. Forward the three parameters.♻️ Forward timeout arguments
async def submit( self, *, taskset: Sequence[AgentEvalTask], target: AgentEvalTarget | None = None, trials: Sequence[AgentEvalTrial] | None = None, config: AgentEvalRunConfig | None = None, + poll_interval_seconds: float = DEFAULT_POLL_INTERVAL_SECONDS, + job_timeout_seconds: float = DEFAULT_JOB_TIMEOUT_SECONDS, + pending_timeout_seconds: float = DEFAULT_PENDING_TIMEOUT_SECONDS, ) -> AgentEvalResult: @@ job = await self._backend.evaluate(taskset=taskset, target=target, trials=trials, config=config) - await job.wait_until_done() + await job.wait_until_done( + poll_interval_seconds=poll_interval_seconds, + job_timeout_seconds=job_timeout_seconds, + pending_timeout_seconds=pending_timeout_seconds, + ) return await job.get_result()🤖 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/src/nemo_evaluator_sdk/execution/evaluator.py` around lines 200 - 242, Expose the polling and timeout parameters accepted by job.wait_until_done through Evaluator.submit, including them in its signature and documentation. Forward all three values to job.wait_until_done instead of relying on defaults, and propagate the same parameters through submit_sync so both APIs support longer-running evaluations.
🤖 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/execution/backends/local/backend.py`:
- Around line 58-71: Update LocalBackend.evaluate to prepare each task’s metrics
with the backend resolver flow before invoking AgentEvaluator.run, using
prepare_metric_for_execution consistently with evaluate_dataset. Ensure
registered ModelRef and SecretRef values are resolved while preserving the
existing task, trial, target, and config execution behavior.
In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/jobs.py`:
- Around line 30-31: Update EvaluationJob to use Python 3.11-compatible generic
Protocol syntax with a separately declared TypeVar, or consistently raise the
package’s minimum Python version and Ruff target to 3.12 in pyproject.toml; keep
the existing generic behavior intact.
In `@plugins/nemo-evaluator/README.md`:
- Around line 102-103: Update plugins/nemo-evaluator/README.md lines 102-103 so
the evaluate_dataset documentation states that it returns EvaluatorJobResource.
In skills/nemo-evaluator-plugin/references/execution.md lines 50-54, replace
both stale submit API references with evaluate_dataset references, preserving
the surrounding usage guidance.
In `@plugins/nemo-evaluator/src/nemo_evaluator/sdk/_agent_eval_executor.py`:
- Around line 133-143: Update _target_spec so target-only configuration is
rejected when target is None instead of returning silently; validate
config.params and config.prompt_template and raise the established configuration
error for either field. Preserve the existing behavior for compatible
ModelTarget or AgentTarget values, and add a regression test covering
precomputed trials with RunConfigOnlineModel.
In `@plugins/nemo-evaluator/src/nemo_evaluator/sdk/agent_eval_job_resources.py`:
- Around line 97-128: Carry the platform timeout through _JobAddress and pass it
as the timeout argument to every synchronous and asynchronous GET request,
including get_job_status, wait_until_done, get_result, and the related methods
around the async implementation. Ensure each status and bundle request is
bounded so the existing timeout checks can execute.
In `@plugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.py`:
- Around line 155-184: The plugin resource method evaluate_dataset must not
satisfy the EvaluationBackend contract while accepting incompatible config and
returning EvaluatorJobResource. Prevent _validate_backend_client from accepting
plugin resources, or add an explicit adapter that translates params to config,
waits for the submitted job, and returns BenchmarkEvaluationResult; ensure
unsupported keyword arguments cannot reach _executor.submit.
In `@skills/nemo-evaluator-plugin/SKILL.md`:
- Around line 54-57: Update the recommendation sentence in the plugin evaluation
guidance to default to evaluate_dataset only for dataset-driven evaluations.
Preserve the existing task-driven routing to nemo evaluator agent-evaluate
submit and do not imply that evaluate_dataset applies to every plugin
evaluation.
---
Nitpick comments:
In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/evaluator.py`:
- Around line 200-242: Expose the polling and timeout parameters accepted by
job.wait_until_done through Evaluator.submit, including them in its signature
and documentation. Forward all three values to job.wait_until_done instead of
relying on defaults, and propagate the same parameters through submit_sync so
both APIs support longer-running evaluations.
🪄 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: f5e85650-8fe0-4543-9efe-a9d92022b123
⛔ Files ignored due to path filters (7)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/backends/base.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/backends/local/backend.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/evaluator.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/jobs.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/utils.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/multi_metric_results.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/skills/nemo-evaluator/references/sdk-execution.mdis excluded by!sdk/**
📒 Files selected for processing (37)
e2e/test_evaluator_plugin.pypackages/nemo_evaluator_sdk/examples/examples.pypackages/nemo_evaluator_sdk/examples/high_level_evaluate_walkthrough.ipynbpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/README.mdpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/backends/base.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/backends/local/backend.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/evaluator.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/jobs.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/utils.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/multi_metric_results.pypackages/nemo_evaluator_sdk/tests/execution/backends/local/test_backend.pypackages/nemo_evaluator_sdk/tests/execution/test_evaluator.pypackages/nemo_evaluator_sdk/tests/execution/test_metric_execution.pypackages/nemo_evaluator_sdk/tests/execution/test_resolvers.pypackages/nemo_evaluator_sdk/tests/metrics/test_bleu.pypackages/nemo_evaluator_sdk/tests/metrics/test_f1.pypackages/nemo_evaluator_sdk/tests/metrics/test_number_check.pypackages/nemo_evaluator_sdk/tests/metrics/test_rouge.pypackages/nemo_evaluator_sdk/tests/metrics/test_string_check.pypackages/nemo_evaluator_sdk/tests/metrics/test_tool_calling.pypackages/nemo_evaluator_sdk/tests/test_api.pypackages/nemo_platform_ext/src/nemo_platform_ext/skills/nemo-evaluator/references/sdk-execution.mdplugins/nemo-evaluator/README.mdplugins/nemo-evaluator/examples/plugin_examples.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/_agent_eval_bundle.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/_agent_eval_executor.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/_executor.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/agent_eval_job_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.pyplugins/nemo-evaluator/tests/test_agent_eval_executor.pyplugins/nemo-evaluator/tests/test_evaluate_job.pyplugins/nemo-evaluator/tests/test_sdk.pyplugins/nemo-evaluator/tests/test_skill_examples.pyskills/nemo-evaluator-plugin/SKILL.mdskills/nemo-evaluator-plugin/assets/examples/plugin_sdk_examples.pyskills/nemo-evaluator-plugin/references/execution.md
SandyChapman
force-pushed
the
evaluator-agent-eval-submit/schapman
branch
from
August 9, 2026 23:41
6e36303 to
740c115
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/execution/jobs.py`:
- Around line 136-147: Serialize the completion check and _run() execution in
the job’s wait method with a single asyncio.Lock, so concurrent callers cannot
both start the once-only operation. Keep existing timeout handling, error
caching, and result propagation intact, and add a regression test that awaits
the same job concurrently and verifies _run() executes only once.
🪄 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: ac743aef-2c77-414f-ba68-64db7ce95b2b
⛔ Files ignored due to path filters (4)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/evaluator.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/backends/base.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/backends/local/backend.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/jobs.pyis excluded by!sdk/**
📒 Files selected for processing (5)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/evaluator.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/backends/base.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/backends/local/backend.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/jobs.pypackages/nemo_evaluator_sdk/tests/execution/test_evaluator.py
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/backends/base.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/backends/local/backend.py
SandyChapman
force-pushed
the
evaluator-agent-eval-submit/schapman
branch
from
August 9, 2026 23:58
740c115 to
8ef831e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/nemo_evaluator_sdk/tests/execution/test_evaluator.py (2)
86-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_CompletedSyncJob.waitsis never asserted.No test reads
waits. Either assert the forwarded wait parameters intest_submit_bridges_a_sync_backend_job_off_the_loop, or drop the field.🤖 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/execution/test_evaluator.py` around lines 86 - 101, Update test_submit_bridges_a_sync_backend_job_off_the_loop to assert the _CompletedSyncJob.waits entry contains the forwarded poll_interval_seconds, job_timeout_seconds, and pending_timeout_seconds values; alternatively, remove the unused waits field and its recording logic if those parameters are not part of the test’s assertions.
636-643: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCancelled task is never awaited; the test also reaches into
LocalJob._task.
job._task.cancel()returns before the task processes the cancellation, so asyncio can emit "Task was destroyed but it is pending!" and pollute output. Await the cancellation and suppressCancelledError. Consider exposing acancel()onLocalJobif backends need it.🧹 Proposed fix
def test_get_result_before_the_run_finishes_says_so(self): async def _drive(): - job = LocalJob(asyncio.create_task(asyncio.sleep(30))) + task = asyncio.create_task(asyncio.sleep(30)) + job = LocalJob(task) with pytest.raises(RuntimeError, match="has not finished yet"): await job.get_result() - job._task.cancel() + task.cancel() + with contextlib.suppress(asyncio.CancelledError): + await task asyncio.run(_drive())Add
import contextlibat the top of the file.🤖 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/execution/test_evaluator.py` around lines 636 - 643, Update test_get_result_before_the_run_finishes_says_so to await the cancelled task and suppress asyncio.CancelledError, using contextlib as suggested, instead of directly cancelling LocalJob._task; if cancellation belongs in the public API, use or add LocalJob.cancel() and await its completion.
🤖 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/tests/execution/test_evaluator.py`:
- Around line 645-663: Make
test_job_timeout_gives_up_waiting_without_cancelling_the_run deterministic by
increasing _run’s sleep duration substantially relative to the 0.01-second
timeout, such as 1.0 second, and remove the finished == [1] assertion and any
now-unnecessary tracking. Preserve verification that the later wait collects
_TASKSET_RESULT without cancelling the run.
---
Nitpick comments:
In `@packages/nemo_evaluator_sdk/tests/execution/test_evaluator.py`:
- Around line 86-101: Update test_submit_bridges_a_sync_backend_job_off_the_loop
to assert the _CompletedSyncJob.waits entry contains the forwarded
poll_interval_seconds, job_timeout_seconds, and pending_timeout_seconds values;
alternatively, remove the unused waits field and its recording logic if those
parameters are not part of the test’s assertions.
- Around line 636-643: Update test_get_result_before_the_run_finishes_says_so to
await the cancelled task and suppress asyncio.CancelledError, using contextlib
as suggested, instead of directly cancelling LocalJob._task; if cancellation
belongs in the public API, use or add LocalJob.cancel() and await its
completion.
🪄 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: f0cbeec1-b957-4c11-8df0-1fc908a59cf8
⛔ Files ignored due to path filters (2)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/backends/local/backend.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/jobs.pyis excluded by!sdk/**
📒 Files selected for processing (3)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/backends/local/backend.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/jobs.pypackages/nemo_evaluator_sdk/tests/execution/test_evaluator.py
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/backends/local/backend.py
SandyChapman
force-pushed
the
evaluator-agent-eval-submit/schapman
branch
from
August 10, 2026 01:06
8ef831e to
b6037de
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/nemo_evaluator_sdk/tests/execution/backends/local/test_backend.py (1)
33-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
LocalBackend.evaluate. This file only exercisesevaluate_dataset. The new taskset path addsvalidate_run_inputsand per-task metric resolution throughprepare_metric_for_executionin_run_taskset, and neither is covered here. Add tests that assert invalid input raises before the job is created, and that a task metric carrying aModelRef/SecretRefreachesAgentEvaluatorresolved.🤖 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/execution/backends/local/test_backend.py` around lines 33 - 34, Add focused tests for LocalBackend.evaluate covering _run_taskset: verify invalid inputs raise via validate_run_inputs before any job is created, and verify task metrics containing ModelRef or SecretRef are resolved through prepare_metric_for_execution before being passed to AgentEvaluator. Reuse the existing test fixtures and mocks, and assert both the failure ordering and resolved metric delegation.
🤖 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 `@plugins/nemo-evaluator/src/nemo_evaluator/sdk/job_resources.py`:
- Around line 42-44: Update the comment immediately above
_RES_FULL_RESULT_DOWNLOAD to remove the obsolete references to aggregate and
row-score artifacts, leaving only an accurate description of the full result
download route.
---
Nitpick comments:
In `@packages/nemo_evaluator_sdk/tests/execution/backends/local/test_backend.py`:
- Around line 33-34: Add focused tests for LocalBackend.evaluate covering
_run_taskset: verify invalid inputs raise via validate_run_inputs before any job
is created, and verify task metrics containing ModelRef or SecretRef are
resolved through prepare_metric_for_execution before being passed to
AgentEvaluator. Reuse the existing test fixtures and mocks, and assert both the
failure ordering and resolved metric delegation.
🪄 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: 1676d732-1f31-4e5f-bbbd-03ffe299c132
⛔ Files ignored due to path filters (5)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/backends/base.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/backends/local/backend.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/evaluator.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/jobs.pyis excluded by!sdk/**
📒 Files selected for processing (12)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/__init__.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/backends/base.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/backends/local/backend.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/evaluator.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/jobs.pypackages/nemo_evaluator_sdk/tests/execution/backends/local/test_backend.pypackages/nemo_evaluator_sdk/tests/execution/test_evaluator.pypackages/nemo_evaluator_sdk/tests/execution/test_metric_execution.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/_agent_eval_executor.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/agent_eval_job_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/job_resources.pyplugins/nemo-evaluator/tests/test_sdk_job_resources.py
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/agent_eval_job_resources.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/jobs.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/_agent_eval_executor.py
- packages/nemo_evaluator_sdk/tests/execution/test_metric_execution.py
15 tasks
SandyChapman
force-pushed
the
evaluator-agent-eval-submit/schapman
branch
4 times, most recently
from
August 10, 2026 12:09
3098e8e to
6929a38
Compare
Contributor
SandyChapman
force-pushed
the
evaluator-agent-eval-submit/schapman
branch
7 times, most recently
from
August 10, 2026 18:52
4e86660 to
5c95cb3
Compare
SandyChapman
added a commit
that referenced
this pull request
Aug 11, 2026
…change #1173 retires `run_sync`/`submit` for `run_dataset_sync`/`evaluate_dataset`, which needed the same six skill files updated and so put that PR behind the NVSkills gate too. Rather than open a second skills PR that waits on the same broken gate, its skill updates join this one. `plugin_sdk_examples.py` is the one file both PRs touch, and they touch different functions — #1071 moves `store_resources` to the discriminated `spec`, #1173 moves `evaluate_standalone` and `submit_and_collect` to the dataset API — so the two are merged here rather than one overwriting the other. `test_skill_examples.py` likewise carries both sets of assertions. `test_skill_standalone_example_scores_pass_and_failure` fails on this branch and is expected to: it calls `run_dataset_sync`, which exists on #1173's branch, not on this PR's #1071 base. It passes once #1173 lands. It is left failing rather than skipped, so that it is re-verified for real instead of quietly staying off. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
15 tasks
…_dataset The SDK backend protocol carried two dataset entrypoints that differed only in metric arity and return type: `evaluate` took one metric and returned an `EvaluationResult`, `evaluate_benchmark` took a list and returned a `BenchmarkEvaluationResult`. One metric is the degenerate case of several, so collapse them into a single `evaluate_dataset(metrics=[...])` and free the `evaluate` name for the taskset entrypoint. `Evaluator.run`/`run_sync` lose their single-metric overloads; callers pass a list. `BenchmarkEvaluationResult` gains the `__str__` that `EvaluationResult` had, so `print(result)` stays readable through the collapsed path. Introduce a job-handle contract. `EvaluationJob`/`SyncEvaluationJob` declare `wait_until_done` and `get_result`, so a backend that runs work elsewhere hands back a handle and the caller chooses when to wait; in-process execution returns an already-finished `CompletedJob`. `Evaluator.submit` waits on the caller's behalf, so the convenience API still returns a result either way. The plugin's job resources satisfy that contract structurally, so both `evaluate` and `evaluate_dataset` now return handles and the resource is consistent. The agent-eval handle carries the taskset it submitted: rebuilding the result needs the caller's live tasks, because persisted task metrics serialize as descriptors that cannot be validated back into `Metric` objects, and holding them on the handle means no caller has to know that. Polling and bundle reassembly move out of the executor onto the handle, which drops the duplicated URL, wait, and download logic from both executor flavours and replaces two hard-coded timeout constants with the three the dataset handles already take. Retire plugin-local execution. `client.evaluator.run()` is gone and `submit` is now `evaluate_dataset`, taking a metric list. That orphaned the executor's local paths, so remove `run_local`, `evaluate`, `evaluate_benchmark` and `evaluate_remote` from both executors along with the two metric type-guards whose only purpose was the single-vs-sequence dispatch. Reject two silent failures the agent-eval path allowed: a task with no instruction (the wire schema permits null, so a job would reach the agent with nothing to do) and run params that do not match the target kind (previously dropped, so a caller's parallelism vanished without a word). BREAKING CHANGE: `EvaluationBackend.evaluate` now takes a taskset and returns an `EvaluationJob`; dataset evaluation moves to `evaluate_dataset` with a metric list. `Evaluator.run`/`run_sync` no longer accept a bare metric. `client.evaluator.run()` is removed, `client.evaluator.submit()` is now `client.evaluator.evaluate_dataset()`, and `client.evaluator.evaluate()` returns a job handle rather than a completed result. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
Retiring `run_sync`/`submit` in favour of `run_dataset_sync`/`evaluate_dataset` meant updating the evaluator skill to match, which put six `skills/` files in the diff and so put this PR behind the NVSkills gate. That gate cannot currently pass: tier 3 is invoked with `--env-mode local`, whose bubblewrap sandbox fails its smoke test on the nvcarps runners, so nothing is evaluated and it blocks on empty coverage. It is an infrastructure problem, already reported, and nothing in this repo can resolve it. With no `skills/` file touched, the gate no longer applies and the backend contract change can land on its own merits. The skill updates move to #1237, which can sit behind the gate for as long as it takes. Unlike the equivalent split on #1071, this one has a cost worth naming. The skill's `evaluate_standalone` example is *executed* by `test_skill_standalone_example_scores_pass_and_failure`, and the reverted example calls the retired `Evaluator.run_sync`, so the test now fails for a real reason: the shipped example is genuinely broken against this refactor. It is skipped rather than deleted, with the reason and the restoring PR named in the marker, so the gap is visible and expires. The other 29 tests in that file still run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
The poll loop billed both ceilings from one wall-clock reading, so a job that ran longer than `pending_timeout_seconds` and then reported a pending status tripped the pending ceiling and was reported as never having started. The dataset handles in `job_resources` already keep the two totals apart; this brings the agent-eval handles in line with a `_Clock` that bills each tick to whichever total the status belongs to. `_PENDING` also carried `queued` and `scheduled`, neither of which exists in `PlatformJobStatus`. It now names the enum's pre-start members, so every other non-terminal status is charged against the job ceiling. Nothing covered either ceiling, which is why this survived. Four tests drive a controlled clock through the cases that distinguish them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
`test_prefers_sync_sdk_for_fileset_ref_when_both_sdks_injected` arrived from main in #1211, after this branch renamed the evaluate job's dataset call to `run_dataset_sync`. It mocked `run_sync`, so the job reached an auto-created Mock instead, which failed on the way into the result artifact as `TypeError: Object of type Mock is not JSON serializable`. Its four siblings in the same class already mock `run_dataset_sync` with the same helper; this brings the newcomer in line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
evaluator-agent-eval-submit/schapman
branch
from
August 12, 2026 16:03
415b724 to
8f51e4f
Compare
SandyChapman
added a commit
that referenced
this pull request
Aug 20, 2026
The skill drifted from the code because the NVSkills CI gate blocked any PR touching top-level `skills/`, so several evaluator changes landed with their docs updated and the skill left behind. That gate is gone as of #1302, so this catches the skill up. Stored tasks (#1071, #566). `TaskInput` now carries a runner-discriminated `spec`, and `EvaluatorTaskDefinition` has the grader-only `reference` field. The skill still showed the flat pre-#1071 shape and told readers that held-out ground truth required an inline `AgentEvalTaskInput` -- which would cost them tasksets and revision pinning for a limitation that no longer exists. Three places said it; all three are corrected. Local execution (#1262). The skill lumped `client.evaluator.run()` together with the `nemo evaluator ... run` CLI verb as "being retired", but only the CLI verb still exists -- the method was removed a week ago. SKILL.md now warns about the CLI verb alone: naming a method that cannot be called, four lines from the seven live `.run()` calls the skill teaches (`AgentEvaluator().run`, `Evaluator().run_sync`), invited the wrong generalization. The removal is recorded in `troubleshooting.md` instead, which is symptom-indexed and so only reached by someone who already called it from memory. `GymRunnerTarget` was also missing from SKILL.md's platform-target list, alongside the same omission in the agent-evaluation reference. Taskset submission (#1367). `submit` grew a second shape -- `tasks` + `target` against a live runner -- which was previously CLI-only and went out with no skill or docs coverage. Added to the interface table and the agent-evaluation reference, along with `GymRunnerTarget` in the target table, the four row-only options the taskset path refuses, and the Gym-only translation limit. The returned `AgentEvaluatorJobResource` deliberately has no `get_result()` or `download_artifacts()`, while every other job example in the skill ends in `get_result()`. That trap gets its own troubleshooting row. `evals.json` graded the agent on producing `nemo evaluator evaluate run --spec`, the very path SKILL.md says not to build on. Both verbs take identical spec flags, so the eval was rewarding the discouraged one for no benefit. Deliberately NOT included: the skill updates written for #1173. That PR closed unmerged, so `Evaluator.run_dataset_sync` and `client.evaluator.evaluate_dataset` do not exist. `evaluate_dataset` on main is the *backend* contract method, which makes the rename look landed when it is not. The public surface is still `run_sync` and `submit(metric=..., config=...)`. Every claim was verified by executing it against main rather than reading the source, which caught two errors in my own first draft: an example missing the required `resources_server`, and a claim that `env_vars` can hold a callable. It cannot -- it is `dict[str, str]`, so pydantic refuses one at construction and it never reaches the serializability guard. Only `hydra_params` is `dict[str, Any]`. (The `_gym_target` docstring names both and is likewise overstated, but that is merged code and out of scope here.) Four tests added, each mutation-verified. The largest gap they close is that `store_resources` -- the skill's canonical stored-task example -- was only ever asserted as text, so no schema change to `TaskInput` could fail it. It now runs against the real resource signatures and re-validates through the wire form `create` actually posts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
added a commit
that referenced
this pull request
Aug 20, 2026
The skill drifted from the code because the NVSkills CI gate blocked any PR touching top-level `skills/`, so several evaluator changes landed with their docs updated and the skill left behind. That gate is gone as of #1302, so this catches the skill up. Stored tasks (#1071, #566). `TaskInput` now carries a runner-discriminated `spec`, and `EvaluatorTaskDefinition` has the grader-only `reference` field. The skill still showed the flat pre-#1071 shape and told readers that held-out ground truth required an inline `AgentEvalTaskInput` -- which would cost them tasksets and revision pinning for a limitation that no longer exists. Three places said it; all three are corrected. Local execution (#1262). The skill lumped `client.evaluator.run()` together with the `nemo evaluator ... run` CLI verb as "being retired", but only the CLI verb still exists -- the method was removed a week ago. SKILL.md now warns about the CLI verb alone: naming a method that cannot be called, four lines from the seven live `.run()` calls the skill teaches (`AgentEvaluator().run`, `Evaluator().run_sync`), invited the wrong generalization. The removal is recorded in `troubleshooting.md` instead, which is symptom-indexed and so only reached by someone who already called it from memory. `GymRunnerTarget` was also missing from SKILL.md's platform-target list, alongside the same omission in the agent-evaluation reference. Taskset submission (#1367). `submit` grew a second shape -- `tasks` + `target` against a live runner -- which was previously CLI-only and went out with no skill or docs coverage. Added to the interface table and the agent-evaluation reference, along with `GymRunnerTarget` in the target table, the four row-only options the taskset path refuses, and the Gym-only translation limit. The returned `AgentEvaluatorJobResource` deliberately has no `get_result()` or `download_artifacts()`, while every other job example in the skill ends in `get_result()`. That trap gets its own troubleshooting row. `evals.json` graded the agent on producing `nemo evaluator evaluate run --spec`, the very path SKILL.md says not to build on. Both verbs take identical spec flags, so the eval was rewarding the discouraged one for no benefit. Deliberately NOT included: the skill updates written for #1173. That PR closed unmerged, so `Evaluator.run_dataset_sync` and `client.evaluator.evaluate_dataset` do not exist. `evaluate_dataset` on main is the *backend* contract method, which makes the rename look landed when it is not. The public surface is still `run_sync` and `submit(metric=..., config=...)`. Every claim was verified by executing it against main rather than reading the source, which caught two errors in my own first draft: an example missing the required `resources_server`, and a claim that `env_vars` can hold a callable. It cannot -- it is `dict[str, str]`, so pydantic refuses one at construction and it never reaches the serializability guard. Only `hydra_params` is `dict[str, Any]`. (The `_gym_target` docstring names both and is likewise overstated, but that is merged code and out of scope here.) Four tests added, each mutation-verified. The largest gap they close is that `store_resources` -- the skill's canonical stored-task example -- was only ever asserted as text, so no schema change to `TaskInput` could fail it. It now runs against the real resource signatures and re-validates through the wire form `create` actually posts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
added a commit
that referenced
this pull request
Aug 20, 2026
The skill drifted from the code because the NVSkills CI gate blocked any PR touching top-level `skills/`, so several evaluator changes landed with their docs updated and the skill left behind. That gate is gone as of #1302, so this catches the skill up. Stored tasks (#1071, #566). `TaskInput` now carries a runner-discriminated `spec`, and `EvaluatorTaskDefinition` has the grader-only `reference` field. The skill still showed the flat pre-#1071 shape and told readers that held-out ground truth required an inline `AgentEvalTaskInput` -- which would cost them tasksets and revision pinning for a limitation that no longer exists. Three places said it; all three are corrected. Local execution (#1262). The skill lumped `client.evaluator.run()` together with the `nemo evaluator ... run` CLI verb as "being retired", but only the CLI verb still exists -- the method was removed a week ago. SKILL.md now warns about the CLI verb alone: naming a method that cannot be called, four lines from the seven live `.run()` calls the skill teaches (`AgentEvaluator().run`, `Evaluator().run_sync`), invited the wrong generalization. The removal is recorded in `troubleshooting.md` instead, which is symptom-indexed and so only reached by someone who already called it from memory. `GymRunnerTarget` was also missing from SKILL.md's platform-target list, alongside the same omission in the agent-evaluation reference. Taskset submission (#1367). `submit` grew a second shape -- `tasks` + `target` against a live runner -- which was previously CLI-only and went out with no skill or docs coverage. Added to the interface table and the agent-evaluation reference, along with `GymRunnerTarget` in the target table, the four row-only options the taskset path refuses, and the Gym-only translation limit. The returned `AgentEvaluatorJobResource` deliberately has no `get_result()` or `download_artifacts()`, while every other job example in the skill ends in `get_result()`. That trap gets its own troubleshooting row. `evals.json` graded the agent on producing `nemo evaluator evaluate run --spec`, the very path SKILL.md says not to build on. Both verbs take identical spec flags, so the eval was rewarding the discouraged one for no benefit. Deliberately NOT included: the skill updates written for #1173. That PR closed unmerged, so `Evaluator.run_dataset_sync` and `client.evaluator.evaluate_dataset` do not exist. `evaluate_dataset` on main is the *backend* contract method, which makes the rename look landed when it is not. The public surface is still `run_sync` and `submit(metric=..., config=...)`. Every claim was verified by executing it against main rather than reading the source, which caught two errors in my own first draft: an example missing the required `resources_server`, and a claim that `env_vars` can hold a callable. It cannot -- it is `dict[str, str]`, so pydantic refuses one at construction and it never reaches the serializability guard. Only `hydra_params` is `dict[str, Any]`. (The `_gym_target` docstring names both and is likewise overstated, but that is merged code and out of scope here.) Four tests added, each mutation-verified. The largest gap they close is that `store_resources` -- the skill's canonical stored-task example -- was only ever asserted as text, so no schema change to `TaskInput` could fail it. It now runs against the real resource signatures and re-validates through the wire form `create` actually posts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
added a commit
that referenced
this pull request
Aug 20, 2026
The skill drifted from the code because the NVSkills CI gate blocked any PR touching top-level `skills/`, so several evaluator changes landed with their docs updated and the skill left behind. That gate is gone as of #1302, so this catches the skill up. Stored tasks (#1071, #566). `TaskInput` now carries a runner-discriminated `spec`, and `EvaluatorTaskDefinition` has the grader-only `reference` field. The skill still showed the flat pre-#1071 shape and told readers that held-out ground truth required an inline `AgentEvalTaskInput` -- which would cost them tasksets and revision pinning for a limitation that no longer exists. Three places said it; all three are corrected. Local execution (#1262). The skill lumped `client.evaluator.run()` together with the `nemo evaluator ... run` CLI verb as "being retired", but only the CLI verb still exists -- the method was removed a week ago. SKILL.md now warns about the CLI verb alone: naming a method that cannot be called, four lines from the seven live `.run()` calls the skill teaches (`AgentEvaluator().run`, `Evaluator().run_sync`), invited the wrong generalization. The removal is recorded in `troubleshooting.md` instead, which is symptom-indexed and so only reached by someone who already called it from memory. `GymRunnerTarget` was also missing from SKILL.md's platform-target list, alongside the same omission in the agent-evaluation reference. Taskset submission (#1367). `submit` grew a second shape -- `tasks` + `target` against a live runner -- which was previously CLI-only and went out with no skill or docs coverage. Added to the interface table and the agent-evaluation reference, along with `GymRunnerTarget` in the target table, the four row-only options the taskset path refuses, and the Gym-only translation limit. The returned `AgentEvaluatorJobResource` deliberately has no `get_result()` or `download_artifacts()`, while every other job example in the skill ends in `get_result()`. That trap gets its own troubleshooting row. `evals.json` graded the agent on producing `nemo evaluator evaluate run --spec`, the very path SKILL.md says not to build on. Both verbs take identical spec flags, so the eval was rewarding the discouraged one for no benefit. Deliberately NOT included: the skill updates written for #1173. That PR closed unmerged, so `Evaluator.run_dataset_sync` and `client.evaluator.evaluate_dataset` do not exist. `evaluate_dataset` on main is the *backend* contract method, which makes the rename look landed when it is not. The public surface is still `run_sync` and `submit(metric=..., config=...)`. Every claim was verified by executing it against main rather than reading the source, which caught two errors in my own first draft: an example missing the required `resources_server`, and a claim that `env_vars` can hold a callable. It cannot -- it is `dict[str, str]`, so pydantic refuses one at construction and it never reaches the serializability guard. Only `hydra_params` is `dict[str, Any]`. (The `_gym_target` docstring names both and is likewise overstated, but that is merged code and out of scope here.) Four tests added, each mutation-verified. The largest gap they close is that `store_resources` -- the skill's canonical stored-task example -- was only ever asserted as text, so no schema change to `TaskInput` could fail it. It now runs against the real resource signatures and re-validates through the wire form `create` actually posts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
added a commit
that referenced
this pull request
Aug 21, 2026
The skill drifted from the code because the NVSkills CI gate blocked any PR touching top-level `skills/`, so several evaluator changes landed with their docs updated and the skill left behind. That gate is gone as of #1302, so this catches the skill up. Stored tasks (#1071, #566). `TaskInput` now carries a runner-discriminated `spec`, and `EvaluatorTaskDefinition` has the grader-only `reference` field. The skill still showed the flat pre-#1071 shape and told readers that held-out ground truth required an inline `AgentEvalTaskInput` -- which would cost them tasksets and revision pinning for a limitation that no longer exists. Three places said it; all three are corrected. Local execution (#1262). The skill lumped `client.evaluator.run()` together with the `nemo evaluator ... run` CLI verb as "being retired", but only the CLI verb still exists -- the method was removed a week ago. SKILL.md now warns about the CLI verb alone: naming a method that cannot be called, four lines from the seven live `.run()` calls the skill teaches (`AgentEvaluator().run`, `Evaluator().run_sync`), invited the wrong generalization. The removal is recorded in `troubleshooting.md` instead, which is symptom-indexed and so only reached by someone who already called it from memory. `GymRunnerTarget` was also missing from SKILL.md's platform-target list, alongside the same omission in the agent-evaluation reference. Taskset submission (#1367). `submit` grew a second shape -- `tasks` + `target` against a live runner -- which was previously CLI-only and went out with no skill or docs coverage. Added to the interface table and the agent-evaluation reference, along with `GymRunnerTarget` in the target table, the four row-only options the taskset path refuses, and the Gym-only translation limit. The returned `AgentEvaluatorJobResource` deliberately has no `get_result()` or `download_artifacts()`, while every other job example in the skill ends in `get_result()`. That trap gets its own troubleshooting row. `evals.json` graded the agent on producing `nemo evaluator evaluate run --spec`, the very path SKILL.md says not to build on. Both verbs take identical spec flags, so the eval was rewarding the discouraged one for no benefit. Deliberately NOT included: the skill updates written for #1173. That PR closed unmerged, so `Evaluator.run_dataset_sync` and `client.evaluator.evaluate_dataset` do not exist. `evaluate_dataset` on main is the *backend* contract method, which makes the rename look landed when it is not. The public surface is still `run_sync` and `submit(metric=..., config=...)`. Every claim was verified by executing it against main rather than reading the source, which caught two errors in my own first draft: an example missing the required `resources_server`, and a claim that `env_vars` can hold a callable. It cannot -- it is `dict[str, str]`, so pydantic refuses one at construction and it never reaches the serializability guard. Only `hydra_params` is `dict[str, Any]`. (The `_gym_target` docstring names both and is likewise overstated, but that is merged code and out of scope here.) Four tests added, each mutation-verified. The largest gap they close is that `store_resources` -- the skill's canonical stored-task example -- was only ever asserted as text, so no schema change to `TaskInput` could fail it. It now runs against the real resource signatures and re-validates through the wire form `create` actually posts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
added a commit
that referenced
this pull request
Aug 21, 2026
The skill drifted from the code because the NVSkills CI gate blocked any PR touching top-level `skills/`, so several evaluator changes landed with their docs updated and the skill left behind. That gate is gone as of #1302, so this catches the skill up. Stored tasks (#1071, #566). `TaskInput` now carries a runner-discriminated `spec`, and `EvaluatorTaskDefinition` has the grader-only `reference` field. The skill still showed the flat pre-#1071 shape and told readers that held-out ground truth required an inline `AgentEvalTaskInput` -- which would cost them tasksets and revision pinning for a limitation that no longer exists. Three places said it; all three are corrected. Local execution (#1262). The skill lumped `client.evaluator.run()` together with the `nemo evaluator ... run` CLI verb as "being retired", but only the CLI verb still exists -- the method was removed a week ago. SKILL.md now warns about the CLI verb alone: naming a method that cannot be called, four lines from the seven live `.run()` calls the skill teaches (`AgentEvaluator().run`, `Evaluator().run_sync`), invited the wrong generalization. The removal is recorded in `troubleshooting.md` instead, which is symptom-indexed and so only reached by someone who already called it from memory. `GymRunnerTarget` was also missing from SKILL.md's platform-target list, alongside the same omission in the agent-evaluation reference. Taskset submission (#1367). `submit` grew a second shape -- `tasks` + `target` against a live runner -- which was previously CLI-only and went out with no skill or docs coverage. Added to the interface table and the agent-evaluation reference, along with `GymRunnerTarget` in the target table, the four row-only options the taskset path refuses, and the Gym-only translation limit. The returned `AgentEvaluatorJobResource` deliberately has no `get_result()` or `download_artifacts()`, while every other job example in the skill ends in `get_result()`. That trap gets its own troubleshooting row. `evals.json` graded the agent on producing `nemo evaluator evaluate run --spec`, the very path SKILL.md says not to build on. Both verbs take identical spec flags, so the eval was rewarding the discouraged one for no benefit. Deliberately NOT included: the skill updates written for #1173. That PR closed unmerged, so `Evaluator.run_dataset_sync` and `client.evaluator.evaluate_dataset` do not exist. `evaluate_dataset` on main is the *backend* contract method, which makes the rename look landed when it is not. The public surface is still `run_sync` and `submit(metric=..., config=...)`. Every claim was verified by executing it against main rather than reading the source, which caught two errors in my own first draft: an example missing the required `resources_server`, and a claim that `env_vars` can hold a callable. It cannot -- it is `dict[str, str]`, so pydantic refuses one at construction and it never reaches the serializability guard. Only `hydra_params` is `dict[str, Any]`. (The `_gym_target` docstring names both and is likewise overstated, but that is merged code and out of scope here.) Four tests added, each mutation-verified. The largest gap they close is that `store_resources` -- the skill's canonical stored-task example -- was only ever asserted as text, so no schema change to `TaskInput` could fail it. It now runs against the real resource signatures and re-validates through the wire form `create` actually posts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
dnandakumar-nv
pushed a commit
to dnandakumar-nv/nemo-platform
that referenced
this pull request
Aug 21, 2026
The skill drifted from the code because the NVSkills CI gate blocked any PR touching top-level `skills/`, so several evaluator changes landed with their docs updated and the skill left behind. That gate is gone as of NVIDIA-NeMo#1302, so this catches the skill up. Stored tasks (NVIDIA-NeMo#1071, NVIDIA-NeMo#566). `TaskInput` now carries a runner-discriminated `spec`, and `EvaluatorTaskDefinition` has the grader-only `reference` field. The skill still showed the flat pre-NVIDIA-NeMo#1071 shape and told readers that held-out ground truth required an inline `AgentEvalTaskInput` -- which would cost them tasksets and revision pinning for a limitation that no longer exists. Three places said it; all three are corrected. Local execution (NVIDIA-NeMo#1262). The skill lumped `client.evaluator.run()` together with the `nemo evaluator ... run` CLI verb as "being retired", but only the CLI verb still exists -- the method was removed a week ago. SKILL.md now warns about the CLI verb alone: naming a method that cannot be called, four lines from the seven live `.run()` calls the skill teaches (`AgentEvaluator().run`, `Evaluator().run_sync`), invited the wrong generalization. The removal is recorded in `troubleshooting.md` instead, which is symptom-indexed and so only reached by someone who already called it from memory. `GymRunnerTarget` was also missing from SKILL.md's platform-target list, alongside the same omission in the agent-evaluation reference. Taskset submission (NVIDIA-NeMo#1367). `submit` grew a second shape -- `tasks` + `target` against a live runner -- which was previously CLI-only and went out with no skill or docs coverage. Added to the interface table and the agent-evaluation reference, along with `GymRunnerTarget` in the target table, the four row-only options the taskset path refuses, and the Gym-only translation limit. The returned `AgentEvaluatorJobResource` deliberately has no `get_result()` or `download_artifacts()`, while every other job example in the skill ends in `get_result()`. That trap gets its own troubleshooting row. `evals.json` graded the agent on producing `nemo evaluator evaluate run --spec`, the very path SKILL.md says not to build on. Both verbs take identical spec flags, so the eval was rewarding the discouraged one for no benefit. Deliberately NOT included: the skill updates written for NVIDIA-NeMo#1173. That PR closed unmerged, so `Evaluator.run_dataset_sync` and `client.evaluator.evaluate_dataset` do not exist. `evaluate_dataset` on main is the *backend* contract method, which makes the rename look landed when it is not. The public surface is still `run_sync` and `submit(metric=..., config=...)`. Every claim was verified by executing it against main rather than reading the source, which caught two errors in my own first draft: an example missing the required `resources_server`, and a claim that `env_vars` can hold a callable. It cannot -- it is `dict[str, str]`, so pydantic refuses one at construction and it never reaches the serializability guard. Only `hydra_params` is `dict[str, Any]`. (The `_gym_target` docstring names both and is likewise overstated, but that is merged code and out of scope here.) Four tests added, each mutation-verified. The largest gap they close is that `store_resources` -- the skill's canonical stored-task example -- was only ever asserted as text, so no schema change to `TaskInput` could fail it. It now runs against the real resource signatures and re-validates through the wire form `create` actually posts. Signed-off-by: Sandy Chapman <schapman@nvidia.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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
The evaluator SDK carried two dataset entrypoints that differed only in metric arity and return type —
evaluatetook one metric and returned anEvaluationResult,evaluate_benchmarktook a list and returned aBenchmarkEvaluationResult. One metric is the degenerate case of several, so this collapses them intoevaluate_dataset(metrics=[...])and frees theevaluatename for the taskset entrypoint. It also introduces a job-handle contract (wait_until_done+get_result) that both evaluation paths return, so the plugin resource is consistent and satisfies the backend protocol structurally.Related Issue
None.
Changes
SDK backend contract (
packages/nemo_evaluator_sdk)EvaluationBackend/SyncEvaluationBackendnow declareevaluate(taskset=...)andevaluate_dataset(metrics=[...]).evaluate_benchmarkis gone.Evaluator.run/run_synclose their single-metric overloads (6 dropped, 3 remain, discriminating on config/target rather than metric arity).BenchmarkEvaluationResultgains the__str__EvaluationResultalready had, soprint(result)stays readable through the collapsed path instead of falling back to a pydantic dump.is_metric/is_metric_sequence, whose only purpose was the single-vs-sequence dispatch.Job-handle contract (new
execution/jobs.py)EvaluationJob[ResultT]/SyncEvaluationJob[ResultT]declarewait_until_doneandget_result. A backend that runs work elsewhere returns a handle so the caller chooses when to wait; in-process execution returns an already-finishedCompletedJob.Evaluator.submit/submit_syncwait on the caller's behalf, so the public SDK API still returns anAgentEvalResult.Plugin agent evaluation (
plugins/nemo-evaluator)agent_eval_job_resources.pyhandles satisfy the SDK contract structurally, so the SDK imports nothing from the plugin. Bothevaluateandevaluate_datasetreturn handles.Metricobjects. Holding them on the handle means no caller has to know that.parallelismvanished without a word).Retiring plugin-local execution
client.evaluator.run()removed;client.evaluator.submit()renamed toevaluate_dataset()and now takes a metric list.run_local,evaluate,evaluate_benchmarkandevaluate_remoteare removed from both executors, together with two dead spec resolvers.bundle_metrics_for_spechandles sequences only.Docs, examples, skills, e2e
metrics=[...]andevaluate_datasetacross README, skill references, examples, the walkthrough notebook, and the e2e suite.plugin_examples.pyloses itsexecution_modeswitch, which selected between a local path that no longer exists and the platform path.make vendorpersdk/python/AGENTS.md.Type of Change
Quality Gates
New
tests/test_agent_eval_executor.pycovers the previously untested executor and handle: spec construction, metric packaging and the cloudpickle opt-in, the two new rejections, bundle reading including the traversal case, result assembly, and create/wait/get_result on both flavours.Evaluator.submithad no coverage at all before this change;TestEvaluatorSubmitnow covers it, including that a sync backend's handle is driven off the event loop. Tests for the removed local-execution paths were deleted rather than left pinning dead code.Verification
Signed-off-by:traileruv run pre-commit run -apasses, or any blocked checks are identified belowTargeted validation, after rebasing onto
main:uv run --frozen pytest packages/nemo_evaluator_sdk/tests -quv run --frozen pytest plugins/nemo-evaluator/tests -q --ignore=.../integrationuv run ruff check/ruff format --checkbash tools/lint/run-ty-check.shmake vendoruv run pre-commit run -aRun uv lock with platform uv— see belowRun uv lock with platform uvfails locally because the hook requires uv 0.9.14 exactly and this machine has 0.9.30; it exits on the version check before reading any file.Check for uv.lock driftpasses and this PR changes nopyproject.toml, so there is nothing to relock. Flagging it rather than marking the gate passed.Integration tests were not run — they need a live platform.
Limitations
EvaluatorJobResource.get_result()still returns anEvaluationResultrather than aBenchmarkEvaluationResult, soevaluate_datasetsatisfies the handle contract but not its result type. The fullBenchmarkEvaluationResultis already persisted as theevaluation-resultsartifact, so closing this is a download-and-validate against an existing route.CompletedJob; a third-party sync backend would need the equivalent sync shape, which currently exists only as a test double.Summary by CodeRabbit
New Features
evaluate_datasetfor synchronous and asynchronous clients.Breaking Changes
evaluate_datasetand metrics lists.