feat(evaluator): add content-addressed revisions for tasks and tasksets - #1023
Open
SandyChapman wants to merge 1 commit into
Open
feat(evaluator): add content-addressed revisions for tasks and tasksets#1023SandyChapman wants to merge 1 commit into
SandyChapman wants to merge 1 commit into
Conversation
Contributor
Contributor
|
SandyChapman
force-pushed
the
schapman/harbor-cli-dataset-endpoint-45cb1d
branch
from
August 4, 2026 12:16
38b2279 to
2a18647
Compare
SandyChapman
marked this pull request as ready for review
August 4, 2026 14:39
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThe evaluator now supports immutable, content-hashed task and taskset revisions. It adds revision tags, digest-pinned membership, idempotent publishing, REST APIs, SDK methods, pinned evaluation reads, and validation coverage. Evaluator revision management
Sequence Diagram(s)sequenceDiagram
participant Client
participant EvaluatorAPI
participant TasksetService
participant TaskService
participant EntityStore
Client->>EvaluatorAPI: PUT taskset with task references
EvaluatorAPI->>TasksetService: replace_taskset
TasksetService->>TaskService: resolve each task revision
TaskService->>EntityStore: resolve tag or digest
EntityStore-->>TaskService: return content digest
TasksetService->>EntityStore: publish digest-pinned taskset revision
EntityStore-->>EvaluatorAPI: return revision-backed taskset
EvaluatorAPI-->>Client: return 201 or 200
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
plugins/nemo-evaluator/src/nemo_evaluator/api/schemas.py (1)
484-485: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the single digest pattern.
content_hash.DIGEST_PATTERNalready defines^[0-9a-f]{64}$.revisions._DIGEST_FRAGMENTis a third copy. Import the constant here so the digest shape has one definition.♻️ Proposed change
-#: Shape of a content digest in a ref fragment: full-length lowercase hex, never truncated. -_DIGEST_FRAGMENT_PATTERN = re.compile(r"^[0-9a-f]{64}$") +#: Shape of a content digest in a ref fragment: full-length lowercase hex, never truncated. +_DIGEST_FRAGMENT_PATTERN = re.compile(DIGEST_PATTERN)Add the import (
from nemo_evaluator.content_hash import DIGEST_PATTERN).content_hashimports nothing fromschemas, so no cycle.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-evaluator/src/nemo_evaluator/api/schemas.py` around lines 484 - 485, Replace the local _DIGEST_FRAGMENT_PATTERN definition in schemas.py with the shared DIGEST_PATTERN imported from nemo_evaluator.content_hash, preserving the existing 64-character lowercase hexadecimal validation and avoiding duplicate definitions.plugins/nemo-evaluator/tests/test_revisions.py (2)
103-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign
FakeStore.listwith the protocol it stands in for.
EntityStoreProtocol.listdeclaressort,page, andpage_size.revisions.list_revisionspassessort="-created_at"andpage=page. This fake accepts neither, so any test that exerciseslist_revisionsfails withTypeErrorinstead of testing behavior. Accept both keywords.♻️ Proposed change
- async def list(self, entity_type, *, workspace, filter_operation=None, page_size=100): + async def list(self, entity_type, *, workspace, filter_operation=None, sort=None, page=1, page_size=100):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-evaluator/tests/test_revisions.py` around lines 103 - 109, Update the FakeStore.list method to accept the protocol’s sort and page keyword arguments in addition to workspace, filter_operation, and page_size. Preserve its existing filter-evaluation behavior while allowing revisions.list_revisions to pass sort="-created_at" and page=page without raising TypeError.
204-210: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated assertion and fix the test name.
Line 209 and line 210 assert the same thing. The name says
indexes_the_digest, but no assertion covers a digest index. Drop the duplicate and rename to match what the test proves.♻️ Proposed change
-async def test_first_publish_applies_latest_and_indexes_the_digest() -> None: +async def test_first_publish_applies_latest_and_advances_latest_revision() -> None: store = FakeStore() head = _head(store) revision, _ = await _publish(store, head) assert head.tags[LATEST_TAG] == revision.revision assert head.latest_revision == 1 - assert head.latest_revision == 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-evaluator/tests/test_revisions.py` around lines 204 - 210, Update test_first_publish_applies_latest_and_indexes_the_digest by removing the duplicated head.latest_revision assertion and renaming the test to describe only the latest-tag and revision behavior it verifies, since it does not assert digest indexing.plugins/nemo-evaluator/src/nemo_evaluator/task_refs.py (1)
81-98: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMember expansion now costs two sequential store calls per member.
Each member requires a head
getplus a revision lookup. A digest fragment adds a filteredlistquery. The loop awaits them one member at a time, so expansion latency scales with member count times two round-trips. If tasksets can hold many members, fetch members concurrently withasyncio.gather. Note that concurrency changes which failure surfaces first, so keep the error text keyed to the failing ref.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-evaluator/src/nemo_evaluator/task_refs.py` around lines 81 - 98, The taskset member expansion loop should fetch task entities and pinned revisions concurrently instead of awaiting each member’s head and revision calls sequentially. Update the expansion flow around parse_subentity_ref, entity_client.get, and get_revision to use asyncio.gather while retaining per-member error handling and messages keyed to task_ref.root and ref.root, including digest-fragment lookups.plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.py (1)
194-218: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBoth revision-listing routes skip the module's error-handling convention. Every other route in these modules wraps its service call, logs the exception, and returns a 500. These two let store failures propagate raw.
plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.py#L194-L218: wrapservice.list_revisionsin try/except, log withlogger.exception, and raise a 500.plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasksets.py#L197-L218: apply the same wrapping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.py` around lines 194 - 218, Wrap the service call in list_task_revisions in tasks.py (plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.py, lines 194-218) with the module’s standard try/except, log failures using logger.exception, and raise an HTTP 500 while preserving the existing not-found handling. Apply the same wrapping to the corresponding revision-listing route in tasksets.py (plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasksets.py, lines 197-218).plugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.py (1)
60-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThese four methods duplicate
task_resources.pyexactly.Only the DTO type differs. Two more copies exist in the async class. A generic base parameterized on the DTO and path segment would collapse eight bodies into two. This follows the module's existing pattern, so defer it if you prefer to keep the layers independent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.py` around lines 60 - 108, The methods replace, list_revisions, tag, and retrieve in this class duplicate identical logic from task_resources.py with only the DTO type differing. Create a generic base class parameterized on the DTO type and resource path segment, implement the shared logic once in the base, and have both the sync taskset resource class and the async variant inherit from it to eliminate duplication across all eight method bodies. Follow the existing module pattern for generic parameterization.plugins/nemo-evaluator/tests/api/v2/test_tasks_routes.py (1)
257-273: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the
clientfixture.The
clientfixture already receives the sameentity_store, so this test can request both and drop the duplicated app wiring.♻️ Proposed refactor
-def test_concurrent_replace_returns_409_not_500(entity_store) -> None: +def test_concurrent_replace_returns_409_not_500(client: TestClient, 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🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-evaluator/tests/api/v2/test_tasks_routes.py` around lines 257 - 273, The test_concurrent_replace_returns_409_not_500 function manually creates a FastAPI app, includes the router, instantiates the service, and creates a TestClient, but a client fixture already exists that is configured with the same entity_store. Add client as a parameter to the test function signature alongside entity_store, then remove the manual app initialization, router inclusion, service instantiation, and client creation code. Keep the stubbing of entity_store.update with _stale and the POST/PUT request assertions intact.plugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.py (1)
56-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
replacehides the publish outcome.The route distinguishes "published" (201) from "unchanged" (200), but the SDK discards the status code. A caller that wants to know whether a revision was cut must diff
revisionagainst a prior read. Consider surfacing the status, or document the workaround in the docstring.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.py` around lines 56 - 69, The replace method discards the HTTP status code distinction between a published revision (201) and an unchanged response (200), making it impossible for callers to know whether a new revision was cut. Either modify the replace method to return status information along with the Task object, or update the existing docstring to document that callers must compare the returned Task revision against a prior read to determine if a publish occurred. Choose one approach and implement it consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/evaluator/manage-tasks-tasksets.mdx`:
- Around line 150-159: Update the “Read a specific revision” example using
tasks.list_revisions so it does not imply data[-1] is always revision 1: either
explicitly state that the example assumes the complete history fits on one page,
or select the intended revision by its ordinal across paginated results.
Preserve the comparison between retrieving the selected revision and the current
content.
- Around line 131-134: The revised_task variable used in the tasks.replace call
is undefined, making the code example incomplete. Define revised_task inline
before the replace call so the example is self-contained and runs as written.
Preserve the tasks.replace call and its output unchanged.
In `@plugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.py`:
- Around line 220-226: The replace operation in both task_service.py and
taskset_service.py updates and persists the head via entity_client.update()
before publishing via _publish(), creating a window where publishing failure
leaves the head with content not covered by any revision. Fix this inconsistency
in both files by either reordering to call _publish() before
entity_client.update() commits the head, or by adding compensation logic to
restore the previous head content when _publish() raises an exception. Ensure
the approach matches how create_task and create_taskset handle the same
scenario. Apply this fix at task_service.py lines 220-226 in the code around
entity_client.update(head) and _publish(stored, tags=set(task_input.tags)), and
at taskset_service.py lines 286-290 with the corresponding
entity_client.update(head) and _publish() calls.
In `@plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.py`:
- Around line 136-139: The responses maps in the PUT routes are incomplete. They
declare the status codes but omit the response model for 201 and do not declare
the 409 conflict status that the routes raise. At
plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.py lines 136-139, add the
Task model reference to the HTTP_201_CREATED entry and add a new
HTTP_409_CONFLICT entry to the responses dict. At
plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasksets.py lines 141-144, add
the Taskset model reference to the HTTP_201_CREATED entry and add a new
HTTP_409_CONFLICT entry to the responses dict. After updating both route
handlers, regenerate plugins/nemo-evaluator/openapi/openapi.yaml by running make
refresh-openapi and do not manually edit the OpenAPI file.
In `@plugins/nemo-evaluator/src/nemo_evaluator/content_hash.py`:
- Line 82: Raise the minimum Pydantic version to 2.12.0 in both pyproject.toml
dependency declarations for nemo_platform_plugin and nemo-evaluator, ensuring
unconstrained installations support EntityBase.model_dump with
exclude_computed_fields; leave the existing lockfile version unchanged.
In `@plugins/nemo-evaluator/tests/conftest.py`:
- Around line 112-130: Update the list implementation around the sort and
pagination logic: allow an empty items collection to return an empty page
without raising NotImplementedError, while still validating sortable fields when
items exist. Capture total_results before slicing, and calculate total_pages
from the full result count and page_size instead of hardcoding 1; keep
current_page_size based on the sliced page.
In `@plugins/nemo-evaluator/tests/integration/test_task_revisions.py`:
- Around line 205-207: Update the assertions in the task revision test around
created.tasks[0].root to verify that member equals the task revision’s
content_hash, rather than only checking for a "#" fragment or extracting
pinned_digest. Preserve the existing taskset write coverage while asserting the
stored member resolves to the exact content digest.
In `@plugins/nemo-evaluator/tests/test_content_hash.py`:
- Around line 88-91: Update test_mapping_insertion_order_does_not_affect_digest
to construct equivalent mapping data with different insertion orders, rather
than identical metadata lists. Use a mapping-valued field or other mapping input
supported by _task and content_hash, while preserving the assertion that both
digests are equal; do not use metadata because list order is significant.
---
Nitpick comments:
In `@plugins/nemo-evaluator/src/nemo_evaluator/api/schemas.py`:
- Around line 484-485: Replace the local _DIGEST_FRAGMENT_PATTERN definition in
schemas.py with the shared DIGEST_PATTERN imported from
nemo_evaluator.content_hash, preserving the existing 64-character lowercase
hexadecimal validation and avoiding duplicate definitions.
In `@plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.py`:
- Around line 194-218: Wrap the service call in list_task_revisions in tasks.py
(plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.py, lines 194-218) with
the module’s standard try/except, log failures using logger.exception, and raise
an HTTP 500 while preserving the existing not-found handling. Apply the same
wrapping to the corresponding revision-listing route in tasksets.py
(plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasksets.py, lines 197-218).
In `@plugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.py`:
- Around line 56-69: The replace method discards the HTTP status code
distinction between a published revision (201) and an unchanged response (200),
making it impossible for callers to know whether a new revision was cut. Either
modify the replace method to return status information along with the Task
object, or update the existing docstring to document that callers must compare
the returned Task revision against a prior read to determine if a publish
occurred. Choose one approach and implement it consistently.
In `@plugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.py`:
- Around line 60-108: The methods replace, list_revisions, tag, and retrieve in
this class duplicate identical logic from task_resources.py with only the DTO
type differing. Create a generic base class parameterized on the DTO type and
resource path segment, implement the shared logic once in the base, and have
both the sync taskset resource class and the async variant inherit from it to
eliminate duplication across all eight method bodies. Follow the existing module
pattern for generic parameterization.
In `@plugins/nemo-evaluator/src/nemo_evaluator/task_refs.py`:
- Around line 81-98: The taskset member expansion loop should fetch task
entities and pinned revisions concurrently instead of awaiting each member’s
head and revision calls sequentially. Update the expansion flow around
parse_subentity_ref, entity_client.get, and get_revision to use asyncio.gather
while retaining per-member error handling and messages keyed to task_ref.root
and ref.root, including digest-fragment lookups.
In `@plugins/nemo-evaluator/tests/api/v2/test_tasks_routes.py`:
- Around line 257-273: The test_concurrent_replace_returns_409_not_500 function
manually creates a FastAPI app, includes the router, instantiates the service,
and creates a TestClient, but a client fixture already exists that is configured
with the same entity_store. Add client as a parameter to the test function
signature alongside entity_store, then remove the manual app initialization,
router inclusion, service instantiation, and client creation code. Keep the
stubbing of entity_store.update with _stale and the POST/PUT request assertions
intact.
In `@plugins/nemo-evaluator/tests/test_revisions.py`:
- Around line 103-109: Update the FakeStore.list method to accept the protocol’s
sort and page keyword arguments in addition to workspace, filter_operation, and
page_size. Preserve its existing filter-evaluation behavior while allowing
revisions.list_revisions to pass sort="-created_at" and page=page without
raising TypeError.
- Around line 204-210: Update
test_first_publish_applies_latest_and_indexes_the_digest by removing the
duplicated head.latest_revision assertion and renaming the test to describe only
the latest-tag and revision behavior it verifies, since it does not assert
digest indexing.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6867909a-c938-493f-8c21-4b855b071a6a
📒 Files selected for processing (27)
docs/evaluator/manage-tasks-tasksets.mdxplugins/nemo-evaluator/openapi/openapi.yamlplugins/nemo-evaluator/src/nemo_evaluator/api/schemas.pyplugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/service/taskset_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasksets.pyplugins/nemo-evaluator/src/nemo_evaluator/content_hash.pyplugins/nemo-evaluator/src/nemo_evaluator/entities.pyplugins/nemo-evaluator/src/nemo_evaluator/revisions.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/task_refs.pyplugins/nemo-evaluator/tests/api/service/test_task_service.pyplugins/nemo-evaluator/tests/api/service/test_taskset_service.pyplugins/nemo-evaluator/tests/api/v2/test_tasks_routes.pyplugins/nemo-evaluator/tests/api/v2/test_tasksets_routes.pyplugins/nemo-evaluator/tests/conftest.pyplugins/nemo-evaluator/tests/integration/test_task_revisions.pyplugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.pyplugins/nemo-evaluator/tests/sdk/test_taskset_sdk_resources.pyplugins/nemo-evaluator/tests/test_content_hash.pyplugins/nemo-evaluator/tests/test_revision_entity.pyplugins/nemo-evaluator/tests/test_revisions.pyplugins/nemo-evaluator/tests/test_subentity_refs.pyplugins/nemo-evaluator/tests/test_task_refs.pyservices/core/entities/tests/repository/test_child_entity_filtering.py
SandyChapman
force-pushed
the
schapman/harbor-cli-dataset-endpoint-45cb1d
branch
from
August 5, 2026 11:59
2a18647 to
38648ad
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/evaluator/manage-tasks-tasksets.mdx`:
- Around line 27-29: Update the revision-retention statement near the versioning
description to clarify that earlier revisions remain readable only until their
parent task or taskset is deleted. Preserve the existing behavior description
that replacing content publishes a new revision.
In `@plugins/nemo-evaluator/src/nemo_evaluator/revisions.py`:
- Around line 133-151: Update validate_tag_name to enforce the same [\w\-.]+
reference-fragment syntax accepted by TaskRef, while preserving the existing
empty and digest-shaped validations. Reject tags containing slashes or
whitespace, and add tests covering both invalid forms.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 519e6d18-6014-4110-ad14-264cef39bb24
📒 Files selected for processing (27)
docs/evaluator/manage-tasks-tasksets.mdxplugins/nemo-evaluator/openapi/openapi.yamlplugins/nemo-evaluator/src/nemo_evaluator/api/schemas.pyplugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/service/taskset_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasksets.pyplugins/nemo-evaluator/src/nemo_evaluator/content_hash.pyplugins/nemo-evaluator/src/nemo_evaluator/entities.pyplugins/nemo-evaluator/src/nemo_evaluator/revisions.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/task_refs.pyplugins/nemo-evaluator/tests/api/service/test_task_service.pyplugins/nemo-evaluator/tests/api/service/test_taskset_service.pyplugins/nemo-evaluator/tests/api/v2/test_tasks_routes.pyplugins/nemo-evaluator/tests/api/v2/test_tasksets_routes.pyplugins/nemo-evaluator/tests/conftest.pyplugins/nemo-evaluator/tests/integration/test_task_revisions.pyplugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.pyplugins/nemo-evaluator/tests/sdk/test_taskset_sdk_resources.pyplugins/nemo-evaluator/tests/test_content_hash.pyplugins/nemo-evaluator/tests/test_revision_entity.pyplugins/nemo-evaluator/tests/test_revisions.pyplugins/nemo-evaluator/tests/test_subentity_refs.pyplugins/nemo-evaluator/tests/test_task_refs.pyservices/core/entities/tests/repository/test_child_entity_filtering.py
🚧 Files skipped from review as they are similar to previous changes (21)
- services/core/entities/tests/repository/test_child_entity_filtering.py
- plugins/nemo-evaluator/tests/test_task_refs.py
- plugins/nemo-evaluator/tests/test_subentity_refs.py
- plugins/nemo-evaluator/src/nemo_evaluator/task_refs.py
- plugins/nemo-evaluator/tests/integration/test_task_revisions.py
- plugins/nemo-evaluator/tests/test_revisions.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.py
- plugins/nemo-evaluator/tests/api/v2/test_tasks_routes.py
- plugins/nemo-evaluator/tests/sdk/test_taskset_sdk_resources.py
- plugins/nemo-evaluator/tests/api/v2/test_tasksets_routes.py
- plugins/nemo-evaluator/tests/api/service/test_task_service.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.py
- plugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.py
- plugins/nemo-evaluator/src/nemo_evaluator/entities.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.py
- plugins/nemo-evaluator/tests/conftest.py
- plugins/nemo-evaluator/tests/api/service/test_taskset_service.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasksets.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/service/taskset_service.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.py
- plugins/nemo-evaluator/openapi/openapi.yaml
SandyChapman
force-pushed
the
schapman/harbor-cli-dataset-endpoint-45cb1d
branch
from
August 5, 2026 12:10
38648ad to
47c27f8
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/nemo-evaluator/src/nemo_evaluator/revisions.py`:
- Around line 337-351: Update the publish retry flow in revisions.py so a
conflict during the child create step is treated as proof that the current
ordinal is already allocated, not just a transient failure. In the logic around
_point_tags and the NemoEntityConflictError handling, advance the
allocation/latest revision state after the conflicting rev.N create before
retrying, so the next attempt moves on to N+1 instead of looping on the same
ordinal. Keep the existing refresh-on-contention behavior for head pointer
allocation, but ensure the conflicting child create path also updates the
ordinal state. Add a regression test covering an existing rev.N, a head at N-1,
and new content that verifies the publish succeeds by advancing past the
occupied ordinal.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 20d70923-bf03-4e3a-94d6-c06702109d23
📒 Files selected for processing (27)
docs/evaluator/manage-tasks-tasksets.mdxplugins/nemo-evaluator/openapi/openapi.yamlplugins/nemo-evaluator/src/nemo_evaluator/api/schemas.pyplugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/service/taskset_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasksets.pyplugins/nemo-evaluator/src/nemo_evaluator/content_hash.pyplugins/nemo-evaluator/src/nemo_evaluator/entities.pyplugins/nemo-evaluator/src/nemo_evaluator/revisions.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/task_refs.pyplugins/nemo-evaluator/tests/api/service/test_task_service.pyplugins/nemo-evaluator/tests/api/service/test_taskset_service.pyplugins/nemo-evaluator/tests/api/v2/test_tasks_routes.pyplugins/nemo-evaluator/tests/api/v2/test_tasksets_routes.pyplugins/nemo-evaluator/tests/conftest.pyplugins/nemo-evaluator/tests/integration/test_task_revisions.pyplugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.pyplugins/nemo-evaluator/tests/sdk/test_taskset_sdk_resources.pyplugins/nemo-evaluator/tests/test_content_hash.pyplugins/nemo-evaluator/tests/test_revision_entity.pyplugins/nemo-evaluator/tests/test_revisions.pyplugins/nemo-evaluator/tests/test_subentity_refs.pyplugins/nemo-evaluator/tests/test_task_refs.pyservices/core/entities/tests/repository/test_child_entity_filtering.py
🚧 Files skipped from review as they are similar to previous changes (21)
- services/core/entities/tests/repository/test_child_entity_filtering.py
- plugins/nemo-evaluator/tests/test_subentity_refs.py
- plugins/nemo-evaluator/src/nemo_evaluator/task_refs.py
- plugins/nemo-evaluator/tests/test_task_refs.py
- plugins/nemo-evaluator/tests/api/v2/test_tasksets_routes.py
- plugins/nemo-evaluator/tests/integration/test_task_revisions.py
- plugins/nemo-evaluator/tests/sdk/test_taskset_sdk_resources.py
- plugins/nemo-evaluator/tests/api/v2/test_tasks_routes.py
- plugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.py
- plugins/nemo-evaluator/tests/test_revisions.py
- plugins/nemo-evaluator/tests/conftest.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/service/taskset_service.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.py
- plugins/nemo-evaluator/tests/api/service/test_task_service.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasksets.py
- plugins/nemo-evaluator/src/nemo_evaluator/entities.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.py
- plugins/nemo-evaluator/tests/api/service/test_taskset_service.py
- plugins/nemo-evaluator/openapi/openapi.yaml
SandyChapman
force-pushed
the
schapman/harbor-cli-dataset-endpoint-45cb1d
branch
from
August 5, 2026 12:15
47c27f8 to
df436ba
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
SandyChapman
force-pushed
the
schapman/harbor-cli-dataset-endpoint-45cb1d
branch
from
August 5, 2026 12:18
df436ba to
05071c2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/nemo-evaluator/src/nemo_evaluator/api/schemas.py (1)
476-484: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNormalize member order before hashing.
This validator returns the caller order.
PinnedTaskRefListtherefore preserves[A, B]versus[B, A], while canonical hashing treats list order as significant. Reordering the same taskset members can publish a new revision despite set semantics.Return refs in a stable order here. Add a regression test with reversed digest-pinned members.
Proposed fix
def _reject_duplicate_task_refs(refs: list[TaskRef]) -> list[TaskRef]: ... - return refs + return sorted(refs, key=lambda ref: ref.root)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-evaluator/src/nemo_evaluator/api/schemas.py` around lines 476 - 484, Update _reject_duplicate_task_refs to return the validated refs in a deterministic order, such as sorting by each ref.root, while preserving duplicate rejection. Add a regression test using the same digest-pinned members in reversed order and verify both inputs produce the same canonical hash or revision.
🧹 Nitpick comments (3)
plugins/nemo-evaluator/tests/test_revisions.py (2)
286-291: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
range(1, 50)couples the test to an unstated retry bound.The literal
50must exceed the retry limit inpublish_revision. Import that limit and derive the range from it, so the test tracks the implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-evaluator/tests/test_revisions.py` around lines 286 - 291, Update test_persistent_contention_raises_rather_than_looping to import the retry-limit constant used by publish_revision and derive store.contend_ordinals from that symbol, rather than hard-coding 50. Keep the test’s persistent-conflict behavior and RevisionConflictError assertion unchanged.
153-188: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_headand_head_namedduplicate each other.The two builders differ only in the
nameandintentvalues. Merge them into one helper with both as keyword parameters.♻️ Proposed change
-def _head(store: FakeStore, *, intent: str = "Answer the question.") -> TaskEntity: +def _head(store: FakeStore, *, name: str = "task-1", intent: str = "Answer the question.") -> TaskEntity: head = TaskEntity( - name="task-1", + name=name, workspace="default", intent=intent, inputs=TaskInputs(instruction="What is 2+2?"), metrics=[MetricRef("default/stored-metric")], ) - head._id = "head-1" + 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, "task-1", "default", None)] = stored + store.records[store._key(TaskEntity, name, "default", None)] = stored return headThen replace
_head_named(store, "task-2")with_head(store, name="task-2")at Lines 446 and 459.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-evaluator/tests/test_revisions.py` around lines 153 - 188, Merge _head_named into _head by adding keyword parameters for name and intent, preserving the current defaults and deriving the ID and storage key from the supplied name. Remove _head_named and update its callers to invoke _head with name="task-2".plugins/nemo-evaluator/tests/test_content_hash.py (1)
118-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStrengthen the exclude assertion.
The current assertion proves only that
excludechanges the digest. It passes even ifexcludecorrupted the payload instead of dropping the field. Compare against an entity with no metadata to prove the field was actually removed.♻️ Proposed change
entity = _task() - assert content_hash(entity, exclude={"metadata"}) != content_hash(entity) + assert content_hash(entity, exclude={"metadata"}) != content_hash(entity) + assert content_hash(entity, exclude={"metadata"}) == content_hash(_task(metadata=[]), exclude={"metadata"})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-evaluator/tests/test_content_hash.py` around lines 118 - 122, Strengthen test_extra_exclude_is_honoured by comparing content_hash(entity, exclude={"metadata"}) with the hash of an equivalent entity whose metadata has been removed. Keep the existing assertion only if useful, but ensure the test proves exclusion produces the same digest as the payload without metadata rather than merely changing the digest.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/nemo-evaluator/tests/test_content_hash.py`:
- Around line 132-133: Correct test_absent_vs_empty_collection_differ so its
name matches the values being compared: rename it to
test_empty_and_populated_metrics_differ when retaining _task()’s populated
default, or explicitly construct an entity with metrics absent if testing
absent-versus-empty behavior. Keep the assertion aligned with the selected
scenario.
In `@plugins/nemo-evaluator/tests/test_revisions.py`:
- Around line 72-75: Update _win_race in test_revisions.py so it resolves the
head record from the winning revision’s parent id instead of hardcoding
"task-1"; use the existing _key(TaskEntity, ..., winner.workspace, None) lookup
with the parent-derived identifier from the revision object. Keep the
latest_revision and LATEST_TAG advancement logic unchanged, but ensure the
lookup works for heads created by _head_named with arbitrary names so contention
tests always advance the correct head.
---
Outside diff comments:
In `@plugins/nemo-evaluator/src/nemo_evaluator/api/schemas.py`:
- Around line 476-484: Update _reject_duplicate_task_refs to return the
validated refs in a deterministic order, such as sorting by each ref.root, while
preserving duplicate rejection. Add a regression test using the same
digest-pinned members in reversed order and verify both inputs produce the same
canonical hash or revision.
---
Nitpick comments:
In `@plugins/nemo-evaluator/tests/test_content_hash.py`:
- Around line 118-122: Strengthen test_extra_exclude_is_honoured by comparing
content_hash(entity, exclude={"metadata"}) with the hash of an equivalent entity
whose metadata has been removed. Keep the existing assertion only if useful, but
ensure the test proves exclusion produces the same digest as the payload without
metadata rather than merely changing the digest.
In `@plugins/nemo-evaluator/tests/test_revisions.py`:
- Around line 286-291: Update
test_persistent_contention_raises_rather_than_looping to import the retry-limit
constant used by publish_revision and derive store.contend_ordinals from that
symbol, rather than hard-coding 50. Keep the test’s persistent-conflict behavior
and RevisionConflictError assertion unchanged.
- Around line 153-188: Merge _head_named into _head by adding keyword parameters
for name and intent, preserving the current defaults and deriving the ID and
storage key from the supplied name. Remove _head_named and update its callers to
invoke _head with name="task-2".
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9de48a86-12db-4d8f-a71a-41ceb76ed5c3
📒 Files selected for processing (27)
docs/evaluator/manage-tasks-tasksets.mdxplugins/nemo-evaluator/openapi/openapi.yamlplugins/nemo-evaluator/src/nemo_evaluator/api/schemas.pyplugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/service/taskset_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasksets.pyplugins/nemo-evaluator/src/nemo_evaluator/content_hash.pyplugins/nemo-evaluator/src/nemo_evaluator/entities.pyplugins/nemo-evaluator/src/nemo_evaluator/revisions.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/task_refs.pyplugins/nemo-evaluator/tests/api/service/test_task_service.pyplugins/nemo-evaluator/tests/api/service/test_taskset_service.pyplugins/nemo-evaluator/tests/api/v2/test_tasks_routes.pyplugins/nemo-evaluator/tests/api/v2/test_tasksets_routes.pyplugins/nemo-evaluator/tests/conftest.pyplugins/nemo-evaluator/tests/integration/test_task_revisions.pyplugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.pyplugins/nemo-evaluator/tests/sdk/test_taskset_sdk_resources.pyplugins/nemo-evaluator/tests/test_content_hash.pyplugins/nemo-evaluator/tests/test_revision_entity.pyplugins/nemo-evaluator/tests/test_revisions.pyplugins/nemo-evaluator/tests/test_subentity_refs.pyplugins/nemo-evaluator/tests/test_task_refs.pyservices/core/entities/tests/repository/test_child_entity_filtering.py
🚧 Files skipped from review as they are similar to previous changes (18)
- plugins/nemo-evaluator/tests/test_subentity_refs.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.py
- plugins/nemo-evaluator/src/nemo_evaluator/entities.py
- plugins/nemo-evaluator/tests/integration/test_task_revisions.py
- plugins/nemo-evaluator/tests/api/v2/test_tasks_routes.py
- services/core/entities/tests/repository/test_child_entity_filtering.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.py
- plugins/nemo-evaluator/tests/conftest.py
- plugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.py
- plugins/nemo-evaluator/src/nemo_evaluator/task_refs.py
- plugins/nemo-evaluator/tests/test_task_refs.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.py
- plugins/nemo-evaluator/tests/api/service/test_taskset_service.py
- plugins/nemo-evaluator/tests/api/service/test_task_service.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasksets.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/service/taskset_service.py
- plugins/nemo-evaluator/openapi/openapi.yaml
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/evaluator/manage-tasks-tasksets.mdx (1)
163-166: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPaginate before selecting revision 1.
list_revisions()returns one page. When revision 1 is outside that page,next(...)raisesStopIteration. Fetch pages until the ordinal is found, or use a known digest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/evaluator/manage-tasks-tasksets.mdx` around lines 163 - 166, The revision lookup example in the taskset docs assumes revision 1 is always present in the first page returned by list_revisions(), but this can raise StopIteration when that ordinal is on a later page. Update the example around page and digest to either iterate through successive list_revisions() pages until revision == 1 is found, or switch to using a known digest so the guidance matches the paginated API behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/nemo-evaluator/src/nemo_evaluator/revisions.py`:
- Around line 331-334: Update the deduplication branch in _apply_pointers so an
existing digest is reused only when that revision is the current latest
revision; if find_by_digest matches older history, continue through new-revision
publication instead. Add a regression test covering revision 2 reverting to
revision-1 content and verify current and `#latest` reads remain consistent.
In `@plugins/nemo-evaluator/tests/test_task_refs.py`:
- Around line 36-45: Update _store to capture each entity returned by
client.create and use that persisted result when publishing TaskEntity
revisions, rather than the original input. Ensure the returned task is also used
for head_digest and the subsequent update in
test_expansion_uses_the_pinned_revision_not_current_content, avoiding reliance
on FakeEntityStore.create mutating inputs.
---
Duplicate comments:
In `@docs/evaluator/manage-tasks-tasksets.mdx`:
- Around line 163-166: The revision lookup example in the taskset docs assumes
revision 1 is always present in the first page returned by list_revisions(), but
this can raise StopIteration when that ordinal is on a later page. Update the
example around page and digest to either iterate through successive
list_revisions() pages until revision == 1 is found, or switch to using a known
digest so the guidance matches the paginated API behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7c5b04c2-740c-4b31-bef4-2ab8480bf424
📒 Files selected for processing (27)
docs/evaluator/manage-tasks-tasksets.mdxplugins/nemo-evaluator/openapi/openapi.yamlplugins/nemo-evaluator/src/nemo_evaluator/api/schemas.pyplugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/service/taskset_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasksets.pyplugins/nemo-evaluator/src/nemo_evaluator/content_hash.pyplugins/nemo-evaluator/src/nemo_evaluator/entities.pyplugins/nemo-evaluator/src/nemo_evaluator/revisions.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/task_refs.pyplugins/nemo-evaluator/tests/api/service/test_task_service.pyplugins/nemo-evaluator/tests/api/service/test_taskset_service.pyplugins/nemo-evaluator/tests/api/v2/test_tasks_routes.pyplugins/nemo-evaluator/tests/api/v2/test_tasksets_routes.pyplugins/nemo-evaluator/tests/conftest.pyplugins/nemo-evaluator/tests/integration/test_task_revisions.pyplugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.pyplugins/nemo-evaluator/tests/sdk/test_taskset_sdk_resources.pyplugins/nemo-evaluator/tests/test_content_hash.pyplugins/nemo-evaluator/tests/test_revision_entity.pyplugins/nemo-evaluator/tests/test_revisions.pyplugins/nemo-evaluator/tests/test_subentity_refs.pyplugins/nemo-evaluator/tests/test_task_refs.pyservices/core/entities/tests/repository/test_child_entity_filtering.py
🚧 Files skipped from review as they are similar to previous changes (19)
- plugins/nemo-evaluator/tests/sdk/test_taskset_sdk_resources.py
- plugins/nemo-evaluator/tests/test_subentity_refs.py
- plugins/nemo-evaluator/tests/test_revisions.py
- plugins/nemo-evaluator/src/nemo_evaluator/task_refs.py
- services/core/entities/tests/repository/test_child_entity_filtering.py
- plugins/nemo-evaluator/tests/sdk/test_task_sdk_resources.py
- plugins/nemo-evaluator/tests/integration/test_task_revisions.py
- plugins/nemo-evaluator/tests/api/v2/test_tasksets_routes.py
- plugins/nemo-evaluator/tests/api/service/test_taskset_service.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.py
- plugins/nemo-evaluator/tests/api/v2/test_tasks_routes.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/service/task_service.py
- plugins/nemo-evaluator/src/nemo_evaluator/entities.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/service/taskset_service.py
- plugins/nemo-evaluator/tests/conftest.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasksets.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/task_resources.py
- plugins/nemo-evaluator/openapi/openapi.yaml
SandyChapman
force-pushed
the
schapman/harbor-cli-dataset-endpoint-45cb1d
branch
4 times, most recently
from
August 5, 2026 13:15
c69388a to
58fccbe
Compare
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.<n>`, 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#<tag-or-digest>` — 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 <schapman@nvidia.com>
SandyChapman
force-pushed
the
schapman/harbor-cli-dataset-endpoint-45cb1d
branch
from
August 5, 2026 13:24
58fccbe to
a584f54
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
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.
This is phase 0 of backing a Harbor-compatible dataset publish endpoint with NeMo entities — Harbor pins dataset members by digest, which we had no way to represent.
How it works
A revision is an immutable child entity named
rev.<n>, addressed by a SHA-256 digest of its content. The head record keeps current content plus mutable pointers (latest_revision, and a tag → ordinal map). Digest lookups are a real(parent, data.content_hash)query, not a denormalized index.References gain the platform's standard
#sub-entity fragment —workspace/name#<tag-or-digest>— with an absent fragment meaninglatest. This uses the same convention filesets already use for a contained file.Taskset membership is resolved to exact digests on write, and taskset expansion at eval time loads each member's pinned revision. That second half matters: without it, membership was pinned in storage and ignored at runtime, so a suite looked reproducible and wasn't.
API
POST/{tasks,tasksets}/{name}PUT/{tasks,tasksets}/{name}GET/{tasks,tasksets}/{name}/revisionsGET/{tasks,tasksets}/{name}/revisions/{revision}PUT/{tasks,tasksets}/{name}/tags/{tag}?revision=Publishing is idempotent against the current revision: re-PUTting unchanged content cuts no revision but still applies tags, which is how you tag a revision after the fact. Reverting to older content does publish — the record genuinely changed, and deduping onto the old revision would leave the head serving content
latestdoesn't name.Reviewing this
It's a large diff, but most of it isn't judgement calls:
openapi/openapi.yamlmake refresh-openapi— skimWithin the source,
tasks.py/tasksets.py, the two services, and the two SDK modules are near-symmetric pairs — read one and skim its twin.Suggested order, highest value first:
entities.py— what a revision is, and which fields the digest covers (REVISION_POINTER_FIELDS/REVISION_SELF_FIELDS). Everything else depends on this projection.revisions.py— publish, resolve, tag. The concurrency lives here: ordinal allocation, the forward-onlylatestrule, and what happens to head content when a pointer write loses its race.api/service/task_service.py— where those semantics become observable (200 vs 201, the rollback, the no-op path).Worth the most scrutiny: the interleavings in
publish_revisionand_point_tags. Three defects found during review were concurrency or ordering cases where a comment described the intended invariant and the code handled only the common path.Invariants
latestonly moves forward and can't be moved by hand. Ordinal allocation is serialized by the child create and retries on contention.#latestread always agree. The head's content is always the revisionlatestnames — including when a publish fails midway, and when two publishers race and the loser's revision is older.Testing
RUN_AGENT_EVAL_INTEGRATION=1), covering what only real persistence confirms: parent-scoped ordinals, the digest query, server-side ordering, FK cascade, cross-record isolationcontent_hash100%,task_refs100%,revisions96%, services 94%tools/lint/lint-all.sh: 13/13 passNotes for review
plugins/nemo-customizer/openapi/openapi.yamlis deliberately not included.make refresh-openapiregenerates ~1100 lines there from pre-existing drift unrelated to this change; reverted to keep the diff focused. Worth a separate cleanup PR.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Bug Fixes