Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ async def _flow() -> str:
task_template=plan.task_template,
agent_source=plan.agent,
storage=plan.config.storage.model_dump(exclude_unset=True),
evaluator=plan.config.evaluator,
require_template=plan.insight is not None,
probes=_PREFLIGHT_PROBES,
)
Expand Down Expand Up @@ -412,6 +413,7 @@ def doctor(
task_template=plan.task_template,
agent_source=plan.agent,
storage=plan.config.storage.model_dump(),
evaluator=plan.config.evaluator,
require_template=plan.insight is not None,
probes=_PREFLIGHT_PROBES,
)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
# SPDX-License-Identifier: Apache-2.0

"""The agent entrypoint contract: one definition of what the evaluator imports.

Preflight resolves the same reference the evaluator imports, so it reports a
missing wrapper before a run instead of at the first trial. It must not import
``harbor`` to do so, because whether harbor is importable is itself a check.
"""

from importlib.machinery import PathFinder
from pathlib import Path

DEFAULT_AGENT_IMPORT_PATH = "harbor_wrapper:WrappedAgent"


def split_import_path(import_path: str) -> tuple[str, str]:
"""Split ``module:attribute``, the only form the evaluator can import.

The evaluator reaches the attribute with ``getattr``, so a name that is not
an identifier — absent, dotted, or holding a second ``:`` — can never
resolve.
"""
module_name, _, attribute = import_path.partition(":")
module_name = module_name.strip().lstrip(".")
attribute = attribute.strip()
if not module_name or not attribute.isidentifier():
raise ValueError("import_path must be <module>:<attribute>")
Comment thread
coderabbitai[bot] marked this conversation as resolved.
return module_name, attribute


def find_entrypoint_module(agent_dir: Path, module_name: str) -> Path | None:
"""Return the file the evaluator would import for *module_name*, or None.

Searches *agent_dir* alone, as the evaluator's scoped import does, and never
executes agent code.
"""
search = [str(agent_dir)]
origin: str | None = None
for part in module_name.split("."):
spec = PathFinder.find_spec(part, search)
if spec is None:
return None
search = list(spec.submodule_search_locations or ())
origin = spec.origin
return Path(origin) if origin is not None else None
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@
EvaluatorConfig,
EvaluatorType,
)
from nemo_experimentalist_plugin.experimentalist.components.evaluator.entrypoint import (
DEFAULT_AGENT_IMPORT_PATH,
split_import_path,
)
from pydantic import Field


