diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 9be24164a..045ed6295 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -29,7 +29,11 @@ jobs: - name: Install dependencies run: | python -m pip install --upgrade pip - pip install -e ".[dev]" + # openadapt-ml is installed explicitly so the cross-package + # seam tests (tests/test_import_integrity.py and the cmd_serve + # contract in tests/test_cli_smoke.py) run instead of skipping. + # Issue #999 shipped because this seam was never tested in CI. + pip install -e ".[dev]" openadapt-ml - name: Lint with ruff run: | @@ -40,3 +44,6 @@ jobs: run: | python -c "from openadapt.cli import main; print('CLI import OK')" python -c "from openadapt.version import __version__; print(f'Version: {__version__}')" + + - name: Run tests + run: pytest tests/ -v diff --git a/.github/workflows/release-and-publish.yml b/.github/workflows/release-and-publish.yml index 75e7d1a1d..e277f60af 100644 --- a/.github/workflows/release-and-publish.yml +++ b/.github/workflows/release-and-publish.yml @@ -54,3 +54,21 @@ jobs: poetry config pypi-token.pypi $PYPI_TOKEN poetry build poetry publish --no-interaction --skip-existing + + # Releases in openadapt-ml failed silently for 3 months (Mar-Jun + # 2026) while PyPI went stale; see issue #999. Fail loudly instead. + - name: File issue on release failure + if: failure() + env: + GH_TOKEN: ${{ secrets.ADMIN_TOKEN }} + run: | + TITLE="Release workflow failed on main" + BODY="The release workflow failed: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + + Until this is fixed, merged fix/feat commits are NOT being published to PyPI, and users install stale versions." + EXISTING=$(gh issue list --repo "${{ github.repository }}" --state open --search "in:title \"$TITLE\"" --json number --jq '.[0].number // empty') + if [ -n "$EXISTING" ]; then + gh issue comment "$EXISTING" --repo "${{ github.repository }}" --body "$BODY" + else + gh issue create --repo "${{ github.repository }}" --title "$TITLE" --body "$BODY" + fi diff --git a/openadapt/__init__.py b/openadapt/__init__.py index 07bcec409..7d6a2a868 100644 --- a/openadapt/__init__.py +++ b/openadapt/__init__.py @@ -59,11 +59,10 @@ def __getattr__(name: str): return locals()[name] # ML package (heavy - only import if explicitly requested) - if name in ("QwenVLAdapter", "train", "Trainer"): - from openadapt_ml import QwenVLAdapter # noqa: F401 - from openadapt_ml.training import Trainer # noqa: F401 + if name == "QwenVLAdapter": + from openadapt_ml.models.qwen_vl import QwenVLAdapter # noqa: F401 - return locals().get(name) + return QwenVLAdapter # Grounding package (optional) if name in ("Grounder", "OmniGrounder", "GeminiGrounder"): @@ -115,7 +114,6 @@ def __getattr__(name: str): "HTMLBuilder", # From ml "QwenVLAdapter", - "Trainer", # From grounding (optional) "Grounder", "OmniGrounder", diff --git a/openadapt/cli.py b/openadapt/cli.py index 748154d3e..d707f6149 100644 --- a/openadapt/cli.py +++ b/openadapt/cli.py @@ -441,14 +441,6 @@ def serve(port: int, output: str, open: bool): # so --output is honored. oa_local.TRAINING_OUTPUT = Path(output) - if open: - import threading - import webbrowser - - threading.Timer( - 1.0, webbrowser.open, args=(f"http://localhost:{port}",) - ).start() - sys.exit( oa_local.cmd_serve( argparse.Namespace( @@ -457,6 +449,7 @@ def serve(port: int, output: str, open: bool): no_regenerate=False, start_page=None, quiet=False, + open=open, ) ) ) diff --git a/tests/test_cli_smoke.py b/tests/test_cli_smoke.py new file mode 100644 index 000000000..361939aa5 --- /dev/null +++ b/tests/test_cli_smoke.py @@ -0,0 +1,231 @@ +"""CLI smoke and seam-contract tests. + +Issue #999: `openadapt serve` and `openadapt train start` were broken +for months while CI stayed green, because cli.py's imports of +openadapt-ml only execute inside command bodies and the broad +`except ImportError` handlers reported every failure as +"openadapt-ml not installed". + +Three layers of defense here: + +1. test_every_command_help — walks the whole Click tree and renders + --help for every command, so any module-level wiring error fails CI. +2. Contract tests — monkeypatch the openadapt-ml entry points and + assert cli.py calls them the way they're actually shaped today. +3. test_cmd_serve_reads_only_provided_args — parses the installed + openadapt-ml's cmd_serve and asserts every `args.` it reads is + provided by cli.py's Namespace, so the seam can't drift silently in + either direction. + +The openadapt-ml-dependent tests skip when it isn't installed; CI +installs it so they always run there. +""" + +from __future__ import annotations + +import ast +import sys +from pathlib import Path + +import click +import pytest +from click.testing import CliRunner + +from openadapt.cli import main as cli_main + +# Namespace attributes cli.py's serve command provides to cmd_serve. +# Keep in sync with openadapt/cli.py::serve. +SERVE_NAMESPACE_ATTRS = { + "port", + "benchmark", + "no_regenerate", + "start_page", + "quiet", + "open", +} + + +def _iter_commands(group, prefix=()): + yield prefix, group + if isinstance(group, click.Group): + for name, cmd in group.commands.items(): + yield from _iter_commands(cmd, prefix + (name,)) + + +def test_every_command_help(): + """Render --help for every command in the tree.""" + runner = CliRunner() + failures = [] + for path, _cmd in _iter_commands(cli_main): + args = list(path) + ["--help"] + result = runner.invoke(cli_main, args) + if result.exit_code != 0: + failures.append(f"{' '.join(args)!r} exited {result.exit_code}") + assert not failures, "Commands whose --help failed:\n " + "\n ".join(failures) + + +def test_version_command(): + runner = CliRunner() + result = runner.invoke(cli_main, ["version"]) + assert result.exit_code == 0 + + +# --------------------------------------------------------------------------- +# Seam contracts with openadapt-ml +# --------------------------------------------------------------------------- + + +def _require_openadapt_ml(): + return pytest.importorskip("openadapt_ml", reason="openadapt-ml not installed") + + +def test_train_start_calls_real_entry_point(monkeypatch, tmp_path): + """`openadapt train start` must call scripts.train.main with kwargs + that exist in its signature.""" + _require_openadapt_ml() + import inspect + + from openadapt_ml.scripts import train as train_module + + real_params = set(inspect.signature(train_module.main).parameters) + calls = [] + + def fake_main(**kwargs): + unknown = set(kwargs) - real_params + assert not unknown, ( + f"cli.py passes kwargs {sorted(unknown)} that " + f"openadapt_ml.scripts.train.main does not accept " + f"(it takes {sorted(real_params)})" + ) + calls.append(kwargs) + + monkeypatch.setattr(train_module, "main", fake_main) + + capture_dir = tmp_path / "my-capture" + capture_dir.mkdir() + config = tmp_path / "config.yaml" + config.write_text("model:\n name: test\n") + + runner = CliRunner() + result = runner.invoke( + cli_main, + [ + "train", + "start", + "--capture", + str(capture_dir), + "--config", + str(config), + "--no-open", + ], + ) + assert result.exit_code == 0, result.output + assert len(calls) == 1 + kwargs = calls[0] + assert kwargs["config_path"] == str(config) + assert kwargs["capture_path"] == str(capture_dir) + assert kwargs["open_dashboard"] is False + + +def test_serve_calls_cmd_serve_with_expected_namespace(monkeypatch): + """`openadapt serve` must call cmd_serve with the agreed Namespace.""" + _require_openadapt_ml() + from openadapt_ml.cloud import local as oa_local + + received = [] + + def fake_cmd_serve(args): + received.append(args) + return 0 + + monkeypatch.setattr(oa_local, "cmd_serve", fake_cmd_serve) + + runner = CliRunner() + result = runner.invoke(cli_main, ["serve", "--port", "8123", "--no-open"]) + assert result.exit_code == 0, result.output + assert len(received) == 1 + ns = received[0] + assert ns.port == 8123 + assert ns.open is False # --no-open passes through to cmd_serve + for attr in SERVE_NAMESPACE_ATTRS: + assert hasattr(ns, attr), f"Namespace missing {attr}" + + +def test_serve_honors_output_directory(monkeypatch, tmp_path): + """--output must repoint openadapt-ml's TRAINING_OUTPUT.""" + _require_openadapt_ml() + from openadapt_ml.cloud import local as oa_local + + monkeypatch.setattr(oa_local, "cmd_serve", lambda args: 0) + + runner = CliRunner() + out = tmp_path / "runs" + result = runner.invoke(cli_main, ["serve", "--output", str(out), "--no-open"]) + assert result.exit_code == 0, result.output + assert Path(oa_local.TRAINING_OUTPUT) == out + + +def test_cmd_serve_reads_only_provided_args(): + """Every `args.` cmd_serve reads must be in cli.py's Namespace. + + This is the direction the contract can silently drift: openadapt-ml + adds a new required Namespace attribute and cli.py doesn't provide + it. Parse the installed cmd_serve and check. + """ + ml = _require_openadapt_ml() + local_path = Path(next(iter(ml.__path__))) / "cloud" / "local.py" + tree = ast.parse(local_path.read_text(encoding="utf-8")) + cmd_serve = next( + ( + node + for node in tree.body + if isinstance(node, ast.FunctionDef) and node.name == "cmd_serve" + ), + None, + ) + assert cmd_serve is not None, "cmd_serve not found in openadapt-ml" + + args_param = cmd_serve.args.args[0].arg + read_attrs = { + node.attr + for node in ast.walk(cmd_serve) + if isinstance(node, ast.Attribute) + and isinstance(node.value, ast.Name) + and node.value.id == args_param + } + missing = read_attrs - SERVE_NAMESPACE_ATTRS + assert not missing, ( + f"openadapt-ml's cmd_serve reads args attributes " + f"{sorted(missing)} that openadapt's serve command does not " + f"provide; update openadapt/cli.py (and SERVE_NAMESPACE_ATTRS)" + ) + + +def test_import_error_messages_not_masked(monkeypatch): + """Internal ImportErrors must surface the real error, not claim + openadapt-ml isn't installed.""" + _require_openadapt_ml() + + import builtins + + real_import = builtins.__import__ + + def broken_import(name, *args, **kwargs): + if name == "openadapt_ml.cloud" or name.startswith("openadapt_ml.cloud."): + raise ImportError( + "cannot import name 'definitely_phantom' from " + "'openadapt_ml.cloud.local'" + ) + return real_import(name, *args, **kwargs) + + monkeypatch.setattr(builtins, "__import__", broken_import) + monkeypatch.delitem(sys.modules, "openadapt_ml.cloud.local", raising=False) + monkeypatch.delitem(sys.modules, "openadapt_ml.cloud", raising=False) + + runner = CliRunner() + result = runner.invoke(cli_main, ["serve", "--no-open"]) + assert result.exit_code != 0 + assert "definitely_phantom" in result.output, ( + "The underlying ImportError must appear in the CLI output; " + f"got: {result.output}" + ) diff --git a/tests/test_import_integrity.py b/tests/test_import_integrity.py new file mode 100644 index 000000000..a8124267c --- /dev/null +++ b/tests/test_import_integrity.py @@ -0,0 +1,253 @@ +"""Static import-integrity checks for the openadapt meta-package. + +Issue #999 class of bug: cli.py lazily imported names from openadapt-ml +that didn't exist (`serve_dashboard`, `train_main`). Imports inside +function bodies never execute during plain import tests, so this walks +the AST instead — for the local `openadapt` package AND across the seam +into the installed `openadapt_ml` package. + +Cross-package checks skip when openadapt-ml isn't installed; CI +installs it so they always run there. +""" + +from __future__ import annotations + +import ast +import importlib.util +from pathlib import Path + +import pytest + +LOCAL_PACKAGE = "openadapt" +LOCAL_ROOT = Path(__file__).resolve().parent.parent / LOCAL_PACKAGE + +EXTERNAL_PACKAGES = ("openadapt_ml",) + +PHANTOM_IMPORT_ALLOWLIST: set[tuple[str, str]] = set() + + +def _package_module_map(name: str, root: Path) -> dict[str, Path]: + modules: dict[str, Path] = {} + for path in root.rglob("*.py"): + rel = path.relative_to(root.parent) + parts = list(rel.with_suffix("").parts) + if parts[-1] == "__init__": + parts = parts[:-1] + modules[".".join(parts)] = path + return modules + + +def _build_module_map() -> dict[str, Path]: + modules = _package_module_map(LOCAL_PACKAGE, LOCAL_ROOT) + for pkg in EXTERNAL_PACKAGES: + spec = importlib.util.find_spec(pkg) + if spec and spec.submodule_search_locations: + root = Path(next(iter(spec.submodule_search_locations))) + modules.update(_package_module_map(pkg, root)) + return modules + + +MODULES = _build_module_map() +CHECKED_PREFIXES = tuple( + p + for p in (LOCAL_PACKAGE, *EXTERNAL_PACKAGES) + if p == LOCAL_PACKAGE or any(m.startswith(p) for m in MODULES) +) + + +def _is_checked(target: str) -> bool: + return any(target == p or target.startswith(p + ".") for p in CHECKED_PREFIXES) + + +def _collect_defined(tree: ast.Module) -> tuple[set[str], bool]: + defined: set[str] = set() + dynamic = False + + def visit_body(body: list[ast.stmt]) -> None: + nonlocal dynamic + for node in body: + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + defined.add(node.name) + if node.name == "__getattr__": + dynamic = True + elif isinstance(node, ast.ClassDef): + defined.add(node.name) + elif isinstance(node, ast.Assign): + for target in node.targets: + for name_node in ast.walk(target): + if isinstance(name_node, ast.Name): + defined.add(name_node.id) + elif isinstance(node, (ast.AnnAssign, ast.AugAssign)): + if isinstance(node.target, ast.Name): + defined.add(node.target.id) + elif isinstance(node, ast.Import): + for alias in node.names: + defined.add((alias.asname or alias.name).split(".")[0]) + elif isinstance(node, ast.ImportFrom): + for alias in node.names: + if alias.name == "*": + dynamic = True + else: + defined.add(alias.asname or alias.name) + elif isinstance(node, (ast.If, ast.Try, ast.With)): + visit_body(getattr(node, "body", [])) + visit_body(getattr(node, "orelse", [])) + visit_body(getattr(node, "finalbody", [])) + for handler in getattr(node, "handlers", []): + visit_body(handler.body) + + visit_body(tree.body) + return defined, dynamic + + +def _parse(path: Path) -> ast.Module: + return ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + + +_DEFINED_CACHE: dict[str, tuple[set[str], bool]] = {} + + +def _defined_in(module: str) -> tuple[set[str], bool] | None: + if module not in MODULES: + return None + if module not in _DEFINED_CACHE: + _DEFINED_CACHE[module] = _collect_defined(_parse(MODULES[module])) + return _DEFINED_CACHE[module] + + +def _resolve_relative(current_module: str, node: ast.ImportFrom) -> str | None: + if node.level == 0: + return node.module + parts = current_module.split(".") + if MODULES.get(current_module, Path()).name != "__init__.py": + parts = parts[:-1] + cut = node.level - 1 + if cut: + parts = parts[:-cut] if cut <= len(parts) else [] + base = ".".join(parts) + if node.module: + return f"{base}.{node.module}" if base else node.module + return base or None + + +def _local_modules() -> list[tuple[str, Path]]: + return sorted( + (m, p) + for m, p in MODULES.items() + if m == LOCAL_PACKAGE or m.startswith(LOCAL_PACKAGE + ".") + ) + + +def test_external_packages_installed_in_ci(): + """The cross-package checks are only meaningful with openadapt-ml + present. Locally this may skip; CI installs it.""" + for pkg in EXTERNAL_PACKAGES: + if importlib.util.find_spec(pkg) is None: + pytest.skip(f"{pkg} not installed; cross-package checks limited") + + +def test_no_phantom_imports(): + problems: list[str] = [] + + for current, path in _local_modules(): + tree = _parse(path) + for node in ast.walk(tree): + if not isinstance(node, ast.ImportFrom): + continue + target = _resolve_relative(current, node) + if not target or not _is_checked(target): + continue + info = _defined_in(target) + if info is None: + if any(m.startswith(target + ".") for m in MODULES): + continue + problems.append( + f"{path}:{node.lineno}: imports from missing module '{target}'" + ) + continue + defined, dynamic = info + if dynamic: + continue + for alias in node.names: + if alias.name == "*": + continue + if alias.name in defined: + continue + if f"{target}.{alias.name}" in MODULES: + continue + if (target, alias.name) in PHANTOM_IMPORT_ALLOWLIST: + continue + problems.append( + f"{path}:{node.lineno}: 'from {target} import " + f"{alias.name}' but '{alias.name}' is not defined " + f"there" + ) + + assert not problems, ( + "Phantom imports detected (#999 failure class):\n " + "\n ".join(problems) + ) + + +def _function_params(module: str, func_name: str) -> set[str] | None: + info = _defined_in(module) + if info is None or info[1]: + return None + tree = _parse(MODULES[module]) + for node in tree.body: + if ( + isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) + and node.name == func_name + ): + if node.decorator_list or node.args.kwarg is not None: + return None + params = [a.arg for a in node.args.posonlyargs] + params += [a.arg for a in node.args.args] + params += [a.arg for a in node.args.kwonlyargs] + return set(params) + return None + + +def test_no_phantom_kwargs(): + problems: list[str] = [] + + for current, path in _local_modules(): + tree = _parse(path) + + imported: dict[str, tuple[str, str]] = {} + for node in ast.walk(tree): + if isinstance(node, ast.ImportFrom): + target = _resolve_relative(current, node) + if target and _is_checked(target): + for alias in node.names: + if alias.name != "*": + imported[alias.asname or alias.name] = ( + target, + alias.name, + ) + + if not imported: + continue + + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + if not isinstance(node.func, ast.Name): + continue + if node.func.id not in imported: + continue + target_module, original = imported[node.func.id] + params = _function_params(target_module, original) + if params is None: + continue + for kw in node.keywords: + if kw.arg is not None and kw.arg not in params: + problems.append( + f"{path}:{node.lineno}: call to " + f"{target_module}.{original}(... {kw.arg}=...) " + f"but its parameters are {sorted(params)}" + ) + + assert not problems, ( + "Phantom keyword arguments (TypeError at call time):\n " + + "\n ".join(problems) + )