From a584f5401c4dcb775b2f559394c2bce4f45e301f Mon Sep 17 00:00:00 2001 From: Sandy Chapman Date: Fri, 31 Jul 2026 17:20:03 -0300 Subject: [PATCH 1/2] feat(evaluator): add content-addressed revisions for tasks and tasksets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stored tasks and tasksets are now versioned. Creating one publishes revision 1; replacing its content publishes the next. Earlier revisions stay readable, so an evaluation can be re-run against exactly the content it ran against before. A revision is an immutable child entity named `rev.`, addressed by a SHA-256 digest of its content. The head record keeps the current content plus mutable pointers (`latest_revision` and a tag -> ordinal map); digest lookups are a real `(parent, data.content_hash)` query rather than a denormalized index. References gain the platform's standard `#` sub-entity fragment — `workspace/name#` — with an absent fragment meaning `latest`. Taskset membership is resolved to exact digests on write, and taskset expansion at eval time now loads each member's *pinned* revision rather than its current content. Without that, membership was pinned in storage and ignored at runtime, so a suite looked reproducible and was not. API: POST /{tasks,tasksets}/{name} create + publish rev 1 PUT /{tasks,tasksets}/{name} replace + publish (201/200) GET /{tasks,tasksets}/{name}/revisions list, newest first GET /{tasks,tasksets}/{name}/revisions/{rev} read a pinned revision PUT /{tasks,tasksets}/{name}/tags/{tag} tag an existing revision Publishing is idempotent: identical content cuts no revision but still applies tags. Ordinal allocation is serialized by the child create and retries on contention; `latest` only moves forward. Creating a record rolls back if its first publish fails, so no head ever exists without a revision. Signed-off-by: Sandy Chapman --- docs/evaluator/manage-tasks-tasksets.mdx | 149 +++- plugins/nemo-evaluator/openapi/openapi.yaml | 596 +++++++++++++- .../src/nemo_evaluator/api/schemas.py | 144 +++- .../api/service/task_service.py | 236 +++++- .../api/service/taskset_service.py | 279 ++++++- .../src/nemo_evaluator/api/v2/tasks.py | 182 ++++- .../src/nemo_evaluator/api/v2/tasksets.py | 168 +++- .../src/nemo_evaluator/content_hash.py | 91 +++ .../src/nemo_evaluator/entities.py | 151 +++- .../src/nemo_evaluator/revisions.py | 493 ++++++++++++ .../src/nemo_evaluator/sdk/task_resources.py | 104 ++- .../nemo_evaluator/sdk/taskset_resources.py | 106 ++- .../src/nemo_evaluator/task_refs.py | 67 +- .../tests/api/service/test_task_service.py | 213 +++-- .../tests/api/service/test_taskset_service.py | 242 ++++-- .../tests/api/v2/test_tasks_routes.py | 216 +++-- .../tests/api/v2/test_tasksets_routes.py | 183 +++-- plugins/nemo-evaluator/tests/conftest.py | 163 ++++ .../tests/integration/test_task_revisions.py | 249 ++++++ .../tests/sdk/test_task_sdk_resources.py | 138 +++- .../tests/sdk/test_taskset_sdk_resources.py | 105 ++- .../nemo-evaluator/tests/test_content_hash.py | 225 ++++++ .../tests/test_revision_entity.py | 205 +++++ .../nemo-evaluator/tests/test_revisions.py | 738 ++++++++++++++++++ .../tests/test_subentity_refs.py | 99 +++ .../nemo-evaluator/tests/test_task_refs.py | 106 ++- .../repository/test_child_entity_filtering.py | 117 +++ 27 files changed, 5313 insertions(+), 452 deletions(-) create mode 100644 plugins/nemo-evaluator/src/nemo_evaluator/content_hash.py create mode 100644 plugins/nemo-evaluator/src/nemo_evaluator/revisions.py create mode 100644 plugins/nemo-evaluator/tests/conftest.py create mode 100644 plugins/nemo-evaluator/tests/integration/test_task_revisions.py create mode 100644 plugins/nemo-evaluator/tests/test_content_hash.py create mode 100644 plugins/nemo-evaluator/tests/test_revision_entity.py create mode 100644 plugins/nemo-evaluator/tests/test_revisions.py create mode 100644 plugins/nemo-evaluator/tests/test_subentity_refs.py create mode 100644 services/core/entities/tests/repository/test_child_entity_filtering.py diff --git a/docs/evaluator/manage-tasks-tasksets.mdx b/docs/evaluator/manage-tasks-tasksets.mdx index 0a3a76ec6b..97edfeea23 100644 --- a/docs/evaluator/manage-tasks-tasksets.mdx +++ b/docs/evaluator/manage-tasks-tasksets.mdx @@ -18,15 +18,21 @@ metrics inline every time. | Concept | What it is | Members | |---------|------------|---------| | **Task** | A reusable agent-eval unit: `intent`, `inputs`, and the `metrics` that score it. | References the metrics that score it. | -| **Taskset** | A flexible grouping of tasks with a description and metadata. | References member tasks by `workspace/name`. Membership is a **set** — order is not significant and duplicate references are rejected. | +| **Taskset** | A flexible grouping of tasks with a description and metadata. | References member tasks by `workspace/name`, each pinned to an exact revision. Membership is a **set** — order is not significant and duplicate references are rejected. | +| **Revision** | An immutable published snapshot of a task's or taskset's content, addressed by a content digest. | Belongs to the task or taskset it snapshots. | Both are addressed by `workspace/name`. Names are unique within a workspace, limited to 255 characters, and must match `^[\w\-\.]+$`. +Every stored task and taskset is **versioned**. Creating one publishes revision 1; replacing its +content publishes the next revision. Earlier revisions stay readable for as long as the task or +taskset exists — deleting it removes its revisions with it — which is what lets an evaluation be +re-run against exactly the content it ran against the first time. + -Tasks and tasksets support **create, retrieve, list, and delete** — there is no update. To change a -stored task or taskset, delete it and create a new one, or store a new version under a different -name. +Publishing is **idempotent**. Replacing a task with content identical to its current revision +publishes nothing and returns the existing revision — so a pipeline can re-submit the same +definition freely without accumulating versions. ## Initialize the SDK @@ -89,6 +95,7 @@ print(stored.id, stored.metrics) | `metrics` | `list[MetricRefOrInline]` | No | The metrics that score the task, as `MetricRef` references (`workspace/name`) to stored metrics. Pre-built inline metric bundles (`MetricInline`) are also accepted and are normalized to stored metrics on create. | | `views` | `dict[str, SemanticView]` | No | Optional reporting views mapping metric outputs into named semantic scores. | | `metadata` | `list[MetadataItem]` | No | Key/value annotations. Keys must be unique. | +| `tags` | `list[str]` | No | Tags to point at the revision this request publishes. `latest` is always applied server-side. | A stored task holds **metric references only**. Any inline metric bundle you pass on create is stored @@ -99,26 +106,97 @@ why `stored.metrics` always comes back as a list of `MetricRef` references. ### Retrieve, list, and delete ```python -# Retrieve one task by name +# Retrieve one task by name (its current content) task = tasks.retrieve("capital-of-france") +print(task.revision, task.tags) # e.g. 1 {'latest': 1} # List tasks in the workspace (paginated) page = tasks.list(page=1, page_size=100, sort="-created_at") for item in page.data: print(item.name, item.intent) -# Delete a task +# Delete a task (this also removes all of its revisions) tasks.delete("capital-of-france") ``` `sort` accepts `name`, `created_at`, or `updated_at`, each optionally prefixed with `-` for descending order. +## Revisions + +### Publish a new revision + +Use `replace` to publish new content. It creates the task if it does not exist, so a publisher needs +no existence check. + +```python +revised_task = TaskInput( + intent="Answer the user's geography question with the capital city.", + inputs=TaskInputs(instruction="Name the capital city of France."), + metrics=[MetricRef("default/answer-exact-match")], + metadata=[MetadataItem(key="suite", value="geography")], +) + +updated = tasks.replace("capital-of-france", task=revised_task) +print(updated.revision) # 2 +``` + +Submitting content identical to the current revision publishes nothing and returns the existing +revision — but any tags in the body are still applied, which is how you tag a revision after the +fact. + +### List revisions + +Each entry carries the `content_hash` used to pin a reference. + +```python +page = tasks.list_revisions("capital-of-france") +for revision in page.data: # newest first + print(revision.revision, revision.content_hash, revision.tags) +``` + +### Read a specific revision + +Pass a content digest or a tag. This returns the content **as published**, not the current content. + +```python +# Revisions come back newest-first and paginated, so index by ordinal rather than by position — +# `data[-1]` is only the oldest entry on the page you happen to have fetched. +page = tasks.list_revisions("capital-of-france") +digest = next(revision.content_hash for revision in page.data if revision.revision == 1) + +original = tasks.retrieve("capital-of-france", revision=digest) +current = tasks.retrieve("capital-of-france") +``` + +### Tag a revision + +A tag is a mutable pointer to a revision — useful for marking one as reviewed or approved after it +has been evaluated. + +```python +tasks.tag("capital-of-france", "blessed", revision=digest) + +blessed = tasks.retrieve("capital-of-france", revision="blessed") +``` + + +`latest` is managed automatically and always names the most recently published revision; it cannot be +moved by hand. A tag name may not be empty, and may not look like a content digest (64 hexadecimal +characters) — such a tag could be stored but never resolved, because a digest-shaped reference is +looked up as a digest rather than as a tag. + + ## Manage Tasksets A taskset references existing tasks by `workspace/name`. All referenced tasks must already exist when the taskset is created; a missing or duplicate reference is rejected. +Member references are **resolved to an exact revision when the taskset is stored**. You may submit a +bare name (`capital-of-france`), a tag (`capital-of-france#latest`), or a digest — what gets stored +is always `workspace/name#`. This is why a stored taskset keeps naming the same content even +after a member task publishes something new, and it is what makes a suite reproducible. + ```python from nemo_evaluator.api.schemas import TaskRef, TasksetInput @@ -132,6 +210,7 @@ taskset = TasksetInput( stored = tasksets.create("geography-suite", taskset=taskset) print(stored.tasks) +# ['default/capital-of-france#a1b2...', 'default/capital-of-japan#c3d4...'] ``` ### `TasksetInput` fields @@ -139,8 +218,9 @@ print(stored.tasks) | Field | Type | Required | Description | |-------|------|----------|-------------| | `description` | `str` | No | Human-readable description of the grouping. | -| `tasks` | `list[TaskRef]` | No | References to member tasks (`workspace/name`, or bare `name` within the same workspace). Set semantics — duplicates rejected. | +| `tasks` | `list[TaskRef]` | No | References to member tasks (`workspace/name`, or bare `name` within the same workspace), optionally pinned with `#`. Each is resolved to an exact digest when stored. Set semantics — duplicates rejected. | | `metadata` | `list[MetadataItem]` | No | Key/value annotations. Keys must be unique. | +| `tags` | `list[str]` | No | Tags to point at the revision this request publishes. `latest` is always applied server-side. | ### Retrieve, list, and delete @@ -154,6 +234,23 @@ for item in page.data: tasksets.delete("geography-suite") ``` +Tasksets carry the same revision surface as tasks — `replace`, `list_revisions`, `tag`, and +`retrieve(revision=...)`: + +```python +# Re-resolving membership after a member task published new content cuts a new revision. +tasksets.replace("geography-suite", taskset=taskset) + +for revision in tasksets.list_revisions("geography-suite").data: + print(revision.revision, revision.content_hash) +``` + + +Re-submitting the *same* member names can still publish a new revision. Members are re-resolved on +every write, so if a member task published in the meantime the grouping now names different content +and genuinely differs. A taskset's identity is the exact revisions it names, not the names alone. + + Deleting a taskset does not delete its member tasks — a taskset only holds references. ## Run an evaluation over a taskset @@ -178,8 +275,10 @@ input_spec = AgentEvalInputSpec( ) ``` -When the job runs, the taskset reference is resolved: its member tasks are loaded, and each task's -stored metric references are hydrated into runnable metrics — exactly as if you had inlined them. The +When the job runs, the taskset reference is resolved: each member's **pinned revision** is loaded — +not whatever that task currently contains — and its stored metric references are hydrated into +runnable metrics, exactly as if you had inlined them. Re-running the same taskset therefore evaluates +the same content, even if a member task has been republished since. The same spec is submitted as the agent-evaluate job input; see [Agent Evaluation](/documentation/evaluate-models/agent-eval) for the full run, target, and results flow. @@ -227,17 +326,29 @@ The SDK resources are a thin client over the Evaluator plugin REST API, mounted | Method | Path | Description | |--------|------|-------------| | `GET` | `/tasks` | List tasks (paginated). | -| `POST` | `/tasks/{name}` | Create a task. | -| `GET` | `/tasks/{name}` | Retrieve a task. | -| `DELETE` | `/tasks/{name}` | Delete a task. | +| `POST` | `/tasks/{name}` | Create a task and publish revision 1. | +| `PUT` | `/tasks/{name}` | Replace a task's content and publish; creates it if absent. | +| `GET` | `/tasks/{name}` | Retrieve a task's current content. | +| `GET` | `/tasks/{name}/revisions` | List published revisions (paginated, newest first). | +| `GET` | `/tasks/{name}/revisions/{revision}` | Retrieve content as of a digest or tag. | +| `PUT` | `/tasks/{name}/tags/{tag}?revision=` | Point a tag at an existing revision. | +| `DELETE` | `/tasks/{name}` | Delete a task and all of its revisions. | | `GET` | `/tasksets` | List tasksets (paginated). | -| `POST` | `/tasksets/{name}` | Create a taskset. | -| `GET` | `/tasksets/{name}` | Retrieve a taskset. | -| `DELETE` | `/tasksets/{name}` | Delete a taskset. | - -Creating a name that already exists returns `409`. An invalid metric reference (task) or a missing or -duplicate task reference (taskset) returns `422`. Retrieving or deleting a name that does not exist -returns `404`. +| `POST` | `/tasksets/{name}` | Create a taskset and publish revision 1. | +| `PUT` | `/tasksets/{name}` | Replace a taskset's membership and publish; creates it if absent. | +| `GET` | `/tasksets/{name}` | Retrieve a taskset's current membership. | +| `GET` | `/tasksets/{name}/revisions` | List published revisions (paginated, newest first). | +| `GET` | `/tasksets/{name}/revisions/{revision}` | Retrieve membership as of a digest or tag. | +| `PUT` | `/tasksets/{name}/tags/{tag}?revision=` | Point a tag at an existing revision. | +| `DELETE` | `/tasksets/{name}` | Delete a taskset and all of its revisions. | + +`PUT` distinguishes its two outcomes by status: **`201`** when a new revision was published, and +**`200`** when the submitted content was already the current revision and nothing was cut. + +`POST` on a name that already exists returns `409`, as does a `PUT` that loses a race with a +concurrent write. An invalid metric reference (task), a missing or duplicate task reference +(taskset), or a reserved or malformed tag name returns `422`. Retrieving or deleting a name — or a +revision — that does not exist returns `404`. ## Related Topics diff --git a/plugins/nemo-evaluator/openapi/openapi.yaml b/plugins/nemo-evaluator/openapi/openapi.yaml index 68ffee6c31..ced5332c68 100644 --- a/plugins/nemo-evaluator/openapi/openapi.yaml +++ b/plugins/nemo-evaluator/openapi/openapi.yaml @@ -1319,7 +1319,9 @@ paths: tags: - Evaluator Plugin Tasks Routes summary: Create Task - description: Store a new task, addressed by workspace/name. + description: "Store a new task, addressed by workspace/name, and publish it\ + \ as revision 1.\n\nStrict create \u2014 409 if the name is taken. Use ``PUT``\ + \ to publish a further revision of an\nexisting task." operationId: create_task_apis_evaluator_v2_workspaces__workspace__tasks__name__post parameters: - name: workspace @@ -1365,11 +1367,78 @@ paths: application/json: schema: $ref: '#/components/schemas/HTTPValidationError' + put: + tags: + - Evaluator Plugin Tasks Routes + summary: Replace Task + description: "Replace a task's content and publish the result, creating the\ + \ task if it does not exist.\n\nUpsert rather than 404-on-missing so a publisher\ + \ can issue one idempotent call without first\nchecking existence \u2014 that\ + \ check is both an extra round trip and a race between two publishers.\n\n\ + Idempotent in the strict sense: submitting content that matches the current\ + \ revision publishes\nnothing and returns **200**, while a genuine change\ + \ returns **201**. Tags in the body are\napplied either way, so re-PUTting\ + \ unchanged content is how you tag an existing revision." + operationId: replace_task_apis_evaluator_v2_workspaces__workspace__tasks__name__put + parameters: + - name: workspace + in: path + required: true + schema: + type: string + title: Workspace + - name: name + in: path + required: true + schema: + type: string + maxLength: 255 + pattern: ^[\w\-\.]+$ + title: Name + - name: project + in: query + required: false + schema: + description: Optional project to associate with the task. + title: Project + type: string + description: Optional project to associate with the task. + requestBody: + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/TaskInput' + responses: + '200': + description: Content already published; no new revision was cut + content: + application/json: + schema: + $ref: '#/components/schemas/Task' + '201': + description: A new revision was published + content: + application/json: + schema: + $ref: '#/components/schemas/Task' + '409': + description: Concurrent write; refresh and retry + '422': + description: Validation Error + content: + application/json: + schema: + $ref: '#/components/schemas/HTTPValidationError' get: tags: - Evaluator Plugin Tasks Routes summary: Get Task - description: Get a stored task by workspace and name. + description: 'Get a stored task''s current content. + + + Use ``GET /tasks/{name}/revisions/{revision}`` to read it as of a specific + published revision.' operationId: get_task_apis_evaluator_v2_workspaces__workspace__tasks__name__get parameters: - name: workspace @@ -1429,6 +1498,180 @@ paths: application/json: schema: $ref: '#/components/schemas/HTTPValidationError' + /apis/evaluator/v2/workspaces/{workspace}/tasks/{name}/revisions: + get: + tags: + - Evaluator Plugin Tasks Routes + summary: List Task Revisions + description: 'List a task''s published revisions. + + + This is how a caller discovers what it can pin to: each entry carries the + ``content_hash`` that + + goes in a reference''s ``#fragment``.' + operationId: list_task_revisions_apis_evaluator_v2_workspaces__workspace__tasks__name__revisions_get + parameters: + - name: workspace + in: path + required: true + schema: + type: string + title: Workspace + - name: name + in: path + required: true + schema: + type: string + maxLength: 255 + pattern: ^[\w\-\.]+$ + title: Name + - name: page + in: query + required: false + schema: + type: integer + minimum: 1 + description: Page number. + default: 1 + title: Page + description: Page number. + - name: page_size + in: query + required: false + schema: + type: integer + maximum: 1000 + minimum: 1 + description: Page size. + default: 100 + title: Page Size + description: Page size. + responses: + '200': + description: Return the task's published revisions, newest first + content: + application/json: + schema: + $ref: '#/components/schemas/RevisionsPage' + '404': + description: Task not found + '422': + description: Validation Error + content: + application/json: + schema: + $ref: '#/components/schemas/HTTPValidationError' + /apis/evaluator/v2/workspaces/{workspace}/tasks/{name}/revisions/{revision}: + get: + tags: + - Evaluator Plugin Tasks Routes + summary: Get Task Revision + description: "Get a task's content as of a published revision.\n\n``revision``\ + \ is a content digest or a tag \u2014 exactly what a reference's ``#fragment``\ + \ carries,\nso a consumer holding ``workspace/task-a#`` reads what\ + \ was published rather than\nwhatever is current. Returns the full task DTO;\ + \ the collection route above is a thin index of\nwhat can be pinned to." + operationId: get_task_revision_apis_evaluator_v2_workspaces__workspace__tasks__name__revisions__revision__get + parameters: + - name: workspace + in: path + required: true + schema: + type: string + title: Workspace + - name: name + in: path + required: true + schema: + type: string + maxLength: 255 + pattern: ^[\w\-\.]+$ + title: Name + - name: revision + in: path + required: true + schema: + type: string + maxLength: 255 + pattern: ^[\w\-\.]+$ + title: Revision + responses: + '200': + description: Return the task's content as of a published revision + content: + application/json: + schema: + $ref: '#/components/schemas/Task' + '404': + description: Task or revision not found + '422': + description: Validation Error + content: + application/json: + schema: + $ref: '#/components/schemas/HTTPValidationError' + /apis/evaluator/v2/workspaces/{workspace}/tasks/{name}/tags/{tag}: + put: + tags: + - Evaluator Plugin Tasks Routes + summary: Tag Task Revision + description: 'Point a tag at an already-published revision. + + + Separate from publishing because blessing a revision usually happens *after* + it has been + + evaluated. ``latest`` is reserved: it is machine-managed, always names the + most recent publish, + + and moving it by hand would break the forward-only guarantee that keeps concurrent + publishes + + consistent.' + operationId: tag_task_revision_apis_evaluator_v2_workspaces__workspace__tasks__name__tags__tag__put + parameters: + - name: workspace + in: path + required: true + schema: + type: string + title: Workspace + - name: name + in: path + required: true + schema: + type: string + maxLength: 255 + pattern: ^[\w\-\.]+$ + title: Name + - name: tag + in: path + required: true + schema: + type: string + maxLength: 255 + pattern: ^[\w\-\.]+$ + title: Tag + - name: revision + in: query + required: true + schema: + type: string + description: "Revision to tag \u2014 a content digest, or another tag." + title: Revision + description: "Revision to tag \u2014 a content digest, or another tag." + responses: + '200': + description: Point a tag at an existing revision + content: + application/json: + schema: + $ref: '#/components/schemas/Task' + '404': + description: Task or revision not found + '422': + description: Tag is reserved /apis/evaluator/v2/workspaces/{workspace}/tasksets: get: tags: @@ -1501,7 +1744,9 @@ paths: tags: - Evaluator Plugin Tasksets Routes summary: Create Taskset - description: Store a new taskset, addressed by workspace/name. + description: "Store a new taskset, addressed by workspace/name, and publish\ + \ it as revision 1.\n\nStrict create \u2014 409 if the name is taken. Use\ + \ ``PUT`` to publish a further revision." operationId: create_taskset_apis_evaluator_v2_workspaces__workspace__tasksets__name__post parameters: - name: workspace @@ -1547,11 +1792,76 @@ paths: application/json: schema: $ref: '#/components/schemas/HTTPValidationError' + put: + tags: + - Evaluator Plugin Tasksets Routes + summary: Replace Taskset + description: "Replace a taskset's membership and publish the result, creating\ + \ it if it does not exist.\n\nMembers are re-resolved to exact revision digests\ + \ on every write, so re-submitting the *same*\nmember names can still publish\ + \ a new revision \u2014 if a member task published in the meantime,\nthis\ + \ grouping now names different content and genuinely differs. A taskset's\ + \ identity is the\nexact revisions it names, not the names alone." + operationId: replace_taskset_apis_evaluator_v2_workspaces__workspace__tasksets__name__put + parameters: + - name: workspace + in: path + required: true + schema: + type: string + title: Workspace + - name: name + in: path + required: true + schema: + type: string + maxLength: 255 + pattern: ^[\w\-\.]+$ + title: Name + - name: project + in: query + required: false + schema: + description: Optional project to associate with the taskset. + title: Project + type: string + description: Optional project to associate with the taskset. + requestBody: + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/TasksetInput' + responses: + '200': + description: Content already published; no new revision was cut + content: + application/json: + schema: + $ref: '#/components/schemas/Taskset' + '201': + description: A new revision was published + content: + application/json: + schema: + $ref: '#/components/schemas/Taskset' + '409': + description: Concurrent write; refresh and retry + '422': + description: Validation Error + content: + application/json: + schema: + $ref: '#/components/schemas/HTTPValidationError' get: tags: - Evaluator Plugin Tasksets Routes summary: Get Taskset - description: Get a stored taskset by workspace and name. + description: 'Get a stored taskset''s current membership. + + + Use ``GET /tasksets/{name}/revisions/{revision}`` to read it as of a published + revision.' operationId: get_taskset_apis_evaluator_v2_workspaces__workspace__tasksets__name__get parameters: - name: workspace @@ -1611,6 +1921,162 @@ paths: application/json: schema: $ref: '#/components/schemas/HTTPValidationError' + /apis/evaluator/v2/workspaces/{workspace}/tasksets/{name}/revisions: + get: + tags: + - Evaluator Plugin Tasksets Routes + summary: List Taskset Revisions + description: "List a taskset's published revisions \u2014 each entry's ``content_hash``\ + \ is what a pinned\nreference carries." + operationId: list_taskset_revisions_apis_evaluator_v2_workspaces__workspace__tasksets__name__revisions_get + parameters: + - name: workspace + in: path + required: true + schema: + type: string + title: Workspace + - name: name + in: path + required: true + schema: + type: string + maxLength: 255 + pattern: ^[\w\-\.]+$ + title: Name + - name: page + in: query + required: false + schema: + type: integer + minimum: 1 + description: Page number. + default: 1 + title: Page + description: Page number. + - name: page_size + in: query + required: false + schema: + type: integer + maximum: 1000 + minimum: 1 + description: Page size. + default: 100 + title: Page Size + description: Page size. + responses: + '200': + description: Return the taskset's published revisions, newest first + content: + application/json: + schema: + $ref: '#/components/schemas/RevisionsPage' + '404': + description: Taskset not found + '422': + description: Validation Error + content: + application/json: + schema: + $ref: '#/components/schemas/HTTPValidationError' + /apis/evaluator/v2/workspaces/{workspace}/tasksets/{name}/revisions/{revision}: + get: + tags: + - Evaluator Plugin Tasksets Routes + summary: Get Taskset Revision + description: "Get a taskset's membership as of a published revision.\n\n``revision``\ + \ is a content digest or a tag \u2014 what a reference's ``#fragment`` carries\ + \ \u2014 so a\npinned consumer reads the exact grouping that was published." + operationId: get_taskset_revision_apis_evaluator_v2_workspaces__workspace__tasksets__name__revisions__revision__get + parameters: + - name: workspace + in: path + required: true + schema: + type: string + title: Workspace + - name: name + in: path + required: true + schema: + type: string + maxLength: 255 + pattern: ^[\w\-\.]+$ + title: Name + - name: revision + in: path + required: true + schema: + type: string + maxLength: 255 + pattern: ^[\w\-\.]+$ + title: Revision + responses: + '200': + description: Return the taskset's membership as of a published revision + content: + application/json: + schema: + $ref: '#/components/schemas/Taskset' + '404': + description: Taskset or revision not found + '422': + description: Validation Error + content: + application/json: + schema: + $ref: '#/components/schemas/HTTPValidationError' + /apis/evaluator/v2/workspaces/{workspace}/tasksets/{name}/tags/{tag}: + put: + tags: + - Evaluator Plugin Tasksets Routes + summary: Tag Taskset Revision + description: Point a tag at an already-published revision. ``latest`` is reserved + and machine-managed. + operationId: tag_taskset_revision_apis_evaluator_v2_workspaces__workspace__tasksets__name__tags__tag__put + parameters: + - name: workspace + in: path + required: true + schema: + type: string + title: Workspace + - name: name + in: path + required: true + schema: + type: string + maxLength: 255 + pattern: ^[\w\-\.]+$ + title: Name + - name: tag + in: path + required: true + schema: + type: string + maxLength: 255 + pattern: ^[\w\-\.]+$ + title: Tag + - name: revision + in: query + required: true + schema: + type: string + description: "Revision to tag \u2014 a content digest, or another tag." + title: Revision + description: "Revision to tag \u2014 a content digest, or another tag." + responses: + '200': + description: Point a tag at an existing revision + content: + application/json: + schema: + $ref: '#/components/schemas/Taskset' + '404': + description: Taskset or revision not found + '422': + description: Tag is reserved components: schemas: AgentEvalInputSpec: @@ -4194,6 +4660,64 @@ components: - -updated_at title: ResultSort description: Sort fields for result queries (``-`` prefix sorts descending). + Revision: + properties: + revision: + type: integer + title: Revision + description: Monotonic 1-based ordinal within the record. + content_hash: + type: string + title: Content Hash + description: 'Full 64-char hex SHA-256 of the revision''s content. This + is what a pinned reference carries: ''workspace/name#''.' + tags: + items: + type: string + type: array + title: Tags + description: Tags currently pointing at this revision, if any. + created_at: + type: string + format: date-time + title: Created At + description: Timestamp the revision was published. + type: object + required: + - revision + - content_hash + - created_at + title: Revision + description: "A published revision of a task or taskset.\n\nDeliberately thin:\ + \ it identifies a revision and says when it was cut, without repeating the\n\ + content. Listing a record's history is a \"what can I pin to?\" question,\ + \ and answering it with\nfull content on every entry would make the response\ + \ large for no benefit \u2014 fetch the record at\na specific revision to\ + \ get its content." + RevisionsPage: + properties: + data: + items: + $ref: '#/components/schemas/Revision' + type: array + title: Data + pagination: + allOf: + - $ref: '#/components/schemas/PaginationData' + description: Pagination information. + sort: + title: Sort + description: The field on which the results are sorted. + type: string + filter: + title: Filter + description: Filtering information. + additionalProperties: true + type: object + type: object + required: + - data + title: RevisionsPage RubricScoreStat: properties: label: @@ -4442,6 +4966,21 @@ components: type: array title: Metadata description: Key/value annotations for the task. + revision: + type: integer + title: Revision + description: "Ordinal of the published revision this content corresponds\ + \ to. Every stored task has at least one revision \u2014 creating a task\ + \ publishes revision 1 \u2014 so this is never 0." + tags: + additionalProperties: + type: integer + type: object + title: Tags + description: "Tag \u2192 revision-ordinal pointers. Reading the record's\ + \ current content returns every tag, including 'latest'. Reading a *specific*\ + \ revision returns only the tags pointing at that revision, which may\ + \ be none \u2014 so do not assume 'latest' is present." created_at: type: string format: date-time @@ -4458,6 +4997,7 @@ components: - name - workspace - intent + - revision - created_at - updated_at title: Task @@ -4521,6 +5061,13 @@ components: type: array title: Metadata description: Key/value annotations for the task. + tags: + items: + type: string + type: array + title: Tags + description: Tags to point at the revision this request publishes. 'latest' + is always applied server-side and need not be listed. additionalProperties: false type: object required: @@ -4548,12 +5095,18 @@ components: runtime falls back to the task ``intent`` when it is unset.' TaskRef: type: string - pattern: ^[\w\-.]+(/[\w\-.]+)?$ + pattern: ^[\w\-.]+(/[\w\-.]+)?(#[\w\-.]+)?$ title: TaskRef - description: "Reference to a persisted task (format: ``workspace/name`` or ``name``).\n\ - \nSame shape and charset as :class:`MetricRef` \u2014 a taskset points at\ - \ its member tasks by reference\n(there are no inline tasks), so a stored\ - \ taskset only ever holds refs." + description: "Reference to a persisted task (format: ``workspace/name``, ``name``,\ + \ or either with a\n``#revision`` fragment).\n\nA taskset points at its member\ + \ tasks by reference (there are no inline tasks), so a stored\ntaskset only\ + \ ever holds refs. Unlike :class:`MetricRef`, a task ref may address a specific\n\ + revision via the platform's standard ``#`` sub-entity fragment.\n\nThe fragment\ + \ is optional *on input* and means :data:`LATEST_TAG` when absent \u2014 a\ + \ bare\n``workspace/name`` is \"the current revision\", not \"unpinned\".\ + \ It may name a tag or a content\ndigest. Anything **persisted** as a published\ + \ snapshot must carry a resolved digest: tags move,\nand a stored tag fragment\ + \ would silently re-point published membership." TaskSort: type: string enum: @@ -4623,6 +5176,20 @@ components: type: array title: Metadata description: Key/value annotations for the taskset. + revision: + type: integer + title: Revision + description: Ordinal of the published revision this content corresponds + to. Every stored taskset has at least one revision, so this is never 0. + tags: + additionalProperties: + type: integer + type: object + title: Tags + description: "Tag \u2192 revision-ordinal pointers. Reading the record's\ + \ current content returns every tag, including 'latest'. Reading a *specific*\ + \ revision returns only the tags pointing at that revision, which may\ + \ be none \u2014 so do not assume 'latest' is present." created_at: type: string format: date-time @@ -4638,6 +5205,7 @@ components: - id - name - workspace + - revision - created_at - updated_at title: Taskset @@ -4680,12 +5248,22 @@ components: type: array title: Tasks description: References to the member tasks (set semantics; duplicates rejected). + Each may be bare, tag-pinned ('task-a#latest'), or digest-pinned; all + are resolved to an exact digest when stored, so the grouping cannot change + underneath you when a member republishes. metadata: items: $ref: '#/components/schemas/MetadataItem' type: array title: Metadata description: Key/value annotations for the taskset. + tags: + items: + type: string + type: array + title: Tags + description: Tags to point at the revision this request publishes. 'latest' + is always applied server-side and need not be listed. additionalProperties: false type: object title: TasksetInput diff --git a/plugins/nemo-evaluator/src/nemo_evaluator/api/schemas.py b/plugins/nemo-evaluator/src/nemo_evaluator/api/schemas.py index 10c5a62b60..bdda7dfed6 100644 --- a/plugins/nemo-evaluator/src/nemo_evaluator/api/schemas.py +++ b/plugins/nemo-evaluator/src/nemo_evaluator/api/schemas.py @@ -5,10 +5,12 @@ from __future__ import annotations +import re from datetime import datetime from enum import StrEnum from typing import Annotated, Any, Literal, TypeAlias +from nemo_evaluator.content_hash import DIGEST_PATTERN from nemo_evaluator.shared.metric_bundles.bundles import ( BundledMetricOutputSpec, MetricMetadata, @@ -138,6 +140,28 @@ class MetricInline(BaseModel): # empty/malformed refs are rejected at validation rather than during parsing. _ENTITY_REF_PATTERN = r"^[\w\-.]+(/[\w\-.]+)?$" +#: The charset a ``#fragment`` may use. Exported because anything that *mints* a fragment — notably +#: revision tag names — has to be constrained by it: a value outside this set can be stored happily +#: and then never appear in a reference, which is a silent dead end rather than an error. +REF_FRAGMENT_CHARSET = r"[\w\-.]+" + +# A *sub-entity* reference adds an optional ``#fragment``, the platform's standard way of addressing +# something contained within an entity (filesets address a contained file the same way: +# ``workspace/fileset#path``). For a revisioned entity the fragment selects a revision — either a tag +# (``#latest``, ``#candidate``) or a full 64-char content digest. +# +# Deliberately a sibling of ``_ENTITY_REF_PATTERN`` rather than a widening of it: that constant is +# shared by ``MetricRef`` and ``TasksetRef``, neither of which has revisions yet, and admitting a +# fragment there would accept input nothing is built to resolve. ``TasksetRef`` moves onto this +# pattern when taskset revisions are addressable; ``MetricRef`` when (if) metrics gain revisions. +_SUBENTITY_REF_PATTERN = rf"^[\w\-.]+(/[\w\-.]+)?(#{REF_FRAGMENT_CHARSET})?$" + +#: The fragment separator for sub-entity references. Matches the fileset/job ref convention. +REF_FRAGMENT_SEPARATOR = "#" + +#: The tag applied to every publish and used when a ref carries no fragment. +LATEST_TAG = "latest" + def parse_entity_ref(root: str, default_workspace: str) -> tuple[str, str]: """Split a validated ``workspace/name`` (or bare ``name``) reference into ``(workspace, name)``. @@ -145,11 +169,27 @@ def parse_entity_ref(root: str, default_workspace: str) -> tuple[str, str]: The ``workspace/name`` vs bare-``name`` shape is guaranteed by the field's ``_ENTITY_REF_PATTERN``, so this only needs to split. Shared by every reference type (metrics, tasks); lives here — next to the pattern, with no entity dependency — so ref-owning modules can reuse it without cycling. + + Any ``#fragment`` is stripped before splitting, so callers that don't care about revisions keep + working unchanged against a pinned ref. Use :func:`parse_subentity_ref` to read the fragment. """ - workspace, separator, name = root.partition("/") + base, _, _ = root.partition(REF_FRAGMENT_SEPARATOR) + workspace, separator, name = base.partition("/") if separator: return workspace, name - return default_workspace, root + return default_workspace, base + + +def parse_subentity_ref(root: str, default_workspace: str) -> tuple[str, str, str]: + """Split a reference into ``(workspace, name, fragment)``. + + An absent fragment resolves to :data:`LATEST_TAG` — a bare ``workspace/name`` means "the current + revision", never "unpinned". The fragment is returned verbatim: it may be a tag or a content + digest, and telling them apart is resolution's job, not parsing's. + """ + base, separator, fragment = root.partition(REF_FRAGMENT_SEPARATOR) + workspace, name = parse_entity_ref(base, default_workspace) + return workspace, name, fragment if separator and fragment else LATEST_TAG class MetricRef(RootModel[str]): @@ -168,15 +208,23 @@ class MetricRef(RootModel[str]): class TaskRef(RootModel[str]): - """Reference to a persisted task (format: ``workspace/name`` or ``name``). + """Reference to a persisted task (format: ``workspace/name``, ``name``, or either with a + ``#revision`` fragment). - Same shape and charset as :class:`MetricRef` — a taskset points at its member tasks by reference - (there are no inline tasks), so a stored taskset only ever holds refs. + A taskset points at its member tasks by reference (there are no inline tasks), so a stored + taskset only ever holds refs. Unlike :class:`MetricRef`, a task ref may address a specific + revision via the platform's standard ``#`` sub-entity fragment. + + The fragment is optional *on input* and means :data:`LATEST_TAG` when absent — a bare + ``workspace/name`` is "the current revision", not "unpinned". It may name a tag or a content + digest. Anything **persisted** as a published snapshot must carry a resolved digest: tags move, + and a stored tag fragment would silently re-point published membership. """ root: str = Field( - pattern=_ENTITY_REF_PATTERN, - description="Reference to a stored task (format: workspace/task-name, or task-name in the taskset workspace).", + pattern=_SUBENTITY_REF_PATTERN, + description="Reference to a stored task (format: workspace/task-name, or task-name in the " + "taskset workspace), optionally pinned to a revision with '#'.", ) @@ -348,6 +396,16 @@ class Task(BaseModel): default_factory=dict, description="Optional reporting views mapping metric outputs into named semantic scores." ) metadata: TaskMetadataList = Field(default_factory=list, description="Key/value annotations for the task.") + revision: int = Field( + description="Ordinal of the published revision this content corresponds to. Every stored task " + "has at least one revision — creating a task publishes revision 1 — so this is never 0." + ) + tags: dict[str, int] = Field( + default_factory=dict, + description="Tag → revision-ordinal pointers. Reading the record's current content returns " + "every tag, including 'latest'. Reading a *specific* revision returns only the tags pointing " + "at that revision, which may be none — so do not assume 'latest' is present.", + ) created_at: datetime = Field(description="Timestamp the task was created.") updated_at: datetime = Field(description="Timestamp the task was last updated.") @@ -370,6 +428,29 @@ class TaskInput(BaseModel): default_factory=dict, description="Optional reporting views mapping metric outputs into named semantic scores." ) metadata: TaskMetadataList = Field(default_factory=list, description="Key/value annotations for the task.") + tags: list[str] = Field( + default_factory=list, + description="Tags to point at the revision this request publishes. 'latest' is always applied " + "server-side and need not be listed.", + ) + + +class Revision(BaseModel): + """A published revision of a task or taskset. + + Deliberately thin: it identifies a revision and says when it was cut, without repeating the + content. Listing a record's history is a "what can I pin to?" question, and answering it with + full content on every entry would make the response large for no benefit — fetch the record at + a specific revision to get its content. + """ + + revision: int = Field(description="Monotonic 1-based ordinal within the record.") + content_hash: str = Field( + description="Full 64-char hex SHA-256 of the revision's content. This is what a pinned " + "reference carries: 'workspace/name#'." + ) + tags: list[str] = Field(default_factory=list, description="Tags currently pointing at this revision, if any.") + created_at: datetime = Field(description="Timestamp the revision was published.") class TaskSort(StrEnum): @@ -406,6 +487,35 @@ def _reject_duplicate_task_refs(refs: list[TaskRef]) -> list[TaskRef]: #: A list of task references with set semantics (order not significant, duplicates rejected). TaskRefList: TypeAlias = Annotated[list[TaskRef], AfterValidator(_reject_duplicate_task_refs)] +#: Shape of a content digest in a ref fragment: full-length lowercase hex, never truncated. +_DIGEST_FRAGMENT_PATTERN = re.compile(DIGEST_PATTERN) + + +def _require_pinned_task_refs(refs: list[TaskRef]) -> list[TaskRef]: + """Every member of a *published* taskset revision must name an exact content digest. + + Enforced on the field rather than in the publish path so it cannot be bypassed by any other + writer. A ref that is bare (``workspace/name``) or tag-pinned (``#latest``, ``#candidate``) + resolves through a mutable pointer: the moment that tag moves, the published revision's + membership silently changes under it, and a "reproducible" dataset stops being reproducible. + Tags are resolution *inputs*, resolved to digests at publish time; only digests persist. + """ + for ref in refs: + _, _, fragment = parse_subentity_ref(ref.root, "") + if not _DIGEST_FRAGMENT_PATTERN.match(fragment): + raise ValueError( + f"task reference {ref.root!r} is not pinned to a content digest: a published taskset " + f"revision must reference an exact revision (got fragment {fragment!r}). Tags move; " + "resolve them to a digest before persisting." + ) + return refs + + +#: Member refs of a published taskset revision: set semantics *and* every ref digest-pinned. +PinnedTaskRefList: TypeAlias = Annotated[ + list[TaskRef], AfterValidator(_reject_duplicate_task_refs), AfterValidator(_require_pinned_task_refs) +] + class Taskset(BaseModel): """API representation of a stored taskset — a flexible grouping of tasks with metadata. @@ -423,6 +533,16 @@ class Taskset(BaseModel): default_factory=list, description="References to the member tasks (set semantics; duplicates rejected)." ) metadata: TaskMetadataList = Field(default_factory=list, description="Key/value annotations for the taskset.") + revision: int = Field( + description="Ordinal of the published revision this content corresponds to. Every stored " + "taskset has at least one revision, so this is never 0." + ) + tags: dict[str, int] = Field( + default_factory=dict, + description="Tag → revision-ordinal pointers. Reading the record's current content returns " + "every tag, including 'latest'. Reading a *specific* revision returns only the tags pointing " + "at that revision, which may be none — so do not assume 'latest' is present.", + ) created_at: datetime = Field(description="Timestamp the taskset was created.") updated_at: datetime = Field(description="Timestamp the taskset was last updated.") @@ -438,9 +558,17 @@ class TasksetInput(BaseModel): description: str | None = Field(default=None, description="Human-readable description of the grouping.") tasks: TaskRefList = Field( - default_factory=list, description="References to the member tasks (set semantics; duplicates rejected)." + default_factory=list, + description="References to the member tasks (set semantics; duplicates rejected). Each may be " + "bare, tag-pinned ('task-a#latest'), or digest-pinned; all are resolved to an exact digest " + "when stored, so the grouping cannot change underneath you when a member republishes.", ) metadata: TaskMetadataList = Field(default_factory=list, description="Key/value annotations for the taskset.") + tags: list[str] = Field( + default_factory=list, + description="Tags to point at the revision this request publishes. 'latest' is always applied " + "server-side and need not be listed.", + ) class TasksetSort(StrEnum): diff --git a/plugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.py b/plugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.py index a64e4ceb58..171bda0fae 100644 --- a/plugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.py +++ b/plugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.py @@ -13,13 +13,30 @@ from __future__ import annotations import logging -from typing import Protocol +from typing import Protocol, cast -from nemo_evaluator.api.schemas import MetricInline, MetricRef, Task, TaskInput, parse_entity_ref -from nemo_evaluator.entities import TaskEntity -from nemo_platform_plugin.entities import PaginationInfo +from nemo_evaluator.api.schemas import ( + LATEST_TAG, + MetricInline, + MetricRef, + Revision, + Task, + TaskInput, + parse_entity_ref, +) +from nemo_evaluator.entities import TaskEntity, TaskRevisionEntity +from nemo_evaluator.revisions import ( + apply_tag, + get_revision, + list_revisions, + publish_revision, +) +from nemo_platform_plugin.entities import ( + EntityClientProtocol, + EntityUpdateClientProtocol, + PaginationInfo, +) from nemo_platform_plugin.entity_client import ( - NemoEntitiesClientProtocol, NemoEntityConflictError, NemoEntityNotFoundError, ) @@ -56,11 +73,53 @@ def _entity_to_task(entity: TaskEntity) -> Task: metrics=entity.metrics, views=entity.views, metadata=entity.metadata, + revision=entity.latest_revision, + tags=entity.tags, created_at=created_at, updated_at=updated_at, ) +def _revision_to_task(head: TaskEntity, revision: TaskRevisionEntity) -> Task: + """Present a published revision as the ``Task`` DTO. + + Identity (id, name, workspace, project) comes from the head — it is the same task — while every + content field comes from the revision. ``tags`` are the head's, because tags are current state: + "which tags point here now" is a question about the present, not about what was frozen. + """ + created_at = revision.created_at + if created_at is None: + raise ValueError(f"Revision {revision.revision} of '{head.workspace}/{head.name}' has no timestamp") + return Task( + id=head.id, + name=head.name, + workspace=head.workspace, + project=head.project, + intent=revision.intent, + inputs=revision.inputs, + metrics=revision.metrics, + views=revision.views, + metadata=revision.metadata, + revision=revision.revision, + tags={tag: ordinal for tag, ordinal in head.tags.items() if ordinal == revision.revision}, + created_at=created_at, + updated_at=revision.updated_at or created_at, + ) + + +def _entity_to_revision(revision: TaskRevisionEntity, tags: dict[str, int]) -> Revision: + """Map a stored revision to its API DTO, attaching the tags that currently point at it.""" + created_at = revision.created_at + if created_at is None: + raise ValueError(f"Revision {revision.revision} is missing its creation timestamp") + return Revision( + revision=revision.revision, + content_hash=revision.content_hash, + tags=sorted(tag for tag, ordinal in tags.items() if ordinal == revision.revision), + created_at=created_at, + ) + + def _pagination(src: PaginationInfo, current_page_size: int) -> PaginationData: """Carry the entity-store pagination counts into the API ``Page`` envelope.""" return PaginationData( @@ -72,11 +131,25 @@ def _pagination(src: PaginationInfo, current_page_size: int) -> PaginationData: ) +class TaskEntityStoreProtocol(EntityClientProtocol[TaskEntity], EntityUpdateClientProtocol[TaskEntity], Protocol): + """The store surface this service needs for its own task records. + + Composed from the shared entity-client protocols rather than a bespoke one, so the surface a + service depends on cannot drift from the client that satisfies it. + """ + + class TaskService: """Create/get/list/delete for persisted agent-eval task entities, exposed as the ``Task`` DTO.""" - def __init__(self, entity_client: NemoEntitiesClientProtocol[TaskEntity], metric_service: _MetricService): + def __init__(self, entity_client: TaskEntityStoreProtocol, metric_service: _MetricService): self.entity_client = entity_client + #: The same client, viewed at the revision type. Python has no intersection types, so a + #: single annotation cannot say "serves TaskEntity *and* TaskRevisionEntity" — but the + #: concrete client's methods are generic over the entity type and genuinely satisfy both. + self.revision_client: EntityClientProtocol[TaskRevisionEntity] = cast( + EntityClientProtocol[TaskRevisionEntity], entity_client + ) self.metric_service = metric_service async def _normalize_metrics(self, metrics: list[MetricRef | MetricInline], *, workspace: str) -> list[MetricRef]: @@ -97,35 +170,156 @@ async def _normalize_metrics(self, metrics: list[MetricRef | MetricInline], *, w refs.append(await self.metric_service.store_derived_metric(metric, workspace=workspace)) return refs + async def _apply_content(self, entity: TaskEntity, task_input: TaskInput, *, workspace: str) -> TaskEntity: + """Overwrite a head record's content from a request body (leaving revision pointers alone).""" + entity.intent = task_input.intent + entity.inputs = task_input.inputs + entity.metrics = await self._normalize_metrics(task_input.metrics, workspace=workspace) + entity.views = task_input.views + entity.metadata = task_input.metadata + return entity + async def create_task( self, name: str, task_input: TaskInput, *, workspace: str, project: str | None = None - ) -> Task: - """Store a new task (addressed by workspace/name). Raises ``ValueError`` if it already exists.""" - entity = TaskEntity( - name=name, + ) -> tuple[Task, bool]: + """Store a new task and publish it as revision 1. + + Strict create: raises ``ValueError`` if the name is taken. Returns ``(task, published)``, + where ``published`` is always ``True`` here — a fresh task always cuts a revision. Use + :meth:`replace_task` to publish a further revision of an existing task. + """ + entity = await self._apply_content( + TaskEntity(name=name, workspace=workspace, project=project, intent=task_input.intent), + task_input, workspace=workspace, - project=project, - intent=task_input.intent, - inputs=task_input.inputs, - metrics=await self._normalize_metrics(task_input.metrics, workspace=workspace), - views=task_input.views, - metadata=task_input.metadata, ) try: created = await self.entity_client.create(entity) except NemoEntityConflictError as exc: raise ValueError(f"Task '{workspace}/{name}' already exists") from exc + try: + head, published = await self._publish(created, tags=set(task_input.tags)) + except Exception: + # A head with no revision would violate the invariant every consumer relies on — that + # `#latest` always resolves and `revision` is never 0. There is no cross-entity + # transaction, so roll the head back by hand rather than leaving a half-created task. + logger.exception("Publishing revision 1 failed; rolling back the task record") + try: + await self.entity_client.delete(TaskEntity, name=name, workspace=workspace) + except Exception: + # Report the rollback failure, but re-raise the *original* error: replacing it + # would hide why the publish failed and leave the caller debugging the cleanup. + logger.exception("Rollback of the orphaned task record also failed") + raise logger.info( "Task created", extra={"workspace": sanitize_for_log(workspace), "task_name": sanitize_for_log(name)} ) - return _entity_to_task(created) + return _entity_to_task(head), published + + async def replace_task( + self, name: str, task_input: TaskInput, *, workspace: str, project: str | None = None + ) -> tuple[Task, bool]: + """Replace a task's content and publish the result, creating the task if absent. + + Upsert rather than 404-on-missing so a publisher can issue one idempotent call without + first checking existence — checking then creating is both an extra round trip and a race + between two publishers of the same task. + + Returns ``(task, published)``. ``published`` is ``False`` when the submitted content matches + the current revision: the request is then a no-op that still applies any new tags, which is + what makes repeated PUTs of the same content cheap and genuinely idempotent. + """ + try: + head = await self.entity_client.get(TaskEntity, name=name, workspace=workspace) + except NemoEntityNotFoundError: + return await self.create_task(name, task_input, workspace=workspace, project=project) + + await self._apply_content(head, task_input, workspace=workspace) + if project is not None: + # Applied rather than ignored. ``project`` is a query parameter, so an omitted value + # means "leave it alone" — only an explicit value moves the record. + head.project = project + # Publish the staged content *without* committing the head first. Publishing already writes + # the head (pointers and content together), so a pre-write would be a second round trip + # whose only distinct effect is a window: if publishing then failed, the head would hold + # content no revision covers and a plain GET would serve it. + published_head, published = await self._publish(head, tags=set(task_input.tags)) + if not published: + # Content matched a revision that is already tagged as requested, so publishing wrote + # nothing. Anything outside the digest — ``project`` — still has to be persisted. + published_head = await self.entity_client.update(published_head) + logger.info( + "Task replaced", + extra={ + "workspace": sanitize_for_log(workspace), + "task_name": sanitize_for_log(name), + "published": published, + }, + ) + return _entity_to_task(published_head), published + + async def _publish(self, head: TaskEntity, *, tags: set[str]) -> tuple[TaskEntity, bool]: + """Freeze the head as a revision. The returned head already carries the new pointers.""" + _, published_head, created = await publish_revision( + self.entity_client, self.revision_client, head, TaskRevisionEntity, tags=tags + ) + return published_head, created + + async def resolve_revision(self, workspace: str, name: str, fragment: str = LATEST_TAG) -> str: + """Return the content digest of the revision a ref fragment names. + + This is what turns a *tag*-pinned member reference into a *digest*-pinned one at taskset + publish time. Raises :class:`RevisionNotFoundError` if the task has no such revision, and + ``NemoEntityNotFoundError`` if the task itself is missing. + """ + head = await self.entity_client.get(TaskEntity, name=name, workspace=workspace) + revision = await get_revision(self.revision_client, TaskRevisionEntity, head, fragment) + return revision.content_hash + + async def list_revisions( + self, workspace: str, name: str, *, page: int = 1, page_size: int = 100 + ) -> Page[Revision] | None: + """List a task's published revisions, newest first; ``None`` if the task is absent. + + Paged rather than returning every revision: a bare list would silently truncate at the + store's page size, making a capped history indistinguishable from a complete one. + """ + try: + head = await self.entity_client.get(TaskEntity, name=name, workspace=workspace) + except NemoEntityNotFoundError: + return None + result = await list_revisions(self.revision_client, TaskRevisionEntity, head, page=page, page_size=page_size) + data = [_entity_to_revision(revision, head.tags) for revision in result.data] + return Page(data=data, pagination=_pagination(result.pagination, len(data)), sort=None, filter=None) + + async def tag_revision(self, workspace: str, name: str, tag: str, fragment: str) -> Task | None: + """Point a tag at an existing revision; ``None`` if the task is absent.""" + try: + head = await self.entity_client.get(TaskEntity, name=name, workspace=workspace) + except NemoEntityNotFoundError: + return None + updated = await apply_tag(self.entity_client, self.revision_client, TaskRevisionEntity, head, tag, fragment) + logger.info( + "Task revision tagged", + extra={"workspace": sanitize_for_log(workspace), "task_name": sanitize_for_log(name)}, + ) + return _entity_to_task(updated) + + async def get_task(self, workspace: str, name: str, revision: str | None = None) -> Task | None: + """Get a stored task; ``None`` if absent. - async def get_task(self, workspace: str, name: str) -> Task | None: + ``revision`` is a tag or a content digest — the same thing a ref's ``#fragment`` carries. + Omitted, the current content is returned. Supplied, the task is returned *as of* that + revision, which is how a consumer holding a pinned reference reads what was published + rather than what happens to be current. + """ try: - entity = await self.entity_client.get(TaskEntity, workspace=workspace, name=name) + head = await self.entity_client.get(TaskEntity, name=name, workspace=workspace) except NemoEntityNotFoundError: return None - return _entity_to_task(entity) + if revision is None: + return _entity_to_task(head) + return _revision_to_task(head, await get_revision(self.revision_client, TaskRevisionEntity, head, revision)) async def list_tasks( self, @@ -150,7 +344,7 @@ async def list_tasks( async def delete_task(self, workspace: str, name: str) -> bool: """Delete a stored task; ``False`` if absent.""" try: - await self.entity_client.delete(TaskEntity, name, workspace=workspace) + await self.entity_client.delete(TaskEntity, name=name, workspace=workspace) except NemoEntityNotFoundError: return False logger.info( diff --git a/plugins/nemo-evaluator/src/nemo_evaluator/api/service/taskset_service.py b/plugins/nemo-evaluator/src/nemo_evaluator/api/service/taskset_service.py index adb404c9ea..3ac6d87bbc 100644 --- a/plugins/nemo-evaluator/src/nemo_evaluator/api/service/taskset_service.py +++ b/plugins/nemo-evaluator/src/nemo_evaluator/api/service/taskset_service.py @@ -9,20 +9,40 @@ ``TaskService`` uses — so the wire contract round-trips cleanly (an ``EntityBase``'s ``id``/ ``created_at`` are computed and don't deserialize from the entity's own serialized form). -Unlike ``TaskService`` there are no inline members to normalize; instead, each referenced task is -validated to exist at create time (a taskset that points at missing tasks is rejected). +Unlike ``TaskService`` there are no inline members to normalize. Instead, every member reference is +resolved on write to the exact revision digest it names, which is also where a missing member is +caught — resolution has to fetch the task anyway, so existence falls out of it rather than costing +a separate round trip per member. """ from __future__ import annotations +import asyncio import logging -from typing import Protocol - -from nemo_evaluator.api.schemas import TaskRef, Taskset, TasksetInput, parse_entity_ref -from nemo_evaluator.entities import TasksetEntity -from nemo_platform_plugin.entities import PaginationInfo +from typing import Protocol, cast + +from nemo_evaluator.api.schemas import ( + Revision, + TaskRef, + Taskset, + TasksetInput, + parse_entity_ref, + parse_subentity_ref, +) +from nemo_evaluator.entities import TasksetEntity, TasksetRevisionEntity +from nemo_evaluator.revisions import ( + RevisionNotFoundError, + apply_tag, + get_revision, + list_revisions, + publish_revision, +) +from nemo_platform_plugin.entities import ( + EntityClientProtocol, + EntityUpdateClientProtocol, + PaginationInfo, +) from nemo_platform_plugin.entity_client import ( - NemoEntitiesClientProtocol, NemoEntityConflictError, NemoEntityNotFoundError, ) @@ -32,10 +52,17 @@ logger = logging.getLogger(__name__) +#: How many member refs resolve at once when publishing a taskset. Bounded so a large grouping +#: cannot flood the entity store with simultaneous requests, but high enough that a hundred-member +#: dataset does not publish at one round trip per member. +_MEMBER_RESOLUTION_CONCURRENCY = 10 + class _TaskService(Protocol): async def get_task(self, workspace: str, name: str) -> object | None: ... + async def resolve_revision(self, workspace: str, name: str, fragment: str = "latest") -> str: ... + class TaskRefNotFoundError(ValueError): """A taskset references a task that does not exist. @@ -75,12 +102,48 @@ def _entity_to_taskset(entity: TasksetEntity) -> Taskset: project=entity.project, description=entity.description, tasks=entity.tasks, + revision=entity.latest_revision, + tags=entity.tags, metadata=entity.metadata, created_at=created_at, updated_at=updated_at, ) +def _revision_to_taskset(head: TasksetEntity, revision: TasksetRevisionEntity) -> Taskset: + """Present a published revision as the ``Taskset`` DTO — identity from the head, content from + the revision. Tags are the head's, since "what points here now" is a question about the present.""" + created_at = revision.created_at + if created_at is None: + raise ValueError(f"Revision {revision.revision} of '{head.workspace}/{head.name}' has no timestamp") + return Taskset( + id=head.id, + name=head.name, + workspace=head.workspace, + project=head.project, + description=revision.description, + tasks=revision.tasks, + metadata=revision.metadata, + revision=revision.revision, + tags={tag: ordinal for tag, ordinal in head.tags.items() if ordinal == revision.revision}, + created_at=created_at, + updated_at=revision.updated_at or created_at, + ) + + +def _entity_to_revision(revision: TasksetRevisionEntity, tags: dict[str, int]) -> Revision: + """Map a stored revision to its API DTO, attaching the tags that currently point at it.""" + created_at = revision.created_at + if created_at is None: + raise ValueError(f"Revision {revision.revision} is missing its creation timestamp") + return Revision( + revision=revision.revision, + content_hash=revision.content_hash, + tags=sorted(tag for tag, ordinal in tags.items() if ordinal == revision.revision), + created_at=created_at, + ) + + def _pagination(src: PaginationInfo, current_page_size: int) -> PaginationData: """Carry the entity-store pagination counts into the API ``Page`` envelope.""" return PaginationData( @@ -92,21 +155,35 @@ def _pagination(src: PaginationInfo, current_page_size: int) -> PaginationData: ) +class TasksetEntityStoreProtocol( + EntityClientProtocol[TasksetEntity], EntityUpdateClientProtocol[TasksetEntity], Protocol +): + """The store surface this service needs for its own taskset records. + + Composed from the shared entity-client protocols rather than a bespoke one, so the surface a + service depends on cannot drift from the client that satisfies it. + """ + + class TasksetService: """Create/get/list/delete for persisted taskset entities, exposed as the ``Taskset`` DTO.""" - def __init__(self, entity_client: NemoEntitiesClientProtocol[TasksetEntity], task_service: _TaskService): + def __init__(self, entity_client: TasksetEntityStoreProtocol, task_service: _TaskService): self.entity_client = entity_client + #: The same client, viewed at the revision type. Python has no intersection types, so a + #: single annotation cannot say "serves TasksetEntity *and* TasksetRevisionEntity" — but the + #: concrete client's methods are generic over the entity type and genuinely satisfy both. + self.revision_client: EntityClientProtocol[TasksetRevisionEntity] = cast( + EntityClientProtocol[TasksetRevisionEntity], entity_client + ) self.task_service = task_service - async def _validate_tasks_exist(self, tasks: list[TaskRef], *, workspace: str) -> None: - """Validate the member refs: each must resolve to a stored task, and no two may resolve to the - same one. + def _reject_duplicate_members(self, tasks: list[TaskRef], *, workspace: str) -> None: + """Reject two refs resolving to the same task. Pure in-memory, so it runs before any I/O. - A bare ``name`` ref resolves against the taskset's own workspace; a ``workspace/name`` ref - resolves against the named workspace. Raises :class:`DuplicateTaskRefError` if two refs point - at the same ``(workspace, name)`` and :class:`TaskRefNotFoundError` if a referenced task is - missing. + The field validator only catches byte-identical refs; this catches refs that differ in form + but resolve to the same ``(workspace, name)`` — e.g. ``task-a`` and ``default/task-a`` in + the ``default`` workspace. """ seen: set[tuple[str, str]] = set() for ref in tasks: @@ -116,41 +193,187 @@ async def _validate_tasks_exist(self, tasks: list[TaskRef], *, workspace: str) - f"Task reference '{ref.root}' resolves to '{resolved[0]}/{resolved[1]}', already in this taskset" ) seen.add(resolved) - if await self.task_service.get_task(*resolved) is None: - raise TaskRefNotFoundError(f"Task reference '{ref.root}' not found in workspace '{resolved[0]}'") + + async def _pin_member(self, ref: TaskRef, *, workspace: str) -> TaskRef: + """Resolve one member ref to ``workspace/name#``. + + Existence and revision resolution are one operation: ``resolve_revision`` already fetches + the task, so a separate existence check would just re-read the same record. + """ + ref_workspace, name, fragment = parse_subentity_ref(ref.root, workspace) + try: + digest = await self.task_service.resolve_revision(ref_workspace, name, fragment) + except NemoEntityNotFoundError as exc: + raise TaskRefNotFoundError(f"Task reference '{ref.root}' not found in workspace '{ref_workspace}'") from exc + except RevisionNotFoundError as exc: + raise TaskRefNotFoundError(f"Task reference '{ref.root}' names no published revision: {exc}") from exc + return TaskRef(f"{ref_workspace}/{name}#{digest}") + + async def _resolved_content(self, taskset_input: TasksetInput, *, workspace: str) -> list[TaskRef]: + """Validate membership and resolve it to digest-pinned refs. + + Members resolve **concurrently**, bounded by :data:`_MEMBER_RESOLUTION_CONCURRENCY`. A + Harbor-scale dataset can name hundreds of tasks, and resolving them one at a time made + publish latency linear in membership. Bounded rather than unbounded so a large taskset + cannot open hundreds of simultaneous connections to the entity store. + + Resolution happens on write because tags move: a stored ``#latest`` would silently re-point + this taskset's membership the next time that task published, and a published grouping that + changes underneath you is not a grouping. Same reason a lockfile records resolved versions, + not ranges. + """ + self._reject_duplicate_members(taskset_input.tasks, workspace=workspace) + limit = asyncio.Semaphore(_MEMBER_RESOLUTION_CONCURRENCY) + + async def _bounded(ref: TaskRef) -> TaskRef: + async with limit: + return await self._pin_member(ref, workspace=workspace) + + # ``gather`` preserves input order, so membership order survives resolution — which matters + # because a taskset's digest depends on member order. + pending = [asyncio.create_task(_bounded(ref)) for ref in taskset_input.tasks] + try: + return list(await asyncio.gather(*pending)) + except BaseException: + # ``gather`` propagates the first failure but leaves its siblings running. One bad + # member in a large grouping would otherwise keep issuing reads long after the request + # failed, and their own errors would surface as unretrieved-task warnings. + for task in pending: + task.cancel() + raise async def create_taskset( self, name: str, taskset_input: TasksetInput, *, workspace: str, project: str | None = None - ) -> Taskset: - """Store a new taskset (addressed by workspace/name). + ) -> tuple[Taskset, bool]: + """Store a new taskset and publish it as revision 1. - Raises ``ValueError`` if it already exists or if any referenced task does not exist. + Strict create: raises :class:`TasksetExistsError` if the name is taken, and + :class:`TaskRefNotFoundError` if a member does not exist or has no such revision. Returns + ``(taskset, published)``; ``published`` is always ``True`` here. """ - await self._validate_tasks_exist(taskset_input.tasks, workspace=workspace) entity = TasksetEntity( name=name, workspace=workspace, project=project, description=taskset_input.description, - tasks=taskset_input.tasks, + tasks=await self._resolved_content(taskset_input, workspace=workspace), metadata=taskset_input.metadata, ) try: created = await self.entity_client.create(entity) except NemoEntityConflictError as exc: raise TasksetExistsError(f"Taskset '{workspace}/{name}' already exists") from exc + try: + head, published = await self._publish(created, tags=set(taskset_input.tags)) + except Exception: + # A head with no revision would break the invariant consumers rely on — `#latest` + # always resolves and `revision` is never 0. No cross-entity transaction exists, so + # roll the head back rather than leave a half-created taskset behind. + logger.exception("Publishing revision 1 failed; rolling back the taskset record") + try: + await self.entity_client.delete(TasksetEntity, name=name, workspace=workspace) + except Exception: + # Report the rollback failure, but re-raise the *original* error: replacing it + # would hide why the publish failed and leave the caller debugging the cleanup. + logger.exception("Rollback of the orphaned taskset record also failed") + raise logger.info( "Taskset created", extra={"workspace": sanitize_for_log(workspace), "taskset_name": sanitize_for_log(name)}, ) - return _entity_to_taskset(created) + return _entity_to_taskset(head), published + + async def replace_taskset( + self, name: str, taskset_input: TasksetInput, *, workspace: str, project: str | None = None + ) -> tuple[Taskset, bool]: + """Replace a taskset's content and publish the result, creating it if absent. + + Note that re-submitting *identical* membership can still publish a new revision: members are + re-resolved on every write, so if a member task published since last time, ``#latest`` now + names a different digest and this grouping genuinely differs. That is the intended + behavior — the taskset's content is the exact revisions it names, not the names alone. + """ + try: + head = await self.entity_client.get(TasksetEntity, name=name, workspace=workspace) + except NemoEntityNotFoundError: + return await self.create_taskset(name, taskset_input, workspace=workspace, project=project) + + if project is not None: + # Applied rather than ignored. ``project`` is a query parameter, so an omitted value + # means "leave it alone" — only an explicit value moves the record. + head.project = project + head.description = taskset_input.description + head.tasks = await self._resolved_content(taskset_input, workspace=workspace) + head.metadata = taskset_input.metadata + # Publish the staged content *without* committing the head first — see the matching comment + # in ``TaskService.replace_task``. Publishing writes the head itself, so a pre-write would + # only open a window where a failed publish leaves the head serving uncovered content. + published_head, published = await self._publish(head, tags=set(taskset_input.tags)) + if not published: + # Publishing wrote nothing (content already published and tagged as requested), so + # persist what sits outside the digest — ``project``. + published_head = await self.entity_client.update(published_head) + logger.info( + "Taskset replaced", + extra={ + "workspace": sanitize_for_log(workspace), + "taskset_name": sanitize_for_log(name), + "published": published, + }, + ) + return _entity_to_taskset(published_head), published + + async def _publish(self, head: TasksetEntity, *, tags: set[str]) -> tuple[TasksetEntity, bool]: + """Freeze the head as a revision. The returned head already carries the new pointers.""" + _, published_head, created = await publish_revision( + self.entity_client, self.revision_client, head, TasksetRevisionEntity, tags=tags + ) + return published_head, created - async def get_taskset(self, workspace: str, name: str) -> Taskset | None: + async def list_revisions( + self, workspace: str, name: str, *, page: int = 1, page_size: int = 100 + ) -> Page[Revision] | None: + """List a taskset's published revisions, newest first; ``None`` if the taskset is absent. + + Paged rather than returning every revision: a bare list would silently truncate at the + store's page size, making a capped history indistinguishable from a complete one. + """ try: - entity = await self.entity_client.get(TasksetEntity, workspace=workspace, name=name) + head = await self.entity_client.get(TasksetEntity, name=name, workspace=workspace) except NemoEntityNotFoundError: return None - return _entity_to_taskset(entity) + result = await list_revisions(self.revision_client, TasksetRevisionEntity, head, page=page, page_size=page_size) + data = [_entity_to_revision(revision, head.tags) for revision in result.data] + return Page(data=data, pagination=_pagination(result.pagination, len(data)), sort=None, filter=None) + + async def tag_revision(self, workspace: str, name: str, tag: str, fragment: str) -> Taskset | None: + """Point a tag at an existing revision; ``None`` if the taskset is absent.""" + try: + head = await self.entity_client.get(TasksetEntity, name=name, workspace=workspace) + except NemoEntityNotFoundError: + return None + updated = await apply_tag(self.entity_client, self.revision_client, TasksetRevisionEntity, head, tag, fragment) + logger.info( + "Taskset revision tagged", + extra={"workspace": sanitize_for_log(workspace), "taskset_name": sanitize_for_log(name)}, + ) + return _entity_to_taskset(updated) + + async def get_taskset(self, workspace: str, name: str, revision: str | None = None) -> Taskset | None: + """Get a stored taskset; ``None`` if absent. + + ``revision`` is a tag or content digest — what a ref's ``#fragment`` carries — so a consumer + holding a pinned reference reads the membership that was published, not what is current. + """ + try: + head = await self.entity_client.get(TasksetEntity, name=name, workspace=workspace) + except NemoEntityNotFoundError: + return None + if revision is None: + return _entity_to_taskset(head) + return _revision_to_taskset( + head, await get_revision(self.revision_client, TasksetRevisionEntity, head, revision) + ) async def list_tasksets( self, @@ -175,7 +398,7 @@ async def list_tasksets( async def delete_taskset(self, workspace: str, name: str) -> bool: """Delete a stored taskset; ``False`` if absent.""" try: - await self.entity_client.delete(TasksetEntity, name, workspace=workspace) + await self.entity_client.delete(TasksetEntity, name=name, workspace=workspace) except NemoEntityNotFoundError: return False logger.info( diff --git a/plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.py b/plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.py index a07bd3c4c3..194de74c0d 100644 --- a/plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.py +++ b/plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.py @@ -8,12 +8,13 @@ import logging from typing import Annotated -from fastapi import APIRouter, Depends, HTTPException, Path, Query, status +from fastapi import APIRouter, Depends, HTTPException, Path, Query, Response, status from nemo_evaluator.api.dependencies import get_task_service -from nemo_evaluator.api.schemas import Task, TaskFilter, TaskInput, TaskSort +from nemo_evaluator.api.schemas import Revision, Task, TaskFilter, TaskInput, TaskSort from nemo_evaluator.api.service.task_service import MetricRefNotFoundError, TaskService from nemo_evaluator.authz import scope from nemo_evaluator.entities import MAX_NAME_LENGTH, NAME_PATTERN +from nemo_evaluator.revisions import RevisionConflictError, RevisionNotFoundError from nemo_platform_plugin.api.parsed_filter import ParsedFilter, make_filter_dep from nemo_platform_plugin.authz import CallerKind, PermissionSet, path_rule, perm from nemo_platform_plugin.entities import EntityValidationError @@ -94,12 +95,17 @@ async def create_task( project: str | None = Query(default=None, description="Optional project to associate with the task."), service: TaskService = Depends(get_task_service), ) -> Task: - """Store a new task, addressed by workspace/name.""" + """Store a new task, addressed by workspace/name, and publish it as revision 1. + + Strict create — 409 if the name is taken. Use ``PUT`` to publish a further revision of an + existing task. + """ safe_workspace = sanitize_for_log(workspace) safe_name = sanitize_for_log(name) logger.info(f"Creating task: {safe_workspace}/{safe_name}") try: - return await service.create_task(name, task, workspace=workspace, project=project) + created, _ = await service.create_task(name, task, workspace=workspace, project=project) + return created except EntityValidationError as e: logger.warning(f"Entity store validation error during task creation: {e}") raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(e)) @@ -122,6 +128,169 @@ async def create_task( raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Internal server error") +@router.put( + "/tasks/{name}", + summary="Replace Task", + response_description="Publish a revision of the task", + status_code=status.HTTP_200_OK, + responses={ + status.HTTP_200_OK: {"description": "Content already published; no new revision was cut"}, + status.HTTP_201_CREATED: {"model": Task, "description": "A new revision was published"}, + status.HTTP_409_CONFLICT: {"description": "Concurrent write; refresh and retry"}, + }, +) +@scope.write +@path_rule(callers=[CallerKind.PRINCIPAL], permissions=[TaskPerms.CREATE]) +async def replace_task( + workspace: str, + name: Annotated[str, Path(max_length=MAX_NAME_LENGTH, pattern=NAME_PATTERN)], + task: TaskInput, + response: Response, + project: str | None = Query(default=None, description="Optional project to associate with the task."), + service: TaskService = Depends(get_task_service), +) -> Task: + """Replace a task's content and publish the result, creating the task if it does not exist. + + Upsert rather than 404-on-missing so a publisher can issue one idempotent call without first + checking existence — that check is both an extra round trip and a race between two publishers. + + Idempotent in the strict sense: submitting content that matches the current revision publishes + nothing and returns **200**, while a genuine change returns **201**. Tags in the body are + applied either way, so re-PUTting unchanged content is how you tag an existing revision. + """ + safe_workspace = sanitize_for_log(workspace) + safe_name = sanitize_for_log(name) + logger.info(f"Replacing task: {safe_workspace}/{safe_name}") + try: + replaced, published = await service.replace_task(name, task, workspace=workspace, project=project) + except EntityValidationError as e: + logger.warning(f"Entity store validation error during task replace: {e}") + raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(e)) + except MetricRefNotFoundError as e: + logger.warning(f"Task has an invalid metric reference: {e}") + raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(e)) + except RevisionConflictError as e: + logger.warning(f"Task revision allocation contended: {e}") + raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=str(e)) + except NemoEntityConflictError as e: + # Another request updated this record between our read and our write. A retry against the + # current state is the caller's move; a 500 would wrongly suggest a server fault. + logger.warning(f"Task modified concurrently during replace: {e}") + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=f"Task was modified by another request: {workspace}/{name}. Refresh and try again.", + ) + except ValueError as e: + logger.warning(f"Task replace validation error: {e}") + raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="Invalid task data") + except HTTPException: + raise + except Exception: + logger.exception("Failed to replace task") + raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Internal server error") + response.status_code = status.HTTP_201_CREATED if published else status.HTTP_200_OK + return replaced + + +@router.get( + "/tasks/{name}/revisions", + summary="List Task Revisions", + response_description="Return the task's published revisions, newest first", + status_code=status.HTTP_200_OK, + responses={status.HTTP_404_NOT_FOUND: {"description": "Task not found"}}, +) +@scope.read +@path_rule(callers=[CallerKind.PRINCIPAL], permissions=[TaskPerms.READ]) +async def list_task_revisions( + workspace: str, + name: Annotated[str, Path(max_length=MAX_NAME_LENGTH, pattern=NAME_PATTERN)], + page: int = Query(default=1, ge=1, description="Page number."), + page_size: int = Query(default=100, ge=1, le=1000, description="Page size."), + service: TaskService = Depends(get_task_service), +) -> Page[Revision]: + """List a task's published revisions. + + This is how a caller discovers what it can pin to: each entry carries the ``content_hash`` that + goes in a reference's ``#fragment``. + """ + try: + revisions = await service.list_revisions(workspace, name, page=page, page_size=page_size) + except Exception: + logger.exception("Failed to list task revisions") + raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Internal server error") + if revisions is None: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=f"Task '{name}' not found") + return revisions + + +@router.get( + "/tasks/{name}/revisions/{revision}", + summary="Get Task Revision", + response_description="Return the task's content as of a published revision", + status_code=status.HTTP_200_OK, + responses={status.HTTP_404_NOT_FOUND: {"description": "Task or revision not found"}}, +) +@scope.read +@path_rule(callers=[CallerKind.PRINCIPAL], permissions=[TaskPerms.READ]) +async def get_task_revision( + workspace: str, + name: Annotated[str, Path(max_length=MAX_NAME_LENGTH, pattern=NAME_PATTERN)], + revision: Annotated[str, Path(max_length=MAX_NAME_LENGTH, pattern=NAME_PATTERN)], + service: TaskService = Depends(get_task_service), +) -> Task: + """Get a task's content as of a published revision. + + ``revision`` is a content digest or a tag — exactly what a reference's ``#fragment`` carries, + so a consumer holding ``workspace/task-a#`` reads what was published rather than + whatever is current. Returns the full task DTO; the collection route above is a thin index of + what can be pinned to. + """ + try: + task = await service.get_task(workspace, name, revision) + except RevisionNotFoundError as e: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(e)) + if task is None: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=f"Task not found: {workspace}/{name}") + return task + + +@router.put( + "/tasks/{name}/tags/{tag}", + summary="Tag Task Revision", + response_description="Point a tag at an existing revision", + status_code=status.HTTP_200_OK, + responses={ + status.HTTP_404_NOT_FOUND: {"description": "Task or revision not found"}, + status.HTTP_422_UNPROCESSABLE_ENTITY: {"description": "Tag is reserved"}, + }, +) +@scope.write +@path_rule(callers=[CallerKind.PRINCIPAL], permissions=[TaskPerms.CREATE]) +async def tag_task_revision( + workspace: str, + name: Annotated[str, Path(max_length=MAX_NAME_LENGTH, pattern=NAME_PATTERN)], + tag: Annotated[str, Path(max_length=MAX_NAME_LENGTH, pattern=NAME_PATTERN)], + revision: str = Query(description="Revision to tag — a content digest, or another tag."), + service: TaskService = Depends(get_task_service), +) -> Task: + """Point a tag at an already-published revision. + + Separate from publishing because blessing a revision usually happens *after* it has been + evaluated. ``latest`` is reserved: it is machine-managed, always names the most recent publish, + and moving it by hand would break the forward-only guarantee that keeps concurrent publishes + consistent. + """ + try: + tagged = await service.tag_revision(workspace, name, tag, revision) + except ValueError as e: + raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(e)) + except RevisionNotFoundError as e: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(e)) + if tagged is None: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=f"Task '{name}' not found") + return tagged + + @router.get( "/tasks/{name}", summary="Get Task", @@ -136,7 +305,10 @@ async def get_task( name: str, service: TaskService = Depends(get_task_service), ) -> Task: - """Get a stored task by workspace and name.""" + """Get a stored task's current content. + + Use ``GET /tasks/{name}/revisions/{revision}`` to read it as of a specific published revision. + """ logger.debug(f"Getting task: {sanitize_for_log(workspace)}/{sanitize_for_log(name)}") try: task = await service.get_task(workspace, name) diff --git a/plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasksets.py b/plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasksets.py index 697e3dbc43..a697b9cdcc 100644 --- a/plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasksets.py +++ b/plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasksets.py @@ -8,9 +8,9 @@ import logging from typing import Annotated -from fastapi import APIRouter, Depends, HTTPException, Path, Query, status +from fastapi import APIRouter, Depends, HTTPException, Path, Query, Response, status from nemo_evaluator.api.dependencies import get_taskset_service -from nemo_evaluator.api.schemas import Taskset, TasksetFilter, TasksetInput, TasksetSort +from nemo_evaluator.api.schemas import Revision, Taskset, TasksetFilter, TasksetInput, TasksetSort from nemo_evaluator.api.service.taskset_service import ( DuplicateTaskRefError, TaskRefNotFoundError, @@ -19,6 +19,7 @@ ) from nemo_evaluator.authz import scope from nemo_evaluator.entities import MAX_NAME_LENGTH, NAME_PATTERN +from nemo_evaluator.revisions import RevisionConflictError, RevisionNotFoundError from nemo_platform_plugin.api.parsed_filter import ParsedFilter, make_filter_dep from nemo_platform_plugin.authz import CallerKind, PermissionSet, path_rule, perm from nemo_platform_plugin.entities import EntityValidationError @@ -99,12 +100,16 @@ async def create_taskset( project: str | None = Query(default=None, description="Optional project to associate with the taskset."), service: TasksetService = Depends(get_taskset_service), ) -> Taskset: - """Store a new taskset, addressed by workspace/name.""" + """Store a new taskset, addressed by workspace/name, and publish it as revision 1. + + Strict create — 409 if the name is taken. Use ``PUT`` to publish a further revision. + """ safe_workspace = sanitize_for_log(workspace) safe_name = sanitize_for_log(name) logger.info(f"Creating taskset: {safe_workspace}/{safe_name}") try: - return await service.create_taskset(name, taskset, workspace=workspace, project=project) + created, _ = await service.create_taskset(name, taskset, workspace=workspace, project=project) + return created except EntityValidationError as e: logger.warning(f"Entity store validation error during taskset creation: {e}") raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(e)) @@ -128,6 +133,156 @@ async def create_taskset( raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Internal server error") +@router.put( + "/tasksets/{name}", + summary="Replace Taskset", + response_description="Publish a revision of the taskset", + status_code=status.HTTP_200_OK, + responses={ + status.HTTP_200_OK: {"description": "Content already published; no new revision was cut"}, + status.HTTP_201_CREATED: {"model": Taskset, "description": "A new revision was published"}, + status.HTTP_409_CONFLICT: {"description": "Concurrent write; refresh and retry"}, + }, +) +@scope.write +@path_rule(callers=[CallerKind.PRINCIPAL], permissions=[TasksetPerms.CREATE]) +async def replace_taskset( + workspace: str, + name: Annotated[str, Path(max_length=MAX_NAME_LENGTH, pattern=NAME_PATTERN)], + taskset: TasksetInput, + response: Response, + project: str | None = Query(default=None, description="Optional project to associate with the taskset."), + service: TasksetService = Depends(get_taskset_service), +) -> Taskset: + """Replace a taskset's membership and publish the result, creating it if it does not exist. + + Members are re-resolved to exact revision digests on every write, so re-submitting the *same* + member names can still publish a new revision — if a member task published in the meantime, + this grouping now names different content and genuinely differs. A taskset's identity is the + exact revisions it names, not the names alone. + """ + safe_workspace = sanitize_for_log(workspace) + safe_name = sanitize_for_log(name) + logger.info(f"Replacing taskset: {safe_workspace}/{safe_name}") + try: + replaced, published = await service.replace_taskset(name, taskset, workspace=workspace, project=project) + except EntityValidationError as e: + logger.warning(f"Entity store validation error during taskset replace: {e}") + raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(e)) + except (TaskRefNotFoundError, DuplicateTaskRefError) as e: + logger.warning(f"Taskset has an invalid task reference: {e}") + raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(e)) + except RevisionConflictError as e: + logger.warning(f"Taskset revision allocation contended: {e}") + raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=str(e)) + except NemoEntityConflictError as e: + # Another request updated this record between our read and our write. A retry against the + # current state is the caller's move; a 500 would wrongly suggest a server fault. + logger.warning(f"Taskset modified concurrently during replace: {e}") + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=f"Taskset was modified by another request: {workspace}/{name}. Refresh and try again.", + ) + except ValueError as e: + logger.warning(f"Taskset replace validation error: {e}") + raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="Invalid taskset data") + except HTTPException: + raise + except Exception: + logger.exception("Failed to replace taskset") + raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Internal server error") + response.status_code = status.HTTP_201_CREATED if published else status.HTTP_200_OK + return replaced + + +@router.get( + "/tasksets/{name}/revisions", + summary="List Taskset Revisions", + response_description="Return the taskset's published revisions, newest first", + status_code=status.HTTP_200_OK, + responses={status.HTTP_404_NOT_FOUND: {"description": "Taskset not found"}}, +) +@scope.read +@path_rule(callers=[CallerKind.PRINCIPAL], permissions=[TasksetPerms.READ]) +async def list_taskset_revisions( + workspace: str, + name: Annotated[str, Path(max_length=MAX_NAME_LENGTH, pattern=NAME_PATTERN)], + page: int = Query(default=1, ge=1, description="Page number."), + page_size: int = Query(default=100, ge=1, le=1000, description="Page size."), + service: TasksetService = Depends(get_taskset_service), +) -> Page[Revision]: + """List a taskset's published revisions — each entry's ``content_hash`` is what a pinned + reference carries.""" + try: + revisions = await service.list_revisions(workspace, name, page=page, page_size=page_size) + except Exception: + logger.exception("Failed to list taskset revisions") + raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Internal server error") + if revisions is None: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=f"Taskset '{name}' not found") + return revisions + + +@router.get( + "/tasksets/{name}/revisions/{revision}", + summary="Get Taskset Revision", + response_description="Return the taskset's membership as of a published revision", + status_code=status.HTTP_200_OK, + responses={status.HTTP_404_NOT_FOUND: {"description": "Taskset or revision not found"}}, +) +@scope.read +@path_rule(callers=[CallerKind.PRINCIPAL], permissions=[TasksetPerms.READ]) +async def get_taskset_revision( + workspace: str, + name: Annotated[str, Path(max_length=MAX_NAME_LENGTH, pattern=NAME_PATTERN)], + revision: Annotated[str, Path(max_length=MAX_NAME_LENGTH, pattern=NAME_PATTERN)], + service: TasksetService = Depends(get_taskset_service), +) -> Taskset: + """Get a taskset's membership as of a published revision. + + ``revision`` is a content digest or a tag — what a reference's ``#fragment`` carries — so a + pinned consumer reads the exact grouping that was published. + """ + try: + taskset = await service.get_taskset(workspace, name, revision) + except RevisionNotFoundError as e: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(e)) + if taskset is None: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=f"Taskset not found: {workspace}/{name}") + return taskset + + +@router.put( + "/tasksets/{name}/tags/{tag}", + summary="Tag Taskset Revision", + response_description="Point a tag at an existing revision", + status_code=status.HTTP_200_OK, + responses={ + status.HTTP_404_NOT_FOUND: {"description": "Taskset or revision not found"}, + status.HTTP_422_UNPROCESSABLE_ENTITY: {"description": "Tag is reserved"}, + }, +) +@scope.write +@path_rule(callers=[CallerKind.PRINCIPAL], permissions=[TasksetPerms.CREATE]) +async def tag_taskset_revision( + workspace: str, + name: Annotated[str, Path(max_length=MAX_NAME_LENGTH, pattern=NAME_PATTERN)], + tag: Annotated[str, Path(max_length=MAX_NAME_LENGTH, pattern=NAME_PATTERN)], + revision: str = Query(description="Revision to tag — a content digest, or another tag."), + service: TasksetService = Depends(get_taskset_service), +) -> Taskset: + """Point a tag at an already-published revision. ``latest`` is reserved and machine-managed.""" + try: + tagged = await service.tag_revision(workspace, name, tag, revision) + except ValueError as e: + raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(e)) + except RevisionNotFoundError as e: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(e)) + if tagged is None: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=f"Taskset '{name}' not found") + return tagged + + @router.get( "/tasksets/{name}", summary="Get Taskset", @@ -142,7 +297,10 @@ async def get_taskset( name: str, service: TasksetService = Depends(get_taskset_service), ) -> Taskset: - """Get a stored taskset by workspace and name.""" + """Get a stored taskset's current membership. + + Use ``GET /tasksets/{name}/revisions/{revision}`` to read it as of a published revision. + """ logger.debug(f"Getting taskset: {sanitize_for_log(workspace)}/{sanitize_for_log(name)}") # Only the service call can fail unexpectedly; wrap just that so the 404 below is raised outside # the try (no catching HTTPException only to re-raise it). diff --git a/plugins/nemo-evaluator/src/nemo_evaluator/content_hash.py b/plugins/nemo-evaluator/src/nemo_evaluator/content_hash.py new file mode 100644 index 0000000000..78dd55e9b8 --- /dev/null +++ b/plugins/nemo-evaluator/src/nemo_evaluator/content_hash.py @@ -0,0 +1,91 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Canonical content hashing for revisioned entities (tasks and tasksets). + +A revision is addressed by a digest of its *content* — the same projection the entity store +persists (``model_dump(exclude=__base_fields__)``), serialized canonically and hashed with SHA-256. +Two independent computations of the same content must agree, because a consumer reading a pinned +ref (``workspace/task-name#``) recomputes the digest and compares. + +Three properties this deliberately has: + +**Content only, never identity.** The name, workspace, and revision index are *not* inputs. Salting +the digest with them would break the two things it exists for: republishing identical content would +produce a new digest (defeating publish-time dedup), and verify-on-read would pass whenever +identity matched, regardless of whether the content beneath it had changed. Identity is carried by +the ref, which already names the entity; a cross-entity digest collision can't cause a +misresolution because lookups are scoped by name before the digest is consulted. + +**Full digest, never truncated.** SHA-256's collision resistance is bounded at 2**128 by the +birthday paradox, which is ample. A truncated prefix is not: 12 hex chars is 48 bits, with a +birthday bound near 2**24 — reachable by accident at scale. Short forms belong in display and +prefix *matching* against stored full digests (git's model), never in the stored value. + +**Bare hex, no algorithm prefix.** Matches the existing derived-metric digest idiom in +``metric_service`` and keeps the ``#`` ref fragment free of ``:``, which the entity ref charset +does not admit. See ``docs`` in the backend design for the algorithm-agility tradeoff. + +The weak link is not SHA-256 — it is *canonicalization*. If two semantically different revisions +serialize identically they collide with probability 1 and no hash strength helps. Hence the +explicit rules below, and the near-miss tests that pin them. +""" + +from __future__ import annotations + +import hashlib +import json +from collections.abc import Set + +from nemo_platform_plugin.entities import EntityBase + +#: Length of a SHA-256 digest rendered as lowercase hex. +DIGEST_LENGTH = 64 + +#: Charset/shape of a digest as it appears in a ``#`` ref fragment. +DIGEST_PATTERN = r"^[0-9a-f]{64}$" + + +def canonical_payload(entity: EntityBase, *, exclude: Set[str] | None = None) -> str: + """Return the canonical JSON serialization that :func:`content_hash` digests. + + Exposed separately because it is the actual compatibility contract: if this string changes + shape for unchanged content, every stored digest is invalidated. Tests assert on it directly. + + Canonicalization rules: + + - Server-owned fields are dropped via ``EntityBase.__base_fields__`` — the same exclusion the + entity store itself uses when persisting custom fields, so the hash input tracks what is + actually stored rather than a parallel hand-maintained list. + - ``mode="json"`` normalizes rich types (datetimes, enums, sub-models) to JSON primitives. + - ``sort_keys=True`` makes mapping order irrelevant. + - ``separators=(",", ":")`` removes insignificant whitespace. + + Rules this does NOT impose, deliberately: + + - **Sequence order is significant.** A list that is semantically a set must be normalized by + its own model (as ``TaskRefList`` does) before hashing; this function will not reorder it, + because it cannot tell a set from an ordered sequence. + - **``1`` and ``1.0`` hash differently**, per JSON. That is desired: an int and a float are + distinguishable values, not the same value formatted twice. + - **Absent and default-valued fields collapse**, because ``model_dump`` materializes defaults. + A model that needs to distinguish "unset" from "set to the default" must model that + explicitly (e.g. with an optional field defaulting to ``None``). + + Args: + entity: The entity whose content to serialize. + 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. + """ + excluded = set(entity.__base_fields__) | set(exclude or ()) + payload = entity.model_dump(exclude=excluded, exclude_computed_fields=True, mode="json") + return json.dumps(payload, sort_keys=True, separators=(",", ":")) + + +def content_hash(entity: EntityBase, *, exclude: Set[str] | None = None) -> str: + """Return the full 64-char lowercase hex SHA-256 digest of an entity's content. + + See :func:`canonical_payload` for what is and is not included. + """ + return hashlib.sha256(canonical_payload(entity, exclude=exclude).encode("utf-8")).hexdigest() diff --git a/plugins/nemo-evaluator/src/nemo_evaluator/entities.py b/plugins/nemo-evaluator/src/nemo_evaluator/entities.py index c91d5d9f48..cd2404db77 100644 --- a/plugins/nemo-evaluator/src/nemo_evaluator/entities.py +++ b/plugins/nemo-evaluator/src/nemo_evaluator/entities.py @@ -19,7 +19,14 @@ from typing import ClassVar -from nemo_evaluator.api.schemas import MetricRef, TaskInputs, TaskMetadataList, TaskRefList +from nemo_evaluator.api.schemas import ( + MetricRef, + PinnedTaskRefList, + TaskInputs, + TaskMetadataList, + TaskRefList, +) +from nemo_evaluator.content_hash import DIGEST_LENGTH, DIGEST_PATTERN from nemo_evaluator.shared.metric_bundles.bundles import BundledMetricOutputSpec from nemo_evaluator_sdk.agent_eval.tasks import SemanticView from nemo_evaluator_sdk.values.common import SecretRef @@ -34,6 +41,17 @@ MAX_DESCRIPTION_LENGTH = 1000 NAME_PATTERN = r"^[\w\-\.]+$" +#: Fields on a publishable record that are revision *bookkeeping*, not content. Excluded when +#: digesting a head record so that hashing the head yields the same digest as hashing the +#: corresponding revision. Two things depend on that equality: a publish recognizing "the head +#: already matches the current revision", and a read re-hashing a revision to check it against the +#: digest stored beside it. +REVISION_POINTER_FIELDS = frozenset({"latest_revision", "tags"}) + +#: The mirror of :data:`REVISION_POINTER_FIELDS` on a revision record. A revision's digest covers +#: neither itself nor the ordinal that was assigned because of it. +REVISION_SELF_FIELDS = frozenset({"content_hash", "revision"}) + class MetricBundleEntity(EntityBase): """Persisted index for a stored metric, addressed by workspace/name. @@ -164,7 +182,33 @@ class EvaluateResultEntity(_EvalResultCommon, EntityBase): ) -class TaskEntity(EntityBase): +class _RevisionedCommon(BaseModel): + """Mutable revision bookkeeping carried by a record that can be published. + + Both fields are pointers into the record's immutable revision children, and both are excluded + from the content digest — they describe *which* content is current, not what the content is. + Including them would make the digest change whenever a tag moved. + """ + + latest_revision: int = Field( + default=0, + description="Ordinal of the most recently published revision; 0 before the first publish. " + "The next publish allocates ``latest_revision + 1`` under the record's optimistic lock, so a " + "concurrent publisher that raced loses with a conflict and retries.", + ge=0, + ) + tags: dict[str, int] = Field( + default_factory=dict, + description="Mutable tag → revision-ordinal pointers. ``latest`` is reserved and re-applied " + "on every publish; other tags are user-supplied and may be moved after the fact. Tags are " + "resolution *inputs* only — anything persisted (e.g. a published taskset's membership) " + "stores the resolved content digest, never the tag, so a moved tag cannot re-point it. " + "Ordinals rather than digests because a revision's identity within its parent is its " + "ordinal, which makes tag resolution a direct child lookup with no query.", + ) + + +class TaskEntity(_RevisionedCommon, EntityBase): """Persisted, queryable agent-eval task, addressed by workspace/name. Maps to the SDK :class:`~nemo_evaluator_sdk.agent_eval.tasks.AgentEvalTask`: the task's stable @@ -189,7 +233,7 @@ class TaskEntity(EntityBase): metadata: TaskMetadataList = Field(default_factory=list, description="Key/value annotations for the task.") -class TasksetEntity(EntityBase): +class TasksetEntity(_RevisionedCommon, EntityBase): """Persisted, queryable taskset, addressed by workspace/name. A taskset is a flexible grouping of stored tasks: it holds references to its members @@ -209,3 +253,104 @@ class TasksetEntity(EntityBase): description="References to the member tasks (set semantics; duplicates rejected).", ) metadata: TaskMetadataList = Field(default_factory=list, description="Key/value annotations for the taskset.") + + +# --- Revisions --------------------------------------------------------------- +# +# A revision is an immutable snapshot of a task's or taskset's content, addressed by a digest of +# that content (see ``nemo_evaluator.content_hash``). Revisions exist so a reference can be *pinned* +# — ``workspace/task-name#`` resolves to exactly the content that was published, and a +# consumer re-derives the digest on read to confirm it. +# +# Persistence shape. A revision is a **child entity** of the record it snapshots, so entity-store +# parent-scoped uniqueness — unique within ``(workspace, entity_type, parent, name)`` — makes an +# ordinal collide rather than silently duplicate. Allocation of the next ordinal rides on the +# parent's ``db_version`` optimistic lock: bump ``latest_revision`` on the parent, and a concurrent +# publisher that raced loses with a conflict and retries. +# +# Naming. Revisions are named ``rev.``, NOT by their digest. Entity names are capped at 63 chars +# and must start with a lowercase letter (``entity_naming.NAME_PATTERN``); a full 64-char hex digest +# violates both. Truncating it to fit — as derived metric names do — would shrink the collision +# bound for no benefit here, since a ref carries the digest in its ``#`` fragment (governed by the +# ref pattern, not the entity-name rules) and the digest lives on the record as an ordinary field +# with no length pressure. Resolving a pinned ref is a filtered lookup on ``content_hash`` scoped to +# the parent. As a bonus, ``rev.7`` is legible in a log line in a way a hex string is not. +# +# Head vs history. The parent record keeps its content fields as the *head* — the current version — +# and revisions accumulate alongside. This keeps the change additive: existing stored records stay +# valid, ``get_task`` and the ``Task`` DTO are unchanged, and nothing needs migrating. The cost is a +# denormalized copy: head and its corresponding revision can drift if a write fails between the two +# (there is no cross-entity transaction). Publishing reconciles — it hashes the head and creates the +# revision only if no revision carries that digest — so a torn write self-heals on the next publish +# rather than persisting a lie. + + +class _RevisionCommon(BaseModel): + """Fields shared by every revision record: its digest and its ordinal. + + A mixin rather than an ``EntityBase`` so each concrete revision type declares its own + ``__entity_type__``, matching the ``_EvalResultCommon`` split above. + + Both fields are excluded from the content digest — a revision's digest cannot cover the ordinal + that was assigned *because of* that digest, nor cover itself. See + ``content_hash.content_hash(..., exclude=...)``. + """ + + content_hash: str = Field( + description="Full 64-char lowercase hex SHA-256 of this revision's content. Never truncated: " + "a shortened digest collapses the birthday bound, and there is no length pressure here " + "because this is a field, not an entity name.", + min_length=DIGEST_LENGTH, + max_length=DIGEST_LENGTH, + pattern=DIGEST_PATTERN, + ) + revision: int = Field( + description="Monotonic 1-based ordinal within the parent record. Matches the ``rev.`` " + "entity name; carried as a field too so it can be sorted and filtered on directly.", + ge=1, + ) + + +class TaskRevisionEntity(_RevisionCommon, EntityBase): + """An immutable published snapshot of a :class:`TaskEntity`'s content. + + Child of the task it snapshots. Content fields mirror ``TaskEntity`` exactly — a revision is + that content frozen, not a different shape. + """ + + __entity_type__: ClassVar[str] = "task_revision" + + intent: str = Field(description="Human-readable description of the desired agent behavior.") + inputs: TaskInputs = Field(default_factory=TaskInputs, description="The task's recognized input fields.") + metrics: list[MetricRef] = Field( + default_factory=list, + description="References to the metrics that score this task, as of this revision.", + ) + views: dict[str, SemanticView] = Field( + default_factory=dict, + description="Reporting views mapping this task's metric outputs into named semantic scores.", + ) + metadata: TaskMetadataList = Field(default_factory=list, description="Key/value annotations for the task.") + + +class TasksetRevisionEntity(_RevisionCommon, EntityBase): + """An immutable published snapshot of a :class:`TasksetEntity`'s content. + + Child of the taskset it snapshots. Its ``tasks`` are **fully pinned** — every member ref carries + a ``#`` fragment resolved at publish time. A stored revision must never retain a tag + fragment (``#latest``, ``#some-tag``): tags move, and a moved tag would silently re-point a + published taskset's membership, which is the failure this whole design exists to prevent. + """ + + __entity_type__: ClassVar[str] = "taskset_revision" + + description: str | None = Field( + default=None, + description="Human-readable description of the grouping.", + max_length=MAX_DESCRIPTION_LENGTH, + ) + tasks: PinnedTaskRefList = Field( + default_factory=list, + description="Digest-pinned references to the member tasks, resolved at publish time.", + ) + metadata: TaskMetadataList = Field(default_factory=list, description="Key/value annotations for the taskset.") diff --git a/plugins/nemo-evaluator/src/nemo_evaluator/revisions.py b/plugins/nemo-evaluator/src/nemo_evaluator/revisions.py new file mode 100644 index 0000000000..624b30e9d1 --- /dev/null +++ b/plugins/nemo-evaluator/src/nemo_evaluator/revisions.py @@ -0,0 +1,493 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Publishing and resolving revisions of tasks and tasksets. + +A *head* record (``TaskEntity`` / ``TasksetEntity``) holds the current content plus revision +bookkeeping. Publishing freezes that content into an immutable *revision* child +(``TaskRevisionEntity`` / ``TasksetRevisionEntity``) addressed by a digest of the content, and +points the requested tags at it. Resolution goes the other way: a ref's ``#fragment`` — a tag or a +digest — becomes a concrete revision. + +Two ways to address a revision, two lookup costs. A **tag** is a mutable pointer stored on the head +as ``tag → ordinal``, so resolving one is a direct child lookup with no query — which matters +because ``#latest`` is the common path. A **digest** is content-addressed and resolved by querying +the children on ``(parent, content_hash)``. Nothing is denormalized: the head stores only pointers, +never a copy of what the children already say. + +Concurrency. Two publishers racing on the same record both compute ordinal N; the entity store's +parent-scoped uniqueness — unique within ``(workspace, entity_type, parent, name)`` — makes the +second ``rev.N`` create conflict rather than silently duplicate. The loser refreshes its allocation +state and retries against N+1, keeping the content it was asked to publish. The child create, not +the head update, is the serialization point, so a revision is never allocated twice even if the +head update is slow. +""" + +from __future__ import annotations + +import logging +import re +from typing import Protocol, TypeVar + +from nemo_evaluator.api.schemas import LATEST_TAG, REF_FRAGMENT_CHARSET +from nemo_evaluator.content_hash import DIGEST_PATTERN, content_hash +from nemo_evaluator.entities import ( + REVISION_POINTER_FIELDS, + REVISION_SELF_FIELDS, + TaskEntity, + TaskRevisionEntity, + TasksetEntity, + TasksetRevisionEntity, +) +from nemo_platform_plugin.entities import ( + EntityBase, + EntityClientProtocol, + EntityGetterProtocol, + EntityUpdateClientProtocol, + ListResponse, +) +from nemo_platform_plugin.entity_client import ( + NemoEntityConflictError, + NemoEntityNotFoundError, +) +from nemo_platform_plugin.filter_ops import ComparisonOperation, FilterOperator, LogicalOperation + +logger = logging.getLogger(__name__) + +#: Attempts to allocate an ordinal before giving up. Each retry costs one head re-read; contention +#: on a single task is expected to be rare (publishes are human- or pipeline-paced), so a small +#: bound is enough to absorb a race without masking a genuine, persistent conflict. +_MAX_ALLOCATION_ATTEMPTS = 5 + +#: Prefix for revision entity names. Entity names must start with a lowercase letter and cap at 63 +#: chars, so a revision is ``rev.`` — a bare ordinal is not a legal name, and the full +#: 64-char digest is both too long and usually digit-leading. +_REVISION_NAME_PREFIX = "rev." + +#: Shape of a content digest in a ref fragment — what distinguishes a digest from a tag name. +_DIGEST_FRAGMENT = re.compile(DIGEST_PATTERN) + +#: A tag has to be usable as a ref ``#fragment``, so it is bound by the same charset. Sharing the +#: constant keeps the two from drifting into a state where a tag is mintable but unreferenceable. +_TAG_NAME = re.compile(REF_FRAGMENT_CHARSET) + +HeadT = TypeVar("HeadT", TaskEntity, TasksetEntity) +RevisionT = TypeVar("RevisionT", TaskRevisionEntity, TasksetRevisionEntity) +_EntityT = TypeVar("_EntityT", bound=EntityBase) + + +class HeadStoreProtocol(EntityGetterProtocol[_EntityT], EntityUpdateClientProtocol[_EntityT], Protocol[_EntityT]): + """The head-record surface: read it, then write its pointers under the optimistic lock. + + Composed from the shared protocols rather than restated — ``EntityUpdateClientProtocol`` is + deliberately separate from the CRUD one precisely so callers needing ``update`` can add it. + """ + + +#: The revision-child surface: create a revision, fetch one by ``(name, parent)``, query by digest. +#: The shared CRUD protocol covers all three, parameterised on the revision type rather than erased +#: to "any entity", so a wrong entity type at a call site is a type error rather than a cast. +RevisionStoreProtocol = EntityClientProtocol + + +class RevisionNotFoundError(LookupError): + """A ref names a revision — by tag or digest — that the record does not have.""" + + +class RevisionConflictError(RuntimeError): + """Ordinal allocation lost too many races to concurrent publishers.""" + + +class RevisionContentMismatchError(RuntimeError): + """A stored revision's content no longer hashes to the digest recorded with it. + + Corruption rather than absence: the revision resolved, but serving it would break the promise + a pinned reference makes, so it is refused instead. + """ + + +def revision_name(ordinal: int) -> str: + """Entity name for a revision ordinal.""" + return f"{_REVISION_NAME_PREFIX}{ordinal}" + + +def head_digest(head: EntityBase) -> str: + """Digest of a head record's content, excluding its revision bookkeeping. + + The exclusion is what makes this comparable to a revision's own digest: pointers describe + *which* content is current, not what the content is. + """ + return content_hash(head, exclude=REVISION_POINTER_FIELDS) + + +def validate_tag_name(tag: str) -> str: + """Reject tag names that could be stored but never resolved. + + - **Empty**: an absent fragment already means ``latest``, so an empty tag is unaddressable. + - **Outside the fragment charset** (a slash, a space): a ref's ``#fragment`` admits only + ``[\\w\\-.]+``, so such a tag could be applied and listed but never written into a member + reference — the one thing tags exist for. + - **Digest-shaped** (64 lowercase hex): resolution checks the digest form *first*, so such a + tag would be queried as a content hash and the tag map never consulted. + + Each is silently useless rather than obviously wrong, which is why they are refused here + rather than left to surprise someone later. ``latest`` is *not* rejected: whether it is + acceptable depends on the operation, so that check lives with the caller. + """ + if not tag.strip(): + raise ValueError("tag name must not be empty") + if not _TAG_NAME.fullmatch(tag): + raise ValueError( + f"tag {tag!r} contains characters that cannot appear in a reference fragment; such a " + "tag could be stored but never used to pin a member, because a ref's '#fragment' " + "admits only letters, digits, underscores, hyphens and dots" + ) + if is_digest(tag): + raise ValueError( + f"tag {tag!r} looks like a content digest; such a tag could be stored but never " + "resolved, because a digest-shaped reference is looked up as a digest, not as a tag" + ) + return tag + + +def validate_movable_tag(tag: str) -> str: + """Validate a tag a caller is asking to *point somewhere* by hand. + + Adds the ``latest`` restriction on top of :func:`validate_tag_name`: ``latest`` is + machine-managed and only ever advances on publish, so moving it manually would break the + forward-only guarantee that keeps concurrent publishes consistent. + """ + if tag == LATEST_TAG: + raise ValueError( + f"{LATEST_TAG!r} is managed automatically and always names the most recently published " + "revision; it cannot be moved by hand" + ) + return validate_tag_name(tag) + + +def is_digest(fragment: str) -> bool: + """Whether a ref fragment is a content digest rather than a tag. + + Unambiguous by construction: a digest is exactly 64 lowercase hex chars, and tag names are + bound to the same charset a fragment allows, which cannot produce that shape by accident. + + ``fullmatch`` rather than ``match``: Python's ``$`` also matches before a trailing newline, so + ``match`` would classify a 64-hex string with a newline glued on as a digest and send it down + the query path instead of the tag path. + """ + return bool(_DIGEST_FRAGMENT.fullmatch(fragment)) + + +async def find_by_digest( + entity_client: RevisionStoreProtocol[RevisionT], + revision_type: type[RevisionT], + head: TaskEntity | TasksetEntity, + digest: str, +) -> RevisionT | None: + """Find this record's newest revision with a given content digest, or ``None``. + + Scoped by parent as well as digest: identical content published under two different records + yields the same digest, so the digest alone does not identify a revision. + + One record *can* hold two revisions with the same digest — reverting to earlier content + republishes it — so this orders newest-first rather than taking whichever row came back. The + two are interchangeable in content, which is all a digest pin promises, but a pin should not + resolve to a different ordinal from one call to the next. + """ + result = await entity_client.list( + revision_type, + workspace=head.workspace, + filter_operation=LogicalOperation( + operator=FilterOperator.AND, + operations=[ + ComparisonOperation(field="parent", operator=FilterOperator.EQ, value=head.id), + ComparisonOperation(field="data.content_hash", operator=FilterOperator.EQ, value=digest), + ], + ), + sort="-created_at", + page_size=1, + ) + return result.data[0] if result.data else None + + +async def _current_revision( + revision_client: RevisionStoreProtocol[RevisionT], + revision_type: type[RevisionT], + head: TaskEntity | TasksetEntity, +) -> RevisionT | None: + """The revision ``latest`` names, or ``None`` if this record has none yet. + + ``None`` also covers the record whose ``latest`` names a revision that is missing: publishing + is then the recovery, not an error to propagate. + """ + ordinal = head.tags.get(LATEST_TAG) + if ordinal is None: + return None + try: + return await revision_client.get( + revision_type, name=revision_name(ordinal), workspace=head.workspace, parent=head.id + ) + except NemoEntityNotFoundError: + return None + + +async def get_revision( + entity_client: RevisionStoreProtocol[RevisionT], + revision_type: type[RevisionT], + head: TaskEntity | TasksetEntity, + fragment: str = LATEST_TAG, +) -> RevisionT: + """Fetch the revision a ref fragment names, verifying its content on the way out. + + A tag resolves through ``tags`` to an ordinal and then a direct child lookup — no query. A + digest is queried by ``(parent, content_hash)``. Either way the content that comes back is + re-hashed and compared against the digest stored beside it, because a pinned ref is a claim + about what the consumer will get and this is the one place that claim can be checked. Without + it the digest is only a label: a revision mutated in place would keep resolving and quietly + serve content nobody pinned. + """ + if is_digest(fragment): + revision = await find_by_digest(entity_client, revision_type, head, fragment) + if revision is None: + raise RevisionNotFoundError(f"'{head.workspace}/{head.name}' has no revision with digest {fragment!r}") + _verify_content(head, revision) + return revision + + ordinal = head.tags.get(fragment) + if ordinal is None: + known = ", ".join(sorted(head.tags)) or "none" + raise RevisionNotFoundError( + f"'{head.workspace}/{head.name}' has no revision tagged {fragment!r} (known tags: {known})" + ) + try: + revision = await entity_client.get( + revision_type, name=revision_name(ordinal), workspace=head.workspace, parent=head.id + ) + except NemoEntityNotFoundError as exc: + raise RevisionNotFoundError( + f"'{head.workspace}/{head.name}' tags {fragment!r} as revision {ordinal}, but that " + "revision record is missing" + ) from exc + _verify_content(head, revision) + return revision + + +def _verify_content(head: TaskEntity | TasksetEntity, revision: TaskRevisionEntity | TasksetRevisionEntity) -> None: + """Re-hash a revision's content and check it against the digest stored alongside it. + + A revision is immutable by convention, not by enforcement — the store will happily accept a + write to one. This turns that convention into something detectable rather than something the + reader has to assume. + """ + actual = content_hash(revision, exclude=REVISION_SELF_FIELDS) + if actual != revision.content_hash: + raise RevisionContentMismatchError( + f"revision {revision.revision} of '{head.workspace}/{head.name}' does not match its " + f"recorded digest (recorded {revision.content_hash}, actual {actual}); the stored " + "content has been modified since it was published" + ) + + +async def list_revisions( + entity_client: RevisionStoreProtocol[RevisionT], + revision_type: type[RevisionT], + head: TaskEntity | TasksetEntity, + *, + page: int = 1, + page_size: int = 100, +) -> ListResponse[RevisionT]: + """Return a page of a record's published revisions, newest first. + + Scoped by parent, so it returns this record's history rather than every revision of every + record in the workspace. + + Ordering is server-side on ``-created_at`` rather than sorted client-side by ordinal: sorting + one page locally would order *within* the page while paging silently returned an arbitrary + slice. Creation order matches ordinal order because ordinal allocation is serialized by the + child create, so the two agree. + + Returns the store's ``ListResponse`` — including pagination counts — so a caller can tell a + complete history from a truncated one. Returning a bare list would make a capped result + indistinguishable from the whole thing. + """ + return await entity_client.list( + revision_type, + workspace=head.workspace, + filter_operation=ComparisonOperation(field="parent", operator=FilterOperator.EQ, value=head.id), + sort="-created_at", + page=page, + page_size=page_size, + ) + + +async def apply_tag( + head_client: HeadStoreProtocol[HeadT], + revision_client: RevisionStoreProtocol[RevisionT], + revision_type: type[RevisionT], + head: HeadT, + tag: str, + fragment: str, +) -> HeadT: + """Point an existing tag at the revision ``fragment`` names. + + Separate from publishing because a tag is not always applied at publish time — blessing a + revision after it has been evaluated is the common case, and Harbor exposes the same operation + (``tag_package_version``) independently of publish. + + ``latest`` is refused: it is machine-managed and moves only forward, on publish. Letting a + caller point it at an arbitrary revision would make it disagree with ``latest_revision`` and + break the forward-only guarantee that keeps concurrent publishes consistent. + """ + validate_movable_tag(tag) + revision = await get_revision(revision_client, revision_type, head, fragment) + return await _point_tags(head_client, head, {tag}, revision.revision) + + +async def publish_revision( + head_client: HeadStoreProtocol[HeadT], + revision_client: RevisionStoreProtocol[RevisionT], + head: HeadT, + revision_type: type[RevisionT], + *, + tags: set[str] | None = None, +) -> tuple[RevisionT, HeadT, bool]: + """Freeze a head record's current content as a revision and point tags at it. + + Returns ``(revision, head, created)``. The head is returned already carrying the new pointers, + so callers do not have to re-read it. ``created`` is ``False`` when the content was already + published — republishing identical content is a no-op that still applies any newly requested + tags, which is what makes a re-publish cheap and idempotent. + + ``latest`` is always applied, on top of any caller-supplied tags. + """ + # ``latest`` is applied regardless, so a caller who lists it explicitly is asking for what + # already happens — tolerated rather than rejected. Only *moving* latest by hand is refused + # (see :func:`validate_movable_tag`). + applied = {LATEST_TAG} | {validate_tag_name(tag) for tag in tags or set() if tag != LATEST_TAG} + + for attempt in range(_MAX_ALLOCATION_ATTEMPTS): + digest = head_digest(head) + + # Dedup against the revision ``latest`` names, *not* against any revision ever published. + # Matching any would make reverting to older content a no-op: the head would be rewritten + # with that content while ``latest`` — which only moves forward — kept naming the newer + # revision, so a plain read and a ``#latest`` read would disagree. A revert is a genuine + # change to what this record currently is, so it publishes. + current = await _current_revision(revision_client, revision_type, head) + if current is not None and current.content_hash == digest: + if any(head.tags.get(tag) != current.revision for tag in applied): + head = await _point_tags(head_client, head, applied, current.revision) + return current, head, False + + ordinal = head.latest_revision + 1 + content = head.model_dump(exclude=set(REVISION_POINTER_FIELDS) | set(head.__base_fields__), mode="json") + revision = revision_type( + name=revision_name(ordinal), + workspace=head.workspace, + project=head.project, + content_hash=digest, + revision=ordinal, + **content, + ) + revision._parent = head.id + + try: + created_revision = await revision_client.create(revision) + except NemoEntityConflictError: + # Another publisher took this ordinal. Refresh only the allocation state — re-reading + # the whole head would replace the content the caller staged for publishing with + # whatever is currently stored, and we'd publish the wrong thing. + logger.info( + "Revision ordinal contended, retrying", + extra={"record": f"{head.workspace}/{head.name}", "ordinal": ordinal, "attempt": attempt + 1}, + ) + _refresh_pointers(head, await head_client.get(type(head), name=head.name, workspace=head.workspace)) + # The refresh above adopts the *stored* allocation state, which does not necessarily + # account for the ordinal we just lost: if the winner's own pointer write never landed, + # the stored head still names N-1 and we would recompute N and lose again, every attempt + # until the retries run out — and every later publish would do the same, leaving the + # record permanently unpublishable. The failed create is itself proof that N is taken, + # so step past it explicitly rather than trusting the head to say so. + head.latest_revision = max(head.latest_revision, ordinal) + continue + + # If the pointer write below exhausts its retries, the revision child still exists while + # `latest_revision` never advanced. That is safe but wasteful: the next publish computes the + # same ordinal, loses the create, and — via the step-past above — retries onto N+1. It + # self-heals rather than corrupting anything, so it is left alone deliberately. + head = await _point_tags(head_client, head, applied, ordinal) + return created_revision, head, True + + raise RevisionConflictError( + f"could not allocate a revision ordinal for '{head.workspace}/{head.name}' after " + f"{_MAX_ALLOCATION_ATTEMPTS} attempts" + ) + + +def _refresh_pointers(head: HeadT, fresh: HeadT) -> HeadT: + """Adopt another writer's revision bookkeeping without touching our content. + + A publish carries the caller's staged content; only the allocation state (which ordinals exist + and the version we're writing against) can go stale underneath it. So a retry refreshes exactly + that and leaves the content alone. + """ + head.latest_revision = fresh.latest_revision + head.tags = fresh.tags + head._db_version = fresh._db_version + return head + + +def _apply_pointers(head: HeadT, tags: set[str], ordinal: int) -> HeadT: + """Fold one publish's pointers into a head record (pure; no I/O). + + ``latest`` moves **forward only**. Two publishers can create their revisions in one order and + reach the head update in the other; without this guard the second writer's ``latest`` wins even + though it names the older revision, leaving ``latest`` and ``latest_revision`` disagreeing. + ``latest`` is machine-managed, so forward-only is the right rule. User tags are explicit intent + and may legitimately be moved backwards — retagging ``blessed`` onto an older revision is a + rollback, not a race. + """ + moves_forward = ordinal >= head.tags.get(LATEST_TAG, 0) + applied = {tag: ordinal for tag in tags if tag != LATEST_TAG or moves_forward} + head.tags = {**head.tags, **applied} + head.latest_revision = max(head.latest_revision, ordinal) + return head + + +async def _point_tags( + head_client: HeadStoreProtocol[HeadT], + head: HeadT, + tags: set[str], + ordinal: int, +) -> HeadT: + """Point tags at a revision ordinal, under the head's optimistic lock. + + The read-modify-write is retried on conflict rather than propagated: losing this race means + another publisher updated the head between our read and our write, and their pointers must be + preserved — so we re-read *their* head and fold our pointers into it, instead of overwriting + with a copy that predates them. + + Which content survives depends on who is newer. If the revision we are pointing at is the + newest, the head takes our content. If the winner published a *later* revision, the head has to + keep theirs: writing ours would leave the head serving one revision's content while ``latest`` + named another, which is the same divergence a plain read and a ``#latest`` read must never show. + Our non-``latest`` tags still apply either way — pointing ``blessed`` at an older revision is + legitimate; dragging the head's content back with it is not. + """ + for attempt in range(_MAX_ALLOCATION_ATTEMPTS): + try: + return await head_client.update(_apply_pointers(head, tags, ordinal)) + except NemoEntityConflictError: + logger.info( + "Head pointer update contended, re-reading", + extra={"record": f"{head.workspace}/{head.name}", "attempt": attempt + 1}, + ) + fresh = await head_client.get(type(head), name=head.name, workspace=head.workspace) + if ordinal < fresh.tags.get(LATEST_TAG, 0): + head = fresh + else: + _refresh_pointers(head, fresh) + raise RevisionConflictError( + f"could not update revision pointers for '{head.workspace}/{head.name}' after " + f"{_MAX_ALLOCATION_ATTEMPTS} attempts" + ) diff --git a/plugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.py b/plugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.py index 1973c0f419..69435d499b 100644 --- a/plugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.py +++ b/plugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.py @@ -12,7 +12,7 @@ from urllib.parse import quote -from nemo_evaluator.api.schemas import Task, TaskInput +from nemo_evaluator.api.schemas import Revision, Task, TaskInput from nemo_evaluator.sdk import http_utils from nemo_platform import AsyncNeMoPlatform, NeMoPlatform from nemo_platform_plugin.schema import Page @@ -53,14 +53,58 @@ def create(self, name: str, *, task: TaskInput, project: str | None = None, work response.raise_for_status() return Task.model_validate(response.json()) - def retrieve(self, name: str, *, workspace: str | None = None) -> Task: - """Get a stored task by name.""" + def replace(self, name: str, *, task: TaskInput, project: str | None = None, workspace: str | None = None) -> Task: + """Publish a revision of a task, creating it if absent. + + Upsert, so a publisher needs no existence check. Submitting content identical to the current + revision publishes nothing and returns the task unchanged. + + The response body is the same either way, so this does not report whether a revision was + cut — the server signals that with 201 vs 200, which is discarded here. Compare the returned + ``revision`` against a prior read if you need to know.""" + response = self._http_client.put( + self._item_url(name, workspace), + json=task.model_dump(mode="json"), + params={"project": project} if project is not None else None, + headers=self._headers(), + timeout=self._platform.timeout, + ) + response.raise_for_status() + return Task.model_validate(response.json()) + + def list_revisions( + self, name: str, *, page: int = 1, page_size: int = 100, workspace: str | None = None + ) -> Page[Revision]: + """List a task's published revisions, newest first.""" response = self._http_client.get( - self._item_url(name, workspace), headers=self._headers(), timeout=self._platform.timeout + f"{self._item_url(name, workspace)}/revisions", + params={"page": page, "page_size": page_size}, + headers=self._headers(), + timeout=self._platform.timeout, + ) + response.raise_for_status() + return Page[Revision].model_validate(response.json()) + + def tag(self, name: str, tag: str, *, revision: str, workspace: str | None = None) -> Task: + """Point a tag at an existing revision, named by digest or by another tag.""" + response = self._http_client.put( + f"{self._item_url(name, workspace)}/tags/{quote(tag, safe='')}", + params={"revision": revision}, + headers=self._headers(), + timeout=self._platform.timeout, ) response.raise_for_status() return Task.model_validate(response.json()) + def retrieve(self, name: str, *, revision: str | None = None, workspace: str | None = None) -> Task: + """Get a stored task by name, or as of a published revision (digest or tag).""" + url = self._item_url(name, workspace) + if revision is not None: + url = f"{url}/revisions/{quote(revision, safe='')}" + response = self._http_client.get(url, headers=self._headers(), timeout=self._platform.timeout) + response.raise_for_status() + return Task.model_validate(response.json()) + def list( self, *, workspace: str | None = None, page: int = 1, page_size: int = 100, sort: str | None = None ) -> Page[Task]: @@ -112,12 +156,58 @@ async def create( response.raise_for_status() return Task.model_validate(response.json()) - async def retrieve(self, name: str, *, workspace: str | None = None) -> Task: - """Get a stored task by name.""" + async def replace( + self, name: str, *, task: TaskInput, project: str | None = None, workspace: str | None = None + ) -> Task: + """Publish a revision of a task, creating it if absent. + + Upsert, so a publisher needs no existence check. Submitting content identical to the current + revision publishes nothing and returns the task unchanged. + + The response body is the same either way, so this does not report whether a revision was + cut — the server signals that with 201 vs 200, which is discarded here. Compare the returned + ``revision`` against a prior read if you need to know.""" + response = await self._http_client.put( + self._item_url(name, workspace), + json=task.model_dump(mode="json"), + params={"project": project} if project is not None else None, + headers=self._headers(), + timeout=self._platform.timeout, + ) + response.raise_for_status() + return Task.model_validate(response.json()) + + async def list_revisions( + self, name: str, *, page: int = 1, page_size: int = 100, workspace: str | None = None + ) -> Page[Revision]: + """List a task's published revisions, newest first.""" response = await self._http_client.get( - self._item_url(name, workspace), headers=self._headers(), timeout=self._platform.timeout + f"{self._item_url(name, workspace)}/revisions", + params={"page": page, "page_size": page_size}, + headers=self._headers(), + timeout=self._platform.timeout, ) response.raise_for_status() + return Page[Revision].model_validate(response.json()) + + async def tag(self, name: str, tag: str, *, revision: str, workspace: str | None = None) -> Task: + """Point a tag at an existing revision, named by digest or by another tag.""" + response = await self._http_client.put( + f"{self._item_url(name, workspace)}/tags/{quote(tag, safe='')}", + params={"revision": revision}, + headers=self._headers(), + timeout=self._platform.timeout, + ) + response.raise_for_status() + return Task.model_validate(response.json()) + + async def retrieve(self, name: str, *, revision: str | None = None, workspace: str | None = None) -> Task: + """Get a stored task by name, or as of a published revision (digest or tag).""" + url = self._item_url(name, workspace) + if revision is not None: + url = f"{url}/revisions/{quote(revision, safe='')}" + response = await self._http_client.get(url, headers=self._headers(), timeout=self._platform.timeout) + response.raise_for_status() return Task.model_validate(response.json()) async def list( diff --git a/plugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.py b/plugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.py index 8526992fd9..9a45d201d9 100644 --- a/plugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.py +++ b/plugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.py @@ -12,7 +12,7 @@ from urllib.parse import quote -from nemo_evaluator.api.schemas import Taskset, TasksetInput +from nemo_evaluator.api.schemas import Revision, Taskset, TasksetInput from nemo_evaluator.sdk import http_utils from nemo_platform import AsyncNeMoPlatform, NeMoPlatform from nemo_platform_plugin.schema import Page @@ -57,14 +57,60 @@ def create( response.raise_for_status() return Taskset.model_validate(response.json()) - def retrieve(self, name: str, *, workspace: str | None = None) -> Taskset: - """Get a stored taskset by name.""" + def replace( + self, name: str, *, taskset: TasksetInput, project: str | None = None, workspace: str | None = None + ) -> Taskset: + """Publish a revision of a taskset, creating it if absent. + + Members are re-resolved to exact revision digests on every call, so identical member names + can still publish a new revision if a member task published in the meantime. + + The response body is the same either way, so this does not report whether a revision was + cut — the server signals that with 201 vs 200, which is discarded here. Compare the returned + ``revision`` against a prior read if you need to know.""" + response = self._http_client.put( + self._item_url(name, workspace), + json=taskset.model_dump(mode="json"), + params={"project": project} if project is not None else None, + headers=self._headers(), + timeout=self._platform.timeout, + ) + response.raise_for_status() + return Taskset.model_validate(response.json()) + + def list_revisions( + self, name: str, *, page: int = 1, page_size: int = 100, workspace: str | None = None + ) -> Page[Revision]: + """List a taskset's published revisions, newest first.""" response = self._http_client.get( - self._item_url(name, workspace), headers=self._headers(), timeout=self._platform.timeout + f"{self._item_url(name, workspace)}/revisions", + params={"page": page, "page_size": page_size}, + headers=self._headers(), + timeout=self._platform.timeout, + ) + response.raise_for_status() + return Page[Revision].model_validate(response.json()) + + def tag(self, name: str, tag: str, *, revision: str, workspace: str | None = None) -> Taskset: + """Point a tag at an existing revision, named by digest or by another tag.""" + response = self._http_client.put( + f"{self._item_url(name, workspace)}/tags/{quote(tag, safe='')}", + params={"revision": revision}, + headers=self._headers(), + timeout=self._platform.timeout, ) response.raise_for_status() return Taskset.model_validate(response.json()) + def retrieve(self, name: str, *, revision: str | None = None, workspace: str | None = None) -> Taskset: + """Get a stored taskset by name, or as of a published revision (digest or tag).""" + url = self._item_url(name, workspace) + if revision is not None: + url = f"{url}/revisions/{quote(revision, safe='')}" + response = self._http_client.get(url, headers=self._headers(), timeout=self._platform.timeout) + response.raise_for_status() + return Taskset.model_validate(response.json()) + def list( self, *, workspace: str | None = None, page: int = 1, page_size: int = 100, sort: str | None = None ) -> Page[Taskset]: @@ -118,14 +164,60 @@ async def create( response.raise_for_status() return Taskset.model_validate(response.json()) - async def retrieve(self, name: str, *, workspace: str | None = None) -> Taskset: - """Get a stored taskset by name.""" + async def replace( + self, name: str, *, taskset: TasksetInput, project: str | None = None, workspace: str | None = None + ) -> Taskset: + """Publish a revision of a taskset, creating it if absent. + + Members are re-resolved to exact revision digests on every call, so identical member names + can still publish a new revision if a member task published in the meantime. + + The response body is the same either way, so this does not report whether a revision was + cut — the server signals that with 201 vs 200, which is discarded here. Compare the returned + ``revision`` against a prior read if you need to know.""" + response = await self._http_client.put( + self._item_url(name, workspace), + json=taskset.model_dump(mode="json"), + params={"project": project} if project is not None else None, + headers=self._headers(), + timeout=self._platform.timeout, + ) + response.raise_for_status() + return Taskset.model_validate(response.json()) + + async def list_revisions( + self, name: str, *, page: int = 1, page_size: int = 100, workspace: str | None = None + ) -> Page[Revision]: + """List a taskset's published revisions, newest first.""" response = await self._http_client.get( - self._item_url(name, workspace), headers=self._headers(), timeout=self._platform.timeout + f"{self._item_url(name, workspace)}/revisions", + params={"page": page, "page_size": page_size}, + headers=self._headers(), + timeout=self._platform.timeout, + ) + response.raise_for_status() + return Page[Revision].model_validate(response.json()) + + async def tag(self, name: str, tag: str, *, revision: str, workspace: str | None = None) -> Taskset: + """Point a tag at an existing revision, named by digest or by another tag.""" + response = await self._http_client.put( + f"{self._item_url(name, workspace)}/tags/{quote(tag, safe='')}", + params={"revision": revision}, + headers=self._headers(), + timeout=self._platform.timeout, ) response.raise_for_status() return Taskset.model_validate(response.json()) + async def retrieve(self, name: str, *, revision: str | None = None, workspace: str | None = None) -> Taskset: + """Get a stored taskset by name, or as of a published revision (digest or tag).""" + url = self._item_url(name, workspace) + if revision is not None: + url = f"{url}/revisions/{quote(revision, safe='')}" + response = await self._http_client.get(url, headers=self._headers(), timeout=self._platform.timeout) + response.raise_for_status() + return Taskset.model_validate(response.json()) + async def list( self, *, workspace: str | None = None, page: int = 1, page_size: int = 100, sort: str | None = None ) -> Page[Taskset]: diff --git a/plugins/nemo-evaluator/src/nemo_evaluator/task_refs.py b/plugins/nemo-evaluator/src/nemo_evaluator/task_refs.py index 0ce535fdc5..589da09313 100644 --- a/plugins/nemo-evaluator/src/nemo_evaluator/task_refs.py +++ b/plugins/nemo-evaluator/src/nemo_evaluator/task_refs.py @@ -16,35 +16,51 @@ from __future__ import annotations -from nemo_evaluator.api.schemas import TasksetRef, parse_entity_ref -from nemo_evaluator.entities import TaskEntity, TasksetEntity +from typing import cast + +from nemo_evaluator.api.schemas import TasksetRef, parse_entity_ref, parse_subentity_ref +from nemo_evaluator.entities import TaskEntity, TaskRevisionEntity, TasksetEntity from nemo_evaluator.jobs.agent_spec import AgentEvalTaskInput -from nemo_platform_plugin.entity_client import NemoAnyEntityGetterProtocol, NemoEntityNotFoundError +from nemo_evaluator.revisions import RevisionNotFoundError, get_revision +from nemo_platform_plugin.entities import EntityClientProtocol +from nemo_platform_plugin.entity_client import NemoEntityNotFoundError + +def _entity_to_task_input(entity: TaskEntity, revision: TaskRevisionEntity) -> AgentEvalTaskInput: + """Project a stored task's *published revision* onto the submitter-facing inline task DTO. -def _entity_to_task_input(entity: TaskEntity) -> AgentEvalTaskInput: - """Project a stored task onto the submitter-facing inline task DTO. + Identity (``id``) comes from the head record — it is the same task — while every content field + comes from the revision the taskset pinned. That split is what makes a taskset-driven evaluation + reproducible: re-running it expands to the same content even if the member task has published + since. - The task's stable ``id`` is its record ``name``. A stored task holds metric *references* (inline - metrics were normalized to derived stored metrics on create); those resolve to inline bundles in - the shared metric-ref pass that runs after expansion. A stored task carries no grader-only - ``reference`` (the entity has no such field), so taskset-driven tasks run with an empty one. + A stored task holds metric *references* (inline metrics were normalized to derived stored + metrics on create); those resolve to inline bundles in the shared metric-ref pass that runs + after expansion. A stored task carries no grader-only ``reference`` (the entity has no such + field), so taskset-driven tasks run with an empty one. """ return AgentEvalTaskInput( id=entity.name, - intent=entity.intent, - inputs=entity.inputs, - metrics=list(entity.metrics), - views=entity.views, - metadata=entity.metadata, + intent=revision.intent, + inputs=revision.inputs, + metrics=list(revision.metrics), + views=revision.views, + metadata=revision.metadata, ) +#: Expanding a taskset reads three entity types through one client — the taskset head, each member +#: task's head, and the pinned revision of each member. Python has no intersection types, so the +#: parameter is annotated at one of them and the other two are taken as typed views of the same +#: object; the concrete client's methods are generic over the entity type and satisfy all three. +TasksetStoreProtocol = EntityClientProtocol[TasksetEntity] + + async def resolve_taskset_ref( ref: TasksetRef, *, workspace: str, - entity_client: NemoAnyEntityGetterProtocol | None, + entity_client: TasksetStoreProtocol | None, ) -> list[AgentEvalTaskInput]: """Load a stored taskset and expand its members into inline task DTOs. @@ -56,6 +72,9 @@ async def resolve_taskset_ref( "A TasksetRef requires a platform connection (entity store) to resolve; it cannot be used " "in local execution. Pass an inline task list instead." ) + task_store = cast(EntityClientProtocol[TaskEntity], entity_client) + revision_store = cast(EntityClientProtocol[TaskRevisionEntity], entity_client) + ref_workspace, name = parse_entity_ref(ref.root, workspace) try: taskset = await entity_client.get(TasksetEntity, name=name, workspace=ref_workspace) @@ -72,14 +91,24 @@ async def resolve_taskset_ref( tasks: list[AgentEvalTaskInput] = [] seen_ids: set[str] = set() for task_ref in taskset.tasks: - task_workspace, task_name = parse_entity_ref(task_ref.root, ref_workspace) + task_workspace, task_name, fragment = parse_subentity_ref(task_ref.root, ref_workspace) try: - entity = await entity_client.get(TaskEntity, name=task_name, workspace=task_workspace) + entity = await task_store.get(TaskEntity, name=task_name, workspace=task_workspace) except NemoEntityNotFoundError as exc: raise ValueError( f"Task '{task_ref.root}' referenced by taskset '{ref.root}' was not found; " "the stored task may have been deleted after the taskset was created." ) from exc + # Expand the *pinned* revision, not the task's current content. A published taskset names + # exact revisions; resolving to whatever is current would silently defeat the pinning and + # make an evaluation irreproducible the moment a member republished. + try: + revision = await get_revision(revision_store, TaskRevisionEntity, entity, fragment) + except RevisionNotFoundError as exc: + raise ValueError( + f"Task '{task_ref.root}' referenced by taskset '{ref.root}' names a revision that no " + f"longer resolves: {exc}" + ) from exc # Agent-eval task ids must be unique within a run. Member refs are unique per (workspace, # name), but refs from different workspaces can share a name — surface that as a clear error # rather than letting the SDK evaluator reject duplicate ids deeper in the run. @@ -89,7 +118,7 @@ async def resolve_taskset_ref( "task ids must be unique within an evaluation." ) seen_ids.add(entity.name) - tasks.append(_entity_to_task_input(entity)) + tasks.append(_entity_to_task_input(entity, revision)) return tasks @@ -97,7 +126,7 @@ async def resolve_agent_eval_tasks( tasks: TasksetRef | list[AgentEvalTaskInput], *, workspace: str, - entity_client: NemoAnyEntityGetterProtocol | None, + entity_client: TasksetStoreProtocol | None, ) -> list[AgentEvalTaskInput]: """Normalize an agent-eval ``tasks`` field to an inline task list. diff --git a/plugins/nemo-evaluator/tests/api/service/test_task_service.py b/plugins/nemo-evaluator/tests/api/service/test_task_service.py index 6287817f27..10913536a3 100644 --- a/plugins/nemo-evaluator/tests/api/service/test_task_service.py +++ b/plugins/nemo-evaluator/tests/api/service/test_task_service.py @@ -3,16 +3,12 @@ from __future__ import annotations -from datetime import datetime, timezone - import pytest from nemo_evaluator.api.schemas import MetadataItem, MetricInline, MetricRef, Task, TaskInput, TaskInputs from nemo_evaluator.api.service.task_service import MetricRefNotFoundError, TaskService -from nemo_evaluator.entities import TaskEntity from nemo_evaluator.shared.metric_bundles.bundles import bundle_metric from nemo_evaluator.shared.metric_bundles.cloudpickle import CloudpickleMetricBundlePackager from nemo_evaluator_sdk.metrics.exact_match import ExactMatchMetric -from nemo_platform_plugin.entities import ListResponse, PaginationInfo from nemo_platform_plugin.entity_client import NemoEntityConflictError, NemoEntityNotFoundError @@ -39,68 +35,6 @@ def _inline_metric() -> MetricInline: return MetricInline.model_validate(bundle.model_dump(mode="json")) -class _FakeEntityClient: - def __init__(self) -> None: - self.entities: dict[tuple[str, str, str], TaskEntity] = {} - - async def create(self, entity: TaskEntity) -> TaskEntity: - key = (entity.__entity_type__, entity.workspace, entity.name) - if key in self.entities: - raise NemoEntityConflictError(f"{key} exists") - now = datetime.now(timezone.utc) - entity._id = f"{entity.__entity_type__}-{entity.name}" - entity._created_at = now - entity._updated_at = now - self.entities[key] = entity - return entity - - async def get( - self, entity_type: type[TaskEntity], *, workspace: str, name: str, parent: str | None = None - ) -> TaskEntity: - key = (entity_type.__entity_type__, workspace, name) - if key not in self.entities: - raise NemoEntityNotFoundError(f"{workspace}/{name} not found") - return self.entities[key] - - async def delete( - self, - entity_type: type[TaskEntity], - name: str, - *, - workspace: str, - parent: str | None = None, - expected_db_version: int | None = None, - ) -> None: - key = (entity_type.__entity_type__, workspace, name) - if key not in self.entities: - raise NemoEntityNotFoundError(f"{workspace}/{name} not found") - del self.entities[key] - - async def list( - self, - entity_type: type[TaskEntity], - *, - workspace: str, - filter_operation: object | None = None, - sort: str | None = None, - page: int = 1, - page_size: int = 100, - ) -> ListResponse[TaskEntity]: - items = [ - e for (etype, ws, _), e in self.entities.items() if etype == entity_type.__entity_type__ and ws == workspace - ] - return ListResponse( - data=items, - pagination=PaginationInfo( - page=page, - page_size=page_size, - current_page_size=len(items), - total_pages=1, - total_results=len(items), - ), - ) - - def _task_input() -> TaskInput: return TaskInput( intent="Answer the question.", @@ -116,12 +50,12 @@ def metric_service() -> _FakeMetricService: @pytest.fixture -def service(metric_service: _FakeMetricService) -> TaskService: - return TaskService(_FakeEntityClient(), metric_service) +def service(metric_service: _FakeMetricService, entity_store) -> TaskService: + return TaskService(entity_store, metric_service) async def test_create_then_get(service: TaskService) -> None: - created = await service.create_task("task-1", _task_input(), workspace="default") + created, _ = await service.create_task("task-1", _task_input(), workspace="default") assert isinstance(created, Task) assert created.name == "task-1" @@ -144,7 +78,7 @@ async def test_create_normalizes_inline_metrics_to_refs( metrics=[MetricRef("default/stored-metric"), inline], ) - created = await service.create_task("task-1", task_input, workspace="default") + created, _ = await service.create_task("task-1", task_input, workspace="default") # The inline metric was offloaded to the metric service (stored as a derived metric)... assert metric_service.stored == [inline] @@ -163,7 +97,7 @@ async def test_create_rejects_missing_metric_ref(service: TaskService) -> None: async def test_create_canonicalizes_bare_metric_ref(service: TaskService) -> None: # A bare "stored-metric" ref resolves against the task workspace and is persisted as "default/stored-metric". task_input = TaskInput(intent="x", inputs=TaskInputs(instruction="?"), metrics=[MetricRef("stored-metric")]) - created = await service.create_task("task-1", task_input, workspace="default") + created, _ = await service.create_task("task-1", task_input, workspace="default") assert created.metrics[0].root == "default/stored-metric" @@ -195,3 +129,140 @@ async def test_delete(service: TaskService) -> None: async def test_delete_returns_false_when_missing(service: TaskService) -> None: assert await service.delete_task("default", "nope") is False + + +# --- Failure handling --------------------------------------------------------- + + +async def test_create_rolls_back_the_head_when_publishing_fails( + metric_service: _FakeMetricService, entity_store +) -> None: + """A head with no revision would break the invariant every consumer relies on — `#latest` + always resolves and `revision` is never 0. There is no cross-entity transaction, so create + must undo itself rather than leave a half-created task behind.""" + service = TaskService(entity_store, metric_service) + + real_create = type(entity_store).create + + async def _boom(entity): + if entity.__entity_type__ == "task_revision": + raise RuntimeError("store unavailable") + return await real_create(entity_store, entity) + + entity_store.create = _boom + + with pytest.raises(RuntimeError): + await service.create_task("task-1", _task_input(), workspace="default") + + entity_store.create = real_create.__get__(entity_store) + assert await service.get_task("default", "task-1") is None, "the orphaned head must not survive" + + +async def test_replace_propagates_a_concurrent_write_conflict(metric_service: _FakeMetricService, entity_store) -> None: + """Losing the optimistic lock is a client-retryable conflict, not a server fault — the route + maps this to 409, so the service must let it through rather than swallowing it.""" + service = TaskService(entity_store, metric_service) + await service.create_task("task-1", _task_input(), workspace="default") + + async def _stale(entity, *, original_name=None): + raise NemoEntityConflictError("modified by another request") + + entity_store.update = _stale + + with pytest.raises(NemoEntityConflictError): + await service.replace_task("task-1", _task_input(), workspace="default") + + +async def test_replace_leaves_no_uncovered_head_content_when_publishing_fails( + metric_service: _FakeMetricService, entity_store +) -> None: + """A failed publish must not leave the head serving content no revision covers. + + Replace stages content in memory and lets publishing commit it, so a publish that never + happens leaves nothing behind. Committing the head first would instead make a plain GET — + which reads the head — return content that `#latest` does not resolve to. + """ + service = TaskService(entity_store, metric_service) + await service.create_task("task-1", _task_input(), workspace="default") + + real_create = type(entity_store).create + + async def _boom(entity): + if entity.__entity_type__ == "task_revision": + raise RuntimeError("store unavailable") + return await real_create(entity_store, entity) + + entity_store.create = _boom + + changed = TaskInput( + intent="Rewritten.", inputs=TaskInputs(instruction="?"), metrics=[MetricRef("default/stored-metric")] + ) + with pytest.raises(RuntimeError): + await service.replace_task("task-1", changed, workspace="default") + + entity_store.create = real_create.__get__(entity_store) + head = await service.get_task("default", "task-1") + assert head is not None + assert head.intent == "Answer the question.", "the head must still hold the last published content" + + +async def test_tag_revision_returns_none_for_a_missing_task(service: TaskService) -> None: + assert await service.tag_revision("default", "nope", "blessed", "latest") is None + + +async def test_list_revisions_returns_none_for_a_missing_task(service: TaskService) -> None: + assert await service.list_revisions("default", "nope") is None + + +async def test_replace_applies_project(service: TaskService) -> None: + """`project` used to be accepted on replace and silently discarded.""" + await service.create_task("task-1", _task_input(), workspace="default", project="proj-a") + replaced, _ = await service.replace_task("task-1", _task_input(), workspace="default", project="proj-b") + assert replaced.project == "proj-b" + + +async def test_replace_without_project_leaves_it_unchanged(service: TaskService) -> None: + """An omitted query parameter means "leave it alone", not "clear it".""" + await service.create_task("task-1", _task_input(), workspace="default", project="proj-a") + replaced, _ = await service.replace_task("task-1", _task_input(), workspace="default") + assert replaced.project == "proj-a" + + +async def test_rollback_failure_does_not_mask_the_original_error( + metric_service: _FakeMetricService, entity_store +) -> None: + """If cleanup also fails, the caller must still see *why* the publish failed.""" + service = TaskService(entity_store, metric_service) + real_create = type(entity_store).create + + async def _boom(entity): + if entity.__entity_type__ == "task_revision": + raise RuntimeError("the original failure") + return await real_create(entity_store, entity) + + async def _delete_also_fails(*args, **kwargs): + raise RuntimeError("rollback failed too") + + entity_store.create = _boom + entity_store.delete = _delete_also_fails + + with pytest.raises(RuntimeError, match="the original failure"): + await service.create_task("task-1", _task_input(), workspace="default") + + +async def test_resolve_revision_returns_the_digest_for_a_tag(service: TaskService) -> None: + """The hook taskset publishing uses to turn a member's tag into an exact digest.""" + await service.create_task("task-1", _task_input(), workspace="default") + + digest = await service.resolve_revision("default", "task-1") + + revisions = await service.list_revisions("default", "task-1") + assert revisions is not None + assert digest == revisions.data[0].content_hash + + +async def test_resolve_revision_raises_for_a_missing_task(service: TaskService) -> None: + """Existence surfaces from resolution itself — taskset publishing relies on this to reject a + member that does not exist, now that the separate existence check is gone.""" + with pytest.raises(NemoEntityNotFoundError): + await service.resolve_revision("default", "nope") diff --git a/plugins/nemo-evaluator/tests/api/service/test_taskset_service.py b/plugins/nemo-evaluator/tests/api/service/test_taskset_service.py index cb9c2c2550..97f81ee6c6 100644 --- a/plugins/nemo-evaluator/tests/api/service/test_taskset_service.py +++ b/plugins/nemo-evaluator/tests/api/service/test_taskset_service.py @@ -3,7 +3,8 @@ from __future__ import annotations -from datetime import datetime, timezone +import asyncio +import hashlib import pytest from nemo_evaluator.api.schemas import MetadataItem, TaskRef, Taskset, TasksetInput @@ -13,9 +14,8 @@ TasksetExistsError, TasksetService, ) -from nemo_evaluator.entities import TasksetEntity -from nemo_platform_plugin.entities import ListResponse, PaginationInfo -from nemo_platform_plugin.entity_client import NemoEntityConflictError, NemoEntityNotFoundError +from nemo_evaluator.revisions import RevisionNotFoundError +from nemo_platform_plugin.entity_client import NemoEntityNotFoundError class _FakeTaskService: @@ -27,67 +27,15 @@ def __init__(self, existing: set[tuple[str, str]]) -> None: async def get_task(self, workspace: str, name: str) -> object | None: return object() if (workspace, name) in self.existing else None + async def resolve_revision(self, workspace: str, name: str, fragment: str = "latest") -> str: + """A stable per-task digest, so pinned membership is deterministic across a test. -class _FakeEntityClient: - def __init__(self) -> None: - self.entities: dict[tuple[str, str, str], TasksetEntity] = {} - - async def create(self, entity: TasksetEntity) -> TasksetEntity: - key = (entity.__entity_type__, entity.workspace, entity.name) - if key in self.entities: - raise NemoEntityConflictError(f"{key} exists") - now = datetime.now(timezone.utc) - entity._id = f"{entity.__entity_type__}-{entity.name}" - entity._created_at = now - entity._updated_at = now - self.entities[key] = entity - return entity - - async def get( - self, entity_type: type[TasksetEntity], *, workspace: str, name: str, parent: str | None = None - ) -> TasksetEntity: - key = (entity_type.__entity_type__, workspace, name) - if key not in self.entities: + Raises for an unknown task, matching the real service: resolution fetches the task, so a + missing one surfaces here rather than from a separate existence check. + """ + if (workspace, name) not in self.existing: raise NemoEntityNotFoundError(f"{workspace}/{name} not found") - return self.entities[key] - - async def delete( - self, - entity_type: type[TasksetEntity], - name: str, - *, - workspace: str, - parent: str | None = None, - expected_db_version: int | None = None, - ) -> None: - key = (entity_type.__entity_type__, workspace, name) - if key not in self.entities: - raise NemoEntityNotFoundError(f"{workspace}/{name} not found") - del self.entities[key] - - async def list( - self, - entity_type: type[TasksetEntity], - *, - workspace: str, - filter_operation: object | None = None, - sort: str | None = None, - page: int = 1, - page_size: int = 100, - ) -> ListResponse[TasksetEntity]: - items = [ - e for (etype, ws, _), e in self.entities.items() if etype == entity_type.__entity_type__ and ws == workspace - ] - return ListResponse( - data=items, - pagination=PaginationInfo( - page=page, - page_size=page_size, - current_page_size=len(items), - total_pages=1, - total_results=len(items), - ), - ) + return hashlib.sha256(f"{workspace}/{name}".encode()).hexdigest() def _taskset_input() -> TasksetInput: @@ -104,18 +52,21 @@ def existing_tasks() -> set[tuple[str, str]]: @pytest.fixture -def service(existing_tasks: set[tuple[str, str]]) -> TasksetService: - return TasksetService(_FakeEntityClient(), _FakeTaskService(existing_tasks)) +def service(existing_tasks: set[tuple[str, str]], entity_store) -> TasksetService: + return TasksetService(entity_store, _FakeTaskService(existing_tasks)) async def test_create_then_get(service: TasksetService) -> None: - created = await service.create_taskset("ts-1", _taskset_input(), workspace="default") + created, _ = await service.create_taskset("ts-1", _taskset_input(), workspace="default") assert isinstance(created, Taskset) assert created.name == "ts-1" assert created.id == "taskset-ts-1" assert created.description == "A smoke-test grouping." - assert {t.root for t in created.tasks} == {"task-a", "default/task-b"} + # Members are stored workspace-qualified *and* digest-pinned: a bare "task-a" is resolved on + # write, so the stored grouping names exact revisions rather than moving targets. + assert {t.root.split("#")[0] for t in created.tasks} == {"default/task-a", "default/task-b"} + assert all(len(t.root.split("#")[1]) == 64 for t in created.tasks) assert created.created_at is not None got = await service.get_taskset("default", "ts-1") @@ -128,9 +79,11 @@ async def test_create_validates_missing_task_ref(service: TasksetService) -> Non await service.create_taskset("ts-1", taskset_input, workspace="default") -async def test_create_resolves_bare_ref_against_taskset_workspace(existing_tasks: set[tuple[str, str]]) -> None: +async def test_create_resolves_bare_ref_against_taskset_workspace( + existing_tasks: set[tuple[str, str]], entity_store +) -> None: # A bare "task-a" ref must resolve against the taskset's own workspace ("other"), where it is absent. - service = TasksetService(_FakeEntityClient(), _FakeTaskService(existing_tasks)) + service = TasksetService(entity_store, _FakeTaskService(existing_tasks)) with pytest.raises(ValueError, match="not found in workspace 'other'"): await service.create_taskset("ts-1", TasksetInput(tasks=[TaskRef("task-a")]), workspace="other") @@ -153,8 +106,8 @@ async def test_create_allows_same_name_in_different_workspaces(service: TasksetS # Taskset names are unique per workspace, not globally: the same name in another workspace is a # distinct taskset and must not raise TasksetExistsError (409). Empty task lists keep this focused # on name scoping rather than per-workspace task-ref validation. - first = await service.create_taskset("ts-1", TasksetInput(), workspace="default") - second = await service.create_taskset("ts-1", TasksetInput(), workspace="other") + first, _ = await service.create_taskset("ts-1", TasksetInput(), workspace="default") + second, _ = await service.create_taskset("ts-1", TasksetInput(), workspace="other") assert first.name == second.name == "ts-1" assert first.workspace == "default" @@ -183,3 +136,150 @@ async def test_delete(service: TasksetService) -> None: async def test_delete_returns_false_when_missing(service: TasksetService) -> None: assert await service.delete_taskset("default", "nope") is False + + +# --- Failure handling and membership resolution ------------------------------- + + +async def test_create_rolls_back_the_head_when_publishing_fails( + existing_tasks: set[tuple[str, str]], entity_store +) -> None: + """Mirrors the task-side rollback: a taskset head with no revision would break the invariant + that `#latest` always resolves.""" + service = TasksetService(entity_store, _FakeTaskService(existing_tasks)) + real_create = type(entity_store).create + + async def _boom(entity): + if entity.__entity_type__ == "taskset_revision": + raise RuntimeError("store unavailable") + return await real_create(entity_store, entity) + + entity_store.create = _boom + + with pytest.raises(RuntimeError): + await service.create_taskset("ts-1", _taskset_input(), workspace="default") + + entity_store.create = real_create.__get__(entity_store) + assert await service.get_taskset("default", "ts-1") is None + + +async def test_rollback_failure_does_not_mask_the_original_error( + existing_tasks: set[tuple[str, str]], entity_store +) -> None: + """If cleanup also fails, the caller must still see *why* the publish failed — otherwise they + debug the rollback instead of the actual fault.""" + service = TasksetService(entity_store, _FakeTaskService(existing_tasks)) + real_create = type(entity_store).create + + async def _boom(entity): + if entity.__entity_type__ == "taskset_revision": + raise RuntimeError("the original failure") + return await real_create(entity_store, entity) + + async def _delete_also_fails(*args, **kwargs): + raise RuntimeError("rollback failed too") + + entity_store.create = _boom + entity_store.delete = _delete_also_fails + + with pytest.raises(RuntimeError, match="the original failure"): + await service.create_taskset("ts-1", _taskset_input(), workspace="default") + + +async def test_member_naming_an_unknown_revision_is_rejected( + existing_tasks: set[tuple[str, str]], entity_store +) -> None: + """A member pinned to a revision that does not exist is a client error on the submitted body, + not a server fault — the route maps this to 422.""" + + class _NoSuchRevision(_FakeTaskService): + async def resolve_revision(self, workspace: str, name: str, fragment: str = "latest") -> str: + raise RevisionNotFoundError(f"no revision {fragment!r}") + + service = TasksetService(entity_store, _NoSuchRevision(existing_tasks)) + + with pytest.raises(TaskRefNotFoundError, match="no published revision"): + await service.create_taskset("ts-1", TasksetInput(tasks=[TaskRef(f"task-a#{'c' * 64}")]), workspace="default") + + +async def test_members_resolve_concurrently(existing_tasks: set[tuple[str, str]], entity_store) -> None: + """Membership resolution fans out rather than running one member at a time — a Harbor-scale + dataset names hundreds of tasks, and serial resolution made publish latency linear in size.""" + overlap = {"peak": 0, "current": 0} + + class _Tracking(_FakeTaskService): + async def resolve_revision(self, workspace: str, name: str, fragment: str = "latest") -> str: + overlap["current"] += 1 + overlap["peak"] = max(overlap["peak"], overlap["current"]) + await asyncio.sleep(0) # yield, so overlapping calls can interleave + overlap["current"] -= 1 + return hashlib.sha256(f"{workspace}/{name}".encode()).hexdigest() + + members = {("default", f"task-{i}") for i in range(5)} + service = TasksetService(entity_store, _Tracking(members)) + + await service.create_taskset( + "ts-1", TasksetInput(tasks=[TaskRef(f"task-{i}") for i in range(5)]), workspace="default" + ) + + assert overlap["peak"] > 1, "members must resolve concurrently, not one at a time" + + +async def test_member_resolution_preserves_order(existing_tasks: set[tuple[str, str]], entity_store) -> None: + """Concurrency must not reorder membership — `gather` preserves input order, and the digest of + a taskset depends on member order.""" + members = {("default", f"task-{i}") for i in range(5)} + service = TasksetService(entity_store, _FakeTaskService(members)) + + created, _ = await service.create_taskset( + "ts-1", TasksetInput(tasks=[TaskRef(f"task-{i}") for i in range(5)]), workspace="default" + ) + + assert [t.root.split("#")[0] for t in created.tasks] == [f"default/task-{i}" for i in range(5)] + + +async def test_tag_revision_returns_none_for_a_missing_taskset(service: TasksetService) -> None: + assert await service.tag_revision("default", "nope", "blessed", "latest") is None + + +async def test_list_revisions_returns_none_for_a_missing_taskset(service: TasksetService) -> None: + assert await service.list_revisions("default", "nope") is None + + +async def test_replace_applies_project(service: TasksetService) -> None: + """`project` used to be accepted and silently discarded on replace.""" + await service.create_taskset("ts-1", _taskset_input(), workspace="default", project="proj-a") + replaced, _ = await service.replace_taskset("ts-1", _taskset_input(), workspace="default", project="proj-b") + assert replaced.project == "proj-b" + + +async def test_replace_without_project_leaves_it_unchanged(service: TasksetService) -> None: + """An omitted query parameter means "leave it alone", not "clear it".""" + await service.create_taskset("ts-1", _taskset_input(), workspace="default", project="proj-a") + replaced, _ = await service.replace_taskset("ts-1", _taskset_input(), workspace="default") + assert replaced.project == "proj-a" + + +async def test_a_failed_member_cancels_its_siblings(existing_tasks: set[tuple[str, str]], entity_store) -> None: + """`gather` propagates the first failure but leaves siblings running; one bad member in a large + grouping would otherwise keep reading long after the request failed.""" + finished: list[str] = [] + + class _SlowExceptOne(_FakeTaskService): + async def resolve_revision(self, workspace: str, name: str, fragment: str = "latest") -> str: + if name == "task-0": + raise NemoEntityNotFoundError("missing") + await asyncio.sleep(0.05) + finished.append(name) + return hashlib.sha256(name.encode()).hexdigest() + + members = {("default", f"task-{i}") for i in range(5)} + service = TasksetService(entity_store, _SlowExceptOne(members)) + + with pytest.raises(TaskRefNotFoundError): + await service.create_taskset( + "ts-1", TasksetInput(tasks=[TaskRef(f"task-{i}") for i in range(5)]), workspace="default" + ) + + await asyncio.sleep(0.1) # give any un-cancelled sibling time to finish + assert finished == [], "siblings must be cancelled once a member has failed" diff --git a/plugins/nemo-evaluator/tests/api/v2/test_tasks_routes.py b/plugins/nemo-evaluator/tests/api/v2/test_tasks_routes.py index f249c52e7b..d8a71339d7 100644 --- a/plugins/nemo-evaluator/tests/api/v2/test_tasks_routes.py +++ b/plugins/nemo-evaluator/tests/api/v2/test_tasks_routes.py @@ -9,8 +9,6 @@ from __future__ import annotations -from datetime import datetime, timezone - import pytest from fastapi import FastAPI from fastapi.testclient import TestClient @@ -18,68 +16,7 @@ from nemo_evaluator.api.schemas import MetricInline, MetricRef, TaskInput, TaskInputs from nemo_evaluator.api.service.task_service import TaskService from nemo_evaluator.api.v2 import tasks as tasks_routes -from nemo_evaluator.entities import TaskEntity -from nemo_platform_plugin.entities import ListResponse, PaginationInfo -from nemo_platform_plugin.entity_client import NemoEntityConflictError, NemoEntityNotFoundError -from nemo_platform_plugin.filter_ops import FilterOperation - - -class _FakeEntityClient: - def __init__(self) -> None: - self.entities: dict[tuple[str, str, str], TaskEntity] = {} - - async def create(self, entity: TaskEntity) -> TaskEntity: - key = (entity.__entity_type__, entity.workspace, entity.name) - if key in self.entities: - raise NemoEntityConflictError(f"{key} exists") - now = datetime.now(timezone.utc) - entity._id = f"{entity.__entity_type__}-{entity.name}" - entity._created_at = now - entity._updated_at = now - self.entities[key] = entity - return entity - - async def get( - self, entity_type: type[TaskEntity], *, workspace: str, name: str, parent: str | None = None - ) -> TaskEntity: - key = (entity_type.__entity_type__, workspace, name) - if key not in self.entities: - raise NemoEntityNotFoundError(f"{workspace}/{name} not found") - return self.entities[key] - - async def delete( - self, - entity_type: type[TaskEntity], - name: str, - *, - workspace: str, - parent: str | None = None, - expected_db_version: int | None = None, - ) -> None: - key = (entity_type.__entity_type__, workspace, name) - if key not in self.entities: - raise NemoEntityNotFoundError(f"{workspace}/{name} not found") - del self.entities[key] - - async def list( - self, - entity_type: type[TaskEntity], - *, - workspace: str, - filter_operation: FilterOperation | None = None, - sort: str | None = None, - page: int = 1, - page_size: int = 100, - ) -> ListResponse[TaskEntity]: - items = [ - e for (etype, ws, _), e in self.entities.items() if etype == entity_type.__entity_type__ and ws == workspace - ] - return ListResponse( - data=items, - pagination=PaginationInfo( - page=page, page_size=page_size, current_page_size=len(items), total_pages=1, total_results=len(items) - ), - ) +from nemo_platform_plugin.entity_client import NemoEntityConflictError class _FakeMetricService: @@ -94,19 +31,20 @@ async def get_metric(self, workspace: str, name: str) -> object | None: @pytest.fixture -def client() -> TestClient: +def client(entity_store) -> TestClient: app = FastAPI() app.include_router(tasks_routes.router, prefix="/v2/workspaces/{workspace}") - service = TaskService(_FakeEntityClient(), _FakeMetricService()) + service = TaskService(entity_store, _FakeMetricService()) app.dependency_overrides[get_task_service] = lambda: service return TestClient(app) -def _body() -> dict: +def _body(*, intent: str = "Answer the question.", tags: list[str] | None = None) -> dict: return TaskInput( - intent="Answer the question.", + intent=intent, inputs=TaskInputs(instruction="What is 2+2?"), metrics=[MetricRef("default/stored-metric")], + tags=tags or [], ).model_dump(mode="json") @@ -191,3 +129,145 @@ async def delete_task(self, workspace: str, name: str) -> bool: client = TestClient(app) assert client.delete(f"{_BASE}/task-1").status_code == 409 + + +# --- Publishing revisions (POST creates, PUT replaces) ------------------------ + + +def test_create_publishes_revision_one(client: TestClient) -> None: + """Every stored task has a revision from the moment it exists — there is no unpublished head, + so no consumer has to define what one would mean.""" + created = client.post(f"{_BASE}/task-1", json=_body()).json() + assert created["revision"] == 1 + assert created["tags"] == {"latest": 1} + + +def test_put_on_missing_task_creates_it(client: TestClient) -> None: + """Upsert, so a publisher makes one call without first checking existence — that check is both + a round trip and a race between two publishers of the same task.""" + response = client.put(f"{_BASE}/task-1", json=_body()) + assert response.status_code == 201 + assert response.json()["revision"] == 1 + + +def test_put_with_changed_content_publishes_a_new_revision(client: TestClient) -> None: + client.post(f"{_BASE}/task-1", json=_body()) + response = client.put(f"{_BASE}/task-1", json=_body(intent="Do something else.")) + assert response.status_code == 201 + assert response.json()["revision"] == 2 + assert response.json()["tags"]["latest"] == 2 + + +def test_put_with_identical_content_publishes_nothing(client: TestClient) -> None: + """Strict idempotency: PUT twice leaves exactly the same state, and says so with 200.""" + client.post(f"{_BASE}/task-1", json=_body()) + response = client.put(f"{_BASE}/task-1", json=_body()) + assert response.status_code == 200 + assert response.json()["revision"] == 1 + + +def test_put_applies_tags_without_publishing(client: TestClient) -> None: + """Re-PUTting unchanged content is how an existing revision gets tagged.""" + client.post(f"{_BASE}/task-1", json=_body()) + response = client.put(f"{_BASE}/task-1", json=_body(tags=["blessed"])) + assert response.status_code == 200 + assert response.json()["tags"] == {"latest": 1, "blessed": 1} + + +def test_post_on_existing_task_still_conflicts(client: TestClient) -> None: + """POST keeps its guard: a typo'd name must not silently overwrite someone else's task.""" + client.post(f"{_BASE}/task-1", json=_body()) + assert client.post(f"{_BASE}/task-1", json=_body(intent="Different.")).status_code == 409 + + +def test_get_returns_the_current_revision(client: TestClient) -> None: + client.post(f"{_BASE}/task-1", json=_body()) + client.put(f"{_BASE}/task-1", json=_body(intent="Do something else.")) + got = client.get(f"{_BASE}/task-1").json() + assert got["revision"] == 2 + assert got["intent"] == "Do something else." + + +# --- Reading and tagging a specific revision --------------------------------- + + +def test_list_revisions_newest_first(client: TestClient) -> None: + """How a caller discovers what it can pin to.""" + client.post(f"{_BASE}/task-1", json=_body()) + client.put(f"{_BASE}/task-1", json=_body(intent="Second.")) + + revisions = client.get(f"{_BASE}/task-1/revisions").json()["data"] + assert [r["revision"] for r in revisions] == [2, 1] + assert all(len(r["content_hash"]) == 64 for r in revisions) + assert revisions[0]["tags"] == ["latest"] + + +def test_list_revisions_missing_task_returns_404(client: TestClient) -> None: + assert client.get(f"{_BASE}/nope/revisions").status_code == 404 + + +def test_get_by_digest_returns_the_published_content(client: TestClient) -> None: + """The point of pinning: a consumer holding a digest reads what was published, not what is + current.""" + first = client.post(f"{_BASE}/task-1", json=_body()).json() + digest = client.get(f"{_BASE}/task-1/revisions").json()["data"][0]["content_hash"] + client.put(f"{_BASE}/task-1", json=_body(intent="Newer.")) + + pinned = client.get(f"{_BASE}/task-1/revisions/{digest}").json() + assert pinned["intent"] == first["intent"] + assert pinned["revision"] == 1 + assert client.get(f"{_BASE}/task-1").json()["intent"] == "Newer." + + +def test_get_by_tag_resolves(client: TestClient) -> None: + client.post(f"{_BASE}/task-1", json=_body(tags=["blessed"])) + client.put(f"{_BASE}/task-1", json=_body(intent="Newer.")) + assert client.get(f"{_BASE}/task-1/revisions/blessed").json()["revision"] == 1 + + +def test_get_by_unknown_revision_returns_404(client: TestClient) -> None: + client.post(f"{_BASE}/task-1", json=_body()) + assert client.get(f"{_BASE}/task-1/revisions/{'c' * 64}").status_code == 404 + + +def test_tag_an_existing_revision(client: TestClient) -> None: + """Blessing a revision usually happens after it has been evaluated, not at publish time.""" + client.post(f"{_BASE}/task-1", json=_body()) + digest = client.get(f"{_BASE}/task-1/revisions").json()["data"][0]["content_hash"] + client.put(f"{_BASE}/task-1", json=_body(intent="Newer.")) + + tagged = client.put(f"{_BASE}/task-1/tags/blessed", params={"revision": digest}) + assert tagged.status_code == 200 + assert tagged.json()["tags"]["blessed"] == 1 + assert tagged.json()["tags"]["latest"] == 2, "tagging must not disturb latest" + + +def test_cannot_move_latest_by_hand(client: TestClient) -> None: + """``latest`` is machine-managed; moving it would break the forward-only guarantee.""" + client.post(f"{_BASE}/task-1", json=_body()) + digest = client.get(f"{_BASE}/task-1/revisions").json()["data"][0]["content_hash"] + assert client.put(f"{_BASE}/task-1/tags/latest", params={"revision": digest}).status_code == 422 + + +def test_tag_unknown_revision_returns_404(client: TestClient) -> None: + client.post(f"{_BASE}/task-1", json=_body()) + assert client.put(f"{_BASE}/task-1/tags/blessed", params={"revision": "c" * 64}).status_code == 404 + + +def test_concurrent_replace_returns_409_not_500(entity_store) -> None: + """A lost optimistic lock is a retryable client conflict. Before this was mapped it fell to the + catch-all and surfaced as a 500, wrongly implying a server fault.""" + + async def _stale(entity, *, original_name=None): + raise NemoEntityConflictError("modified by another request") + + app = FastAPI() + app.include_router(tasks_routes.router, prefix="/v2/workspaces/{workspace}") + service = TaskService(entity_store, _FakeMetricService()) + app.dependency_overrides[get_task_service] = lambda: service + client = TestClient(app) + + client.post(f"{_BASE}/task-1", json=_body()) + entity_store.update = _stale + + assert client.put(f"{_BASE}/task-1", json=_body(intent="Newer.")).status_code == 409 diff --git a/plugins/nemo-evaluator/tests/api/v2/test_tasksets_routes.py b/plugins/nemo-evaluator/tests/api/v2/test_tasksets_routes.py index eb743eea10..f62cb38c8c 100644 --- a/plugins/nemo-evaluator/tests/api/v2/test_tasksets_routes.py +++ b/plugins/nemo-evaluator/tests/api/v2/test_tasksets_routes.py @@ -9,7 +9,7 @@ from __future__ import annotations -from datetime import datetime, timezone +import hashlib import pytest from fastapi import FastAPI @@ -18,68 +18,7 @@ from nemo_evaluator.api.schemas import TaskRef, TasksetInput from nemo_evaluator.api.service.taskset_service import TasksetService from nemo_evaluator.api.v2 import tasksets as tasksets_routes -from nemo_evaluator.entities import TasksetEntity -from nemo_platform_plugin.entities import ListResponse, PaginationInfo from nemo_platform_plugin.entity_client import NemoEntityConflictError, NemoEntityNotFoundError -from nemo_platform_plugin.filter_ops import FilterOperation - - -class _FakeEntityClient: - def __init__(self) -> None: - self.entities: dict[tuple[str, str, str], TasksetEntity] = {} - - async def create(self, entity: TasksetEntity) -> TasksetEntity: - key = (entity.__entity_type__, entity.workspace, entity.name) - if key in self.entities: - raise NemoEntityConflictError(f"{key} exists") - now = datetime.now(timezone.utc) - entity._id = f"{entity.__entity_type__}-{entity.name}" - entity._created_at = now - entity._updated_at = now - self.entities[key] = entity - return entity - - async def get( - self, entity_type: type[TasksetEntity], *, workspace: str, name: str, parent: str | None = None - ) -> TasksetEntity: - key = (entity_type.__entity_type__, workspace, name) - if key not in self.entities: - raise NemoEntityNotFoundError(f"{workspace}/{name} not found") - return self.entities[key] - - async def delete( - self, - entity_type: type[TasksetEntity], - name: str, - *, - workspace: str, - parent: str | None = None, - expected_db_version: int | None = None, - ) -> None: - key = (entity_type.__entity_type__, workspace, name) - if key not in self.entities: - raise NemoEntityNotFoundError(f"{workspace}/{name} not found") - del self.entities[key] - - async def list( - self, - entity_type: type[TasksetEntity], - *, - workspace: str, - filter_operation: FilterOperation | None = None, - sort: str | None = None, - page: int = 1, - page_size: int = 100, - ) -> ListResponse[TasksetEntity]: - items = [ - e for (etype, ws, _), e in self.entities.items() if etype == entity_type.__entity_type__ and ws == workspace - ] - return ListResponse( - data=items, - pagination=PaginationInfo( - page=page, page_size=page_size, current_page_size=len(items), total_pages=1, total_results=len(items) - ), - ) class _FakeTaskService: @@ -88,20 +27,27 @@ class _FakeTaskService: async def get_task(self, workspace: str, name: str) -> object | None: return object() if name in {"task-a", "task-b"} else None + async def resolve_revision(self, workspace: str, name: str, fragment: str = "latest") -> str: + """A stable per-task digest; raises for an unknown task, as the real service does.""" + if name not in {"task-a", "task-b"}: + raise NemoEntityNotFoundError(f"{workspace}/{name} not found") + return hashlib.sha256(f"{workspace}/{name}".encode()).hexdigest() + @pytest.fixture -def client() -> TestClient: +def client(entity_store) -> TestClient: app = FastAPI() app.include_router(tasksets_routes.router, prefix="/v2/workspaces/{workspace}") - service = TasksetService(_FakeEntityClient(), _FakeTaskService()) + service = TasksetService(entity_store, _FakeTaskService()) app.dependency_overrides[get_taskset_service] = lambda: service return TestClient(app) -def _body() -> dict: +def _body(*, description: str = "A grouping.", members: list[str] | None = None, tags: list[str] | None = None) -> dict: return TasksetInput( - description="A grouping.", - tasks=[TaskRef("task-a"), TaskRef("default/task-b")], + description=description, + tasks=[TaskRef(m) for m in (members or ["task-a", "default/task-b"])], + tags=tags or [], ).model_dump(mode="json") @@ -117,7 +63,9 @@ def test_create_then_get(client: TestClient) -> None: assert got.status_code == 200 body = got.json() assert body["description"] == "A grouping." - assert body["tasks"] == ["task-a", "default/task-b"] # TaskRef serializes to a bare string + # TaskRef serializes to a bare string, and stored membership is workspace-qualified and + # digest-pinned — a bare "task-a" is resolved to an exact revision on write. + assert [t.split("#")[0] for t in body["tasks"]] == ["default/task-a", "default/task-b"] def test_create_rejects_unknown_body_key(client: TestClient) -> None: @@ -199,3 +147,102 @@ async def delete_taskset(self, workspace: str, name: str) -> bool: client = TestClient(app) assert client.delete(f"{_BASE}/ts-1").status_code == 409 + + +# --- Publishing revisions (POST creates, PUT replaces) ------------------------ + + +def test_create_publishes_revision_one(client: TestClient) -> None: + created = client.post(f"{_BASE}/ts-1", json=_body()).json() + assert created["revision"] == 1 + assert created["tags"] == {"latest": 1} + + +def test_members_are_stored_digest_pinned(client: TestClient) -> None: + """A published grouping names exact revisions. Storing '#latest' would let a member republish + silently change what this taskset contains — which is the whole failure this design prevents.""" + body = client.post(f"{_BASE}/ts-1", json=_body()).json() + assert all(len(ref.split("#")[1]) == 64 for ref in body["tasks"]) + + +def test_tag_pinned_member_is_resolved_to_a_digest(client: TestClient) -> None: + """Tags are resolution *inputs*: accepted on the way in, never persisted.""" + body = client.post(f"{_BASE}/ts-1", json=_body(members=["task-a#latest"])).json() + assert "#latest" not in body["tasks"][0] + assert len(body["tasks"][0].split("#")[1]) == 64 + + +def test_put_on_missing_taskset_creates_it(client: TestClient) -> None: + response = client.put(f"{_BASE}/ts-1", json=_body()) + assert response.status_code == 201 + assert response.json()["revision"] == 1 + + +def test_put_with_changed_membership_publishes_a_new_revision(client: TestClient) -> None: + client.post(f"{_BASE}/ts-1", json=_body()) + response = client.put(f"{_BASE}/ts-1", json=_body(members=["task-a"])) + assert response.status_code == 201 + assert response.json()["revision"] == 2 + + +def test_put_with_identical_membership_publishes_nothing(client: TestClient) -> None: + """Idempotent when nothing underneath moved — the member digests resolve the same.""" + client.post(f"{_BASE}/ts-1", json=_body()) + response = client.put(f"{_BASE}/ts-1", json=_body()) + assert response.status_code == 200 + assert response.json()["revision"] == 1 + + +def test_put_applies_tags_without_publishing(client: TestClient) -> None: + client.post(f"{_BASE}/ts-1", json=_body()) + response = client.put(f"{_BASE}/ts-1", json=_body(tags=["blessed"])) + assert response.status_code == 200 + assert response.json()["tags"] == {"latest": 1, "blessed": 1} + + +def test_post_on_existing_taskset_still_conflicts(client: TestClient) -> None: + client.post(f"{_BASE}/ts-1", json=_body()) + assert client.post(f"{_BASE}/ts-1", json=_body(description="Other.")).status_code == 409 + + +# --- Reading and tagging a specific revision --------------------------------- + + +def test_list_revisions_newest_first(client: TestClient) -> None: + client.post(f"{_BASE}/ts-1", json=_body()) + client.put(f"{_BASE}/ts-1", json=_body(members=["task-a"])) + + revisions = client.get(f"{_BASE}/ts-1/revisions").json()["data"] + assert [r["revision"] for r in revisions] == [2, 1] + assert revisions[0]["tags"] == ["latest"] + + +def test_get_by_digest_returns_published_membership(client: TestClient) -> None: + """The reason a dataset is reproducible: a pinned read returns the membership as published.""" + client.post(f"{_BASE}/ts-1", json=_body()) + digest = client.get(f"{_BASE}/ts-1/revisions").json()["data"][0]["content_hash"] + client.put(f"{_BASE}/ts-1", json=_body(members=["task-a"])) + + pinned = client.get(f"{_BASE}/ts-1/revisions/{digest}").json() + assert len(pinned["tasks"]) == 2 + assert len(client.get(f"{_BASE}/ts-1").json()["tasks"]) == 1 + + +def test_tag_an_existing_revision(client: TestClient) -> None: + client.post(f"{_BASE}/ts-1", json=_body()) + digest = client.get(f"{_BASE}/ts-1/revisions").json()["data"][0]["content_hash"] + client.put(f"{_BASE}/ts-1", json=_body(members=["task-a"])) + + tagged = client.put(f"{_BASE}/ts-1/tags/blessed", params={"revision": digest}) + assert tagged.status_code == 200 + assert tagged.json()["tags"] == {"latest": 2, "blessed": 1} + + +def test_cannot_move_latest_by_hand(client: TestClient) -> None: + client.post(f"{_BASE}/ts-1", json=_body()) + digest = client.get(f"{_BASE}/ts-1/revisions").json()["data"][0]["content_hash"] + assert client.put(f"{_BASE}/ts-1/tags/latest", params={"revision": digest}).status_code == 422 + + +def test_list_revisions_missing_taskset_returns_404(client: TestClient) -> None: + assert client.get(f"{_BASE}/nope/revisions").status_code == 404 diff --git a/plugins/nemo-evaluator/tests/conftest.py b/plugins/nemo-evaluator/tests/conftest.py new file mode 100644 index 0000000000..6926410d87 --- /dev/null +++ b/plugins/nemo-evaluator/tests/conftest.py @@ -0,0 +1,163 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Shared test doubles for the evaluator plugin.""" + +from __future__ import annotations + +import math +from datetime import datetime, timedelta, timezone + +import pytest +from nemo_platform_plugin.entities import EntityBase, ListResponse, PaginationInfo +from nemo_platform_plugin.entity_client import NemoEntityConflictError, NemoEntityNotFoundError +from nemo_platform_plugin.filter_ops import LogicalOperation + + +def matches_filter(entity, operation) -> bool: + """Evaluate the AND-of-equality filters services emit. + + Filters are *evaluated*, not ignored: a fake that returned every row would make a revision + lookup that forgot its ``parent`` predicate look correct, and cross-record confusion is exactly + the bug that predicate prevents. + """ + if operation is None: + return True + if isinstance(operation, LogicalOperation): + return all(matches_filter(entity, child) for child in operation.operations) + field = operation.field + actual = entity.parent if field == "parent" else getattr(entity, field.removeprefix("data."), None) + return actual == operation.value + + +class FakeEntityStore: + """In-memory entity store standing in for ``NemoEntitiesClient``. + + Two behaviors are reproduced deliberately, because service logic depends on them: + + - **Parent-scoped uniqueness.** Records key on ``(entity_type, workspace, name, parent)``, which + is what lets revision children of different tasks both be named ``rev.1`` while making a + duplicate ordinal under one parent conflict. + - **Optimistic locking.** ``update`` rejects a write whose ``db_version`` is stale. A fake that + accepted every update would make correct retry logic untestable and broken retry logic look + fine. + - **Copy-on-read and copy-on-write.** Every record crosses the boundary as a deep copy, because + the real client serializes over HTTP and cannot share objects with its caller. A fake that + handed back the stored instance would let a service mutate the store just by touching an + entity it read — so staging changes in memory and writing them later would be + indistinguishable from writing them immediately, and a missing write would still pass. + + Signatures follow the *concrete* client (positional ``name``, optional ``parent``) rather than + the narrower shared protocols, since that is what services actually call. + """ + + def __init__(self) -> None: + self.entities: dict[tuple[str, str, str, str | None], EntityBase] = {} + #: Monotonic tick for creation timestamps. Wall-clock ``now()`` can repeat within a test, + #: which would make ``-created_at`` ordering non-deterministic — the real store's inserts + #: are genuinely ordered, so the fake must be too. + self._tick = 0 + + def _now(self) -> datetime: + self._tick += 1 + return datetime(2026, 1, 1, tzinfo=timezone.utc) + timedelta(seconds=self._tick) + + def _key(self, entity_type, name: str, workspace: str, parent: str | None): + return (entity_type.__entity_type__, workspace, name, parent) + + async def create(self, entity): + key = self._key(type(entity), entity.name, entity.workspace, entity._parent) + if key in self.entities: + raise NemoEntityConflictError(f"{key} exists") + now = self._now() + entity._id = f"{entity.__entity_type__}-{entity.name}" + entity._created_at = now + entity._updated_at = now + entity._db_version = 0 + self.entities[key] = entity.model_copy(deep=True) + return entity.model_copy(deep=True) + + async def get(self, entity_type, name: str, *, workspace: str, parent: str | None = None): + key = self._key(entity_type, name, workspace, parent) + if key not in self.entities: + raise NemoEntityNotFoundError(f"{workspace}/{name} not found") + return self.entities[key].model_copy(deep=True) + + async def update(self, entity, *, original_name: str | None = None): + key = self._key(type(entity), original_name or entity.name, entity.workspace, entity._parent) + stored = self.entities.get(key) + if stored is not None and entity._db_version != stored._db_version: + raise NemoEntityConflictError(f"stale update for {entity.name}") + entity._db_version = (stored._db_version if stored is not None else 0) + 1 + entity._updated_at = self._now() + self.entities[key] = entity.model_copy(deep=True) + return entity.model_copy(deep=True) + + async def delete( + self, + entity_type, + name: str, + *, + workspace: str, + parent: str | None = None, + expected_db_version: int | None = None, + ) -> None: + # ``parent`` is part of the key, not decoration: revision children of different heads share + # the name ``rev.1``, so ignoring it here would delete whichever one was inserted first. + key = self._key(entity_type, name, workspace, parent) + if key not in self.entities: + raise NemoEntityNotFoundError(f"{workspace}/{name} not found") + del self.entities[key] + + async def list( + self, + entity_type, + *, + workspace: str, + filter_operation=None, + sort: str | None = None, + page: int = 1, + page_size: int = 100, + ) -> ListResponse: + items = [ + entity + for (entity_type_name, record_workspace, _, _), entity in self.entities.items() + if entity_type_name == entity_type.__entity_type__ and record_workspace == workspace + ] + items = [entity for entity in items if matches_filter(entity, filter_operation)] + if sort and items: + # Honour ``sort`` rather than ignoring it: services rely on server-side ordering, and a + # fake that returned insertion order would make a wrong ``sort`` argument invisible. + # Nothing to order when the result is empty, and probing the field on a bare ``object`` + # would reject a legitimate sort over an empty page. + field = sort.lstrip("-") + if not hasattr(items[0], field): + raise NotImplementedError(f"fake cannot sort on {field!r}") + items = sorted(items, key=lambda entity: getattr(entity, field), reverse=sort.startswith("-")) + # Totals describe the whole result set, not the page — that distinction is the only way a + # caller can tell a truncated history from a complete one, which is what ``list_revisions`` + # documents. + total_results = len(items) + start = (page - 1) * page_size + items = [entity.model_copy(deep=True) for entity in items[start : start + page_size]] + return ListResponse( + data=items, + pagination=PaginationInfo( + page=page, + page_size=page_size, + current_page_size=len(items), + total_pages=max(1, math.ceil(total_results / page_size)), + total_results=total_results, + ), + ) + + +@pytest.fixture +def entity_store() -> FakeEntityStore: + """A fresh in-memory entity store. + + Exposed as a fixture rather than an importable name because ``from conftest import ...`` is + ambiguous once more than one test root is collected in the same run — the wrong ``conftest`` + module wins and the import fails. + """ + return FakeEntityStore() diff --git a/plugins/nemo-evaluator/tests/integration/test_task_revisions.py b/plugins/nemo-evaluator/tests/integration/test_task_revisions.py new file mode 100644 index 0000000000..31d1b9883e --- /dev/null +++ b/plugins/nemo-evaluator/tests/integration/test_task_revisions.py @@ -0,0 +1,249 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Integration tests for task/taskset revisions through the SDK against a real platform. + +Everything else about revisions is covered by unit tests against an in-memory fake. This suite +exists because that fake has repeatedly been *wrong* in ways that hid real behavior — it initially +ignored filters, ignored ``sort``, and aliased stored and in-memory objects so optimistic-lock +conflicts could never fire. Each of those made correct-looking tests pass over broken assumptions. + +So these tests target exactly the things only a real entity store can confirm: + +- child records are addressable under a parent, and ordinals collide per-parent rather than globally; +- the ``(parent, data.content_hash)`` query really resolves a digest to its revision; +- server-side ``-created_at`` ordering actually returns revisions newest-first; +- deleting a task cascades to its revisions; +- a published revision is immutable in practice: reading a pinned digest returns the old content + after the task has moved on. + +Pure CRUD (no codex/IGW), so it only needs the host subprocess backend. Shares the evaluator-plugin +integration opt-in (``RUN_AGENT_EVAL_INTEGRATION``) and the session-scoped ``subprocess_platform``. +""" + +from __future__ import annotations + +import os +import uuid + +import pytest +from nemo_evaluator.api.schemas import TaskInput, TasksetInput +from nemo_platform import NeMoPlatform + +pytestmark = [ + pytest.mark.integration, + pytest.mark.skipif( + not os.environ.get("RUN_AGENT_EVAL_INTEGRATION"), + reason="opt-in; set RUN_AGENT_EVAL_INTEGRATION=1 to run (spins real nemo services platforms)", + ), +] + +WORKSPACE = "default" + + +def _unique(prefix: str) -> str: + return f"{prefix}-{uuid.uuid4().hex[:8]}" + + +def _task_input(intent: str = "Answer the question.", *, tags: list[str] | None = None) -> TaskInput: + return TaskInput(intent=intent, inputs={"instruction": "What is 2+2?"}, tags=tags or []) + + +def _client(base_url: str) -> NeMoPlatform: + client = NeMoPlatform(base_url=base_url, max_retries=2) + client.workspaces.create(name=WORKSPACE, exist_ok=True) + return client + + +@pytest.mark.timeout(300) +def test_publish_and_read_a_pinned_revision(subprocess_platform: str) -> None: + """The core promise: a digest-pinned read returns what was published, not what is current.""" + client = _client(subprocess_platform) + name = _unique("task") + try: + created = client.evaluator.tasks.create(name, task=_task_input("First."), workspace=WORKSPACE) + assert created.revision == 1 + assert created.tags["latest"] == 1 + + first_digest = client.evaluator.tasks.list_revisions(name, workspace=WORKSPACE).data[0].content_hash + + replaced = client.evaluator.tasks.replace(name, task=_task_input("Second."), workspace=WORKSPACE) + assert replaced.revision == 2 + + pinned = client.evaluator.tasks.retrieve(name, revision=first_digest, workspace=WORKSPACE) + assert pinned.intent == "First." + assert pinned.revision == 1 + assert client.evaluator.tasks.retrieve(name, workspace=WORKSPACE).intent == "Second." + finally: + client.evaluator.tasks.delete(name, workspace=WORKSPACE) + + +@pytest.mark.timeout(300) +def test_republishing_identical_content_cuts_no_revision(subprocess_platform: str) -> None: + """Publish-time dedup against a real store: the ``(parent, content_hash)`` query must find the + existing revision, or every republish would allocate a new ordinal.""" + client = _client(subprocess_platform) + name = _unique("task") + try: + client.evaluator.tasks.create(name, task=_task_input("Same."), workspace=WORKSPACE) + again = client.evaluator.tasks.replace(name, task=_task_input("Same."), workspace=WORKSPACE) + + assert again.revision == 1 + assert client.evaluator.tasks.list_revisions(name, workspace=WORKSPACE).pagination.total_results == 1 + finally: + client.evaluator.tasks.delete(name, workspace=WORKSPACE) + + +@pytest.mark.timeout(300) +def test_revisions_come_back_newest_first(subprocess_platform: str) -> None: + """Ordering is server-side (``-created_at``); the fake ignored ``sort`` entirely at first, so + this is the only place the real ordering is confirmed.""" + client = _client(subprocess_platform) + name = _unique("task") + try: + client.evaluator.tasks.create(name, task=_task_input("One."), workspace=WORKSPACE) + client.evaluator.tasks.replace(name, task=_task_input("Two."), workspace=WORKSPACE) + client.evaluator.tasks.replace(name, task=_task_input("Three."), workspace=WORKSPACE) + + page = client.evaluator.tasks.list_revisions(name, workspace=WORKSPACE) + assert [r.revision for r in page.data] == [3, 2, 1] + finally: + client.evaluator.tasks.delete(name, workspace=WORKSPACE) + + +@pytest.mark.timeout(300) +def test_ordinals_are_scoped_per_task(subprocess_platform: str) -> None: + """Two tasks each own a ``rev.1``. Parent-scoped uniqueness is what allows that; without it the + second task's first publish would collide on the name.""" + client = _client(subprocess_platform) + first, second = _unique("task-a"), _unique("task-b") + try: + a = client.evaluator.tasks.create(first, task=_task_input("A."), workspace=WORKSPACE) + b = client.evaluator.tasks.create(second, task=_task_input("B."), workspace=WORKSPACE) + assert a.revision == b.revision == 1 + + a_digest = client.evaluator.tasks.list_revisions(first, workspace=WORKSPACE).data[0].content_hash + b_digest = client.evaluator.tasks.list_revisions(second, workspace=WORKSPACE).data[0].content_hash + assert a_digest != b_digest + finally: + client.evaluator.tasks.delete(first, workspace=WORKSPACE) + client.evaluator.tasks.delete(second, workspace=WORKSPACE) + + +@pytest.mark.timeout(300) +def test_identical_content_under_two_tasks_does_not_cross_resolve(subprocess_platform: str) -> None: + """Same content under two tasks yields one digest, so the digest alone cannot identify a + revision — resolution must be parent-scoped. Only a real store proves the query is.""" + client = _client(subprocess_platform) + first, second = _unique("task-a"), _unique("task-b") + try: + client.evaluator.tasks.create(first, task=_task_input("Shared."), workspace=WORKSPACE) + client.evaluator.tasks.create(second, task=_task_input("Shared."), workspace=WORKSPACE) + + a_digest = client.evaluator.tasks.list_revisions(first, workspace=WORKSPACE).data[0].content_hash + b_digest = client.evaluator.tasks.list_revisions(second, workspace=WORKSPACE).data[0].content_hash + assert a_digest == b_digest, "identical content must digest identically" + + # Each resolves under its own parent, and neither leaks the other's record. + assert client.evaluator.tasks.retrieve(first, revision=a_digest, workspace=WORKSPACE).name == first + assert client.evaluator.tasks.retrieve(second, revision=b_digest, workspace=WORKSPACE).name == second + finally: + client.evaluator.tasks.delete(first, workspace=WORKSPACE) + client.evaluator.tasks.delete(second, workspace=WORKSPACE) + + +@pytest.mark.timeout(300) +def test_tagging_an_older_revision_leaves_latest_alone(subprocess_platform: str) -> None: + client = _client(subprocess_platform) + name = _unique("task") + try: + client.evaluator.tasks.create(name, task=_task_input("First."), workspace=WORKSPACE) + first_digest = client.evaluator.tasks.list_revisions(name, workspace=WORKSPACE).data[0].content_hash + client.evaluator.tasks.replace(name, task=_task_input("Second."), workspace=WORKSPACE) + + tagged = client.evaluator.tasks.tag(name, "blessed", revision=first_digest, workspace=WORKSPACE) + + assert tagged.tags["blessed"] == 1 + assert tagged.tags["latest"] == 2, "latest is machine-managed and must not follow a manual tag" + assert client.evaluator.tasks.retrieve(name, revision="blessed", workspace=WORKSPACE).intent == "First." + finally: + client.evaluator.tasks.delete(name, workspace=WORKSPACE) + + +@pytest.mark.timeout(300) +def test_deleting_a_task_removes_its_revisions(subprocess_platform: str) -> None: + """Cascade is a DB-level FK behavior, so it can only be confirmed against real persistence.""" + client = _client(subprocess_platform) + name = _unique("task") + client.evaluator.tasks.create(name, task=_task_input("One."), workspace=WORKSPACE) + client.evaluator.tasks.replace(name, task=_task_input("Two."), workspace=WORKSPACE) + + client.evaluator.tasks.delete(name, workspace=WORKSPACE) + + # Recreating under the same name starts from revision 1 — the old children are gone, so the + # ordinal is free. A surviving `rev.1` would make this publish conflict. + recreated = client.evaluator.tasks.create(name, task=_task_input("Fresh."), workspace=WORKSPACE) + try: + assert recreated.revision == 1 + assert client.evaluator.tasks.list_revisions(name, workspace=WORKSPACE).pagination.total_results == 1 + finally: + client.evaluator.tasks.delete(name, workspace=WORKSPACE) + + +@pytest.mark.timeout(300) +def test_taskset_membership_is_pinned_and_stays_pinned(subprocess_platform: str) -> None: + """The reproducibility guarantee, end to end: a published taskset keeps naming the member + revision it was created with, even after that member publishes new content.""" + client = _client(subprocess_platform) + task_name, set_name = _unique("task"), _unique("ts") + try: + client.evaluator.tasks.create(task_name, task=_task_input("Original."), workspace=WORKSPACE) + + created = client.evaluator.tasksets.create( + set_name, taskset=TasksetInput(tasks=[task_name]), workspace=WORKSPACE + ) + member = created.tasks[0].root + assert "#" in member, "membership must be stored digest-pinned" + pinned_digest = member.split("#", 1)[1] + # A fragment alone only proves *some* sub-entity was named. Pin it to the member's actual + # content digest, which is what makes the reference content-addressed rather than a label. + task_revisions = client.evaluator.tasks.list_revisions(task_name, workspace=WORKSPACE) + assert pinned_digest == task_revisions.data[0].content_hash + + # The member moves on; the published taskset must not. + client.evaluator.tasks.replace(task_name, task=_task_input("Updated."), workspace=WORKSPACE) + + assert client.evaluator.tasksets.retrieve(set_name, workspace=WORKSPACE).tasks[0].root == member + assert ( + client.evaluator.tasks.retrieve(task_name, revision=pinned_digest, workspace=WORKSPACE).intent + == "Original." + ) + finally: + client.evaluator.tasksets.delete(set_name, workspace=WORKSPACE) + client.evaluator.tasks.delete(task_name, workspace=WORKSPACE) + + +@pytest.mark.timeout(300) +def test_republishing_a_taskset_after_a_member_moves_cuts_a_revision(subprocess_platform: str) -> None: + """Members re-resolve on every write, so identical member *names* can still be a real change. + This is the semantic that is hardest to change later, so it is worth pinning against a real + store rather than only against a fake.""" + client = _client(subprocess_platform) + task_name, set_name = _unique("task"), _unique("ts") + try: + client.evaluator.tasks.create(task_name, task=_task_input("v1."), workspace=WORKSPACE) + created = client.evaluator.tasksets.create( + set_name, taskset=TasksetInput(tasks=[task_name]), workspace=WORKSPACE + ) + assert created.revision == 1 + + client.evaluator.tasks.replace(task_name, task=_task_input("v2."), workspace=WORKSPACE) + republished = client.evaluator.tasksets.replace( + set_name, taskset=TasksetInput(tasks=[task_name]), workspace=WORKSPACE + ) + + assert republished.revision == 2, "the grouping names different content, so it is a new revision" + assert republished.tasks[0].root != created.tasks[0].root + finally: + client.evaluator.tasksets.delete(set_name, workspace=WORKSPACE) + client.evaluator.tasks.delete(task_name, workspace=WORKSPACE) diff --git a/plugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.py b/plugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.py index f09f7a99f3..579f948044 100644 --- a/plugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.py +++ b/plugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.py @@ -9,7 +9,7 @@ from typing import Any from unittest.mock import AsyncMock, MagicMock -from nemo_evaluator.api.schemas import MetricRef, Task, TaskInput +from nemo_evaluator.api.schemas import MetricRef, Revision, Task, TaskInput from nemo_evaluator.sdk.task_resources import AsyncEvaluatorTasksResource, EvaluatorTasksResource _BASE = "http://localhost:8080/apis/evaluator/v2/workspaces/default" @@ -24,6 +24,8 @@ def _task_payload(name: str) -> dict[str, Any]: intent="Answer the question.", inputs={"instruction": "What is 2+2?"}, metrics=[MetricRef("default/stored-metric")], + revision=1, + tags={"latest": 1}, created_at=now, updated_at=now, ).model_dump(mode="json") @@ -118,3 +120,137 @@ async def test_async_retrieve_parses_dto() -> None: assert isinstance(result, Task) assert result.name == "task-9" assert http_client.get.call_args[0][0] == f"{_BASE}/tasks/task-9" + + +# --- Revision-aware resources ------------------------------------------------- + + +def _revision_payload(ordinal: int, digest: str) -> dict[str, Any]: + return Revision( + revision=ordinal, + content_hash=digest, + tags=["latest"] if ordinal == 2 else [], + created_at=datetime.now(timezone.utc), + ).model_dump(mode="json") + + +def test_sync_replace_puts_task_input_to_item_url() -> None: + http_client = MagicMock() + http_client.put.return_value = _response(_task_payload("task-1")) + resource = EvaluatorTasksResource(_platform(http_client)) + + result = resource.replace("task-1", task=_task_input()) + + assert http_client.put.call_args.args[0] == f"{_BASE}/tasks/task-1" + assert http_client.put.call_args.kwargs["json"]["intent"] == "Answer." + assert isinstance(result, Task) + + +def test_sync_replace_passes_project_through() -> None: + http_client = MagicMock() + http_client.put.return_value = _response(_task_payload("task-1")) + resource = EvaluatorTasksResource(_platform(http_client)) + + resource.replace("task-1", task=_task_input(), project="proj-a") + + assert http_client.put.call_args.kwargs["params"] == {"project": "proj-a"} + + +def test_sync_retrieve_without_revision_targets_the_item_url() -> None: + http_client = MagicMock() + http_client.get.return_value = _response(_task_payload("task-1")) + resource = EvaluatorTasksResource(_platform(http_client)) + + resource.retrieve("task-1") + + assert http_client.get.call_args.args[0] == f"{_BASE}/tasks/task-1" + + +def test_sync_retrieve_with_revision_targets_the_revision_sub_path() -> None: + """The revision is a path segment, matching the tags route rather than a query parameter.""" + http_client = MagicMock() + http_client.get.return_value = _response(_task_payload("task-1")) + resource = EvaluatorTasksResource(_platform(http_client)) + digest = "a" * 64 + + resource.retrieve("task-1", revision=digest) + + assert http_client.get.call_args.args[0] == f"{_BASE}/tasks/task-1/revisions/{digest}" + + +def test_sync_list_revisions_parses_the_page() -> None: + http_client = MagicMock() + http_client.get.return_value = _response( + { + "data": [_revision_payload(2, "b" * 64), _revision_payload(1, "a" * 64)], + "pagination": { + "page": 1, + "page_size": 100, + "current_page_size": 2, + "total_pages": 1, + "total_results": 2, + }, + } + ) + resource = EvaluatorTasksResource(_platform(http_client)) + + page = resource.list_revisions("task-1") + + assert http_client.get.call_args.args[0] == f"{_BASE}/tasks/task-1/revisions" + assert [r.revision for r in page.data] == [2, 1] + assert page.data[0].content_hash == "b" * 64 + + +def test_sync_tag_puts_to_the_tag_url_with_the_revision() -> None: + http_client = MagicMock() + http_client.put.return_value = _response(_task_payload("task-1")) + resource = EvaluatorTasksResource(_platform(http_client)) + digest = "a" * 64 + + resource.tag("task-1", "blessed", revision=digest) + + assert http_client.put.call_args.args[0] == f"{_BASE}/tasks/task-1/tags/blessed" + assert http_client.put.call_args.kwargs["params"] == {"revision": digest} + + +def test_sync_tag_escapes_the_tag_name() -> None: + """Tag names reach the URL as a path segment; anything needing escaping must be escaped.""" + http_client = MagicMock() + http_client.put.return_value = _response(_task_payload("task-1")) + resource = EvaluatorTasksResource(_platform(http_client)) + + resource.tag("task-1", "release/v1", revision="a" * 64) + + assert http_client.put.call_args.args[0] == f"{_BASE}/tasks/task-1/tags/release%2Fv1" + + +async def test_async_replace_puts_task_input() -> None: + http_client = MagicMock() + http_client.put = AsyncMock(return_value=_response(_task_payload("task-1"))) + resource = AsyncEvaluatorTasksResource(_platform(http_client)) + + result = await resource.replace("task-1", task=_task_input()) + + assert http_client.put.call_args.args[0] == f"{_BASE}/tasks/task-1" + assert isinstance(result, Task) + + +async def test_async_retrieve_with_revision_targets_the_revision_sub_path() -> None: + http_client = MagicMock() + http_client.get = AsyncMock(return_value=_response(_task_payload("task-1"))) + resource = AsyncEvaluatorTasksResource(_platform(http_client)) + digest = "a" * 64 + + await resource.retrieve("task-1", revision=digest) + + assert http_client.get.call_args.args[0] == f"{_BASE}/tasks/task-1/revisions/{digest}" + + +async def test_async_tag_puts_to_the_tag_url() -> None: + http_client = MagicMock() + http_client.put = AsyncMock(return_value=_response(_task_payload("task-1"))) + resource = AsyncEvaluatorTasksResource(_platform(http_client)) + + await resource.tag("task-1", "blessed", revision="a" * 64) + + assert http_client.put.call_args.args[0] == f"{_BASE}/tasks/task-1/tags/blessed" diff --git a/plugins/nemo-evaluator/tests/sdk/test_taskset_sdk_resources.py b/plugins/nemo-evaluator/tests/sdk/test_taskset_sdk_resources.py index ba98ace269..0d94d86866 100644 --- a/plugins/nemo-evaluator/tests/sdk/test_taskset_sdk_resources.py +++ b/plugins/nemo-evaluator/tests/sdk/test_taskset_sdk_resources.py @@ -9,7 +9,7 @@ from typing import Any from unittest.mock import AsyncMock, MagicMock -from nemo_evaluator.api.schemas import TaskRef, Taskset, TasksetInput +from nemo_evaluator.api.schemas import Revision, TaskRef, Taskset, TasksetInput from nemo_evaluator.sdk.taskset_resources import AsyncEvaluatorTasksetsResource, EvaluatorTasksetsResource _BASE = "http://localhost:8080/apis/evaluator/v2/workspaces/default" @@ -23,6 +23,8 @@ def _taskset_payload(name: str) -> dict[str, Any]: workspace="default", description="A grouping.", tasks=[TaskRef("default/task-a")], + revision=1, + tags={"latest": 1}, created_at=now, updated_at=now, ).model_dump(mode="json") @@ -117,3 +119,104 @@ async def test_async_retrieve_parses_dto() -> None: assert isinstance(result, Taskset) assert result.name == "ts-9" assert http_client.get.call_args[0][0] == f"{_BASE}/tasksets/ts-9" + + +# --- Revision-aware resources ------------------------------------------------- + + +def _revision_payload(ordinal: int, digest: str) -> dict[str, Any]: + return Revision(revision=ordinal, content_hash=digest, tags=[], created_at=datetime.now(timezone.utc)).model_dump( + mode="json" + ) + + +def test_sync_replace_puts_taskset_input_to_item_url() -> None: + http_client = MagicMock() + http_client.put.return_value = _response(_taskset_payload("ts-1")) + resource = EvaluatorTasksetsResource(_platform(http_client)) + + result = resource.replace("ts-1", taskset=_taskset_input()) + + assert http_client.put.call_args.args[0] == f"{_BASE}/tasksets/ts-1" + assert isinstance(result, Taskset) + + +def test_sync_retrieve_with_revision_targets_the_revision_sub_path() -> None: + http_client = MagicMock() + http_client.get.return_value = _response(_taskset_payload("ts-1")) + resource = EvaluatorTasksetsResource(_platform(http_client)) + digest = "a" * 64 + + resource.retrieve("ts-1", revision=digest) + + assert http_client.get.call_args.args[0] == f"{_BASE}/tasksets/ts-1/revisions/{digest}" + + +def test_sync_list_revisions_requests_a_page() -> None: + http_client = MagicMock() + http_client.get.return_value = _response( + { + "data": [_revision_payload(1, "a" * 64)], + "pagination": { + "page": 2, + "page_size": 50, + "current_page_size": 1, + "total_pages": 2, + "total_results": 51, + }, + } + ) + resource = EvaluatorTasksetsResource(_platform(http_client)) + + page = resource.list_revisions("ts-1", page=2, page_size=50) + + assert http_client.get.call_args.args[0] == f"{_BASE}/tasksets/ts-1/revisions" + assert http_client.get.call_args.kwargs["params"] == {"page": 2, "page_size": 50} + # The envelope is carried through so a caller can tell a truncated history from a complete one. + assert page.pagination is not None and page.pagination.total_results == 51 + + +def test_sync_tag_puts_to_the_tag_url_with_the_revision() -> None: + http_client = MagicMock() + http_client.put.return_value = _response(_taskset_payload("ts-1")) + resource = EvaluatorTasksetsResource(_platform(http_client)) + digest = "a" * 64 + + resource.tag("ts-1", "blessed", revision=digest) + + assert http_client.put.call_args.args[0] == f"{_BASE}/tasksets/ts-1/tags/blessed" + assert http_client.put.call_args.kwargs["params"] == {"revision": digest} + + +async def test_async_replace_puts_taskset_input() -> None: + http_client = MagicMock() + http_client.put = AsyncMock(return_value=_response(_taskset_payload("ts-1"))) + resource = AsyncEvaluatorTasksetsResource(_platform(http_client)) + + result = await resource.replace("ts-1", taskset=_taskset_input()) + + assert http_client.put.call_args.args[0] == f"{_BASE}/tasksets/ts-1" + assert isinstance(result, Taskset) + + +async def test_async_list_revisions_parses_the_page() -> None: + http_client = MagicMock() + http_client.get = AsyncMock( + return_value=_response( + { + "data": [_revision_payload(1, "a" * 64)], + "pagination": { + "page": 1, + "page_size": 100, + "current_page_size": 1, + "total_pages": 1, + "total_results": 1, + }, + } + ) + ) + resource = AsyncEvaluatorTasksetsResource(_platform(http_client)) + + page = await resource.list_revisions("ts-1") + + assert [r.revision for r in page.data] == [1] diff --git a/plugins/nemo-evaluator/tests/test_content_hash.py b/plugins/nemo-evaluator/tests/test_content_hash.py new file mode 100644 index 0000000000..6dd082ec48 --- /dev/null +++ b/plugins/nemo-evaluator/tests/test_content_hash.py @@ -0,0 +1,225 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Canonical content-hash tests. + +The digest is what a consumer recomputes when reading a pinned ref, so these tests pin the +properties that make that comparison meaningful: determinism, insensitivity to identity and to +server-owned fields, and — the part most likely to break quietly — that near-miss content +variations hash *differently* rather than collapsing onto one digest. +""" + +from __future__ import annotations + +import hashlib +import json +import re +from typing import ClassVar + +from nemo_evaluator.api.schemas import MetadataItem, MetricRef, TaskInputs, TaskRef +from nemo_evaluator.content_hash import DIGEST_PATTERN, canonical_payload, content_hash +from nemo_evaluator.entities import TaskEntity, TasksetEntity +from nemo_evaluator_sdk.agent_eval.tasks import SemanticReducer, SemanticView, ViewSignal +from nemo_platform_plugin.entities import EntityBase +from pydantic import Field + +_DEFAULT_VIEWS = { + "correctness": SemanticView( + reducer=SemanticReducer.SINGLE, + signals=[ViewSignal(metric="exact-match", output="score")], + ) +} +_DEFAULT_METADATA = [MetadataItem(key="suite", value="smoke")] + + +def _task( + *, + name: str = "task-1", + workspace: str = "default", + project: str | None = None, + intent: str = "Answer the question.", + inputs: TaskInputs | None = None, + metrics: list[MetricRef] | None = None, + views: dict[str, SemanticView] | None = None, + metadata: list[MetadataItem] | None = None, +) -> TaskEntity: + return TaskEntity( + name=name, + workspace=workspace, + project=project, + intent=intent, + inputs=inputs if inputs is not None else TaskInputs(instruction="What is 2+2?"), + metrics=metrics if metrics is not None else [MetricRef("default/stored-metric")], + views=views if views is not None else _DEFAULT_VIEWS, + metadata=metadata if metadata is not None else _DEFAULT_METADATA, + ) + + +# --- Shape ------------------------------------------------------------------- + + +def test_digest_is_full_length_lowercase_hex() -> None: + """Never truncated: a shortened prefix would collapse the birthday bound from 2**128.""" + assert re.match(DIGEST_PATTERN, content_hash(_task())) + + +def test_digest_matches_sha256_of_canonical_payload() -> None: + """The payload is the compatibility contract; the digest is just its SHA-256.""" + entity = _task() + expected = hashlib.sha256(canonical_payload(entity).encode("utf-8")).hexdigest() + assert content_hash(entity) == expected + + +def test_canonical_payload_is_compact_and_key_sorted() -> None: + payload = canonical_payload(_task()) + assert ", " not in payload and '": ' not in payload + keys = list(json.loads(payload).keys()) + assert keys == sorted(keys) + + +# --- Determinism ------------------------------------------------------------- + + +def test_identical_content_hashes_identically() -> None: + """The property publish-time dedup depends on: republishing the same content is a no-op.""" + assert content_hash(_task()) == content_hash(_task()) + + +def test_mapping_insertion_order_does_not_affect_digest() -> None: + """Canonicalization sorts mapping keys, so two equal mappings built in different orders are one + piece of content — otherwise republishing an unchanged task would cut a spurious revision. + + ``views`` is the mapping to test this with. ``metadata`` is a *list*, where order is significant + and genuinely changes the digest (see the ordering test below). + """ + first = SemanticView(reducer=SemanticReducer.SINGLE, signals=[ViewSignal(metric="exact-match", output="score")]) + second = SemanticView(reducer=SemanticReducer.SINGLE, signals=[ViewSignal(metric="contains", output="score")]) + + a = _task(views={"correctness": first, "coverage": second}) + b = _task(views={"coverage": second, "correctness": first}) + + assert content_hash(a) == content_hash(b) + + +# --- Identity is not an input ------------------------------------------------ + + +def test_name_and_workspace_do_not_affect_digest() -> None: + """Content-only. Salting with identity would break dedup and defeat verify-on-read: the + recomputed digest would match whenever identity matched, regardless of the content beneath.""" + assert content_hash(_task()) == content_hash(_task(name="task-2", workspace="other")) + + +def test_project_does_not_affect_digest() -> None: + """``project`` is a server-owned base field, not content.""" + assert content_hash(_task()) == content_hash(_task(project="proj-a")) + + +def test_extra_exclude_is_honoured() -> None: + """Revisioned entities exclude their own revision/tag bookkeeping — a revision's digest must + not cover the index that was assigned because of that digest.""" + entity = _task() + assert content_hash(entity, exclude={"metadata"}) != content_hash(entity) + + +# --- Near misses: these must NOT collide ------------------------------------- + + +def test_differing_intent_changes_digest() -> None: + assert content_hash(_task()) != content_hash(_task(intent="Do something else.")) + + +def test_populated_and_empty_metrics_differ() -> None: + """Dropping a task's metrics is a content change, so it must cut a new revision. + + Note this is *not* "absent differs from empty": ``metrics`` defaults to ``[]`` and + ``model_dump`` materializes defaults, so an unset list and an explicitly empty one hash + identically — see :func:`test_absent_field_collapses_onto_its_default`. + """ + assert content_hash(_task(metrics=[])) != content_hash(_task()) + + +def test_metric_ref_order_changes_digest() -> None: + """Sequence order is significant: this function cannot tell a set from an ordered list, so a + set-semantics field must be normalized by its own model before hashing.""" + a = _task(metrics=[MetricRef("default/m-a"), MetricRef("default/m-b")]) + b = _task(metrics=[MetricRef("default/m-b"), MetricRef("default/m-a")]) + assert content_hash(a) != content_hash(b) + + +def test_nested_view_change_changes_digest() -> None: + """Nested sub-models participate; a change buried in a view must not be invisible.""" + changed = _task( + views={ + "correctness": SemanticView( + reducer=SemanticReducer.SINGLE, + signals=[ViewSignal(metric="exact-match", output="other")], + ) + } + ) + assert content_hash(_task()) != content_hash(changed) + + +def test_empty_and_populated_metadata_value_differ() -> None: + a = _task(metadata=[MetadataItem(key="suite", value="smoke")]) + b = _task(metadata=[MetadataItem(key="suite", value="")]) + assert content_hash(a) != content_hash(b) + + +def test_absent_field_collapses_onto_its_default() -> None: + """Documented behavior, asserted so a future change to it is deliberate: ``model_dump`` + materializes defaults, so "unset" and "set to the default" are indistinguishable. A model + needing that distinction must express it (e.g. an optional defaulting to ``None``).""" + assert content_hash(_task(inputs=TaskInputs())) == content_hash(_task(inputs=TaskInputs(instruction=None))) + + +class _NumericEntity(EntityBase): + """Local entity for canonicalization properties the real task schemas can't express. + + ``TaskInputs`` is ``extra="forbid"`` with a single string field, so no current entity carries a + number. The canonicalizer is schema-agnostic, so exercise it directly rather than not at all. + """ + + __entity_type__: ClassVar[str] = "test_numeric" + + value: float | int = Field(description="A number, to pin JSON numeric rendering.") + + +def test_int_and_float_render_distinctly() -> None: + """``1`` and ``1.0`` are distinguishable values, and JSON renders them distinctly.""" + assert content_hash(_NumericEntity(name="n", workspace="default", value=1)) != content_hash( + _NumericEntity(name="n", workspace="default", value=1.0) + ) + + +# --- Tasksets ---------------------------------------------------------------- + + +def _taskset( + *, + name: str = "set-1", + workspace: str = "default", + description: str | None = "A grouping.", + tasks: list[TaskRef] | None = None, +) -> TasksetEntity: + return TasksetEntity( + name=name, + workspace=workspace, + description=description, + tasks=tasks if tasks is not None else [TaskRef("default/task-a"), TaskRef("default/task-b")], + metadata=[], + ) + + +def test_taskset_digest_is_content_only() -> None: + assert content_hash(_taskset()) == content_hash(_taskset(name="set-2", workspace="other")) + + +def test_taskset_membership_change_changes_digest() -> None: + """A dataset's identity is its membership: changing a member must change the digest.""" + changed = _taskset(tasks=[TaskRef("default/task-a"), TaskRef("default/task-c")]) + assert content_hash(_taskset()) != content_hash(changed) + + +def test_taskset_description_change_changes_digest() -> None: + assert content_hash(_taskset()) != content_hash(_taskset(description="Different.")) diff --git a/plugins/nemo-evaluator/tests/test_revision_entity.py b/plugins/nemo-evaluator/tests/test_revision_entity.py new file mode 100644 index 0000000000..ca1dffb4bf --- /dev/null +++ b/plugins/nemo-evaluator/tests/test_revision_entity.py @@ -0,0 +1,205 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Revision entity shape and the head/revision digest invariant. + +The load-bearing property here: hashing a *head* record (excluding its revision pointers) must +yield the same digest as hashing the corresponding *revision* (excluding its own digest and +ordinal). Publish depends on it to recognize "this content is already published" — if the two +digests can't be compared, every publish allocates a new revision and dedup never fires. +""" + +from __future__ import annotations + +import re + +import pytest +from nemo_evaluator.api.schemas import MetadataItem, MetricRef, TaskInputs, TaskRef +from nemo_evaluator.content_hash import content_hash +from nemo_evaluator.entities import ( + REVISION_POINTER_FIELDS, + REVISION_SELF_FIELDS, + TaskEntity, + TaskRevisionEntity, + TasksetEntity, + TasksetRevisionEntity, +) +from nemo_platform_plugin.entity_naming import NAME_MAX_LENGTH, NAME_PATTERN +from pydantic import ValidationError + +_DIGEST = "a" * 64 +_OTHER_DIGEST = "b" * 64 + +_INTENT = "Answer the question." +_INPUTS = TaskInputs(instruction="What is 2+2?") +_METRICS = [MetricRef("default/stored-metric")] +_ANNOTATIONS = [MetadataItem(key="suite", value="smoke")] + +_DESCRIPTION = "A grouping." +_MEMBERS = [TaskRef(f"default/task-a#{_DIGEST}")] + + +def _task_head(*, intent: str = _INTENT, latest_revision: int = 0, tags: dict[str, int] | None = None) -> TaskEntity: + return TaskEntity( + name="task-1", + workspace="default", + intent=intent, + inputs=_INPUTS, + metrics=_METRICS, + metadata=_ANNOTATIONS, + latest_revision=latest_revision, + tags=tags or {}, + ) + + +def _task_revision(*, intent: str = _INTENT, revision: int = 1, digest: str = _DIGEST) -> TaskRevisionEntity: + return TaskRevisionEntity( + name=f"rev.{revision}", + workspace="default", + content_hash=digest, + revision=revision, + intent=intent, + inputs=_INPUTS, + metrics=_METRICS, + metadata=_ANNOTATIONS, + ) + + +def _taskset_head(*, members: list[TaskRef] | None = None) -> TasksetEntity: + return TasksetEntity( + name="set-1", + workspace="default", + description=_DESCRIPTION, + tasks=members if members is not None else _MEMBERS, + ) + + +def _taskset_revision(*, members: list[TaskRef] | None = None) -> TasksetRevisionEntity: + return TasksetRevisionEntity( + name="rev.1", + workspace="default", + content_hash=_DIGEST, + revision=1, + description=_DESCRIPTION, + tasks=members if members is not None else _MEMBERS, + ) + + +# --- The head/revision invariant --------------------------------------------- + + +def test_task_head_and_revision_digests_agree() -> None: + assert content_hash(_task_head(), exclude=REVISION_POINTER_FIELDS) == content_hash( + _task_revision(), exclude=REVISION_SELF_FIELDS + ) + + +def test_taskset_head_and_revision_digests_agree() -> None: + assert content_hash(_taskset_head(), exclude=REVISION_POINTER_FIELDS) == content_hash( + _taskset_revision(), exclude=REVISION_SELF_FIELDS + ) + + +def test_moving_a_tag_does_not_change_the_head_digest() -> None: + """Tags are pointers, not content. If they were digested, every retag would fork history.""" + tagged = _task_head(latest_revision=7, tags={"latest": 7, "candidate": 3}) + assert content_hash(_task_head(), exclude=REVISION_POINTER_FIELDS) == content_hash( + tagged, exclude=REVISION_POINTER_FIELDS + ) + + +def test_ordinal_does_not_change_the_revision_digest() -> None: + """Two revisions of identical content digest identically regardless of when they were cut.""" + assert content_hash(_task_revision(revision=1), exclude=REVISION_SELF_FIELDS) == content_hash( + _task_revision(revision=9), exclude=REVISION_SELF_FIELDS + ) + + +def test_content_change_changes_the_revision_digest() -> None: + assert content_hash(_task_revision(), exclude=REVISION_SELF_FIELDS) != content_hash( + _task_revision(intent="Do something else."), exclude=REVISION_SELF_FIELDS + ) + + +def test_membership_change_changes_the_taskset_revision_digest() -> None: + """A published dataset's identity is its membership — including which revision of each member.""" + repinned = _taskset_revision(members=[TaskRef(f"default/task-a#{_OTHER_DIGEST}")]) + assert content_hash(_taskset_revision(), exclude=REVISION_SELF_FIELDS) != content_hash( + repinned, exclude=REVISION_SELF_FIELDS + ) + + +# --- Published membership must be digest-pinned ------------------------------ + + +@pytest.mark.parametrize( + "unpinned", + [ + "default/task-a", # bare — resolves through `latest` + "task-a", # bare, workspace-relative + "default/task-a#latest", # the reserved moving tag + "default/task-a#candidate", # a user tag, equally mutable + f"default/task-a#{'a' * 63}", # truncated digest is not a digest + f"default/task-a#{'A' * 64}", # uppercase hex + ], +) +def test_published_taskset_rejects_unpinned_members(unpinned: str) -> None: + """Enforced on the field, not in the publish path, so no writer can bypass it. A tag-pinned + member would silently re-point the published revision the moment that tag moved.""" + with pytest.raises(ValidationError): + _taskset_revision(members=[TaskRef(unpinned)]) + + +def test_published_taskset_accepts_digest_pinned_members() -> None: + assert _taskset_revision(members=[TaskRef(f"other/task-a#{_OTHER_DIGEST}")]).tasks[0].root.endswith(_OTHER_DIGEST) + + +def test_head_taskset_still_accepts_unpinned_members() -> None: + """Only *published* membership must be pinned. The head is a working record; a bare ref there + means `latest`, which is exactly what publish resolves.""" + assert _taskset_head(members=[TaskRef("default/task-a")]).tasks[0].root == "default/task-a" + + +# --- Digest field validation ------------------------------------------------- + + +@pytest.mark.parametrize( + "bad", + [ + "a" * 63, # too short + "a" * 65, # too long + "A" * 64, # uppercase + "g" * 64, # non-hex + f"sha256:{'a' * 57}", # algorithm-prefixed + ], +) +def test_revision_rejects_malformed_digest(bad: str) -> None: + """The digest is what a consumer compares against on read; a malformed one must not persist.""" + with pytest.raises(ValidationError): + _task_revision(digest=bad) + + +def test_revision_ordinal_is_one_based() -> None: + with pytest.raises(ValidationError): + _task_revision(revision=0) + + +# --- Naming ------------------------------------------------------------------ + + +def test_ordinal_names_are_legal_entity_names() -> None: + """``rev.`` exists because the entity-name rules reject the alternatives.""" + for ordinal in (1, 9, 10, 12345): + assert re.match(NAME_PATTERN, f"rev.{ordinal}") + + +def test_a_full_digest_is_not_a_legal_entity_name() -> None: + """Why revisions aren't named by digest: 64 chars exceeds the cap, and a hex digest usually + starts with a digit while names must start with a lowercase letter.""" + assert len(_DIGEST) > NAME_MAX_LENGTH + assert not re.match(NAME_PATTERN, "0" + "a" * 63) + + +def test_bare_ordinal_is_not_a_legal_entity_name() -> None: + """Why the ``rev.`` prefix exists rather than naming a revision ``1``.""" + assert not re.match(NAME_PATTERN, "1") diff --git a/plugins/nemo-evaluator/tests/test_revisions.py b/plugins/nemo-evaluator/tests/test_revisions.py new file mode 100644 index 0000000000..4ce5574d8a --- /dev/null +++ b/plugins/nemo-evaluator/tests/test_revisions.py @@ -0,0 +1,738 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Revision publishing and resolution. + +Exercised against an in-memory store that reproduces the two entity-store behaviors this logic +leans on: parent-scoped name uniqueness (which is what serializes concurrent ordinal allocation) +and conflict-on-duplicate-create. +""" + +from __future__ import annotations + +from collections.abc import Awaitable, Callable +from datetime import datetime, timedelta, timezone +from types import SimpleNamespace +from typing import TypeVar + +import pytest +from nemo_evaluator.api.schemas import LATEST_TAG, MetricRef, TaskInputs, TaskRef +from nemo_evaluator.entities import TaskEntity, TaskRevisionEntity +from nemo_evaluator.revisions import ( + RevisionConflictError, + RevisionContentMismatchError, + RevisionNotFoundError, + apply_tag, + get_revision, + head_digest, + publish_revision, + revision_name, +) +from nemo_platform_plugin.entities import EntityBase +from nemo_platform_plugin.entity_client import NemoEntityConflictError, NemoEntityNotFoundError +from nemo_platform_plugin.filter_ops import FilterOperator, LogicalOperation + +_E = TypeVar("_E", bound=EntityBase) + + +class FakeStore: + """In-memory stand-in keyed the way the entity store keys records.""" + + def __init__(self) -> None: + self.records: dict[tuple[str, str, str, str | None], EntityBase] = {} + self._next_id = 0 + #: Monotonic tick for creation timestamps. ``list``/``find_by_digest`` order on + #: ``-created_at``, so leaving it unset would make ordering undefined — and sorting several + #: ``None`` timestamps raises rather than quietly returning insertion order. + self._tick = 0 + #: Ordinals to fail the first create for, simulating a lost allocation race. + self.contend_ordinals: set[int] = set() + #: One-shot hook fired just before a *head* update, modelling a competing publisher that + #: lands in the window between our child create and our pointer write. Lives on the fake + #: rather than a monkeypatch so the race is expressed the same way ``contend_ordinals`` is. + self.before_head_update: Callable[[], Awaitable[None]] | None = None + + def _key(self, entity_type: type[EntityBase], name: str, workspace: str, parent: str | None): + return (entity_type.__entity_type__, workspace, name, parent) + + async def create(self, entity: _E) -> _E: + key = self._key(type(entity), entity.name, entity.workspace, entity._parent) + if key in self.records: + raise NemoEntityConflictError(f"{entity.name} exists") + ordinal = getattr(entity, "revision", None) + if ordinal in self.contend_ordinals: + self.contend_ordinals.discard(ordinal) + self._win_race(entity, ordinal) + raise NemoEntityConflictError(f"{entity.name} taken by a concurrent publisher") + self._next_id += 1 + self._tick += 1 + entity._id = f"id-{self._next_id}" + entity._created_at = datetime(2026, 1, 1, tzinfo=timezone.utc) + timedelta(seconds=self._tick) + self.records[key] = entity.model_copy(deep=True) + return entity + + def _win_race(self, losing_entity, ordinal: int) -> None: + """Model a *complete* competing publish, not just a failed create. + + The winner's record exists under the contended name and the head has advanced. Without + both, the loser re-reads an unchanged head and recomputes the same ordinal — which no real + race would do, and which would make the retry look broken when it isn't. The competitor + publishes *different* content (its own digest), so the loser genuinely needs a new ordinal + rather than discovering its content already published. + """ + winner = losing_entity.model_copy(update={"content_hash": f"{ordinal:064x}"}) + winner._parent = losing_entity._parent + self._next_id += 1 + winner._id = f"id-{self._next_id}" + self.records[self._key(type(winner), winner.name, winner.workspace, winner._parent)] = winner + head = self.records.get(self._key(TaskEntity, "task-1", winner.workspace, None)) + if head is not None: + head.latest_revision = max(head.latest_revision, ordinal) + head.tags = {**head.tags, LATEST_TAG: ordinal} + + async def get(self, entity_type: type[_E], name, *, workspace=None, parent=None) -> _E: + """Hand back a *copy*, as the real client does — it rebuilds entities from an HTTP response + and cannot share objects with its caller. Returning the stored instance would let code + under test mutate the store just by touching what it read.""" + key = self._key(entity_type, name, workspace or "default", parent) + if key not in self.records: + raise NemoEntityNotFoundError(f"{name} not found") + stored = self.records[key] + copy = stored.model_copy(deep=True) + copy._parent, copy._id, copy._db_version = stored._parent, stored._id, stored._db_version + return copy + + async def update(self, entity: _E, *, original_name=None) -> _E: + """Enforce the ``db_version`` optimistic lock the real store enforces. + + A fake that accepts every update makes correct retry logic untestable and incorrect retry + logic look fine, so this rejects a write whose base version is stale. + """ + if self.before_head_update is not None and isinstance(entity, TaskEntity): + hook, self.before_head_update = self.before_head_update, None + await hook() + key = self._key(type(entity), original_name or entity.name, entity.workspace, entity._parent) + stored = self.records.get(key) + if stored is not None and entity._db_version != stored._db_version: + raise NemoEntityConflictError( + f"stale update for {entity.name}: base version {entity._db_version}, " + f"stored version {stored._db_version}" + ) + saved = entity.model_copy(deep=True) + saved._parent = entity._parent + saved._id = entity._id + saved._db_version = (stored._db_version if stored is not None else 0) + 1 + self.records[key] = saved + return saved + + async def delete(self, entity_type, name, *, workspace, parent=None, expected_db_version=None) -> object: + """Part of the standard client surface, so the fake carries it even though publishing and + resolving never delete — a stand-in that omits it would not be substitutable.""" + key = self._key(entity_type, name, workspace, parent) + if key not in self.records: + raise NemoEntityNotFoundError(f"{name} not found") + return self.records.pop(key) + + async def list(self, entity_type, *, workspace, filter_operation=None, sort=None, page=1, page_size=100): + """Evaluate the filter the way the store does, so the query path is actually exercised. + + Supports exactly what this module emits: an AND of equality comparisons over ``parent`` and + ``data.``. Anything else raises rather than silently matching everything — a fake + that ignores filters would make a broken query look correct. ``sort`` and ``page`` are + honoured for the same reason: accepting them and returning insertion order would make a + caller that ordered wrongly look right. + """ + rows = [ + record + for (entity_type_name, record_workspace, _, _), record in self.records.items() + if entity_type_name == entity_type.__entity_type__ and record_workspace == workspace + ] + for comparison in self._comparisons(filter_operation): + if comparison.operator is not FilterOperator.EQ: + raise NotImplementedError(f"fake supports only EQ, got {comparison.operator}") + if comparison.field == "parent": + rows = [r for r in rows if r.parent == comparison.value] + elif comparison.field.startswith("data."): + attribute = comparison.field.removeprefix("data.") + rows = [r for r in rows if getattr(r, attribute, None) == comparison.value] + else: + raise NotImplementedError(f"fake cannot filter on {comparison.field!r}") + if sort and rows: + field = sort.lstrip("-") + if not hasattr(rows[0], field): + raise NotImplementedError(f"fake cannot sort on {field!r}") + rows = sorted(rows, key=lambda record: getattr(record, field), reverse=sort.startswith("-")) + start = (page - 1) * page_size + return SimpleNamespace(data=rows[start : start + page_size]) + + def _comparisons(self, operation): + if operation is None: + return [] + if isinstance(operation, LogicalOperation): + if operation.operator is not FilterOperator.AND: + raise NotImplementedError(f"fake supports only AND, got {operation.operator}") + return [c for child in operation.operations for c in self._comparisons(child)] + return [operation] + + def concurrent_head_write(self, head: EntityBase, *, tags: dict[str, int]) -> None: + """Simulate another publisher committing to the head between our read and our write.""" + key = self._key(type(head), head.name, head.workspace, None) + stored = self.records[key] + stored.tags = {**stored.tags, **tags} + stored.latest_revision = max([stored.latest_revision, *tags.values()], default=stored.latest_revision) + stored._db_version += 1 + + +def _head(store: FakeStore, *, intent: str = "Answer the question.") -> TaskEntity: + head = TaskEntity( + name="task-1", + workspace="default", + intent=intent, + inputs=TaskInputs(instruction="What is 2+2?"), + metrics=[MetricRef("default/stored-metric")], + ) + head._id = "head-1" + head._db_version = 0 + # Store a *distinct* copy: the real client returns freshly deserialized objects, so a caller's + # in-memory head and the stored record are never the same object. Aliasing them would make + # every optimistic-lock conflict invisible (bumping one bumps the other). + stored = head.model_copy(deep=True) + stored._id = head.id + stored._db_version = 0 + store.records[store._key(TaskEntity, "task-1", "default", None)] = stored + return head + + +def _head_named(store: FakeStore, name: str) -> TaskEntity: + """A second record with content identical to :func:`_head`'s — same digest, different parent.""" + head = TaskEntity( + name=name, + workspace="default", + intent="Answer the question.", + inputs=TaskInputs(instruction="What is 2+2?"), + metrics=[MetricRef("default/stored-metric")], + ) + head._id = f"head-{name}" + head._db_version = 0 + stored = head.model_copy(deep=True) + stored._id = head.id + stored._db_version = 0 + store.records[store._key(TaskEntity, name, "default", None)] = stored + return head + + +async def _publish(store: FakeStore, head: TaskEntity, *, tags: set[str] | None = None): + """Publish and drop the returned head — these tests assert against their own head object.""" + revision, _head, created = await publish_revision(store, store, head, TaskRevisionEntity, tags=tags) + return revision, created + + +# --- First publish ----------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_first_publish_creates_revision_one() -> None: + store = FakeStore() + head = _head(store) + revision, created = await _publish(store, head) + assert created + assert revision.revision == 1 + assert revision.name == revision_name(1) + assert revision.content_hash == head_digest(head) + + +@pytest.mark.asyncio +async def test_first_publish_points_latest_at_revision_one() -> None: + store = FakeStore() + head = _head(store) + revision, _ = await _publish(store, head) + assert head.tags[LATEST_TAG] == revision.revision + assert head.latest_revision == 1 + + +@pytest.mark.asyncio +async def test_revision_is_a_child_of_its_head() -> None: + """Parent scoping is what makes ordinals collide instead of silently duplicating.""" + store = FakeStore() + head = _head(store) + revision, _ = await _publish(store, head) + assert revision.parent == head.id + + +# --- Idempotency ------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_republishing_identical_content_creates_nothing() -> None: + """The property that makes a re-publish cheap: same content, same digest, no new revision.""" + store = FakeStore() + head = _head(store) + first, created_first = await _publish(store, head) + second, created_second = await _publish(store, head) + assert created_first and not created_second + assert second.revision == first.revision + assert head.latest_revision == 1 + + +@pytest.mark.asyncio +async def test_republishing_identical_content_still_applies_new_tags() -> None: + """A no-op publish is not a no-op tag operation — that's how you tag an existing revision.""" + store = FakeStore() + head = _head(store) + revision, _ = await _publish(store, head) + _, created = await _publish(store, head, tags={"blessed"}) + assert not created + assert head.tags["blessed"] == revision.revision + + +@pytest.mark.asyncio +async def test_changed_content_allocates_the_next_ordinal() -> None: + store = FakeStore() + head = _head(store) + first, _ = await _publish(store, head) + head.intent = "Do something else." + second, created = await _publish(store, head) + assert created + assert second.revision == 2 + assert second.content_hash != first.content_hash + assert head.tags[LATEST_TAG] == second.revision + assert head.latest_revision == 2 + + +# --- Concurrency ------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_contended_ordinal_is_retried() -> None: + """A publisher that loses the race for ordinal N re-reads and takes N+1 rather than failing.""" + store = FakeStore() + head = _head(store) + await _publish(store, head) + head.intent = "Changed." + store.contend_ordinals = {2} + revision, created = await _publish(store, head) + assert created + assert revision.revision == 3 + + +@pytest.mark.asyncio +async def test_publishing_recovers_when_a_revision_exists_but_the_head_never_advanced() -> None: + """The one case where losing a create does *not* mean someone else advanced the head. + + If a publisher creates ``rev.N`` and then exhausts its pointer-write retries, the child exists + while the head still names ``N-1``. A later publish computes N, loses the create, re-reads the + head — which still says ``N-1`` — and computes N again, every attempt until it gives up. Every + publish after that does the same, so the record would be permanently unpublishable rather than + merely wasteful. The failed create is proof enough that N is taken. + """ + store = FakeStore() + head = _head(store) + await _publish(store, head) + + # Simulate the pointer write never landing: rev.1 exists, the head still names revision 0. + head.latest_revision, head.tags = 0, {} + stored = store.records[store._key(TaskEntity, head.name, head.workspace, None)] + assert isinstance(stored, TaskEntity) + stored.latest_revision, stored.tags = 0, {} + + head.intent = "Changed." + revision, created = await _publish(store, head) + + assert created + assert revision.revision == 2, "must step past the orphaned rev.1 instead of retrying it" + + +@pytest.mark.asyncio +async def test_losing_the_head_race_does_not_leave_the_head_on_an_older_revision() -> None: + """A publisher that loses the *pointer* race must not drag the head's content backwards. + + A creates rev.2 and stalls; B publishes rev.3 and commits first; A's pointer write then loses + the lock and retries. ``latest`` correctly stays at 3 — but if A's staged content still landed + on the head, a plain read would return rev.2's content while ``#latest`` returned rev.3's, and + the record would report itself as revision 3 while serving something else. + """ + store = FakeStore() + head = _head(store) + await publish_revision(store, store, head, TaskRevisionEntity) # rev.1 + + a = await store.get(TaskEntity, name="task-1", workspace="default") + b = await store.get(TaskEntity, name="task-1", workspace="default") + a.intent, b.intent = "A's content.", "B's content." + + async def b_publishes() -> None: + await publish_revision(store, store, b, TaskRevisionEntity) + + store.before_head_update = b_publishes + a_revision, _, _ = await publish_revision(store, store, a, TaskRevisionEntity) + + stored = store.records[store._key(TaskEntity, "task-1", "default", None)] + assert isinstance(stored, TaskEntity) + latest = await get_revision(store, TaskRevisionEntity, stored, LATEST_TAG) + + assert stored.tags[LATEST_TAG] == 3 + assert stored.intent == latest.intent == "B's content." + assert stored.latest_revision == latest.revision, "the reported revision must describe the content served" + + # A's publish is not lost — it is a real revision, still resolvable by digest. + assert a_revision.revision == 2 + pinned = await get_revision(store, TaskRevisionEntity, stored, a_revision.content_hash) + assert pinned.intent == "A's content." + + +@pytest.mark.asyncio +async def test_persistent_contention_raises_rather_than_looping() -> None: + store = FakeStore() + head = _head(store) + store.contend_ordinals = set(range(1, 50)) + with pytest.raises(RevisionConflictError): + await _publish(store, head) + + +@pytest.mark.asyncio +async def test_latest_revision_never_rewinds() -> None: + """Re-tagging an older revision must not hand the next publish an ordinal already in use. + + ``latest_revision`` is the allocation watermark, not a pointer: pointing a user tag backwards + is legitimate, but if that dragged the watermark back with it, the next publish would compute + an ordinal that already exists and lose its create. + """ + store = FakeStore() + head = _head(store) + first, _ = await _publish(store, head) + head.intent = "Changed." + await _publish(store, head) + + head = await store.get(TaskEntity, "task-1", workspace="default") + await apply_tag(store, store, TaskRevisionEntity, head, "rollback", first.content_hash) + + stored = store.records[store._key(TaskEntity, "task-1", "default", None)] + assert stored.tags["rollback"] == first.revision + assert stored.latest_revision == 2, "the watermark must not follow a backwards tag" + assert stored.tags[LATEST_TAG] == 2 + + +@pytest.mark.asyncio +async def test_head_update_retries_on_optimistic_lock_conflict() -> None: + """A publisher that loses the head write re-reads and folds its pointers into the winner's + record, rather than propagating the conflict or overwriting with a stale copy.""" + store = FakeStore() + head = _head(store) + store.concurrent_head_write(head, tags={"other": 1}) + revision, created = await _publish(store, head) + assert created + stored = store.records[store._key(TaskEntity, "task-1", "default", None)] + assert stored.tags["other"] == 1, "the winner's tag must survive our retry" + assert stored.tags[LATEST_TAG] == revision.revision + + +@pytest.mark.asyncio +async def test_persistent_head_contention_raises() -> None: + """Bounded retries: a head that keeps moving must fail loudly, not loop forever.""" + + store = FakeStore() + head = _head(store) + original_update = store.update + + async def always_stale(entity, *, original_name=None): + store.concurrent_head_write(entity, tags={}) + return await original_update(entity, original_name=original_name) + + store.update = always_stale # type: ignore[method-assign] + with pytest.raises(RevisionConflictError): + await _publish(store, head) + + +@pytest.mark.asyncio +async def test_latest_does_not_regress_when_writes_interleave() -> None: + """The interleaving that motivates forward-only ``latest``: A creates rev.1 and B creates + rev.2, but B commits its head pointers first. A's later write must not drag ``latest`` back + onto rev.1 while ``latest_revision`` says 2.""" + store = FakeStore() + head = _head(store) + + # A has staged content and is about to publish as rev.1. B publishes rev.2 and commits its head + # pointers first, so A's pointer write loses the optimistic lock and retries against B's head. + store.concurrent_head_write(head, tags={LATEST_TAG: 2}) + + revision, created = await _publish(store, head) + + assert created and revision.revision == 1 + stored = store.records[store._key(TaskEntity, "task-1", "default", None)] + assert stored.tags[LATEST_TAG] == 2, "latest must not regress onto A's older revision" + assert stored.latest_revision == 2 + + +@pytest.mark.asyncio +async def test_user_tags_may_be_moved_backwards() -> None: + """Only ``latest`` is forward-only. Retagging onto an older revision is a rollback — explicit + user intent, not a race — and must be honoured.""" + store = FakeStore() + head = _head(store) + older, _ = await _publish(store, head, tags={"blessed"}) + head.intent = "Newer content." + await _publish(store, head) + + head = await store.get(TaskEntity, "task-1", workspace="default") + await apply_tag(store, store, TaskRevisionEntity, head, "blessed", older.content_hash) + + stored = store.records[store._key(TaskEntity, "task-1", "default", None)] + assert stored.tags["blessed"] == older.revision + assert stored.tags[LATEST_TAG] == 2, "only the user tag moves; latest stays put" + + +@pytest.mark.asyncio +async def test_reverting_to_earlier_content_publishes_a_new_revision() -> None: + """Dedup is against the *current* revision, not the whole history. + + Reverting is a real change to what the record is now. If it deduped onto the old revision the + head would hold that content while ``latest`` — forward-only — kept naming the newer one, so a + plain read and a ``#latest`` read would disagree about the same record. + """ + store = FakeStore() + head = _head(store) + await _publish(store, head) # rev.1: "Answer the question." + head.intent = "Changed." + await _publish(store, head) # rev.2 + + head.intent = "Answer the question." # back to rev.1's content + revision, created = await _publish(store, head) + + assert created, "a revert is a publish, not a no-op" + assert revision.revision == 3 + assert head.tags[LATEST_TAG] == 3 + + latest = await get_revision(store, TaskRevisionEntity, head, LATEST_TAG) + assert latest.intent == head.intent, "the head and #latest must describe the same content" + + +@pytest.mark.asyncio +async def test_a_digest_shared_by_two_revisions_resolves_to_the_newer_one() -> None: + """Reverting makes one record hold two revisions with the same digest, so a digest lookup has + to pick deterministically. Both carry identical content — all a pin promises — but a pin must + not resolve to a different ordinal from one call to the next.""" + store = FakeStore() + head = _head(store) + first, _ = await _publish(store, head) # rev.1 + head.intent = "Changed." + await _publish(store, head) # rev.2 + head.intent = "Answer the question." + third, _ = await _publish(store, head) # rev.3, same digest as rev.1 + + assert third.content_hash == first.content_hash + + for _ in range(3): + resolved = await get_revision(store, TaskRevisionEntity, head, first.content_hash) + assert resolved.revision == third.revision + + +@pytest.mark.asyncio +async def test_republishing_the_current_content_is_still_a_no_op() -> None: + """The idempotency the narrower dedup must not cost: re-PUTting unchanged content.""" + store = FakeStore() + head = _head(store) + first, _ = await _publish(store, head) + revision, created = await _publish(store, head) + + assert not created + assert revision.revision == first.revision + + +# --- Resolution -------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_resolves_latest_by_default() -> None: + store = FakeStore() + head = _head(store) + await _publish(store, head) + head.intent = "Changed." + second, _ = await _publish(store, head) + assert (await get_revision(store, TaskRevisionEntity, head)).content_hash == second.content_hash + + +@pytest.mark.asyncio +async def test_resolves_a_digest_to_its_revision() -> None: + store = FakeStore() + head = _head(store) + first, _ = await _publish(store, head) + head.intent = "Changed." + await _publish(store, head) + resolved = await get_revision(store, TaskRevisionEntity, head, first.content_hash) + assert resolved.revision == 1 + + +@pytest.mark.asyncio +async def test_resolves_a_user_tag() -> None: + store = FakeStore() + head = _head(store) + revision, _ = await _publish(store, head, tags={"blessed"}) + assert (await get_revision(store, TaskRevisionEntity, head, "blessed")).revision == revision.revision + + +@pytest.mark.asyncio +async def test_unknown_fragment_raises() -> None: + store = FakeStore() + head = _head(store) + await _publish(store, head) + with pytest.raises(RevisionNotFoundError): + await get_revision(store, TaskRevisionEntity, head, "nonexistent") + + +@pytest.mark.asyncio +async def test_index_pointing_at_a_missing_record_raises() -> None: + """A torn write must surface, not resolve to nothing.""" + store = FakeStore() + head = _head(store) + revision, _ = await _publish(store, head) + del store.records[store._key(TaskRevisionEntity, revision.name, "default", head.id)] + with pytest.raises(RevisionNotFoundError): + await get_revision(store, TaskRevisionEntity, head) + + +@pytest.mark.asyncio +async def test_unknown_digest_raises() -> None: + store = FakeStore() + head = _head(store) + await _publish(store, head) + with pytest.raises(RevisionNotFoundError): + await get_revision(store, TaskRevisionEntity, head, "c" * 64) + + +@pytest.mark.asyncio +async def test_digest_resolution_is_scoped_to_the_parent() -> None: + """Identical content under two records yields one digest, so the digest alone does not identify + a revision. Without parent scoping this would resolve to the other record's revision.""" + store = FakeStore() + mine = _head(store) + theirs = _head_named(store, "task-2") + published, _, _ = await publish_revision(store, store, theirs, TaskRevisionEntity) + + assert head_digest(mine) == published.content_hash, "same content, same digest" + with pytest.raises(RevisionNotFoundError): + await get_revision(store, TaskRevisionEntity, mine, published.content_hash) + + +@pytest.mark.asyncio +async def test_identical_content_under_two_records_publishes_independently() -> None: + """The flip side: one record's publish must not make another's look already-published.""" + store = FakeStore() + mine = _head(store) + theirs = _head_named(store, "task-2") + await publish_revision(store, store, theirs, TaskRevisionEntity) + revision, created = await _publish(store, mine) + assert created and revision.revision == 1 + + +@pytest.mark.asyncio +async def test_reading_a_revision_whose_content_was_tampered_with_is_refused() -> None: + """The digest has to be checked on the way out or it is only a label. + + A revision is immutable by convention; the store will still accept a write to one. If reading + trusted the recorded digest, a pinned ref would keep resolving and quietly serve content nobody + pinned — the exact failure content-addressing exists to prevent. + """ + store = FakeStore() + head = _head(store) + revision, _ = await _publish(store, head) + + stored = store.records[store._key(TaskRevisionEntity, revision_name(1), "default", head.id)] + assert isinstance(stored, TaskRevisionEntity) + stored.intent = "Tampered with after publication." + + with pytest.raises(RevisionContentMismatchError, match="does not match its recorded digest"): + await get_revision(store, TaskRevisionEntity, head, LATEST_TAG) + + +@pytest.mark.asyncio +async def test_a_digest_pinned_read_is_verified_too() -> None: + """Both resolution paths verify — a digest lookup filters on the *recorded* digest, so without + re-hashing it would happily return a record whose content no longer matches it.""" + store = FakeStore() + head = _head(store) + revision, _ = await _publish(store, head) + + stored = store.records[store._key(TaskRevisionEntity, revision_name(1), "default", head.id)] + assert isinstance(stored, TaskRevisionEntity) + stored.intent = "Tampered with after publication." + + with pytest.raises(RevisionContentMismatchError): + await get_revision(store, TaskRevisionEntity, head, revision.content_hash) + + +# --- Tag validation ----------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_a_digest_shaped_tag_is_rejected() -> None: + """Such a tag could be stored but never resolved: a digest-shaped reference is looked up as a + digest, so the tag map would never be consulted. Rejected rather than silently useless.""" + store = FakeStore() + head = _head(store) + with pytest.raises(ValueError, match="looks like a content digest"): + await _publish(store, head, tags={"a" * 64}) + + +@pytest.mark.asyncio +async def test_latest_cannot_be_moved_by_hand() -> None: + """Refused where it matters: pointing ``latest`` somewhere would break the forward-only rule.""" + store = FakeStore() + head = _head(store) + revision, _ = await _publish(store, head) + with pytest.raises(ValueError, match="managed automatically"): + await apply_tag(store, store, TaskRevisionEntity, head, LATEST_TAG, revision.content_hash) + + +@pytest.mark.asyncio +async def test_latest_in_publish_tags_is_tolerated() -> None: + """Listing ``latest`` at publish time asks for exactly what the server does anyway, so it is a + no-op rather than an error. Rejecting it would break clients that pass it defensively — which + is what an earlier version of this validation did.""" + store = FakeStore() + head = _head(store) + revision, created = await _publish(store, head, tags={LATEST_TAG, "blessed"}) + assert created + assert head.tags[LATEST_TAG] == revision.revision + assert head.tags["blessed"] == revision.revision + + +@pytest.mark.asyncio +async def test_an_empty_tag_is_rejected() -> None: + """An absent fragment already means ``latest``, so an empty tag could never be addressed.""" + store = FakeStore() + head = _head(store) + with pytest.raises(ValueError, match="must not be empty"): + await _publish(store, head, tags={" "}) + + +@pytest.mark.asyncio +async def test_a_nearly_digest_shaped_tag_is_allowed() -> None: + """The rejection is exact — 64 lowercase hex — so ordinary names stay usable.""" + store = FakeStore() + head = _head(store) + _, created = await _publish(store, head, tags={"a" * 63, "deadbeef"}) + assert created + assert head.tags["deadbeef"] == 1 + + +@pytest.mark.parametrize("tag", ["release/2026", "release candidate"]) +@pytest.mark.asyncio +async def test_a_tag_outside_the_fragment_charset_is_rejected(tag: str) -> None: + """A tag exists to be written after ``#`` in a member reference. ``TaskRef`` admits only + ``[\\w\\-.]+`` there, so a tag with a slash or a space would apply and list cleanly and then be + unusable for the one thing it is for — the same silent dead end as a digest-shaped tag.""" + store = FakeStore() + head = _head(store) + with pytest.raises(ValueError, match="cannot appear in a reference fragment"): + await _publish(store, head, tags={tag}) + + +@pytest.mark.asyncio +async def test_an_accepted_tag_is_usable_as_a_ref_fragment() -> None: + """The other half of the rule: whatever publishing accepts, ``TaskRef`` must also accept. Pins + the two charsets together so neither can drift into rejecting the other's output.""" + store = FakeStore() + head = _head(store) + await _publish(store, head, tags={"blessed", "v1.2.3", "rc_2"}) + + for tag in head.tags: + TaskRef(f"{head.workspace}/{head.name}#{tag}") diff --git a/plugins/nemo-evaluator/tests/test_subentity_refs.py b/plugins/nemo-evaluator/tests/test_subentity_refs.py new file mode 100644 index 0000000000..e26dc7cc71 --- /dev/null +++ b/plugins/nemo-evaluator/tests/test_subentity_refs.py @@ -0,0 +1,99 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Sub-entity (``#fragment``) reference parsing and validation. + +A revision is addressed with the platform's standard ``#`` fragment — the same convention filesets +use for a contained file (``workspace/fileset#path``). These tests pin two things: that an absent +fragment means ``latest`` rather than "unpinned", and that existing fragment-unaware callers keep +working against a pinned ref (``parse_entity_ref`` strips it). +""" + +from __future__ import annotations + +import pytest +from nemo_evaluator.api.schemas import ( + LATEST_TAG, + MetricRef, + TaskRef, + TasksetRef, + parse_entity_ref, + parse_subentity_ref, +) +from pydantic import ValidationError + +_DIGEST = "a" * 64 + + +# --- Parsing ----------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("ref", "expected"), + [ + ("task-a", ("default", "task-a", LATEST_TAG)), + ("other/task-a", ("other", "task-a", LATEST_TAG)), + ("task-a#latest", ("default", "task-a", "latest")), + ("other/task-a#candidate", ("other", "task-a", "candidate")), + (f"other/task-a#{_DIGEST}", ("other", "task-a", _DIGEST)), + ], +) +def test_parse_subentity_ref(ref: str, expected: tuple[str, str, str]) -> None: + assert parse_subentity_ref(ref, "default") == expected + + +def test_absent_fragment_means_latest_not_unpinned() -> None: + """A bare ref is "the current revision", which is what makes `#latest` a real default rather + than a special case the resolver has to guess at.""" + _, _, fragment = parse_subentity_ref("task-a", "default") + assert fragment == LATEST_TAG + + +def test_empty_fragment_falls_back_to_latest() -> None: + """``task-a#`` is degenerate input; treat it as unpinned rather than as a tag named ''.""" + assert parse_subentity_ref("task-a#", "default") == ("default", "task-a", LATEST_TAG) + + +def test_fragment_is_returned_verbatim() -> None: + """Parsing does not decide whether a fragment is a tag or a digest — that's resolution's job.""" + _, _, fragment = parse_subentity_ref(f"task-a#{_DIGEST}", "default") + assert fragment == _DIGEST + + +# --- Backward compatibility -------------------------------------------------- + + +def test_parse_entity_ref_strips_the_fragment() -> None: + """Fragment-unaware callers (metric resolution, taskset member existence checks) keep working + against a pinned ref instead of trying to look up a task literally named 'task-a#'.""" + assert parse_entity_ref(f"other/task-a#{_DIGEST}", "default") == ("other", "task-a") + assert parse_entity_ref("task-a#latest", "default") == ("default", "task-a") + + +def test_pinned_and_bare_refs_resolve_to_the_same_task() -> None: + """The property taskset duplicate-detection relies on: two refs differing only by fragment are + the same member, and must not both be admitted.""" + assert parse_entity_ref(f"task-a#{_DIGEST}", "default") == parse_entity_ref("task-a", "default") + + +# --- Field validation -------------------------------------------------------- + + +@pytest.mark.parametrize("ref", ["task-a", "other/task-a", "task-a#latest", f"other/task-a#{_DIGEST}"]) +def test_task_ref_accepts_fragments(ref: str) -> None: + assert TaskRef(ref).root == ref + + +@pytest.mark.parametrize("ref", ["task-a#one#two", "task-a#bad/frag", "#latest", "task a#latest"]) +def test_task_ref_rejects_malformed_fragments(ref: str) -> None: + with pytest.raises(ValidationError): + TaskRef(ref) + + +@pytest.mark.parametrize("ref_type", [MetricRef, TasksetRef]) +def test_sibling_ref_types_still_reject_fragments(ref_type: type) -> None: + """The fragment pattern is a sibling, not a widening of the shared constant: metrics and + tasksets have no revisions yet, so admitting a fragment would accept input nothing resolves. + They move onto it when they gain revisions — deliberately, at that point.""" + with pytest.raises(ValidationError): + ref_type(f"other/thing#{_DIGEST}") diff --git a/plugins/nemo-evaluator/tests/test_task_refs.py b/plugins/nemo-evaluator/tests/test_task_refs.py index 97b1beb62b..9da12bcc2b 100644 --- a/plugins/nemo-evaluator/tests/test_task_refs.py +++ b/plugins/nemo-evaluator/tests/test_task_refs.py @@ -9,35 +9,15 @@ import pytest from nemo_evaluator.api.schemas import MetadataItem, MetricRef, TaskInputs, TaskRef, TasksetRef -from nemo_evaluator.entities import TaskEntity, TasksetEntity +from nemo_evaluator.entities import TaskEntity, TaskRevisionEntity, TasksetEntity from nemo_evaluator.jobs.agent_spec import AgentEvalTaskInput +from nemo_evaluator.revisions import head_digest, publish_revision from nemo_evaluator.task_refs import resolve_agent_eval_tasks, resolve_taskset_ref from nemo_platform_plugin.entities import EntityBase -from nemo_platform_plugin.entity_client import NemoEntityNotFoundError _EntityT = TypeVar("_EntityT", bound=EntityBase) -class _FakeEntityClient: - """Minimal entity store keyed by (type, workspace, name), mirroring EntityClient.get.""" - - def __init__(self) -> None: - self.entities: dict[tuple[str, str, str], EntityBase] = {} - - def add(self, entity: EntityBase) -> None: - self.entities[(entity.__entity_type__, entity.workspace, entity.name)] = entity - - async def get( - self, entity_type: type[_EntityT], *, workspace: str, name: str, parent: str | None = None - ) -> _EntityT: - key = (entity_type.__entity_type__, workspace, name) - if key not in self.entities: - raise NemoEntityNotFoundError(f"{workspace}/{name} not found") - entity = self.entities[key] - assert isinstance(entity, entity_type) - return entity - - def _task(name: str, *, workspace: str = "default", metric: str = "default/m") -> TaskEntity: return TaskEntity( name=name, @@ -53,15 +33,22 @@ def _taskset(name: str, task_refs: list[str], *, workspace: str = "default") -> return TasksetEntity(name=name, workspace=workspace, tasks=[TaskRef(r) for r in task_refs]) -def _store(*entities: EntityBase) -> _FakeEntityClient: - client = _FakeEntityClient() +async def _store(client, *entities: EntityBase): + """Build a store and *publish* every task, so members resolve to a real revision. + + Tasks are published rather than merely inserted because taskset expansion now reads the pinned + revision's content, not the head's — the same thing the service does on create. + """ for entity in entities: - client.add(entity) + await client.create(entity) + if isinstance(entity, TaskEntity): + await publish_revision(client, client, entity, TaskRevisionEntity) return client -async def test_resolves_taskset_members_to_inline_task_inputs() -> None: - client = _store( +async def test_resolves_taskset_members_to_inline_task_inputs(entity_store) -> None: + client = await _store( + entity_store, _task("capital-of-france"), _task("capital-of-japan"), _taskset("geo", ["default/capital-of-france", "default/capital-of-japan"]), @@ -79,34 +66,37 @@ async def test_resolves_taskset_members_to_inline_task_inputs() -> None: assert tasks[0].reference == {} -async def test_bare_member_ref_resolves_against_taskset_workspace() -> None: - client = _store(_task("t1", workspace="team"), _taskset("ts", ["t1"], workspace="team")) +async def test_bare_member_ref_resolves_against_taskset_workspace(entity_store) -> None: + client = await _store(entity_store, _task("t1", workspace="team"), _taskset("ts", ["t1"], workspace="team")) tasks = await resolve_taskset_ref(TasksetRef("team/ts"), workspace="default", entity_client=client) assert [t.id for t in tasks] == ["t1"] -async def test_unknown_taskset_raises_clear_error() -> None: +async def test_unknown_taskset_raises_clear_error(entity_store) -> None: with pytest.raises(ValueError, match="Taskset reference 'default/missing' not found"): - await resolve_taskset_ref(TasksetRef("default/missing"), workspace="default", entity_client=_store()) + await resolve_taskset_ref( + TasksetRef("default/missing"), workspace="default", entity_client=await _store(entity_store) + ) -async def test_missing_member_task_raises_clear_error() -> None: - client = _store(_taskset("geo", ["default/gone"])) +async def test_missing_member_task_raises_clear_error(entity_store) -> None: + client = await _store(entity_store, _taskset("geo", ["default/gone"])) with pytest.raises(ValueError, match="Task 'default/gone' referenced by taskset 'default/geo'"): await resolve_taskset_ref(TasksetRef("default/geo"), workspace="default", entity_client=client) -async def test_empty_taskset_raises_clear_error() -> None: - client = _store(_taskset("empty", [])) +async def test_empty_taskset_raises_clear_error(entity_store) -> None: + client = await _store(entity_store, _taskset("empty", [])) with pytest.raises(ValueError, match="has no member tasks"): await resolve_taskset_ref(TasksetRef("default/empty"), workspace="default", entity_client=client) -async def test_duplicate_expanded_task_ids_rejected() -> None: +async def test_duplicate_expanded_task_ids_rejected(entity_store) -> None: # Two members from different workspaces share the name 'dup' -> ambiguous task id. - client = _store( + client = await _store( + entity_store, _task("dup", workspace="a"), _task("dup", workspace="b"), _taskset("geo", ["a/dup", "b/dup"]), @@ -115,18 +105,52 @@ async def test_duplicate_expanded_task_ids_rejected() -> None: await resolve_taskset_ref(TasksetRef("default/geo"), workspace="default", entity_client=client) -async def test_taskset_ref_requires_entity_client() -> None: +async def test_taskset_ref_requires_entity_client(entity_store) -> None: with pytest.raises(ValueError, match="requires a platform connection"): await resolve_taskset_ref(TasksetRef("default/geo"), workspace="default", entity_client=None) -async def test_resolve_agent_eval_tasks_passes_inline_list_through() -> None: +async def test_resolve_agent_eval_tasks_passes_inline_list_through(entity_store) -> None: inline = [AgentEvalTaskInput(id="t", intent="x", metrics=[])] result = await resolve_agent_eval_tasks(inline, workspace="default", entity_client=None) assert result is inline -async def test_resolve_agent_eval_tasks_expands_a_taskset_ref() -> None: - client = _store(_task("only"), _taskset("geo", ["default/only"])) +async def test_resolve_agent_eval_tasks_expands_a_taskset_ref(entity_store) -> None: + client = await _store(entity_store, _task("only"), _taskset("geo", ["default/only"])) result = await resolve_agent_eval_tasks(TasksetRef("default/geo"), workspace="default", entity_client=client) assert [t.id for t in result] == ["only"] + + +async def test_expansion_uses_the_pinned_revision_not_current_content(entity_store) -> None: + """The property the whole pinning design exists for: an evaluation re-run expands to the same + content even after a member task has published newer content. + + Before this was wired, expansion read the member's *head*, silently defeating the pin — the + taskset looked reproducible and wasn't. + """ + task = _task("capital-of-france") + client = await _store(entity_store, task) + pinned_digest = head_digest(task) + + taskset = _taskset("geo", [f"default/capital-of-france#{pinned_digest}"]) + await client.create(taskset) + + # The member publishes newer content after the taskset was pinned. + task.intent = "Something else entirely." + await client.update(task) + await publish_revision(client, client, task, TaskRevisionEntity) + + tasks = await resolve_taskset_ref(TasksetRef("default/geo"), workspace="default", entity_client=client) + + assert tasks[0].intent == "Do capital-of-france.", "expansion must return the pinned content" + + +async def test_expansion_fails_loudly_when_a_pin_no_longer_resolves(entity_store) -> None: + """Verify-on-read at the point it matters most: a pin that cannot be honoured must stop the + evaluation rather than quietly substituting whatever is current.""" + client = await _store(entity_store, _task("only")) + await client.create(_taskset("geo", [f"default/only#{'c' * 64}"])) + + with pytest.raises(ValueError, match="no longer resolves"): + await resolve_taskset_ref(TasksetRef("default/geo"), workspace="default", entity_client=client) diff --git a/services/core/entities/tests/repository/test_child_entity_filtering.py b/services/core/entities/tests/repository/test_child_entity_filtering.py new file mode 100644 index 0000000000..6148e347a0 --- /dev/null +++ b/services/core/entities/tests/repository/test_child_entity_filtering.py @@ -0,0 +1,117 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Filtering child entities by parent and by a custom (``data.*``) field. + +``list_entities`` takes no ``parent`` argument, which reads as "you cannot query children". These +tests establish that you can: ``parent`` is a mapped column, so it is reachable through an ordinary +``filter_op`` — no API change needed. Combined with ``data.*`` JSON extraction, that covers +"find the child of this parent whose custom field equals X", which is what a content-addressed +revision lookup needs. +""" + +import pytest +from nmp.common.api.filter import ComparisonOperation, FilterOperator, LogicalOperation +from nmp.core.entities.app.repository import SQLAlchemyEntityRepository + +pytestmark = pytest.mark.asyncio + +_DIGEST_A = "a" * 64 +_DIGEST_B = "b" * 64 + + +async def _seed(entity_repo: SQLAlchemyEntityRepository): + """Two parents, each with two revision children; digests repeat across parents on purpose.""" + parents = {} + for parent_name in ("task-one", "task-two"): + parent = await entity_repo.create_entity(workspace="workspace-1", entity_type="task", name=parent_name, data={}) + parents[parent_name] = parent + for ordinal, digest in ((1, _DIGEST_A), (2, _DIGEST_B)): + await entity_repo.create_entity( + workspace="workspace-1", + entity_type="task_revision", + name=f"rev.{ordinal}", + parent=parent.id, + data={"content_hash": digest, "revision": ordinal}, + ) + return parents + + +async def test_children_can_be_filtered_by_parent(entity_repo: SQLAlchemyEntityRepository, setup_workspaces): + """``parent`` is a real column, so it filters like any other — despite ``list_entities`` + exposing no dedicated argument for it.""" + parents = await _seed(entity_repo) + rows, total = await entity_repo.list_entities( + workspace="workspace-1", + entity_type="task_revision", + filter_op=ComparisonOperation(field="parent", operator=FilterOperator.EQ, value=parents["task-one"].id), + ) + assert total == 2 + assert {row.name for row in rows} == {"rev.1", "rev.2"} + + +async def test_children_can_be_filtered_by_custom_data_field(entity_repo: SQLAlchemyEntityRepository, setup_workspaces): + """``data.*`` paths are JSON-extracted by the filter translator, so a custom field on a stored + entity is queryable without promoting it to a column.""" + await _seed(entity_repo) + rows, total = await entity_repo.list_entities( + workspace="workspace-1", + entity_type="task_revision", + filter_op=ComparisonOperation(field="data.content_hash", operator=FilterOperator.EQ, value=_DIGEST_A), + ) + assert total == 2, "the same content digest exists under both parents" + assert {row.data["content_hash"] for row in rows} == {_DIGEST_A} + + +async def test_parent_and_custom_field_compose(entity_repo: SQLAlchemyEntityRepository, setup_workspaces): + """The query a digest-pinned ref actually needs: this parent's child with this digest. + + Parent scoping is what disambiguates — identical content under two different tasks yields the + same digest, so filtering on the digest alone returns both. + """ + parents = await _seed(entity_repo) + rows, total = await entity_repo.list_entities( + workspace="workspace-1", + entity_type="task_revision", + filter_op=LogicalOperation( + operator=FilterOperator.AND, + operations=[ + ComparisonOperation(field="parent", operator=FilterOperator.EQ, value=parents["task-two"].id), + ComparisonOperation(field="data.content_hash", operator=FilterOperator.EQ, value=_DIGEST_A), + ], + ), + ) + assert total == 1 + assert rows[0].parent == parents["task-two"].id + assert rows[0].data["content_hash"] == _DIGEST_A + + +async def test_unknown_digest_returns_nothing(entity_repo: SQLAlchemyEntityRepository, setup_workspaces): + await _seed(entity_repo) + _, total = await entity_repo.list_entities( + workspace="workspace-1", + entity_type="task_revision", + filter_op=ComparisonOperation(field="data.content_hash", operator=FilterOperator.EQ, value="c" * 64), + ) + assert total == 0 + + +async def test_deleting_a_parent_removes_its_children(entity_repo: SQLAlchemyEntityRepository, setup_workspaces): + """The parent FK is ``ondelete="CASCADE"``, so deleting a task takes its revisions with it — + no orphaned children left addressable by a stale parent id.""" + parents = await _seed(entity_repo) + await entity_repo.delete_entity(entity_id=parents["task-one"].id) + + _, orphaned = await entity_repo.list_entities( + workspace="workspace-1", + entity_type="task_revision", + filter_op=ComparisonOperation(field="parent", operator=FilterOperator.EQ, value=parents["task-one"].id), + ) + assert orphaned == 0 + + _, survivors = await entity_repo.list_entities( + workspace="workspace-1", + entity_type="task_revision", + filter_op=ComparisonOperation(field="parent", operator=FilterOperator.EQ, value=parents["task-two"].id), + ) + assert survivors == 2, "the other task's revisions must be untouched" From c0155711e5f2c8bea4977e6e7306d1be5e2f224d Mon Sep 17 00:00:00 2001 From: Sandy Chapman Date: Thu, 6 Aug 2026 10:08:18 -0300 Subject: [PATCH 2/2] fix(evaluator): address review on task/taskset revisions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two concurrency/hashing defects found in review, plus the SDK and docs changes that came with them. Deduplicate identical contended publishes. The dedup check reads the revision `latest` names, so it misses a winner whose pointer write has not landed: the loser saw a head still naming N-1, recomputed the same digest, and stepped past the contended ordinal onto a byte-identical rev.N+1. Both callers then reported a new revision for the same content. On conflict, inspect the revision that actually beat us and adopt it when its digest matches, so one of the two racers correctly gets 200. Canonicalize taskset membership before hashing. Membership is documented and validated as a set, but the digest covered the stored list, so [a, b] and [b, a] were one grouping under two digests and a reordered re-submit cut a revision that changed nothing. Sort the resolved digest-pinned refs on write — sorting the stored content rather than only the hash input keeps the digest a hash of exactly what is stored, which is what verification on read re-checks. SDK: split the revision selector into `revision=` (a content digest) and `tag=`, and make `tag()`'s two selectors keyword-only. Reading a tag back meant writing `revision="blessed"`, which reads as a contradiction at the call site; both still resolve through the same route segment. Docs: assert what the revision example actually demonstrates, explain taskset pin resolution with a worked diagram, and organize the status codes into an edge-case table. Signed-off-by: Sandy Chapman --- docs/evaluator/manage-tasks-tasksets.mdx | 87 +++++++++++++++---- plugins/nemo-evaluator/openapi/openapi.yaml | 10 ++- .../src/nemo_evaluator/api/schemas.py | 4 +- .../api/service/taskset_service.py | 11 ++- .../src/nemo_evaluator/revisions.py | 41 +++++++-- .../src/nemo_evaluator/sdk/http_utils.py | 16 ++++ .../src/nemo_evaluator/sdk/task_resources.py | 46 +++++++--- .../nemo_evaluator/sdk/taskset_resources.py | 46 +++++++--- .../tests/api/service/test_taskset_service.py | 32 ++++++- .../tests/integration/test_task_revisions.py | 4 +- .../tests/sdk/test_task_sdk_resources.py | 41 ++++++++- .../tests/sdk/test_taskset_sdk_resources.py | 2 +- .../nemo-evaluator/tests/test_revisions.py | 79 +++++++++++++++-- 13 files changed, 346 insertions(+), 73 deletions(-) diff --git a/docs/evaluator/manage-tasks-tasksets.mdx b/docs/evaluator/manage-tasks-tasksets.mdx index 97edfeea23..a8ca305897 100644 --- a/docs/evaluator/manage-tasks-tasksets.mdx +++ b/docs/evaluator/manage-tasks-tasksets.mdx @@ -165,22 +165,32 @@ Pass a content digest or a tag. This returns the content **as published**, not t page = tasks.list_revisions("capital-of-france") digest = next(revision.content_hash for revision in page.data if revision.revision == 1) -original = tasks.retrieve("capital-of-france", revision=digest) -current = tasks.retrieve("capital-of-france") +original = tasks.retrieve("capital-of-france", revision=digest) # revision 1, as published +current = tasks.retrieve("capital-of-france") # revision 2, the current content + +assert original.revision == 1 and current.revision == 2 +assert original.inputs.instruction != current.inputs.instruction ``` ### Tag a revision A tag is a mutable pointer to a revision — useful for marking one as reviewed or approved after it -has been evaluated. +has been evaluated. Read it back with `tag=`, the counterpart to `revision=`: ```python -tasks.tag("capital-of-france", "blessed", revision=digest) +tasks.tag("capital-of-france", tag="blessed", revision=digest) -blessed = tasks.retrieve("capital-of-france", revision="blessed") +blessed = tasks.retrieve("capital-of-france", tag="blessed") ``` +`revision=` takes a content digest and `tag=` takes a tag name. They select the same thing two ways, +so pass one or the other — passing both raises `ValueError`. + +A tag names exactly one revision. Re-tagging moves the pointer rather than adding a second one, so +`retrieve(tag=...)` always resolves to a single revision — there is no way for two revisions to +share a tag. + `latest` is managed automatically and always names the most recently published revision; it cannot be moved by hand. A tag name may not be empty, and may not look like a content digest (64 hexadecimal characters) — such a tag could be stored but never resolved, because a digest-shaped reference is @@ -235,7 +245,7 @@ tasksets.delete("geography-suite") ``` Tasksets carry the same revision surface as tasks — `replace`, `list_revisions`, `tag`, and -`retrieve(revision=...)`: +`retrieve(revision=...)` / `retrieve(tag=...)`: ```python # Re-resolving membership after a member task published new content cuts a new revision. @@ -249,6 +259,9 @@ for revision in tasksets.list_revisions("geography-suite").data: Re-submitting the *same* member names can still publish a new revision. Members are re-resolved on every write, so if a member task published in the meantime the grouping now names different content and genuinely differs. A taskset's identity is the exact revisions it names, not the names alone. + +Member *order*, by contrast, is not part of that identity: membership is a set, so it is stored in a +canonical order and reordering the same members publishes nothing. Deleting a taskset does not delete its member tasks — a taskset only holds references. @@ -275,17 +288,44 @@ input_spec = AgentEvalInputSpec( ) ``` -When the job runs, the taskset reference is resolved: each member's **pinned revision** is loaded — -not whatever that task currently contains — and its stored metric references are hydrated into -runnable metrics, exactly as if you had inlined them. Re-running the same taskset therefore evaluates -the same content, even if a member task has been republished since. The -same spec is submitted as the agent-evaluate job input; see +When the job runs, the taskset reference is resolved like this: + +- Each member is loaded at the **revision pinned in the taskset**, not the task's current tip. +- Metric references on those members are hydrated into runnable metrics, the same as for inline tasks. +- Re-running the same taskset therefore evaluates the same content, even if a member has been + republished since. + +```text +# What is stored on the taskset (digests fixed at create/replace time) +geography-suite@1 + └─ tasks: + - default/capital-of-france#aaa111... ← revision 1 content + - default/capital-of-japan#bbb222... + +# Later: the member task publishes new content +capital-of-france tip → revision 2 (#ccc333...) + +# Re-grade still uses the taskset pins, not the tip +JobRun A ──TasksetRef("geography-suite")──► load #aaa111..., #bbb222... +JobRun B ──TasksetRef("geography-suite")──► load #aaa111..., #bbb222... ← same content +``` + +To evaluate the *new* task content, publish a new taskset revision (for example +`tasksets.replace(...)`) so membership is re-resolved to the newer digests. + +Submit this spec as the agent-evaluate job input; see [Agent Evaluation](/documentation/evaluate-models/agent-eval) for the full run, target, and results flow. The inline form remains available for one-off tasks — swap `tasks=TasksetRef(...)` for `tasks=[AgentEvalTaskInput(...), ...]`. + +A `TasksetRef` names the taskset's current revision; it cannot yet carry a `#` +fragment of its own. Member content is pinned, so a re-run always grades the same task content — but +if the taskset itself is replaced, a re-submitted spec expands the new membership. + + Stored tasks carry no grader-only `reference` (held-out ground truth): that field lives only on inline `AgentEvalTaskInput`. Taskset-driven tasks therefore run with an empty `reference`, so use a taskset @@ -342,13 +382,26 @@ The SDK resources are a thin client over the Evaluator plugin REST API, mounted | `PUT` | `/tasksets/{name}/tags/{tag}?revision=` | Point a tag at an existing revision. | | `DELETE` | `/tasksets/{name}` | Delete a taskset and all of its revisions. | -`PUT` distinguishes its two outcomes by status: **`201`** when a new revision was published, and -**`200`** when the submitted content was already the current revision and nothing was cut. +### Edge Cases -`POST` on a name that already exists returns `409`, as does a `PUT` that loses a race with a -concurrent write. An invalid metric reference (task), a missing or duplicate task reference -(taskset), or a reserved or malformed tag name returns `422`. Retrieving or deleting a name — or a -revision — that does not exist returns `404`. +`PUT` distinguishes its two outcomes by status: **`201`** when a new revision was published, and +**`200`** when the submitted content was already the current revision and nothing was cut. The rest: + +| Case | Status | Behavior | +|------|--------|----------| +| `PUT` with new content | `201` | A revision is published and `latest` moves to it. | +| `PUT` with content identical to the current revision | `200` | Nothing is published; any tags in the body are still applied. | +| `PUT` with taskset members reordered | `200` | Membership is a set, so a reordering is not a content change. | +| `PUT` reverting to *older* content | `201` | The record genuinely changed, so it publishes a new ordinal rather than reusing the old one. | +| `PUT` re-resolving a member that has since republished | `201` | A taskset's identity is the exact revisions it names. | +| Two identical `PUT`s racing | `201` + `200` | One publishes; the other adopts its revision rather than cutting a duplicate. | +| `POST` on an existing name | `409` | Create is strict; use `PUT` to upsert. | +| `PUT` losing a race with a concurrent write | `409` | Retry against the current state. | +| Invalid metric reference (task) | `422` | Rejected at validation. | +| Missing or duplicate task reference (taskset) | `422` | Members must exist, and must resolve to distinct tasks. | +| Reserved or malformed tag name | `422` | `latest` cannot be moved by hand; a digest-shaped tag is refused. | +| Retrieving or deleting an unknown name or revision | `404` | Applies to both records and revisions. | +| `DELETE` on a task a taskset pins | `204` | Not prevented; the taskset's reference dangles and fails on read. | ## Related Topics diff --git a/plugins/nemo-evaluator/openapi/openapi.yaml b/plugins/nemo-evaluator/openapi/openapi.yaml index ced5332c68..a91e57bc18 100644 --- a/plugins/nemo-evaluator/openapi/openapi.yaml +++ b/plugins/nemo-evaluator/openapi/openapi.yaml @@ -5247,10 +5247,12 @@ components: $ref: '#/components/schemas/TaskRef' type: array title: Tasks - description: References to the member tasks (set semantics; duplicates rejected). - Each may be bare, tag-pinned ('task-a#latest'), or digest-pinned; all - are resolved to an exact digest when stored, so the grouping cannot change - underneath you when a member republishes. + description: 'References to the member tasks (set semantics; duplicates + rejected). Each may be bare, tag-pinned (''task-a#latest''), or digest-pinned; + all are resolved to an exact digest when stored, so the grouping cannot + change underneath you when a member republishes. Because membership is + a set, the stored order is canonical rather than the submitted order: + reordering the same members is not a content change and publishes no revision.' metadata: items: $ref: '#/components/schemas/MetadataItem' diff --git a/plugins/nemo-evaluator/src/nemo_evaluator/api/schemas.py b/plugins/nemo-evaluator/src/nemo_evaluator/api/schemas.py index bdda7dfed6..97fb2d81be 100644 --- a/plugins/nemo-evaluator/src/nemo_evaluator/api/schemas.py +++ b/plugins/nemo-evaluator/src/nemo_evaluator/api/schemas.py @@ -561,7 +561,9 @@ class TasksetInput(BaseModel): default_factory=list, description="References to the member tasks (set semantics; duplicates rejected). Each may be " "bare, tag-pinned ('task-a#latest'), or digest-pinned; all are resolved to an exact digest " - "when stored, so the grouping cannot change underneath you when a member republishes.", + "when stored, so the grouping cannot change underneath you when a member republishes. " + "Because membership is a set, the stored order is canonical rather than the submitted order: " + "reordering the same members is not a content change and publishes no revision.", ) metadata: TaskMetadataList = Field(default_factory=list, description="Key/value annotations for the taskset.") tags: list[str] = Field( diff --git a/plugins/nemo-evaluator/src/nemo_evaluator/api/service/taskset_service.py b/plugins/nemo-evaluator/src/nemo_evaluator/api/service/taskset_service.py index 3ac6d87bbc..359fea7a24 100644 --- a/plugins/nemo-evaluator/src/nemo_evaluator/api/service/taskset_service.py +++ b/plugins/nemo-evaluator/src/nemo_evaluator/api/service/taskset_service.py @@ -221,6 +221,13 @@ async def _resolved_content(self, taskset_input: TasksetInput, *, workspace: str this taskset's membership the next time that task published, and a published grouping that changes underneath you is not a grouping. Same reason a lockfile records resolved versions, not ranges. + + The resolved refs are then sorted into a **canonical order**. Membership is a set — the + field documents it as such and duplicates are rejected — but the digest covers the stored + list, so without this ``[a, b]`` and ``[b, a]`` would be the same grouping under two + different digests, and re-submitting a reordered request would cut a revision that changed + nothing. Sorting the stored content rather than only the hash input keeps the digest a hash + of exactly what is stored, which is what verification on read re-checks. """ self._reject_duplicate_members(taskset_input.tasks, workspace=workspace) limit = asyncio.Semaphore(_MEMBER_RESOLUTION_CONCURRENCY) @@ -229,11 +236,9 @@ async def _bounded(ref: TaskRef) -> TaskRef: async with limit: return await self._pin_member(ref, workspace=workspace) - # ``gather`` preserves input order, so membership order survives resolution — which matters - # because a taskset's digest depends on member order. pending = [asyncio.create_task(_bounded(ref)) for ref in taskset_input.tasks] try: - return list(await asyncio.gather(*pending)) + return sorted(await asyncio.gather(*pending), key=lambda ref: ref.root) except BaseException: # ``gather`` propagates the first failure but leaves its siblings running. One bad # member in a large grouping would otherwise keep issuing reads long after the request diff --git a/plugins/nemo-evaluator/src/nemo_evaluator/revisions.py b/plugins/nemo-evaluator/src/nemo_evaluator/revisions.py index 624b30e9d1..5b02566018 100644 --- a/plugins/nemo-evaluator/src/nemo_evaluator/revisions.py +++ b/plugins/nemo-evaluator/src/nemo_evaluator/revisions.py @@ -210,6 +210,25 @@ async def find_by_digest( return result.data[0] if result.data else None +async def _revision_at( + revision_client: RevisionStoreProtocol[RevisionT], + revision_type: type[RevisionT], + head: TaskEntity | TasksetEntity, + ordinal: int, +) -> RevisionT | None: + """This record's revision ``ordinal``, or ``None`` if no such child exists. + + A direct child lookup rather than a query: a revision's identity within its parent is its + ordinal, so the name is already known. + """ + try: + return await revision_client.get( + revision_type, name=revision_name(ordinal), workspace=head.workspace, parent=head.id + ) + except NemoEntityNotFoundError: + return None + + async def _current_revision( revision_client: RevisionStoreProtocol[RevisionT], revision_type: type[RevisionT], @@ -223,12 +242,7 @@ async def _current_revision( ordinal = head.tags.get(LATEST_TAG) if ordinal is None: return None - try: - return await revision_client.get( - revision_type, name=revision_name(ordinal), workspace=head.workspace, parent=head.id - ) - except NemoEntityNotFoundError: - return None + return await _revision_at(revision_client, revision_type, head, ordinal) async def get_revision( @@ -402,7 +416,20 @@ async def publish_revision( extra={"record": f"{head.workspace}/{head.name}", "ordinal": ordinal, "attempt": attempt + 1}, ) _refresh_pointers(head, await head_client.get(type(head), name=head.name, workspace=head.workspace)) - # The refresh above adopts the *stored* allocation state, which does not necessarily + + # The winner may have published exactly what we are publishing. Two identical requests + # that overlap both compute this digest; the dedup check above missed it only because + # the winner's pointer write had not landed when we read ``latest``. Stepping past their + # ordinal here would cut a byte-identical duplicate and report ``created`` to both + # callers, so idempotency has to be re-checked against the revision that actually beat + # us rather than against a head that may still be stale. + contended = await _revision_at(revision_client, revision_type, head, ordinal) + if contended is not None and contended.content_hash == digest: + if any(head.tags.get(tag) != contended.revision for tag in applied): + head = await _point_tags(head_client, head, applied, contended.revision) + return contended, head, False + + # The refresh earlier adopts the *stored* allocation state, which does not necessarily # account for the ordinal we just lost: if the winner's own pointer write never landed, # the stored head still names N-1 and we would recompute N and lose again, every attempt # until the retries run out — and every later publish would do the same, leaving the diff --git a/plugins/nemo-evaluator/src/nemo_evaluator/sdk/http_utils.py b/plugins/nemo-evaluator/src/nemo_evaluator/sdk/http_utils.py index c93c8be7ba..75e6a06c75 100644 --- a/plugins/nemo-evaluator/src/nemo_evaluator/sdk/http_utils.py +++ b/plugins/nemo-evaluator/src/nemo_evaluator/sdk/http_utils.py @@ -42,6 +42,22 @@ def url(platform: PlatformClient, path: str, workspace: str | None = None) -> st return _join_url(str(platform.base_url), f"{_API_PREFIX}/{resolved_path}") +def revision_selector(revision: str | None, tag: str | None) -> str | None: + """Encode a revision selector for a ``/revisions/{selector}`` path segment. + + The route takes one selector that may be either a digest or a tag, but the SDK splits it into + two named arguments. One argument would mean writing ``revision="blessed"`` to read a tag, which + reads as a contradiction at the call site even though the server resolves it happily; two names + make the caller's intent explicit and cost nothing, since both still resolve server-side. + + Returns ``None`` when neither is given — the caller then reads current content instead. + """ + if revision is not None and tag is not None: + raise ValueError("pass either 'revision' (a content digest) or 'tag', not both") + selector = revision if revision is not None else tag + return None if selector is None else quote(selector, safe="") + + def platform_default_headers(platform: PlatformClient) -> dict[str, str]: """Return string-valued default platform headers for direct evaluator HTTP calls.""" return {str(key): value for key, value in platform.default_headers.items() if isinstance(value, str)} diff --git a/plugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.py b/plugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.py index 69435d499b..f80556017d 100644 --- a/plugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.py +++ b/plugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.py @@ -85,8 +85,12 @@ def list_revisions( response.raise_for_status() return Page[Revision].model_validate(response.json()) - def tag(self, name: str, tag: str, *, revision: str, workspace: str | None = None) -> Task: - """Point a tag at an existing revision, named by digest or by another tag.""" + def tag(self, name: str, *, tag: str, revision: str, workspace: str | None = None) -> Task: + """Point ``tag`` at an existing revision, named by digest or by another tag. + + Both selectors are keyword-only: ``tag`` names the pointer being written and ``revision`` + names what it points at, and two bare strings in a row gave no hint which was which. + """ response = self._http_client.put( f"{self._item_url(name, workspace)}/tags/{quote(tag, safe='')}", params={"revision": revision}, @@ -96,11 +100,18 @@ def tag(self, name: str, tag: str, *, revision: str, workspace: str | None = Non response.raise_for_status() return Task.model_validate(response.json()) - def retrieve(self, name: str, *, revision: str | None = None, workspace: str | None = None) -> Task: - """Get a stored task by name, or as of a published revision (digest or tag).""" + def retrieve( + self, name: str, *, revision: str | None = None, tag: str | None = None, workspace: str | None = None + ) -> Task: + """Get a stored task by name, or as of a published revision. + + Pass ``revision`` for a content digest or ``tag`` for a named pointer — not both. With + neither, this returns the task's current content. + """ url = self._item_url(name, workspace) - if revision is not None: - url = f"{url}/revisions/{quote(revision, safe='')}" + selector = http_utils.revision_selector(revision, tag) + if selector is not None: + url = f"{url}/revisions/{selector}" response = self._http_client.get(url, headers=self._headers(), timeout=self._platform.timeout) response.raise_for_status() return Task.model_validate(response.json()) @@ -190,8 +201,12 @@ async def list_revisions( response.raise_for_status() return Page[Revision].model_validate(response.json()) - async def tag(self, name: str, tag: str, *, revision: str, workspace: str | None = None) -> Task: - """Point a tag at an existing revision, named by digest or by another tag.""" + async def tag(self, name: str, *, tag: str, revision: str, workspace: str | None = None) -> Task: + """Point ``tag`` at an existing revision, named by digest or by another tag. + + Both selectors are keyword-only: ``tag`` names the pointer being written and ``revision`` + names what it points at, and two bare strings in a row gave no hint which was which. + """ response = await self._http_client.put( f"{self._item_url(name, workspace)}/tags/{quote(tag, safe='')}", params={"revision": revision}, @@ -201,11 +216,18 @@ async def tag(self, name: str, tag: str, *, revision: str, workspace: str | None response.raise_for_status() return Task.model_validate(response.json()) - async def retrieve(self, name: str, *, revision: str | None = None, workspace: str | None = None) -> Task: - """Get a stored task by name, or as of a published revision (digest or tag).""" + async def retrieve( + self, name: str, *, revision: str | None = None, tag: str | None = None, workspace: str | None = None + ) -> Task: + """Get a stored task by name, or as of a published revision. + + Pass ``revision`` for a content digest or ``tag`` for a named pointer — not both. With + neither, this returns the task's current content. + """ url = self._item_url(name, workspace) - if revision is not None: - url = f"{url}/revisions/{quote(revision, safe='')}" + selector = http_utils.revision_selector(revision, tag) + if selector is not None: + url = f"{url}/revisions/{selector}" response = await self._http_client.get(url, headers=self._headers(), timeout=self._platform.timeout) response.raise_for_status() return Task.model_validate(response.json()) diff --git a/plugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.py b/plugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.py index 9a45d201d9..37c42e045d 100644 --- a/plugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.py +++ b/plugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.py @@ -91,8 +91,12 @@ def list_revisions( response.raise_for_status() return Page[Revision].model_validate(response.json()) - def tag(self, name: str, tag: str, *, revision: str, workspace: str | None = None) -> Taskset: - """Point a tag at an existing revision, named by digest or by another tag.""" + def tag(self, name: str, *, tag: str, revision: str, workspace: str | None = None) -> Taskset: + """Point ``tag`` at an existing revision, named by digest or by another tag. + + Both selectors are keyword-only: ``tag`` names the pointer being written and ``revision`` + names what it points at, and two bare strings in a row gave no hint which was which. + """ response = self._http_client.put( f"{self._item_url(name, workspace)}/tags/{quote(tag, safe='')}", params={"revision": revision}, @@ -102,11 +106,18 @@ def tag(self, name: str, tag: str, *, revision: str, workspace: str | None = Non response.raise_for_status() return Taskset.model_validate(response.json()) - def retrieve(self, name: str, *, revision: str | None = None, workspace: str | None = None) -> Taskset: - """Get a stored taskset by name, or as of a published revision (digest or tag).""" + def retrieve( + self, name: str, *, revision: str | None = None, tag: str | None = None, workspace: str | None = None + ) -> Taskset: + """Get a stored taskset by name, or as of a published revision. + + Pass ``revision`` for a content digest or ``tag`` for a named pointer — not both. With + neither, this returns the taskset's current membership. + """ url = self._item_url(name, workspace) - if revision is not None: - url = f"{url}/revisions/{quote(revision, safe='')}" + selector = http_utils.revision_selector(revision, tag) + if selector is not None: + url = f"{url}/revisions/{selector}" response = self._http_client.get(url, headers=self._headers(), timeout=self._platform.timeout) response.raise_for_status() return Taskset.model_validate(response.json()) @@ -198,8 +209,12 @@ async def list_revisions( response.raise_for_status() return Page[Revision].model_validate(response.json()) - async def tag(self, name: str, tag: str, *, revision: str, workspace: str | None = None) -> Taskset: - """Point a tag at an existing revision, named by digest or by another tag.""" + async def tag(self, name: str, *, tag: str, revision: str, workspace: str | None = None) -> Taskset: + """Point ``tag`` at an existing revision, named by digest or by another tag. + + Both selectors are keyword-only: ``tag`` names the pointer being written and ``revision`` + names what it points at, and two bare strings in a row gave no hint which was which. + """ response = await self._http_client.put( f"{self._item_url(name, workspace)}/tags/{quote(tag, safe='')}", params={"revision": revision}, @@ -209,11 +224,18 @@ async def tag(self, name: str, tag: str, *, revision: str, workspace: str | None response.raise_for_status() return Taskset.model_validate(response.json()) - async def retrieve(self, name: str, *, revision: str | None = None, workspace: str | None = None) -> Taskset: - """Get a stored taskset by name, or as of a published revision (digest or tag).""" + async def retrieve( + self, name: str, *, revision: str | None = None, tag: str | None = None, workspace: str | None = None + ) -> Taskset: + """Get a stored taskset by name, or as of a published revision. + + Pass ``revision`` for a content digest or ``tag`` for a named pointer — not both. With + neither, this returns the taskset's current membership. + """ url = self._item_url(name, workspace) - if revision is not None: - url = f"{url}/revisions/{quote(revision, safe='')}" + selector = http_utils.revision_selector(revision, tag) + if selector is not None: + url = f"{url}/revisions/{selector}" response = await self._http_client.get(url, headers=self._headers(), timeout=self._platform.timeout) response.raise_for_status() return Taskset.model_validate(response.json()) diff --git a/plugins/nemo-evaluator/tests/api/service/test_taskset_service.py b/plugins/nemo-evaluator/tests/api/service/test_taskset_service.py index 97f81ee6c6..ee6679fc2e 100644 --- a/plugins/nemo-evaluator/tests/api/service/test_taskset_service.py +++ b/plugins/nemo-evaluator/tests/api/service/test_taskset_service.py @@ -225,19 +225,43 @@ async def resolve_revision(self, workspace: str, name: str, fragment: str = "lat assert overlap["peak"] > 1, "members must resolve concurrently, not one at a time" -async def test_member_resolution_preserves_order(existing_tasks: set[tuple[str, str]], entity_store) -> None: - """Concurrency must not reorder membership — `gather` preserves input order, and the digest of - a taskset depends on member order.""" +async def test_membership_is_stored_in_canonical_order(existing_tasks: set[tuple[str, str]], entity_store) -> None: + """Membership is a set, so it is stored sorted rather than in submission order. + + The digest covers the stored list, so leaving order to the caller would give one grouping two + identities. Sorting also has to survive concurrent resolution, which completes out of order. + """ members = {("default", f"task-{i}") for i in range(5)} service = TasksetService(entity_store, _FakeTaskService(members)) created, _ = await service.create_taskset( - "ts-1", TasksetInput(tasks=[TaskRef(f"task-{i}") for i in range(5)]), workspace="default" + "ts-1", TasksetInput(tasks=[TaskRef(f"task-{i}") for i in reversed(range(5))]), workspace="default" ) assert [t.root.split("#")[0] for t in created.tasks] == [f"default/task-{i}" for i in range(5)] +async def test_reordering_members_publishes_no_revision(existing_tasks: set[tuple[str, str]], entity_store) -> None: + """Re-submitting the same members in a different order is not a content change. + + This is the property canonical ordering exists for: without it the two requests hash + differently, and a caller that merely rebuilt its list from a set would cut a revision whose + content is indistinguishable from its predecessor's. + """ + members = {("default", f"task-{i}") for i in range(3)} + service = TasksetService(entity_store, _FakeTaskService(members)) + + created, _ = await service.create_taskset( + "ts-1", TasksetInput(tasks=[TaskRef(f"task-{i}") for i in range(3)]), workspace="default" + ) + replaced, published = await service.replace_taskset( + "ts-1", TasksetInput(tasks=[TaskRef(f"task-{i}") for i in reversed(range(3))]), workspace="default" + ) + + assert not published, "reordered membership is the same set, so nothing should be published" + assert replaced.revision == created.revision == 1 + + async def test_tag_revision_returns_none_for_a_missing_taskset(service: TasksetService) -> None: assert await service.tag_revision("default", "nope", "blessed", "latest") is None diff --git a/plugins/nemo-evaluator/tests/integration/test_task_revisions.py b/plugins/nemo-evaluator/tests/integration/test_task_revisions.py index 31d1b9883e..4e814251ff 100644 --- a/plugins/nemo-evaluator/tests/integration/test_task_revisions.py +++ b/plugins/nemo-evaluator/tests/integration/test_task_revisions.py @@ -161,11 +161,11 @@ def test_tagging_an_older_revision_leaves_latest_alone(subprocess_platform: str) first_digest = client.evaluator.tasks.list_revisions(name, workspace=WORKSPACE).data[0].content_hash client.evaluator.tasks.replace(name, task=_task_input("Second."), workspace=WORKSPACE) - tagged = client.evaluator.tasks.tag(name, "blessed", revision=first_digest, workspace=WORKSPACE) + tagged = client.evaluator.tasks.tag(name, tag="blessed", revision=first_digest, workspace=WORKSPACE) assert tagged.tags["blessed"] == 1 assert tagged.tags["latest"] == 2, "latest is machine-managed and must not follow a manual tag" - assert client.evaluator.tasks.retrieve(name, revision="blessed", workspace=WORKSPACE).intent == "First." + assert client.evaluator.tasks.retrieve(name, tag="blessed", workspace=WORKSPACE).intent == "First." finally: client.evaluator.tasks.delete(name, workspace=WORKSPACE) diff --git a/plugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.py b/plugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.py index 579f948044..6334460ece 100644 --- a/plugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.py +++ b/plugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.py @@ -9,6 +9,7 @@ from typing import Any from unittest.mock import AsyncMock, MagicMock +import pytest from nemo_evaluator.api.schemas import MetricRef, Revision, Task, TaskInput from nemo_evaluator.sdk.task_resources import AsyncEvaluatorTasksResource, EvaluatorTasksResource @@ -178,6 +179,40 @@ def test_sync_retrieve_with_revision_targets_the_revision_sub_path() -> None: assert http_client.get.call_args.args[0] == f"{_BASE}/tasks/task-1/revisions/{digest}" +def test_sync_retrieve_with_tag_targets_the_same_sub_path() -> None: + """``tag`` and ``revision`` are two names for one route segment, resolved server-side. + + Splitting them is a call-site readability change only, so a tag must reach exactly the URL a + digest would — a separate query parameter or route would be a behaviour change nobody asked for. + """ + http_client = MagicMock() + http_client.get.return_value = _response(_task_payload("task-1")) + resource = EvaluatorTasksResource(_platform(http_client)) + + resource.retrieve("task-1", tag="blessed") + + assert http_client.get.call_args.args[0] == f"{_BASE}/tasks/task-1/revisions/blessed" + + +def test_sync_retrieve_rejects_both_selectors() -> None: + """Two selectors is ambiguous intent, not a precedence question — refuse rather than pick one.""" + resource = EvaluatorTasksResource(_platform(MagicMock())) + + with pytest.raises(ValueError, match="not both"): + resource.retrieve("task-1", revision="a" * 64, tag="blessed") + + +def test_sync_retrieve_percent_encodes_a_tag() -> None: + """Tags admit ``/`` (``release/v1``), which would otherwise open a path segment.""" + http_client = MagicMock() + http_client.get.return_value = _response(_task_payload("task-1")) + resource = EvaluatorTasksResource(_platform(http_client)) + + resource.retrieve("task-1", tag="release/v1") + + assert http_client.get.call_args.args[0] == f"{_BASE}/tasks/task-1/revisions/release%2Fv1" + + def test_sync_list_revisions_parses_the_page() -> None: http_client = MagicMock() http_client.get.return_value = _response( @@ -207,7 +242,7 @@ def test_sync_tag_puts_to_the_tag_url_with_the_revision() -> None: resource = EvaluatorTasksResource(_platform(http_client)) digest = "a" * 64 - resource.tag("task-1", "blessed", revision=digest) + resource.tag("task-1", tag="blessed", revision=digest) assert http_client.put.call_args.args[0] == f"{_BASE}/tasks/task-1/tags/blessed" assert http_client.put.call_args.kwargs["params"] == {"revision": digest} @@ -219,7 +254,7 @@ def test_sync_tag_escapes_the_tag_name() -> None: http_client.put.return_value = _response(_task_payload("task-1")) resource = EvaluatorTasksResource(_platform(http_client)) - resource.tag("task-1", "release/v1", revision="a" * 64) + resource.tag("task-1", tag="release/v1", revision="a" * 64) assert http_client.put.call_args.args[0] == f"{_BASE}/tasks/task-1/tags/release%2Fv1" @@ -251,6 +286,6 @@ async def test_async_tag_puts_to_the_tag_url() -> None: http_client.put = AsyncMock(return_value=_response(_task_payload("task-1"))) resource = AsyncEvaluatorTasksResource(_platform(http_client)) - await resource.tag("task-1", "blessed", revision="a" * 64) + await resource.tag("task-1", tag="blessed", revision="a" * 64) assert http_client.put.call_args.args[0] == f"{_BASE}/tasks/task-1/tags/blessed" diff --git a/plugins/nemo-evaluator/tests/sdk/test_taskset_sdk_resources.py b/plugins/nemo-evaluator/tests/sdk/test_taskset_sdk_resources.py index 0d94d86866..335489761f 100644 --- a/plugins/nemo-evaluator/tests/sdk/test_taskset_sdk_resources.py +++ b/plugins/nemo-evaluator/tests/sdk/test_taskset_sdk_resources.py @@ -182,7 +182,7 @@ def test_sync_tag_puts_to_the_tag_url_with_the_revision() -> None: resource = EvaluatorTasksetsResource(_platform(http_client)) digest = "a" * 64 - resource.tag("ts-1", "blessed", revision=digest) + resource.tag("ts-1", tag="blessed", revision=digest) assert http_client.put.call_args.args[0] == f"{_BASE}/tasksets/ts-1/tags/blessed" assert http_client.put.call_args.kwargs["params"] == {"revision": digest} diff --git a/plugins/nemo-evaluator/tests/test_revisions.py b/plugins/nemo-evaluator/tests/test_revisions.py index 4ce5574d8a..b263389dfe 100644 --- a/plugins/nemo-evaluator/tests/test_revisions.py +++ b/plugins/nemo-evaluator/tests/test_revisions.py @@ -25,6 +25,7 @@ apply_tag, get_revision, head_digest, + list_revisions, publish_revision, revision_name, ) @@ -47,6 +48,9 @@ def __init__(self) -> None: self._tick = 0 #: Ordinals to fail the first create for, simulating a lost allocation race. self.contend_ordinals: set[int] = set() + #: Ordinals to lose to a publisher of *identical* content whose pointer write has not landed + #: yet — the interleaving that makes two concurrent identical requests each look novel. + self.contend_identically: set[int] = set() #: One-shot hook fired just before a *head* update, modelling a competing publisher that #: lands in the window between our child create and our pointer write. Lives on the fake #: rather than a monkeypatch so the race is expressed the same way ``contend_ordinals`` is. @@ -64,6 +68,10 @@ async def create(self, entity: _E) -> _E: self.contend_ordinals.discard(ordinal) self._win_race(entity, ordinal) raise NemoEntityConflictError(f"{entity.name} taken by a concurrent publisher") + if ordinal in self.contend_identically: + self.contend_identically.discard(ordinal) + self._win_race(entity, ordinal, digest=entity.content_hash, advance_head=False) + raise NemoEntityConflictError(f"{entity.name} taken by a concurrent publisher") self._next_id += 1 self._tick += 1 entity._id = f"id-{self._next_id}" @@ -71,20 +79,29 @@ async def create(self, entity: _E) -> _E: self.records[key] = entity.model_copy(deep=True) return entity - def _win_race(self, losing_entity, ordinal: int) -> None: + def _win_race(self, losing_entity, ordinal: int, *, digest: str | None = None, advance_head: bool = True) -> None: """Model a *complete* competing publish, not just a failed create. - The winner's record exists under the contended name and the head has advanced. Without - both, the loser re-reads an unchanged head and recomputes the same ordinal — which no real - race would do, and which would make the retry look broken when it isn't. The competitor - publishes *different* content (its own digest), so the loser genuinely needs a new ordinal - rather than discovering its content already published. + By default the winner's record exists under the contended name and the head has advanced. + Without both, the loser re-reads an unchanged head and recomputes the same ordinal — which + no real race would do, and which would make the retry look broken when it isn't. The + competitor publishes *different* content (its own digest), so the loser genuinely needs a + new ordinal rather than discovering its content already published. + + ``digest`` and ``advance_head`` express the opposite interleaving: a winner publishing the + *same* content whose pointer write has not landed yet. The loser then sees a head that still + names the previous revision, so nothing but the contended child itself reveals that its + content is already published. """ - winner = losing_entity.model_copy(update={"content_hash": f"{ordinal:064x}"}) + winner = losing_entity.model_copy(update={"content_hash": digest or f"{ordinal:064x}"}) winner._parent = losing_entity._parent self._next_id += 1 winner._id = f"id-{self._next_id}" + self._tick += 1 + winner._created_at = datetime(2026, 1, 1, tzinfo=timezone.utc) + timedelta(seconds=self._tick) self.records[self._key(type(winner), winner.name, winner.workspace, winner._parent)] = winner + if not advance_head: + return head = self.records.get(self._key(TaskEntity, "task-1", winner.workspace, None)) if head is not None: head.latest_revision = max(head.latest_revision, ordinal) @@ -315,6 +332,54 @@ async def test_contended_ordinal_is_retried() -> None: assert revision.revision == 3 +@pytest.mark.asyncio +async def test_identical_contended_publish_adopts_the_winners_revision() -> None: + """Two identical publishes that overlap must still cut exactly one revision. + + The dedup check reads the revision ``latest`` names, so it misses a winner whose pointer write + has not landed: the loser sees a head still naming ``N-1``, computes the same digest, and would + step past the contended ordinal onto a byte-identical ``rev.N+1``. Both callers would then + report a new revision for the same content, which is exactly what publishing is supposed to be + idempotent against. + """ + store = FakeStore() + head = _head(store) + await _publish(store, head) + + head.intent = "Changed." + store.contend_identically = {2} + revision, created = await _publish(store, head) + + assert not created, "the loser must report the winner's revision, not a publish of its own" + assert revision.revision == 2, "adopt the contended ordinal rather than allocating past it" + + page = await list_revisions(store, TaskRevisionEntity, head) + assert [entry.revision for entry in page.data] == [2, 1], "no duplicate-content revision was cut" + + stored = store.records[store._key(TaskEntity, head.name, head.workspace, None)] + assert isinstance(stored, TaskEntity) + assert stored.tags[LATEST_TAG] == 2, "adopting must still point latest at the revision it adopted" + + +@pytest.mark.asyncio +async def test_contended_publish_of_different_content_still_allocates_a_new_ordinal() -> None: + """Adoption is keyed on the digest, not on losing the race. + + The guard added for identical publishes must not swallow a genuine one: a loser whose content + differs from the winner's still needs its own ordinal. + """ + store = FakeStore() + head = _head(store) + await _publish(store, head) + + head.intent = "Changed." + store.contend_ordinals = {2} # winner publishes *different* content + revision, created = await _publish(store, head) + + assert created + assert revision.revision == 3 + + @pytest.mark.asyncio async def test_publishing_recovers_when_a_revision_exists_but_the_head_never_advanced() -> None: """The one case where losing a create does *not* mean someone else advanced the head.