feat(evaluator): make a stored task runner-polymorphic via kind - #1071
Conversation
49d82b5 to
59c4f39
Compare
58fccbe to
a584f54
Compare
59c4f39 to
6d780b1
Compare
|
6d780b1 to
4ccd57c
Compare
|
/nvskills-ci |
|
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:
📝 WalkthroughWalkthroughTasks now use nested, discriminated evaluator or Harbor specifications. Persistence, revision hashing, taskset expansion, tests, examples, and documentation use the new model and pinned grader references. ChangesUnified task definitions and task execution
Sequence Diagram(s)sequenceDiagram
participant Client
participant TaskService
participant RevisionStore
participant TasksetResolver
Client->>TaskService: Submit TaskInput.spec
TaskService->>RevisionStore: Store normalized task specification
RevisionStore-->>TaskService: Return task revision
TasksetResolver->>RevisionStore: Load pinned revision
RevisionStore-->>TasksetResolver: Return evaluator spec and reference
TasksetResolver-->>Client: Return expanded task input
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 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/nemo-evaluator-plugin/references/resources.md (1)
64-99: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd supported CLI variants in tab sets.
The evaluator CLI does not manage stored metrics, tasks, or tasksets. Add
curlREST examples for the CRUD calls in both affected workflows instead of undocumentednemo evaluatorcommands.
skills/nemo-evaluator-plugin/references/resources.md#L64-L99docs/evaluator/manage-tasks-tasksets.mdx#L76-L89🤖 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 `@skills/nemo-evaluator-plugin/references/resources.md` around lines 64 - 99, Add supported curl REST examples for creating the metric and stored task in skills/nemo-evaluator-plugin/references/resources.md lines 64-99, replacing any undocumented nemo evaluator CLI usage while preserving the reference-backed workflow. Apply the same CLI-to-curl REST replacement for the task and taskset CRUD workflow in docs/evaluator/manage-tasks-tasksets.mdx lines 76-89, covering each documented CRUD call.Source: Coding guidelines
🧹 Nitpick comments (3)
plugins/nemo-evaluator/tests/test_content_hash.py (1)
231-257: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd the Harbor head/revision digest-agreement test.
Both tests here exercise
head_digestonly.entities.pystates thatREVISION_POINTER_EXCLUDEandREVISION_SELF_EXCLUDEmust exclude the same derived fields, otherwise publish-time dedup never fires. That mirror is asserted for the evaluator kind intest_revision_entity.py::test_task_head_and_revision_digests_agree, but not for Harbor — the only kind the exclusion actually applies to.💚 Proposed test
def test_harbor_head_and_revision_digests_agree() -> None: """The exclusion is declared twice, once per constant. If they drift, a Harbor task republishes an identical archive as a new revision on every PUT.""" head = _harbor_task(config={"verifier": {"type": "pytest"}}) revision = TaskRevisionEntity( spec=head.spec, name="rev.1", workspace="default", content_hash="a" * 64, revision=1, ) assert head_digest(head) == content_hash(revision, exclude=REVISION_SELF_EXCLUDE)🤖 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/test_content_hash.py` around lines 231 - 257, Add a Harbor-specific test near test_harbor_config_does_not_change_digest that constructs a configured task via _harbor_task, creates the matching TaskRevisionEntity, and asserts head_digest(head) equals content_hash(revision, exclude=REVISION_SELF_EXCLUDE). Import or reuse the existing TaskRevisionEntity and REVISION_SELF_EXCLUDE symbols, preserving the test’s focus on agreement between the two exclusion definitions.plugins/nemo-evaluator/src/nemo_evaluator/entities.py (1)
53-84: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake the digest exclusion kind-aware, or assert it.
_DERIVED_SPEC_FIELDSexcludes anyspecfield namedconfig, for every variant in theTaskDefinitionunion. The safety argument documented above holds only forHarborTaskDefinition, wherearchive_digestcovers the same content.EvaluatorTaskDefinitionhas noconfigtoday, so there is no current defect.If a future variant adds a
configthat is a real execution or grading input, it is silently dropped from the revision digest. Two tasks that grade differently would then share a digest, and a pinned taskset would re-grade without cutting a revision. Name-based exclusion cannot detect that.Bind the exclusion to the kind that owns it, or fail loudly when another variant declares the name.
♻️ Kind-scoped exclusion
-_DERIVED_SPEC_FIELDS = {"config"} +#: Keyed by the variant that owns the field, so the safety argument travels with the kind. +_DERIVED_SPEC_FIELDS_BY_KIND: dict[type, set[str]] = {HarborTaskDefinition: {"config"}} +_DERIVED_SPEC_FIELDS = { + field for fields in _DERIVED_SPEC_FIELDS_BY_KIND.values() for field in fields +} +# A variant that is not the owner must not declare a name on the exclusion list, or its field +# would be dropped from the digest by accident. +for _variant in (EvaluatorTaskDefinition, HarborTaskDefinition): + _unowned = (set(_variant.model_fields) & _DERIVED_SPEC_FIELDS) - _DERIVED_SPEC_FIELDS_BY_KIND.get(_variant, set()) + if _unowned: + raise RuntimeError( + f"{_variant.__name__} declares digest-excluded field(s) {sorted(_unowned)} it does not own; " + "digest every field that affects execution or grading." + )🤖 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/entities.py` around lines 53 - 84, Make the config exclusion in REVISION_POINTER_EXCLUDE and REVISION_SELF_EXCLUDE kind-aware rather than applying _DERIVED_SPEC_FIELDS to every TaskDefinition variant. Scope config removal to HarborTaskDefinition, or add validation that fails loudly if another kind declares config without an explicit digest policy; preserve config exclusion for Harbor while preventing silently omitted execution or grading inputs.plugins/nemo-evaluator/src/nemo_evaluator/content_hash.py (1)
50-96: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUpdate the
excludeargument documentationPydantic accepts nested set syntax and ignores unknown exclusion keys. The shared
REVISION_POINTER_EXCLUDEis therefore safe forTasksetEntity, which has nospecfield. Update theArgsentry to describe both flat sets and nested mappings.📝 Docstring fix
- exclude: Extra field names to drop on top of ``__base_fields__``. Revisioned entities pass - their own revision/tag bookkeeping here — a revision's digest must not cover the - revision index that was assigned *because of* that digest. + exclude: Extra fields to drop on top of ``__base_fields__``, as either a flat set of field + names or pydantic's nested mapping form (``{"spec": {"config"}}``) for dropping a + field inside a sub-model. Revisioned entities pass their own revision/tag bookkeeping + here — a revision's digest must not cover the revision index that was assigned + *because of* that digest.🤖 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/content_hash.py` around lines 50 - 96, Update the Args documentation for canonical_payload’s exclude parameter to describe support for both flat field-name sets and nested mappings, including that unknown exclusion keys are ignored by Pydantic. Keep the existing behavior and implementation of _as_exclude_map unchanged.
🤖 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 `@docs/evaluator/manage-tasks-tasksets.mdx`:
- Around line 91-112: Update docs/evaluator/manage-tasks-tasksets.mdx lines
91-112 to use the public schema name TaskDefinition instead of TaskSpecInput,
and change the adjacent stored.metrics reference to stored.spec.metrics. At
docs/evaluator/manage-tasks-tasksets.mdx line 139, replace agent_eval with
evaluator.
In `@plugins/nemo-evaluator/src/nemo_evaluator/task_refs.py`:
- Around line 61-64: Update the UnsupportedTaskKindError message in task_refs.py
lines 61-64 to state that stored Harbor task execution is not yet available,
removing guidance to select a compatible target. Update
docs/evaluator/manage-tasks-tasksets.mdx lines 123-126 to state that taskset
expansion rejects Harbor members until stored Harbor execution is implemented.
In `@plugins/nemo-evaluator/tests/api/service/test_task_service.py`:
- Around line 74-75: The Task.spec union must be narrowed before
variant-specific fields are accessed. In
plugins/nemo-evaluator/tests/api/service/test_task_service.py at lines 74-75,
99-101, 123-125, 145-146, 253, 390-393, and 423, and in
plugins/nemo-evaluator/tests/integration/test_task_derived_metrics.py at lines
77-84, plugins/nemo-evaluator/tests/integration/test_task_revisions.py at lines
81-83, 175, 225-226, and 324-325, plus
plugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.py at line 89, add an
appropriate EvaluatorTaskDefinition or HarborTaskDefinition isinstance assertion
before accessing intent, metrics, reference, or config.
In `@skills/nemo-evaluator-plugin/assets/examples/plugin_sdk_examples.py`:
- Around line 76-80: Update
skills/nemo-evaluator-plugin/assets/examples/plugin_sdk_examples.py lines 76-80
to use the bare resource references MetricRef("answer-exact") and
TaskRef("capital-france") so they resolve in the caller workspace. Update
skills/nemo-evaluator-plugin/references/resources.md lines 44-60 to use the same
bare metric and task references for resources created by the example.
---
Outside diff comments:
In `@skills/nemo-evaluator-plugin/references/resources.md`:
- Around line 64-99: Add supported curl REST examples for creating the metric
and stored task in skills/nemo-evaluator-plugin/references/resources.md lines
64-99, replacing any undocumented nemo evaluator CLI usage while preserving the
reference-backed workflow. Apply the same CLI-to-curl REST replacement for the
task and taskset CRUD workflow in docs/evaluator/manage-tasks-tasksets.mdx lines
76-89, covering each documented CRUD call.
---
Nitpick comments:
In `@plugins/nemo-evaluator/src/nemo_evaluator/content_hash.py`:
- Around line 50-96: Update the Args documentation for canonical_payload’s
exclude parameter to describe support for both flat field-name sets and nested
mappings, including that unknown exclusion keys are ignored by Pydantic. Keep
the existing behavior and implementation of _as_exclude_map unchanged.
In `@plugins/nemo-evaluator/src/nemo_evaluator/entities.py`:
- Around line 53-84: Make the config exclusion in REVISION_POINTER_EXCLUDE and
REVISION_SELF_EXCLUDE kind-aware rather than applying _DERIVED_SPEC_FIELDS to
every TaskDefinition variant. Scope config removal to HarborTaskDefinition, or
add validation that fails loudly if another kind declares config without an
explicit digest policy; preserve config exclusion for Harbor while preventing
silently omitted execution or grading inputs.
In `@plugins/nemo-evaluator/tests/test_content_hash.py`:
- Around line 231-257: Add a Harbor-specific test near
test_harbor_config_does_not_change_digest that constructs a configured task via
_harbor_task, creates the matching TaskRevisionEntity, and asserts
head_digest(head) equals content_hash(revision, exclude=REVISION_SELF_EXCLUDE).
Import or reuse the existing TaskRevisionEntity and REVISION_SELF_EXCLUDE
symbols, preserving the test’s focus on agreement between the two exclusion
definitions.
🪄 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: ede6d620-8093-4dbc-a705-f7bd33e5b261
📒 Files selected for processing (25)
docs/evaluator/manage-tasks-tasksets.mdxplugins/nemo-evaluator/openapi/openapi.yamlplugins/nemo-evaluator/src/nemo_evaluator/api/fields.pyplugins/nemo-evaluator/src/nemo_evaluator/api/schemas.pyplugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/task_definitions/evaluator.pyplugins/nemo-evaluator/src/nemo_evaluator/api/task_definitions/harbor.pyplugins/nemo-evaluator/src/nemo_evaluator/content_hash.pyplugins/nemo-evaluator/src/nemo_evaluator/entities.pyplugins/nemo-evaluator/src/nemo_evaluator/revisions.pyplugins/nemo-evaluator/src/nemo_evaluator/task_refs.pyplugins/nemo-evaluator/tests/api/service/test_task_service.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/integration/test_task_revisions.pyplugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.pyplugins/nemo-evaluator/tests/test_content_hash.pyplugins/nemo-evaluator/tests/test_revision_entity.pyplugins/nemo-evaluator/tests/test_revisions.pyplugins/nemo-evaluator/tests/test_skill_examples.pyplugins/nemo-evaluator/tests/test_task_entity.pyplugins/nemo-evaluator/tests/test_task_refs.pyskills/nemo-evaluator-plugin/assets/examples/plugin_sdk_examples.pyskills/nemo-evaluator-plugin/references/resources.md
4ccd57c to
ca82875
Compare
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/tests/api/service/test_task_service.py`:
- Around line 376-382: Update _FakeMetricService to record get_metric
invocations, then extend test_a_harbor_task_never_reaches_the_metric_service to
assert that the recorded get_metric call list is empty alongside
metric_service.stored. Preserve the existing assertion covering
store_derived_metric.
🪄 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: ca32a535-4ee6-493d-b7c8-c6b2353f1437
📒 Files selected for processing (8)
docs/evaluator/manage-tasks-tasksets.mdxplugins/nemo-evaluator/src/nemo_evaluator/task_refs.pyplugins/nemo-evaluator/tests/api/service/test_task_service.pyplugins/nemo-evaluator/tests/integration/test_task_derived_metrics.pyplugins/nemo-evaluator/tests/integration/test_task_revisions.pyplugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.pyskills/nemo-evaluator-plugin/assets/examples/plugin_sdk_examples.pyskills/nemo-evaluator-plugin/references/resources.md
🚧 Files skipped from review as they are similar to previous changes (5)
- plugins/nemo-evaluator/tests/integration/test_task_derived_metrics.py
- plugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.py
- plugins/nemo-evaluator/src/nemo_evaluator/task_refs.py
- skills/nemo-evaluator-plugin/references/resources.md
- plugins/nemo-evaluator/tests/integration/test_task_revisions.py
ca82875 to
2345ddb
Compare
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/tests/api/service/test_task_service.py`:
- Around line 425-430: Update the test around replace_task to fetch “fix-test”
again after the no-revision replacement, then assert the Harbor config on that
freshly retrieved task rather than on same; retain the existing assertions that
no revision was published and the revision remains 1.
🪄 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: edb05a94-f83a-4de8-8711-00ff1f596280
📒 Files selected for processing (1)
plugins/nemo-evaluator/tests/api/service/test_task_service.py
2345ddb to
3bed91e
Compare
|
/nvskills-ci |
A task is an evaluation unit; how it runs is a property of the task, not a
different kind of record. The target side already models this — `AgentRunnerTarget`
is a `kind`-discriminated union of codex/fabric/harbor — so the stored side now
matches, and a user manages every evaluation unit in one place regardless of
which runner executes it.
Task content moves under a discriminated `spec`:
- `EvaluatorTaskDefinition` (kind="evaluator") — intent, inputs, reference,
metrics, views
- `HarborTaskDefinition` (kind="harbor") — a reference to the task's packaged
directory in the Files service, plus Harbor's own config
Nested rather than flattened with nullable per-kind fields, so each variant's
required fields stay required and the revision digest covers the spec as a unit;
two kinds with coincidentally similar metadata cannot collide on content.
`kind` is a `Literal`, matching how the runner targets discriminate. The two
definitions live in their own modules under `api/task_definitions/`; the shared
field types they need moved to `api/fields.py`, since the definitions are
imported *by* `schemas` and cannot import back from it.
A single model per kind, rather than a stored/input pair: only `metrics` widens
on the way in, and the service narrows it to references when storing. That keeps
the API surface small at the cost of making the narrowing a service invariant
rather than a type-level one.
`EvaluatorTaskDefinition` gains the grader-only `reference` — held-out ground
truth, surfaced to metrics but never seeded into the agent's workspace. It has
existed on the inline `AgentEvalTaskInput` since #566, where persisting it was
deferred because the stored schemas then lived in the root OpenAPI/SDK; they are
plugin-owned now, so that reason has lapsed. Until this, a taskset-driven run
expanded to an empty reference, so any task needing ground truth the agent cannot
edit had to give up stored tasks and tasksets entirely.
It is covered by the revision digest. The rule: the digest covers anything that
affects a task's execution output or how it is graded, and `reference` decides
what a metric grades against — two revisions that score differently must not
share a digest, or publish-time dedup would collapse them and a pin would stop
fixing the grading. Held out from the *agent*, not from the API: anyone who can
read the task can read it.
Harbor's `config` is the one exclusion, and it does not contradict that rule. It
is a projection of `task.toml`; Harbor reads the real file out of the
materialized archive at run time, and `archive_digest` is authoritative over
every file in that directory. A config change that genuinely alters execution or
grading therefore already moves the digest, while hashing the projection would
make revision history sensitive to Harbor's serialization. That makes
`archive_digest` load-bearing: a Harbor field ever read from the stored record
rather than from the archive would have to be digested.
Harbor specifics:
- One fileset per task, so a task shared by several tasksets is stored once.
- `archive_ref` is shape-validated, so a malformed reference is rejected at
publish rather than surfacing as a download failure mid-run.
- `config` is stored but excluded from the revision digest, as above.
- Which agent runs a task is not stored: that comes from the run's target, so
the same stored task can be evaluated against different agents.
Taskset expansion rejects a `harbor` member rather than projecting it onto an
agent-eval DTO: that content is a directory of files, not fields, so a pure
projection would silently produce a task with no intent and no metrics — an
evaluation that runs and scores nothing. Mixed tasksets stay storable; the
mismatch surfaces at submit as a 422.
The rejection is unconditional, not target-dependent. Storage landed ahead of
the execution bridge, so no target can run a stored `harbor` task yet and the
message says so plainly instead of suggesting the reader find a compatible one.
Bridging the two — and encoding runner/task-kind compatibility declaratively
rather than as an isinstance check here — is AALGO-481.
Note for anyone with existing task rows: this is a breaking schema change with
no migration. Rows stored in the previous flat shape fail validation on read,
which surfaces as a 500 when listing tasks. Clear them before upgrading.
Signed-off-by: Sandy Chapman <schapman@nvidia.com>
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 3466-3470: Make the discriminator property kind required in both
TaskDefinition DTO variants, remove any schema behavior that permits it to be
omitted, and regenerate the OpenAPI schema. Update the documentation example to
include kind set to evaluator, and add raw JSON POST and PUT coverage verifying
bodies without kind are rejected while valid bodies include it.
🪄 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: 5da970de-6c33-4b03-b88d-8a752d845b53
📒 Files selected for processing (25)
docs/evaluator/manage-tasks-tasksets.mdxplugins/nemo-evaluator/openapi/openapi.yamlplugins/nemo-evaluator/src/nemo_evaluator/api/fields.pyplugins/nemo-evaluator/src/nemo_evaluator/api/schemas.pyplugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/task_definitions/evaluator.pyplugins/nemo-evaluator/src/nemo_evaluator/api/task_definitions/harbor.pyplugins/nemo-evaluator/src/nemo_evaluator/content_hash.pyplugins/nemo-evaluator/src/nemo_evaluator/entities.pyplugins/nemo-evaluator/src/nemo_evaluator/revisions.pyplugins/nemo-evaluator/src/nemo_evaluator/task_refs.pyplugins/nemo-evaluator/tests/api/service/test_task_service.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/integration/test_task_revisions.pyplugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.pyplugins/nemo-evaluator/tests/test_content_hash.pyplugins/nemo-evaluator/tests/test_revision_entity.pyplugins/nemo-evaluator/tests/test_revisions.pyplugins/nemo-evaluator/tests/test_skill_examples.pyplugins/nemo-evaluator/tests/test_task_entity.pyplugins/nemo-evaluator/tests/test_task_refs.pyskills/nemo-evaluator-plugin/assets/examples/plugin_sdk_examples.pyskills/nemo-evaluator-plugin/references/resources.md
🚧 Files skipped from review as they are similar to previous changes (21)
- 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/revisions.py
- plugins/nemo-evaluator/tests/test_revision_entity.py
- plugins/nemo-evaluator/tests/test_skill_examples.py
- skills/nemo-evaluator-plugin/assets/examples/plugin_sdk_examples.py
- plugins/nemo-evaluator/tests/test_task_entity.py
- plugins/nemo-evaluator/tests/test_content_hash.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/task_definitions/harbor.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/task_definitions/evaluator.py
- plugins/nemo-evaluator/tests/integration/test_agent_evaluate_job.py
- plugins/nemo-evaluator/src/nemo_evaluator/task_refs.py
- plugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.py
- plugins/nemo-evaluator/tests/test_task_refs.py
- skills/nemo-evaluator-plugin/references/resources.md
- plugins/nemo-evaluator/tests/test_revisions.py
- plugins/nemo-evaluator/src/nemo_evaluator/entities.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/schemas.py
- plugins/nemo-evaluator/tests/integration/test_task_revisions.py
- plugins/nemo-evaluator/tests/api/service/test_task_service.py
- plugins/nemo-evaluator/tests/api/v2/test_tasks_routes.py
`TaskDefinition` is discriminated on `kind`, but both variants defaulted it, so the generated schema left `kind` out of `required` while the validator demanded it. A raw create or replace body without `kind` fails with `union_tag_not_found` — meaning a client generated from that spec would omit the field and 422 on every write. Make `kind` a required field on both definitions, matching how the metric payload DTOs in the same package already declare their discriminator, and regenerate the plugin spec. Tests cover both halves of the mismatch: raw POST and PUT bodies without `kind` are rejected, and the published schema keeps `kind` in `required`. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
|
/nvskills-ci |
ngoncharenko
left a comment
There was a problem hiding this comment.
Manual review against origin/main: three actionable issues.
…he task docs Review follow-ups on #1071. Reference parsing was duplicated. `nmp.common.entities.utils` already re-exports `nemo_platform_plugin.refs.parse_entity_ref`, which ~10 services and three other plugins use; the evaluator was the last place carrying its own copy under the same name. Delete it and delegate: `parse_subentity_ref` now adds only the `#fragment` that a revisioned entity needs on top of the shared split, and `ENTITY_REF_PATTERN` / `FILESET_REF_PATTERN` move next to the parser and the `FilesetRef` type they describe. `_SUBENTITY_REF_PATTERN` is spliced from the shared constant, so widening what counts as a `workspace/name` widens both shapes at once instead of leaving one behind. One behavior detail this makes explicit: taskset duplicate-detection relied on the old parser silently stripping `#fragment`, so `task-a` and `task-a#<digest>` deduped as one member. The platform parser does not strip, so that path now discards the fragment deliberately. Restore the `CloudpickleMetricPayload` / `InlineMetricPayload` / `MetricPayload` re-exports from `api.schemas`, which `fields.py` promises in its module docstring and lost when they moved. The `manage-tasks-tasksets` revision snippets still passed the pre-`spec` flat shape. `make docs-check-python-snippets` did not catch it because the snippet linter passes ty a rule name that was renamed upstream, so ty answered with `warning[unknown-rule]` and the check failed for every doc regardless of its content. Fix the rule name, fix the snippets, and name both task kinds before the sentence that refers to "both kinds". Type-checking a snippet would not have caught one that type-checks and then fails at run time, nor a documented output gone stale — which is the shape of what review found here. So add an integration test that walks the doc top to bottom against a real platform and asserts the results it claims. No OpenAPI change: the spec regenerates byte-identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
|
/nvskills-ci |
1 similar comment
|
/nvskills-ci |
The evaluator skill's `resources.md` and `plugin_sdk_examples.py` were updated here for the new `spec` shape, which put a `skills/` file in the diff and so put the 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 runners, so no evaluation runs and the gate blocks on empty coverage. It is an infrastructure problem with the nvcarps pipeline, already reported, and nothing in this repo can resolve it. With no `skills/` file touched, the gate no longer applies to this PR and the storage change can land on its own merits. The skill updates are not lost — they move to a stacked follow-up PR, which can sit behind the gate for as long as it takes without holding this one. Reverting them costs nothing in tests: no test invokes `store_resources`, and the one assertion in `test_skill_examples.py` that pinned the new wording is reverted alongside the content it describes. Known cost while the two are apart: the skill documents the pre-`spec` task shape, which no longer validates. Anyone following the skill in that window writes a task the API rejects. That is the price of unblocking, and it ends when the follow-up lands. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
Moving a stored task's content under a discriminated `spec` invalidates the skill example's `TaskInput(intent=..., inputs=..., metrics=...)`, and ty checks `skills/**/*.py`, so the example fails the type gate. The fix for the example is written and sits in #1237. It cannot ride along here: editing any file under `skills/` puts the PR behind the NVSkills gate, and that gate currently cannot pass — tier 3 runs 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. Keeping the example correct and keeping this PR out of the gate are mutually exclusive until that is fixed. Chosen as an override rather than a `[tool.ty.src].exclude` entry, which that list's own header asks contributors not to grow: an override keeps every other rule live on the file and names the two the stale call actually produces, so it cannot quietly widen into cover for unrelated drift. #1237 removes it in the same commit that corrects the example. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
Split out of #1071 so that PR is not held behind the NVSkills gate. The gate cannot currently pass for this repo: tier 3 is invoked with `--env-mode local`, whose bubblewrap sandbox fails its smoke test on the nvcarps runners, so nothing is evaluated and the gate blocks on empty coverage. This PR carries the whole cost of that, and can wait for the infrastructure fix without blocking the storage change. `store_resources` moves to the discriminated `spec`, and `resources.md` teaches held-out ground truth on a *stored* task rather than an inline one — the skill steered users to `AgentEvalTaskInput` only because the stored schema had no `reference` field, and #1071 gives it one. Routing them back to inline would cost them tasksets and revision pinning for no reason. Until this lands, the published skill documents the pre-`spec` shape, which no longer validates against #1071. Merge promptly once the gate is healthy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
The override added alongside #1071 exists only because the skill example was left stale there. This PR corrects the example, so the exemption expires with it — ty passes on this branch with no override at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 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>
…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>
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>
Why
A task is an evaluation unit; how it runs is a property of the task, not a different kind of record. The target side already models this —
AgentRunnerTargetis akind-discriminated union ofcodex | fabric | harbor, with a comment saying "widen with more members as runners land."This makes the stored side match, so a user manages every evaluation unit in one place regardless of which runner executes it. It's the foundation for publishing Harbor tasks into NeMo.
What
Task content moves under a discriminated
spec:evaluatorintent,inputs,reference,metrics,viewsharborarchive_ref(fileset),archive_digest,instruction, opaque HarborconfigNested rather than flattened with nullable per-kind fields. Flat would make every field optional with nothing enforcing a coherent set; nesting keeps each variant's required fields required, and the revision digest then covers the spec as a unit — so two kinds with coincidentally similar metadata can't collide on content. There's a test for that.
Named
…Definition, not…Spec.AgentEvalTaskSpecalready names the runtime task DTO injobs.agent_spec; two models sharing a class name across modules break OpenAPI generation with a schema-name collision. (Found by regenerating the spec — see below.)One fileset per Harbor task, so a task shared by several tasksets is stored once.
archive_refis shape-validated, so a malformed reference is rejected at publish rather than surfacing as a download failure mid-run — matching how a metric reference is checked when a task is stored.Grader-only
referenceon stored tasksEvaluatorTaskDefinitiongainsreference— held-out ground truth, surfaced to metrics but never seeded into the agent's workspace or shown to the agent. It has existed on the inlineAgentEvalTaskInputsince #566, where persisting it was deferred because the stored schemas then lived in the root OpenAPI/SDK; they're plugin-owned now, so that reason has lapsed.Until this, a taskset-driven run expanded to an empty reference, so any task needing ground truth the agent cannot edit had to give up stored tasks and tasksets entirely — which is the reward-hacking hole #566 was opened to close.
Held out from the agent, not from the API: anyone who can read the task can read it.
What the revision digest covers
The rule: the digest covers anything that affects a task's execution output or how it is graded. A field may be excluded only if it's a derived view of content the digest already covers by another route.
referenceis therefore in — it decides what a metric grades against, so two revisions that score differently must not share a digest, or publish-time dedup would collapse them and a pin would stop fixing the grading.Harbor's
configis the one exclusion, and it doesn't contradict that rule: it's a projection oftask.toml, Harbor reads the real file out of the materialized archive at run time, andarchive_digestis authoritative over every file in that directory. So a config change that genuinely alters execution or grading already moves the digest, while hashing the projection would make revision history sensitive to Harbor's serialization — a release that reordered keys would cut a revision for byte-identical files.That makes
archive_digestload-bearing, and there's a test guarding it: a Harbor field ever read from the stored record rather than from the archive would have to be digested.Scope: storage, not execution
This PR makes the storage model runner-polymorphic. It touches no execution-path source file.
evaluatorkindharborkindresolve_taskset_refpreviously projected every member onto an agent-eval DTO. A Harbor task's content is a directory of files, not fields, so that projection would silently produce a task with no intent and no metrics — an evaluation that runs and scores nothing. It now raisesUnsupportedTaskKindError, surfaced as a 422 before the run starts.Harbor execution itself already works on
main(HarborRunnerTarget, driven byharbor_dataset_pathmetadata) and is unaffected by this PR — it just isn't fed by stored task entities yet.Follow-up: AALGO-481 — bridge stored harbor tasks to the harbor runner, and encode runner ↔ task-kind compatibility declaratively instead of via an
isinstancecheck in the expansion path. That ticket also covers fixing the current rejection message, which unhelpfully suggests submitting against "a target whose runner executes 'harbor' tasks" when no such target exists yet.Breaking change
No migration. Rows stored in the previous flat shape fail validation on read, which surfaces as a 500 when listing tasks. Clear them before upgrading.
Verification
tools/lint/lint-python-types.sh— 0 errorsruff check/ruff format --check— cleanDocs updated: examples, the
TaskInputfield table, the task-kinds section, and the evaluator skill reference.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation