feat(evaluator-sdk): skip re-execution if trial is present - #1040
Merged
Conversation
Signed-off-by: Nick Goncharenko <ngoncharenko@nvidia.com>
Contributor
📝 WalkthroughWalkthroughChangesHarbor cache and resume reliability
Sequence Diagram(s)sequenceDiagram
participant NativeRunner
participant JobDirectory
participant Harbor
participant TrialResults
NativeRunner->>JobDirectory: Validate cache stamp and task coverage
NativeRunner->>Harbor: Resume or execute the resolved job
Harbor->>TrialResults: Write trial results
NativeRunner->>JobDirectory: Adapt results and write the cache stamp
Possibly related PRs
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.
🧹 Nitpick comments (1)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.py (1)
369-369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the string-based annotation.
"hashlib._Hash"is a string hint on a private typeshed-only symbol. The coding guidelines require concrete type hints.hashlib._Hashcannot be imported at runtime, so use a protocol you own or a concrete alias instead.♻️ Proposed refactor
-def _feed(digest: "hashlib._Hash", label: bytes, payload: bytes) -> None: +class _Digest(Protocol): + def update(self, data: bytes, /) -> None: ... + + +def _feed(digest: _Digest, label: bytes, payload: bytes) -> None:Add
from typing import Protocolto the imports.As per coding guidelines: "prefer concrete type hints over string-based type hints".
🤖 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 `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.py` at line 369, Replace the string annotation on _feed’s digest parameter with a concrete runtime-safe type, such as an owned Protocol defining the hash interface used by _feed; add the required typing import and avoid referencing the private, typeshed-only hashlib._Hash symbol.Source: Coding guidelines
🤖 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.
Nitpick comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.py`:
- Line 369: Replace the string annotation on _feed’s digest parameter with a
concrete runtime-safe type, such as an owned Protocol defining the hash
interface used by _feed; add the required typing import and avoid referencing
the private, typeshed-only hashlib._Hash symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2faf796d-1eed-4aac-93d1-3175cdaacd25
⛔ Files ignored due to path filters (1)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/harbor_runtime.pyis excluded by!sdk/**
📒 Files selected for processing (3)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime_e2e.py
Contributor
|
ngoncharenko
changed the title
arpitsardhana
approved these changes
Aug 3, 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.
Summary
Cache walkthrough
Why
job_namewas not enoughOld, broken cache key:
Example failure:
The same stale-result hole existed when a task such as
tests/test.shchanged, or when a result-affecting runtime option changed.What the SDK stores
For a pinned
job_name, the SDK writes a versioned stamp beside Harbor results:The stamp covers the agent contents, the task contents and executable bits, and the
HarborRuntimeConfigoptions that can change results. Runtime/build noise is excluded. Ifjobs_diris nested inside the agent or dataset tree, that result subtree is excluded too; otherwise each completed run would change its own fingerprint and make the next run look stale.Reuse, resume, or rerun
force_rerun=TrueFileExistsErrorCallers choose whether to force a wipe.
HarborAgentTaskRunnerowns whether existing results are valid for the current inputs and whether incomplete work can resume. This keeps cache validity and per-task resume in the SDK rather than reimplementing them in each consumer.Stable scoped agent imports
Harbor persists the agent import path in
JobConfigand compares it before resuming. A random UUID in that path made an unchanged agent look different on every run, so partial resume was unreachable wheneveragent_dirwas used.The import package name is now derived from an agent-content digest. Unchanged contents produce the same persisted import path; edited contents produce a different one. Identical agents can share that package name safely because overlapping scopes are reference-counted and removed only after the final scope exits. The same nested-
jobs_direxclusion used by the cache stamp is applied to this digest so accumulating results cannot move the import path. Symlink resolution failures, including loops on supported Python versions, degrade to a safe unresolved absolute path instead of crashing this best-effort guard.Relationship to the Experimentalist PR
PR #955 targets this branch, so its diff contains only Experimentalist integration code. This change carries the cache/resume work previously merged into that feature branch through #964, plus the follow-up symlink-loop hardening.
Refs AALGO-312, AALGO-427, and AALGO-430.
Verification
uv run --frozen pytest packages/nemo_evaluator_sdk/tests/agent_eval -q: 475 passed, 1 skipped (real Harbor/Docker coverage included)uv run --frozen ruff check packages/nemo_evaluator_sdk: passeduv run --frozen ruff format --check packages/nemo_evaluator_sdk: 206 files already formattedgit diff --check origin/main...HEAD: passedSummary by CodeRabbit
New Features
Bug Fixes