diff --git a/packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.py b/packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.py index cfd3b3da5a..374f2defa6 100644 --- a/packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.py +++ b/packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.py @@ -32,9 +32,11 @@ from __future__ import annotations import contextlib +import hashlib import importlib.machinery import json import logging +import os import re import shutil import sys @@ -45,7 +47,6 @@ from pathlib import Path from types import ModuleType from typing import Any -from uuid import uuid4 from nemo_evaluator_sdk.agent_eval.results import AgentEvalResult from nemo_evaluator_sdk.agent_eval.scores import AgentEvalScoreStatus @@ -71,6 +72,42 @@ _AGENT_IMPORT_ROOT = "_nemo_evaluator_harbor_agents" # Guards the sys.modules mutation while injecting/removing scoped agent packages. _IMPORT_LOCK = threading.Lock() +# Open scopes per content-addressed agent package. Identical agent contents share a +# package name, so teardown must wait for the last scope rather than the first. +_AGENT_PACKAGE_REFCOUNTS: dict[str, int] = {} +# Characters of the agent-content digest used to disambiguate the package name. Long +# enough that distinct agents don't collide; short enough to keep import paths (and +# Harbor's persisted JobConfig) readable. +_IMPORT_DIGEST_CHARS = 12 +# Records which inputs produced a job dir, so a rerun can tell a reusable cache from +# a stale one. A file, not a directory: Harbor rmtree's stray directories in a job dir. +CACHE_STAMP_FILENAME = ".nemo-eval-harbor-cache.json" +# Public so downstreams can assert the SDK is new enough to own cache staleness. +CACHE_STAMP_VERSION = 1 +# Excluded from the cache fingerprint — see :func:`_cache_stamp` for why each one. +_CACHE_IRRELEVANT_OPTIONS = frozenset( + {"jobs_dir", "job_name", "force_rerun", "quiet", "n_concurrent_trials", "agent_dir", "reward_key"} +) +# Where Harbor persists the JobConfig it will compare a resume against. +_HARBOR_JOB_CONFIG_FILENAME = "config.json" +# Fields Harbor's own JobConfig equality ignores, so they can never be why it refused +# to resume. Pinned against Harbor upstream by the drift-guard test. +_HARBOR_EQ_IGNORED_FIELDS = frozenset({"job_name", "debug"}) +# How Harbor says "this job dir cannot be resumed": one from the JobConfig comparison +# in `Job.create`, one from the lock.json check early in `Job.run`. Matching on the +# message is deliberate coupling, and it fails in the safe direction — an +# unrecognized FileExistsError propagates untouched rather than costing a job dir, so +# a Harbor reword degrades to a loud crash, never to a silent deletion. +_HARBOR_RESUME_REFUSALS = ("resumed with a different config", "does not match the resolved job lock") +# Cap on each value rendered into the "what differed" log line: enough for a scalar +# like `n_concurrent_trials`, bounded for a whole nested `agents` list. +_DRIFT_VALUE_CHARS = 80 +# Derived/VCS noise skipped when digesting a directory. Deliberately NOT skipped: +# `node_modules` and other vendored dependency trees, which ship with the agent and +# change what it does. `.venv`/`.uv` stay skipped because they are environment, not +# deliverable — the Harbor wrapper does not upload them into the task container. +_DIGEST_SKIP_DIRS = frozenset({".git", "__pycache__", ".venv", ".uv", ".mypy_cache", ".pytest_cache"}) +_DIGEST_CHUNK_BYTES = 1 << 20 RunJob = Callable[[], Awaitable[None]] @@ -209,17 +246,78 @@ async def run_tasks( In native mode the dataset directory is recovered from the tasks (each carries ``metadata['harbor_dataset_path']`` from :func:`discover_harbor_tasks`) unless a ``dataset_path`` override was given, - so callers don't repeat it. ``job_dir`` doubles as a cache: the Harbor run - is skipped and results are simply re-adapted when every requested task - already has ``n_attempts`` completed, non-errored results there (unless - ``force_rerun`` is set). The cache only engages when the config pins a - stable ``job_name``; the default timestamped name never hits it. + so callers don't repeat it. + + ``job_dir`` doubles as a cache. Results are served straight off it, without + importing Harbor at all, only when **both** hold: every requested task already + has ``n_attempts`` completed, non-errored results there, *and* the directory + carries a cache stamp matching this run's inputs (agent contents, task + contents, result-affecting options). + + Otherwise Harbor runs, and what happens to the directory depends on *which* + check failed. A **stamp mismatch** discards it first: those results came from + different inputs, so there is nothing safe to resume onto. A directory that + merely lacks **coverage** — stamp matches, but not enough completed results — + is handed to Harbor intact so its per-trial resume keeps the finished trials + and runs only what is missing. Harbor may still refuse a directory on its own + (stricter) terms; :func:`_build_native_job` then discards it and re-runs. + + The cache only engages when the config pins a stable ``job_name``; with the + default timestamped name no fingerprint is computed at all. + + Assumes a **single writer per job directory**. Neither this runtime nor + Harbor locks it, so two processes sharing a pinned ``job_name`` on a shared + volume will race. """ if self._config is not None: dataset_path = self._dataset_path or _dataset_path_from_tasks(tasks) - job_dir, run_job = _build_native_job(self._config, dataset_path, self._task_names) - if self._config.force_rerun or not _all_tasks_cached(job_dir, tasks, n_attempts=self._config.n_attempts): + job_name, job_dir = _resolve_job_dir(self._config) + + # Only fingerprint when the answer can depend on it: an unpinned job name + # can never hit, and force_rerun/a missing dir already decided. This keeps + # the digest I/O off every run of callers that don't pin a job name. + stamp: dict[str, Any] | None = None + if self._config.job_name is None or self._config.force_rerun or not job_dir.is_dir(): + stale = True + else: + stamp = _cache_stamp(self._config, dataset_path, tasks) + stale = _cache_is_stale(job_dir, stamp) + + if stale or not _all_tasks_cached(job_dir, tasks, n_attempts=self._config.n_attempts): + job_dir, run_job = _build_native_job( + self._config, + dataset_path, + self._task_names, + job_name=job_name, + # Discard only when the inputs changed. Otherwise leave it off so + # Harbor resumes per trial and keeps completed work — including + # with `agent_dir` set, now that the scoped import path is + # content-addressed rather than a fresh uuid per run and Harbor's + # JobConfig comparison can therefore match (AALGO-430). + force_rerun=(self._config.force_rerun or stale), + ) + # Fingerprint the inputs *before* running and confirm they are + # unchanged afterwards. Stamping only the post-run state would label + # results produced from the old sources with the new fingerprint, so + # a later run would happily serve them. Covers what Harbor actually + # ran: with no task_names filter that is the whole dataset, and + # recording only the requested subset would make the next full-set + # run look stale and re-run a complete job dir. + coverage = _stamp_coverage(dataset_path, tasks, self._task_names) + before = _cache_stamp(self._config, dataset_path, coverage) await run_job() + if self._config.job_name is not None: + after = _cache_stamp(self._config, dataset_path, coverage) + if after == before: + _write_cache_stamp(job_dir, after) + else: + # Leaving it unstamped re-runs next time, which is the safe + # direction: we cannot say which inputs produced these results. + logger.warning( + "Agent or task contents changed while Harbor job %s was running; leaving it unstamped " + "so the next run re-executes rather than trusting these results.", + job_dir, + ) return build_trials_from_job_dir(job_dir, tasks, reward_key=self._reward_key) if self._job_dir is None: # unreachable: __init__ guarantees config or job_dir @@ -268,10 +366,352 @@ def _all_tasks_cached(job_dir: Path, tasks: Sequence[AgentEvalTask], *, n_attemp return all(counts.get(task.id, 0) >= n_attempts for task in tasks) +def _feed(digest: "hashlib._Hash", label: bytes, payload: bytes) -> None: + """Append a length-framed field to ``digest``. + + Framing matters: concatenating ``name \0 content \0`` is ambiguous because file + *contents* may contain NUL, so two different trees can produce an identical byte + stream. Prefixing every variable-length field with its length makes the encoding + injective, which is what stops a collision from being read as "unchanged". + """ + digest.update(label) + digest.update(len(payload).to_bytes(8, "big")) + digest.update(payload) + + +def _safe_resolve(path: Path) -> Path: + """``Path.resolve()`` that degrades instead of raising. + + The digest is a best-effort guard, not a reason to fail a run that would + otherwise succeed, so fall back to the unresolved absolute path. + + ``RuntimeError`` is caught alongside ``OSError`` and is the case that actually + fires: on CPython 3.12 — the floor this package targets — a **symlink loop** + surfaces as ``RuntimeError("Symlink loop from ...")``, because ``resolve()`` + translates ``ELOOP`` before re-raising. It is not an ``OSError``, so catching + only that would let a loop under a task directory kill the run. A loop raises + deterministically, not as a race. ``OSError`` covers the narrower case of a + symlink that disappears mid-walk, since ``resolve()`` calls ``os.readlink``. + + Both are 3.12/3.13 behaviours: 3.14 resolves a loop without raising at all. + """ + try: + return path.resolve() + except (OSError, RuntimeError): + return path.absolute() + + +def _is_executable(path: Path) -> bool: + """Whether the owner-execute bit is set, following symlinks. + + Only the execute bit, mirroring what git tracks: read/write bits vary with umask + and would evict the cache for nothing, but flipping +x on ``tests/test.sh`` or an + agent entrypoint genuinely changes what Harbor does. + """ + try: + return bool(path.stat().st_mode & 0o100) + except OSError: + return False + + +def _digest_directory(root: Path, *, exclude: frozenset[Path] = frozenset()) -> str: + """Content-hash a directory tree, skipping build/VCS noise and excluded roots. + + Contents rather than mtimes: callers routinely materialize the directory with + ``copytree`` (the optimizer does, per candidate), which rewrites every mtime and + would defeat the cache entirely. + + ``exclude`` takes *resolved* directories to skip wholesale. It exists because + ``jobs_dir`` is caller-chosen and may sit **under** the dataset or agent + directory; without excluding it the digest would hash the growing results tree + it is meant to validate, and would never stabilize. + + Symlinks are **followed**, not skipped. Skipping them silently defeats the whole + guard: a directory assembled out of links to shared sources would hash to the + empty digest, so edits behind those links would never invalidate the cache. The + link target is folded in alongside the contents, so re-pointing a link is a + change even when both targets happen to hold identical bytes. + + Unreadable or vanished files are folded in as a marker rather than raised: a + transient read failure must not kill a run that would otherwise have succeeded. + """ + digest = hashlib.sha256() + if not root.is_dir(): + return digest.hexdigest() + # Keep only exclusions strictly *inside* this tree. An excluded root that is an + # ancestor of (or equal to) `root` would otherwise match every entry and yield + # an empty digest — silently disabling invalidation for the whole directory, + # which is exactly the failure this function exists to prevent. jobs_dir being + # a parent of the agent/task dir is a legitimate layout, not a reason to stop + # hashing it. + root_resolved = _safe_resolve(root) + excluded = { + resolved + for resolved in (_safe_resolve(path) for path in exclude) + if resolved != root_resolved and resolved.is_relative_to(root_resolved) + } + # Resolved dirs already walked, so a symlink cycle terminates instead of hanging. + visited: set[Path] = set() + + def walk(directory: Path) -> None: + resolved_dir = _safe_resolve(directory) + if resolved_dir in visited: + return + visited.add(resolved_dir) + try: + entries = sorted(directory.iterdir()) + except OSError as exc: + logger.warning("Could not list %s while fingerprinting %s: %s", directory, root, exc) + _feed(digest, b"unlistable", b"") + return + for path in entries: + if path.name in _DIGEST_SKIP_DIRS: + continue + resolved = _safe_resolve(path) + if any(resolved == item or item in resolved.parents for item in excluded): + continue + + _feed(digest, b"name", path.relative_to(root).as_posix().encode("utf-8")) + _feed(digest, b"mode", b"x" if _is_executable(path) else b"-") + if path.is_symlink(): + # Record where the link points, so retargeting counts as a change. + try: + target = os.readlink(path).encode("utf-8") + except OSError as exc: + # The link vanished mid-walk. Same contract as an unreadable + # file: degrade to a marker rather than kill the run. + logger.warning("Could not read link %s while fingerprinting %s: %s", path, root, exc) + target = b"" + _feed(digest, b"symlink", target) + if path.is_dir(): + _feed(digest, b"dir", b"") + walk(path) + continue + if not path.is_file(): + # Broken link, socket, fifo: nothing to hash, but its presence counts. + _feed(digest, b"not-a-file", b"") + continue + content = hashlib.sha256() + try: + with path.open("rb") as handle: + # Streamed: Harbor datasets may ship large seeds or build contexts. + for chunk in iter(lambda: handle.read(_DIGEST_CHUNK_BYTES), b""): + content.update(chunk) + except OSError as exc: + logger.warning("Could not read %s while fingerprinting %s: %s", path, root, exc) + content.update(b"") + # The sub-digest is fixed width, so file bytes can never be confused with + # the framing around them. + _feed(digest, b"file", content.digest()) + + walk(root) + return digest.hexdigest() + + +def _task_dirs_for(dataset_path: Path, tasks: Sequence[AgentEvalTask]) -> dict[str, Path | None]: + """Resolve each task's on-disk directory, falling back to re-discovery. + + :func:`discover_harbor_tasks` stamps ``metadata['harbor_task_dir']``, but callers + may build :class:`AgentEvalTask` objects by hand (the Evaluator plugin builds + them from a job spec), so the metadata is not guaranteed. + + A task that cannot be resolved maps to ``None`` — the caller must treat that as + un-cacheable rather than silently omitting it from the fingerprint, which would + be a stale-cache hole. + """ + # `_safe_resolve`, not bare `resolve()`: this walk is a best-effort cache guard, so + # a symlink that vanishes mid-run must degrade to an unresolved absolute path + # rather than raise out of a job that would otherwise succeed. + dataset_root = _safe_resolve(dataset_path) + resolved: dict[str, Path | None] = {} + for task in tasks: + stamped = task.metadata.get("harbor_task_dir") + candidate = Path(stamped) if isinstance(stamped, str) and stamped else None + # The stamp records where a task was *discovered*, which is not necessarily + # where this run executes it: `dataset_path` can be overridden on the runner. + # Trusting a stale or foreign path would fingerprint one dataset while Harbor + # runs another, so anything missing or outside the active dataset is dropped + # and re-discovered below. + if candidate is not None: + candidate_resolved = _safe_resolve(candidate) + if not candidate.is_dir() or not candidate_resolved.is_relative_to(dataset_root): + logger.debug( + "Ignoring stamped harbor_task_dir %s for task %r: not a directory under the active dataset %s", + candidate, + task.id, + dataset_root, + ) + candidate = None + resolved[task.id] = candidate + if all(path is not None for path in resolved.values()): + return resolved + + try: + discovered = { + task.id: Path(str(task.metadata["harbor_task_dir"])) for task in discover_harbor_tasks(dataset_path) + } + except (OSError, ValueError) as exc: + # discover_harbor_tasks raises on ANY malformed task.toml in the dataset. + # Refusing the cache is the safe reading; failing the run is not, since this + # path previously never read those files. + logger.warning( + "Could not resolve Harbor task dirs under %s; treating the cache as stale: %s", dataset_path, exc + ) + return dict.fromkeys(resolved, None) + return {task_id: path or discovered.get(task_id) for task_id, path in resolved.items()} + + +def _stamp_coverage( + dataset_path: Path, + tasks: Sequence[AgentEvalTask], + task_names: Sequence[str] | None, +) -> Sequence[AgentEvalTask]: + """Tasks a written stamp must cover: everything Harbor was asked to run. + + ``task_names`` is the filter handed to Harbor's ``DatasetConfig``. When it is + ``None`` Harbor runs every task in the dataset, which can be a superset of the + tasks this call was asked to score — and a stamp that recorded only the smaller + set would report the larger one as stale on the next run. + """ + if task_names is not None: + return tasks + try: + discovered = discover_harbor_tasks(dataset_path) + except (OSError, ValueError): + # Same reasoning as _task_dirs_for: a malformed sibling task must not fail a + # run. Recording only the requested tasks just costs a re-run later. + return tasks + covered = {task.id: task for task in discovered} + covered.update({task.id: task for task in tasks}) + return list(covered.values()) + + +def _cache_stamp( + config: HarborRuntimeConfig, + dataset_path: Path, + tasks: Sequence[AgentEvalTask], +) -> dict[str, Any]: + """Fingerprint the inputs that decide whether a job dir can be reused. + + Covers the result-affecting options, the contents of ``agent_dir``, and the + contents of every task directory. Two gaps are deliberate and worth knowing + before trusting a hit: when ``agent_dir`` is ``None`` the agent is an already + importable module, so only its *import path* is fingerprinted and edits to that + installed package are invisible; and a task whose directory cannot be resolved + is recorded as ````, which always forces a re-run. + + Recorded per task rather than as one job-wide hash so that evaluating a + **subset** of a previously-cached job still hits: staleness is decided only over + the tasks actually requested. + + Excluded from the option hash: presentation and placement knobs (``quiet``, + ``n_concurrent_trials``, ``jobs_dir``, ``job_name``, ``force_rerun``), which + change nothing about the results; ``agent_dir``, an absolute path whose + *content* is hashed separately, so a relocated but identical agent still hits; + and ``reward_key``, which only selects which reward + :func:`build_trials_from_job_dir` reads back and must not cost a Docker re-run. + """ + options = config.model_dump(exclude=set(_CACHE_IRRELEVANT_OPTIONS), mode="json") + # `_safe_resolve` throughout, matching `_task_dirs_for`: fingerprinting is + # best-effort, so a symlink loop or a vanished link under any of these must + # degrade to an unresolved path rather than raise out of `run_tasks` and fail a + # run that would otherwise succeed. + excluded_roots = frozenset({_safe_resolve(config.jobs_dir.expanduser())}) + + agent_digest = "" + if config.agent_dir is not None: + agent_digest = _digest_directory(_safe_resolve(config.agent_dir.expanduser()), exclude=excluded_roots) + + task_digests: dict[str, str] = {} + for task_id, task_dir in sorted(_task_dirs_for(dataset_path, tasks).items()): + task_digests[task_id] = ( + "" if task_dir is None else _digest_directory(_safe_resolve(task_dir), exclude=excluded_roots) + ) + + return { + "version": CACHE_STAMP_VERSION, + "options": hashlib.sha256(json.dumps(options, sort_keys=True, default=str).encode("utf-8")).hexdigest(), + "agent": agent_digest, + "tasks": task_digests, + } + + +def _cache_is_stale(job_dir: Path, stamp: Mapping[str, Any]) -> bool: + """Return True when ``job_dir`` was not produced by the inputs in ``stamp``. + + A directory with no stamp is stale: it predates this check, or was written by + plain Harbor, and re-running is the safe reading. An ```` task + digest is likewise always stale — we could not prove the inputs match. A + directory that does not exist is stale too: there is nothing there to reuse, and + answering "not stale" would be an invitation to serve zero trials. + """ + if not job_dir.is_dir(): + return True + try: + stored = json.loads((job_dir / CACHE_STAMP_FILENAME).read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError): + stored = None + + reason: str | None = None + if not isinstance(stored, Mapping): + reason = "no usable cache stamp" + elif stored.get("version") != stamp["version"]: + reason = "cache stamp version changed" + elif stored.get("options") != stamp["options"]: + reason = "a result-affecting option changed" + elif stored.get("agent") != stamp["agent"]: + reason = "the agent directory changed" + else: + stored_tasks = stored.get("tasks") + stored_tasks = stored_tasks if isinstance(stored_tasks, Mapping) else {} + for task_id, digest in stamp["tasks"].items(): + if digest == "": + reason = f"task {task_id!r} could not be resolved on disk" + break + if stored_tasks.get(task_id) != digest: + reason = f"task {task_id!r} changed or was not part of the cached run" + break + + if reason is None: + return False + logger.info("Re-running Harbor job %s instead of serving it from cache: %s.", job_dir, reason) + return True + + +def _write_cache_stamp(job_dir: Path, stamp: Mapping[str, Any]) -> None: + """Record the inputs a completed job dir was produced from. + + Best-effort: a job dir that could not be stamped simply re-runs next time, which + is the safe direction. Written as a *file* deliberately — Harbor deletes any + stray *directory* in a job dir that lacks ``result.json``. + """ + try: + (job_dir / CACHE_STAMP_FILENAME).write_text( + json.dumps(stamp, indent=2, sort_keys=True) + "\n", encoding="utf-8" + ) + except OSError as exc: + logger.warning("Could not stamp Harbor job dir %s with its cache key: %s", job_dir, exc) + + +def _resolve_job_dir(config: HarborRuntimeConfig) -> tuple[str, Path]: + """Resolve ``(job_name, job_dir)`` without importing Harbor. + + Split out from :func:`_build_native_job` because the caller must know the job + directory *before* deciding whether to run: an unpinned ``job_name`` is a + timestamp with microsecond precision, so resolving it twice would yield two + different directories and the cache decision would be made about the wrong one. + """ + job_name = config.job_name or datetime.now(timezone.utc).strftime("%Y-%m-%d__%H-%M-%S__%f") + return job_name, config.jobs_dir / job_name + + def _build_native_job( config: HarborRuntimeConfig, dataset_path: Path, task_names: Sequence[str] | None, + *, + job_name: str | None = None, + force_rerun: bool | None = None, ) -> tuple[Path, RunJob]: """Build a Harbor ``JobConfig`` from ``config`` and return ``(job_dir, run_job)``. @@ -280,9 +720,18 @@ def _build_native_job( without importing Harbor. When ``agent_import_path`` is set, ``run_job`` scopes the user's agent package into ``sys.modules`` for the run and removes it afterwards (see :func:`scoped_harbor_agent_import`). + + Args: + job_name: Pre-resolved job name from :func:`_resolve_job_dir`. Pass it when + the caller already resolved the directory, so an unpinned name is not + re-generated into a different timestamp. + force_rerun: Overrides ``config.force_rerun`` for this build. Passed rather + than applied via ``model_copy`` so the caller's config is never mutated + and the job name stays fixed. """ - job_name = config.job_name or datetime.now(timezone.utc).strftime("%Y-%m-%d__%H-%M-%S__%f") - job_dir = config.jobs_dir / job_name + resolved_name = job_name if job_name is not None else _resolve_job_dir(config)[0] + job_dir = config.jobs_dir / resolved_name + effective_force_rerun = config.force_rerun if force_rerun is None else force_rerun async def run_job() -> None: try: @@ -295,7 +744,7 @@ async def run_job() -> None: '(it requires Python >=3.12). Install it separately: uv pip install "harbor>=0.16.1"' ) from exc - if config.force_rerun and job_dir.exists(): + if effective_force_rerun and job_dir.exists(): shutil.rmtree(job_dir) artifacts: list[str | ArtifactConfig] = list(config.artifacts) @@ -316,7 +765,7 @@ async def run_job() -> None: async def _create_and_run(agent: Any) -> None: job_config = JobConfig( - job_name=job_name, + job_name=resolved_name, jobs_dir=config.jobs_dir, n_attempts=config.n_attempts, n_concurrent_trials=config.n_concurrent_trials, @@ -327,15 +776,52 @@ async def _create_and_run(agent: Any) -> None: datasets=[DatasetConfig(path=dataset_path, task_names=list(task_names) if task_names else None)], **timeout_kwargs, ) - job = await Job.create(job_config) - await job.run() + + async def _attempt() -> None: + job = await Job.create(job_config) + await job.run() + + try: + await _attempt() + except FileExistsError as exc: + # Harbor refuses to resume a job dir whose persisted `config.json` or + # `lock.json` differs from this run's — and it refuses by raising, not + # by re-running. Its comparison is deliberately stricter than the SDK + # cache stamp: `quiet`, `n_concurrent_trials` and the `task_names` + # filter all change the JobConfig without changing the results, so the + # stamp excludes them (a full cache hit must not pay for a concurrency + # tweak) while Harbor still rejects the directory. Honour the intent of + # the rerun rather than propagating a crash. + # + # Identify the refusal positively before deleting anything. Both of + # Harbor's refusals fire before any trial executes, so discarding costs + # only completed work — but that reasoning holds *only* for those two. + # An ordinary "file exists" raised from inside a trial, a hook, or an + # environment build must not be mistaken for drift and answered by + # destroying the directory. + if not (job_dir.exists() and _is_harbor_resume_refusal(exc, job_dir)): + raise + drift = _describe_job_config_drift(job_dir, job_config) + logger.warning( + "Harbor refused to resume job dir %s, so it is being discarded and re-run from scratch: %s%s", + job_dir, + exc, + f" Differing config: {drift}." if drift else "", + ) + shutil.rmtree(job_dir) + await _attempt() if config.agent_import_path is None: await _create_and_run(AgentConfig(name=config.agent_name or "oracle", model_name=config.agent_model_name)) elif config.agent_dir is not None: - # Loose wrapper file: make its directory importable for the run. + # Loose wrapper file: make its directory importable for the run. The + # jobs_dir exclusion must match _cache_stamp's, or a jobs_dir nested under + # agent_dir would shift the package name as results accumulate. agent_dir = config.agent_dir.expanduser().resolve() - with scoped_harbor_agent_import(agent_dir, config.agent_import_path) as scoped_import: + excluded_roots = frozenset({config.jobs_dir.expanduser().resolve()}) + with scoped_harbor_agent_import( + agent_dir, config.agent_import_path, exclude=excluded_roots + ) as scoped_import: await _create_and_run(AgentConfig(import_path=scoped_import, model_name=config.agent_model_name)) else: # Already-importable module (installed package): let Harbor import it directly. @@ -344,13 +830,73 @@ async def _create_and_run(agent: Any) -> None: return job_dir, run_job +def _is_harbor_resume_refusal(exc: FileExistsError, job_dir: Path) -> bool: + """Return True when ``exc`` is Harbor declining to resume ``job_dir``. + + Separates Harbor's refusal — the one case where deleting the directory is the + right answer — from an ordinary "file exists" surfacing from a trial, a hook or an + environment build, where deleting it would destroy completed work to no purpose. + + Two signals, both required. Harbor constructs its refusals with a bare message, so + ``errno`` is unset, while an OS-level ``EEXIST`` always carries one; and both + refusals name the job directory and end in a known phrase. + """ + if exc.errno is not None: + return False + message = str(exc) + return str(job_dir) in message and any(phrase in message for phrase in _HARBOR_RESUME_REFUSALS) + + +def _describe_job_config_drift(job_dir: Path, job_config: Any) -> str: + """Name the fields that differ between ``job_dir``'s persisted JobConfig and this run's. + + Harbor reports *that* an existing config differs, never *which* field, which + leaves the resulting discard looking arbitrary. This reproduces enough of its + comparison to say — turning "Harbor refused" into "n_concurrent_trials: 10 -> 4". + + Best-effort by construction. Returns ``""`` when the difference cannot be + located: no ``config.json``, unparseable, or a refusal that came from + ``lock.json`` instead, which has no JobConfig difference to report. Diagnostics + must never mask the failure they explain, so every error here is swallowed. + """ + try: + stored_text = (job_dir / _HARBOR_JOB_CONFIG_FILENAME).read_text(encoding="utf-8") + # Harbor persists with exclude_defaults=True, so the JSON omits every field + # left at its default and comparing it raw would report phantom differences. + # Round-tripping through the model refills them, which is what Harbor itself + # compares after re-validating the stored config. + stored = type(job_config).model_validate_json(stored_text).model_dump() + current = job_config.model_dump() + return ", ".join( + f"{field}: {_truncated_repr(stored.get(field))} -> {_truncated_repr(value)}" + for field, value in current.items() + if field not in _HARBOR_EQ_IGNORED_FIELDS and stored.get(field) != value + ) + except Exception: + return "" + + +def _truncated_repr(value: Any) -> str: + """Render ``value`` for a log line, bounded so a nested config can't flood it.""" + text = repr(value) + return text if len(text) <= _DRIFT_VALUE_CHARS else f"{text[:_DRIFT_VALUE_CHARS]}..." + + @contextlib.contextmanager -def scoped_harbor_agent_import(agent_dir: Path, import_path: str) -> Iterator[str]: - """Make ``agent_dir`` importable under a unique synthetic package for the block. +def scoped_harbor_agent_import( + agent_dir: Path, import_path: str, *, exclude: frozenset[Path] = frozenset() +) -> Iterator[str]: + """Make ``agent_dir`` importable under a content-addressed package for the block. Args: agent_dir: directory containing the module referenced by ``import_path``. import_path: Harbor agent path, ``"module"`` or ``"module:attribute"``. + exclude: resolved directories to leave out of the content digest. Pass the + same set :func:`_cache_stamp` uses — in practice ``jobs_dir``, which is + caller-chosen and may sit *under* ``agent_dir``. Omitting it lets the + growing results tree feed the package name, so the import path would + change on every run and the resume this function exists to enable would + never happen. See :func:`_digest_directory`. Yields: str: the rewritten import path Harbor should load (the module rooted under @@ -359,9 +905,41 @@ def scoped_harbor_agent_import(agent_dir: Path, import_path: str) -> Iterator[st Raises: ValueError: if ``import_path`` has no module component. - On exit the injected ``sys.modules`` entries are removed. The mutation is - guarded by a process-wide lock so concurrent runs don't corrupt import state; - each run gets its own uniquely-named package so distinct agents never collide. + **The package name is derived from the directory's contents, not a random + UUID, and that is load-bearing.** This string becomes ``AgentConfig.import_path`` + and therefore part of Harbor's ``JobConfig``, which Harbor compares field-by-field + when deciding whether an existing job directory may be resumed. A random suffix + made that comparison fail on every rerun, so Harbor raised ``FileExistsError`` + instead of resuming and its per-trial resume was unreachable for any caller that + sets ``agent_dir`` (AALGO-430). Content-addressing keeps distinct agents isolated + while letting an unchanged agent resume — and makes an *edited* agent invalidate + the job dir on Harbor's own terms. + + Identical contents therefore share a package name, so overlapping scopes are + refcounted: the injected ``sys.modules`` entries are removed when the last + scope exits, not the first (see :func:`_uninstall_agent_package`). The mutation + is guarded by a process-wide lock. ``sys.modules`` is per-process, so concurrent + *processes* were never at risk here. + + The name is ``_``, so it tracks the directory's *location* as + well as its contents — deliberately, because an opaque hash makes every traceback + and import error unreadable. That is a narrow, knowing divergence from the cache + stamp, which excludes ``agent_dir`` so a relocated but identical agent still hits + (see :func:`_cache_stamp`). Relocating an agent while pinning the same + ``job_name`` therefore leaves the stamp valid but changes this string, and Harbor + declines to resume; :func:`_build_native_job` absorbs that into a clean re-run. + The results stay correct — it costs one repeated job. Callers that rebuild agents + under changing directory names (the Experimentalist does) are unaffected, because + the agent name feeds their ``job_name`` too, so a rename lands in a different job + dir with nothing to resume. + + **That last sentence only holds if the caller derives its job name from the + *resolved* directory, as this function does.** Deriving it from the caller's + spelling instead lets the two disagree: a symlink keeps its own name while + resolving elsewhere, so flipping it at a fixed ``job_name`` would reuse one job + dir for two different agents, caught only by Harbor's refusal rather than by + design. The Experimentalist resolves first for exactly this reason + (``resolve_harbor_run_inputs``). Only ``agent_dir`` (not ``sys.path``) is made importable, so a loose wrapper must be self-contained: a single module, or one that reaches siblings via @@ -373,7 +951,10 @@ def scoped_harbor_agent_import(agent_dir: Path, import_path: str) -> Iterator[st module_name = module_name.strip().lstrip(".") if not module_name: raise ValueError("import_path must be 'module' or 'module:attribute'") - package = f"{_AGENT_IMPORT_ROOT}.{_safe_identifier(agent_dir.name)}_{uuid4().hex[:8]}" + # Hashed here rather than reused from the cache stamp: this must describe the tree + # as it is about to be imported, and the extra walk is noise next to Docker. + suffix = _digest_directory(agent_dir, exclude=exclude)[:_IMPORT_DIGEST_CHARS] + package = f"{_AGENT_IMPORT_ROOT}.{_safe_identifier(agent_dir.name)}_{suffix}" with _IMPORT_LOCK: _install_agent_package(package, agent_dir) try: @@ -393,7 +974,11 @@ def _safe_identifier(value: str) -> str: def _install_agent_package(package: str, agent_dir: Path) -> None: - """Register ``package`` (and its parents) in ``sys.modules`` rooted at ``agent_dir``.""" + """Register ``package`` (and its parents) in ``sys.modules`` rooted at ``agent_dir``. + + Refcounted: package names are content-addressed, so two overlapping scopes on the + same agent directory legitimately share one. Callers must hold ``_IMPORT_LOCK``. + """ parts = package.split(".") for idx in range(1, len(parts) + 1): name = ".".join(parts[:idx]) @@ -404,11 +989,41 @@ def _install_agent_package(package: str, agent_dir: Path) -> None: sys.modules[name] = module if idx > 1: setattr(sys.modules[".".join(parts[: idx - 1])], parts[idx - 1], module) - sys.modules[package].__path__ = [str(agent_dir)] + installed = sys.modules[package] + if not installed.__path__: + installed.__path__ = [str(agent_dir)] + elif installed.__path__ != [str(agent_dir)]: + # Two directories sharing this package name share a content digest, so their + # trees are byte-identical and the path already installed is exactly as + # correct as this one — the excluded content (`.git`, `__pycache__`, the + # env dirs, `jobs_dir`) is not importable. Repointing would swap the + # directory out from under a scope that is still open, for no gain. + logger.debug( + "Agent package %s is already installed from %s; keeping it for the identical tree at %s", + package, + installed.__path__[0], + agent_dir, + ) + # Counted only once the injection it guards has succeeded. Incrementing first + # would strand the count above zero if any step above raised — the scope never + # opens, so nothing ever decrements it, and the package could never be torn down + # again for the life of the process. + _AGENT_PACKAGE_REFCOUNTS[package] = _AGENT_PACKAGE_REFCOUNTS.get(package, 0) + 1 def _uninstall_agent_package(package: str) -> None: - """Remove ``package`` and any submodules imported through it from ``sys.modules``.""" + """Remove ``package`` and its submodules from ``sys.modules`` on the last exit. + + Tearing down on the *first* exit would break a still-open scope sharing the same + content-addressed name, so the removal waits for the refcount to reach zero. + Callers must hold ``_IMPORT_LOCK``. + """ + remaining = _AGENT_PACKAGE_REFCOUNTS.get(package, 0) - 1 + if remaining > 0: + _AGENT_PACKAGE_REFCOUNTS[package] = remaining + return + _AGENT_PACKAGE_REFCOUNTS.pop(package, None) + for name in [n for n in sys.modules if n == package or n.startswith(f"{package}.")]: sys.modules.pop(name, None) parent, _, child = package.rpartition(".") @@ -731,6 +1346,8 @@ def reward_payload_from_result( __all__ = [ + "CACHE_STAMP_FILENAME", + "CACHE_STAMP_VERSION", "DEFAULT_REWARD_KEY", "HarborAgentTaskRunner", "HarborRewardMetric", diff --git a/packages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime.py b/packages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime.py index b87917b2ef..7b53c1c6c8 100644 --- a/packages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime.py +++ b/packages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime.py @@ -2,11 +2,16 @@ # SPDX-License-Identifier: Apache-2.0 import asyncio +import builtins +import hashlib import importlib import json import logging +import os import sys +from collections.abc import Awaitable, Callable from pathlib import Path +from types import ModuleType import pytest from nemo_evaluator_sdk.agent_eval.evaluator import AgentEvaluator @@ -15,6 +20,7 @@ HarborRewardMetric, HarborRuntimeConfig, HarborTasksetLoader, + _build_native_job, build_trials_from_job_dir, discover_harbor_tasks, reward_payload_from_result, @@ -23,14 +29,19 @@ from nemo_evaluator_sdk.agent_eval.tasks import AgentEvalRunConfig, AgentEvalTask from nemo_evaluator_sdk.agent_eval.trials import AgentEvalTrialStatus from nemo_evaluator_sdk.metrics.utils import metric_type_name -from pydantic import ValidationError +from pydantic import BaseModel, ValidationError _HELLO_WORLD_DATASET = Path(__file__).resolve().parents[2] / "examples" / "harbor" / "hello_world_dataset" def _write_trial( - job_dir: Path, trial_name: str, task_name: str, *, reward: float | None, exception: str | None = None + job_dir: Path, trial_name: str, task_name: str, *, reward: float | None, exception: object | None = None ) -> None: + """Write one Harbor trial dir. ``exception`` is stored verbatim as ``exception_info``. + + Typed loosely on purpose: Harbor writes a mapping there, older runs wrote a bare + string, so the adapter has to cope with both. + """ trial_dir = job_dir / trial_name (trial_dir / "agent").mkdir(parents=True) (trial_dir / "verifier").mkdir(parents=True) @@ -164,31 +175,265 @@ def test_runtime_config_defaults_and_runner_requires_a_source() -> None: asyncio.run(runner.run_tasks([AgentEvalTask(id="t", intent="x", inputs={})])) -@pytest.mark.asyncio -async def test_native_runner_uses_job_dir_as_cache(tmp_path: Path) -> None: - # A native run whose job_dir already covers every requested task is re-adapted, - # not re-run: run_job is never awaited, so Harbor is never imported here. This - # also exercises recovering the dataset dir from task metadata (no dataset_path). - jobs_dir = tmp_path / "jobs" +def _cached_task(dataset_path: Path, task_dir: Path, task_id: str = "t") -> AgentEvalTask: + """A task whose dataset and on-disk directory the cache stamp can resolve.""" + return AgentEvalTask( + id=task_id, + intent="x", + inputs={"instruction": "x"}, + metrics=[HarborRewardMetric()], + metadata={"harbor_dataset_path": str(dataset_path), "harbor_task_dir": str(task_dir)}, + ) + + +def _seed_cached_job(tmp_path: Path, *, task_id: str = "t") -> tuple[HarborRuntimeConfig, Path, AgentEvalTask]: + """A complete job dir plus the config/task that produced it, stamped as a real run would.""" + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _cache_stamp, _write_cache_stamp + + dataset_path = tmp_path / "dataset" + task_dir = dataset_path / task_id + task_dir.mkdir(parents=True) + (task_dir / "task.toml").write_text(f'[task]\nname = "{task_id}"\n') + + # jobs_dir deliberately nested under the dataset dir: the digest must exclude it, + # or it would hash its own growing results tree and never stabilize. + jobs_dir = dataset_path / "jobs" job_dir = jobs_dir / "cached-job" job_dir.mkdir(parents=True) - _write_trial(job_dir, "t__aaa", "t", reward=1.0) + _write_trial(job_dir, f"{task_id}__aaa", task_id, reward=1.0) config = HarborRuntimeConfig(jobs_dir=jobs_dir, job_name="cached-job") - runner = HarborAgentTaskRunner(config=config) - tasks = [ - AgentEvalTask( - id="t", - intent="x", - inputs={"instruction": "x"}, - metrics=[HarborRewardMetric()], - metadata={"harbor_dataset_path": str(tmp_path)}, - ) - ] + task = _cached_task(dataset_path, task_dir, task_id) + _write_cache_stamp(job_dir, _cache_stamp(config, dataset_path, [task])) + return config, job_dir, task + + +@pytest.mark.asyncio +async def test_native_runner_uses_job_dir_as_cache(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + # A native run whose job_dir covers every requested task AND carries a matching + # cache stamp is re-adapted, not re-run: run_job is never awaited, so Harbor is + # never imported here (which is why this test needs no harbor install). + config, _job_dir, task = _seed_cached_job(tmp_path) + # Watch the lazy import directly instead of mutating sys.modules: popping only + # "harbor" would leave already-imported harbor.* submodules parentless and + # corrupt the module identity other suites monkeypatch. + imported: list[str] = [] + real_import = builtins.__import__ + + def recording_import(name, *args, **kwargs): + if name == "harbor" or name.startswith("harbor."): + imported.append(name) + return real_import(name, *args, **kwargs) + + monkeypatch.setattr(builtins, "__import__", recording_import) + trials = await HarborAgentTaskRunner(config=config).run_tasks([task]) + monkeypatch.undo() - trials = await runner.run_tasks(tasks) assert [trial.task_id for trial in trials] == ["t"] assert trials[0].metadata["reward"] == 1.0 + assert imported == [], f"a cache hit must not import harbor, but imported {imported}" + + +def _stamp_for(config: HarborRuntimeConfig, task: AgentEvalTask, job_dir: Path) -> None: + """Stamp ``job_dir`` as though ``config`` had just produced it.""" + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _cache_stamp, _write_cache_stamp + + dataset_path = Path(str(task.metadata["harbor_dataset_path"])) + _write_cache_stamp(job_dir, _cache_stamp(config, dataset_path, [task])) + + +def _spy_on_run_job(monkeypatch: pytest.MonkeyPatch, calls: list[bool]) -> None: + """Replace the native job build so run_tasks is observable without Harbor. + + Records whether the run was attempted and what force_rerun it was built with. + """ + from nemo_evaluator_sdk.agent_eval.runtimes import harbor_runtime + + def fake(config, _dataset_path, _task_names, *, job_name=None, force_rerun=None): + async def run_job() -> None: + calls.append(bool(force_rerun)) + + return config.jobs_dir / (job_name or "job"), run_job + + monkeypatch.setattr(harbor_runtime, "_build_native_job", fake) + + +@pytest.mark.asyncio +async def test_unstamped_job_dir_is_not_trusted(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + # A complete job dir with no stamp predates this check, or was written by plain + # Harbor. Re-running is the safe reading. + config, job_dir, task = _seed_cached_job(tmp_path) + (job_dir / ".nemo-eval-harbor-cache.json").unlink() + calls: list[bool] = [] + _spy_on_run_job(monkeypatch, calls) + + await HarborAgentTaskRunner(config=config).run_tasks([task]) + + assert calls == [True], "an unstamped dir must be re-run, and discarded rather than resumed" + + +@pytest.mark.asyncio +async def test_changed_inputs_discard_the_job_dir(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + # A stamp mismatch means the surviving results were produced by different + # inputs, so they must be deleted rather than resumed onto. + config, _job_dir, task = _seed_cached_job(tmp_path) + agent_dir = tmp_path / "agent" + agent_dir.mkdir() + (agent_dir / "wrapper.py").write_text("x = 1\n") + config = config.model_copy(update={"agent_import_path": "wrapper:Agent", "agent_dir": agent_dir}) + calls: list[bool] = [] + _spy_on_run_job(monkeypatch, calls) + + await HarborAgentTaskRunner(config=config).run_tasks([task]) + + assert calls == [True], "changed inputs must discard, not resume" + + +@pytest.mark.asyncio +async def test_under_covered_job_resumes_with_agent_dir_set(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + # Regression for AALGO-430. This used to discard unconditionally: the scoped + # import path carried a fresh uuid per run, so Harbor's JobConfig never matched + # and it raised FileExistsError instead of resuming. Now the path is + # content-addressed, so an unchanged agent resumes and keeps completed Docker + # work — the same as the agent_dir-unset case below. + agent_dir = tmp_path / "agent" + agent_dir.mkdir() + (agent_dir / "wrapper.py").write_text("x = 1\n") + config, job_dir, task = _seed_cached_job(tmp_path) + config = config.model_copy(update={"agent_import_path": "wrapper:Agent", "agent_dir": agent_dir, "n_attempts": 2}) + _stamp_for(config, task, job_dir) # stamp matches; only coverage is short + calls: list[bool] = [] + _spy_on_run_job(monkeypatch, calls) + + await HarborAgentTaskRunner(config=config).run_tasks([task]) + + assert calls == [False], "an unchanged agent must resume, not discard completed trials" + + +@pytest.mark.asyncio +async def test_under_covered_job_resumes_when_harbor_can(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + # Inputs unchanged, some attempts missing, agent_dir unset — Harbor's + # AgentConfig is deterministic, so it resumes per trial. Discarding would throw + # away completed Docker work for nothing. The agent_dir-set case above now + # behaves identically (AALGO-430). + config, job_dir, task = _seed_cached_job(tmp_path) + config = config.model_copy(update={"n_attempts": 2}) + _stamp_for(config, task, job_dir) # stamp matches the new config + calls: list[bool] = [] + _spy_on_run_job(monkeypatch, calls) + + await HarborAgentTaskRunner(config=config).run_tasks([task]) + + assert calls == [False], "a resumable miss must not delete completed trials" + + +@pytest.mark.asyncio +@pytest.mark.parametrize("mutation", ["agent", "task", "option"]) +async def test_changed_inputs_invalidate_the_cache(tmp_path: Path, mutation: str) -> None: + # Each of these changes what a run would produce, so the stamped dir must not be + # served. Reaching run_job (and failing there) is the observable signal. + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _cache_is_stale, _cache_stamp + + config, job_dir, task = _seed_cached_job(tmp_path) + dataset_path = Path(str(task.metadata["harbor_dataset_path"])) + + if mutation == "agent": + agent_dir = tmp_path / "agent" + agent_dir.mkdir() + (agent_dir / "wrapper.py").write_text("x = 1\n") + config = config.model_copy( + update={"agent_import_path": "wrapper:Agent", "agent_dir": agent_dir}, + ) + elif mutation == "task": + (dataset_path / "t" / "task.toml").write_text('[task]\nname = "t"\nchanged = true\n') + else: + config = config.model_copy(update={"n_attempts": 2}) + + assert _cache_is_stale(job_dir, _cache_stamp(config, dataset_path, [task])) is True + + +@pytest.mark.asyncio +async def test_cosmetic_options_do_not_evict_the_cache(tmp_path: Path) -> None: + # Presentation and placement knobs change nothing about the results; evicting on + # them would cost a full Docker re-run for nothing. + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _cache_is_stale, _cache_stamp + + config, job_dir, task = _seed_cached_job(tmp_path) + dataset_path = Path(str(task.metadata["harbor_dataset_path"])) + relaxed = config.model_copy(update={"quiet": False, "n_concurrent_trials": 1, "reward_key": "other"}) + + assert _cache_is_stale(job_dir, _cache_stamp(relaxed, dataset_path, [task])) is False + + +@pytest.mark.asyncio +async def test_task_subset_of_a_cached_run_still_hits(tmp_path: Path) -> None: + # Stamping per task (not one job-wide hash) means evaluating a subset of a + # previously cached job is still a hit. + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import ( + _cache_is_stale, + _cache_stamp, + _write_cache_stamp, + ) + + config, job_dir, task_a = _seed_cached_job(tmp_path, task_id="t") + dataset_path = Path(str(task_a.metadata["harbor_dataset_path"])) + task_b_dir = dataset_path / "u" + task_b_dir.mkdir() + (task_b_dir / "task.toml").write_text('[task]\nname = "u"\n') + task_b = _cached_task(dataset_path, task_b_dir, "u") + + _write_cache_stamp(job_dir, _cache_stamp(config, dataset_path, [task_a, task_b])) + + assert _cache_is_stale(job_dir, _cache_stamp(config, dataset_path, [task_a])) is False + + +def test_unpinned_job_name_writes_no_stamp_and_reads_no_files(tmp_path: Path) -> None: + # The default timestamped job name can never hit the cache, so the fingerprint + # must not be computed at all — this is the path plugins/nemo-evaluator takes. + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _resolve_job_dir + + config = HarborRuntimeConfig(jobs_dir=tmp_path / "jobs") + first = _resolve_job_dir(config)[1] + + assert config.job_name is None + assert first.parent == tmp_path / "jobs" + assert not list((tmp_path / "jobs").glob("**/.nemo-eval-harbor-cache.json")) + + +def test_cache_stamp_survives_harbors_stray_directory_sweep(tmp_path: Path) -> None: + # Harbor rmtree's any *directory* in a job dir lacking result.json. The stamp must + # therefore be a file, or it would be silently deleted on the next Harbor run. + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import CACHE_STAMP_FILENAME + + _config, job_dir, _task = _seed_cached_job(tmp_path) + stamp = job_dir / CACHE_STAMP_FILENAME + + assert stamp.is_file() + assert not stamp.is_dir() + + +def test_cache_stamp_handles_a_missing_agent_dir(tmp_path: Path) -> None: + # agent_dir is None for every built-in-agent caller (including nemo-evaluator). + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _cache_stamp + + config, _job_dir, task = _seed_cached_job(tmp_path) + stamp = _cache_stamp(config, Path(str(task.metadata["harbor_dataset_path"])), [task]) + + assert config.agent_dir is None + assert stamp["agent"] == "" + + +def test_unresolvable_task_dir_is_always_stale(tmp_path: Path) -> None: + # A task we cannot locate on disk must never be silently omitted from the + # fingerprint — that would be a stale-cache hole. + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _cache_is_stale, _cache_stamp + + config, job_dir, _task = _seed_cached_job(tmp_path) + orphan = AgentEvalTask(id="ghost", intent="x", inputs={"instruction": "x"}, metrics=[HarborRewardMetric()]) + stamp = _cache_stamp(config, tmp_path / "nonexistent-dataset", [orphan]) + + assert stamp["tasks"]["ghost"] == "" + assert _cache_is_stale(job_dir, stamp) is True def test_multiple_attempts_map_to_one_trial_each(tmp_path: Path) -> None: @@ -247,3 +492,845 @@ def test_scoped_agent_import_makes_wrapper_importable_then_cleans_up(tmp_path: P # On exit the injected module and its synthetic package are gone from sys.modules. assert module_name not in sys.modules assert package not in sys.modules + + +def test_digest_ignores_an_exclusion_that_contains_the_whole_tree(tmp_path: Path) -> None: + # jobs_dir sitting *above* the agent/task dir is a legitimate layout. Applying the + # exclusion there would match every entry and yield an empty digest, silently + # disabling invalidation for the entire directory. + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _digest_directory + + work = tmp_path / "work" + (work / "agent").mkdir(parents=True) + (work / "agent" / "a.py").write_text("v1") + + before = _digest_directory(work / "agent", exclude=frozenset({work})) + assert before != hashlib.sha256().hexdigest(), "an ancestor exclusion must not empty the digest" + + (work / "agent" / "a.py").write_text("v2-DIFFERENT") + assert _digest_directory(work / "agent", exclude=frozenset({work})) != before + + +def test_digest_still_ignores_a_jobs_dir_nested_inside_the_tree(tmp_path: Path) -> None: + # The case the exclusion actually exists for: results written under the hashed + # tree must not make the fingerprint move on every run. + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _digest_directory + + agent = tmp_path / "agent" + (agent / "jobs").mkdir(parents=True) + (agent / "a.py").write_text("src") + (agent / "jobs" / "result.json").write_text("{}") + + before = _digest_directory(agent, exclude=frozenset({agent / "jobs"})) + (agent / "jobs" / "result.json").write_text('{"more": "output"}') + assert _digest_directory(agent, exclude=frozenset({agent / "jobs"})) == before + + +def test_task_dir_outside_the_active_dataset_is_rediscovered(tmp_path: Path) -> None: + # `dataset_path` can be overridden on the runner, so a task stamped during + # discovery under dataset A must not be fingerprinted when Harbor runs dataset B. + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _task_dirs_for + + dataset_a, dataset_b = tmp_path / "dsA", tmp_path / "dsB" + for dataset in (dataset_a, dataset_b): + (dataset / "t").mkdir(parents=True) + (dataset / "t" / "task.toml").write_text('[task]\nname = "t"\n') + + task = AgentEvalTask( + id="t", + intent="x", + inputs={"instruction": "x"}, + metadata={"harbor_dataset_path": str(dataset_a), "harbor_task_dir": str(dataset_a / "t")}, + ) + + resolved = _task_dirs_for(dataset_b, [task])["t"] + assert resolved is not None + assert resolved.resolve().is_relative_to(dataset_b.resolve()), "must fingerprint the dataset Harbor runs" + + +def test_vanished_task_dir_is_rediscovered(tmp_path: Path) -> None: + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _task_dirs_for + + dataset = tmp_path / "ds" + (dataset / "t").mkdir(parents=True) + (dataset / "t" / "task.toml").write_text('[task]\nname = "t"\n') + task = AgentEvalTask( + id="t", + intent="x", + inputs={"instruction": "x"}, + metadata={"harbor_task_dir": str(tmp_path / "gone" / "t")}, + ) + + assert _task_dirs_for(dataset, [task])["t"] == dataset / "t" + + +def test_symlink_loop_degrades_the_stamp_instead_of_killing_the_run(tmp_path: Path) -> None: + """A loop under a task dir must not take down a run over a best-effort fingerprint. + + Deliberately a *loop*, not a dangling or vanished link. On CPython 3.12 — the + floor this package targets — `Path.resolve()` translates `ELOOP` into + ``RuntimeError``, which is **not** an ``OSError``, so catching only ``OSError`` + leaves this crashing. It also raises deterministically rather than as a race, so + the failure is reproducible rather than occasional. + + What this pins is :func:`_safe_resolve`'s exception set. The loop is reached + through :func:`_digest_directory`'s walk, which already routes every path through + ``_safe_resolve``. It does **not** exercise ``_cache_stamp``'s own resolve calls: + ``_task_dirs_for`` filters candidates with ``is_dir()``, which returns ``False`` + for a loop, so a looping path never reaches them. Those calls use + ``_safe_resolve`` for consistency, not because a live crash was demonstrated. + """ + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import HarborRuntimeConfig, _cache_stamp, _safe_resolve + + dataset = tmp_path / "ds" + task_dir = dataset / "t" + task_dir.mkdir(parents=True) + (task_dir / "task.toml").write_text('[task]\nname = "t"\n') + loop = task_dir / "loop" + loop.symlink_to(loop) + + # Pin the premise: if this stops raising, the guard below is no longer load-bearing. + with pytest.raises(RuntimeError): + loop.resolve() + assert _safe_resolve(loop) == loop.absolute(), "_safe_resolve must swallow the loop, not just OSError" + + task = AgentEvalTask( + id="t", + intent="x", + inputs={"instruction": "x"}, + metadata={"harbor_task_dir": str(task_dir)}, + ) + config = HarborRuntimeConfig(jobs_dir=tmp_path / "jobs", job_name="pinned") + + stamp = _cache_stamp(config, dataset, [task]) + + assert set(stamp["tasks"]) == {"t"}, "the task must still be fingerprinted, not dropped" + + +@pytest.mark.asyncio +async def test_inputs_changing_mid_run_leaves_the_job_unstamped( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # If the candidate is edited while Harbor is running, the results came from the + # OLD sources. Stamping the new fingerprint onto them would let a later run serve + # them as if they matched — so the job dir is deliberately left unstamped. + from nemo_evaluator_sdk.agent_eval.runtimes import harbor_runtime + + config, job_dir, task = _seed_cached_job(tmp_path) + agent_dir = tmp_path / "agent" + agent_dir.mkdir() + (agent_dir / "wrapper.py").write_text("v1\n") + config = config.model_copy(update={"agent_import_path": "wrapper:Agent", "agent_dir": agent_dir}) + (job_dir / harbor_runtime.CACHE_STAMP_FILENAME).unlink() + + def fake(cfg, _dataset_path, _task_names, *, job_name=None, force_rerun=None): + async def run_job() -> None: + (agent_dir / "wrapper.py").write_text("v2-EDITED-MID-RUN\n") + + return cfg.jobs_dir / (job_name or "job"), run_job + + monkeypatch.setattr(harbor_runtime, "_build_native_job", fake) + + await HarborAgentTaskRunner(config=config).run_tasks([task]) + + assert not (job_dir / harbor_runtime.CACHE_STAMP_FILENAME).exists(), ( + "results produced from pre-edit sources must not be stamped with the post-edit fingerprint" + ) + + +def test_digest_is_injective_over_separator_bearing_contents(tmp_path: Path) -> None: + """Distinct trees must never share a digest, even when contents embed the framing. + + A collision here fails CLOSED - the digest matches, so stale results are served. + The historical bug was concatenating ``name \\0 content \\0`` with no length + framing: because file *contents* may contain NUL, ``{a: b"", b: b"Z"}`` and + ``{a: b"\\0b\\0Z"}`` produced an identical byte stream. + + Rather than pin one hand-built pair to one encoding, this asserts the property: + every tree below is structurally different, so every digest must differ. The + contents are chosen to embed the separators an unframed encoding would rely on. + """ + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _digest_directory + + trees: dict[str, dict[str, bytes]] = { + "two_files_empty_then_z": {"a": b"", "b": b"Z"}, + "one_file_absorbing_nul": {"a": b"\0b\0Z"}, + "one_file_absorbing_nul_and_mode": {"a": b"\0b\0-\0Z"}, + "three_files": {"a": b"", "b": b"", "c": b"Z"}, + "two_files_swapped": {"a": b"Z", "b": b""}, + "one_file_named_b": {"b": b"Z"}, + } + + digests: dict[str, str] = {} + for name, files in trees.items(): + root = tmp_path / name + root.mkdir() + for filename, content in files.items(): + (root / filename).write_bytes(content) + digests[name] = _digest_directory(root) + + collisions = { + (left, right) for left in digests for right in digests if left < right and digests[left] == digests[right] + } + assert not collisions, f"distinct trees produced identical digests: {sorted(collisions)}" + + +def test_digest_tracks_the_execute_bit(tmp_path: Path) -> None: + # Harbor discovers and runs tests/test.sh; flipping +x changes what happens + # without changing a single byte of content. + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _digest_directory + + task = tmp_path / "task" + task.mkdir() + script = task / "test.sh" + script.write_text("#!/bin/sh\necho hi\n") + + script.chmod(0o644) + non_executable = _digest_directory(task) + script.chmod(0o755) + assert _digest_directory(task) != non_executable, "+x must invalidate" + + +def test_digest_ignores_read_write_permission_noise(tmp_path: Path) -> None: + # Only the execute bit is tracked, mirroring git: umask differences between two + # checkouts of the same sources must not evict a usable cache. + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _digest_directory + + task = tmp_path / "task" + task.mkdir() + source = task / "a.py" + source.write_text("x = 1\n") + + source.chmod(0o644) + before = _digest_directory(task) + source.chmod(0o600) + assert _digest_directory(task) == before + + +def test_digest_covers_vendored_dependencies_but_not_the_environment(tmp_path: Path) -> None: + # node_modules ships with the agent and changes what it does, so it counts. + # .venv is environment the Harbor wrapper never uploads, so it does not. + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _digest_directory + + agent = tmp_path / "agent" + (agent / "node_modules" / "lib").mkdir(parents=True) + (agent / ".venv").mkdir() + (agent / "main.js").write_text("x") + (agent / "node_modules" / "lib" / "index.js").write_text("v1") + (agent / ".venv" / "marker").write_text("1") + + before = _digest_directory(agent) + (agent / "node_modules" / "lib" / "index.js").write_text("v2-DIFFERENT") + assert _digest_directory(agent) != before, "a vendored dependency change must invalidate" + + after_dep = _digest_directory(agent) + (agent / ".venv" / "marker").write_text("2") + assert _digest_directory(agent) == after_dep, ".venv churn must not evict the cache" + + +def test_digest_survives_a_dangling_symlink(tmp_path: Path) -> None: + # A link whose *target* is missing: is_dir()/is_file() are both False, so it is + # recorded as a marker. (readlink still succeeds here - see the test below for + # the case where readlink itself fails.) + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _digest_directory + + agent = tmp_path / "agent" + agent.mkdir() + (agent / "real.py").write_text("x") + (agent / "dangling").symlink_to(tmp_path / "does-not-exist") + + assert _digest_directory(agent) # no raise + + +def test_digest_distinguishes_a_file_from_a_directory_of_the_same_name(tmp_path: Path) -> None: + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _digest_directory + + as_file = tmp_path / "as_file" + as_file.mkdir() + (as_file / "thing").write_text("") + + as_dir = tmp_path / "as_dir" + as_dir.mkdir() + (as_dir / "thing").mkdir() + + assert _digest_directory(as_file) != _digest_directory(as_dir) + + +def test_digest_survives_readlink_failing_mid_walk(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + # The link itself disappearing between is_symlink() and readlink is a real race + # against any process cleaning up the tree. It must degrade to a marker rather + # than raise out of run_tasks and kill an otherwise-good evaluation. + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _digest_directory + + agent = tmp_path / "agent" + agent.mkdir() + (agent / "real.py").write_text("x") + (agent / "link").symlink_to(agent / "real.py") + + def exploding_readlink(*_args: object, **_kwargs: object) -> str: + raise OSError(2, "No such file or directory") + + monkeypatch.setattr(os, "readlink", exploding_readlink) + + assert _digest_directory(agent) # must not raise + + +def _scoped_path(agent_dir: Path) -> str: + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import scoped_harbor_agent_import + + with scoped_harbor_agent_import(agent_dir, "wrapper:Agent") as scoped: + return scoped + + +def test_scoped_import_path_is_stable_for_unchanged_contents(tmp_path: Path) -> None: + # The import path lands in Harbor's JobConfig, which Harbor compares field-by-field + # when deciding whether a job dir may be resumed. A per-run random suffix made that + # comparison fail every time, so Harbor could never resume (AALGO-430). + agent_dir = tmp_path / "agent" + agent_dir.mkdir() + (agent_dir / "wrapper.py").write_text("x = 1\n") + + assert _scoped_path(agent_dir) == _scoped_path(agent_dir) + + +def test_scoped_import_path_changes_when_the_agent_changes(tmp_path: Path) -> None: + # The flip side: an edited agent must NOT resume a job dir built from the old one. + # Harbor's own config check now catches that without help from the cache stamp. + agent_dir = tmp_path / "agent" + agent_dir.mkdir() + (agent_dir / "wrapper.py").write_text("x = 1\n") + before = _scoped_path(agent_dir) + + (agent_dir / "wrapper.py").write_text("x = 2\n") + + assert _scoped_path(agent_dir) != before + + +def test_distinct_agents_do_not_share_a_scoped_package(tmp_path: Path) -> None: + # Content-addressing must not collapse different agents onto one sys.modules entry. + first = tmp_path / "a" + second = tmp_path / "b" + for path, body in ((first, "x = 1\n"), (second, "x = 2\n")): + path.mkdir() + (path / "wrapper.py").write_text(body) + + assert _scoped_path(first) != _scoped_path(second) + + +def test_overlapping_scopes_on_one_agent_survive_the_inner_exit(tmp_path: Path) -> None: + # Identical contents now share a package name, so teardown is refcounted: the inner + # scope exiting must not strip sys.modules out from under the outer one. + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import scoped_harbor_agent_import + + agent_dir = tmp_path / "agent" + agent_dir.mkdir() + (agent_dir / "wrapper.py").write_text("VALUE = 7\n") + + with scoped_harbor_agent_import(agent_dir, "wrapper:Agent") as outer: + package = outer.split(":")[0].rsplit(".", 1)[0] + with scoped_harbor_agent_import(agent_dir, "wrapper:Agent"): + pass + # Inner scope closed; the outer one is still open and must still resolve. + assert package in sys.modules + assert importlib.import_module(f"{package}.wrapper").VALUE == 7 + + assert package not in sys.modules, "the last scope to exit must clean up" + + +def test_same_named_identical_agents_do_not_repoint_an_open_scope(tmp_path: Path) -> None: + # Content-addressing means two directories with the same basename and identical + # contents share a package name. The second install must not swap `__path__` out + # from under the first, still-open scope. + # + # Deliberately NOT fixed by hashing the resolved path into the package name: that + # would make the name location-dependent, so the same agent evaluated from a + # different path would produce a different JobConfig and Harbor would refuse to + # resume — reintroducing AALGO-430. The trees are byte-identical here, so keeping + # the first path is correct. + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _AGENT_PACKAGE_REFCOUNTS + + first = tmp_path / "one" / "agent" + second = tmp_path / "two" / "agent" + for agent_dir in (first, second): + agent_dir.mkdir(parents=True) + (agent_dir / "wrapper.py").write_text("VALUE = 1\n") + + with scoped_harbor_agent_import(first, "wrapper:Agent") as outer: + package = outer.rsplit(".", 1)[0] + assert sys.modules[package].__path__ == [str(first)] + with scoped_harbor_agent_import(second, "wrapper:Agent") as inner: + assert inner == outer, "identical contents and basename must share one package" + assert sys.modules[package].__path__ == [str(first)], ( + "the second install must not repoint a scope that is still open" + ) + assert sys.modules[package].__path__ == [str(first)], "the inner exit must not tear down the outer scope" + + assert package not in sys.modules + assert _AGENT_PACKAGE_REFCOUNTS == {} + + +def test_scoped_import_teardown_is_complete_after_overlap(tmp_path: Path) -> None: + # Refcounting must not leak: no stray refcount entries or sys.modules residue. + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import ( + _AGENT_IMPORT_ROOT, + _AGENT_PACKAGE_REFCOUNTS, + scoped_harbor_agent_import, + ) + + agent_dir = tmp_path / "agent" + agent_dir.mkdir() + (agent_dir / "wrapper.py").write_text("x = 1\n") + + with scoped_harbor_agent_import(agent_dir, "wrapper:Agent"): + with scoped_harbor_agent_import(agent_dir, "wrapper:Agent"): + pass + + assert _AGENT_PACKAGE_REFCOUNTS == {} + assert not [name for name in sys.modules if name.startswith(f"{_AGENT_IMPORT_ROOT}.")] + + +def test_scoped_import_path_ignores_a_jobs_dir_nested_under_the_agent(tmp_path: Path) -> None: + # jobs_dir is caller-chosen and may sit *under* agent_dir. Without the same + # exclusion the cache stamp applies, Harbor's own results would feed the package + # name, so the import path would move every run and the resume this whole change + # exists to enable could never happen. + agent_dir = tmp_path / "agent" + jobs_dir = agent_dir / "results" + jobs_dir.mkdir(parents=True) + (agent_dir / "wrapper.py").write_text("x = 1\n") + excluded = frozenset({jobs_dir.resolve()}) + + with scoped_harbor_agent_import(agent_dir, "wrapper:Agent", exclude=excluded) as before: + pass + (jobs_dir / "trial-a").mkdir() + (jobs_dir / "trial-a" / "result.json").write_text("{}") + with scoped_harbor_agent_import(agent_dir, "wrapper:Agent", exclude=excluded) as after: + pass + + assert before == after, "accumulating results must not move the agent's import path" + + +def test_failed_scoped_import_install_does_not_wedge_the_refcount( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # The refcount is taken only once the sys.modules injection has succeeded. Taking + # it first would strand the count at 1 when the injection raises — no scope ever + # opened, so nothing decrements it, and the package could never be torn down again. + from nemo_evaluator_sdk.agent_eval.runtimes import harbor_runtime + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _AGENT_IMPORT_ROOT, _AGENT_PACKAGE_REFCOUNTS + + agent_dir = tmp_path / "agent" + agent_dir.mkdir() + (agent_dir / "wrapper.py").write_text("x = 1\n") + + def explode(_name: str) -> ModuleType: + raise RuntimeError("synthetic package could not be built") + + with monkeypatch.context() as patched: + patched.setattr(harbor_runtime, "ModuleType", explode) + with pytest.raises(RuntimeError, match="synthetic package"): + with scoped_harbor_agent_import(agent_dir, "wrapper:Agent"): + pass + + assert _AGENT_PACKAGE_REFCOUNTS == {}, "a failed install must not leave a refcount behind" + # And a later scope must still install and then fully tear down. + with scoped_harbor_agent_import(agent_dir, "wrapper:Agent"): + pass + assert _AGENT_PACKAGE_REFCOUNTS == {} + assert not [name for name in sys.modules if name.startswith(f"{_AGENT_IMPORT_ROOT}.")] + + +class _DriftConfig(BaseModel): + """Stands in for Harbor's JobConfig: a field it ignores, one it compares, one defaulted.""" + + job_name: str = "job" + n_concurrent_trials: int = 4 + quiet: bool = True + + +def _stub_harbor(monkeypatch: pytest.MonkeyPatch, job_create: Callable[[object], Awaitable[object]]) -> None: + """Install a minimal fake ``harbor`` package so ``run_job`` can execute. + + Only the names ``_build_native_job``'s ``run_job`` imports are provided. + ``job_create`` becomes ``Job.create``; every config class is a permissive stub, + since what is under test is the control flow around Harbor, not the payload. + """ + + def _module(name: str, **attrs: object) -> None: + module = ModuleType(name) + for key, value in attrs.items(): + setattr(module, key, value) + monkeypatch.setitem(sys.modules, name, module) + + def _anything(*_args: object, **_kwargs: object) -> object: + return object() + + class _Job: + create = staticmethod(job_create) + + _module("harbor") + # JobConfig is a real model, not `_anything`: the resume-refusal path reads and + # re-validates the persisted one to report what differed. Pydantic ignores the + # kwargs _build_native_job passes that this stand-in doesn't declare. + _module("harbor.job", DatasetConfig=_anything, Job=_Job, JobConfig=_DriftConfig) + _module("harbor.models") + _module("harbor.models.job") + _module("harbor.models.job.config", RetryConfig=_anything) + _module("harbor.models.trial") + _module("harbor.models.trial.config", AgentConfig=_anything, ArtifactConfig=_anything) + + +class _FakeJob: + async def run(self) -> None: + return None + + +@pytest.mark.asyncio +async def test_harbor_refusing_to_resume_discards_and_reruns( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + # Harbor compares its whole persisted JobConfig (and lock.json) before resuming and + # raises FileExistsError on any mismatch. The SDK cache stamp is looser on purpose: + # `quiet`, `n_concurrent_trials` and the `task_names` filter change the JobConfig + # without changing the results, so a job dir that passes the stamp — and is + # therefore handed over with force_rerun=False — can still be rejected by Harbor. + # That must degrade to a clean re-run rather than crash the evaluation. + jobs_dir = tmp_path / "jobs" + job_dir = jobs_dir / "pinned" + (job_dir / "old-trial").mkdir(parents=True) + # The dir was produced on a 10-core box; this run defaults to 4. Nothing about the + # results changed, so the SDK stamp would still call it fresh — Harbor won't. + (job_dir / "config.json").write_text( + _DriftConfig(job_name="pinned", n_concurrent_trials=10).model_dump_json(exclude_defaults=True), + encoding="utf-8", + ) + dir_existed_at_attempt: list[bool] = [] + + async def create(_config: object) -> _FakeJob: + dir_existed_at_attempt.append(job_dir.exists()) + if len(dir_existed_at_attempt) == 1: + raise FileExistsError( + f"Job directory {job_dir} already exists and cannot be resumed with a different config." + ) + return _FakeJob() + + _stub_harbor(monkeypatch, create) + config = HarborRuntimeConfig(jobs_dir=jobs_dir, job_name="pinned") + _built, run_job = _build_native_job(config, tmp_path / "dataset", None, job_name="pinned", force_rerun=False) + + with caplog.at_level(logging.WARNING): + await run_job() + + assert dir_existed_at_attempt == [True, False], "the refused dir must be discarded before the retry" + assert not (job_dir / "old-trial").exists(), "the stale trial must be gone, not resumed onto" + assert "refused to resume" in caplog.text, "silently deleting completed trials must be visible" + assert "n_concurrent_trials: 10 -> 4" in caplog.text, "the warning must name what forced the discard" + + +@pytest.mark.asyncio +async def test_file_exists_error_without_a_job_dir_propagates(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + # The retry is scoped to Harbor's resume refusal. A FileExistsError raised with no + # job dir to discard is something else entirely and must not be swallowed, nor + # turned into a second Docker run. + attempts: list[int] = [] + + async def create(_config: object) -> _FakeJob: + attempts.append(1) + raise FileExistsError("something unrelated") + + _stub_harbor(monkeypatch, create) + config = HarborRuntimeConfig(jobs_dir=tmp_path / "jobs", job_name="pinned") + _built, run_job = _build_native_job(config, tmp_path / "dataset", None, job_name="pinned", force_rerun=False) + + with pytest.raises(FileExistsError, match="something unrelated"): + await run_job() + + assert attempts == [1], "an unrelated FileExistsError must not be retried" + + +@pytest.mark.asyncio +async def test_unrelated_file_exists_error_mid_run_leaves_the_job_dir_alone( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # The dangerous shape: a job dir that *does* exist, and a FileExistsError raised + # from inside the run rather than by Harbor's resume check — a trial, a hook, an + # environment build. Treating that as drift would delete completed work and re-run + # for an error that has nothing to do with the config. + jobs_dir = tmp_path / "jobs" + job_dir = jobs_dir / "pinned" + (job_dir / "finished-trial").mkdir(parents=True) + attempts: list[int] = [] + + class _ExplodingJob: + async def run(self) -> None: + attempts.append(1) + raise FileExistsError(17, "File exists", str(tmp_path / "scratch" / "artifact.tar")) + + async def create(_config: object) -> _ExplodingJob: + return _ExplodingJob() + + _stub_harbor(monkeypatch, create) + config = HarborRuntimeConfig(jobs_dir=jobs_dir, job_name="pinned") + _built, run_job = _build_native_job(config, tmp_path / "dataset", None, job_name="pinned", force_rerun=False) + + with pytest.raises(FileExistsError): + await run_job() + + assert attempts == [1], "an unrelated failure must not be retried" + assert (job_dir / "finished-trial").exists(), "completed work must survive an error that is not resume drift" + + +@pytest.mark.parametrize( + ("message", "errno", "expected"), + [ + ("Job directory {job_dir} already exists and cannot be resumed with a different config.", None, True), + ("Job directory {job_dir} already has a lock.json that does not match the resolved job lock.", None, True), + # Same words, but an OS-level EEXIST: errno is set, so it is not Harbor's refusal. + ("Job directory {job_dir} already exists and cannot be resumed with a different config.", 17, False), + # A refusal naming a *different* job dir is not ours to act on. + ("Job directory /somewhere/else already exists and cannot be resumed with a different config.", None, False), + ("[Errno 17] File exists: '{job_dir}/trial/artifact.tar'", None, False), + ], +) +def test_only_harbors_resume_refusal_authorises_deleting_the_job_dir( + tmp_path: Path, message: str, errno: int | None, expected: bool +) -> None: + # Deleting a job dir is the one irreversible thing this runtime does, so the + # predicate that authorises it is pinned directly. Anything unrecognised must + # answer False and let the error propagate — the safe direction if Harbor rewords. + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _is_harbor_resume_refusal + + job_dir = tmp_path / "jobs" / "pinned" + rendered = message.format(job_dir=job_dir) + exc = FileExistsError(rendered) if errno is None else FileExistsError(errno, "File exists", rendered) + + assert _is_harbor_resume_refusal(exc, job_dir) is expected + + +def test_job_config_drift_names_the_field_that_forced_the_discard(tmp_path: Path) -> None: + # Harbor says only *that* a config differs, so the discard looks arbitrary in the + # log. This pins three things at once: the differing field is named, a field Harbor + # ignores is not, and a field left at its default is not — the last only holds + # because the persisted JSON (written with exclude_defaults=True) is re-validated + # rather than compared raw, which would see a missing key as a difference. + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _describe_job_config_drift + + job_dir = tmp_path / "job" + job_dir.mkdir() + stored = _DriftConfig(job_name="pinned", n_concurrent_trials=10) + (job_dir / "config.json").write_text(stored.model_dump_json(exclude_defaults=True), encoding="utf-8") + + drift = _describe_job_config_drift(job_dir, _DriftConfig(job_name="renamed", n_concurrent_trials=4)) + + assert drift == "n_concurrent_trials: 10 -> 4" + + +def test_job_config_drift_is_silent_when_it_cannot_tell(tmp_path: Path) -> None: + # No config.json is the lock.json-refusal case: there is no JobConfig difference to + # report. Diagnostics must degrade to silence, never to a raised exception that + # would mask the FileExistsError being explained. + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _describe_job_config_drift + + job_dir = tmp_path / "job" + job_dir.mkdir() + + assert _describe_job_config_drift(job_dir, _DriftConfig()) == "" + (job_dir / "config.json").write_text("{not json", encoding="utf-8") + assert _describe_job_config_drift(job_dir, _DriftConfig()) == "" + + +# Harbor JobConfig field -> the HarborRuntimeConfig fields that feed it. Each of these +# must sit inside the cache fingerprint: if one drops out, the stamp could call a job +# dir reusable that Harbor will then reject. `agents` also carries the agent contents, +# which the stamp digests separately (that is why `agent_dir` itself is irrelevant). +_STAMP_COVERED_HARBOR_FIELDS = { + "n_attempts": {"n_attempts"}, + "artifacts": {"artifacts", "trace_dir"}, + "retry": {"max_retries"}, + "agents": {"agent_name", "agent_import_path", "agent_model_name"}, + "timeout_multiplier": {"timeout_multiplier"}, + "agent_timeout_multiplier": {"agent_timeout_multiplier"}, + "verifier_timeout_multiplier": {"verifier_timeout_multiplier"}, + "agent_setup_timeout_multiplier": {"agent_setup_timeout_multiplier"}, + "environment_build_timeout_multiplier": {"environment_build_timeout_multiplier"}, +} +# Left at Harbor's defaults by _build_native_job, so two SDK-built configs can never +# disagree on them. (A dir written by the Harbor CLI could, but it carries no SDK cache +# stamp, so it is stale and gets discarded before Harbor ever sees it.) +_SDK_NEVER_SETS = {"install_only", "environment", "verifier", "metrics", "tasks", "extra_instruction_paths"} +# Compared by Harbor, deliberately *not* keyed by the SDK stamp. Harbor asks "can I +# resume this directory?"; the stamp asks "did these inputs produce these results?". +# Where the answers diverge, _build_native_job absorbs Harbor's refusal. +_KNOWINGLY_LOOSER = { + "jobs_dir": "implied by having found the job dir at all", + "n_concurrent_trials": "scheduling only; keying it would discard a cached run on a box with a different core count", + "quiet": "display only; changes nothing about the results", + "datasets": "`path` is covered by the per-task digests; the `task_names` filter is left unkeyed so a subset of a " + "cached job still hits (see _stamp_coverage)", +} + + +def test_harbor_still_words_its_resume_refusals_the_way_we_match_them() -> None: + # The predicate that authorises deleting a job dir keys off Harbor's message text. + # If Harbor rewords, the predicate stops matching and the refusal propagates as a + # crash — the safe direction, but a silent loss of the graceful re-run. Catch that + # at upgrade time here instead of in someone's failed experiment. + pytest.importorskip("harbor.job", reason="harbor needs python >= 3.12") + import inspect + + from harbor.job import Job + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import _HARBOR_RESUME_REFUSALS + + source = inspect.getsource(Job) + for phrase in _HARBOR_RESUME_REFUSALS: + assert phrase in source, ( + f"Harbor no longer raises its resume refusal with {phrase!r}. Re-read Job._maybe_init_existing_job " + "and Job._write_job_lock, then update _HARBOR_RESUME_REFUSALS. Keep each phrase inside a single " + "source string literal — one spanning an implicit concatenation will not be found here." + ) + + +def test_harbor_job_config_equality_still_behaves_as_the_retry_assumes() -> None: + # The FileExistsError retry exists because Harbor compares its whole JobConfig and + # ignores only identity/logging fields. Pin that behaviourally, so a Harbor upgrade + # that changes the rule surfaces here rather than as a mystery re-run in production. + job_config = pytest.importorskip("harbor.models.job.config", reason="harbor needs python >= 3.12") + + baseline = job_config.JobConfig(job_name="a") + assert baseline == job_config.JobConfig(job_name="b"), "job_name must stay outside Harbor's comparison" + assert baseline != job_config.JobConfig(job_name="a", n_concurrent_trials=99), ( + "n_concurrent_trials must stay inside it — that is the case the retry absorbs" + ) + + +def test_every_harbor_job_config_field_is_classified_against_the_sdk_stamp() -> None: + # Drift guard. The SDK's fingerprint is deliberately looser than Harbor's + # comparison, but only in ways we have reasoned about. A Harbor upgrade that adds a + # compared field would silently widen that gap into unexplained full re-runs, so + # every field must land in exactly one bucket before it can ship. + job_config = pytest.importorskip("harbor.models.job.config", reason="harbor needs python >= 3.12") + from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import ( + _CACHE_IRRELEVANT_OPTIONS, + _HARBOR_EQ_IGNORED_FIELDS, + ) + + # Derived, not hardcoded: dropping a field into _CACHE_IRRELEVANT_OPTIONS re-checks + # here instead of quietly diverging from a copied list. + fingerprinted = set(HarborRuntimeConfig.model_fields) - set(_CACHE_IRRELEVANT_OPTIONS) + for harbor_field, sdk_fields in _STAMP_COVERED_HARBOR_FIELDS.items(): + missing = sdk_fields - fingerprinted + assert not missing, ( + f"Harbor compares {harbor_field!r}, but {sorted(missing)} left the cache fingerprint. " + "Either restore it, or move the field to _KNOWINGLY_LOOSER with a reason." + ) + + classified = ( + set(_HARBOR_EQ_IGNORED_FIELDS) | _SDK_NEVER_SETS | set(_STAMP_COVERED_HARBOR_FIELDS) | set(_KNOWINGLY_LOOSER) + ) + actual = set(job_config.JobConfig.model_fields) + assert not actual - classified, ( + f"Harbor's JobConfig grew {sorted(actual - classified)}. Classify each one: covered by the cache stamp " + "(_STAMP_COVERED_HARBOR_FIELDS), never set by the SDK (_SDK_NEVER_SETS), or knowingly unkeyed " + "(_KNOWINGLY_LOOSER, with a reason)." + ) + assert not classified - actual, ( + f"{sorted(classified - actual)} no longer exist on Harbor's JobConfig; drop them from the classification." + ) + + +def test_trial_dir_without_result_json_is_skipped_and_its_task_reported_missing( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A trial that died before the verifier leaves a dir but no result.json. + + Harbor still creates the trial dir (and often an exception.txt), so the adapter + has to tolerate the absence rather than raise, while the task it belonged to + must not silently vanish from the run — it is reported as having no result. + """ + job_dir = tmp_path / "job" + job_dir.mkdir() + _write_trial(job_dir, "ok-task__aaa", "ok-task", reward=1.0) + crashed = job_dir / "crashed-task__bbb" + (crashed / "agent").mkdir(parents=True) + (crashed / "exception.txt").write_text("Traceback (most recent call last): ...") + + tasks = [ + AgentEvalTask(id="ok-task", intent="x", inputs={"instruction": "p"}, metrics=[HarborRewardMetric()]), + AgentEvalTask(id="crashed-task", intent="y", inputs={"instruction": "q"}, metrics=[HarborRewardMetric()]), + ] + with caplog.at_level(logging.WARNING): + trials = build_trials_from_job_dir(job_dir, tasks) + + assert [trial.task_id for trial in trials] == ["ok-task"] + assert "crashed-task" in caplog.text + + +def test_unreadable_result_json_is_skipped_without_raising(tmp_path: Path, caplog: pytest.LogCaptureFixture) -> None: + """A truncated result.json must not take the whole job's adaptation down.""" + job_dir = tmp_path / "job" + job_dir.mkdir() + _write_trial(job_dir, "ok-task__aaa", "ok-task", reward=1.0) + broken = job_dir / "broken-task__bbb" + broken.mkdir() + (broken / "result.json").write_text("{not json") + + tasks = [ + AgentEvalTask(id="ok-task", intent="x", inputs={"instruction": "p"}, metrics=[HarborRewardMetric()]), + AgentEvalTask(id="broken-task", intent="y", inputs={"instruction": "q"}, metrics=[HarborRewardMetric()]), + ] + with caplog.at_level(logging.WARNING): + trials = build_trials_from_job_dir(job_dir, tasks) + + assert [trial.task_id for trial in trials] == ["ok-task"] + assert "broken-task" in caplog.text + + +@pytest.mark.parametrize( + ("exception_info", "expected_type"), + [ + ({"exception_type": "TimeoutError"}, "TimeoutError"), + ({"type": "TimeoutError"}, "TimeoutError"), + ({"name": "TimeoutError"}, "TimeoutError"), + ({"class": "TimeoutError"}, "TimeoutError"), + # A mapping Harbor filled with something unexpected still counts as failed. + ({"message": "boom"}, "UnknownException"), + ({}, "UnknownException"), + # Older/other writers put a bare value there. + ("NonZeroAgentExitCodeError", "NonZeroAgentExitCodeError"), + (17, "17"), + ], +) +def test_exception_info_shapes_all_resolve_to_a_type( + tmp_path: Path, exception_info: object, expected_type: str +) -> None: + """Any non-null exception_info must mark the trial failed, whatever its shape. + + Only a bare string was covered before, so a mapping — which is what Harbor + actually writes — went untested. Resolving to None here would silently promote + a crashed trial to COMPLETED and let it score. + """ + job_dir = tmp_path / "job" + job_dir.mkdir() + _write_trial(job_dir, "t__aaa", "t", reward=1.0, exception=exception_info) + + trials = build_trials_from_job_dir( + job_dir, [AgentEvalTask(id="t", intent="x", inputs={"instruction": "p"}, metrics=[HarborRewardMetric()])] + ) + + assert len(trials) == 1 + assert trials[0].metadata["exception_type"] == expected_type + assert trials[0].status is AgentEvalTrialStatus.PARTIAL + + +def test_absent_exception_info_leaves_the_trial_completed(tmp_path: Path) -> None: + """The negative case that gives the parametrization above its meaning.""" + job_dir = tmp_path / "job" + job_dir.mkdir() + _write_trial(job_dir, "t__aaa", "t", reward=1.0, exception=None) + + trials = build_trials_from_job_dir( + job_dir, [AgentEvalTask(id="t", intent="x", inputs={"instruction": "p"}, metrics=[HarborRewardMetric()])] + ) + + assert "exception_type" not in trials[0].metadata + assert trials[0].status is AgentEvalTrialStatus.COMPLETED diff --git a/packages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime_e2e.py b/packages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime_e2e.py index 5d6266ab6b..92425b1256 100644 --- a/packages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime_e2e.py +++ b/packages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime_e2e.py @@ -28,6 +28,27 @@ _DATASET_DIR = Path(__file__).resolve().parents[2] / "examples" / "harbor" / "hello_world_dataset" _TASK_NAME = "harbor/hello-world" +# Minimal BaseAgent for the resume probe. Satisfies the bundled hello-world verifier, +# which passes iff /app/hello.txt contains exactly "Hello, world!". +_RESUME_PROBE_AGENT = """\ +from harbor import BaseAgent + + +class WrappedAgent(BaseAgent): + @staticmethod + def name() -> str: + return "resume-probe" + + def version(self) -> str | None: + return "1.0.0" + + async def setup(self, environment) -> None: + return None + + async def run(self, instruction, environment, context) -> None: + await environment.exec("printf 'Hello, world!' > /app/hello.txt") +""" + def _docker_available() -> bool: if shutil.which("docker") is None: @@ -66,3 +87,68 @@ async def test_sdk_runs_harbor_hello_world_natively(tmp_path: Path) -> None: payload = reward_payload_from_result(result) assert payload["reward"]["harbor_reward.reward"] == 1.0 assert payload["exceptions"] == {} + + +@pytest.mark.asyncio +async def test_harbor_resumes_a_partial_job_with_a_custom_agent_dir(tmp_path: Path) -> None: + """Regression for AALGO-430 — a real Harbor resume with ``agent_dir`` set. + + This is the case every faked-``Job`` test misses, and the reason the bug went + unnoticed: the scoped agent import path used to carry a fresh uuid per run, so + Harbor's ``JobConfig`` comparison failed on the second call and it raised + ``FileExistsError`` rather than resuming. Now the path is content-addressed, so + an unchanged agent resumes and only the missing trial is re-run. + + Runs two attempts and drops one, so "resumed" is distinguishable from "discarded + and re-run from scratch" — with a single attempt the two are observationally + identical and the test would pass either way. + """ + pytest.importorskip("harbor") + if not _docker_available(): + pytest.skip("Docker daemon is required to run a Harbor job") + + # A loose wrapper file next to the dataset — the shape `agent_dir` exists for, + # and the shape the Experimentalist always uses. + agent_dir = tmp_path / "agent" + agent_dir.mkdir() + (agent_dir / "harbor_wrapper.py").write_text(_RESUME_PROBE_AGENT, encoding="utf-8") + + jobs_dir = tmp_path / "jobs" + config = HarborRuntimeConfig( + jobs_dir=jobs_dir, + job_name="resume-job", # pinned: the cache and Harbor's resume both need it + agent_import_path="harbor_wrapper:WrappedAgent", + agent_dir=agent_dir, + # Two attempts so one can be dropped and one kept. With a single attempt, + # discarding the whole job dir and re-running is observationally identical to + # resuming, and the assertions below could not tell them apart. + n_attempts=2, + ) + + first = await run_harbor_eval(config, _DATASET_DIR) + assert [trial.status for trial in first.trials] == [AgentEvalTrialStatus.COMPLETED] * 2 + + job_dir = jobs_dir / "resume-job" + trial_dirs = sorted(path.parent for path in job_dir.glob("*/result.json")) + assert len(trial_dirs) == 2, "the first run must have written both attempts" + survivor, dropped = trial_dirs + survivor_result = (survivor / "result.json").read_text(encoding="utf-8") + + # Drop one attempt's result so the job is under-covered, leaving the job dir (and + # its config.json) in place — the exact state that used to raise FileExistsError. + (dropped / "result.json").unlink() + + second = await run_harbor_eval(config, _DATASET_DIR) + + # The point of the test: the completed attempt was *resumed*, not re-run. Harbor + # suffixes each trial dir with a shortuuid, so a discarded job dir would come back + # under a different name, and a re-executed trial would rewrite result.json. + assert survivor.is_dir(), "the completed attempt's trial dir must survive the rerun" + assert (survivor / "result.json").read_text(encoding="utf-8") == survivor_result, ( + "the completed attempt must be reused untouched, not re-executed" + ) + assert not dropped.is_dir(), "the result-less attempt must be cleared and re-run" + + assert [trial.status for trial in second.trials] == [AgentEvalTrialStatus.COMPLETED] * 2 + assert {trial.task_id for trial in second.trials} == {_TASK_NAME} + assert [trial.metadata["reward"] for trial in second.trials] == [1.0, 1.0] diff --git a/plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/config.yaml b/plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/config.yaml index 99fa4b0d93..08cc95dcee 100644 --- a/plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/config.yaml +++ b/plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/config.yaml @@ -10,7 +10,7 @@ workspace: default base_url: http://localhost:8080 mode: local -evaluator_type: harbor +evaluator_type: harbor_native # Required per run. insight: "" diff --git a/plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/run.py b/plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/run.py index b0ea41dddf..c61b31dc91 100644 --- a/plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/run.py +++ b/plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/run.py @@ -49,7 +49,7 @@ async def run_eval_author( base_url: str | None, config: EvalAuthorConfig, agent: Path | str | None = None, - evaluator_type: EvaluatorType = "harbor", + evaluator_type: EvaluatorType = "harbor_native", mode: Literal["local", "remote"] = "local", ) -> EvalAuthorResult: """Build and run the Eval Author against an Insight and evaluator datasets. diff --git a/plugins/nemo-eval-author/tests/test_eval_author_repair_e2e.py b/plugins/nemo-eval-author/tests/test_eval_author_repair_e2e.py index 3dfbf4cfb0..1c97ecd19e 100644 --- a/plugins/nemo-eval-author/tests/test_eval_author_repair_e2e.py +++ b/plugins/nemo-eval-author/tests/test_eval_author_repair_e2e.py @@ -18,6 +18,8 @@ ) from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor import ( HarborDataset, +) +from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_native import ( HarborEvaluator, HarborEvaluatorConfig, ) diff --git a/plugins/nemo-eval-author/tests/test_eval_author_run.py b/plugins/nemo-eval-author/tests/test_eval_author_run.py index 6b31bf9be5..007312c85f 100644 --- a/plugins/nemo-eval-author/tests/test_eval_author_run.py +++ b/plugins/nemo-eval-author/tests/test_eval_author_run.py @@ -8,6 +8,7 @@ import pytest from nemo_eval_author_plugin.eval_author import run as eval_author_run from nemo_eval_author_plugin.eval_author.models import EvalAuthorConfig, EvalAuthorResult +from nemo_experimentalist_plugin.experimentalist.components.evaluator.base import EvaluatorType from nemo_experimentalist_plugin.experimentalist.components.evaluator.models import Dataset, DatasetRef, Task from nemo_insights_plugin.entities import Insight @@ -113,10 +114,12 @@ async def run( ) +@pytest.mark.parametrize("evaluator_type", ["harbor_native", "harbor_evaluator"]) @pytest.mark.asyncio async def test_run_eval_author_builds_and_runs_complete_contract( monkeypatch: pytest.MonkeyPatch, tmp_path: Path, + evaluator_type: EvaluatorType, ) -> None: client = ClosingClient() insight = Insight( @@ -168,6 +171,7 @@ def build_eval_author_agent(*, experiment_dir: Path, config: EvalAuthorConfig) - workspace="workspace-a", base_url="http://platform.test", config=config, + evaluator_type=evaluator_type, mode="local", ) @@ -187,10 +191,13 @@ def build_eval_author_agent(*, experiment_dir: Path, config: EvalAuthorConfig) - dest=experiment_dir / "eval_author" / "source-agent", ) ] - assert dataset_factory.dataset_refs == [("harbor", train_ref), ("harbor", validation_ref)] + assert dataset_factory.dataset_refs == [ + (evaluator_type, train_ref), + (evaluator_type, validation_ref), + ] assert dataset_factory.template_refs == [ ( - "harbor", + evaluator_type, template_ref.model_copy(update={"uri": str(experiment_dir / "dataset" / "task-template")}), ) ] @@ -253,7 +260,9 @@ async def download(self, *, remote_path: str, local_path: str, workspace: str) - "workspace": "workspace-a", } ] - assert dataset_factory.template_refs == [("harbor", template_ref.model_copy(update={"uri": str(staged_path)}))] + assert dataset_factory.template_refs == [ + ("harbor_native", template_ref.model_copy(update={"uri": str(staged_path)})) + ] assert client.closed diff --git a/plugins/nemo-experimentalist/README.md b/plugins/nemo-experimentalist/README.md index 4ccd8c7f72..8d2855417a 100644 --- a/plugins/nemo-experimentalist/README.md +++ b/plugins/nemo-experimentalist/README.md @@ -10,27 +10,22 @@ or Git-backed agent against Harbor-compatible train and validation datasets. ## Install and develop -From the root of this checkout: +This plugin is a workspace member of the NeMo Platform monorepo. Its agent +framework (NOOA) and evaluator (Harbor) are both Python 3.12-only, so the whole +plugin sits behind an optional dependency group. From the **platform root**: ```bash -uv sync +uv sync --group experimentalist export NEMO="$PWD/.venv/bin/nemo" ``` -For a NeMo Platform source checkout, use the -[source-Platform installer](docs/e2e/install-experimentalist-plugin.sh), which keeps -Platform packages editable while installing both plugins' direct runtime -dependencies: - -```bash -REPO="$PWD" PLAT=/path/to/nemo-platform bash docs/e2e/install-experimentalist-plugin.sh -export NEMO=/path/to/nemo-platform/.venv/bin/nemo -``` - The source dependencies are pinned to tagged or immutable revisions in `pyproject.toml`. NVIDIA-labs OO Agents (NOOA) is pinned to a public GitHub commit, currently one past `v0.0.6` that carries an MCP transport-timeout fix. +Verify with `$NEMO experimentalist doctor`. Harbor evaluation also needs a +running Docker daemon — `doctor` treats both as required checks. + ## Insight-to-experiment flow The supported handoff is: @@ -117,9 +112,25 @@ $NEMO experimentalist run \ ``` Pass one or more framework skill directories with `--framework-skills` when -the agent needs framework-specific modification guidance. The checked-in Tau2 -profile demonstrates profile-owned datasets and task template configuration: -[`examples/tau2-nemo-oo-agent/optimizer.yaml`](examples/tau2-nemo-oo-agent/optimizer.yaml). +the agent needs framework-specific modification guidance. The checked-in +[`tau3-nooa-agent`](examples/tau3-nooa-agent/README.md) demonstrates the +realistic NOOA, MCP, and inference-backed path. Follow the +[getting-started guide](../../docs/get-started/example-agent.mdx) to prepare its +train and validation datasets before running the SDK-backed smoke config. + +For a first run, prefer the fully local example — no dataset registry, no +Platform, and a validation evaluation that finishes in seconds. From the +platform root: + +```bash +$NEMO experimentalist run \ + --profile plugins/nemo-experimentalist/examples/hello-harbor-agent/optimizer.yaml \ + --no-insight \ + --experiment-dir tmp/exp-hello +``` + +See [`examples/hello-harbor-agent/README.md`](examples/hello-harbor-agent/README.md) +for what it contains and how to run it. Each run writes its local artifacts under `--experiment-dir`, or under `.nemo-optimizer/experiments/` beside the governing profile by default. diff --git a/plugins/nemo-experimentalist/benchmarks/run.py b/plugins/nemo-experimentalist/benchmarks/run.py index 2fe7d2e0a1..43c4e21f2f 100644 --- a/plugins/nemo-experimentalist/benchmarks/run.py +++ b/plugins/nemo-experimentalist/benchmarks/run.py @@ -15,8 +15,8 @@ import yaml from harbor.registry.client.package import PackageDatasetClient -from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor import ( - HarborDataset, +from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor import HarborDataset +from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_native import ( HarborEvaluator, HarborEvaluatorConfig, ) diff --git a/plugins/nemo-experimentalist/examples/README.md b/plugins/nemo-experimentalist/examples/README.md new file mode 100644 index 0000000000..01e64dbda8 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/README.md @@ -0,0 +1,30 @@ + + + +# Experimentalist examples + +These examples make Experimentalist behavior reviewable at three levels. They +show the complete agent-to-Harbor adapter contract without presenting sample +agents as production applications. + +| Example | Why it exists | What it demonstrates | How to use it | +|---|---|---|---| +| [`hello-harbor-agent`](hello-harbor-agent/README.md) | Small onboarding and debugger fixture. | Fully local agent, train/validation tasks, deterministic traces, two metrics, and a deliberate arithmetic gap for one optimizer round to diagnose. | Start with its README. No model key is needed for the Docker-backed evaluator A/B pytest. | +| [`tau3-nooa-agent`](tau3-nooa-agent/README.md) | Realistic interactive-agent target. | NOOA CodeAct agent, Tau3 airline tasks, MCP runtime sidecar, prepared train/validation datasets, and inference-backed user simulation. | Follow the [getting-started guide](../../../docs/get-started/example-agent.mdx) to prepare datasets, record traces, and run one SDK-backed Experimentalist smoke round. | +| [`terminal-bench-agent`](terminal-bench-agent/README.md) | Canonical benchmark runtime fixture. | Locked LangChain agent installed inside unmodified Terminal-Bench task containers, with no sidecar or task-definition changes. | Use through the [canonical benchmark runner](../benchmarks/README.md), or invoke its module directly as documented in its agent spec. | + +The examples expose the same agent-to-Harbor adapter shape: + +```text +AGENT-SPEC.md behavior contract supplied to optimizer components +agent.py / main.py code under optimization and its entry point +harbor_wrapper.py upload, install, execute, trace, and artifact bridge +dataset/ optional local tasks +optimizer.yaml optional profile for self-contained fixtures +``` + +Recommended order: + +1. Use `hello-harbor-agent` to inspect evaluator wiring and one optimizer round. +2. Use `tau3-nooa-agent` to inspect realistic MCP and inference-backed behavior. +3. Use `terminal-bench-agent` for reproducible benchmark runs. diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/.env.example b/plugins/nemo-experimentalist/examples/hello-harbor-agent/.env.example new file mode 100644 index 0000000000..9c93e60d68 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/.env.example @@ -0,0 +1,28 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Copy to `.env` in this directory. `nemo experimentalist run` auto-loads it from +# the profile directory on every run (variables already exported in your shell +# win). Verify with `nemo experimentalist doctor --profile ./optimizer.yaml`. +# +# This agent makes no LLM calls of its own — these credentials are for the +# Experimentalist's own components (Coder, Analyzer, Proposer, Terminator). + +# The one required credential: an NVIDIA Inference Gateway virtual key (sk-...). +# On the gateway, EXPERIMENTALIST_API_KEY is filled from this automatically. +INFERENCE_API_KEY=sk-... + +# Only needed for a non-gateway LLM provider (then set both; the gateway key is +# never sent to a custom endpoint): +# EXPERIMENTALIST_API_BASE=https://inference-api.nvidia.com/v1 +# EXPERIMENTALIST_API_KEY=sk-... + +# Optional model overrides. Must be models your key serves — list them with: +# curl -s $EXPERIMENTALIST_API_BASE/models -H "Authorization: Bearer $EXPERIMENTALIST_API_KEY" +# +# Prefix the served id with an EXTRA "openai/": LiteLLM strips the first path +# segment as the provider, so only the remainder reaches the gateway. +# served "openai/openai/gpt-5.6-luna" -> set "openai/openai/openai/gpt-5.6-luna" +# served "azure/openai/gpt-5.6-terra" -> set "openai/azure/openai/gpt-5.6-terra" +# EXPERIMENTALIST_SMART_MODEL_NAME=openai/openai/openai/gpt-5.5 +# EXPERIMENTALIST_FAST_MODEL_NAME=openai/openai/openai/gpt-5-mini diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/AGENT-SPEC.md b/plugins/nemo-experimentalist/examples/hello-harbor-agent/AGENT-SPEC.md new file mode 100644 index 0000000000..5a494df709 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/AGENT-SPEC.md @@ -0,0 +1,39 @@ + + + +# hello-harbor-agent + +## Job + +Read one task instruction, produce the single line of text the instruction asks +for, and write that line to `/app/artifacts/output.txt`. + +## Interface + +- Invoked as `python main.py --prompt ""` with `/app` as the + working directory. +- Writes exactly one line (plus a trailing newline) to + `/app/artifacts/output.txt`. +- Writes an OTLP JSONL trace to `/app/traces/agent.jsonl`. + +## Design + +`HelloAgent.solve` dispatches the instruction across an ordered list of +handlers and returns the first non-`None` answer, falling back to a fixed +"I do not know how to answer that." string. Today the only handler is +`handle_greeting`, which echoes a `Hello, !` line quoted in the +instruction. + +## Constraints + +- Standard library only. The task container is a bare Python image with no + package installs, no network access, and no LLM credentials. +- Deterministic: the same instruction must always produce the same answer, so + reward differences between candidates come from code changes rather than + sampling noise. + +## Known gap + +The agent has no arithmetic capability, so any task that asks it to compute a +value scores 0. This is intentional — it gives the optimization loop a real +root cause to diagnose and close. diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/README.md b/plugins/nemo-experimentalist/examples/hello-harbor-agent/README.md new file mode 100644 index 0000000000..ee69573995 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/README.md @@ -0,0 +1,198 @@ + + + +# hello-harbor-agent + +The smallest complete Experimentalist setup: a baseline agent, a Harbor +benchmark, and a profile. It exists to be **read and stepped through**, not to +measure anything useful. + +Everything is local — no dataset registry, no NeMo Platform, no LLM or network +inside the task container. A full validation evaluation takes about 8 seconds +once the image is cached. Only the Experimentalist's own components (Coder, +Analyzer, Proposer, Terminator) call an LLM. + +## Layout + +```text +optimizer.yaml profile: agent name, source, datasets, task template +AGENT-SPEC.md what the agent is supposed to do (fed to the LLM components) +agent.py the code under optimization +main.py container entry point +tracing.py hand-rolled OTLP JSONL trace writer (stdlib only) +harbor_wrapper.py the Harbor adapter — WrappedAgent.setup() / .run() +dataset/train/ greet-world, sum-two +dataset/validation/ greet-universe, sum-three +dataset/task-template/ the shape the Eval Author clones in Mode 1 +``` + +Each task directory is a standard Harbor task: + +```text +task.toml config + `artifacts = [...]` collection declaration +instruction.md the prompt handed to the agent +environment/ Dockerfile — a bare python:3.12-slim +tests/test.sh the verifier: writes /logs/verifier/reward.json +tests/expected.txt the exact line the agent should have produced +``` + +## The deliberate capability gap + +`HelloAgent.solve` dispatches to a list of handlers, and the only handler today +is `handle_greeting`. So: + +| Task | Split | Baseline result | +|---|---|---| +| `greet-world` | train | reward 1.0 | +| `sum-two` | train | reward 0.0 — falls through to the fallback string | +| `greet-universe` | validation | reward 1.0 | +| `sum-three` | validation | reward 0.0 | + +Baseline validation reward is `{"reward": 0.5, "format_ok": 1.0}`. The missing +arithmetic handler is a real root cause for the Analyzer to find, the Proposer +to describe, and the Coder to fix — which is what the one-round debug config +exercises. + +Two metric keys (`reward` and `format_ok`) rather than one is also deliberate: +candidates are then compared in 2-D, so the Pareto ranking does something +visible. + +## What a one-round run actually does + +Observed with a one-round Experimentalist run against this profile: + +1. The Analyzer reads the failing trial's trace and diagnoses it correctly — + *"the baseline agent lacks a routing or handler capability for two-integer sum + requests … falls through to fallback"*. +2. The Proposer emits an `add_concrete_method` improvement: add a `handle_sum` + node ahead of `handle_greeting`. +3. The Coder implements it, and `agent-1` now passes `sum-two`. +4. **Validation still scores 0.5.** With `max_train_batch_tasks: 1` the loop only + saw the two-operand `sum-two`, so the Coder wrote a deliberately two-integer + handler — and the held-out `sum-three` ("sum of 8, 13 and 4") is three-operand. +5. `agent-1` ties `agent-0` on validation, so the baseline stays the winner. + +That is the held-out split doing its job: a change that fixes the training +failure is caught not generalizing. It is the most useful thing to watch on a +first run, so the example is tuned to produce it. + +## Running it + +```bash +export INFERENCE_API_KEY=sk-... +bash tmp/run.sh +``` + +Or from VS Code, the `Experimentalist: hello — eval only` launch configuration. +Check prerequisites first: + +```bash +uv run nemo experimentalist doctor --profile plugins/nemo-experimentalist/examples/hello-harbor-agent/optimizer.yaml +``` + +## Running the evaluation through the NeMo Evaluator SDK + +The example ships two ways to run the exact same Harbor evaluation: + +| `evaluator_type` | Who owns the orchestration | +|---|---| +| `harbor_native` (default) | The plugin builds Harbor's `JobConfig` and drives `Job` itself. | +| `harbor_evaluator` | The NeMo Evaluator SDK's `HarborAgentTaskRunner` owns the `JobConfig`, the job-directory cache, and the scoped agent import. | + +**The SDK runner still uses Harbor underneath.** Same containers, same verifiers, +same `result.json` tree. Only orchestration ownership moves, and results are read +back off the job directory by the same adapter either way — so the two produce +equivalent trials and identical metrics. Nothing here talks to NeMo Platform, so +no service needs to be running and the platform port is irrelevant. + +### Prerequisites + +- Python 3.12 and a synced workspace: `uv sync --group experimentalist` +- A running Docker daemon (`docker info` must succeed) +- **No model API key** for the commands in this section — the evaluator seam + makes no LLM calls. A key is only needed for the full optimizer loop below, + whose Coder, Analyzer, and Proposer do call a model. + +### A/B the two evaluators (no model key) + +The live A/B is a pytest module. It needs Docker and `harbor`, and is skipped +otherwise. Every command below runs from the **platform root**, not this example +directory: + +```bash +uv run pytest plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_ab_e2e.py -v +``` + +Both evaluator arms should report the same aggregate (about 10 s each once the +image is cached): + +```text +{"format_ok": 1.0, "reward": 0.5} +``` + +That is the deliberate capability gap from the table above: `greet-universe` +passes, `sum-three` does not, and both emit `format_ok`. + +Artifacts land under the test's temporary experiment directory, one directory +per trial: + +```text +agent-0-validation/ + result.json aggregate job result + greet-universe__/ + result.json task_name, verifier rewards, exception info, timings + trial.log Harbor's orchestration log + verifier/reward.json what tests/test.sh wrote + artifacts/traces/*.jsonl OTLP traces the Analyzer reads +``` + +### Caching + +`harbor_evaluator` treats the job directory as a **success-aware cache**. +Re-running the same evaluation finishes quickly without touching Docker when +every requested task already has `n_attempts` completed, non-errored trials. A +run that errored, was interrupted, or is under-sampled is re-run rather than +served from a partial cache. + +The cache only engages because the loop pins a deterministic job name +(`-`). Setting a custom `job_name` per run defeats it. + +### The full optimizer loop + +These commands **do** need a model key, because the loop's Coder writes +`architecture.md` and the Terminator writes `OPTIMIZATION.md`: + +```bash +export INFERENCE_API_KEY=sk-... +``` + +```bash +uv run nemo experimentalist run \ + --profile plugins/nemo-experimentalist/examples/hello-harbor-agent/optimizer.yaml \ + --no-insight \ + --experiment-dir tmp/exp-hello +``` + +Pass a local `--config` YAML when you want to pin `evaluator_type`, round +limits, or other loop knobs. The only difference that matters for the Harbor A/B +is `evaluator_type`; the validation aggregate is `{"reward": 0.5, "format_ok": 1.0}` +either way. + +### Configuring the SDK evaluator + +Keys under `evaluator:` map 1:1 onto the SDK's `HarborRuntimeConfig`, and unknown +keys are **rejected rather than ignored**: + +```yaml +evaluator_type: harbor_evaluator +evaluator: + n_attempts: 1 + n_concurrent_trials: 4 + max_retries: 0 # NOT `retry:` — the plain evaluator's RetryConfig has no SDK equivalent + quiet: false + trace_dir: /app/traces + agent_timeout_multiplier: 1.0 +``` + +`agent_dir` is not configurable: it is always the candidate being evaluated, so a +config cannot point the run at different code. diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/agent.py b/plugins/nemo-experimentalist/examples/hello-harbor-agent/agent.py new file mode 100644 index 0000000000..8071c1e2f9 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/agent.py @@ -0,0 +1,55 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""The agent under optimization. + +Deliberately simple and deterministic: no LLM, no network, standard library +only. The Experimentalist treats this file as the thing to mutate, so what +matters is that it has an obvious capability gap (it can greet, but it cannot do +arithmetic) for the loop to discover and close. +""" + +from __future__ import annotations + +import re + +GREETING_RE = re.compile(r"\bhello,\s*([A-Za-z0-9 _-]+)!", re.IGNORECASE) + +FALLBACK = "I do not know how to answer that." + + +class HelloAgent: + """Route a task instruction to a handler and return the answer line.""" + + def solve(self, instruction: str) -> str: + """Return the single output line this agent believes the task wants. + + Args: + instruction: The full task instruction text. + + Returns: + The answer line to write to the output file. + """ + # Intentionally a one-element tuple: the arithmetic gap this leaves is the + # example's whole point, and adding a `handle_sum` node here is precisely + # the round-1 improvement the Proposer and Coder are supposed to discover. + # Do not "fix" the baseline — see README.md, "The deliberate capability gap". + for handler in (self.handle_greeting,): + answer = handler(instruction) + if answer is not None: + return answer + return FALLBACK + + def handle_greeting(self, instruction: str) -> str | None: + """Echo back a `Hello, !` line quoted in the instruction. + + Args: + instruction: The full task instruction text. + + Returns: + The greeting line, or None when the instruction is not a greeting task. + """ + match = GREETING_RE.search(instruction) + if match is None: + return None + return f"Hello, {match.group(1).strip()}!" diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/README.md b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/README.md new file mode 100644 index 0000000000..dce76bfc7f --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/README.md @@ -0,0 +1,16 @@ + + + +# task-template + +The shape the Eval Author clones when it turns a production trace into a new +Harbor task (Mode 1, `--insight`). It is a complete, runnable task in its own +right — a copy of `train/greet-world` — so `nemo experimentalist doctor` can +validate it. + +Mode 2 (`--no-insight`, what the debug launch configs use) never reads this +directory, but the profile schema still requires `task_template` to be set. + +Note the name: `HarborDataset._find_task_dirs` skips a directory literally named +`task_template`, so a template nested inside a dataset is not picked up as a +task. This one lives outside `train/` and `validation/`, so either name works. diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/environment/Dockerfile b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/environment/Dockerfile new file mode 100644 index 0000000000..455684d845 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/environment/Dockerfile @@ -0,0 +1,11 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# The whole task environment: a bare Python image. The agent is stdlib-only, so +# there is nothing to install and the image builds in seconds. + +FROM python:3.12-slim + +WORKDIR /app + +RUN mkdir -p /app/artifacts /app/traces diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/instruction.md b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/instruction.md new file mode 100644 index 0000000000..0c8166b79f --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/instruction.md @@ -0,0 +1,9 @@ +Write a single line of text to `/app/artifacts/output.txt`. + +The line must be exactly: + +``` +Hello, world! +``` + +No extra words, no surrounding quotes, no leading or trailing spaces. diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/task.toml b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/task.toml new file mode 100644 index 0000000000..38226555e7 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/task.toml @@ -0,0 +1,27 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +schema_version = "1.1" + +# Collected into the trial's artifacts/ directory on the host after the agent +# phase. The Experimentalist separately injects /app/traces -> artifacts/traces. +artifacts = [{ source = "/app/artifacts", destination = "output" }] + +[task] +name = "hello/task-template" +authors = [{ name = "NVIDIA" }] +keywords = ["hello", "onboarding"] + +[metadata] +difficulty = "easy" +category = "smoke" + +[agent] +timeout_sec = 300.0 + +[verifier] +timeout_sec = 60.0 + +[environment] +build_timeout_sec = 600.0 +cpus = 1 +memory_mb = 1024 diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/expected.txt b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/expected.txt new file mode 100644 index 0000000000..af5626b4a1 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/expected.txt @@ -0,0 +1 @@ +Hello, world! diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/test.sh b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/test.sh new file mode 100755 index 0000000000..dce0a62405 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/test.sh @@ -0,0 +1,61 @@ +#!/usr/bin/env bash +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Harbor copies this directory to /tests in the task container and runs this +# script after the agent phase. Its only job is to write numeric rewards to +# /logs/verifier/reward.json — every value must be a plain number, and every +# task must emit the SAME keys, because the Experimentalist averages metrics +# across trials and rejects inconsistent metric sets. +# +# Emitted metrics: +# reward 1.0 when the output line matches tests/expected.txt exactly +# format_ok 1.0 when the agent wrote an output file at all +# +# Never `set -e` here: a non-zero exit before reward.json is written turns a +# legitimate 0 score into a missing metric. +set -uo pipefail + +mkdir -p /logs/verifier + +OUTPUT=/app/artifacts/output.txt +# Whole-file comparison, not just the first line: the agent under test is +# LLM-generated code the optimizer is actively reward-maximizing, so a verifier +# that ignores trailing output is a reward-hacking surface. The comparison is +# byte-for-byte after end-of-line CRLF normalization: a missing, extra, or +# duplicated trailing newline all fail. +# +# CRLF pairs are normalized at end-of-line only. `tr -d '\r'` would delete +# *every* carriage return, so `sum=42` would collapse to `sum=42` and score a +# false 1.0 — the same reward-hacking class as the trailing-output hole above. +EXPECTED_FILE=/tests/expected.txt +reward=0.0 +format_ok=0.0 + +# Fail closed on a missing fixture. `set -e` is deliberately off (see above), so a +# failed read would otherwise leave EXPECTED empty and let an empty output compare +# equal to it and score 1.0. +if [ ! -r "$EXPECTED_FILE" ]; then + echo "FAIL: ${EXPECTED_FILE} is missing or unreadable; refusing to score" +elif [ -f "$OUTPUT" ]; then + format_ok=1.0 + # Byte-for-byte via `cmp`, not `[ "$ACTUAL" = "$EXPECTED" ]`: command substitution + # strips *every* trailing newline on both sides, so an agent could append blank + # lines and still score 1.0. That is a reward-hacking surface, because the code + # under test is LLM-generated and the optimizer is actively maximizing this number. + EXPECTED_NORM="$(mktemp)" + ACTUAL_NORM="$(mktemp)" + sed 's/\r$//' "$EXPECTED_FILE" > "$EXPECTED_NORM" + sed 's/\r$//' "$OUTPUT" > "$ACTUAL_NORM" + echo "expected: [$(cat "$EXPECTED_NORM")]" + echo "actual: [$(cat "$ACTUAL_NORM")]" + if cmp -s "$EXPECTED_NORM" "$ACTUAL_NORM"; then + reward=1.0 + fi + rm -f "$EXPECTED_NORM" "$ACTUAL_NORM" +else + echo "FAIL: ${OUTPUT} was not created by the agent" +fi + +printf '{"reward": %s, "format_ok": %s}\n' "$reward" "$format_ok" > /logs/verifier/reward.json +cat /logs/verifier/reward.json diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/environment/Dockerfile b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/environment/Dockerfile new file mode 100644 index 0000000000..455684d845 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/environment/Dockerfile @@ -0,0 +1,11 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# The whole task environment: a bare Python image. The agent is stdlib-only, so +# there is nothing to install and the image builds in seconds. + +FROM python:3.12-slim + +WORKDIR /app + +RUN mkdir -p /app/artifacts /app/traces diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/instruction.md b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/instruction.md new file mode 100644 index 0000000000..0c8166b79f --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/instruction.md @@ -0,0 +1,9 @@ +Write a single line of text to `/app/artifacts/output.txt`. + +The line must be exactly: + +``` +Hello, world! +``` + +No extra words, no surrounding quotes, no leading or trailing spaces. diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/task.toml b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/task.toml new file mode 100644 index 0000000000..f2f8d25427 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/task.toml @@ -0,0 +1,27 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +schema_version = "1.1" + +# Collected into the trial's artifacts/ directory on the host after the agent +# phase. The Experimentalist separately injects /app/traces -> artifacts/traces. +artifacts = [{ source = "/app/artifacts", destination = "output" }] + +[task] +name = "hello/greet-world" +authors = [{ name = "NVIDIA" }] +keywords = ["hello", "onboarding"] + +[metadata] +difficulty = "easy" +category = "smoke" + +[agent] +timeout_sec = 300.0 + +[verifier] +timeout_sec = 60.0 + +[environment] +build_timeout_sec = 600.0 +cpus = 1 +memory_mb = 1024 diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/expected.txt b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/expected.txt new file mode 100644 index 0000000000..af5626b4a1 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/expected.txt @@ -0,0 +1 @@ +Hello, world! diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/test.sh b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/test.sh new file mode 100755 index 0000000000..dce0a62405 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/test.sh @@ -0,0 +1,61 @@ +#!/usr/bin/env bash +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Harbor copies this directory to /tests in the task container and runs this +# script after the agent phase. Its only job is to write numeric rewards to +# /logs/verifier/reward.json — every value must be a plain number, and every +# task must emit the SAME keys, because the Experimentalist averages metrics +# across trials and rejects inconsistent metric sets. +# +# Emitted metrics: +# reward 1.0 when the output line matches tests/expected.txt exactly +# format_ok 1.0 when the agent wrote an output file at all +# +# Never `set -e` here: a non-zero exit before reward.json is written turns a +# legitimate 0 score into a missing metric. +set -uo pipefail + +mkdir -p /logs/verifier + +OUTPUT=/app/artifacts/output.txt +# Whole-file comparison, not just the first line: the agent under test is +# LLM-generated code the optimizer is actively reward-maximizing, so a verifier +# that ignores trailing output is a reward-hacking surface. The comparison is +# byte-for-byte after end-of-line CRLF normalization: a missing, extra, or +# duplicated trailing newline all fail. +# +# CRLF pairs are normalized at end-of-line only. `tr -d '\r'` would delete +# *every* carriage return, so `sum=42` would collapse to `sum=42` and score a +# false 1.0 — the same reward-hacking class as the trailing-output hole above. +EXPECTED_FILE=/tests/expected.txt +reward=0.0 +format_ok=0.0 + +# Fail closed on a missing fixture. `set -e` is deliberately off (see above), so a +# failed read would otherwise leave EXPECTED empty and let an empty output compare +# equal to it and score 1.0. +if [ ! -r "$EXPECTED_FILE" ]; then + echo "FAIL: ${EXPECTED_FILE} is missing or unreadable; refusing to score" +elif [ -f "$OUTPUT" ]; then + format_ok=1.0 + # Byte-for-byte via `cmp`, not `[ "$ACTUAL" = "$EXPECTED" ]`: command substitution + # strips *every* trailing newline on both sides, so an agent could append blank + # lines and still score 1.0. That is a reward-hacking surface, because the code + # under test is LLM-generated and the optimizer is actively maximizing this number. + EXPECTED_NORM="$(mktemp)" + ACTUAL_NORM="$(mktemp)" + sed 's/\r$//' "$EXPECTED_FILE" > "$EXPECTED_NORM" + sed 's/\r$//' "$OUTPUT" > "$ACTUAL_NORM" + echo "expected: [$(cat "$EXPECTED_NORM")]" + echo "actual: [$(cat "$ACTUAL_NORM")]" + if cmp -s "$EXPECTED_NORM" "$ACTUAL_NORM"; then + reward=1.0 + fi + rm -f "$EXPECTED_NORM" "$ACTUAL_NORM" +else + echo "FAIL: ${OUTPUT} was not created by the agent" +fi + +printf '{"reward": %s, "format_ok": %s}\n' "$reward" "$format_ok" > /logs/verifier/reward.json +cat /logs/verifier/reward.json diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/environment/Dockerfile b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/environment/Dockerfile new file mode 100644 index 0000000000..455684d845 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/environment/Dockerfile @@ -0,0 +1,11 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# The whole task environment: a bare Python image. The agent is stdlib-only, so +# there is nothing to install and the image builds in seconds. + +FROM python:3.12-slim + +WORKDIR /app + +RUN mkdir -p /app/artifacts /app/traces diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/instruction.md b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/instruction.md new file mode 100644 index 0000000000..586b7bae41 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/instruction.md @@ -0,0 +1,10 @@ +Compute the sum of 17 and 25. + +Write a single line of text to `/app/artifacts/output.txt` in exactly this form: + +``` +sum= +``` + +For example, if the answer were 3, the file would contain `sum=3`. No spaces +around the `=`, no extra words. diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/task.toml b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/task.toml new file mode 100644 index 0000000000..4c12692b84 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/task.toml @@ -0,0 +1,27 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +schema_version = "1.1" + +# Collected into the trial's artifacts/ directory on the host after the agent +# phase. The Experimentalist separately injects /app/traces -> artifacts/traces. +artifacts = [{ source = "/app/artifacts", destination = "output" }] + +[task] +name = "hello/sum-two" +authors = [{ name = "NVIDIA" }] +keywords = ["hello", "onboarding"] + +[metadata] +difficulty = "easy" +category = "smoke" + +[agent] +timeout_sec = 300.0 + +[verifier] +timeout_sec = 60.0 + +[environment] +build_timeout_sec = 600.0 +cpus = 1 +memory_mb = 1024 diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/expected.txt b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/expected.txt new file mode 100644 index 0000000000..fa626191cc --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/expected.txt @@ -0,0 +1 @@ +sum=42 diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/test.sh b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/test.sh new file mode 100755 index 0000000000..dce0a62405 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/test.sh @@ -0,0 +1,61 @@ +#!/usr/bin/env bash +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Harbor copies this directory to /tests in the task container and runs this +# script after the agent phase. Its only job is to write numeric rewards to +# /logs/verifier/reward.json — every value must be a plain number, and every +# task must emit the SAME keys, because the Experimentalist averages metrics +# across trials and rejects inconsistent metric sets. +# +# Emitted metrics: +# reward 1.0 when the output line matches tests/expected.txt exactly +# format_ok 1.0 when the agent wrote an output file at all +# +# Never `set -e` here: a non-zero exit before reward.json is written turns a +# legitimate 0 score into a missing metric. +set -uo pipefail + +mkdir -p /logs/verifier + +OUTPUT=/app/artifacts/output.txt +# Whole-file comparison, not just the first line: the agent under test is +# LLM-generated code the optimizer is actively reward-maximizing, so a verifier +# that ignores trailing output is a reward-hacking surface. The comparison is +# byte-for-byte after end-of-line CRLF normalization: a missing, extra, or +# duplicated trailing newline all fail. +# +# CRLF pairs are normalized at end-of-line only. `tr -d '\r'` would delete +# *every* carriage return, so `sum=42` would collapse to `sum=42` and score a +# false 1.0 — the same reward-hacking class as the trailing-output hole above. +EXPECTED_FILE=/tests/expected.txt +reward=0.0 +format_ok=0.0 + +# Fail closed on a missing fixture. `set -e` is deliberately off (see above), so a +# failed read would otherwise leave EXPECTED empty and let an empty output compare +# equal to it and score 1.0. +if [ ! -r "$EXPECTED_FILE" ]; then + echo "FAIL: ${EXPECTED_FILE} is missing or unreadable; refusing to score" +elif [ -f "$OUTPUT" ]; then + format_ok=1.0 + # Byte-for-byte via `cmp`, not `[ "$ACTUAL" = "$EXPECTED" ]`: command substitution + # strips *every* trailing newline on both sides, so an agent could append blank + # lines and still score 1.0. That is a reward-hacking surface, because the code + # under test is LLM-generated and the optimizer is actively maximizing this number. + EXPECTED_NORM="$(mktemp)" + ACTUAL_NORM="$(mktemp)" + sed 's/\r$//' "$EXPECTED_FILE" > "$EXPECTED_NORM" + sed 's/\r$//' "$OUTPUT" > "$ACTUAL_NORM" + echo "expected: [$(cat "$EXPECTED_NORM")]" + echo "actual: [$(cat "$ACTUAL_NORM")]" + if cmp -s "$EXPECTED_NORM" "$ACTUAL_NORM"; then + reward=1.0 + fi + rm -f "$EXPECTED_NORM" "$ACTUAL_NORM" +else + echo "FAIL: ${OUTPUT} was not created by the agent" +fi + +printf '{"reward": %s, "format_ok": %s}\n' "$reward" "$format_ok" > /logs/verifier/reward.json +cat /logs/verifier/reward.json diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/environment/Dockerfile b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/environment/Dockerfile new file mode 100644 index 0000000000..455684d845 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/environment/Dockerfile @@ -0,0 +1,11 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# The whole task environment: a bare Python image. The agent is stdlib-only, so +# there is nothing to install and the image builds in seconds. + +FROM python:3.12-slim + +WORKDIR /app + +RUN mkdir -p /app/artifacts /app/traces diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/instruction.md b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/instruction.md new file mode 100644 index 0000000000..ed70b3da52 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/instruction.md @@ -0,0 +1,9 @@ +Write a single line of text to `/app/artifacts/output.txt`. + +The line must be exactly: + +``` +Hello, universe! +``` + +No extra words, no surrounding quotes, no leading or trailing spaces. diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/task.toml b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/task.toml new file mode 100644 index 0000000000..21ed4d2f3b --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/task.toml @@ -0,0 +1,27 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +schema_version = "1.1" + +# Collected into the trial's artifacts/ directory on the host after the agent +# phase. The Experimentalist separately injects /app/traces -> artifacts/traces. +artifacts = [{ source = "/app/artifacts", destination = "output" }] + +[task] +name = "hello/greet-universe" +authors = [{ name = "NVIDIA" }] +keywords = ["hello", "onboarding"] + +[metadata] +difficulty = "easy" +category = "smoke" + +[agent] +timeout_sec = 300.0 + +[verifier] +timeout_sec = 60.0 + +[environment] +build_timeout_sec = 600.0 +cpus = 1 +memory_mb = 1024 diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/expected.txt b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/expected.txt new file mode 100644 index 0000000000..4d3a7c2e79 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/expected.txt @@ -0,0 +1 @@ +Hello, universe! diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/test.sh b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/test.sh new file mode 100755 index 0000000000..dce0a62405 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/test.sh @@ -0,0 +1,61 @@ +#!/usr/bin/env bash +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Harbor copies this directory to /tests in the task container and runs this +# script after the agent phase. Its only job is to write numeric rewards to +# /logs/verifier/reward.json — every value must be a plain number, and every +# task must emit the SAME keys, because the Experimentalist averages metrics +# across trials and rejects inconsistent metric sets. +# +# Emitted metrics: +# reward 1.0 when the output line matches tests/expected.txt exactly +# format_ok 1.0 when the agent wrote an output file at all +# +# Never `set -e` here: a non-zero exit before reward.json is written turns a +# legitimate 0 score into a missing metric. +set -uo pipefail + +mkdir -p /logs/verifier + +OUTPUT=/app/artifacts/output.txt +# Whole-file comparison, not just the first line: the agent under test is +# LLM-generated code the optimizer is actively reward-maximizing, so a verifier +# that ignores trailing output is a reward-hacking surface. The comparison is +# byte-for-byte after end-of-line CRLF normalization: a missing, extra, or +# duplicated trailing newline all fail. +# +# CRLF pairs are normalized at end-of-line only. `tr -d '\r'` would delete +# *every* carriage return, so `sum=42` would collapse to `sum=42` and score a +# false 1.0 — the same reward-hacking class as the trailing-output hole above. +EXPECTED_FILE=/tests/expected.txt +reward=0.0 +format_ok=0.0 + +# Fail closed on a missing fixture. `set -e` is deliberately off (see above), so a +# failed read would otherwise leave EXPECTED empty and let an empty output compare +# equal to it and score 1.0. +if [ ! -r "$EXPECTED_FILE" ]; then + echo "FAIL: ${EXPECTED_FILE} is missing or unreadable; refusing to score" +elif [ -f "$OUTPUT" ]; then + format_ok=1.0 + # Byte-for-byte via `cmp`, not `[ "$ACTUAL" = "$EXPECTED" ]`: command substitution + # strips *every* trailing newline on both sides, so an agent could append blank + # lines and still score 1.0. That is a reward-hacking surface, because the code + # under test is LLM-generated and the optimizer is actively maximizing this number. + EXPECTED_NORM="$(mktemp)" + ACTUAL_NORM="$(mktemp)" + sed 's/\r$//' "$EXPECTED_FILE" > "$EXPECTED_NORM" + sed 's/\r$//' "$OUTPUT" > "$ACTUAL_NORM" + echo "expected: [$(cat "$EXPECTED_NORM")]" + echo "actual: [$(cat "$ACTUAL_NORM")]" + if cmp -s "$EXPECTED_NORM" "$ACTUAL_NORM"; then + reward=1.0 + fi + rm -f "$EXPECTED_NORM" "$ACTUAL_NORM" +else + echo "FAIL: ${OUTPUT} was not created by the agent" +fi + +printf '{"reward": %s, "format_ok": %s}\n' "$reward" "$format_ok" > /logs/verifier/reward.json +cat /logs/verifier/reward.json diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/environment/Dockerfile b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/environment/Dockerfile new file mode 100644 index 0000000000..455684d845 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/environment/Dockerfile @@ -0,0 +1,11 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# The whole task environment: a bare Python image. The agent is stdlib-only, so +# there is nothing to install and the image builds in seconds. + +FROM python:3.12-slim + +WORKDIR /app + +RUN mkdir -p /app/artifacts /app/traces diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/instruction.md b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/instruction.md new file mode 100644 index 0000000000..8dfe45ba4c --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/instruction.md @@ -0,0 +1,10 @@ +Compute the sum of 8, 13 and 4. + +Write a single line of text to `/app/artifacts/output.txt` in exactly this form: + +``` +sum= +``` + +For example, if the answer were 3, the file would contain `sum=3`. No spaces +around the `=`, no extra words. diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/task.toml b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/task.toml new file mode 100644 index 0000000000..542d75c10b --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/task.toml @@ -0,0 +1,27 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +schema_version = "1.1" + +# Collected into the trial's artifacts/ directory on the host after the agent +# phase. The Experimentalist separately injects /app/traces -> artifacts/traces. +artifacts = [{ source = "/app/artifacts", destination = "output" }] + +[task] +name = "hello/sum-three" +authors = [{ name = "NVIDIA" }] +keywords = ["hello", "onboarding"] + +[metadata] +difficulty = "easy" +category = "smoke" + +[agent] +timeout_sec = 300.0 + +[verifier] +timeout_sec = 60.0 + +[environment] +build_timeout_sec = 600.0 +cpus = 1 +memory_mb = 1024 diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/expected.txt b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/expected.txt new file mode 100644 index 0000000000..7283bb3427 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/expected.txt @@ -0,0 +1 @@ +sum=25 diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/test.sh b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/test.sh new file mode 100755 index 0000000000..dce0a62405 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/test.sh @@ -0,0 +1,61 @@ +#!/usr/bin/env bash +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Harbor copies this directory to /tests in the task container and runs this +# script after the agent phase. Its only job is to write numeric rewards to +# /logs/verifier/reward.json — every value must be a plain number, and every +# task must emit the SAME keys, because the Experimentalist averages metrics +# across trials and rejects inconsistent metric sets. +# +# Emitted metrics: +# reward 1.0 when the output line matches tests/expected.txt exactly +# format_ok 1.0 when the agent wrote an output file at all +# +# Never `set -e` here: a non-zero exit before reward.json is written turns a +# legitimate 0 score into a missing metric. +set -uo pipefail + +mkdir -p /logs/verifier + +OUTPUT=/app/artifacts/output.txt +# Whole-file comparison, not just the first line: the agent under test is +# LLM-generated code the optimizer is actively reward-maximizing, so a verifier +# that ignores trailing output is a reward-hacking surface. The comparison is +# byte-for-byte after end-of-line CRLF normalization: a missing, extra, or +# duplicated trailing newline all fail. +# +# CRLF pairs are normalized at end-of-line only. `tr -d '\r'` would delete +# *every* carriage return, so `sum=42` would collapse to `sum=42` and score a +# false 1.0 — the same reward-hacking class as the trailing-output hole above. +EXPECTED_FILE=/tests/expected.txt +reward=0.0 +format_ok=0.0 + +# Fail closed on a missing fixture. `set -e` is deliberately off (see above), so a +# failed read would otherwise leave EXPECTED empty and let an empty output compare +# equal to it and score 1.0. +if [ ! -r "$EXPECTED_FILE" ]; then + echo "FAIL: ${EXPECTED_FILE} is missing or unreadable; refusing to score" +elif [ -f "$OUTPUT" ]; then + format_ok=1.0 + # Byte-for-byte via `cmp`, not `[ "$ACTUAL" = "$EXPECTED" ]`: command substitution + # strips *every* trailing newline on both sides, so an agent could append blank + # lines and still score 1.0. That is a reward-hacking surface, because the code + # under test is LLM-generated and the optimizer is actively maximizing this number. + EXPECTED_NORM="$(mktemp)" + ACTUAL_NORM="$(mktemp)" + sed 's/\r$//' "$EXPECTED_FILE" > "$EXPECTED_NORM" + sed 's/\r$//' "$OUTPUT" > "$ACTUAL_NORM" + echo "expected: [$(cat "$EXPECTED_NORM")]" + echo "actual: [$(cat "$ACTUAL_NORM")]" + if cmp -s "$EXPECTED_NORM" "$ACTUAL_NORM"; then + reward=1.0 + fi + rm -f "$EXPECTED_NORM" "$ACTUAL_NORM" +else + echo "FAIL: ${OUTPUT} was not created by the agent" +fi + +printf '{"reward": %s, "format_ok": %s}\n' "$reward" "$format_ok" > /logs/verifier/reward.json +cat /logs/verifier/reward.json diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/harbor_wrapper.py b/plugins/nemo-experimentalist/examples/hello-harbor-agent/harbor_wrapper.py new file mode 100644 index 0000000000..e1b1b8033c --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/harbor_wrapper.py @@ -0,0 +1,124 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Harbor adapter for the agent in this directory. + +`HarborEvaluatorConfig.import_path` defaults to `harbor_wrapper:WrappedAgent`, so +Harbor imports this module out of whichever `agents/agent-N/` directory is being +evaluated and drives the agent through `setup()` then `run()`. + +Artifact contract — the agent just writes to two directories and Harbor collects +them into the trial's `artifacts/` directory on the host: + - `/app/artifacts` -> `artifacts/output/`, declared by `artifacts = [...]` in + each task.toml. + - `/app/traces` -> `artifacts/traces/`, injected automatically by + `HarborEvaluator._run` via `_with_trace_artifact`. +Nothing needs to be copied by hand. +""" + +from __future__ import annotations + +import fnmatch +import logging +import shlex +from pathlib import Path + +from harbor import AgentContext, BaseAgent, BaseEnvironment + +logger = logging.getLogger(__name__) + +AGENT_DIR = Path(__file__).parent + +# Never uploaded into the task container: optimizer bookkeeping, caches, the +# dataset itself, and anything holding credentials. +EXCLUDE = { + "eval-and-optimize", + "__pycache__", + ".git", + ".claude", + ".uv", + ".venv", + ".env", + ".env.example", + "traces", + "artifacts", + "dataset", +} +EXCLUDE_GLOB = {"output.*", "*.md"} + + +class SymlinkedUploadError(RuntimeError): + """A selected upload path is, or contains, a symlink.""" + + +def _reject_symlinks(entries: list[Path]) -> None: + """Raise if any selected entry is a symlink or holds one at any depth. + + Args: + entries: Top-level paths selected for upload. + + Raises: + SymlinkedUploadError: naming the first offending path. + """ + for entry in entries: + offenders = [entry] if entry.is_symlink() else [] + if entry.is_dir() and not entry.is_symlink(): + offenders.extend(child for child in entry.rglob("*") if child.is_symlink()) + if offenders: + raise SymlinkedUploadError( + f"refusing to upload {offenders[0]}: it is a symlink, and following it would copy " + "host files outside the agent directory into the task container" + ) + + +class WrappedAgent(BaseAgent): + """Upload this agent directory into the container and run one task.""" + + @staticmethod + def name() -> str: + return "hello-harbor-agent" + + def version(self) -> str | None: + return "1.0.0" + + async def setup(self, environment: BaseEnvironment) -> None: + """Upload the agent's source files. No dependency install: stdlib only.""" + selected = [ + entry + for entry in AGENT_DIR.iterdir() + if entry.name not in EXCLUDE and not any(fnmatch.fnmatch(entry.name, pattern) for pattern in EXCLUDE_GLOB) + ] + # Refuse symlinks before uploading anything. `upload_dir` follows them, so a + # link anywhere in a selected subtree would copy host files into the container + # — `.env`, an SSH key, anything the exclude list names but a link bypasses. + # Scanned up front so a rejection cannot leave a half-populated /app. + _reject_symlinks(selected) + for entry in selected: + if entry.is_file(): + await environment.upload_file(entry, f"/app/{entry.name}") + elif entry.is_dir(): + await environment.upload_dir(entry, f"/app/{entry.name}") + logger.info("[setup] uploaded agent sources to /app") + + async def run( + self, + instruction: str, + environment: BaseEnvironment, + context: AgentContext, + ) -> None: + """Execute the agent on *instruction* inside the task container.""" + session_id = self.session_id or "local" + proc = await environment.exec( + f"cd /app && python main.py --prompt {shlex.quote(instruction.strip())} " + f"--session-id {shlex.quote(session_id)}" + ) + + # Nothing is copied here: Harbor collects /app/artifacts and /app/traces + # after this method returns, per the declarations described above. + context.metadata = { + "stdout": proc.stdout, + "stderr": proc.stderr, + "returncode": proc.return_code, + } + if proc.return_code != 0: + raise RuntimeError(f"Agent process failed with exit code {proc.return_code}: {proc.stderr or proc.stdout}") diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/main.py b/plugins/nemo-experimentalist/examples/hello-harbor-agent/main.py new file mode 100644 index 0000000000..2b62d0d493 --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/main.py @@ -0,0 +1,37 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Container entry point: solve one task and write the answer + a trace.""" + +from __future__ import annotations + +import argparse +import os +from pathlib import Path + +from agent import FALLBACK, HelloAgent +from tracing import write_trace + +ARTIFACTS_DIR = Path(os.environ.get("ARTIFACTS_DIR", "/app/artifacts")) +OUTPUT_PATH = ARTIFACTS_DIR / "output.txt" + + +def main() -> None: + """Run the agent on --prompt and write /app/artifacts/output.txt.""" + parser = argparse.ArgumentParser() + parser.add_argument("--prompt", required=True) + parser.add_argument("--session-id", default=os.environ.get("HARBOR_SESSION_ID", "local")) + args = parser.parse_args() + + answer = HelloAgent().solve(args.prompt) + handler = "fallback" if answer == FALLBACK else "handle_greeting" + + ARTIFACTS_DIR.mkdir(parents=True, exist_ok=True) + OUTPUT_PATH.write_text(answer + "\n", encoding="utf-8") + + trace_path = write_trace(args.session_id, args.prompt, answer, handler) + print(f"answer={answer!r} handler={handler} output={OUTPUT_PATH} trace={trace_path}") + + +if __name__ == "__main__": + main() diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/optimizer.yaml b/plugins/nemo-experimentalist/examples/hello-harbor-agent/optimizer.yaml new file mode 100644 index 0000000000..515bf2c49e --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/optimizer.yaml @@ -0,0 +1,35 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# Minimal Experimentalist profile — the onboarding/debug target. +# +# Everything here is local: no dataset registry, no NeMo Platform, no LLM inside +# the task container. Only the Experimentalist's own agents call an LLM. +# +# nemo experimentalist doctor --profile examples/hello-harbor-agent/optimizer.yaml +# nemo experimentalist run --profile examples/hello-harbor-agent/optimizer.yaml \ +# --no-insight + +# Logical agent name. In Mode 1 it must match the Insight's agent; in Mode 2 +# (--no-insight) it is only a label. +agent: hello-harbor-agent + +# Where the baseline agent's code lives, relative to this file. "." means this +# directory — dataset/ is excluded from the copy by the loop's ignore rules. +agent_source: . + +# Markdown description of the agent under test, threaded to the analyzer and +# goal-tree components. +agent_spec: ./AGENT-SPEC.md + +# Required by the profile schema even in Mode 2, where it is never read. It +# points at a single Harbor task directory used as the shape for Eval Author +# generated tasks in Mode 1. +task_template: ./dataset/task-template + +datasets: + # A "./" prefix always classifies as a local path, so no registry_url is + # needed and nothing is downloaded. + train: ./dataset/train + validation: ./dataset/validation + +workspace: default diff --git a/plugins/nemo-experimentalist/examples/hello-harbor-agent/tracing.py b/plugins/nemo-experimentalist/examples/hello-harbor-agent/tracing.py new file mode 100644 index 0000000000..029e16097d --- /dev/null +++ b/plugins/nemo-experimentalist/examples/hello-harbor-agent/tracing.py @@ -0,0 +1,99 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Minimal OTLP JSONL trace writer — standard library only. + +Harbor collects `/app/traces` into the trial's `artifacts/traces/` directory, and +the Experimentalist's `TrialResult.trace` points at the first `*.jsonl` it finds +there. The Analyzer's TraceExplorer reads that file, so the shape below is the +contract: one `ExportTraceServiceRequest` JSON object per line, spans under +`resourceSpans[].scopeSpans[].spans[]`, OpenInference semantics in attributes. + +Real agents get this for free from an OpenTelemetry SDK exporter (see +`examples/terminal-bench-agent/tracing.py`). This one is hand-rolled so the task +container needs no dependencies beyond the Python base image. +""" + +from __future__ import annotations + +import json +import os +import time +from pathlib import Path + +TRACES_DIR = Path(os.environ.get("TRACE_DIR", "/app/traces")) + + +def _attrs(values: dict[str, str]) -> list[dict]: + return [{"key": key, "value": {"stringValue": value}} for key, value in values.items()] + + +def write_trace(session_id: str, instruction: str, answer: str, handler: str) -> Path: + """Write a two-span trace (agent chain + its handler) as OTLP JSONL. + + Args: + session_id: Stable id for this run; becomes the trace's session. + instruction: The task instruction the agent received. + answer: The line the agent produced. + handler: Name of the code path that produced the answer. + + Returns: + Path of the written JSONL file. + """ + now = time.time_ns() + trace_id = f"{now:032x}"[-32:] + root_span_id, child_span_id = f"{now:016x}"[-16:], f"{now + 1:016x}"[-16:] + + document = { + "resourceSpans": [ + { + "resource": {"attributes": _attrs({"service.name": "hello-harbor-agent", "session.id": session_id})}, + "scopeSpans": [ + { + "scope": {"name": "hello-harbor-agent"}, + "spans": [ + { + "traceId": trace_id, + "spanId": root_span_id, + "name": "HelloAgent.solve", + "kind": "SPAN_KIND_INTERNAL", + "startTimeUnixNano": str(now), + "endTimeUnixNano": str(now + 1_000_000), + "attributes": _attrs( + { + "openinference.span.kind": "CHAIN", + "input.value": instruction, + "output.value": answer, + } + ), + "status": {"code": "STATUS_CODE_OK"}, + }, + { + "traceId": trace_id, + "spanId": child_span_id, + "parentSpanId": root_span_id, + "name": handler, + "kind": "SPAN_KIND_INTERNAL", + "startTimeUnixNano": str(now), + "endTimeUnixNano": str(now + 500_000), + "attributes": _attrs( + { + "openinference.span.kind": "TOOL", + "tool.name": handler, + "input.value": instruction, + "output.value": answer, + } + ), + "status": {"code": "STATUS_CODE_OK"}, + }, + ], + } + ], + } + ] + } + + TRACES_DIR.mkdir(parents=True, exist_ok=True) + path = TRACES_DIR / "agent.jsonl" + path.write_text(json.dumps(document, separators=(",", ":")) + "\n", encoding="utf-8") + return path diff --git a/plugins/nemo-experimentalist/examples/tau3-nooa-agent/README.md b/plugins/nemo-experimentalist/examples/tau3-nooa-agent/README.md index ae72feb3a5..e85747acd5 100644 --- a/plugins/nemo-experimentalist/examples/tau3-nooa-agent/README.md +++ b/plugins/nemo-experimentalist/examples/tau3-nooa-agent/README.md @@ -30,3 +30,10 @@ uv run python benchmarks/run.py \ --config benchmarks/configs/tau3-smoke.yaml \ --agent examples/tau3-nooa-agent ``` + +For an optimization run, follow the +[example-agent getting-started guide](../../../../docs/get-started/example-agent.mdx). +It prepares bounded airline train and validation splits before invoking +Experimentalist. The checked-in `experimentalist-smoke.yaml` selects the +SDK-backed Harbor evaluator; set `evaluator_type: harbor_native` to compare the +original evaluator against the same tasks. diff --git a/plugins/nemo-experimentalist/examples/tau3-nooa-agent/experimentalist-smoke.yaml b/plugins/nemo-experimentalist/examples/tau3-nooa-agent/experimentalist-smoke.yaml index 2dfa4f9721..e04e35da43 100644 --- a/plugins/nemo-experimentalist/examples/tau3-nooa-agent/experimentalist-smoke.yaml +++ b/plugins/nemo-experimentalist/examples/tau3-nooa-agent/experimentalist-smoke.yaml @@ -10,6 +10,7 @@ max_train_batch_tasks: 4 train_batch_seed: 20260727 disable_trajectory_scoring: true disable_convergence_check: true +evaluator_type: harbor_evaluator evaluator: n_attempts: 1 n_concurrent_trials: 1 diff --git a/plugins/nemo-experimentalist/examples/tau3-nooa-agent/record_tau_airline_traces.py b/plugins/nemo-experimentalist/examples/tau3-nooa-agent/record_tau_airline_traces.py index 086a5e57aa..d9bf6b9cc9 100644 --- a/plugins/nemo-experimentalist/examples/tau3-nooa-agent/record_tau_airline_traces.py +++ b/plugins/nemo-experimentalist/examples/tau3-nooa-agent/record_tau_airline_traces.py @@ -13,8 +13,8 @@ from pathlib import Path from nemo_experimentalist_plugin.client import make_client -from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor import ( - HarborDataset, +from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor import HarborDataset +from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_native import ( HarborEvaluator, HarborEvaluatorConfig, ) diff --git a/plugins/nemo-experimentalist/pyproject.toml b/plugins/nemo-experimentalist/pyproject.toml index 4b1eb0033e..8ad43bd546 100644 --- a/plugins/nemo-experimentalist/pyproject.toml +++ b/plugins/nemo-experimentalist/pyproject.toml @@ -7,6 +7,7 @@ dependencies = [ "pydantic>=2", "httpx", "harbor>=0.16", + "nemo-evaluator-sdk", "opentelemetry-proto>=1.42.1", "protobuf>=6.0.0", "nooa", @@ -38,3 +39,10 @@ packages = ["src/nemo_experimentalist_plugin"] asyncio_mode = "auto" pythonpath = ["src"] testpaths = ["tests"] +# Mirrors the subset of the repo-root pytest.ini markers this suite uses, so the +# plugin's tests carry the same meaning whichever rootdir pytest resolves. +markers = [ + "e2e: End-to-end tests - test complete customer workflows on deployed infrastructure", + "slow: Tests that take a long time to run", + "skip_in_ci: Tests that should be skipped in CI environment", +] diff --git a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.py b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.py index 25261b4e21..fe552dbfc7 100644 --- a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.py +++ b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.py @@ -5,19 +5,68 @@ from __future__ import annotations +import logging from abc import ABC, abstractmethod from collections.abc import Sequence from pathlib import Path -from typing import Literal, TypeAlias +from typing import Annotated, Any, Literal, TypeAlias from nemo_experimentalist_plugin.experimentalist.components.evaluator.models import ( Dataset, EvaluationResult, TrialResult, ) -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, BeforeValidator, ConfigDict, Field + +logger = logging.getLogger(__name__) + +EvaluatorType: TypeAlias = Literal["harbor_native", "harbor_evaluator"] +"""Selects which evaluator drives the run. + +``harbor_native`` is the default: the plugin builds and runs Harbor's ``Job`` +directly. ``harbor_evaluator`` routes orchestration through the NeMo Evaluator +SDK's ``HarborAgentTaskRunner``, which owns the ``JobConfig``, the success-aware +job-dir cache, and agent import scoping. +""" +_DEPRECATED_EVALUATOR_TYPES: dict[str, EvaluatorType] = {"harbor": "harbor_native"} +"""Retired spellings still accepted on input, mapped to their current name. + +``harbor`` shipped before the rename, so experiment configs in the wild carry it. +``harbor_agent_task_runner`` is deliberately absent: it never shipped, so nothing +can be pinned to it. +""" + +_warned_evaluator_types: set[str] = set() + + +def normalize_evaluator_type(value: Any) -> Any: + """Map a retired evaluator-type spelling onto its current name, warning once. + + Non-strings and unknown strings pass through untouched so pydantic still + produces its own error for a genuinely invalid value, rather than this + function masking it with a confusing one. + + Args: + value(Any): Raw ``evaluator_type`` as supplied by config or a caller. + + Returns: + Any: The canonical evaluator type, or ``value`` unchanged. + """ + replacement = _DEPRECATED_EVALUATOR_TYPES.get(value) if isinstance(value, str) else None + if replacement is None: + return value + if value not in _warned_evaluator_types: + _warned_evaluator_types.add(value) + logger.warning( + "evaluator_type %r is deprecated and will be removed; use %r instead.", + value, + replacement, + ) + return replacement + -EvaluatorType: TypeAlias = Literal["harbor"] +EvaluatorTypeField: TypeAlias = Annotated[EvaluatorType, BeforeValidator(normalize_evaluator_type)] +"""``EvaluatorType`` for config models, accepting the retired spellings on input.""" class EvaluatorConfig(BaseModel): @@ -43,10 +92,13 @@ async def aggregate_results(self, results: Sequence[TrialResult]) -> dict[str, f """ Aggregate evaluation results from multiple runs. - Defaults to averaging each metric across all trials, treating trials that - did not emit a metric (e.g. failed trials) as contributing 0. The denominator - is always ``len(results)``, not the number of trials that reported each metric, - so failure counts against the aggregate score. + Averages each metric over trials with ``status == "completed"``. Anything + else is excluded from both the sum and the denominator, so a crash does not + pull the mean down — it shrinks the sample the mean is taken over, and a + round with no completed trial aggregates to ``{}`` rather than to zeros. + + Completed trials must all report the same metric keys; a mismatch raises + rather than silently averaging over different denominators per metric. Args: results(Sequence[TrialResult]): List of trial results to aggregate. @@ -57,7 +109,10 @@ async def aggregate_results(self, results: Sequence[TrialResult]) -> dict[str, f if not results: return {} - completed = [r for r in results if r.status != "failed"] + # Positive predicate on purpose: `!= "failed"` is equivalent while TrialStatus + # is Literal["completed", "failed"], but it would silently start averaging any + # third status someone adds. Opt statuses in, do not opt "failed" out. + completed = [r for r in results if r.status == "completed"] if not completed: return {} diff --git a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/factory.py b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/factory.py index 3b59226cec..acf1ff6c5e 100644 --- a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/factory.py +++ b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/factory.py @@ -11,15 +11,26 @@ EvaluatorConfig, EvaluatorType, ) -from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor import ( - HarborDataset, +from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor import HarborDataset +from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_evaluator import ( + HarborRunnerConfig, + HarborRunnerEvaluator, +) +from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_native import ( HarborEvaluator, HarborEvaluatorConfig, ) from nemo_experimentalist_plugin.experimentalist.components.evaluator.models import Dataset, DatasetRef, Task -_SUPPORTED_EVALUATOR_TYPES = { - "harbor": (HarborDataset, HarborEvaluator, HarborEvaluatorConfig), +# Both Harbor-backed types read the same Harbor dataset layout; only who drives +# the run differs, so they share ``HarborDataset``. +_SUPPORTED_EVALUATOR_TYPES: dict[EvaluatorType, tuple[type[Dataset], type[Evaluator], type[EvaluatorConfig]]] = { + "harbor_native": (HarborDataset, HarborEvaluator, HarborEvaluatorConfig), + "harbor_evaluator": ( + HarborDataset, + HarborRunnerEvaluator, + HarborRunnerConfig, + ), } @@ -98,7 +109,10 @@ def build_evaluator( if isinstance(config, EvaluatorConfig): config = config.model_dump() elif not isinstance(config, dict): - raise TypeError(f"{evaluator_type.capitalize()} evaluator config must be an EvaluatorConfig or dict") + # Quoted rather than .capitalize()d: these names are snake_case, so + # capitalizing produced "Harbor_native" — and it silently changes + # shape every time a type is renamed. + raise TypeError(f"{evaluator_type!r} evaluator config must be an EvaluatorConfig or dict") evaluator_config = _SUPPORTED_EVALUATOR_TYPES[evaluator_type][2].model_validate(config) return _SUPPORTED_EVALUATOR_TYPES[evaluator_type][1]( options=evaluator_config, experiment_dir=experiment_dir diff --git a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py index 24a60ffa37..371978f00b 100644 --- a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py +++ b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py @@ -1,42 +1,33 @@ # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 -"""Harbor dataset adapter for evaluator-domain task objects.""" +"""Shared Harbor dataset, dependency, input, and result adapters.""" from __future__ import annotations import asyncio import hashlib -import importlib.machinery import json import logging import os import re import shutil -import sys import tempfile import tomllib from collections.abc import Sequence from dataclasses import dataclass from pathlib import Path -from types import ModuleType, TracebackType -from typing import Any, TypeAlias, TypedDict +from types import TracebackType +from typing import Any, Protocol, TypeAlias, TypedDict from uuid import uuid4 from harbor.constants import MAIN_SERVICE_NAME from harbor.environments.base import BaseEnvironment from harbor.environments.factory import EnvironmentFactory -from harbor.job import DatasetConfig, Job, JobConfig from harbor.models.environment_type import EnvironmentType -from harbor.models.job.config import AgentConfig, ArtifactConfig, RetryConfig from harbor.models.task.task import Task as HarborTaskModel from harbor.models.trial.config import ServiceVolumeConfig from harbor.models.trial.paths import EnvironmentPaths, TrialPaths -from nemo_experimentalist_plugin.experimentalist.components.evaluator.base import ( - Evaluator, - EvaluatorConfig, - EvaluatorType, -) from nemo_experimentalist_plugin.experimentalist.components.evaluator.models import ( Dataset, DatasetRef, @@ -52,7 +43,6 @@ run_dependency_command, subset_dataset_id, ) -from pydantic import Field class HarborResourceSpec(TypedDict): @@ -110,11 +100,8 @@ class HarborResourceSpec(TypedDict): (_ORACLE_DIRNAME, "oracle_dir", ()), (_STEPS_DIRNAME, "steps_dir", ()), ) -_TRACE_ARTIFACT_SOURCE = "/app/traces" -_TRACE_ARTIFACT_DESTINATION = "traces" +DEFAULT_TRACE_ARTIFACT_SOURCE = "/app/traces" _SHELL_SYNTAX_TIMEOUT_SEC = 10.0 -_AGENT_IMPORT_ROOT = "_nemo_experimentalist_eval_agents" -_IDENTIFIER_RE = re.compile(r"\W+") _TRIAL_LOG_DESCRIPTIONS = { "agent/oracle.txt": "Oracle-agent log captured when Harbor runs the reference solution.", "agent/setup/stdout.txt": "Agent setup stdout captured while Harbor uploads the agent and installs dependencies.", @@ -129,6 +116,22 @@ class HarborResourceSpec(TypedDict): logger = logging.getLogger(__name__) +def __getattr__(name: str) -> Any: + """Lazily preserve the former native-evaluator import path.""" + if name not in {"HarborEvaluator", "HarborEvaluatorConfig"}: + raise AttributeError(f"module {__name__!r} has no attribute {name!r}") + + from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_native import ( + HarborEvaluator, + HarborEvaluatorConfig, + ) + + return { + "HarborEvaluator": HarborEvaluator, + "HarborEvaluatorConfig": HarborEvaluatorConfig, + }[name] + + @dataclass(frozen=True) class HarborVerifierValidationFailure: """Syntax failure found in one task's Harbor verifier.""" @@ -167,29 +170,6 @@ class _VerifierSyntaxFailure: column: int | None = None -class HarborEvaluatorConfig(EvaluatorConfig): - """Configuration for Harbor evaluator.""" - - job_name: str | None = Field( - default=None, description="Name of the job to run. If not provided, a default name will be generated." - ) - jobs_dir: Path = Field( - default=Path("eval-and-optimize") / "results", - description="Directory to store job results, resolved relative to the experiment directory.", - ) - n_attempts: int = Field(default=1) - n_concurrent_trials: int = Field(default=os.cpu_count() or 4) - quiet: bool = Field(default=False) - verifier_timeout_multiplier: float | None = Field(default=1.0) - agent_timeout_multiplier: float | None = Field(default=1.0) - agent_setup_timeout_multiplier: float | None = Field(default=1.0) - environment_build_timeout_multiplier: float | None = Field(default=1.0) - artifacts: list[str] = Field(default=[]) - retry: RetryConfig = Field(default=RetryConfig(exclude_exceptions=set())) - import_path: str = Field(default="harbor_wrapper:WrappedAgent") - trace_dir: str = Field(default=_TRACE_ARTIFACT_SOURCE) - - class HarborDependencyRuntime(DependencyRuntime): """Harbor API-backed task environment runtime.""" @@ -348,76 +328,6 @@ def _chmod_path_chain(path: Path, stop_at: Path) -> None: HarborDataValue: TypeAlias = DataValue | ResourceRef -def _safe_identifier(value: str) -> str: - identifier = _IDENTIFIER_RE.sub("_", value).strip("_") - if not identifier: - identifier = "path" - if not identifier[0].isalpha() and identifier[0] != "_": - identifier = f"_{identifier}" - return identifier - - -def _agent_import_package(agent_path: Path) -> str: - path_parts = [_safe_identifier(part) for part in agent_path.parts if part not in {"", agent_path.anchor}] - tail = path_parts[-6:] or ["agent"] - digest = hashlib.sha256(str(agent_path).encode("utf-8")).hexdigest()[:12] - tail[-1] = f"{tail[-1]}_{digest}" - return ".".join([_AGENT_IMPORT_ROOT, *tail]) - - -def _ensure_package(name: str, search_path: Path | None = None) -> None: - parts = name.split(".") - for idx in range(1, len(parts) + 1): - package_name = ".".join(parts[:idx]) - package = sys.modules.get(package_name) - if package is None: - package = ModuleType(package_name) - package.__package__ = package_name - package.__spec__ = importlib.machinery.ModuleSpec(package_name, loader=None, is_package=True) - package.__path__ = [] # type: ignore[attr-defined] - sys.modules[package_name] = package - if idx > 1: - parent_name = ".".join(parts[: idx - 1]) - setattr(sys.modules[parent_name], parts[idx - 1], package) - if search_path is not None and idx == len(parts): - package.__path__ = [str(search_path)] # type: ignore[attr-defined] - - -def _scoped_import_path(agent_path: Path, import_path: str) -> tuple[str, str]: - module_name, separator, attribute = import_path.partition(":") - module_name = module_name.strip().lstrip(".") - if not module_name: - raise ValueError("import_path module is required") - - package_name = _agent_import_package(agent_path) - _ensure_package(package_name, search_path=agent_path) - scoped = f"{package_name}.{module_name}" - if separator: - scoped = f"{scoped}:{attribute}" - return scoped, package_name - - -def _cleanup_scoped_imports(package_name: str) -> None: - package = sys.modules.get(package_name) - for module_name in list(sys.modules): - if module_name == package_name or module_name.startswith(f"{package_name}."): - sys.modules.pop(module_name, None) - parent_name, _, child_name = package_name.rpartition(".") - parent = sys.modules.get(parent_name) - if parent is not None and getattr(parent, child_name, None) is package: - delattr(parent, child_name) - parts = package_name.split(".") - for idx in range(len(parts) - 1, 0, -1): - module_name = ".".join(parts[:idx]) - if any(name.startswith(f"{module_name}.") for name in sys.modules): - break - package = sys.modules.pop(module_name, None) - parent_name, _, child_name = module_name.rpartition(".") - parent = sys.modules.get(parent_name) - if parent is not None and getattr(parent, child_name, None) is package: - delattr(parent, child_name) - - def _resolve_verifier_dir(task_dir: Path, config: dict[str, Any]) -> Path: verifier_config = config.get("verifier") if isinstance(verifier_config, dict): @@ -580,18 +490,6 @@ def _is_trial_log_path(relative_path: str) -> bool: return len(parts) == 3 and parts[0] == "agent" and parts[1].startswith("command-") and parts[2] == "stdout.txt" -def _with_trace_artifact(artifacts: Sequence[str | ArtifactConfig], trace_source: str) -> list[str | ArtifactConfig]: - for artifact in artifacts: - if isinstance(artifact, ArtifactConfig): - if artifact.source == trace_source or artifact.destination == _TRACE_ARTIFACT_DESTINATION: - return list(artifacts) - elif isinstance(artifact, str) and artifact in {trace_source, _TRACE_ARTIFACT_DESTINATION}: - return list(artifacts) - - trace_artifact = ArtifactConfig(source=trace_source, destination=_TRACE_ARTIFACT_DESTINATION) - return [trace_artifact, *artifacts] - - def _trial_error(exception_info: Any) -> dict[str, DataValue] | None: if exception_info is None: return None @@ -712,6 +610,166 @@ def _trial_resources(trial_dir: Path) -> tuple[dict[str, ResourceRef], ResourceR return resources, trace_ref +class HarborJobOptions(Protocol): + """The two fields any Harbor-backed evaluator config must supply to locate a run. + + Declared structurally so :func:`resolve_harbor_run_inputs` can serve both + evaluator configs without importing either — the SDK-backed one lives in a + module that already imports this one. + """ + + jobs_dir: Path + job_name: str | None + + +@dataclass(frozen=True) +class HarborRunInputs: + """Validated inputs both Harbor-backed evaluators resolve the same way. + + ``dataset`` is carried through already narrowed to :class:`HarborDataset` so + callers need no second ``isinstance`` check to satisfy a type checker — the + validation happened once, in :func:`resolve_harbor_run_inputs`. + """ + + dataset: HarborDataset + dataset_path: Path + agent_path: Path + jobs_dir: Path + job_name: str + + @property + def job_dir(self) -> Path: + """Directory Harbor writes this run's per-trial results into.""" + return self.jobs_dir / self.job_name + + +async def resolve_harbor_run_inputs( + agent: Path, + dataset: Dataset, + options: HarborJobOptions, + experiment_dir: Path | None, +) -> HarborRunInputs: + """Validate an evaluation request and resolve the paths Harbor needs. + + The counterpart to :func:`trials_from_job_dir`: that one owns reading results + back, this one owns getting in. Both evaluator types must agree on what "the + same inputs" means — if one tightened its agent-path check or moved the + verifier preflight, the A/B parity tests would still pass while the two + silently diverged. Keeping the entry symmetric with the exit is what stops that. + + Verifier syntax is validated here, before any caller starts Docker: a typo in + ``tests/test.sh`` is far cheaper to catch now than after an image build. + + Args: + agent: Candidate directory to evaluate. + dataset: Must be a :class:`HarborDataset` with a resolvable source. + options: Evaluator options supplying ``jobs_dir`` and optional ``job_name``. + experiment_dir: Experiment root that ``jobs_dir`` resolves against; the + current working directory when ``None``. + + Returns: + HarborRunInputs: Resolved dataset/agent paths and the run's job location. + + Raises: + ValueError: If the dataset is not a Harbor dataset or has no source. + FileNotFoundError: If the agent directory does not exist. + DatasetValidationError: If a selected task's verifier fails preflight. + """ + if not isinstance(dataset, HarborDataset): + raise ValueError("Dataset must be a Harbor dataset") + if dataset.source is None: + raise ValueError("Harbor dataset source is required") + + agent_path = agent.expanduser().resolve() + if not agent_path.is_dir(): + raise FileNotFoundError(f"Harbor agent path not found: {agent_path}") + + await dataset.validate() + + return HarborRunInputs( + dataset=dataset, + dataset_path=local_path_from_uri(dataset.source.uri, context="Harbor dataset reference").resolve(), + agent_path=agent_path, + jobs_dir=(experiment_dir or Path.cwd()) / options.jobs_dir, + # Derived from the *resolved* directory, not the caller's spelling of it. + # `job_name` is the cache identity, and the SDK's scoped agent import derives + # its package name from the resolved dir too — the two must agree or a job dir + # can be reused for a different agent (`--agent .` has an empty `.name`; a + # symlink keeps its own name while resolving elsewhere). + job_name=options.job_name or f"{agent_path.name}-{dataset.id}", + ) + + +def trials_from_job_dir(job_dir: Path, tasks: Sequence[Task]) -> list[TrialResult]: + """Adapt a finished Harbor job directory into evaluator-domain trial results. + + The job directory is the authoritative source for both Harbor-backed + evaluators: it carries every verifier metric (not just the primary reward), + the attempt index, the trial's error shape, and the on-disk trace and + artifact references that the Analyzer and the Coder read. Whoever + orchestrated the run — Harbor's ``Job`` directly or the SDK's + ``HarborAgentTaskRunner`` — writes the same tree, so both evaluators share + this adapter and produce equivalent :class:`TrialResult` objects. + + Args: + job_dir: Harbor job directory holding one ``/result.json`` per attempt. + tasks: Dataset tasks the run was asked to cover, used to resolve each + trial back to its short Experimentalist task id and metric spec. + + Returns: + list[TrialResult]: One result per trial directory that wrote a ``result.json``. + + Raises: + FileNotFoundError: If the job directory does not exist. Returning no trials + would be aggregated as an empty-but-valid result and read as a run that + legitimately scored nothing, so an orchestrator that produced no job + directory at all is surfaced instead of swallowed. + """ + if not job_dir.is_dir(): + raise FileNotFoundError( + f"Harbor job directory not found: {job_dir}. The run produced no results — " + "check the orchestrator's logs for a job that failed before writing any trial." + ) + + task_map = {task.id: task for task in tasks} + trials: list[TrialResult] = [] + for trial_dir in sorted(path for path in job_dir.iterdir() if path.is_dir()): + result_path = trial_dir / "result.json" + if not result_path.is_file(): + continue + + trial_data = json.loads(result_path.read_text(encoding="utf-8")) + trial_id = trial_data.get("trial_name") + if not isinstance(trial_id, str) or not trial_id: + trial_id = trial_dir.name + + task_id = _resolve_trial_task_id(trial_id, trial_data, task_map) + task = task_map.get(task_id) + # Prefer the dataset's own spec, but only when it points at a verifier; + # a ref-less spec carries no more than the one derived from the trial dir. + metric_spec = task.metric_specs.get("reward") if task is not None else None + if metric_spec is None or metric_spec.ref is None: + metric_spec = _trial_metric_spec(trial_dir, trial_data) + + exception_info = trial_data.get("exception_info") + resources, trace = _trial_resources(trial_dir) + + trials.append( + TrialResult( + id=trial_id, + task_id=task_id, + attempt=_trial_attempt(trial_id), + status="completed" if exception_info is None else "failed", + error=_trial_error(exception_info), + trace=trace, + outputs={}, + resources=resources, + metrics=_trial_metrics(trial_dir, trial_data, metric_spec), + ) + ) + return trials + + class HarborDataset(Dataset): """Harbor task collection mapped onto generic evaluator-domain objects. @@ -1237,92 +1295,3 @@ def subset(self, task_ids: Sequence[str]) -> HarborDataset: tasks=tasks, metadata=dict(self.metadata), ) - - -class HarborEvaluator(Evaluator): - """Run Harbor evaluations and return parsed reward payloads.""" - - evaluator_type: EvaluatorType = "harbor" - - def __init__(self, options: HarborEvaluatorConfig | None = None, experiment_dir: Path | None = None) -> None: - super().__init__(options or HarborEvaluatorConfig(), experiment_dir=experiment_dir) - - async def _run(self, agent: Path, dataset: Dataset, options: HarborEvaluatorConfig) -> Sequence[TrialResult]: - if not isinstance(dataset, HarborDataset): - raise ValueError("Dataset must be a Harbor dataset") - - if dataset.source is None: - raise ValueError("Harbor dataset source is required") - dataset_path = local_path_from_uri(dataset.source.uri, context="Harbor dataset reference").resolve() - - options_dict = options.model_dump() - experiment_dir = self.experiment_dir or Path.cwd() - options_dict["jobs_dir"] = experiment_dir / options.jobs_dir - options_dict["job_name"] = options.job_name or f"{agent.name}-{dataset.id}" - import_path: str = options_dict.pop("import_path") - trace_dir: str = options_dict.pop("trace_dir", _TRACE_ARTIFACT_SOURCE) - options_dict["artifacts"] = _with_trace_artifact(options_dict.get("artifacts") or [], trace_dir) - force_rerun: bool = options_dict.pop("force_rerun", False) - - agent_path = agent.expanduser().resolve() - - if not agent_path.is_dir(): - raise FileNotFoundError(f"Harbor agent path not found: {agent_path}") - - await dataset.validate() - - scoped_import_path, scoped_package = _scoped_import_path(agent_path, import_path) - agents_config = [AgentConfig(import_path=scoped_import_path)] - datasets_config = [DatasetConfig(path=dataset_path, task_names=[task.id for task in dataset.tasks])] - job_config = JobConfig(**options_dict, agents=agents_config, datasets=datasets_config) - if force_rerun: - job_dir = job_config.jobs_dir / job_config.job_name - if job_dir.exists(): - shutil.rmtree(job_dir) - - try: - job = await Job.create(job_config) - await job.run() - finally: - _cleanup_scoped_imports(scoped_package) - - trials = await self._trials_from_dir(job.job_dir, dataset.tasks) - return trials - - async def _trials_from_dir(self, job_dir: Path, tasks: Sequence[Task]) -> Sequence[TrialResult]: - task_map = {task.id: task for task in tasks} - trials: list[TrialResult] = [] - for trial_dir in sorted(path for path in job_dir.iterdir() if path.is_dir()): - result_path = trial_dir / "result.json" - if not result_path.is_file(): - continue - - trial_data = json.loads(result_path.read_text(encoding="utf-8")) - trial_id = trial_data.get("trial_name") - if not isinstance(trial_id, str) or not trial_id: - trial_id = trial_dir.name - - task_id = _resolve_trial_task_id(trial_id, trial_data, task_map) - task = task_map.get(task_id) - metric_spec = task.metric_specs["reward"] if task is not None and "reward" in task.metric_specs else None - if metric_spec is not None and metric_spec.ref is None: - metric_spec = None - metric_spec = metric_spec or _trial_metric_spec(trial_dir, trial_data) - - exception_info = trial_data.get("exception_info") - resources, trace = _trial_resources(trial_dir) - - trials.append( - TrialResult( - id=trial_id, - task_id=task_id, - attempt=_trial_attempt(trial_id), - status="completed" if exception_info is None else "failed", - error=_trial_error(exception_info), - trace=trace, - outputs={}, - resources=resources, - metrics=_trial_metrics(trial_dir, trial_data, metric_spec), - ) - ) - return trials diff --git a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_evaluator.py b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_evaluator.py new file mode 100644 index 0000000000..4b6e0742ef --- /dev/null +++ b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_evaluator.py @@ -0,0 +1,252 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Harbor evaluator that delegates orchestration to the NeMo Evaluator SDK. + +``HarborEvaluator`` builds Harbor's ``JobConfig`` and drives ``Job`` itself. This +evaluator hands that job to the SDK's ``HarborAgentTaskRunner`` instead: the SDK +owns the ``JobConfig``, the success-aware job-directory cache, and the scoped +agent import. Harbor still does the work underneath — the difference is who owns +the orchestration. + +Results are read back off the Harbor job directory through the same +:func:`~...evaluator.harbor.trials_from_job_dir` adapter ``harbor_native`` uses. +**That sharing is the point**: one adapter over one source of truth is what makes +the two evaluator types produce equivalent :class:`TrialResult` objects, rather +than two parsers that have to be kept in agreement. + +Scoring is deliberately left to Harbor. Its verifier already computes the rewards +and writes them to ``/result.json``; the SDK's metric layer only reads them +back, so running the trials through ``AgentEvaluator`` would add a scoring pass +whose output this evaluator discards. When ``harbor_native`` is eventually removed, +the natural move is to consume ``AgentEvalTrial`` directly — see +``temp/evaluator/plans/AgentEvalResult_as_shared_harbor_interface.md``. +""" + +from __future__ import annotations + +import logging +import os +from collections.abc import Sequence +from pathlib import Path + +from nemo_evaluator_sdk.agent_eval.runtimes.harbor_runtime import ( + HarborAgentTaskRunner, + HarborRuntimeConfig, + discover_harbor_tasks, +) +from nemo_evaluator_sdk.agent_eval.tasks import AgentEvalTask +from nemo_experimentalist_plugin.experimentalist.components.evaluator.base import ( + Evaluator, + EvaluatorConfig, + EvaluatorType, +) +from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor import ( + DEFAULT_TRACE_ARTIFACT_SOURCE, + HarborDataset, + resolve_harbor_run_inputs, + trials_from_job_dir, +) +from nemo_experimentalist_plugin.experimentalist.components.evaluator.models import ( + Dataset, + TrialResult, + local_path_from_uri, +) +from pydantic import ConfigDict, Field + +logger = logging.getLogger(__name__) + + +class HarborTaskNameError(ValueError): + """A dataset task could not be mapped onto exactly one Harbor task name.""" + + +class HarborRunnerConfig(EvaluatorConfig): + """Configuration for the SDK-backed Harbor evaluator. + + Every field maps onto exactly one ``HarborRuntimeConfig`` field. Unknown keys + are rejected rather than silently ignored: several plain-``HarborEvaluator`` + options (notably the full ``retry`` model) have no unambiguous SDK equivalent, + so passing them here is a configuration error, not a no-op. + + ``agent_dir`` is deliberately absent — it is always derived from the candidate + being evaluated, so a config cannot point the run at a different agent. + + One asymmetry to know when A/B-ing against ``HarborEvaluatorConfig``: at their + defaults the two are equivalent (Harbor resolves an unset phase multiplier to + the global ``timeout_multiplier``, which defaults to ``1.0``), but they diverge + once tuned. This config exposes the global ``timeout_multiplier`` and leaves the + phase multipliers unset so they inherit it; the plain config has no global knob + and pins each phase to ``1.0``, which masks it. Set the phase multipliers + explicitly on both sides when comparing non-default timeouts. + """ + + model_config = ConfigDict(extra="forbid") + + job_name: str | None = Field( + default=None, + description=( + "Harbor job name. Defaults to the loop's deterministic '-', " + "which is what makes the SDK's success-aware job-dir cache usable." + ), + ) + jobs_dir: Path = Field( + default=Path("eval-and-optimize") / "results", + description="Directory to store job results, resolved relative to the experiment directory.", + ) + n_attempts: int = Field(default=1, ge=1, description="Number of attempts Harbor runs per task.") + n_concurrent_trials: int = Field( + default=os.cpu_count() or 4, ge=1, description="Maximum number of concurrent Harbor trials." + ) + quiet: bool = Field(default=False, description="Suppress Harbor's trial progress display.") + artifacts: list[str] = Field(default=[], description="Additional Harbor artifact sources to collect per trial.") + trace_dir: str = Field( + default=DEFAULT_TRACE_ARTIFACT_SOURCE, + description="Container path of agent traces, collected into the trial's 'traces' artifact directory.", + ) + max_retries: int = Field(default=0, ge=0, description="Harbor per-trial retries on transient failures.") + timeout_multiplier: float | None = Field(default=None, description="Global Harbor timeout multiplier.") + agent_timeout_multiplier: float | None = Field(default=None, description="Agent-phase timeout multiplier.") + verifier_timeout_multiplier: float | None = Field(default=None, description="Verifier-phase timeout multiplier.") + agent_setup_timeout_multiplier: float | None = Field(default=None, description="Agent-setup timeout multiplier.") + environment_build_timeout_multiplier: float | None = Field( + default=None, description="Environment-build timeout multiplier." + ) + import_path: str = Field( + default="harbor_wrapper:WrappedAgent", + description="Harbor agent import path resolved inside the candidate directory.", + ) + + +class HarborRunnerEvaluator(Evaluator): + """Run Harbor through the SDK's ``HarborAgentTaskRunner`` and parse the job dir.""" + + evaluator_type: EvaluatorType = "harbor_evaluator" + + def __init__( + self, + options: HarborRunnerConfig | None = None, + experiment_dir: Path | None = None, + ) -> None: + super().__init__(options or HarborRunnerConfig(), experiment_dir=experiment_dir) + + async def _run( + self, + agent: Path, + dataset: Dataset, + options: EvaluatorConfig, + ) -> Sequence[TrialResult]: + if not isinstance(options, HarborRunnerConfig): + raise TypeError("Options must be a HarborRunnerConfig") + + inputs = await resolve_harbor_run_inputs(agent, dataset, options, self.experiment_dir) + harbor_dataset = inputs.dataset + sdk_tasks = _sdk_tasks_for(harbor_dataset) + + runtime_config = HarborRuntimeConfig( + jobs_dir=inputs.jobs_dir, + job_name=inputs.job_name, + agent_import_path=options.import_path, + agent_dir=inputs.agent_path, + n_attempts=options.n_attempts, + n_concurrent_trials=options.n_concurrent_trials, + quiet=options.quiet, + force_rerun=options.force_rerun, + artifacts=list(options.artifacts), + trace_dir=options.trace_dir, + max_retries=options.max_retries, + timeout_multiplier=options.timeout_multiplier, + agent_timeout_multiplier=options.agent_timeout_multiplier, + verifier_timeout_multiplier=options.verifier_timeout_multiplier, + agent_setup_timeout_multiplier=options.agent_setup_timeout_multiplier, + environment_build_timeout_multiplier=options.environment_build_timeout_multiplier, + ) + + # Two different name spaces, and mixing them up produces either an empty + # run or an empty cache: + # * Harbor's local-dataset `task_names` filter matches the task + # *directory* name, which is the Experimentalist task id. + # * `result.json` records `[task].name` from task.toml, which is what + # the SDK's tasks are keyed by and what its cache counts. + runner = HarborAgentTaskRunner( + config=runtime_config, + dataset_path=inputs.dataset_path, + task_names=[task.id for task in harbor_dataset.tasks], + ) + # Called for its effect — running (or resuming) the Harbor job. The returned + # trials are not the contract: the job dir is, and it is shared with + # `harbor_native`, which is what keeps the two types equivalent. Note the + # trials are not *lossy* — `metadata["reward_details"]` carries every verifier + # reward — so the reason to ignore them is the shared source of truth, not + # missing data. + sdk_trials = await runner.run_tasks(list(sdk_tasks.values())) + logger.debug("SDK Harbor runner returned %d trial(s) for job %s", len(sdk_trials), inputs.job_name) + + return trials_from_job_dir(inputs.job_dir, harbor_dataset.tasks) + + +def _sdk_tasks_for(dataset: HarborDataset) -> dict[str, AgentEvalTask]: + """Map each selected dataset task onto the SDK task carrying its full Harbor name. + + The mapping is by task *directory*, never by name similarity: the SDK reads + ``[task].name`` from the same ``task.toml`` Harbor will read, so matching on + the directory guarantees the ids we hand the runner are exactly the + ``task_name`` values Harbor writes into ``result.json``. + + Args: + dataset: The (possibly subset) Harbor dataset being evaluated. + + Returns: + dict[str, AgentEvalTask]: Selected tasks keyed by Experimentalist task id, + in dataset order. + + Raises: + ValueError: If the dataset has no resolvable source directory. + HarborTaskNameError: If a selected task has no discovered counterpart, or + if two selected tasks resolve to the same full Harbor name. + """ + if dataset.source is None: + raise ValueError("Harbor dataset source is required") + dataset_path = local_path_from_uri(dataset.source.uri, context="Harbor dataset reference").resolve() + discovered = discover_harbor_tasks(dataset_path) + by_dir: dict[Path, AgentEvalTask] = {} + for sdk_task in discovered: + task_dir = sdk_task.metadata.get("harbor_task_dir") + if isinstance(task_dir, str) and task_dir: + by_dir[Path(task_dir).resolve()] = sdk_task + + selected: dict[str, AgentEvalTask] = {} + full_names: dict[str, str] = {} + for task in dataset.tasks: + if not task.uri: + raise HarborTaskNameError(f"Harbor task {task.id!r} has no URI, so its Harbor name cannot be resolved") + task_dir = local_path_from_uri(task.uri, context="Harbor task reference").resolve() + sdk_task = by_dir.get(task_dir) + if sdk_task is None: + raise HarborTaskNameError( + f"Harbor task {task.id!r} at {task_dir} was not discovered under dataset {dataset_path}; " + "the dataset directory and the task directories must agree" + ) + if sdk_task.id in full_names: + raise HarborTaskNameError( + f"Harbor tasks {full_names[sdk_task.id]!r} and {task.id!r} both declare [task].name = " + f"{sdk_task.id!r}; task names must be unique within a dataset" + ) + full_names[sdk_task.id] = task.id + selected[task.id] = sdk_task + return selected + + +def harbor_task_names(dataset: HarborDataset) -> dict[str, str]: + """Return ``{experimentalist_task_id: full_harbor_name}`` for a Harbor dataset. + + The readable view of the two-namespace translation ``_run`` depends on. Pass a + ``dataset.subset(...)`` to scope it to selected tasks. + + Args: + dataset: Harbor dataset whose source directory holds the tasks. + + Returns: + dict[str, str]: Short Experimentalist task id to full Harbor ``[task].name``. + """ + return {task_id: sdk_task.id for task_id, sdk_task in _sdk_tasks_for(dataset).items()} diff --git a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_native.py b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_native.py new file mode 100644 index 0000000000..749a906976 --- /dev/null +++ b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_native.py @@ -0,0 +1,193 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Direct Harbor evaluator orchestration.""" + +import hashlib +import importlib.machinery +import os +import re +import shutil +import sys +from collections.abc import Sequence +from pathlib import Path +from types import ModuleType + +from harbor.job import DatasetConfig, Job, JobConfig +from harbor.models.job.config import AgentConfig, ArtifactConfig, RetryConfig +from nemo_experimentalist_plugin.experimentalist.components.evaluator.base import ( + Evaluator, + EvaluatorConfig, + EvaluatorType, +) +from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor import ( + DEFAULT_TRACE_ARTIFACT_SOURCE, + resolve_harbor_run_inputs, + trials_from_job_dir, +) +from nemo_experimentalist_plugin.experimentalist.components.evaluator.models import ( + Dataset, + Task, + TrialResult, +) +from pydantic import Field + +_TRACE_ARTIFACT_DESTINATION = "traces" +_AGENT_IMPORT_ROOT = "_nemo_experimentalist_eval_agents" +_IDENTIFIER_RE = re.compile(r"\W+") + + +def _safe_identifier(value: str) -> str: + identifier = _IDENTIFIER_RE.sub("_", value).strip("_") + if not identifier: + identifier = "path" + if not identifier[0].isalpha() and identifier[0] != "_": + identifier = f"_{identifier}" + return identifier + + +def _agent_import_package(agent_path: Path) -> str: + path_parts = [_safe_identifier(part) for part in agent_path.parts if part not in {"", agent_path.anchor}] + tail = path_parts[-6:] or ["agent"] + digest = hashlib.sha256(str(agent_path).encode("utf-8")).hexdigest()[:12] + tail[-1] = f"{tail[-1]}_{digest}" + return ".".join([_AGENT_IMPORT_ROOT, *tail]) + + +def _ensure_package(name: str, search_path: Path | None = None) -> None: + parts = name.split(".") + for idx in range(1, len(parts) + 1): + package_name = ".".join(parts[:idx]) + package = sys.modules.get(package_name) + if package is None: + package = ModuleType(package_name) + package.__package__ = package_name + package.__spec__ = importlib.machinery.ModuleSpec(package_name, loader=None, is_package=True) + package.__path__ = [] # type: ignore[attr-defined] + sys.modules[package_name] = package + if idx > 1: + parent_name = ".".join(parts[: idx - 1]) + setattr(sys.modules[parent_name], parts[idx - 1], package) + if search_path is not None and idx == len(parts): + package.__path__ = [str(search_path)] # type: ignore[attr-defined] + + +def _scoped_import_path(agent_path: Path, import_path: str) -> tuple[str, str]: + module_name, separator, attribute = import_path.partition(":") + module_name = module_name.strip().lstrip(".") + if not module_name: + raise ValueError("import_path module is required") + + package_name = _agent_import_package(agent_path) + _ensure_package(package_name, search_path=agent_path) + scoped = f"{package_name}.{module_name}" + if separator: + scoped = f"{scoped}:{attribute}" + return scoped, package_name + + +def _cleanup_scoped_imports(package_name: str) -> None: + package = sys.modules.get(package_name) + for module_name in list(sys.modules): + if module_name == package_name or module_name.startswith(f"{package_name}."): + sys.modules.pop(module_name, None) + parent_name, _, child_name = package_name.rpartition(".") + parent = sys.modules.get(parent_name) + if parent is not None and getattr(parent, child_name, None) is package: + delattr(parent, child_name) + parts = package_name.split(".") + for idx in range(len(parts) - 1, 0, -1): + module_name = ".".join(parts[:idx]) + if any(name.startswith(f"{module_name}.") for name in sys.modules): + break + package = sys.modules.pop(module_name, None) + parent_name, _, child_name = module_name.rpartition(".") + parent = sys.modules.get(parent_name) + if parent is not None and getattr(parent, child_name, None) is package: + delattr(parent, child_name) + + +def _with_trace_artifact(artifacts: Sequence[str | ArtifactConfig], trace_source: str) -> list[str | ArtifactConfig]: + for artifact in artifacts: + if isinstance(artifact, ArtifactConfig): + if artifact.source == trace_source or artifact.destination == _TRACE_ARTIFACT_DESTINATION: + return list(artifacts) + elif isinstance(artifact, str) and artifact in {trace_source, _TRACE_ARTIFACT_DESTINATION}: + return list(artifacts) + + trace_artifact = ArtifactConfig(source=trace_source, destination=_TRACE_ARTIFACT_DESTINATION) + return [trace_artifact, *artifacts] + + +class HarborEvaluatorConfig(EvaluatorConfig): + """Configuration for direct Harbor evaluation.""" + + job_name: str | None = Field( + default=None, description="Name of the job to run. If not provided, a default name will be generated." + ) + jobs_dir: Path = Field( + default=Path("eval-and-optimize") / "results", + description="Directory to store job results, resolved relative to the experiment directory.", + ) + n_attempts: int = Field(default=1) + n_concurrent_trials: int = Field(default=os.cpu_count() or 4) + quiet: bool = Field(default=False) + verifier_timeout_multiplier: float | None = Field(default=1.0) + agent_timeout_multiplier: float | None = Field(default=1.0) + agent_setup_timeout_multiplier: float | None = Field(default=1.0) + environment_build_timeout_multiplier: float | None = Field(default=1.0) + artifacts: list[str] = Field(default=[]) + retry: RetryConfig = Field(default=RetryConfig(exclude_exceptions=set())) + import_path: str = Field(default="harbor_wrapper:WrappedAgent") + trace_dir: str = Field(default=DEFAULT_TRACE_ARTIFACT_SOURCE) + + +class HarborEvaluator(Evaluator): + """Run Harbor evaluations directly and return parsed reward payloads.""" + + evaluator_type: EvaluatorType = "harbor_native" + + def __init__(self, options: HarborEvaluatorConfig | None = None, experiment_dir: Path | None = None) -> None: + super().__init__(options or HarborEvaluatorConfig(), experiment_dir=experiment_dir) + + async def _run(self, agent: Path, dataset: Dataset, options: EvaluatorConfig) -> Sequence[TrialResult]: + # Widened from HarborEvaluatorConfig to match the base class contract: + # Evaluator.run() passes an EvaluatorConfig instance through unchanged, so + # narrowing here would be an unsound override. The guard is defensive only — + # both the factory and the loop build this config via type(self.options). + if not isinstance(options, HarborEvaluatorConfig): + raise TypeError("Options must be a HarborEvaluatorConfig") + + inputs = await resolve_harbor_run_inputs(agent, dataset, options, self.experiment_dir) + harbor_dataset = inputs.dataset + dataset_path = inputs.dataset_path + agent_path = inputs.agent_path + + options_dict = options.model_dump() + options_dict["jobs_dir"] = inputs.jobs_dir + options_dict["job_name"] = inputs.job_name + import_path: str = options_dict.pop("import_path") + trace_dir: str = options_dict.pop("trace_dir", DEFAULT_TRACE_ARTIFACT_SOURCE) + options_dict["artifacts"] = _with_trace_artifact(options_dict.get("artifacts") or [], trace_dir) + force_rerun: bool = options_dict.pop("force_rerun", False) + + scoped_import_path, scoped_package = _scoped_import_path(agent_path, import_path) + agents_config = [AgentConfig(import_path=scoped_import_path)] + datasets_config = [DatasetConfig(path=dataset_path, task_names=[task.id for task in harbor_dataset.tasks])] + job_config = JobConfig(**options_dict, agents=agents_config, datasets=datasets_config) + if force_rerun: + job_dir = job_config.jobs_dir / job_config.job_name + if job_dir.exists(): + shutil.rmtree(job_dir) + + try: + job = await Job.create(job_config) + await job.run() + finally: + _cleanup_scoped_imports(scoped_package) + + trials = await self._trials_from_dir(job.job_dir, harbor_dataset.tasks) + return trials + + async def _trials_from_dir(self, job_dir: Path, tasks: Sequence[Task]) -> Sequence[TrialResult]: + return trials_from_job_dir(job_dir, tasks) diff --git a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/deps.py b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/deps.py index eaa3d805bb..2f376696fe 100644 --- a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/deps.py +++ b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/deps.py @@ -40,6 +40,9 @@ class ExperimentalistDeps(BaseModel): A :class:`~pathlib.Path` means a local directory; a plain string means a fileset ID that the backend will resolve at evaluation time. Defaults to None and must be set before ``run()``. + evaluator_type: Which evaluator drives the run. ``run_experimentalist`` + passes the value resolved from ``EvolutionaryOptimizerConfig``; the + default here only applies to callers that construct deps directly. backend: Shared data-access backend used by every tool. The CLI owns the backend's client lifecycle — tools must not close it. config: Optional per-run override of the EvolutionaryOptimizerConfig. @@ -54,7 +57,7 @@ class ExperimentalistDeps(BaseModel): train_dataset: DatasetRef validation_dataset: DatasetRef task_template: DatasetRef | None = None - evaluator_type: EvaluatorType = "harbor" + evaluator_type: EvaluatorType = "harbor_native" agent_spec: str | None = None backend: ExperimentalistBackend | None = None reporter: RunReporter | None = None diff --git a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/run.py b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/run.py index 136ae224d1..2b5e95c5a2 100644 --- a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/run.py +++ b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/run.py @@ -110,6 +110,7 @@ async def run_experimentalist( train_dataset=train_dataset, validation_dataset=validation_dataset, task_template=task_template, + evaluator_type=config.evaluator_type, backend=backend, reporter=reporter, config=config, diff --git a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/preflight.py b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/preflight.py index d47b23fbec..8f120fdf3e 100644 --- a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/preflight.py +++ b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/preflight.py @@ -455,7 +455,7 @@ def _check_agent_source( "EXPERIMENTALIST_API_KEY": "API key for EXPERIMENTALIST_API_BASE (on the gateway, INFERENCE_API_KEY fills this)", } -_ENV_EXAMPLE_POINTER = "see examples/tau2-nemo-oo-agent/.env.example" +_ENV_EXAMPLE_POINTER = "see examples/tau3-nooa-agent/.env.example" def _check_env(p: Probes, group: str, names: tuple[str, ...], profile_dir: Path | None) -> list[CheckResult]: diff --git a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/resolve.py b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/resolve.py index 82be6af0b4..86d5c2fc29 100644 --- a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/resolve.py +++ b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/resolve.py @@ -23,6 +23,7 @@ import yaml from nemo_eval_author_plugin.eval_author.models import EvalAuthorConfig +from nemo_experimentalist_plugin.experimentalist.components.evaluator.base import EvaluatorTypeField from nemo_experimentalist_plugin.experimentalist.components.evaluator.models import DatasetRef from nemo_experimentalist_plugin.experimentalist.components.repository import looks_like_git from nemo_experimentalist_plugin.profile import AgentProfile @@ -162,6 +163,7 @@ def reject_legacy_curator_config(cls, data: Any) -> Any: coder: CoderConfig = Field(default_factory=CoderConfig) analyzer: AnalyzerConfig = Field(default_factory=AnalyzerConfig) proposer: ProposerConfig = Field(default_factory=ProposerConfig) + evaluator_type: EvaluatorTypeField = "harbor_native" evaluator: dict[str, Any] = Field(default_factory=dict) eval_author: EvalAuthorConfig = Field(default_factory=EvalAuthorConfig) diff --git a/plugins/nemo-experimentalist/tests/experimentalist/conftest.py b/plugins/nemo-experimentalist/tests/experimentalist/conftest.py new file mode 100644 index 0000000000..f09ad1d71f --- /dev/null +++ b/plugins/nemo-experimentalist/tests/experimentalist/conftest.py @@ -0,0 +1,49 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Shared fixtures for the evaluator test modules. + +Test directories carry no ``__init__.py`` in this repo, so helpers are shared as +fixtures rather than imports. +""" + +from __future__ import annotations + +from collections.abc import Callable, Sequence +from typing import Any + +import pytest + + +def _comparable_trials(trials: Sequence[Any], *, include_id: bool = False) -> list[dict[str, Any]]: + projected = [] + for trial in trials: + entry: dict[str, Any] = { + "task_id": trial.task_id, + "attempt": trial.attempt, + "status": trial.status, + "error": trial.error, + "metrics": {name: metric.value for name, metric in trial.metrics.items()}, + "has_trace": trial.trace is not None, + "resource_kinds": sorted({key.split(":")[0] for key in trial.resources}), + } + if include_id: + entry["id"] = trial.id + projected.append(entry) + return sorted(projected, key=lambda entry: str(entry["task_id"])) + + +@pytest.fixture +def comparable_trials() -> Callable[..., list[dict[str, Any]]]: + """Project trials down to the fields the optimizer loop actually consumes. + + Both A/B parity tests compare evaluator output through this one projection, so + they cannot drift on what "equivalent trials" means — which is the single thing + those tests exist to pin down. ``resources`` is compared by key *kind* (the part + before ``:``) rather than by full key, because artifact keys embed per-trial file + names that legitimately differ between runs. + + Pass ``include_id=True`` to also compare trial ids. That is only meaningful when + both sides read the same job directory — Harbor mints a random suffix per run. + """ + return _comparable_trials diff --git a/plugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging_loop.py b/plugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging_loop.py index a03990f009..508537eece 100644 --- a/plugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging_loop.py +++ b/plugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging_loop.py @@ -88,7 +88,7 @@ async def run( backend=backend, workspace="default", config=config, - evaluator_type="harbor", + evaluator_type="harbor_native", train_dataset=DatasetRef(uri=str(train)), validation_dataset=DatasetRef(uri=str(validation)), task_template=DatasetRef(uri=str(template)), diff --git a/plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_factory.py b/plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_factory.py index eb7fea0353..c9ab5a74b0 100644 --- a/plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_factory.py +++ b/plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_factory.py @@ -2,11 +2,14 @@ # SPDX-License-Identifier: Apache-2.0 import json +import subprocess +import sys from pathlib import Path from typing import Sequence import pytest from nemo_experimentalist_plugin.experimentalist.components.evaluator import DatasetRef +from nemo_experimentalist_plugin.experimentalist.components.evaluator import harbor as harbor_contract from nemo_experimentalist_plugin.experimentalist.components.evaluator.base import ( Dataset, Evaluator, @@ -14,7 +17,7 @@ TrialResult, ) from nemo_experimentalist_plugin.experimentalist.components.evaluator.factory import DatasetFactory, EvaluatorFactory -from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor import ( +from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_native import ( HarborEvaluator, HarborEvaluatorConfig, ) @@ -118,7 +121,7 @@ def test_build_dataset_falsy_evaluator_type(): def test_build_dataset_falsy_dataset_ref(): with pytest.raises(ValueError, match="Evaluator type and dataset reference are required"): - DatasetFactory().build_dataset("harbor", None) + DatasetFactory().build_dataset("harbor_native", None) def test_build_task_template_zero_tasks(tmp_path): @@ -126,7 +129,7 @@ def test_build_task_template_zero_tasks(tmp_path): empty_dir.mkdir() (empty_dir / "not-a-task").mkdir() with pytest.raises(ValueError, match="contains no Harbor task directories"): - DatasetFactory().build_task_template("harbor", DatasetRef(uri=str(empty_dir))) + DatasetFactory().build_task_template("harbor_native", DatasetRef(uri=str(empty_dir))) def test_build_task_template_multiple_tasks(tmp_path): @@ -135,30 +138,62 @@ def test_build_task_template_multiple_tasks(tmp_path): (dataset_dir / "task-a" / "task.toml").write_text("") (dataset_dir / "task-b").mkdir() (dataset_dir / "task-b" / "task.toml").write_text("") - with pytest.raises(ValueError, match="exactly one harbor task"): - DatasetFactory().build_task_template("harbor", DatasetRef(uri=str(dataset_dir))) + with pytest.raises(ValueError, match="exactly one harbor_native task"): + DatasetFactory().build_task_template("harbor_native", DatasetRef(uri=str(dataset_dir))) def test_build_task_template_single_task(tmp_path): task_dir = tmp_path / "task-only" task_dir.mkdir() (task_dir / "task.toml").write_text("") - task = DatasetFactory().build_task_template("harbor", DatasetRef(uri=str(task_dir))) + task = DatasetFactory().build_task_template("harbor_native", DatasetRef(uri=str(task_dir))) assert task.id == "task-only" def test_evaluator_factory_build_evaluator_with_config(): factory = EvaluatorFactory() - evaluator = factory.build_evaluator("harbor", HarborEvaluatorConfig()) + evaluator = factory.build_evaluator("harbor_native", HarborEvaluatorConfig()) assert isinstance(evaluator, HarborEvaluator) def test_evaluator_factory_build_evaluator_with_dict(): factory = EvaluatorFactory() - evaluator = factory.build_evaluator("harbor", {"import_path": "x:Y"}) + evaluator = factory.build_evaluator("harbor_native", {"import_path": "x:Y"}) assert isinstance(evaluator, HarborEvaluator) +def test_harbor_orchestrators_live_outside_the_shared_harbor_module(): + factory = EvaluatorFactory() + + native = factory.build_evaluator("harbor_native", {}) + sdk_backed = factory.build_evaluator("harbor_evaluator", {}) + + assert type(native).__module__ == ("nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_native") + assert type(sdk_backed).__module__ == ( + "nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_evaluator" + ) + + +def test_shared_harbor_module_preserves_native_evaluator_imports_lazily(): + assert harbor_contract.HarborEvaluator is HarborEvaluator + assert harbor_contract.HarborEvaluatorConfig is HarborEvaluatorConfig + + +def test_importing_shared_harbor_module_does_not_load_native_orchestration(): + shared_module = "nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor" + native_module = "nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_native" + script = f"import sys; import {shared_module}; raise SystemExit({native_module!r} in sys.modules)" + + completed = subprocess.run( + [sys.executable, "-c", script], + check=False, + capture_output=True, + text=True, + ) + + assert completed.returncode == 0, completed.stderr + + def test_evaluator_factory_build_evaluator_unsupported(): factory = EvaluatorFactory() with pytest.raises(ValueError, match="Unsupported evaluator type"): @@ -167,5 +202,5 @@ def test_evaluator_factory_build_evaluator_unsupported(): def test_evaluator_factory_build_evaluator_wrong_config_type(): factory = EvaluatorFactory() - with pytest.raises(TypeError, match="Harbor evaluator config must be an EvaluatorConfig or dict"): - factory.build_evaluator("harbor", 42) + with pytest.raises(TypeError, match="'harbor_native' evaluator config must be an EvaluatorConfig or dict"): + factory.build_evaluator("harbor_native", 42) diff --git a/plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor.py b/plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor.py index f869a74586..38f8d496ac 100644 --- a/plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor.py +++ b/plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor.py @@ -11,25 +11,27 @@ import pytest from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor import ( - _TRACE_ARTIFACT_DESTINATION, - _TRACE_ARTIFACT_SOURCE, + DEFAULT_TRACE_ARTIFACT_SOURCE, HarborDataset, HarborDependencyContext, HarborDependencyRuntime, - HarborEvaluator, - HarborEvaluatorConfig, HarborVerifierValidationError, _chmod_path_chain, - _cleanup_scoped_imports, - _ensure_package, _python_syntax_failure, - _safe_identifier, - _scoped_import_path, _shell_syntax_failure, _trial_error, _trial_metric_spec, _trial_metrics, _trial_resources, +) +from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_native import ( + _TRACE_ARTIFACT_DESTINATION, + HarborEvaluator, + HarborEvaluatorConfig, + _cleanup_scoped_imports, + _ensure_package, + _safe_identifier, + _scoped_import_path, _with_trace_artifact, ) from nemo_experimentalist_plugin.experimentalist.components.evaluator.models import ( @@ -720,7 +722,7 @@ async def create(cls, config): async def run(self): return SimpleNamespace(id="job-id", stats=FakeStats()) - monkeypatch.setattr("nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor.Job", FakeJob) + monkeypatch.setattr("nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_native.Job", FakeJob) result = await evaluator.run( agent=tmp_path / "agent", @@ -845,7 +847,7 @@ async def test_harbor_evaluator_rejects_invalid_python_verifiers_before_job_crea dataset = HarborDataset.from_path(dataset_dir) fake_job = _recording_job(tmp_path / "jobs" / "preflight") - monkeypatch.setattr("nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor.Job", fake_job) + monkeypatch.setattr("nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_native.Job", fake_job) compile_calls = 0 original_compile = compile @@ -966,7 +968,7 @@ async def test_harbor_evaluator_accepts_valid_python_verifier( _write(task_dir / "tests" / "check.py", "def check():\n return True\n") dataset = HarborDataset.from_path(task_dir) fake_job = _recording_job(tmp_path / "jobs" / "valid-python") - monkeypatch.setattr("nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor.Job", fake_job) + monkeypatch.setattr("nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_native.Job", fake_job) trials = await HarborEvaluator()._run(agent_dir, dataset, HarborEvaluatorConfig()) @@ -988,7 +990,7 @@ async def test_harbor_evaluator_rejects_invalid_configured_test_sh_before_job_cr _write(task_dir / "test" / "test.sh", "if true; then\n echo broken\n") dataset = HarborDataset.from_path(task_dir) fake_job = _recording_job(tmp_path / "jobs" / "invalid-shell") - monkeypatch.setattr("nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor.Job", fake_job) + monkeypatch.setattr("nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_native.Job", fake_job) with pytest.raises(HarborVerifierValidationError) as exc_info: await HarborEvaluator()._run(agent_dir, dataset, HarborEvaluatorConfig()) @@ -1013,7 +1015,7 @@ async def test_harbor_evaluator_accepts_valid_legacy_test_sh( _write(task_dir / "test" / "test.sh", "if true; then\n echo valid\nfi\n") dataset = HarborDataset.from_path(task_dir) fake_job = _recording_job(tmp_path / "jobs" / "valid-shell") - monkeypatch.setattr("nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor.Job", fake_job) + monkeypatch.setattr("nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_native.Job", fake_job) trials = await HarborEvaluator()._run(agent_dir, dataset, HarborEvaluatorConfig()) @@ -1327,8 +1329,8 @@ def test_trial_metric_spec_task_dir_verifier(tmp_path): def test_with_trace_artifact_already_has_source(): from harbor.models.job.config import ArtifactConfig - existing = ArtifactConfig(source=_TRACE_ARTIFACT_SOURCE, destination="traces") - result = _with_trace_artifact([existing], _TRACE_ARTIFACT_SOURCE) + existing = ArtifactConfig(source=DEFAULT_TRACE_ARTIFACT_SOURCE, destination="traces") + result = _with_trace_artifact([existing], DEFAULT_TRACE_ARTIFACT_SOURCE) assert result == [existing] @@ -1336,7 +1338,7 @@ def test_with_trace_artifact_already_has_destination(): from harbor.models.job.config import ArtifactConfig existing = ArtifactConfig(source="/other", destination=_TRACE_ARTIFACT_DESTINATION) - result = _with_trace_artifact([existing], _TRACE_ARTIFACT_SOURCE) + result = _with_trace_artifact([existing], DEFAULT_TRACE_ARTIFACT_SOURCE) assert result == [existing] @@ -1344,9 +1346,9 @@ def test_with_trace_artifact_adds_when_missing(): from harbor.models.job.config import ArtifactConfig other = ArtifactConfig(source="/other", destination="other") - result = _with_trace_artifact([other], _TRACE_ARTIFACT_SOURCE) + result = _with_trace_artifact([other], DEFAULT_TRACE_ARTIFACT_SOURCE) assert len(result) == 2 - assert result[0].source == _TRACE_ARTIFACT_SOURCE + assert result[0].source == DEFAULT_TRACE_ARTIFACT_SOURCE def test_trial_error_non_dict(): @@ -1419,7 +1421,7 @@ async def create(cls, config): async def run(self): return SimpleNamespace(id="job-id", stats=None) - monkeypatch.setattr("nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor.Job", FakeJob) + monkeypatch.setattr("nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_native.Job", FakeJob) trials = await evaluator._run( agent=agent_dir, @@ -1880,10 +1882,10 @@ def test_resolve_trial_task_id_fallback_to_trial_base(): def test_with_trace_artifact_string_match(): - result = _with_trace_artifact([_TRACE_ARTIFACT_SOURCE], _TRACE_ARTIFACT_SOURCE) - assert result == [_TRACE_ARTIFACT_SOURCE] + result = _with_trace_artifact([DEFAULT_TRACE_ARTIFACT_SOURCE], DEFAULT_TRACE_ARTIFACT_SOURCE) + assert result == [DEFAULT_TRACE_ARTIFACT_SOURCE] def test_with_trace_artifact_string_destination_match(): - result = _with_trace_artifact([_TRACE_ARTIFACT_DESTINATION], _TRACE_ARTIFACT_SOURCE) + result = _with_trace_artifact([_TRACE_ARTIFACT_DESTINATION], DEFAULT_TRACE_ARTIFACT_SOURCE) assert result == [_TRACE_ARTIFACT_DESTINATION] diff --git a/plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_ab_e2e.py b/plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_ab_e2e.py new file mode 100644 index 0000000000..fa67a90661 --- /dev/null +++ b/plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_ab_e2e.py @@ -0,0 +1,173 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Live A/B: plain Harbor vs the SDK's ``HarborAgentTaskRunner``. + +Everything else in the evaluator suite fakes Harbor's ``Job``. This module runs +the real thing over the bundled ``hello-harbor-agent`` example — Docker builds the +task image, the agent runs in the container, and the verifier writes the rewards — +once through each evaluator type, then asserts the two produce the same trials. + +It needs Docker and ``harbor``, and is skipped otherwise. No LLM or network is +involved: the hello agent is stdlib-only and the Experimentalist's own LLM +components (Coder, Analyzer, Proposer) are not in this path. +""" + +from __future__ import annotations + +import shutil +import subprocess +from pathlib import Path +from typing import Any + +import pytest +from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor import ( + HarborDataset, + HarborEvaluator, + HarborEvaluatorConfig, +) +from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_evaluator import ( + HarborRunnerConfig, + HarborRunnerEvaluator, + harbor_task_names, +) + +pytestmark = [pytest.mark.e2e, pytest.mark.slow, pytest.mark.skip_in_ci, pytest.mark.asyncio] + +_EXAMPLE_DIR = Path(__file__).resolve().parents[2] / "examples" / "hello-harbor-agent" +_VALIDATION_DIR = _EXAMPLE_DIR / "dataset" / "validation" +_TRAIN_DIR = _EXAMPLE_DIR / "dataset" / "train" + +# The baseline agent handles greetings but not arithmetic, so exactly one of the +# two tasks in each split scores; both emit format_ok. See the example README. +# test_hello_example_baseline.py guards this without Docker; here we confirm the +# whole container/verifier pipeline reproduces it. +_EXPECTED_AGGREGATE = {"reward": 0.5, "format_ok": 1.0} + + +def _docker_available() -> bool: + if shutil.which("docker") is None: + return False + # Bounded: `docker info` blocks indefinitely when a credential helper prompts + # for keychain access in a non-interactive shell, which would hang collection + # rather than skip the test. + try: + return subprocess.run(["docker", "info"], capture_output=True, timeout=30).returncode == 0 + except (OSError, subprocess.SubprocessError): + return False + + +@pytest.fixture(scope="session") +def requires_docker() -> None: + """Probe Docker once per session. + + Session-scoped on purpose: the probe result cannot change mid-run, and a + function-scoped fixture would fork `docker info` once per test — paying the + 30s timeout four times over when the daemon is wedged, which is the exact + hang this bound exists to avoid. + """ + pytest.importorskip("harbor") + if not _docker_available(): + pytest.skip("Docker daemon is required to run a Harbor job") + + +@pytest.fixture +def hello_agent_dir(tmp_path: Path) -> Path: + """Materialize the example as `agent-0`, the way the loop does.""" + agent_dir = tmp_path / "agents" / "agent-0" + shutil.copytree(_EXAMPLE_DIR, agent_dir, ignore=shutil.ignore_patterns("dataset", "__pycache__")) + return agent_dir + + +async def test_train_split_still_has_a_real_failure_to_diagnose( + requires_docker: None, + hello_agent_dir: Path, + tmp_path: Path, +) -> None: + """End-to-end proof that the optimizer loop has something to work on. + + The Analyzer/Proposer/Coder round only demonstrates anything if a train task + actually fails in a container. A baseline that quietly gained an arithmetic + handler would score 1.0 here and leave the loop with nothing to diagnose. + """ + + result = await HarborRunnerEvaluator(experiment_dir=tmp_path).run( + hello_agent_dir, + HarborDataset.from_path(_TRAIN_DIR), + HarborRunnerConfig(jobs_dir=Path("train-jobs"), quiet=True), + ) + + assert result.aggregate_metrics == pytest.approx(_EXPECTED_AGGREGATE) + rewards = {trial.task_id: trial.metrics["reward"].value for trial in result.trials} + assert rewards == {"greet-world": 1.0, "sum-two": 0.0} + # The failure must be a scored 0, not a crashed trial: the Analyzer reads the + # trace of a completed-but-wrong run, and an errored trial teaches it nothing. + assert all(trial.status == "completed" for trial in result.trials) + assert all(trial.trace is not None for trial in result.trials) + + +async def test_short_ids_translate_to_the_example_full_harbor_names(requires_docker: None) -> None: + """The name translation the SDK path depends on, checked against real task.toml files.""" + dataset = HarborDataset.from_path(_VALIDATION_DIR) + + assert harbor_task_names(dataset) == { + "greet-universe": "hello/greet-universe", + "sum-three": "hello/sum-three", + } + + +async def test_plain_and_sdk_evaluators_agree_on_the_hello_example( + requires_docker: None, + hello_agent_dir: Path, + tmp_path: Path, + comparable_trials: Any, +) -> None: + dataset = HarborDataset.from_path(_VALIDATION_DIR) + + plain = await HarborEvaluator(experiment_dir=tmp_path).run( + hello_agent_dir, dataset, HarborEvaluatorConfig(jobs_dir=Path("plain-jobs"), quiet=True) + ) + sdk = await HarborRunnerEvaluator(experiment_dir=tmp_path).run( + hello_agent_dir, dataset, HarborRunnerConfig(jobs_dir=Path("sdk-jobs"), quiet=True) + ) + + assert sdk.aggregate_metrics == pytest.approx(_EXPECTED_AGGREGATE) + assert plain.aggregate_metrics == pytest.approx(_EXPECTED_AGGREGATE) + assert comparable_trials(sdk.trials) == comparable_trials(plain.trials) + + # Both must cover the same tasks under their short Experimentalist ids, and + # both must surface traces for the Analyzer to read. + assert {trial.task_id for trial in sdk.trials} == {"greet-universe", "sum-three"} + assert all(trial.status == "completed" for trial in sdk.trials) + assert all(trial.trace is not None for trial in sdk.trials) + + +async def test_sdk_evaluator_serves_a_complete_run_from_cache( + requires_docker: None, + hello_agent_dir: Path, + tmp_path: Path, + comparable_trials: Any, +) -> None: + """A second identical run must re-adapt the job dir instead of rebuilding it.""" + dataset = HarborDataset.from_path(_VALIDATION_DIR) + evaluator = HarborRunnerEvaluator(experiment_dir=tmp_path) + options = HarborRunnerConfig(jobs_dir=Path("jobs"), quiet=True) + + first = await evaluator.run(hello_agent_dir, dataset, options) + job_dir = tmp_path / "jobs" / f"{hello_agent_dir.name}-{dataset.id}" + # Harbor names each trial dir `__`, so identical dir names across + # runs is real evidence of reuse. Globbing `.name` would only ever collect + # {"result.json"} and pass no matter what the second run did. + trial_dirs = {path.parent.name for path in job_dir.glob("*/result.json")} + assert trial_dirs, "the first run must have written trial directories" + + cached = await evaluator.run(hello_agent_dir, dataset, options) + + assert comparable_trials(cached.trials) == comparable_trials(first.trials) + assert {path.parent.name for path in job_dir.glob("*/result.json")} == trial_dirs + + # force_rerun must beat the cache: the old job dir is discarded, so Harbor + # mints new randomly-suffixed trial dirs while the scores stay the same. + rerun = await evaluator.run(hello_agent_dir, dataset, options.model_copy(update={"force_rerun": True})) + assert rerun.aggregate_metrics == pytest.approx(_EXPECTED_AGGREGATE) + assert {path.parent.name for path in job_dir.glob("*/result.json")}.isdisjoint(trial_dirs) diff --git a/plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_evaluator.py b/plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_evaluator.py new file mode 100644 index 0000000000..572d647796 --- /dev/null +++ b/plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_evaluator.py @@ -0,0 +1,679 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Regression coverage for the SDK-backed Harbor evaluator. + +Nothing here starts Docker: Harbor's ``Job`` is replaced with a fake that writes +the same on-disk tree a real run would, which is exactly the seam both evaluator +types read their results from. +""" + +from __future__ import annotations + +import inspect +import json +import logging +import sys +from pathlib import Path +from typing import Any + +import pytest +from nemo_experimentalist_plugin.experimentalist.components.evaluator.base import ( + EvaluatorConfig, + _warned_evaluator_types, +) +from nemo_experimentalist_plugin.experimentalist.components.evaluator.factory import EvaluatorFactory +from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor import HarborDataset +from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_evaluator import ( + HarborRunnerConfig, + HarborRunnerEvaluator, + HarborTaskNameError, + harbor_task_names, +) +from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_native import ( + HarborEvaluator, + HarborEvaluatorConfig, +) +from nemo_experimentalist_plugin.experimentalist.components.evaluator.models import local_path_from_uri +from nemo_experimentalist_plugin.experimentalist.deps import ExperimentalistDeps +from nemo_experimentalist_plugin.resolve import EvolutionaryOptimizerConfig +from pydantic import ValidationError + + +def _write(path: Path, text: str) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + + +pytestmark = pytest.mark.asyncio + + +def _dataset_root(dataset: HarborDataset) -> Path: + assert dataset.source is not None + return local_path_from_uri(dataset.source.uri, context="dataset").resolve() + + +def _write_task(task_dir: Path, full_name: str | None = None) -> None: + """Write a minimal Harbor task whose ``[task].name`` differs from its directory.""" + name_block = f'\n[task]\nname = "{full_name}"\n' if full_name is not None else "" + _write(task_dir / "task.toml", f'schema_version = "1.3"\n{name_block}') + _write(task_dir / "instruction.md", f"do {task_dir.name}") + _write(task_dir / "tests" / "test.sh", "echo reward") + + +def _write_trial( + job_dir: Path, + *, + trial_name: str, + task_name: str, + task_dir: Path, + rewards: dict[str, float] | None = None, + exception_info: dict[str, str] | None = None, +) -> None: + trial_dir = job_dir / trial_name + _write( + trial_dir / "result.json", + json.dumps( + { + "trial_name": trial_name, + "task_name": task_name, + "task_id": {"path": str(task_dir.resolve())}, + "verifier_result": {"rewards": rewards if rewards is not None else {}}, + "exception_info": exception_info, + } + ), + ) + + +class _FakeJob: + """Stand-in for Harbor's ``Job`` that records its config and writes trials.""" + + calls: list[Any] = [] + on_run: Any = None + + def __init__(self, config: Any) -> None: + self.config = config + + @classmethod + async def create(cls, config: Any) -> _FakeJob: + cls.calls.append(config) + # Harbor's Job.create lays down the job dir before any trial runs. + (Path(config.jobs_dir) / config.job_name).mkdir(parents=True, exist_ok=True) + return cls(config) + + async def run(self) -> None: + if type(self).on_run is not None: + type(self).on_run(self.config) + + +@pytest.fixture +def fake_job(monkeypatch: pytest.MonkeyPatch) -> type[_FakeJob]: + """Replace Harbor's ``Job`` for the duration of a test. + + The SDK imports ``Job`` inside its ``run_job`` closure, so patching the + module attribute is enough — and it is the only Harbor piece faked, so the + real ``JobConfig`` still validates everything the runtime builds. + """ + pytest.importorskip("harbor") + import harbor.job + + _FakeJob.calls = [] + _FakeJob.on_run = None + monkeypatch.setattr(harbor.job, "Job", _FakeJob) + return _FakeJob + + +@pytest.fixture +def dataset(tmp_path: Path) -> HarborDataset: + """Two tasks whose full Harbor names are namespaced and share a basename prefix.""" + dataset_dir = tmp_path / "dataset" / "validation" + _write_task(dataset_dir / "sum-two", "hello/sum-two") + _write_task(dataset_dir / "sum-three", "hello/sum-three") + return HarborDataset.from_path(dataset_dir) + + +@pytest.fixture +async def cached_job_dir( + tmp_path: Path, + dataset: HarborDataset, + agent_dir: Path, + fake_job: type[_FakeJob], +) -> Path: + """A complete, all-successful cached job dir left by a genuine prior run. + + Driven through ``_run`` rather than hand-built so the SDK stamps its own cache + key exactly as it would in production. Hand-stamping here would couple the test + to the plugin-config → ``HarborRuntimeConfig`` mapping, and an *unstamped* dir is + correctly untrusted — which would make every test using this fixture pass for + the wrong reason. + """ + job_dir = tmp_path / "jobs" / f"{agent_dir.name}-{dataset.id}" + + def write_complete_results(config: Any) -> None: + for task in dataset.tasks: + _write_trial( + Path(config.jobs_dir) / config.job_name, + trial_name=f"{task.id}__0", + task_name=f"hello/{task.id}", + task_dir=_dataset_root(dataset) / task.id, + rewards={"reward": 1.0}, + ) + + fake_job.on_run = write_complete_results + await HarborRunnerEvaluator(experiment_dir=tmp_path)._run( + agent_dir, dataset, HarborRunnerConfig(jobs_dir=Path("jobs")) + ) + fake_job.calls = [] + fake_job.on_run = None + return job_dir + + +@pytest.fixture +def agent_dir(tmp_path: Path) -> Path: + path = tmp_path / "agents" / "agent-0" + _write(path / "harbor_wrapper.py", "class WrappedAgent: ...\n") + return path + + +# -------------------------------------------------------------------------- +# Factory and configuration +# -------------------------------------------------------------------------- + + +async def test_optimizer_config_defaults_to_native_harbor() -> None: + assert EvolutionaryOptimizerConfig().evaluator_type == "harbor_native" + + +async def test_deps_default_matches_the_optimizer_config_default() -> None: + """The two defaults must not drift: run.py threads one into the other.""" + assert ( + ExperimentalistDeps.model_fields["evaluator_type"].default + == EvolutionaryOptimizerConfig.model_fields["evaluator_type"].default + == "harbor_native" + ) + + +async def test_optimizer_config_still_accepts_plain_harbor() -> None: + """Plain Harbor stays selectable — it is the A/B baseline.""" + config = EvolutionaryOptimizerConfig.model_validate({"evaluator_type": "harbor_native"}) + assert config.evaluator_type == "harbor_native" + + +async def test_retired_harbor_spelling_still_resolves_and_warns(caplog: pytest.LogCaptureFixture) -> None: + # `harbor` shipped before the rename, so experiment YAMLs in the wild are pinned + # to it. Those configs must keep running, and the operator must be told once. + _warned_evaluator_types.clear() + with caplog.at_level(logging.WARNING): + config = EvolutionaryOptimizerConfig.model_validate({"evaluator_type": "harbor"}) + again = EvolutionaryOptimizerConfig.model_validate({"evaluator_type": "harbor"}) + + assert config.evaluator_type == "harbor_native" + assert again.evaluator_type == "harbor_native", "the alias must keep resolving, not just the first time" + assert caplog.text.count("is deprecated") == 1, "a pinned config must not warn once per round" + + +async def test_retired_spelling_is_not_extended_to_the_never_shipped_name() -> None: + # `harbor_agent_task_runner` only ever existed on an unmerged branch, so nothing + # can be pinned to it. Accepting it would advertise a name we never released. + with pytest.raises(ValidationError) as excinfo: + EvolutionaryOptimizerConfig.model_validate({"evaluator_type": "harbor_agent_task_runner"}) + + # Pin the error *location*, not just the type: a BeforeValidator that raised for + # some unrelated field would otherwise satisfy this test. + assert [error["loc"] for error in excinfo.value.errors()] == [("evaluator_type",)] + + +async def test_job_name_comes_from_the_resolved_agent_dir( + tmp_path: Path, dataset: HarborDataset, monkeypatch: pytest.MonkeyPatch +) -> None: + """`job_name` is the cache identity, so it must not depend on how the path is spelled. + + `Path(".").name` is empty, so deriving the name from the caller's spelling makes + every `--agent .` run collide on one job dir no matter which directory it points + at — and the SDK's scoped import derives its package name from the *resolved* + directory, so the two identities would disagree. + """ + from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor import resolve_harbor_run_inputs + + job_names: list[str] = [] + for agent_name in ("agent-a", "agent-b"): + agent = tmp_path / "agents" / agent_name + _write(agent / "harbor_wrapper.py", "class WrappedAgent: ...\n") + monkeypatch.chdir(agent) + inputs = await resolve_harbor_run_inputs(Path("."), dataset, HarborRunnerConfig(), tmp_path) + job_names.append(inputs.job_name) + + assert job_names == [f"agent-a-{dataset.id}", f"agent-b-{dataset.id}"] + assert len(set(job_names)) == 2, "two different agents must not share one job dir" + + +async def test_job_name_survives_a_symlinked_agent_dir( + tmp_path: Path, dataset: HarborDataset, monkeypatch: pytest.MonkeyPatch +) -> None: + # A symlink keeps its own name while resolving elsewhere. Following it keeps + # `job_name` in step with the scoped import package, which resolves too. + from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor import resolve_harbor_run_inputs + + real = tmp_path / "agents" / "agent-3" + _write(real / "harbor_wrapper.py", "class WrappedAgent: ...\n") + link = tmp_path / "agents" / "current" + link.symlink_to(real, target_is_directory=True) + + inputs = await resolve_harbor_run_inputs(link, dataset, HarborRunnerConfig(), tmp_path) + + assert inputs.job_name == f"agent-3-{dataset.id}", "the job dir must follow the agent, not the alias" + assert inputs.agent_path == real.resolve() + + +async def test_eval_author_default_tracks_the_experimentalist_default() -> None: + """The two plugins must not disagree about which adapter is canonical. + + Inert today — `run_eval_author` only consults the *dataset* half of the registry + and both types map to `HarborDataset` — but it silently stops being inert the day + the two types get different Dataset classes. + """ + from nemo_eval_author_plugin.eval_author.run import run_eval_author + + assert ( + inspect.signature(run_eval_author).parameters["evaluator_type"].default + == EvolutionaryOptimizerConfig.model_fields["evaluator_type"].default + ) + + +async def test_optimizer_config_rejects_unknown_evaluator_type() -> None: + with pytest.raises(ValidationError): + EvolutionaryOptimizerConfig.model_validate({"evaluator_type": "not-an-evaluator"}) + + +async def test_factory_builds_sdk_evaluator(tmp_path: Path) -> None: + evaluator = EvaluatorFactory().build_evaluator( + "harbor_evaluator", + {"n_attempts": 3, "quiet": True}, + experiment_dir=tmp_path, + ) + + assert isinstance(evaluator, HarborRunnerEvaluator) + assert evaluator.evaluator_type == "harbor_evaluator" + assert isinstance(evaluator.options, HarborRunnerConfig) + assert evaluator.options.n_attempts == 3 + assert evaluator.experiment_dir == tmp_path + + +@pytest.mark.parametrize( + "unsupported", + [ + {"retry": {"max_retries": 2}}, # plain-Harbor RetryConfig has no SDK equivalent + {"agent_dir": "/somewhere/else"}, # always derived from the candidate + {"typo_option": 1}, + ], +) +async def test_sdk_config_rejects_unsupported_options(unsupported: dict[str, Any]) -> None: + with pytest.raises(ValidationError, match="Extra inputs are not permitted"): + HarborRunnerConfig.model_validate(unsupported) + + +async def test_plain_harbor_still_builds() -> None: + """``harbor_native`` stays constructible — it is the A/B baseline. + + This used to also assert it survived a *missing* SDK, back when the runtime was + imported lazily. That premise is gone: ``nemo-evaluator-sdk`` is a declared, + workspace-linked dependency, so it ships with the plugin and cannot be absent. + """ + assert isinstance(EvaluatorFactory().build_evaluator("harbor_native", {}), HarborEvaluator) + + +# -------------------------------------------------------------------------- +# Task-name mapping +# -------------------------------------------------------------------------- + + +async def test_short_ids_map_to_full_harbor_names(dataset: HarborDataset) -> None: + assert harbor_task_names(dataset) == { + "sum-three": "hello/sum-three", + "sum-two": "hello/sum-two", + } + + +async def test_mapping_follows_dataset_subsets(dataset: HarborDataset) -> None: + assert harbor_task_names(dataset.subset(["sum-two"])) == {"sum-two": "hello/sum-two"} + + +async def test_mapping_falls_back_to_directory_name_without_task_block(tmp_path: Path) -> None: + dataset_dir = tmp_path / "unnamed" + _write_task(dataset_dir / "plain-task", full_name=None) + + assert harbor_task_names(HarborDataset.from_path(dataset_dir)) == {"plain-task": "plain-task"} + + +async def test_duplicate_full_names_are_rejected(tmp_path: Path) -> None: + dataset_dir = tmp_path / "dupes" + _write_task(dataset_dir / "task-a", "hello/same") + _write_task(dataset_dir / "task-b", "hello/same") + + with pytest.raises(HarborTaskNameError, match="both declare"): + harbor_task_names(HarborDataset.from_path(dataset_dir)) + + +async def test_task_outside_the_dataset_directory_is_rejected(tmp_path: Path, dataset: HarborDataset) -> None: + stray_dir = tmp_path / "stray" / "sum-four" + _write_task(stray_dir, "hello/sum-four") + stray = HarborDataset.from_path(stray_dir.parent).tasks[0] + dataset.tasks.append(stray) + + with pytest.raises(HarborTaskNameError, match="was not discovered under dataset"): + harbor_task_names(dataset) + + +# -------------------------------------------------------------------------- +# Execution +# -------------------------------------------------------------------------- + + +async def test_runner_receives_expected_job_config( + tmp_path: Path, + dataset: HarborDataset, + agent_dir: Path, + fake_job: type[_FakeJob], +) -> None: + evaluator = HarborRunnerEvaluator(experiment_dir=tmp_path) + options = HarborRunnerConfig( + jobs_dir=Path("jobs"), + n_attempts=2, + n_concurrent_trials=3, + quiet=True, + max_retries=4, + trace_dir="/app/traces", + agent_timeout_multiplier=1.5, + verifier_timeout_multiplier=2.0, + ) + + await evaluator._run(agent_dir, dataset, options) + + assert len(fake_job.calls) == 1 + config = fake_job.calls[0] + assert config.job_name == f"{agent_dir.name}-{dataset.id}" + assert config.jobs_dir == tmp_path / "jobs" + assert config.n_attempts == 2 + assert config.n_concurrent_trials == 3 + assert config.quiet is True + assert config.retry.max_retries == 4 + assert config.agent_timeout_multiplier == 1.5 + assert config.verifier_timeout_multiplier == 2.0 + + # Harbor's local-dataset filter matches directory names, not [task].name. + assert config.datasets[0].path == _dataset_root(dataset) + assert sorted(config.datasets[0].task_names) == ["sum-three", "sum-two"] + + # Traces are collected as the 'traces' artifact so the Analyzer can read them. + trace_artifacts = [a for a in config.artifacts if getattr(a, "destination", None) == "traces"] + assert [a.source for a in trace_artifacts] == ["/app/traces"] + + # The wrapper is imported out of the candidate directory under a scoped package. + import_path = config.agents[0].import_path + assert import_path.endswith(".harbor_wrapper:WrappedAgent") + assert import_path.startswith("_nemo_evaluator_harbor_agents.") + # ...and the scoped package is torn down once the run finishes. + assert not [name for name in sys.modules if name.startswith("_nemo_evaluator_harbor_agents.")] + + +async def test_complete_cached_job_is_not_rerun( + tmp_path: Path, + dataset: HarborDataset, + agent_dir: Path, + cached_job_dir: Path, + fake_job: type[_FakeJob], +) -> None: + trials = await HarborRunnerEvaluator(experiment_dir=tmp_path)._run( + agent_dir, dataset, HarborRunnerConfig(jobs_dir=Path("jobs")) + ) + + assert fake_job.calls == [] + assert {trial.task_id for trial in trials} == {"sum-two", "sum-three"} + + +async def test_errored_cached_job_is_rerun( + tmp_path: Path, + dataset: HarborDataset, + agent_dir: Path, + cached_job_dir: Path, + fake_job: type[_FakeJob], +) -> None: + """An errored trial must force a rerun even when the cache is otherwise valid. + + Built on the *stamped* `cached_job_dir` on purpose. A hand-rolled job dir has + no fingerprint, so it is rejected as untrusted and the run happens for that + reason instead — the assertion would then hold even if error-awareness were + completely broken. Mutating one trial in place keeps the stamp valid, so the + error is the only thing left that can trigger the rerun. + """ + errored = json.loads((cached_job_dir / "sum-three__0" / "result.json").read_text(encoding="utf-8")) + errored["exception_info"] = {"exception_type": "TimeoutError"} + _write(cached_job_dir / "sum-three__0" / "result.json", json.dumps(errored)) + + await HarborRunnerEvaluator(experiment_dir=tmp_path)._run( + agent_dir, dataset, HarborRunnerConfig(jobs_dir=Path("jobs")) + ) + + assert len(fake_job.calls) == 1, "an errored cached trial must not be served from cache" + + +async def test_under_sampled_cached_job_is_rerun( + tmp_path: Path, + dataset: HarborDataset, + agent_dir: Path, + cached_job_dir: Path, + fake_job: type[_FakeJob], +) -> None: + await HarborRunnerEvaluator(experiment_dir=tmp_path)._run( + agent_dir, dataset, HarborRunnerConfig(jobs_dir=Path("jobs"), n_attempts=2) + ) + + assert len(fake_job.calls) == 1, "one cached attempt must not satisfy n_attempts=2" + + +async def test_force_rerun_discards_a_complete_cache( + tmp_path: Path, + dataset: HarborDataset, + agent_dir: Path, + cached_job_dir: Path, + fake_job: type[_FakeJob], +) -> None: + trials = await HarborRunnerEvaluator(experiment_dir=tmp_path)._run( + agent_dir, dataset, HarborRunnerConfig(jobs_dir=Path("jobs"), force_rerun=True) + ) + + assert len(fake_job.calls) == 1 + assert list(cached_job_dir.glob("*/result.json")) == [], "force_rerun must clear the stale results" + assert trials == [] + + +async def test_concurrent_candidates_use_distinct_job_dirs( + tmp_path: Path, + dataset: HarborDataset, + fake_job: type[_FakeJob], +) -> None: + evaluator = HarborRunnerEvaluator(experiment_dir=tmp_path) + options = HarborRunnerConfig(jobs_dir=Path("jobs")) + for name in ("agent-0", "agent-1"): + candidate = tmp_path / "agents" / name + _write(candidate / "harbor_wrapper.py", "class WrappedAgent: ...\n") + await evaluator._run(candidate, dataset, options) + + job_names = [config.job_name for config in fake_job.calls] + assert job_names == [f"agent-0-{dataset.id}", f"agent-1-{dataset.id}"] + assert len(set(job_names)) == 2 + + +async def test_missing_agent_directory_fails_before_docker( + tmp_path: Path, + dataset: HarborDataset, + fake_job: type[_FakeJob], +) -> None: + with pytest.raises(FileNotFoundError, match="Harbor agent path not found"): + await HarborRunnerEvaluator(experiment_dir=tmp_path)._run(tmp_path / "nope", dataset, HarborRunnerConfig()) + assert fake_job.calls == [] + + +async def test_broken_verifier_fails_before_docker( + tmp_path: Path, + agent_dir: Path, + fake_job: type[_FakeJob], +) -> None: + dataset_dir = tmp_path / "broken" + _write_task(dataset_dir / "task-a", "hello/task-a") + _write(dataset_dir / "task-a" / "tests" / "test.sh", "if [ ; then\n") + + from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor import ( + HarborVerifierValidationError, + ) + + with pytest.raises(HarborVerifierValidationError): + await HarborRunnerEvaluator(experiment_dir=tmp_path)._run( + agent_dir, HarborDataset.from_path(dataset_dir), HarborRunnerConfig() + ) + assert fake_job.calls == [] + + +async def test_wrong_options_type_is_rejected(tmp_path: Path, dataset: HarborDataset, agent_dir: Path) -> None: + with pytest.raises(TypeError, match="HarborRunnerConfig"): + await HarborRunnerEvaluator(experiment_dir=tmp_path)._run(agent_dir, dataset, EvaluatorConfig()) + + +# -------------------------------------------------------------------------- +# Result parity with the plain Harbor evaluator +# -------------------------------------------------------------------------- + + +async def test_both_evaluators_produce_equivalent_trials( + tmp_path: Path, + dataset: HarborDataset, + agent_dir: Path, + fake_job: type[_FakeJob], + monkeypatch: pytest.MonkeyPatch, + comparable_trials: Any, +) -> None: + """The two orchestrators differ; the trials they hand the loop must not.""" + dataset_dir = _dataset_root(dataset) + + def write_results(config: Any) -> None: + job_dir = Path(config.jobs_dir) / config.job_name + _write_trial( + job_dir, + trial_name="sum-two__0", + task_name="hello/sum-two", + task_dir=dataset_dir / "sum-two", + rewards={"reward": 1.0, "format_ok": 1.0}, + ) + _write_trial( + job_dir, + trial_name="sum-three__0", + task_name="hello/sum-three", + task_dir=dataset_dir / "sum-three", + rewards={"reward": 0.0, "format_ok": 1.0}, + ) + + fake_job.on_run = write_results + sdk_result = await HarborRunnerEvaluator(experiment_dir=tmp_path).run( + agent_dir, dataset, HarborRunnerConfig(jobs_dir=Path("sdk-jobs")) + ) + + # The plain evaluator imports Job into its own module namespace. + class PlainJob(_FakeJob): + def __init__(self, config: Any) -> None: + super().__init__(config) + self.job_dir = Path(config.jobs_dir) / config.job_name + + monkeypatch.setattr( + "nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_native.Job", + PlainJob, + ) + PlainJob.on_run = write_results + plain_result = await HarborEvaluator(experiment_dir=tmp_path).run( + agent_dir, dataset, HarborEvaluatorConfig(jobs_dir=Path("plain-jobs")) + ) + + assert comparable_trials(sdk_result.trials, include_id=True) == comparable_trials( + plain_result.trials, include_id=True + ) + assert sdk_result.aggregate_metrics == plain_result.aggregate_metrics + # Every verifier metric survives, not just the primary reward. + assert sdk_result.aggregate_metrics == {"reward": 0.5, "format_ok": 1.0} + + +async def test_failed_trials_keep_their_error_shape( + tmp_path: Path, + dataset: HarborDataset, + agent_dir: Path, + fake_job: type[_FakeJob], +) -> None: + dataset_dir = _dataset_root(dataset) + + def write_results(config: Any) -> None: + job_dir = Path(config.jobs_dir) / config.job_name + _write_trial( + job_dir, + trial_name="sum-two__0", + task_name="hello/sum-two", + task_dir=dataset_dir / "sum-two", + rewards={"reward": 1.0}, + ) + _write_trial( + job_dir, + trial_name="sum-three__0", + task_name="hello/sum-three", + task_dir=dataset_dir / "sum-three", + exception_info={"exception_type": "TimeoutError", "exception_message": "boom"}, + ) + + fake_job.on_run = write_results + trials = await HarborRunnerEvaluator(experiment_dir=tmp_path)._run( + agent_dir, dataset, HarborRunnerConfig(jobs_dir=Path("jobs")) + ) + + by_task = {trial.task_id: trial for trial in trials} + assert by_task["sum-three"].status == "failed" + assert by_task["sum-three"].error == {"type": "TimeoutError", "message": "boom"} + assert by_task["sum-two"].status == "completed" + assert by_task["sum-two"].attempt == 0 + + +# -------------------------------------------------------------------------- +# The SDK owns cache identity now — verify the plugin is actually covered by it +# -------------------------------------------------------------------------- + + +async def test_editing_the_candidate_invalidates_the_cache_through_the_sdk( + tmp_path: Path, + dataset: HarborDataset, + agent_dir: Path, + cached_job_dir: Path, + fake_job: type[_FakeJob], +) -> None: + """The staleness guard lives in the SDK; this asserts the plugin inherits it. + + Without it, editing a candidate and re-running in the same experiment directory + silently returns the previous candidate's scores — which is the whole reason + AALGO-427 exists. + """ + _write(agent_dir / "harbor_wrapper.py", "class WrappedAgent:\n version = 2\n") + + await HarborRunnerEvaluator(experiment_dir=tmp_path)._run( + agent_dir, dataset, HarborRunnerConfig(jobs_dir=Path("jobs")) + ) + + assert len(fake_job.calls) == 1, "a changed candidate must not be served from cache" + + +async def test_unchanged_candidate_still_hits_the_cache( + tmp_path: Path, + dataset: HarborDataset, + agent_dir: Path, + cached_job_dir: Path, + fake_job: type[_FakeJob], +) -> None: + """The guard must not be so strict that it defeats caching entirely.""" + await HarborRunnerEvaluator(experiment_dir=tmp_path)._run( + agent_dir, dataset, HarborRunnerConfig(jobs_dir=Path("jobs")) + ) + + assert fake_job.calls == [] diff --git a/plugins/nemo-experimentalist/tests/experimentalist/test_hello_example_baseline.py b/plugins/nemo-experimentalist/tests/experimentalist/test_hello_example_baseline.py new file mode 100644 index 0000000000..be793e2250 --- /dev/null +++ b/plugins/nemo-experimentalist/tests/experimentalist/test_hello_example_baseline.py @@ -0,0 +1,168 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Guard the hello-harbor-agent example's *deliberately failing* baseline. + +The example only teaches anything because the baseline agent has a capability +gap: it greets, but it cannot do arithmetic. The Analyzer diagnoses that gap, the +Proposer describes it, and the Coder closes it — which is the whole documented +one-round debug run. + +A well-meaning edit that adds a ``handle_sum`` to the baseline silently destroys +that: both train tasks pass, the Analyzer gets no failing trial, and the debug +config has nothing to do. It happened once. These tests are fast (no Docker, no +containers) precisely so the gap is guarded on every run, and they read the real +dataset from disk so they stay honest when tasks change. + +The reward rule mirrors ``tests/test.sh``: the agent's whole output must equal +``tests/expected.txt`` once CRLF line endings and trailing newlines are +normalized. Keeping the two in lockstep is itself asserted below. +""" + +from __future__ import annotations + +import functools +import importlib.util +import re +import sys +from pathlib import Path +from typing import Any + +import pytest + +_EXAMPLE_DIR = Path(__file__).resolve().parents[2] / "examples" / "hello-harbor-agent" +_DATASET_DIR = _EXAMPLE_DIR / "dataset" + +# The documented baseline, from README.md "The deliberate capability gap". +# reward 1.0 == the agent produced exactly the expected line. +_EXPECTED_BASELINE = { + ("train", "greet-world"): 1.0, + ("train", "sum-two"): 0.0, + ("validation", "greet-universe"): 1.0, + ("validation", "sum-three"): 0.0, +} + + +@functools.cache +def _load_hello_agent() -> Any: + """Import the example's ``agent.py`` by path; it is not an installed package. + + Cached: the module cannot change mid-session, and without this every + parametrized case re-execs it. + """ + spec = importlib.util.spec_from_file_location("_hello_example_agent", _EXAMPLE_DIR / "agent.py") + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = module + try: + spec.loader.exec_module(module) + finally: + sys.modules.pop(spec.name, None) + return module.HelloAgent + + +def _normalize(text: str) -> str: + """Mirror ``tests/test.sh``: strip CR at end-of-line only, then trailing newlines. + + Deliberately not ``text.replace("\\r", "")`` — that is the shell's + ``tr -d '\\r'``, which deletes *every* carriage return and would let + ``sum=42`` collapse into a passing ``sum=42``. + """ + return re.sub(r"\r$", "", text, flags=re.MULTILINE).rstrip("\n") + + +@functools.cache +def _reward_for(split: str, task_id: str) -> float: + """Replay the verifier's comparison in-process for one task. + + Mirrors ``tests/test.sh`` exactly, including its whole-file rule: both sides + are CRLF-normalized and stripped of trailing newlines, then compared in full. + Comparing only the first line here would let this test pass output the real + verifier rejects. + """ + task_dir = _DATASET_DIR / split / task_id + instruction = (task_dir / "instruction.md").read_text(encoding="utf-8").strip() + expected = _normalize((task_dir / "tests" / "expected.txt").read_text(encoding="utf-8")) + + # main.py writes `solve(prompt) + "\n"` to output.txt. + written = _load_hello_agent()().solve(instruction) + "\n" + return 1.0 if _normalize(written) == expected else 0.0 + + +@pytest.mark.parametrize(("split", "task_id", "expected_reward"), [(*k, v) for k, v in _EXPECTED_BASELINE.items()]) +def test_baseline_scores_match_the_documented_capability_gap( + split: str, + task_id: str, + expected_reward: float, +) -> None: + assert _reward_for(split, task_id) == expected_reward + + +@pytest.mark.parametrize("split", ["train", "validation"]) +def test_every_split_keeps_one_passing_and_one_failing_task(split: str) -> None: + """Both splits must average 0.5 — and for the same structural reason. + + An aggregate of 1.0 means the gap was closed in the baseline; 0.0 means the + greeting handler broke. Either way the example stops demonstrating what its + README says it demonstrates. + """ + expected_ids = {task_id for (s, task_id) in _EXPECTED_BASELINE if s == split} + + # Check the split on disk before scoring it. `rewards` below only walks + # _EXPECTED_BASELINE, so a task added to or removed from the dataset would leave + # every assertion here passing while the split no longer has one pass, one + # failure, or the 0.5 aggregate the README documents. + actual_ids = {path.name for path in (_DATASET_DIR / split).iterdir() if (path / "task.toml").is_file()} + assert actual_ids == expected_ids, ( + f"{split} split's task directories drifted from the documented baseline: " + f"missing={sorted(expected_ids - actual_ids)} unexpected={sorted(actual_ids - expected_ids)}" + ) + + rewards = {task_id: _reward_for(split, task_id) for task_id in sorted(expected_ids)} + + # For a two-task split this also pins the 0.5 aggregate both evaluators report. + assert sorted(rewards.values()) == [0.0, 1.0], f"{split} split lost its one-pass/one-fail shape: {rewards}" + + +def test_baseline_agent_exposes_no_arithmetic_handler() -> None: + """The gap must be a real missing capability, not a regex that happens to miss. + + ``handle_sum`` is the exact method name the Proposer's ``add_concrete_method`` + improvement introduces in round 1, so its presence on the baseline means a + previous run's output leaked back into the example. + """ + agent = _load_hello_agent()() + + assert not hasattr(agent, "handle_sum"), ( + "the baseline agent must not implement handle_sum — that is the round-1 " + "improvement the Coder is supposed to write (see README.md)" + ) + + +def test_unhandled_tasks_fall_through_to_the_fallback() -> None: + """A failing task must fail by falling through, which is what the Analyzer reads.""" + module_agent = _load_hello_agent() + instruction = (_DATASET_DIR / "train" / "sum-two" / "instruction.md").read_text(encoding="utf-8").strip() + + answer = module_agent().solve(instruction) + + assert answer == "I do not know how to answer that." + + +@pytest.mark.parametrize( + ("written", "expected_reward"), + [ + ("sum=42\n", 1.0), + ("sum=42\r\n", 1.0), # CRLF line ending is legitimate + ("sum=4\r2\n", 0.0), # embedded CR must not collapse into a match + ("sum=42\nextra\n", 0.0), # trailing content must not be ignored + ], +) +def test_normalization_matches_the_shell_verifier(written: str, expected_reward: float) -> None: + """Keep `_normalize` in lockstep with `sed 's/\\r$//'` in tests/test.sh. + + Both rules are reward-hacking guards; if this helper ever drifts back to + stripping every CR, the in-process tests would pass output the container + verifier rejects. + """ + assert (1.0 if _normalize(written) == _normalize("sum=42\n") else 0.0) == expected_reward diff --git a/plugins/nemo-experimentalist/tests/experimentalist/test_loop_failure.py b/plugins/nemo-experimentalist/tests/experimentalist/test_loop_failure.py index b5f17225eb..d6cf50387d 100644 --- a/plugins/nemo-experimentalist/tests/experimentalist/test_loop_failure.py +++ b/plugins/nemo-experimentalist/tests/experimentalist/test_loop_failure.py @@ -71,7 +71,7 @@ async def test_baseline_failure_marks_run_failed(monkeypatch, tmp_path, failure_ backend=backend, workspace="default", config=config, - evaluator_type="harbor", + evaluator_type="harbor_native", train_dataset=object(), validation_dataset=object(), insight=None, diff --git a/plugins/nemo-experimentalist/tests/experimentalist/test_loop_insight_suite.py b/plugins/nemo-experimentalist/tests/experimentalist/test_loop_insight_suite.py index e959de65be..b478ed9f0e 100644 --- a/plugins/nemo-experimentalist/tests/experimentalist/test_loop_insight_suite.py +++ b/plugins/nemo-experimentalist/tests/experimentalist/test_loop_insight_suite.py @@ -16,7 +16,7 @@ Task, TrialResult, ) -from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor import HarborEvaluatorConfig +from nemo_experimentalist_plugin.experimentalist.components.evaluator.harbor_native import HarborEvaluatorConfig from nemo_experimentalist_plugin.experimentalist.components.evaluator.models import ( DatasetRef, DataValue, @@ -227,7 +227,7 @@ async def run(self, **kwargs: object) -> SimpleNamespace: backend=backend, workspace="default", config=config, - evaluator_type="harbor", + evaluator_type="harbor_native", train_dataset=DatasetRef(uri="train"), validation_dataset=DatasetRef(uri="validation"), task_template=DatasetRef(uri="template"), diff --git a/plugins/nemo-experimentalist/tests/test_deps.py b/plugins/nemo-experimentalist/tests/test_deps.py index cb509f95dd..32a4c6c360 100644 --- a/plugins/nemo-experimentalist/tests/test_deps.py +++ b/plugins/nemo-experimentalist/tests/test_deps.py @@ -8,6 +8,7 @@ import pytest from nemo_experimentalist_plugin.experimentalist.components.evaluator.models import DatasetRef from nemo_experimentalist_plugin.experimentalist.deps import ExperimentalistDeps +from pydantic import ValidationError def _datasets(tmp_path: Path) -> dict: @@ -53,3 +54,14 @@ def test_insight_without_task_template_raises(tmp_path: Path) -> None: def test_neither_raises(tmp_path: Path) -> None: with pytest.raises(ValueError, match="must be set"): ExperimentalistDeps(**_datasets(tmp_path)) + + +def test_deprecated_evaluator_alias_is_rejected(tmp_path: Path) -> None: + with pytest.raises(ValidationError): + ExperimentalistDeps.model_validate( + { + "agent": "ssh://git@h/g/r.git@main", + "evaluator_type": "harbor", + **_datasets(tmp_path), + } + ) diff --git a/plugins/nemo-experimentalist/tests/test_e2e_helper_defaults.py b/plugins/nemo-experimentalist/tests/test_e2e_helper_defaults.py new file mode 100644 index 0000000000..f1334cf4ac --- /dev/null +++ b/plugins/nemo-experimentalist/tests/test_e2e_helper_defaults.py @@ -0,0 +1,17 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Guards checked-in Experimentalist example configs that select an evaluator.""" + +from pathlib import Path + +import yaml +from nemo_experimentalist_plugin.resolve import EvolutionaryOptimizerConfig + + +def test_tau3_smoke_config_uses_sdk_harbor_evaluator() -> None: + config_path = Path(__file__).parents[1] / "examples" / "tau3-nooa-agent" / "experimentalist-smoke.yaml" + + config = EvolutionaryOptimizerConfig.model_validate(yaml.safe_load(config_path.read_text(encoding="utf-8"))) + + assert config.evaluator_type == "harbor_evaluator" diff --git a/sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/harbor_runtime.py b/sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/harbor_runtime.py index e04dd9b35d..20d56349d3 100644 --- a/sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/harbor_runtime.py +++ b/sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/harbor_runtime.py @@ -32,9 +32,11 @@ from __future__ import annotations import contextlib +import hashlib import importlib.machinery import json import logging +import os import re import shutil import sys @@ -45,7 +47,6 @@ from pathlib import Path from types import ModuleType from typing import Any -from uuid import uuid4 from nemo_platform.beta.evaluator.agent_eval.results import AgentEvalResult from nemo_platform.beta.evaluator.agent_eval.scores import AgentEvalScoreStatus @@ -71,6 +72,42 @@ _AGENT_IMPORT_ROOT = "_nemo_evaluator_harbor_agents" # Guards the sys.modules mutation while injecting/removing scoped agent packages. _IMPORT_LOCK = threading.Lock() +# Open scopes per content-addressed agent package. Identical agent contents share a +# package name, so teardown must wait for the last scope rather than the first. +_AGENT_PACKAGE_REFCOUNTS: dict[str, int] = {} +# Characters of the agent-content digest used to disambiguate the package name. Long +# enough that distinct agents don't collide; short enough to keep import paths (and +# Harbor's persisted JobConfig) readable. +_IMPORT_DIGEST_CHARS = 12 +# Records which inputs produced a job dir, so a rerun can tell a reusable cache from +# a stale one. A file, not a directory: Harbor rmtree's stray directories in a job dir. +CACHE_STAMP_FILENAME = ".nemo-eval-harbor-cache.json" +# Public so downstreams can assert the SDK is new enough to own cache staleness. +CACHE_STAMP_VERSION = 1 +# Excluded from the cache fingerprint — see :func:`_cache_stamp` for why each one. +_CACHE_IRRELEVANT_OPTIONS = frozenset( + {"jobs_dir", "job_name", "force_rerun", "quiet", "n_concurrent_trials", "agent_dir", "reward_key"} +) +# Where Harbor persists the JobConfig it will compare a resume against. +_HARBOR_JOB_CONFIG_FILENAME = "config.json" +# Fields Harbor's own JobConfig equality ignores, so they can never be why it refused +# to resume. Pinned against Harbor upstream by the drift-guard test. +_HARBOR_EQ_IGNORED_FIELDS = frozenset({"job_name", "debug"}) +# How Harbor says "this job dir cannot be resumed": one from the JobConfig comparison +# in `Job.create`, one from the lock.json check early in `Job.run`. Matching on the +# message is deliberate coupling, and it fails in the safe direction — an +# unrecognized FileExistsError propagates untouched rather than costing a job dir, so +# a Harbor reword degrades to a loud crash, never to a silent deletion. +_HARBOR_RESUME_REFUSALS = ("resumed with a different config", "does not match the resolved job lock") +# Cap on each value rendered into the "what differed" log line: enough for a scalar +# like `n_concurrent_trials`, bounded for a whole nested `agents` list. +_DRIFT_VALUE_CHARS = 80 +# Derived/VCS noise skipped when digesting a directory. Deliberately NOT skipped: +# `node_modules` and other vendored dependency trees, which ship with the agent and +# change what it does. `.venv`/`.uv` stay skipped because they are environment, not +# deliverable — the Harbor wrapper does not upload them into the task container. +_DIGEST_SKIP_DIRS = frozenset({".git", "__pycache__", ".venv", ".uv", ".mypy_cache", ".pytest_cache"}) +_DIGEST_CHUNK_BYTES = 1 << 20 RunJob = Callable[[], Awaitable[None]] @@ -209,17 +246,78 @@ async def run_tasks( In native mode the dataset directory is recovered from the tasks (each carries ``metadata['harbor_dataset_path']`` from :func:`discover_harbor_tasks`) unless a ``dataset_path`` override was given, - so callers don't repeat it. ``job_dir`` doubles as a cache: the Harbor run - is skipped and results are simply re-adapted when every requested task - already has ``n_attempts`` completed, non-errored results there (unless - ``force_rerun`` is set). The cache only engages when the config pins a - stable ``job_name``; the default timestamped name never hits it. + so callers don't repeat it. + + ``job_dir`` doubles as a cache. Results are served straight off it, without + importing Harbor at all, only when **both** hold: every requested task already + has ``n_attempts`` completed, non-errored results there, *and* the directory + carries a cache stamp matching this run's inputs (agent contents, task + contents, result-affecting options). + + Otherwise Harbor runs, and what happens to the directory depends on *which* + check failed. A **stamp mismatch** discards it first: those results came from + different inputs, so there is nothing safe to resume onto. A directory that + merely lacks **coverage** — stamp matches, but not enough completed results — + is handed to Harbor intact so its per-trial resume keeps the finished trials + and runs only what is missing. Harbor may still refuse a directory on its own + (stricter) terms; :func:`_build_native_job` then discards it and re-runs. + + The cache only engages when the config pins a stable ``job_name``; with the + default timestamped name no fingerprint is computed at all. + + Assumes a **single writer per job directory**. Neither this runtime nor + Harbor locks it, so two processes sharing a pinned ``job_name`` on a shared + volume will race. """ if self._config is not None: dataset_path = self._dataset_path or _dataset_path_from_tasks(tasks) - job_dir, run_job = _build_native_job(self._config, dataset_path, self._task_names) - if self._config.force_rerun or not _all_tasks_cached(job_dir, tasks, n_attempts=self._config.n_attempts): + job_name, job_dir = _resolve_job_dir(self._config) + + # Only fingerprint when the answer can depend on it: an unpinned job name + # can never hit, and force_rerun/a missing dir already decided. This keeps + # the digest I/O off every run of callers that don't pin a job name. + stamp: dict[str, Any] | None = None + if self._config.job_name is None or self._config.force_rerun or not job_dir.is_dir(): + stale = True + else: + stamp = _cache_stamp(self._config, dataset_path, tasks) + stale = _cache_is_stale(job_dir, stamp) + + if stale or not _all_tasks_cached(job_dir, tasks, n_attempts=self._config.n_attempts): + job_dir, run_job = _build_native_job( + self._config, + dataset_path, + self._task_names, + job_name=job_name, + # Discard only when the inputs changed. Otherwise leave it off so + # Harbor resumes per trial and keeps completed work — including + # with `agent_dir` set, now that the scoped import path is + # content-addressed rather than a fresh uuid per run and Harbor's + # JobConfig comparison can therefore match (AALGO-430). + force_rerun=(self._config.force_rerun or stale), + ) + # Fingerprint the inputs *before* running and confirm they are + # unchanged afterwards. Stamping only the post-run state would label + # results produced from the old sources with the new fingerprint, so + # a later run would happily serve them. Covers what Harbor actually + # ran: with no task_names filter that is the whole dataset, and + # recording only the requested subset would make the next full-set + # run look stale and re-run a complete job dir. + coverage = _stamp_coverage(dataset_path, tasks, self._task_names) + before = _cache_stamp(self._config, dataset_path, coverage) await run_job() + if self._config.job_name is not None: + after = _cache_stamp(self._config, dataset_path, coverage) + if after == before: + _write_cache_stamp(job_dir, after) + else: + # Leaving it unstamped re-runs next time, which is the safe + # direction: we cannot say which inputs produced these results. + logger.warning( + "Agent or task contents changed while Harbor job %s was running; leaving it unstamped " + "so the next run re-executes rather than trusting these results.", + job_dir, + ) return build_trials_from_job_dir(job_dir, tasks, reward_key=self._reward_key) if self._job_dir is None: # unreachable: __init__ guarantees config or job_dir @@ -268,10 +366,352 @@ def _all_tasks_cached(job_dir: Path, tasks: Sequence[AgentEvalTask], *, n_attemp return all(counts.get(task.id, 0) >= n_attempts for task in tasks) +def _feed(digest: "hashlib._Hash", label: bytes, payload: bytes) -> None: + """Append a length-framed field to ``digest``. + + Framing matters: concatenating ``name \0 content \0`` is ambiguous because file + *contents* may contain NUL, so two different trees can produce an identical byte + stream. Prefixing every variable-length field with its length makes the encoding + injective, which is what stops a collision from being read as "unchanged". + """ + digest.update(label) + digest.update(len(payload).to_bytes(8, "big")) + digest.update(payload) + + +def _safe_resolve(path: Path) -> Path: + """``Path.resolve()`` that degrades instead of raising. + + The digest is a best-effort guard, not a reason to fail a run that would + otherwise succeed, so fall back to the unresolved absolute path. + + ``RuntimeError`` is caught alongside ``OSError`` and is the case that actually + fires: on CPython 3.12 — the floor this package targets — a **symlink loop** + surfaces as ``RuntimeError("Symlink loop from ...")``, because ``resolve()`` + translates ``ELOOP`` before re-raising. It is not an ``OSError``, so catching + only that would let a loop under a task directory kill the run. A loop raises + deterministically, not as a race. ``OSError`` covers the narrower case of a + symlink that disappears mid-walk, since ``resolve()`` calls ``os.readlink``. + + Both are 3.12/3.13 behaviours: 3.14 resolves a loop without raising at all. + """ + try: + return path.resolve() + except (OSError, RuntimeError): + return path.absolute() + + +def _is_executable(path: Path) -> bool: + """Whether the owner-execute bit is set, following symlinks. + + Only the execute bit, mirroring what git tracks: read/write bits vary with umask + and would evict the cache for nothing, but flipping +x on ``tests/test.sh`` or an + agent entrypoint genuinely changes what Harbor does. + """ + try: + return bool(path.stat().st_mode & 0o100) + except OSError: + return False + + +def _digest_directory(root: Path, *, exclude: frozenset[Path] = frozenset()) -> str: + """Content-hash a directory tree, skipping build/VCS noise and excluded roots. + + Contents rather than mtimes: callers routinely materialize the directory with + ``copytree`` (the optimizer does, per candidate), which rewrites every mtime and + would defeat the cache entirely. + + ``exclude`` takes *resolved* directories to skip wholesale. It exists because + ``jobs_dir`` is caller-chosen and may sit **under** the dataset or agent + directory; without excluding it the digest would hash the growing results tree + it is meant to validate, and would never stabilize. + + Symlinks are **followed**, not skipped. Skipping them silently defeats the whole + guard: a directory assembled out of links to shared sources would hash to the + empty digest, so edits behind those links would never invalidate the cache. The + link target is folded in alongside the contents, so re-pointing a link is a + change even when both targets happen to hold identical bytes. + + Unreadable or vanished files are folded in as a marker rather than raised: a + transient read failure must not kill a run that would otherwise have succeeded. + """ + digest = hashlib.sha256() + if not root.is_dir(): + return digest.hexdigest() + # Keep only exclusions strictly *inside* this tree. An excluded root that is an + # ancestor of (or equal to) `root` would otherwise match every entry and yield + # an empty digest — silently disabling invalidation for the whole directory, + # which is exactly the failure this function exists to prevent. jobs_dir being + # a parent of the agent/task dir is a legitimate layout, not a reason to stop + # hashing it. + root_resolved = _safe_resolve(root) + excluded = { + resolved + for resolved in (_safe_resolve(path) for path in exclude) + if resolved != root_resolved and resolved.is_relative_to(root_resolved) + } + # Resolved dirs already walked, so a symlink cycle terminates instead of hanging. + visited: set[Path] = set() + + def walk(directory: Path) -> None: + resolved_dir = _safe_resolve(directory) + if resolved_dir in visited: + return + visited.add(resolved_dir) + try: + entries = sorted(directory.iterdir()) + except OSError as exc: + logger.warning("Could not list %s while fingerprinting %s: %s", directory, root, exc) + _feed(digest, b"unlistable", b"") + return + for path in entries: + if path.name in _DIGEST_SKIP_DIRS: + continue + resolved = _safe_resolve(path) + if any(resolved == item or item in resolved.parents for item in excluded): + continue + + _feed(digest, b"name", path.relative_to(root).as_posix().encode("utf-8")) + _feed(digest, b"mode", b"x" if _is_executable(path) else b"-") + if path.is_symlink(): + # Record where the link points, so retargeting counts as a change. + try: + target = os.readlink(path).encode("utf-8") + except OSError as exc: + # The link vanished mid-walk. Same contract as an unreadable + # file: degrade to a marker rather than kill the run. + logger.warning("Could not read link %s while fingerprinting %s: %s", path, root, exc) + target = b"" + _feed(digest, b"symlink", target) + if path.is_dir(): + _feed(digest, b"dir", b"") + walk(path) + continue + if not path.is_file(): + # Broken link, socket, fifo: nothing to hash, but its presence counts. + _feed(digest, b"not-a-file", b"") + continue + content = hashlib.sha256() + try: + with path.open("rb") as handle: + # Streamed: Harbor datasets may ship large seeds or build contexts. + for chunk in iter(lambda: handle.read(_DIGEST_CHUNK_BYTES), b""): + content.update(chunk) + except OSError as exc: + logger.warning("Could not read %s while fingerprinting %s: %s", path, root, exc) + content.update(b"") + # The sub-digest is fixed width, so file bytes can never be confused with + # the framing around them. + _feed(digest, b"file", content.digest()) + + walk(root) + return digest.hexdigest() + + +def _task_dirs_for(dataset_path: Path, tasks: Sequence[AgentEvalTask]) -> dict[str, Path | None]: + """Resolve each task's on-disk directory, falling back to re-discovery. + + :func:`discover_harbor_tasks` stamps ``metadata['harbor_task_dir']``, but callers + may build :class:`AgentEvalTask` objects by hand (the Evaluator plugin builds + them from a job spec), so the metadata is not guaranteed. + + A task that cannot be resolved maps to ``None`` — the caller must treat that as + un-cacheable rather than silently omitting it from the fingerprint, which would + be a stale-cache hole. + """ + # `_safe_resolve`, not bare `resolve()`: this walk is a best-effort cache guard, so + # a symlink that vanishes mid-run must degrade to an unresolved absolute path + # rather than raise out of a job that would otherwise succeed. + dataset_root = _safe_resolve(dataset_path) + resolved: dict[str, Path | None] = {} + for task in tasks: + stamped = task.metadata.get("harbor_task_dir") + candidate = Path(stamped) if isinstance(stamped, str) and stamped else None + # The stamp records where a task was *discovered*, which is not necessarily + # where this run executes it: `dataset_path` can be overridden on the runner. + # Trusting a stale or foreign path would fingerprint one dataset while Harbor + # runs another, so anything missing or outside the active dataset is dropped + # and re-discovered below. + if candidate is not None: + candidate_resolved = _safe_resolve(candidate) + if not candidate.is_dir() or not candidate_resolved.is_relative_to(dataset_root): + logger.debug( + "Ignoring stamped harbor_task_dir %s for task %r: not a directory under the active dataset %s", + candidate, + task.id, + dataset_root, + ) + candidate = None + resolved[task.id] = candidate + if all(path is not None for path in resolved.values()): + return resolved + + try: + discovered = { + task.id: Path(str(task.metadata["harbor_task_dir"])) for task in discover_harbor_tasks(dataset_path) + } + except (OSError, ValueError) as exc: + # discover_harbor_tasks raises on ANY malformed task.toml in the dataset. + # Refusing the cache is the safe reading; failing the run is not, since this + # path previously never read those files. + logger.warning( + "Could not resolve Harbor task dirs under %s; treating the cache as stale: %s", dataset_path, exc + ) + return dict.fromkeys(resolved, None) + return {task_id: path or discovered.get(task_id) for task_id, path in resolved.items()} + + +def _stamp_coverage( + dataset_path: Path, + tasks: Sequence[AgentEvalTask], + task_names: Sequence[str] | None, +) -> Sequence[AgentEvalTask]: + """Tasks a written stamp must cover: everything Harbor was asked to run. + + ``task_names`` is the filter handed to Harbor's ``DatasetConfig``. When it is + ``None`` Harbor runs every task in the dataset, which can be a superset of the + tasks this call was asked to score — and a stamp that recorded only the smaller + set would report the larger one as stale on the next run. + """ + if task_names is not None: + return tasks + try: + discovered = discover_harbor_tasks(dataset_path) + except (OSError, ValueError): + # Same reasoning as _task_dirs_for: a malformed sibling task must not fail a + # run. Recording only the requested tasks just costs a re-run later. + return tasks + covered = {task.id: task for task in discovered} + covered.update({task.id: task for task in tasks}) + return list(covered.values()) + + +def _cache_stamp( + config: HarborRuntimeConfig, + dataset_path: Path, + tasks: Sequence[AgentEvalTask], +) -> dict[str, Any]: + """Fingerprint the inputs that decide whether a job dir can be reused. + + Covers the result-affecting options, the contents of ``agent_dir``, and the + contents of every task directory. Two gaps are deliberate and worth knowing + before trusting a hit: when ``agent_dir`` is ``None`` the agent is an already + importable module, so only its *import path* is fingerprinted and edits to that + installed package are invisible; and a task whose directory cannot be resolved + is recorded as ````, which always forces a re-run. + + Recorded per task rather than as one job-wide hash so that evaluating a + **subset** of a previously-cached job still hits: staleness is decided only over + the tasks actually requested. + + Excluded from the option hash: presentation and placement knobs (``quiet``, + ``n_concurrent_trials``, ``jobs_dir``, ``job_name``, ``force_rerun``), which + change nothing about the results; ``agent_dir``, an absolute path whose + *content* is hashed separately, so a relocated but identical agent still hits; + and ``reward_key``, which only selects which reward + :func:`build_trials_from_job_dir` reads back and must not cost a Docker re-run. + """ + options = config.model_dump(exclude=set(_CACHE_IRRELEVANT_OPTIONS), mode="json") + # `_safe_resolve` throughout, matching `_task_dirs_for`: fingerprinting is + # best-effort, so a symlink loop or a vanished link under any of these must + # degrade to an unresolved path rather than raise out of `run_tasks` and fail a + # run that would otherwise succeed. + excluded_roots = frozenset({_safe_resolve(config.jobs_dir.expanduser())}) + + agent_digest = "" + if config.agent_dir is not None: + agent_digest = _digest_directory(_safe_resolve(config.agent_dir.expanduser()), exclude=excluded_roots) + + task_digests: dict[str, str] = {} + for task_id, task_dir in sorted(_task_dirs_for(dataset_path, tasks).items()): + task_digests[task_id] = ( + "" if task_dir is None else _digest_directory(_safe_resolve(task_dir), exclude=excluded_roots) + ) + + return { + "version": CACHE_STAMP_VERSION, + "options": hashlib.sha256(json.dumps(options, sort_keys=True, default=str).encode("utf-8")).hexdigest(), + "agent": agent_digest, + "tasks": task_digests, + } + + +def _cache_is_stale(job_dir: Path, stamp: Mapping[str, Any]) -> bool: + """Return True when ``job_dir`` was not produced by the inputs in ``stamp``. + + A directory with no stamp is stale: it predates this check, or was written by + plain Harbor, and re-running is the safe reading. An ```` task + digest is likewise always stale — we could not prove the inputs match. A + directory that does not exist is stale too: there is nothing there to reuse, and + answering "not stale" would be an invitation to serve zero trials. + """ + if not job_dir.is_dir(): + return True + try: + stored = json.loads((job_dir / CACHE_STAMP_FILENAME).read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError): + stored = None + + reason: str | None = None + if not isinstance(stored, Mapping): + reason = "no usable cache stamp" + elif stored.get("version") != stamp["version"]: + reason = "cache stamp version changed" + elif stored.get("options") != stamp["options"]: + reason = "a result-affecting option changed" + elif stored.get("agent") != stamp["agent"]: + reason = "the agent directory changed" + else: + stored_tasks = stored.get("tasks") + stored_tasks = stored_tasks if isinstance(stored_tasks, Mapping) else {} + for task_id, digest in stamp["tasks"].items(): + if digest == "": + reason = f"task {task_id!r} could not be resolved on disk" + break + if stored_tasks.get(task_id) != digest: + reason = f"task {task_id!r} changed or was not part of the cached run" + break + + if reason is None: + return False + logger.info("Re-running Harbor job %s instead of serving it from cache: %s.", job_dir, reason) + return True + + +def _write_cache_stamp(job_dir: Path, stamp: Mapping[str, Any]) -> None: + """Record the inputs a completed job dir was produced from. + + Best-effort: a job dir that could not be stamped simply re-runs next time, which + is the safe direction. Written as a *file* deliberately — Harbor deletes any + stray *directory* in a job dir that lacks ``result.json``. + """ + try: + (job_dir / CACHE_STAMP_FILENAME).write_text( + json.dumps(stamp, indent=2, sort_keys=True) + "\n", encoding="utf-8" + ) + except OSError as exc: + logger.warning("Could not stamp Harbor job dir %s with its cache key: %s", job_dir, exc) + + +def _resolve_job_dir(config: HarborRuntimeConfig) -> tuple[str, Path]: + """Resolve ``(job_name, job_dir)`` without importing Harbor. + + Split out from :func:`_build_native_job` because the caller must know the job + directory *before* deciding whether to run: an unpinned ``job_name`` is a + timestamp with microsecond precision, so resolving it twice would yield two + different directories and the cache decision would be made about the wrong one. + """ + job_name = config.job_name or datetime.now(timezone.utc).strftime("%Y-%m-%d__%H-%M-%S__%f") + return job_name, config.jobs_dir / job_name + + def _build_native_job( config: HarborRuntimeConfig, dataset_path: Path, task_names: Sequence[str] | None, + *, + job_name: str | None = None, + force_rerun: bool | None = None, ) -> tuple[Path, RunJob]: """Build a Harbor ``JobConfig`` from ``config`` and return ``(job_dir, run_job)``. @@ -280,9 +720,18 @@ def _build_native_job( without importing Harbor. When ``agent_import_path`` is set, ``run_job`` scopes the user's agent package into ``sys.modules`` for the run and removes it afterwards (see :func:`scoped_harbor_agent_import`). + + Args: + job_name: Pre-resolved job name from :func:`_resolve_job_dir`. Pass it when + the caller already resolved the directory, so an unpinned name is not + re-generated into a different timestamp. + force_rerun: Overrides ``config.force_rerun`` for this build. Passed rather + than applied via ``model_copy`` so the caller's config is never mutated + and the job name stays fixed. """ - job_name = config.job_name or datetime.now(timezone.utc).strftime("%Y-%m-%d__%H-%M-%S__%f") - job_dir = config.jobs_dir / job_name + resolved_name = job_name if job_name is not None else _resolve_job_dir(config)[0] + job_dir = config.jobs_dir / resolved_name + effective_force_rerun = config.force_rerun if force_rerun is None else force_rerun async def run_job() -> None: try: @@ -295,7 +744,7 @@ async def run_job() -> None: '(it requires Python >=3.12). Install it separately: uv pip install "harbor>=0.16.1"' ) from exc - if config.force_rerun and job_dir.exists(): + if effective_force_rerun and job_dir.exists(): shutil.rmtree(job_dir) artifacts: list[str | ArtifactConfig] = list(config.artifacts) @@ -316,7 +765,7 @@ async def run_job() -> None: async def _create_and_run(agent: Any) -> None: job_config = JobConfig( - job_name=job_name, + job_name=resolved_name, jobs_dir=config.jobs_dir, n_attempts=config.n_attempts, n_concurrent_trials=config.n_concurrent_trials, @@ -327,15 +776,52 @@ async def _create_and_run(agent: Any) -> None: datasets=[DatasetConfig(path=dataset_path, task_names=list(task_names) if task_names else None)], **timeout_kwargs, ) - job = await Job.create(job_config) - await job.run() + + async def _attempt() -> None: + job = await Job.create(job_config) + await job.run() + + try: + await _attempt() + except FileExistsError as exc: + # Harbor refuses to resume a job dir whose persisted `config.json` or + # `lock.json` differs from this run's — and it refuses by raising, not + # by re-running. Its comparison is deliberately stricter than the SDK + # cache stamp: `quiet`, `n_concurrent_trials` and the `task_names` + # filter all change the JobConfig without changing the results, so the + # stamp excludes them (a full cache hit must not pay for a concurrency + # tweak) while Harbor still rejects the directory. Honour the intent of + # the rerun rather than propagating a crash. + # + # Identify the refusal positively before deleting anything. Both of + # Harbor's refusals fire before any trial executes, so discarding costs + # only completed work — but that reasoning holds *only* for those two. + # An ordinary "file exists" raised from inside a trial, a hook, or an + # environment build must not be mistaken for drift and answered by + # destroying the directory. + if not (job_dir.exists() and _is_harbor_resume_refusal(exc, job_dir)): + raise + drift = _describe_job_config_drift(job_dir, job_config) + logger.warning( + "Harbor refused to resume job dir %s, so it is being discarded and re-run from scratch: %s%s", + job_dir, + exc, + f" Differing config: {drift}." if drift else "", + ) + shutil.rmtree(job_dir) + await _attempt() if config.agent_import_path is None: await _create_and_run(AgentConfig(name=config.agent_name or "oracle", model_name=config.agent_model_name)) elif config.agent_dir is not None: - # Loose wrapper file: make its directory importable for the run. + # Loose wrapper file: make its directory importable for the run. The + # jobs_dir exclusion must match _cache_stamp's, or a jobs_dir nested under + # agent_dir would shift the package name as results accumulate. agent_dir = config.agent_dir.expanduser().resolve() - with scoped_harbor_agent_import(agent_dir, config.agent_import_path) as scoped_import: + excluded_roots = frozenset({config.jobs_dir.expanduser().resolve()}) + with scoped_harbor_agent_import( + agent_dir, config.agent_import_path, exclude=excluded_roots + ) as scoped_import: await _create_and_run(AgentConfig(import_path=scoped_import, model_name=config.agent_model_name)) else: # Already-importable module (installed package): let Harbor import it directly. @@ -344,13 +830,73 @@ async def _create_and_run(agent: Any) -> None: return job_dir, run_job +def _is_harbor_resume_refusal(exc: FileExistsError, job_dir: Path) -> bool: + """Return True when ``exc`` is Harbor declining to resume ``job_dir``. + + Separates Harbor's refusal — the one case where deleting the directory is the + right answer — from an ordinary "file exists" surfacing from a trial, a hook or an + environment build, where deleting it would destroy completed work to no purpose. + + Two signals, both required. Harbor constructs its refusals with a bare message, so + ``errno`` is unset, while an OS-level ``EEXIST`` always carries one; and both + refusals name the job directory and end in a known phrase. + """ + if exc.errno is not None: + return False + message = str(exc) + return str(job_dir) in message and any(phrase in message for phrase in _HARBOR_RESUME_REFUSALS) + + +def _describe_job_config_drift(job_dir: Path, job_config: Any) -> str: + """Name the fields that differ between ``job_dir``'s persisted JobConfig and this run's. + + Harbor reports *that* an existing config differs, never *which* field, which + leaves the resulting discard looking arbitrary. This reproduces enough of its + comparison to say — turning "Harbor refused" into "n_concurrent_trials: 10 -> 4". + + Best-effort by construction. Returns ``""`` when the difference cannot be + located: no ``config.json``, unparseable, or a refusal that came from + ``lock.json`` instead, which has no JobConfig difference to report. Diagnostics + must never mask the failure they explain, so every error here is swallowed. + """ + try: + stored_text = (job_dir / _HARBOR_JOB_CONFIG_FILENAME).read_text(encoding="utf-8") + # Harbor persists with exclude_defaults=True, so the JSON omits every field + # left at its default and comparing it raw would report phantom differences. + # Round-tripping through the model refills them, which is what Harbor itself + # compares after re-validating the stored config. + stored = type(job_config).model_validate_json(stored_text).model_dump() + current = job_config.model_dump() + return ", ".join( + f"{field}: {_truncated_repr(stored.get(field))} -> {_truncated_repr(value)}" + for field, value in current.items() + if field not in _HARBOR_EQ_IGNORED_FIELDS and stored.get(field) != value + ) + except Exception: + return "" + + +def _truncated_repr(value: Any) -> str: + """Render ``value`` for a log line, bounded so a nested config can't flood it.""" + text = repr(value) + return text if len(text) <= _DRIFT_VALUE_CHARS else f"{text[:_DRIFT_VALUE_CHARS]}..." + + @contextlib.contextmanager -def scoped_harbor_agent_import(agent_dir: Path, import_path: str) -> Iterator[str]: - """Make ``agent_dir`` importable under a unique synthetic package for the block. +def scoped_harbor_agent_import( + agent_dir: Path, import_path: str, *, exclude: frozenset[Path] = frozenset() +) -> Iterator[str]: + """Make ``agent_dir`` importable under a content-addressed package for the block. Args: agent_dir: directory containing the module referenced by ``import_path``. import_path: Harbor agent path, ``"module"`` or ``"module:attribute"``. + exclude: resolved directories to leave out of the content digest. Pass the + same set :func:`_cache_stamp` uses — in practice ``jobs_dir``, which is + caller-chosen and may sit *under* ``agent_dir``. Omitting it lets the + growing results tree feed the package name, so the import path would + change on every run and the resume this function exists to enable would + never happen. See :func:`_digest_directory`. Yields: str: the rewritten import path Harbor should load (the module rooted under @@ -359,9 +905,41 @@ def scoped_harbor_agent_import(agent_dir: Path, import_path: str) -> Iterator[st Raises: ValueError: if ``import_path`` has no module component. - On exit the injected ``sys.modules`` entries are removed. The mutation is - guarded by a process-wide lock so concurrent runs don't corrupt import state; - each run gets its own uniquely-named package so distinct agents never collide. + **The package name is derived from the directory's contents, not a random + UUID, and that is load-bearing.** This string becomes ``AgentConfig.import_path`` + and therefore part of Harbor's ``JobConfig``, which Harbor compares field-by-field + when deciding whether an existing job directory may be resumed. A random suffix + made that comparison fail on every rerun, so Harbor raised ``FileExistsError`` + instead of resuming and its per-trial resume was unreachable for any caller that + sets ``agent_dir`` (AALGO-430). Content-addressing keeps distinct agents isolated + while letting an unchanged agent resume — and makes an *edited* agent invalidate + the job dir on Harbor's own terms. + + Identical contents therefore share a package name, so overlapping scopes are + refcounted: the injected ``sys.modules`` entries are removed when the last + scope exits, not the first (see :func:`_uninstall_agent_package`). The mutation + is guarded by a process-wide lock. ``sys.modules`` is per-process, so concurrent + *processes* were never at risk here. + + The name is ``_``, so it tracks the directory's *location* as + well as its contents — deliberately, because an opaque hash makes every traceback + and import error unreadable. That is a narrow, knowing divergence from the cache + stamp, which excludes ``agent_dir`` so a relocated but identical agent still hits + (see :func:`_cache_stamp`). Relocating an agent while pinning the same + ``job_name`` therefore leaves the stamp valid but changes this string, and Harbor + declines to resume; :func:`_build_native_job` absorbs that into a clean re-run. + The results stay correct — it costs one repeated job. Callers that rebuild agents + under changing directory names (the Experimentalist does) are unaffected, because + the agent name feeds their ``job_name`` too, so a rename lands in a different job + dir with nothing to resume. + + **That last sentence only holds if the caller derives its job name from the + *resolved* directory, as this function does.** Deriving it from the caller's + spelling instead lets the two disagree: a symlink keeps its own name while + resolving elsewhere, so flipping it at a fixed ``job_name`` would reuse one job + dir for two different agents, caught only by Harbor's refusal rather than by + design. The Experimentalist resolves first for exactly this reason + (``resolve_harbor_run_inputs``). Only ``agent_dir`` (not ``sys.path``) is made importable, so a loose wrapper must be self-contained: a single module, or one that reaches siblings via @@ -373,7 +951,10 @@ def scoped_harbor_agent_import(agent_dir: Path, import_path: str) -> Iterator[st module_name = module_name.strip().lstrip(".") if not module_name: raise ValueError("import_path must be 'module' or 'module:attribute'") - package = f"{_AGENT_IMPORT_ROOT}.{_safe_identifier(agent_dir.name)}_{uuid4().hex[:8]}" + # Hashed here rather than reused from the cache stamp: this must describe the tree + # as it is about to be imported, and the extra walk is noise next to Docker. + suffix = _digest_directory(agent_dir, exclude=exclude)[:_IMPORT_DIGEST_CHARS] + package = f"{_AGENT_IMPORT_ROOT}.{_safe_identifier(agent_dir.name)}_{suffix}" with _IMPORT_LOCK: _install_agent_package(package, agent_dir) try: @@ -393,7 +974,11 @@ def _safe_identifier(value: str) -> str: def _install_agent_package(package: str, agent_dir: Path) -> None: - """Register ``package`` (and its parents) in ``sys.modules`` rooted at ``agent_dir``.""" + """Register ``package`` (and its parents) in ``sys.modules`` rooted at ``agent_dir``. + + Refcounted: package names are content-addressed, so two overlapping scopes on the + same agent directory legitimately share one. Callers must hold ``_IMPORT_LOCK``. + """ parts = package.split(".") for idx in range(1, len(parts) + 1): name = ".".join(parts[:idx]) @@ -404,11 +989,41 @@ def _install_agent_package(package: str, agent_dir: Path) -> None: sys.modules[name] = module if idx > 1: setattr(sys.modules[".".join(parts[: idx - 1])], parts[idx - 1], module) - sys.modules[package].__path__ = [str(agent_dir)] + installed = sys.modules[package] + if not installed.__path__: + installed.__path__ = [str(agent_dir)] + elif installed.__path__ != [str(agent_dir)]: + # Two directories sharing this package name share a content digest, so their + # trees are byte-identical and the path already installed is exactly as + # correct as this one — the excluded content (`.git`, `__pycache__`, the + # env dirs, `jobs_dir`) is not importable. Repointing would swap the + # directory out from under a scope that is still open, for no gain. + logger.debug( + "Agent package %s is already installed from %s; keeping it for the identical tree at %s", + package, + installed.__path__[0], + agent_dir, + ) + # Counted only once the injection it guards has succeeded. Incrementing first + # would strand the count above zero if any step above raised — the scope never + # opens, so nothing ever decrements it, and the package could never be torn down + # again for the life of the process. + _AGENT_PACKAGE_REFCOUNTS[package] = _AGENT_PACKAGE_REFCOUNTS.get(package, 0) + 1 def _uninstall_agent_package(package: str) -> None: - """Remove ``package`` and any submodules imported through it from ``sys.modules``.""" + """Remove ``package`` and its submodules from ``sys.modules`` on the last exit. + + Tearing down on the *first* exit would break a still-open scope sharing the same + content-addressed name, so the removal waits for the refcount to reach zero. + Callers must hold ``_IMPORT_LOCK``. + """ + remaining = _AGENT_PACKAGE_REFCOUNTS.get(package, 0) - 1 + if remaining > 0: + _AGENT_PACKAGE_REFCOUNTS[package] = remaining + return + _AGENT_PACKAGE_REFCOUNTS.pop(package, None) + for name in [n for n in sys.modules if n == package or n.startswith(f"{package}.")]: sys.modules.pop(name, None) parent, _, child = package.rpartition(".") @@ -731,6 +1346,8 @@ def reward_payload_from_result( __all__ = [ + "CACHE_STAMP_FILENAME", + "CACHE_STAMP_VERSION", "DEFAULT_REWARD_KEY", "HarborAgentTaskRunner", "HarborRewardMetric", diff --git a/uv.lock b/uv.lock index c6791baae3..3970dec9b1 100644 --- a/uv.lock +++ b/uv.lock @@ -4342,6 +4342,7 @@ dependencies = [ { name = "harbor", marker = "(platform_machine == 'arm64' and sys_platform == 'darwin') or (platform_machine == 'aarch64' and sys_platform == 'linux') or (platform_machine == 'x86_64' and sys_platform == 'linux')" }, { name = "httpx", marker = "(platform_machine == 'arm64' and sys_platform == 'darwin') or (platform_machine == 'aarch64' and sys_platform == 'linux') or (platform_machine == 'x86_64' and sys_platform == 'linux')" }, { name = "nemo-eval-author-plugin", marker = "(platform_machine == 'arm64' and sys_platform == 'darwin') or (platform_machine == 'aarch64' and sys_platform == 'linux') or (platform_machine == 'x86_64' and sys_platform == 'linux')" }, + { name = "nemo-evaluator-sdk", marker = "(platform_machine == 'arm64' and sys_platform == 'darwin') or (platform_machine == 'aarch64' and sys_platform == 'linux') or (platform_machine == 'x86_64' and sys_platform == 'linux')" }, { name = "nemo-insights-plugin", marker = "(platform_machine == 'arm64' and sys_platform == 'darwin') or (platform_machine == 'aarch64' and sys_platform == 'linux') or (platform_machine == 'x86_64' and sys_platform == 'linux')" }, { name = "nemo-platform", marker = "(platform_machine == 'arm64' and sys_platform == 'darwin') or (platform_machine == 'aarch64' and sys_platform == 'linux') or (platform_machine == 'x86_64' and sys_platform == 'linux')" }, { name = "nemo-platform-plugin", marker = "(platform_machine == 'arm64' and sys_platform == 'darwin') or (platform_machine == 'aarch64' and sys_platform == 'linux') or (platform_machine == 'x86_64' and sys_platform == 'linux')" }, @@ -4358,6 +4359,7 @@ requires-dist = [ { name = "harbor", specifier = ">=0.16" }, { name = "httpx" }, { name = "nemo-eval-author-plugin", editable = "plugins/nemo-eval-author" }, + { name = "nemo-evaluator-sdk", editable = "packages/nemo_evaluator_sdk" }, { name = "nemo-insights-plugin", editable = "plugins/nemo-insights" }, { name = "nemo-platform", editable = "packages/nemo_platform" }, { name = "nemo-platform-plugin", editable = "packages/nemo_platform_plugin" },