feat(evaluator): persist agent-eval tasks as entities (AALGO-307) - #527
Merged
Conversation
SandyChapman
force-pushed
the
eval-results-persistence/schapman
branch
from
July 2, 2026 13:07
dcef462 to
cfc423d
Compare
SandyChapman
force-pushed
the
aalgo-307-task-entity/schapman
branch
from
July 2, 2026 14:22
0e5d9cd to
b10bb1d
Compare
SandyChapman
force-pushed
the
aalgo-307-task-entity/schapman
branch
from
July 2, 2026 19:15
9d7eb25 to
637dfb2
Compare
SandyChapman
marked this pull request as ready for review
July 2, 2026 19:18
Contributor
|
Contributor
📝 WalkthroughWalkthroughAdds persisted task CRUD for evaluator workspaces, plus derived metric storage and filtering. Updates the task and metric contracts, wires API/SDK surfaces, and adjusts agent-eval serialization to use structured task inputs and metadata. ChangesTask and derived-metric feature
Possibly related PRs
Suggested labels: 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.py (1)
93-104: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDuplicate metadata keys silently collapse.
{item.key: item.value for item in task.metadata}drops earlier entries whenMetadataItemkeys repeat. Sincemetadatais now a list (presumably to allow duplicates/ordering, unlike the old dict), silently losing entries on collapse could confuse callers who intentionally add multiple annotations with the same key.💡 Optional: warn or reject duplicate keys
- metadata={item.key: item.value for item in task.metadata}, + metadata=_dedupe_metadata(task.metadata),def _dedupe_metadata(items: list[MetadataItem]) -> dict[str, str]: result: dict[str, str] = {} for item in items: if item.key in result: logger.warning("Duplicate task metadata key %r; keeping last value.", item.key) result[item.key] = item.value return 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 `@plugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.py` around lines 93 - 104, The metadata reconstruction in _to_runtime_task currently collapses repeated MetadataItem keys by building a dict comprehension, which can silently drop earlier values. Update this conversion to handle duplicates explicitly: either reject duplicate keys with a clear error or dedupe them in a dedicated helper like _dedupe_metadata, and if deduping, log a warning when a key repeats so callers can detect the overwrite. Keep the change localized to _to_runtime_task and the metadata mapping logic used there.plugins/nemo-evaluator/openapi/openapi.yaml (1)
1708-1793: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftBreaking
/agent-evaluate/jobspayload contractinputsnow only acceptsTaskInputs.instruction, andmetadataisarray<MetadataItem>. Clients still sendinginputs.promptor dict metadata will 422; stored specs in the old shape need migration/backfill or a version bump.Source: Linters/SAST tools
🧹 Nitpick comments (2)
plugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.py (1)
111-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd async coverage for
create/list/delete.Only
retrieveis tested forAsyncEvaluatorTasksResource; sync counterpart covers create/retrieve/list/delete. Since sync/async implementations are independent code, an async-only regression here would go undetected.🤖 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 `@plugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.py` around lines 111 - 121, Add async test coverage for AsyncEvaluatorTasksResource beyond retrieve by creating focused tests for create, list, and delete, since the sync suite already covers these paths but the async implementation is independent. Use the existing async test pattern in test_async_retrieve_parses_dto as a guide, and verify the relevant AsyncEvaluatorTasksResource methods call the expected http_client operations and parse DTOs correctly, referencing create, list, delete, and retrieve for easy location.plugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.py (1)
70-102: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winInline metrics get stored even when task create fails.
_normalize_metricswrites derived metric entities beforecreate_taskattempts the entity write. OnEntityConflictError(or any other create failure), the derived metric is already persisted with nothing referencing it — no rollback, no cleanup path in this PR. Content-addressing limits damage on retries of identical content, but distinct inline metrics on a failed/duplicate create leak permanently.Consider checking for an existing task (or reordering) before normalizing metrics, so a conflict doesn't leave orphaned derived-metric writes.
🤖 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 `@plugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.py` around lines 70 - 102, The task creation flow persists derived metrics too early, so a failed create can leave orphaned inline metric entities behind. Update create_task to avoid calling _normalize_metrics before you know the task write will succeed, ideally by checking for an existing task or otherwise reordering the entity creation so EntityConflictError happens before store_derived_metric is invoked. Keep the fix scoped around create_task and _normalize_metrics, preserving the current MetricRef/MetricInline handling while preventing metric writes on failed task creation.
🤖 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/api/service/metric_service.py`:
- Around line 184-217: store_derived_metric is deduping on only
RuntimeMetricBundle.payload.digest, which can collapse distinct inline metrics
that share code but differ in secrets, outputs, labels, or metric_type. Update
the naming/dedup key in store_derived_metric to use a digest of the full
metric/bundle content (for example, the serialized RuntimeMetricBundle or
equivalent full bundle JSON) so that the existing get/create path only reuses
entries when the entire submitted contract matches.
---
Outside diff comments:
In `@plugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.py`:
- Around line 93-104: The metadata reconstruction in _to_runtime_task currently
collapses repeated MetadataItem keys by building a dict comprehension, which can
silently drop earlier values. Update this conversion to handle duplicates
explicitly: either reject duplicate keys with a clear error or dedupe them in a
dedicated helper like _dedupe_metadata, and if deduping, log a warning when a
key repeats so callers can detect the overwrite. Keep the change localized to
_to_runtime_task and the metadata mapping logic used there.
---
Nitpick comments:
In `@plugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.py`:
- Around line 70-102: The task creation flow persists derived metrics too early,
so a failed create can leave orphaned inline metric entities behind. Update
create_task to avoid calling _normalize_metrics before you know the task write
will succeed, ideally by checking for an existing task or otherwise reordering
the entity creation so EntityConflictError happens before store_derived_metric
is invoked. Keep the fix scoped around create_task and _normalize_metrics,
preserving the current MetricRef/MetricInline handling while preventing metric
writes on failed task creation.
In `@plugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.py`:
- Around line 111-121: Add async test coverage for AsyncEvaluatorTasksResource
beyond retrieve by creating focused tests for create, list, and delete, since
the sync suite already covers these paths but the async implementation is
independent. Use the existing async test pattern in
test_async_retrieve_parses_dto as a guide, and verify the relevant
AsyncEvaluatorTasksResource methods call the expected http_client operations and
parse DTOs correctly, referencing create, list, delete, and retrieve for easy
location.
🪄 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: 95570b2e-1aa8-47c8-80c0-910c765dbf7f
📒 Files selected for processing (25)
plugins/nemo-evaluator/openapi/openapi.yamlplugins/nemo-evaluator/src/nemo_evaluator/api/dependencies.pyplugins/nemo-evaluator/src/nemo_evaluator/api/schemas.pyplugins/nemo-evaluator/src/nemo_evaluator/api/service/metric_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/metrics.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.pyplugins/nemo-evaluator/src/nemo_evaluator/entities.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_spec.pyplugins/nemo-evaluator/src/nemo_evaluator/metric_refs.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/metric_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/service.pyplugins/nemo-evaluator/tests/api/service/test_metric_service.pyplugins/nemo-evaluator/tests/api/service/test_task_service.pyplugins/nemo-evaluator/tests/api/v2/test_metrics_routes.pyplugins/nemo-evaluator/tests/api/v2/test_tasks_routes.pyplugins/nemo-evaluator/tests/integration/test_agent_evaluate_job.pyplugins/nemo-evaluator/tests/integration/test_task_derived_metrics.pyplugins/nemo-evaluator/tests/sdk/test_metric_sdk_resources.pyplugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.pyplugins/nemo-evaluator/tests/test_agent_evaluate.pyplugins/nemo-evaluator/tests/test_task_entity.py
Add Task as a first-class entity / endpoint / SDK resource in the
nemo-evaluator plugin, mapping the SDK AgentEvalTask shape.
- TaskEntity + Task/TaskInput DTOs, /tasks CRUD routes (decorator authz),
and a client.evaluator.tasks SDK resource (sync + async).
- Inline task metrics are normalized to content-addressed *derived* stored
metrics (Files-backed, derived=True); the task holds refs only. Derived
metrics are hidden from the default /metrics listing (include_derived=true
to surface them); metric SDK list() gains include_derived parity.
- Typed task fields instead of opaque dicts: TaskInputs (single recognized
`instruction` field, extra="forbid") and metadata as a list of
MetadataItem {key, value} pairs. Applied to Task/TaskInput/TaskEntity and
the AgentEvalTaskSpec submit path; the runtime AgentEvalTask stays
dict-typed via a conversion at the boundary.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
aalgo-307-task-entity/schapman
branch
from
July 2, 2026 19:58
637dfb2 to
cf09270
Compare
Contributor
Author
|
Thanks @coderabbitai — both findings addressed in
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh api repos/NVIDIA-NeMo/nemo-platform/commits/cf092701a5 --jq '.files[] | select(.filename | test("metric_service.py|schemas.py|entities.py|agent_spec.py")) | .filename'Length of output: 629 Confirmed — commit |
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 `@plugins/nemo-evaluator/openapi/openapi.yaml`:
- Around line 1331-1338: The GET/DELETE task routes are missing the same path
validation used by create_task, so the name parameter is inconsistent across
CRUD operations. Update the path parameter definition for get_task and
delete_task to reuse the same MAX_NAME_LENGTH and NAME_PATTERN constraints
already applied in create_task, using the existing name schema in the OpenAPI
spec so all three routes enforce identical validation.
🪄 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: 64efb1a2-b7f2-4a0b-b265-e23d3d8ad450
📒 Files selected for processing (25)
plugins/nemo-evaluator/openapi/openapi.yamlplugins/nemo-evaluator/src/nemo_evaluator/api/dependencies.pyplugins/nemo-evaluator/src/nemo_evaluator/api/schemas.pyplugins/nemo-evaluator/src/nemo_evaluator/api/service/metric_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/metrics.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.pyplugins/nemo-evaluator/src/nemo_evaluator/entities.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_spec.pyplugins/nemo-evaluator/src/nemo_evaluator/metric_refs.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/metric_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/service.pyplugins/nemo-evaluator/tests/api/service/test_metric_service.pyplugins/nemo-evaluator/tests/api/service/test_task_service.pyplugins/nemo-evaluator/tests/api/v2/test_metrics_routes.pyplugins/nemo-evaluator/tests/api/v2/test_tasks_routes.pyplugins/nemo-evaluator/tests/integration/test_agent_evaluate_job.pyplugins/nemo-evaluator/tests/integration/test_task_derived_metrics.pyplugins/nemo-evaluator/tests/sdk/test_metric_sdk_resources.pyplugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.pyplugins/nemo-evaluator/tests/test_agent_evaluate.pyplugins/nemo-evaluator/tests/test_task_entity.py
🚧 Files skipped from review as they are similar to previous changes (22)
- plugins/nemo-evaluator/tests/api/v2/test_metrics_routes.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.py
- plugins/nemo-evaluator/src/nemo_evaluator/service.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/v2/metrics.py
- plugins/nemo-evaluator/tests/test_agent_evaluate.py
- plugins/nemo-evaluator/tests/integration/test_agent_evaluate_job.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_spec.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/dependencies.py
- plugins/nemo-evaluator/tests/sdk/test_metric_sdk_resources.py
- plugins/nemo-evaluator/tests/api/service/test_metric_service.py
- plugins/nemo-evaluator/tests/api/v2/test_tasks_routes.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/metric_resources.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.py
- plugins/nemo-evaluator/src/nemo_evaluator/entities.py
- plugins/nemo-evaluator/src/nemo_evaluator/metric_refs.py
- plugins/nemo-evaluator/tests/api/service/test_task_service.py
- plugins/nemo-evaluator/tests/integration/test_task_derived_metrics.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/schemas.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.py
- plugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.py
arpitsardhana
approved these changes
Jul 2, 2026
arpitsardhana
pushed a commit
that referenced
this pull request
Jul 9, 2026
Add Task as a first-class entity / endpoint / SDK resource in the
nemo-evaluator plugin, mapping the SDK AgentEvalTask shape.
- TaskEntity + Task/TaskInput DTOs, /tasks CRUD routes (decorator authz),
and a client.evaluator.tasks SDK resource (sync + async).
- Inline task metrics are normalized to content-addressed *derived* stored
metrics (Files-backed, derived=True); the task holds refs only. Derived
metrics are hidden from the default /metrics listing (include_derived=true
to surface them); metric SDK list() gains include_derived parity.
- Typed task fields instead of opaque dicts: TaskInputs (single recognized
`instruction` field, extra="forbid") and metadata as a list of
MetadataItem {key, value} pairs. Applied to Task/TaskInput/TaskEntity and
the AgentEvalTaskSpec submit path; the runtime AgentEvalTask stays
dict-typed via a conversion at the boundary.
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.
Summary
Adds Task as a first-class entity / endpoint / SDK resource in the
nemo-evaluatorplugin, mapping the SDKAgentEvalTaskshape so agent-eval tasks can be persisted and queried (AALGO-307).What's here
TaskEntity+Task/TaskInputDTOs./tasksCRUD routes (list/create/get/delete) with authz + filter/sort, mirroring/metrics.client.evaluator.tasksSDK resource (sync + async).derived=True), so a persisted task holds metric references only. Identical inline metrics across tasks dedupe to one stored bundle./metricslisting; passinclude_derived=trueto surface them. Metric SDKlist()gainsinclude_derivedparity with the route.Testing
TaskService(incl. inline→derived normalization),/tasksroutes, SDK resources, and derived-metric storage (digest naming, content-addressed dedup, list exclusion). Full evaluator unit suite green (428).RUN_AGENT_EVAL_INTEGRATION=1, real entity store + Files): inline task metric → derived ref, cross-task dedup, derived metric retrievable/derived=True/Files-backed, hidden by default and visible withinclude_derived. Verified live — this caught a real bug: the content-addressed namederived.<sha256>(72 chars) exceeded the entity store's 63-char name limit; the digest is now truncated to fit.Not in this PR
task_refson the agent-eval submit path (running an eval over stored tasks by reference) — intended as a follow-up stacked on this branch.Tasksetentity — deferred within AALGO-307.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
include_derived.Bug Fixes