Expand Down Expand Up @@ -187,7 +191,7 @@ class HarborEvaluatorConfig(EvaluatorConfig):
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")
import_path: str = Field(default=DEFAULT_AGENT_IMPORT_PATH)
trace_dir: str = Field(default=_TRACE_ARTIFACT_SOURCE)
trace_format: Literal["otlp", "atif"] = Field(
default="otlp",
Expand Down Expand Up @@ -392,17 +396,10 @@ def _ensure_package(name: str, search_path: Path | None = None) -> None:


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")

module_name, attribute = split_import_path(import_path)
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
return f"{package_name}.{module_name}:{attribute}", package_name


def _cleanup_scoped_imports(package_name: str) -> None:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@
from pathlib import Path

import httpx
from nemo_experimentalist_plugin.experimentalist.components.evaluator.entrypoint import (
DEFAULT_AGENT_IMPORT_PATH,
find_entrypoint_module,
split_import_path,
)
from nemo_experimentalist_plugin.experimentalist.components.repository import (
_redact_url,
looks_like_git,
Expand Down Expand Up @@ -200,6 +205,7 @@ def check_artifacts(
task_template: str | None = None,
agent_source: str | None = None,
storage: dict | None = None,
evaluator: dict | None = None,
require_template: bool = True,
probes: Probes | None = None,
) -> list[CheckResult]:
Expand All @@ -221,7 +227,7 @@ def check_artifacts(
effective_template = task_template or (profile.task_template if profile is not None else None)
if require_template and effective_template is not None:
results += _check_task_template(resolve_profile_path(effective_template, base_dir))
results += _check_agent_source(profile, p, agent_source=agent_source, storage=storage)
results += _check_agent_source(profile, p, agent_source=agent_source, storage=storage, evaluator=evaluator)
return results


Expand Down Expand Up @@ -334,6 +340,7 @@ def _check_agent_source(
*,
agent_source: str | None = None,
storage: dict | None = None,
evaluator: dict | None = None,
) -> list[CheckResult]:
results: list[CheckResult] = []
source = agent_source or (profile.agent_source if profile is not None else None)
Expand Down Expand Up @@ -366,6 +373,8 @@ def _check_agent_source(
f"agent source dir missing: {path}",
)
)
if path.is_dir():
results.append(_check_agent_entrypoint(path, evaluator))
# Effective storage flags from the auto path (resolved config, which may come
# from --config); doctor falls back to reading the profile's inline dict or
# path-form experiment_config without importing the loop chain.
Expand Down Expand Up @@ -451,6 +460,41 @@ def _check_agent_source(
return results


def _check_agent_entrypoint(agent_dir: Path, evaluator: dict | None) -> CheckResult:
"""Resolve the evaluator's entrypoint inside a local agent directory.

The evaluator imports it from that directory alone, so an entrypoint it
cannot find there fails every trial of every candidate.
"""
configured = (evaluator or {}).get("import_path")
import_path = DEFAULT_AGENT_IMPORT_PATH if configured is None else str(configured)
try:
module_name, attribute = split_import_path(import_path)
except ValueError as exc:
return CheckResult(
name="agent-entrypoint",
group="agent-source",
status="fail",
severity="required",
message=f"evaluator import_path {import_path!r} is unusable: {exc}",
hint="set evaluator.import_path in the experiment config to <module>:<attribute>",
)
found = find_entrypoint_module(agent_dir, module_name)
expected = Path(*module_name.split("."))
return make_check_result(
"agent-entrypoint",
"agent-source",
found is not None,
"required",
f"evaluator entrypoint {import_path} at {found}",
f"evaluator cannot import {module_name!r} from the agent source: {agent_dir}",
hint=(
f"add {expected}.py defining {attribute} to the agent directory, "
"or point evaluator.import_path in the experiment config at the module you have"
),
)


def _check_insight_file(
insight: str | None,
selector: str | None,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1292,9 +1292,13 @@ def test_safe_identifier_normal():
assert _safe_identifier("hello world") == "hello_world"


def test_scoped_import_path_empty_module(tmp_path):
with pytest.raises(ValueError, match="import_path module is required"):
_scoped_import_path(tmp_path, ":SomeClass")
@pytest.mark.parametrize(
"import_path",
[":SomeClass", "harbor_wrapper", "harbor_wrapper:", "harbor_wrapper:WrappedAgent:extra"],
)
def test_scoped_import_path_requires_module_and_attribute(tmp_path, import_path):
with pytest.raises(ValueError, match="import_path must be <module>:<attribute>"):
_scoped_import_path(tmp_path, import_path)


def test_cleanup_scoped_imports_removes_parent_attr():
Expand Down
30 changes: 30 additions & 0 deletions plugins/nemo-experimentalist/tests/test_cli_profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,15 @@ def app():
return cli.ExperimentalistCLI().get_cli()


def write_harbor_wrapper(agent_dir: Path) -> None:
(agent_dir / "harbor_wrapper.py").write_text("class WrappedAgent:\n pass\n", encoding="utf-8")


@pytest.fixture()
def profile_tree(tmp_path: Path) -> Path:
for sub in ("evals/task_template", "evals/train", "evals/val"):
(tmp_path / sub).mkdir(parents=True)
write_harbor_wrapper(tmp_path)
(tmp_path / "optimizer.yaml").write_text(
"agent: flight-planner\n"
"task_template: ./evals/task_template\n"
Expand Down Expand Up @@ -134,6 +139,7 @@ def test_experiment_all_flags_no_profile_still_works(app, tmp_path: Path, monkey
monkeypatch.setattr(cli, "_PREFLIGHT_PROBES", quiet_probes())
for sub in ("agent", "t", "v"):
(tmp_path / sub).mkdir()
write_harbor_wrapper(tmp_path / "agent")
monkeypatch.chdir(tmp_path)
result = runner.invoke(
app,
Expand Down Expand Up @@ -246,6 +252,7 @@ def now(*, tz):
monkeypatch.setattr(cli, "run_experimentalist", recorder)
for sub in ("agent", "t", "v"):
(tmp_path / sub).mkdir()
write_harbor_wrapper(tmp_path / "agent")
monkeypatch.chdir(tmp_path)
args = [
"run",
Expand Down Expand Up @@ -1045,6 +1052,29 @@ def test_doctor_validates_full_effective_experiment_config(
assert "Traceback" not in result.output


@pytest.mark.parametrize("command", ["run", "doctor"])
def test_missing_evaluator_entrypoint_blocks_both_commands(
app,
profile_tree: Path,
monkeypatch: pytest.MonkeyPatch,
command: str,
) -> None:
"""Doctor reports the same missing wrapper that stops a run, not a clean bill."""
write_task_toml(profile_tree)
(profile_tree / "harbor_wrapper.py").unlink()
monkeypatch.setattr(cli, "run_experimentalist", RunRecorder())
monkeypatch.chdir(profile_tree)
args = [command]
if command == "run":
args += ["--no-insight", "-o", str(profile_tree / "out")]

result = runner.invoke(app, args)

assert result.exit_code == 1
assert "evaluator cannot import 'harbor_wrapper'" in result.output
assert "Traceback" not in result.output


@pytest.mark.parametrize("command", ["run", "doctor"])
def test_env_file_encoding_failure_is_clean_and_actionable(
app,
Expand Down
4 changes: 3 additions & 1 deletion plugins/nemo-experimentalist/tests/test_experiment_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,10 @@ def _make_paths(tmp_path: Path) -> ExperimentCliPaths:
template = _make_dir(tmp_path / "template")
(template / "task.toml").write_text('[task]\nname = "org/test-template"\n', encoding="utf-8")
(template / "instruction.md").write_text("Test task instructions.\n", encoding="utf-8")
agent = _make_dir(tmp_path / "agent")
(agent / "harbor_wrapper.py").write_text("class WrappedAgent:\n pass\n", encoding="utf-8")
return ExperimentCliPaths(
agent=_make_dir(tmp_path / "agent"),
agent=agent,
train=_make_dir(tmp_path / "train"),
validation=_make_dir(tmp_path / "validation"),
template=template,
Expand Down
45 changes: 45 additions & 0 deletions plugins/nemo-experimentalist/tests/test_preflight.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ def make_probes(*, cmd_ok: bool = True, http: bool = True) -> Probes:
def full_profile(tmp_path: Path, *, agent_source: str | None = None):
tt = tmp_path / "evals" / "task_template"
tt.mkdir(parents=True)
(tmp_path / "harbor_wrapper.py").write_text("class WrappedAgent:\n pass\n", encoding="utf-8")
(tt / "task.toml").write_text('[task]\nname = "org/agent__template"\n', encoding="utf-8")
(tt / "instruction.md").write_text("do the thing", encoding="utf-8")
for sub in ("evals/train", "evals/val"):
Expand Down Expand Up @@ -212,6 +213,50 @@ def test_local_agent_source_dir_stays_required(tmp_path: Path) -> None:
assert src.status == "fail"


def test_missing_evaluator_entrypoint_is_required_failure(tmp_path: Path) -> None:
(tmp_path / "agent").mkdir()

results = check_artifacts(full_profile(tmp_path, agent_source="./agent"), probes=make_probes())

entrypoint = next(r for r in results if r.name == "agent-entrypoint")
assert entrypoint.status == "fail"
assert entrypoint.severity == "required"
assert "harbor_wrapper.py" in (entrypoint.hint or "")
assert "WrappedAgent" in (entrypoint.hint or "")


@pytest.mark.parametrize(
("import_path", "expected_status"),
[
("pkg.wrapper:Agent", "pass"),
("pkg.missing:Agent", "fail"),
("pkg.wrapper", "fail"),
],
ids=["configured", "configured-missing", "no-attribute"],
)
def test_configured_entrypoint_replaces_the_default(tmp_path: Path, import_path: str, expected_status: str) -> None:
(tmp_path / "agent" / "pkg").mkdir(parents=True)
(tmp_path / "agent" / "pkg" / "wrapper.py").write_text("class Agent:\n pass\n", encoding="utf-8")

results = check_artifacts(
full_profile(tmp_path, agent_source="./agent"),
probes=make_probes(),
evaluator={"import_path": import_path},
)

entrypoint = next(r for r in results if r.name == "agent-entrypoint")
assert entrypoint.status == expected_status


def test_git_agent_source_has_no_entrypoint_check(tmp_path: Path) -> None:
results = check_artifacts(
full_profile(tmp_path, agent_source="https://host/g/repo.git@main"),
probes=make_probes(),
)

assert not any(r.name == "agent-entrypoint" for r in results)


@pytest.mark.parametrize(
("source", "expected_repo"),
[
Expand Down