diff --git a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/cli.py b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/cli.py index ff5d6a4c3f..105b05d700 100644 --- a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/cli.py +++ b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/cli.py @@ -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, ) @@ -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, ) diff --git a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/entrypoint.py b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/entrypoint.py new file mode 100644 index 0000000000..0a7039b966 --- /dev/null +++ b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/entrypoint.py @@ -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 :") + 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 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 cfcbf609fe..2a984f6922 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 @@ -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 @@ -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", @@ -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: diff --git a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/preflight.py b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/preflight.py index 29d636a66b..2ae2f5d2cc 100644 --- a/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/preflight.py +++ b/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/preflight.py @@ -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, @@ -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]: @@ -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 @@ -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) @@ -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. @@ -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 :", + ) + 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, diff --git a/plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor.py b/plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor.py index c4c3b40dc8..57eb49ae30 100644 --- a/plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor.py +++ b/plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor.py @@ -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 :"): + _scoped_import_path(tmp_path, import_path) def test_cleanup_scoped_imports_removes_parent_attr(): diff --git a/plugins/nemo-experimentalist/tests/test_cli_profile.py b/plugins/nemo-experimentalist/tests/test_cli_profile.py index 8c093780d8..d9246aec7d 100644 --- a/plugins/nemo-experimentalist/tests/test_cli_profile.py +++ b/plugins/nemo-experimentalist/tests/test_cli_profile.py @@ -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" @@ -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, @@ -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", @@ -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, diff --git a/plugins/nemo-experimentalist/tests/test_experiment_cli.py b/plugins/nemo-experimentalist/tests/test_experiment_cli.py index 6a37ad0ebb..83a0ec8e4a 100644 --- a/plugins/nemo-experimentalist/tests/test_experiment_cli.py +++ b/plugins/nemo-experimentalist/tests/test_experiment_cli.py @@ -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, diff --git a/plugins/nemo-experimentalist/tests/test_preflight.py b/plugins/nemo-experimentalist/tests/test_preflight.py index 8835edbdde..d97dd4286e 100644 --- a/plugins/nemo-experimentalist/tests/test_preflight.py +++ b/plugins/nemo-experimentalist/tests/test_preflight.py @@ -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"): @@ -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"), [