feat(evaluator): unify dataset + task evaluation under one Evaluator#736
Draft
SandyChapman wants to merge 2 commits into
Draft
feat(evaluator): unify dataset + task evaluation under one Evaluator#736SandyChapman wants to merge 2 commits into
SandyChapman wants to merge 2 commits into
Conversation
Merge the two SDK entrypoints (dataset-driven `Evaluator` and task-driven `AgentEvaluator`) into a single `Evaluator` whose methods mirror the backend protocol one-to-one, so moving local -> platform and dataset -> agent eval is a matter of which backend you inject. SDK core (packages/nemo_evaluator_sdk): - Backend protocol gains `evaluate_tasks`; `evaluate` -> `evaluate_metric` so the trio (`evaluate_metric` / `evaluate_benchmark` / `evaluate_tasks`) is parallel. - `Evaluator` exposes `run_dataset_metric_eval` / `run_dataset_benchmark_eval` / `run_task_eval` (+ `_sync`), each mapping to one backend op; `run` / `run_sync` remain backward-compatible dataset dispatchers. - Online-inference transport moves onto `LocalBackend`; `AgentEvaluator` becomes a thin deprecated shim forwarding to `Evaluator().run_task_eval`. Plugin (plugins/nemo-evaluator): - The executor implements the full backend protocol, rejecting local hooks and, for tasks, building an AgentEvalInputSpec + running the agent-eval job locally. - Exposed via `client.evaluator.as_backend()`, so a NeMoPlatform evaluator resource can back the unified `Evaluator` for both dataset and task evaluation. - Job runner migrated off the deprecated shim. Draft for team input on the UX + naming. Signed-off-by: Sandy Chapman <schapman@nvidia.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
| return namespace_result(metric_key, result, aggregate_fields) | ||
|
|
||
| async def evaluate( | ||
| async def evaluate_metric( |
Contributor
There was a problem hiding this comment.
for consistency, should it be evaluate_datasets?
| Raises: | ||
| ValueError: If both ``inference_fn`` and ``agent_inference_fn_factory`` are set. | ||
| """ | ||
| if inference_fn is not None and agent_inference_fn_factory is not None: |
Contributor
There was a problem hiding this comment.
also need a check that only one or the other is provided
Comment on lines
+261
to
+262
| tasks: Tasks to evaluate; each task carries its own metrics. | ||
| trials: Precomputed trials to score. Mutually exclusive with ``target``. |
Contributor
There was a problem hiding this comment.
considering tasks and trials are mutually exclusive, we should expose this, so it's clear before invoking it
| raise TypeError("metrics must be a Metric or a sequence of Metric objects") | ||
| return await self._backend.evaluate( | ||
| metric=metrics, | ||
| async def run( |
Contributor
There was a problem hiding this comment.
should probably become run_dataset_eval since it's specific to that path
|
|
||
| from nemo_evaluator_sdk.execution.evaluator import Evaluator | ||
|
|
||
| evaluator = Evaluator(client.evaluator.as_backend()) |
Contributor
There was a problem hiding this comment.
are we going to allow sdk to be an interface with platform again via injected backend? At some point in the past (~April), we reverted this change
Contributor
Author
There was a problem hiding this comment.
We should discuss today if we can find time, otherwise Monday. I'm not sure why we wouldn't support this as it aligns with the early design we had to make moving from SDK to Plugin as straight-forward as possible.
Contributor
|
Some initial thoughts:
|
…ming Address review feedback (Nick) on the unified Evaluator, items 2-5: - Consolidate the split dataset methods back into one overloaded method per layer: `Evaluator.run_dataset_eval` (single metric -> EvaluationResult, sequence -> BenchmarkEvaluationResult) and backend `evaluate_dataset`. `run` / `run_sync` are plain aliases of `run_dataset_eval` again. - Rename the task-eval surface to the `taskset` noun (a collection of tasks): backend `evaluate_taskset`, `Evaluator.run_taskset_eval` (+ `_sync`), `taskset=` parameter. Wire/data-model fields (AgentEvalInputSpec.tasks, AgentEvalResult.tasks) and the runtime runner protocol keep `tasks` (separate breaking-schema concern). - Backend protocol trio is now `evaluate_dataset` / `evaluate_taskset`, mirrored 1:1 by the Evaluator methods. - Expose mutual-exclusivity in the type signature: `run_taskset_eval` overloads on `target` vs `trials`; `LocalBackend.__init__` overloads on `inference_fn` vs `agent_inference_fn_factory`. Backend protocol + adapter + plugin executor stay flat (union input/return) so implementers conform cleanly; the intent-revealing overloads live on the user-facing Evaluator. Signed-off-by: Sandy Chapman <schapman@nvidia.com> Co-Authored-By: Claude Opus 4.8 <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
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.
Draft — seeking team input on the evaluator SDK UX + naming before I polish.
What this does
Merges the two SDK entrypoints — dataset-driven
Evaluatorand task-drivenAgentEvaluator— into a singleEvaluatorwhose methods map 1:1 onto the backend protocol, so local ↔ platform and dataset ↔ agent eval differ only by which backend you inject.Evaluator(public):run_dataset_metric_eval— one metric over a dataset → backendevaluate_metricrun_dataset_benchmark_eval— many metrics over a dataset → backendevaluate_benchmarkrun_task_eval— tasks (each carries its own metrics) → backendevaluate_tasks_synctwin;run/run_syncremain as backward-compatible dataset dispatchers.Backend protocol (
EvaluationBackend/SyncEvaluationBackend):evaluate_metric/evaluate_benchmark/evaluate_tasks.Plugin: the executor implements the full protocol;
client.evaluator.as_backend()returns it, soEvaluator(NeMoPlatform(...).evaluator.as_backend())runs both dataset and task eval through the platform.AgentEvaluatorbecomes a thin deprecated shim; the agent-eval job runner is migrated onto the unified path.Questions for reviewers
run_dataset_metric_eval/run_dataset_benchmark_eval/run_task_evalonEvaluator, mirroringevaluate_metric/evaluate_benchmark/evaluate_taskson the backend. Good alignment, or too verbose?Evaluator(client=NeMoPlatform(...).evaluator)work without the.as_backend()hop (i.e. make the resource itself satisfy the protocol)? Deliberately deferred — a guard test currently keepsevaluate*off the resource surface._synctwins +SyncEvaluationBackend, or go async-only? (Leaning keep: notebook + job-runner callers, andrun_syncis Jupyter-safe.)AgentEvaluatoras a warning-emitting shim →Evaluator().run_task_eval— path OK?Out of scope (follow-ups)
Durable task
submit, the remaining agent-eval UX papercuts, and result-persistence for the new runner targets.