feat(evaluator): let evaluations pin a taskset revision - #1129
Merged
Conversation
Contributor
Base automatically changed from
schapman/harbor-cli-dataset-endpoint-45cb1d
to
main
August 6, 2026 14:52
SandyChapman
force-pushed
the
taskset-ref-revision-pinning/schapman
branch
from
August 6, 2026 15:57
e3c0575 to
32fc4f7
Compare
Contributor
|
A `TasksetRef` could only ever name a taskset's head, so replacing the taskset changed what a re-submitted spec evaluated. Member content was already digest-pinned within a revision, which made an evaluation look reproducible while its membership could still move underneath it. `TasksetRef` moves onto `_SUBENTITY_REF_PATTERN`, so a reference may carry `#<tag-or-digest>` exactly as a `TaskRef` does, with an absent fragment meaning `latest`. `MetricRef` stays on the narrower pattern until metrics gain revisions. `resolve_taskset_ref` resolves that fragment and expands the members of the named taskset revision. Both the pinned and the bare paths go through `get_revision` rather than reading the head's own `tasks`: a head and its `latest` revision are guaranteed to agree, and resolving one way for pinned refs and another for bare ones would let the two drift apart. It also buys content verification for the bare case. The two pins cover different things and compose — a bare ref grades identical task content across re-runs, and a pinned ref additionally holds membership still across a `replace` that adds or drops a task. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
taskset-ref-revision-pinning/schapman
branch
from
August 7, 2026 11:22
32fc4f7 to
7ef23d6
Compare
SandyChapman
marked this pull request as ready for review
August 7, 2026 12:13
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTaskset references now support optional tag or digest revisions. Evaluation resolves membership from the selected taskset revision and loads each member at its pinned revision. Schemas, OpenAPI definitions, tests, and documentation cover tracking, pinning, validation, and unresolved revisions. ChangesTaskset revision resolution
Sequence Diagram(s)sequenceDiagram
participant Evaluator
participant task_refs
participant EntityClient
Evaluator->>task_refs: resolve TasksetRef
task_refs->>EntityClient: get_revision taskset
EntityClient-->>task_refs: TasksetRevisionEntity
task_refs->>EntityClient: load pinned task revisions
EntityClient-->>task_refs: resolved task members
task_refs-->>Evaluator: expanded taskset
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 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 `@plugins/nemo-evaluator/src/nemo_evaluator/api/schemas.py`:
- Around line 237-240: Qualify the TasksetRef reproducibility documentation:
bare refs resolve latest and keep task content fixed only while that selected
taskset revision remains current; recommend digest-pinned taskset refs for
reproducibility across replace operations. Apply this correction in
plugins/nemo-evaluator/src/nemo_evaluator/api/schemas.py:237-240, regenerate the
matching description in plugins/nemo-evaluator/openapi/openapi.yaml:5167-5172,
update the resolver comment in
plugins/nemo-evaluator/src/nemo_evaluator/task_refs.py:96-98, and correct the
table explanation and recommendation in
docs/evaluator/manage-tasks-tasksets.mdx:352-358.
In `@plugins/nemo-evaluator/tests/test_task_refs.py`:
- Around line 63-71: Update the non-digest branch in the task-reference
resolution flow to use the client’s get_revision method for resolving the
fragment (such as “blessed”), rather than head_digest(task). Store the digest
returned by that resolved revision, while preserving the existing
NemoEntityNotFoundError fallback and pinned TaskRef construction.
🪄 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: 7bca7d3a-d40e-4351-8f62-8589506d0cfd
📒 Files selected for processing (6)
docs/evaluator/manage-tasks-tasksets.mdxplugins/nemo-evaluator/openapi/openapi.yamlplugins/nemo-evaluator/src/nemo_evaluator/api/schemas.pyplugins/nemo-evaluator/src/nemo_evaluator/task_refs.pyplugins/nemo-evaluator/tests/test_subentity_refs.pyplugins/nemo-evaluator/tests/test_task_refs.py
Address CodeRabbit review on #1129. The docstring, resolver comment, and docs table all claimed a bare ref "grades identical task content across re-runs". That overstates it. Members are re-resolved on every write, so a `replace` submitting the same member names can still pin newer task digests — changing the content a bare ref expands to, not just the membership. What actually holds is narrower and worth stating exactly, since the whole feature is about reproducibility: a taskset revision pins its members by digest, so no ref is disturbed when a member republishes on its own. The forms differ only on `replace`, where a bare ref follows the new revision and a pinned ref does not. The test helper's member pinning also diverged from the write path: a member named `task#blessed` was pinned to the head digest rather than to the revision the tag names. It now resolves non-digest fragments through `get_revision`, matching `resolve_revision`. The two agree for a bare ref, whose fragment is `latest`, so no existing fixture changes — and a new test covers the case that was silently wrong. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
The previous commit fixed a test helper that pinned tag-qualified members to the head digest. The same hole existed in the production path's coverage, which is the one that matters. `_FakeTaskService.resolve_revision` derived its digest from `(workspace, name)` and ignored `fragment` entirely, so every taskset service assertion about digest-pinned membership would pass unchanged if `_pin_member` dropped the fragment. The fake now folds the fragment into the digest, and a new test asserts a `#blessed` member is stored as the tagged revision's digest and not the current one. `test_resolve_revision_returns_the_digest_for_a_tag` never used a tag: it called `resolve_revision` with no fragment against a task with a single revision, where every fragment resolves alike. Split into three that mean what they say — default-to-latest, a tag left behind on an older revision, and a digest round-trip — each against a task with two revisions so the fragment can be observed. Verified by mutation: dropping the fragment in `_pin_member` now fails one test, and ignoring it in `TaskService.resolve_revision` fails two. Neither was caught before. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
arpitsardhana
approved these changes
Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
A
TasksetRefcould only ever name a taskset's head, so replacing the taskset changed what a re-submitted spec evaluated. Member content was already digest-pinned within a revision, which made an evaluation look reproducible while its membership could still move underneath it.This lets a reference pin the taskset revision too:
default/geography-suite#<tag-or-digest>.Fast-follow to #1023, raised in review there (thread on
api/schemas.py). Stacked on #1023 — please merge that first; this targets its branch.The two pins compose
They cover different things:
TasksetRef("suite")TasksetRef("suite#<digest>")A bare ref already grades identical task content across re-runs. Pinning the taskset additionally holds membership still across a
replacethat adds or drops a task — what you want when a benchmark number has to stay comparable.How
TasksetRefmoves onto_SUBENTITY_REF_PATTERN, so it carries#<tag-or-digest>exactly asTaskRefdoes, with an absent fragment meaninglatest.MetricRefstays on the narrower pattern until metrics gain revisions — the comment above that constant already anticipated this split and is updated.resolve_taskset_refresolves the fragment and expands the members of the named revision. Both the pinned and the bare paths go throughget_revisionrather than reading the head's owntasks: a head and itslatestrevision are guaranteed to agree, and resolving one way for pinned refs and another for bare ones would let the two drift apart on the next bug. It also buys content verification for the bare case.A fragment that no longer resolves fails the evaluation rather than falling back to the current revision.
Reviewing this
Small: 6 files, +249/−51, and 160 of the added lines are tests.
task_refs.py— the resolution change; everything else follows from it.api/schemas.py— the pattern move and whyMetricRefstays behind.The test fixtures needed fixing, and that is worth a look. They were building tasksets the services cannot produce: inserted with no published revision, and with bare (unpinned) member refs. Routing resolution through
get_revisionsurfaced that immediately, andTasksetRevisionEntitythen rejected the unpinned members outright._create_published/_pin_membersnow publish and digest-pin the way_resolved_contentdoes, so every fixture describes a state reachable through the real API.test_sibling_ref_types_still_reject_fragmentswas parameterized overMetricRefandTasksetRef; it is split, since half of its premise no longer holds.Testing
replacewith identical expansion; unresolvable digest and unresolvable tag both raise; field-level accept/reject of fragment shapes.tools/lint/lint-all.sh: 12/13.lint-web-sdkfails locally only — this box has noweb/node_modulesand runs Node 22.19 against a>=22.23.2engine pin. Its output is gitignored and no web code is touched here, so CI should pass it; I could not verify that locally.openapi.yamlregenerated withmake refresh-openapi(+11/−10: theTasksetRefpattern and description only).Notes
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation