From e67aaa534dbdd48bfaf66a12ebf0e927b0fee5ba Mon Sep 17 00:00:00 2001 From: Rory Byrne Date: Wed, 22 Jul 2026 09:13:36 +0100 Subject: [PATCH 1/2] feat: osa manifest command + clean convention contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a public `osa manifest` command that emits a convention's authored definition as JSON, so a consumer in a different interpreter (e.g. the Amacrin CLI installed as an isolated tool) can obtain the manifest via `uv run osa manifest` instead of importing the convention. Restructure the convention model so definition and build artifact are cleanly separated: - config/limits are authored, on each component; `release` is a pure nested build artifact {image, digest, source_ref}, absent pre-build. - hooks and ingesters are symmetric: both a `Component` (name, config, limits, optional nested release), hook adds `feature`, ingester adds schedule/initial_run. - one typed model tree, serialized at the edge with model_dump(by_alias=True, exclude_none=True) — no custom serializers, no domain/wire two-layer. FieldDefinition drops its serializer (edge exclude_none omits absents). Pairs with the OSA server contract change (config/limits on the hook, release = {image, digest, source_ref}) — deploy them together. Co-Authored-By: Claude Opus 4.8 (1M context) --- osa/cli/deploy.py | 272 +++++++++++++++++++---------- osa/cli/main.py | 56 ++++++ osa/types/schema.py | 61 ++++--- tests/test_deploy.py | 204 +++++++++++++++++++++- tests/test_deploy_v2.py | 179 ++++++++++--------- tests/test_docs_gate.py | 19 +- tests/test_to_field_definitions.py | 80 +++++++-- uv.lock | 2 +- 8 files changed, 637 insertions(+), 236 deletions(-) diff --git a/osa/cli/deploy.py b/osa/cli/deploy.py index a236d1e..a4edded 100644 --- a/osa/cli/deploy.py +++ b/osa/cli/deploy.py @@ -12,12 +12,15 @@ from typing import Any import httpx -from pydantic import BaseModel +from pydantic import BaseModel, ConfigDict, Field from osa._registry import ConventionInfo, HookInfo, IngesterInfo, _conventions, _hooks +from osa.authoring.example import Example from osa.cli.proc import run_streamed, tail from osa.cli.ui import UI, Task -from osa.manifest import generate_columns +from osa.manifest import ColumnDef, generate_columns +from osa.types.ingester import IngesterSchedule, InitialRun, Limits +from osa.types.schema import FieldDefinition class DeployError(RuntimeError): @@ -311,36 +314,15 @@ def _resolve_source_ref(project_dir: Path) -> str: return f"git:{sha}" -def _hook_to_definition( - hook: HookInfo, - image: str, - digest: str, - source_ref: str, -) -> dict[str, Any]: - """Build a HookDefinition dict from a HookInfo + image details.""" - columns: list[dict[str, Any]] = [] +def _hook_columns(hook: HookInfo) -> list[ColumnDef]: + """Feature columns for a hook whose output is a Pydantic model, else [].""" if ( hook.output_type is not None and isinstance(hook.output_type, type) and issubclass(hook.output_type, BaseModel) ): - columns = [c.model_dump() for c in generate_columns(hook.output_type)] - - return { - "name": hook.name, - "feature": { - "kind": "table", - "cardinality": hook.cardinality, - "columns": columns, - }, - "release": { - "image": image, - "digest": digest, - "config": {}, - "limits": hook.limits.model_dump(), - "source_ref": source_ref, - }, - } + return generate_columns(hook.output_type) + return [] _MIN_DISTINCT_TRIGGER_QUESTIONS = 3 @@ -383,65 +365,162 @@ def _check_docs_gate(conventions: list[ConventionInfo]) -> None: ) -def _convention_to_payload( - conv: ConventionInfo, - hook_definitions: list[dict[str, Any]], - ingester_image: tuple[str, str] | None = None, -) -> dict[str, Any]: - """Build the CreateConvention request payload.""" - schema_fields = conv.schema_type.to_field_definitions() +# --- The convention manifest: one typed model, serialized at the edge ------- +# +# Authored definition per component (config/limits on the component) + an +# optional nested build ``release``. ``osa manifest`` emits it release-less; +# ``osa deploy`` binds releases. Both edges dump with +# ``model_dump(by_alias=True, exclude_none=True)`` — no custom serializers. + + +class ComponentRelease(BaseModel): + """A built component's release artifact: the image + its build anchor.""" + + image: str + digest: str + source_ref: str + + +class Feature(BaseModel): + """The feature table a hook emits.""" + + kind: str = "table" + cardinality: str + columns: list[ColumnDef] + + +class FileRequirements(BaseModel): + """Upload constraints from the convention's ``files=`` argument.""" + + accepted_types: list[str] | None = None + max_count: int | None = None + max_file_size: int | None = None + min_count: int | None = None + + +class SchemaRef(BaseModel): + """The record schema a convention indexes.""" + + id: str + version: str + fields: list[FieldDefinition] + + +class ConventionDocs(BaseModel): + """Mandatory agent-facing documentation (#151).""" + + purpose: str + example_questions: list[str] + examples: list[Example] + when_not_to_use: str | None = None + see_also: list[str] | None = None + + +class Component(BaseModel): + """A buildable component: authored ``config``/``limits`` and an optional + built ``release`` (``None`` until ``bind_releases`` fills it).""" + + name: str + # DYNAMIC: free-form runtime config (hook: {} today; ingester: RuntimeConfig). + config: dict[str, Any] | None + limits: Limits + release: ComponentRelease | None = None + +class Hook(Component): + feature: Feature + + +class Ingester(Component): + schedule: IngesterSchedule | None = None + initial_run: InitialRun | None = None + + +class ConventionManifest(BaseModel): + """A convention's authored definition; ``bind_releases`` binds each + component's built release onto it. The single source for both ``osa + manifest`` (release-less) and the ``osa deploy`` body — they cannot drift.""" + + model_config = ConfigDict(populate_by_name=True) + + title: str + description: str + record_schema: SchemaRef = Field(alias="schema") + file_requirements: FileRequirements + hooks: list[Hook] + ingester: Ingester | None + docs: ConventionDocs + + +def build_manifest(conv: ConventionInfo) -> ConventionManifest: + """Build the typed, release-less manifest for a registered convention.""" file_reqs = conv.file_requirements if "min_count" not in file_reqs: file_reqs = {**file_reqs, "min_count": 0} - ingester: dict[str, Any] | None = None - if ingester_image is not None and conv.ingester_info is not None: - image, digest = ingester_image - config = None - ingester_cls = conv.ingester_info.ingester_cls - if hasattr(ingester_cls, "RuntimeConfig"): - config = ingester_cls.RuntimeConfig().model_dump() # type: ignore[union-attr] - - schedule = None - initial_run = None - if conv.ingester_info.schedule is not None: - schedule = conv.ingester_info.schedule.model_dump() - if conv.ingester_info.initial_run is not None: - initial_run = conv.ingester_info.initial_run.model_dump() - - ingester_limits = conv.ingester_info.limits - ingester = { - "image": image, - "digest": digest, - "runner": "oci", - "config": config, - "limits": ingester_limits.model_dump() - if ingester_limits - else {"timeout_seconds": 3600, "memory": "512m", "cpu": "0.5"}, - "schedule": schedule, - "initial_run": initial_run, - } - - return { - "title": conv.title, - "description": conv.description, - "schema": { - "id": conv.schema_type.schema_id(), - "version": conv.version, - "fields": schema_fields, - }, - "file_requirements": file_reqs, - "hooks": hook_definitions, - "ingester": ingester, - "docs": { - "purpose": conv.purpose, - "example_questions": conv.example_questions, - "examples": [e.model_dump() for e in conv.examples], - "when_not_to_use": conv.when_not_to_use, - "see_also": conv.see_also, - }, - } + hooks: list[Hook] = [] + for h in conv.hooks: + info = next((hi for hi in _hooks if hi.name == h.__name__), None) + if info is None: + continue + hooks.append( + Hook( + name=info.name, + config={}, + limits=info.limits, + feature=Feature( + cardinality=info.cardinality, columns=_hook_columns(info) + ), + ) + ) + + ingester: Ingester | None = None + if conv.ingester_info is not None: + info = conv.ingester_info + config: dict[str, Any] | None = None + if hasattr(info.ingester_cls, "RuntimeConfig"): + config = info.ingester_cls.RuntimeConfig().model_dump() # type: ignore[union-attr] + ingester = Ingester( + name=info.name, + config=config, + limits=info.limits, + schedule=info.schedule, + initial_run=info.initial_run, + ) + + return ConventionManifest( + title=conv.title, + description=conv.description, + schema=SchemaRef( + id=conv.schema_type.schema_id(), + version=conv.version, + fields=conv.schema_type.to_field_definitions(), + ), + file_requirements=FileRequirements.model_validate(file_reqs), + hooks=hooks, + ingester=ingester, + docs=ConventionDocs( + purpose=conv.purpose, + example_questions=conv.example_questions, + examples=conv.examples, + when_not_to_use=conv.when_not_to_use, + see_also=conv.see_also, + ), + ) + + +def bind_releases( + manifest: ConventionManifest, releases: dict[str, ComponentRelease] +) -> ConventionManifest: + """Return a copy of the manifest with each built component's release bound by + name — uniform across hooks and the ingester. ``osa deploy`` uses it; ``osa + manifest`` skips it (releases stay ``None``, omitted at serialization).""" + bound = manifest.model_copy(deep=True) + for hook in bound.hooks: + hook.release = releases.get(hook.name) + if bound.ingester is not None: + bound.ingester.release = releases.get(bound.ingester.name) + return bound def _resolve_existing_image(tag: str) -> tuple[str, str]: @@ -674,24 +753,25 @@ def deploy( with ui.phase("Registering conventions", count=len(_conventions)) as reg_phase: for conv in _conventions: - hook_defs = [] + releases: dict[str, ComponentRelease] = {} for h in conv.hooks: - name = h.__name__ - if name in hook_images: - image, digest = hook_images[name] - hook_info = next(hi for hi in _hooks if hi.name == name) - hook_defs.append( - _hook_to_definition(hook_info, image, digest, source_ref) + built = hook_images.get(h.__name__) + if built is not None: + image, digest = built + releases[h.__name__] = ComponentRelease( + image=image, digest=digest, source_ref=source_ref + ) + if conv.ingester_info is not None: + built = ingester_images.get(conv.ingester_info.name) + if built is not None: + image, digest = built + releases[conv.ingester_info.name] = ComponentRelease( + image=image, digest=digest, source_ref=source_ref ) - ingester_img = None - if ( - conv.ingester_info is not None - and conv.ingester_info.name in ingester_images - ): - ingester_img = ingester_images[conv.ingester_info.name] - - payload = _convention_to_payload(conv, hook_defs, ingester_img) + payload = bind_releases(build_manifest(conv), releases).model_dump( + by_alias=True, exclude_none=True + ) with reg_phase.task(conv.title) as task: result = _register_convention(conv, payload, server, token) diff --git a/osa/cli/main.py b/osa/cli/main.py index af15708..153c14e 100644 --- a/osa/cli/main.py +++ b/osa/cli/main.py @@ -75,6 +75,62 @@ def meta() -> None: print(meta_command()) +@app.command() +def manifest( + ctx: typer.Context, + skip_docs_check: Annotated[ + bool, + typer.Option( + "--skip-docs-check", help="Skip the mandatory-docs pre-flight gate." + ), + ] = False, +) -> None: + """Emit each registered convention's release-less deploy body as JSON. + + Discovers conventions via the ``osa.conventions`` entry points, so it must + run where the convention package is installed. stdout carries only the JSON + (a `{manifest_version, conventions}` object); diagnostics go to stderr. + """ + import importlib + import importlib.metadata + import json as _json + + from osa._registry import _conventions + from osa.cli.deploy import DeployError, _check_docs_gate, build_manifest + + ui = _ui(ctx) + + for ep in importlib.metadata.entry_points(group="osa.conventions"): + importlib.import_module(ep.value) + + if not _conventions: + ui.error( + "No conventions registered", + hint="Ensure the convention package is installed and exposes an " + "`osa.conventions` entry point", + ) + raise typer.Exit(1) + + if not skip_docs_check: + try: + _check_docs_gate(_conventions) + except DeployError as e: + ui.error(str(e), cause=e.cause, hint=e.hint) + raise typer.Exit(1) from None + + print( + _json.dumps( + { + "manifest_version": 1, + "conventions": [ + build_manifest(c).model_dump(by_alias=True, exclude_none=True) + for c in _conventions + ], + } + ) + ) + + @app.command() def emit(data: Annotated[str, typer.Argument(help="JSON data to emit.")]) -> None: """Write feature data to $OSA_OUT/features.json.""" diff --git a/osa/types/schema.py b/osa/types/schema.py index c0da462..6aacd2a 100644 --- a/osa/types/schema.py +++ b/osa/types/schema.py @@ -24,6 +24,25 @@ _SCHEMA_ID_RE = re.compile(r"^[a-z][a-z0-9\-]{2,63}$") +class FieldDefinition(BaseModel): + """A schema field in the server's FieldDefinition format. + + ``description``/``examples``/``constraints`` are ``None`` when absent; the + producer omits them via ``model_dump(exclude_none=True)`` at the edge. + ``constraints`` stays an open dict: it is a discriminated union keyed by + ``type`` whose ``text`` variant carries arbitrary ``json_schema_extra`` — the + one genuinely dynamic corner here. + """ + + name: str + type: str + required: bool + cardinality: str = "exactly_one" + description: str | None = None + examples: list[str] | None = None + constraints: dict[str, Any] | None = None + + class MetadataSchema(BaseModel): """Base class for defining typed metadata schemas. @@ -63,38 +82,19 @@ def schema_id(cls) -> str: return cls.__schema_id__ @classmethod - def to_field_definitions(cls) -> list[dict[str, Any]]: - """Convert this schema's fields to server FieldDefinition dicts. + def to_field_definitions(cls) -> list[FieldDefinition]: + """Convert this schema's fields to server FieldDefinitions. Maps Python type hints to the server's FieldType format: str → text, int → number (integer_only), float → number, bool → boolean, date/datetime → date, T | None → required=False. """ - result: list[dict[str, Any]] = [] + result: list[FieldDefinition] = [] for name, field_info in cls.model_fields.items(): - annotation = field_info.annotation - required = field_info.is_required() - # Unwrap Optional[T] / T | None - inner = _unwrap_optional(annotation) - - # Resolve field type + inner = _unwrap_optional(field_info.annotation) field_type = _TYPE_MAP.get(inner, "text") - field_def: dict[str, Any] = { - "name": name, - "type": field_type, - "required": required, - "cardinality": "exactly_one", - } - - if field_info.description: - field_def["description"] = field_info.description - - examples = _field_examples(field_info) - if examples: - field_def["examples"] = examples - # Build constraints (discriminated union with "type" key) constraints: dict[str, Any] | None = None if field_type == "number": @@ -112,10 +112,17 @@ def to_field_definitions(cls) -> list[dict[str, Any]]: if text_extra: constraints = {"type": "text", **text_extra} - if constraints: - field_def["constraints"] = constraints - - result.append(field_def) + examples = _field_examples(field_info) + result.append( + FieldDefinition( + name=name, + type=field_type, + required=field_info.is_required(), + description=field_info.description or None, + examples=examples or None, + constraints=constraints, + ) + ) return result diff --git a/tests/test_deploy.py b/tests/test_deploy.py index d8f292b..6a3e023 100644 --- a/tests/test_deploy.py +++ b/tests/test_deploy.py @@ -264,12 +264,12 @@ class DuctilityRow(BaseModel): assert columns["batch"].description is None assert columns["batch"].unit is None - def test_hook_definition_carries_column_metadata(self) -> None: + def test_hook_columns_carry_column_metadata(self) -> None: from pydantic import BaseModel from pydantic import Field as PydanticField from osa._registry import HookInfo - from osa.cli.deploy import _hook_to_definition + from osa.cli.deploy import _hook_columns class Row(BaseModel): score: float = PydanticField( @@ -287,7 +287,199 @@ def fn(record): output_type=Row, cardinality="many", ) - definition = _hook_to_definition(info, "img:latest", "sha256:abc", "git:abc") - col = definition["feature"]["columns"][0] - assert col["description"] == "Pocket score" - assert col["unit"] == "kcal/mol" + col = _hook_columns(info)[0] + assert col.description == "Pocket score" + assert col.unit == "kcal/mol" + + +def _find_null(obj, path=""): + """Return the first JSON path holding a ``None``, else ``None`` (no nulls).""" + if obj is None: + return path or "" + if isinstance(obj, dict): + for k, v in obj.items(): + hit = _find_null(v, f"{path}.{k}") + if hit: + return hit + elif isinstance(obj, list): + for i, v in enumerate(obj): + hit = _find_null(v, f"{path}[{i}]") + if hit: + return hit + return None + + +class TestConventionManifest: + """One symmetric model: `build_manifest` + `bind_releases` + edge dump. + + Hooks and ingesters are both `Component`s (authored config/limits + an + optional nested build `release`); the wire is produced by plain + `model_dump(by_alias=True, exclude_none=True)` — no custom serializers. + """ + + def _manifest(self): + from osa.cli.deploy import ( + ConventionDocs, + ConventionManifest, + Feature, + Hook, + Ingester, + SchemaRef, + ) + from osa.types.ingester import IngesterSchedule, Limits + + return ConventionManifest( + title="T", + description="d", + schema=SchemaRef(id="s", version="1.0.0", fields=[]), + file_requirements={ + "accepted_types": [".csv"], + "max_count": 3, + "max_file_size": 100, + "min_count": 0, + }, + hooks=[ + Hook( + name="detect", + config={}, + limits=Limits(memory="512m"), + feature=Feature(cardinality="many", columns=[]), + ) + ], + ingester=Ingester( + name="ingest", + config={"k": "v"}, + limits=Limits(), + schedule=IngesterSchedule(cron="0 0 * * *"), + initial_run=None, + ), + docs=ConventionDocs(purpose="p", example_questions=[], examples=[]), + ) + + # --- structure / symmetry ------------------------------------------------- + + def test_hook_and_ingester_share_component_base(self) -> None: + from osa.cli.deploy import Component, Hook, Ingester + + assert issubclass(Hook, Component) and issubclass(Ingester, Component) + for f in ("name", "config", "limits", "release"): + assert f in Component.model_fields + assert "feature" in Hook.model_fields + assert {"schedule", "initial_run"} <= set(Ingester.model_fields) + + def test_manifest_has_no_build_or_cloud_fields(self) -> None: + from osa.cli.deploy import Component, ConventionManifest + + # Build artifacts live only under `release`; cloud policy never here. + for f in ("image", "digest", "source_ref"): + assert f not in Component.model_fields + for f in ("slug", "runtime_version"): + assert f not in ConventionManifest.model_fields + + # --- bind_releases (uniform across hooks + ingester) ---------------------- + + def test_bind_sets_release_on_hook_and_ingester(self) -> None: + from osa.cli.deploy import ComponentRelease, bind_releases + + m = self._manifest() + bound = bind_releases( + m, + { + "detect": ComponentRelease(image="i", digest="d", source_ref="g"), + "ingest": ComponentRelease(image="ii", digest="dd", source_ref="g"), + }, + ) + assert bound.hooks[0].release.image == "i" + assert bound.ingester.release.image == "ii" + assert bound.ingester.release.source_ref == "g" + # bind copies — the input manifest is untouched. + assert m.hooks[0].release is None and m.ingester.release is None + + def test_bind_missing_release_stays_none(self) -> None: + from osa.cli.deploy import ComponentRelease, bind_releases + + bound = bind_releases( + self._manifest(), + {"detect": ComponentRelease(image="i", digest="d", source_ref="g")}, + ) + assert bound.hooks[0].release is not None # only detect was built + assert bound.ingester.release is None + + # --- edge serialization --------------------------------------------------- + + def test_release_less_wire_omits_release_and_has_no_nulls(self) -> None: + wire = self._manifest().model_dump(by_alias=True, exclude_none=True) + hook = wire["hooks"][0] + assert "release" not in hook + assert hook["config"] == {} and "limits" in hook and "feature" in hook + ing = wire["ingester"] + assert "release" not in ing and ing["config"] == {"k": "v"} + # ingester is symmetric with hooks: no flat image/digest/runner. + assert not ({"image", "digest", "runner"} & set(ing)) + assert _find_null(wire) is None, f"unexpected null at {_find_null(wire)}" + + def test_bound_wire_nests_release_on_both_symmetrically(self) -> None: + from osa.cli.deploy import ComponentRelease, bind_releases + + wire = bind_releases( + self._manifest(), + { + "detect": ComponentRelease(image="i", digest="d", source_ref="g"), + "ingest": ComponentRelease(image="ii", digest="dd", source_ref="g"), + }, + ).model_dump(by_alias=True, exclude_none=True) + assert wire["hooks"][0]["release"] == { + "image": "i", + "digest": "d", + "source_ref": "g", + } + assert wire["ingester"]["release"] == { + "image": "ii", + "digest": "dd", + "source_ref": "g", + } + # config/limits stay on the component, never inside release. + assert "config" not in wire["hooks"][0]["release"] + assert wire["hooks"][0]["config"] == {} + assert wire["ingester"]["config"] == {"k": "v"} + assert _find_null(wire) is None + + def test_schema_alias_and_ingester_none(self) -> None: + m = self._manifest().model_copy(update={"ingester": None}) + wire = m.model_dump(by_alias=True, exclude_none=True) + assert "schema" in wire and "record_schema" not in wire + assert "ingester" not in wire # None → omitted by exclude_none + + def test_build_manifest_release_free(self) -> None: + from osa import Example, Field, Schema + from osa._registry import _conventions, clear + from osa.authoring.convention import convention + from osa.cli.deploy import ConventionManifest, build_manifest + + clear() + + class Cell(Schema): + __schema_id__ = "cell-schema" + + organism: str = Field(description="Organism") + + convention( + title="Cell Convention", + description="x", + version="1.0.0", + schema=Cell, + files={"accepted_types": [".csv"], "max_count": 1, "max_file_size": 1}, + hooks=[], + purpose="Test data.", + example_questions=["q1?", "q2?", "q3?"], + examples=[Example(question="q1?", query="GET /x", interpretation="x")], + ) + + manifest = build_manifest(_conventions[0]) + assert isinstance(manifest, ConventionManifest) + assert manifest.title == "Cell Convention" + assert manifest.docs.purpose == "Test data." + assert isinstance(manifest.record_schema.fields, list) + # release-less by construction; clean wire. + wire = manifest.model_dump(by_alias=True, exclude_none=True) + assert _find_null(wire) is None diff --git a/tests/test_deploy_v2.py b/tests/test_deploy_v2.py index cede40f..7aa1cc0 100644 --- a/tests/test_deploy_v2.py +++ b/tests/test_deploy_v2.py @@ -65,9 +65,9 @@ def _docs_kwargs() -> dict: } -class TestConventionToPayload: - def test_builds_payload_with_schema_fields(self) -> None: - from osa.cli.deploy import _convention_to_payload +class TestBuildManifest: + def test_builds_manifest_with_schema_fields(self) -> None: + from osa.cli.deploy import build_manifest conv = ConventionInfo( title="Test Convention", @@ -84,17 +84,17 @@ def test_builds_payload_with_schema_fields(self) -> None: **_docs_kwargs(), ) - payload = _convention_to_payload(conv, []) - assert payload["title"] == "Test Convention" - assert payload["description"] == "A test convention" - assert payload["schema"]["id"] == "fake-schema" - assert payload["schema"]["version"] == "1.0.0" - assert len(payload["schema"]["fields"]) == 2 - assert payload["schema"]["fields"][0]["name"] == "title" - assert payload["ingester"] is None + manifest = build_manifest(conv) + assert manifest.title == "Test Convention" + assert manifest.description == "A test convention" + assert manifest.record_schema.id == "fake-schema" + assert manifest.record_schema.version == "1.0.0" + assert len(manifest.record_schema.fields) == 2 + assert manifest.record_schema.fields[0].name == "title" + assert manifest.ingester is None def test_includes_ingester_definition(self) -> None: - from osa.cli.deploy import _convention_to_payload + from osa.cli.deploy import ComponentRelease, bind_releases, build_manifest ingester_info = IngesterInfo(ingester_cls=FakeIngester, name="test-ingester") @@ -113,25 +113,38 @@ def test_includes_ingester_definition(self) -> None: **_docs_kwargs(), ) - payload = _convention_to_payload( - conv, - [], - ingester_image=( - "osa-hooks-ingesters/test-ingester:latest", - "sha256:abc123", - ), - ) - assert payload["ingester"] is not None - assert ( - payload["ingester"]["image"] == "osa-hooks-ingesters/test-ingester:latest" - ) - assert payload["ingester"]["digest"] == "sha256:abc123" - assert payload["ingester"]["runner"] == "oci" - assert payload["ingester"]["config"] == {"email": "", "batch_size": 100} - assert payload["ingester"]["limits"]["timeout_seconds"] == 3600 + # The manifest carries the ingester's authored definition (release-less). + manifest = build_manifest(conv) + assert manifest.ingester is not None + assert manifest.ingester.name == "test-ingester" + assert manifest.ingester.config == {"email": "", "batch_size": 100} + assert manifest.ingester.limits.timeout_seconds == 3600 + assert manifest.ingester.release is None + # build artifacts live only under `release`, not flat on the ingester. + assert "image" not in type(manifest.ingester).model_fields + + # Binding a built release nests it — symmetric with hooks (no flat image, + # config/limits/name stay on the ingester). + wire = bind_releases( + manifest, + { + "test-ingester": ComponentRelease( + image="osa-hooks-ingesters/test-ingester:latest", + digest="sha256:abc123", + source_ref="git:x", + ) + }, + ).model_dump(by_alias=True, exclude_none=True) + assert wire["ingester"]["release"] == { + "image": "osa-hooks-ingesters/test-ingester:latest", + "digest": "sha256:abc123", + "source_ref": "git:x", + } + assert wire["ingester"]["name"] == "test-ingester" + assert "config" in wire["ingester"] and "limits" in wire["ingester"] def test_ingester_none_when_no_ingester(self) -> None: - from osa.cli.deploy import _convention_to_payload + from osa.cli.deploy import build_manifest conv = ConventionInfo( title="Test", @@ -144,12 +157,12 @@ def test_ingester_none_when_no_ingester(self) -> None: **_docs_kwargs(), ) - payload = _convention_to_payload(conv, []) - assert payload["ingester"] is None + assert build_manifest(conv).ingester is None - def test_ingester_none_when_no_ingester_image_provided(self) -> None: - """Even with ingester_info, if no ingester_image tuple is given, ingester is None.""" - from osa.cli.deploy import _convention_to_payload + def test_manifest_includes_ingester_without_a_build(self) -> None: + """The ingester is part of the definition, so it appears release-less in + the manifest even before any build (unlike the old deploy-only payload).""" + from osa.cli.deploy import bind_releases, build_manifest ingester_info = IngesterInfo(ingester_cls=FakeIngester, name="test-ingester") @@ -164,11 +177,15 @@ def test_ingester_none_when_no_ingester_image_provided(self) -> None: **_docs_kwargs(), ) - payload = _convention_to_payload(conv, []) - assert payload["ingester"] is None + manifest = build_manifest(conv) + assert manifest.ingester is not None + # Release-less wire (no releases bound): present, named, no release. + wire = bind_releases(manifest, {}).model_dump(by_alias=True, exclude_none=True) + assert wire["ingester"]["name"] == "test-ingester" + assert "release" not in wire["ingester"] def test_adds_min_count_if_missing(self) -> None: - from osa.cli.deploy import _convention_to_payload + from osa.cli.deploy import build_manifest conv = ConventionInfo( title="Test", @@ -185,29 +202,23 @@ def test_adds_min_count_if_missing(self) -> None: **_docs_kwargs(), ) - payload = _convention_to_payload(conv, []) - assert payload["file_requirements"]["min_count"] == 0 + assert build_manifest(conv).file_requirements.min_count == 0 def test_includes_hook_definitions(self) -> None: - from osa.cli.deploy import _convention_to_payload + from osa._registry import _hooks + from osa.cli.deploy import ComponentRelease, bind_releases, build_manifest - hook_defs = [ - { - "name": "detect_pockets", - "feature": { - "kind": "table", - "cardinality": "many", - "columns": [], - }, - "release": { - "image": "osa-hooks/detect_pockets:latest", - "digest": "sha256:abc123", - "config": {}, - "limits": {"timeout_seconds": 300, "memory": "2g", "cpu": "2.0"}, - "source_ref": "git:abc1234", - }, - } - ] + clear() + _hooks.append( + HookInfo( + fn=fake_hook, + name="detect_pockets", + hook_type="hook", + schema_type=FakeSchema, + output_type=None, + cardinality="many", + ) + ) conv = ConventionInfo( title="Test", @@ -224,18 +235,34 @@ def test_includes_hook_definitions(self) -> None: **_docs_kwargs(), ) - payload = _convention_to_payload(conv, hook_defs) - assert len(payload["hooks"]) == 1 - assert ( - payload["hooks"][0]["release"]["image"] == "osa-hooks/detect_pockets:latest" - ) + manifest = build_manifest(conv) + assert len(manifest.hooks) == 1 + assert manifest.hooks[0].name == "detect_pockets" + + wire = bind_releases( + manifest, + { + "detect_pockets": ComponentRelease( + image="osa-hooks/detect_pockets:latest", + digest="sha256:abc123", + source_ref="git:abc1234", + ) + }, + ).model_dump(by_alias=True, exclude_none=True) + assert wire["hooks"][0]["release"] == { + "image": "osa-hooks/detect_pockets:latest", + "digest": "sha256:abc123", + "source_ref": "git:abc1234", + } + # config/limits are on the hook, not in release. + assert wire["hooks"][0]["config"] == {} -class TestHookToDefinition: - def test_builds_hook_definition(self) -> None: +class TestHookColumns: + def test_columns_from_output_model(self) -> None: from pydantic import BaseModel - from osa.cli.deploy import _hook_to_definition + from osa.cli.deploy import _hook_columns class Pocket(BaseModel): pocket_id: int @@ -249,19 +276,10 @@ class Pocket(BaseModel): output_type=Pocket, cardinality="many", ) - - defn = _hook_to_definition( - hook_info, "osa-hooks/detect_pockets:latest", "sha256:abc", "git:abc1234" - ) - assert defn["release"]["image"] == "osa-hooks/detect_pockets:latest" - assert defn["release"]["digest"] == "sha256:abc" - assert defn["release"]["source_ref"] == "git:abc1234" - assert defn["name"] == "detect_pockets" - assert defn["feature"]["cardinality"] == "many" - assert len(defn["feature"]["columns"]) == 2 + assert len(_hook_columns(hook_info)) == 2 def test_empty_columns_when_no_output_type(self) -> None: - from osa.cli.deploy import _hook_to_definition + from osa.cli.deploy import _hook_columns hook_info = HookInfo( fn=fake_hook, @@ -271,9 +289,7 @@ def test_empty_columns_when_no_output_type(self) -> None: output_type=None, cardinality="one", ) - - defn = _hook_to_definition(hook_info, "img:latest", "sha256:xyz", "git:unknown") - assert defn["feature"]["columns"] == [] + assert _hook_columns(hook_info) == [] class TestResolveSourceRef: @@ -408,7 +424,10 @@ def test_builds_and_registers(self) -> None: assert payload["title"] == "PDB Structures" assert payload["schema"]["id"] == "fake-schema" assert payload["hooks"][0]["release"]["source_ref"].startswith("git:") + # config/limits are authored, on the component — not inside `release`. + assert payload["hooks"][0]["config"] == {} + assert "config" not in payload["hooks"][0]["release"] assert payload["ingester"] is not None - assert payload["ingester"]["runner"] == "oci" + assert payload["ingester"]["release"]["source_ref"].startswith("git:") assert payload["ingester"]["config"] == {"email": "", "batch_size": 100} assert "Bearer fake-jwt" in call_args[1]["headers"]["Authorization"] diff --git a/tests/test_docs_gate.py b/tests/test_docs_gate.py index 47c98f0..c6bdd39 100644 --- a/tests/test_docs_gate.py +++ b/tests/test_docs_gate.py @@ -159,14 +159,13 @@ def setup_method(self) -> None: def test_payload_carries_required_docs_block(self) -> None: from osa._registry import _conventions - from osa.cli.deploy import _convention_to_payload + from osa.cli.deploy import build_manifest _register() - payload = _convention_to_payload(_conventions[0], []) - docs = payload["docs"] - assert docs["purpose"] == "Test data." - assert docs["example_questions"] == ["q1?", "q2?", "q3?"] - assert docs["examples"] == [ + docs = build_manifest(_conventions[0]).docs + assert docs.purpose == "Test data." + assert docs.example_questions == ["q1?", "q2?", "q3?"] + assert [e.model_dump() for e in docs.examples] == [ {"question": "question 0?", "query": "GET /x", "interpretation": "means x"} ] @@ -187,8 +186,8 @@ def test_payload_carries_optional_docs_fields(self) -> None: when_not_to_use="Not for X.", see_also=["https://other-node.example.org"], ) - from osa.cli.deploy import _convention_to_payload + from osa.cli.deploy import build_manifest - payload = _convention_to_payload(_conventions[0], []) - assert payload["docs"]["when_not_to_use"] == "Not for X." - assert payload["docs"]["see_also"] == ["https://other-node.example.org"] + docs = build_manifest(_conventions[0]).docs + assert docs.when_not_to_use == "Not for X." + assert docs.see_also == ["https://other-node.example.org"] diff --git a/tests/test_to_field_definitions.py b/tests/test_to_field_definitions.py index 3452aa4..f799c7e 100644 --- a/tests/test_to_field_definitions.py +++ b/tests/test_to_field_definitions.py @@ -40,20 +40,29 @@ class UnitSchema(MetadataSchema): class TestToFieldDefinitions: def test_str_maps_to_text(self) -> None: - fields = SimpleSchema.to_field_definitions() + fields = [ + _fd.model_dump(exclude_none=True) + for _fd in SimpleSchema.to_field_definitions() + ] title_field = next(f for f in fields if f["name"] == "title") assert title_field["type"] == "text" assert title_field["required"] is True def test_int_maps_to_number_integer_only(self) -> None: - fields = SimpleSchema.to_field_definitions() + fields = [ + _fd.model_dump(exclude_none=True) + for _fd in SimpleSchema.to_field_definitions() + ] count_field = next(f for f in fields if f["name"] == "count") assert count_field["type"] == "number" assert count_field["constraints"]["type"] == "number" assert count_field["constraints"]["integer_only"] is True def test_float_maps_to_number(self) -> None: - fields = SimpleSchema.to_field_definitions() + fields = [ + _fd.model_dump(exclude_none=True) + for _fd in SimpleSchema.to_field_definitions() + ] score_field = next(f for f in fields if f["name"] == "score") assert score_field["type"] == "number" # float has number constraints but not integer_only @@ -61,49 +70,76 @@ def test_float_maps_to_number(self) -> None: assert score_field["constraints"].get("integer_only") is not True def test_bool_maps_to_boolean(self) -> None: - fields = SimpleSchema.to_field_definitions() + fields = [ + _fd.model_dump(exclude_none=True) + for _fd in SimpleSchema.to_field_definitions() + ] active_field = next(f for f in fields if f["name"] == "active") assert active_field["type"] == "boolean" assert active_field["required"] is True def test_date_maps_to_date(self) -> None: - fields = DateSchema.to_field_definitions() + fields = [ + _fd.model_dump(exclude_none=True) + for _fd in DateSchema.to_field_definitions() + ] created_field = next(f for f in fields if f["name"] == "created") assert created_field["type"] == "date" def test_datetime_maps_to_date(self) -> None: - fields = DateSchema.to_field_definitions() + fields = [ + _fd.model_dump(exclude_none=True) + for _fd in DateSchema.to_field_definitions() + ] updated_field = next(f for f in fields if f["name"] == "updated") assert updated_field["type"] == "date" def test_optional_field_not_required(self) -> None: - fields = OptionalSchema.to_field_definitions() + fields = [ + _fd.model_dump(exclude_none=True) + for _fd in OptionalSchema.to_field_definitions() + ] desc_field = next(f for f in fields if f["name"] == "description") assert desc_field["required"] is False def test_required_field_is_required(self) -> None: - fields = OptionalSchema.to_field_definitions() + fields = [ + _fd.model_dump(exclude_none=True) + for _fd in OptionalSchema.to_field_definitions() + ] name_field = next(f for f in fields if f["name"] == "name") assert name_field["required"] is True def test_unit_in_constraints(self) -> None: - fields = UnitSchema.to_field_definitions() + fields = [ + _fd.model_dump(exclude_none=True) + for _fd in UnitSchema.to_field_definitions() + ] res_field = next(f for f in fields if f["name"] == "resolution") assert res_field["constraints"]["unit"] == "Å" def test_unit_on_required_field(self) -> None: - fields = UnitSchema.to_field_definitions() + fields = [ + _fd.model_dump(exclude_none=True) + for _fd in UnitSchema.to_field_definitions() + ] weight_field = next(f for f in fields if f["name"] == "weight") assert weight_field["constraints"]["unit"] == "kDa" assert weight_field["required"] is True def test_cardinality_defaults_to_exactly_one(self) -> None: - fields = SimpleSchema.to_field_definitions() + fields = [ + _fd.model_dump(exclude_none=True) + for _fd in SimpleSchema.to_field_definitions() + ] for f in fields: assert f["cardinality"] == "exactly_one" def test_returns_list_of_dicts(self) -> None: - fields = SimpleSchema.to_field_definitions() + fields = [ + _fd.model_dump(exclude_none=True) + for _fd in SimpleSchema.to_field_definitions() + ] assert isinstance(fields, list) assert all(isinstance(f, dict) for f in fields) assert len(fields) == 4 @@ -124,24 +160,36 @@ class TestFieldMetadataEmission: """#151: description and examples must reach the server, not be dropped.""" def test_description_is_emitted(self) -> None: - fields = DocumentedSchema.to_field_definitions() + fields = [ + _fd.model_dump(exclude_none=True) + for _fd in DocumentedSchema.to_field_definitions() + ] f = next(f for f in fields if f["name"] == "yield_strength") assert f["description"] == "0.2% offset yield strength" def test_examples_are_emitted(self) -> None: - fields = DocumentedSchema.to_field_definitions() + fields = [ + _fd.model_dump(exclude_none=True) + for _fd in DocumentedSchema.to_field_definitions() + ] f = next(f for f in fields if f["name"] == "yield_strength") assert f["examples"] == ["512"] def test_unit_still_in_constraints(self) -> None: - fields = DocumentedSchema.to_field_definitions() + fields = [ + _fd.model_dump(exclude_none=True) + for _fd in DocumentedSchema.to_field_definitions() + ] f = next(f for f in fields if f["name"] == "yield_strength") assert f["constraints"]["unit"] == "MPa" # examples must NOT leak into the constraints block assert "examples" not in f["constraints"] def test_undocumented_field_has_no_new_keys(self) -> None: - fields = DocumentedSchema.to_field_definitions() + fields = [ + _fd.model_dump(exclude_none=True) + for _fd in DocumentedSchema.to_field_definitions() + ] f = next(f for f in fields if f["name"] == "alloy") assert "description" not in f assert "examples" not in f diff --git a/uv.lock b/uv.lock index a6a6e43..9fb5c5a 100644 --- a/uv.lock +++ b/uv.lock @@ -140,7 +140,7 @@ wheels = [ [[package]] name = "osa-py" -version = "0.5.1" +version = "0.6.0" source = { editable = "." } dependencies = [ { name = "httpx" }, From d7650a1da7b2611f94a23791f3f1a015f0eee3e6 Mon Sep 17 00:00:00 2001 From: Rory Byrne Date: Wed, 22 Jul 2026 09:55:14 +0100 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20address=20PR=20review=20=E2=80=94=20?= =?UTF-8?q?release-map=20collision=20+=20entry-point=20loading?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - bind_releases: bind hook releases (by name) and the single ingester release separately, so a hook and the ingester sharing a name can no longer collide in a shared keyspace (adds a regression test). - entry-point discovery: use ep.load() instead of import_module(ep.value) so object-form entry points (package.module:attr) also load, across the manifest/deploy/test commands. Co-Authored-By: Claude Opus 4.8 (1M context) --- osa/cli/deploy.py | 30 ++++++++++++++---------- osa/cli/main.py | 6 ++--- tests/test_deploy.py | 52 ++++++++++++++++++++++++++++++++++------- tests/test_deploy_v2.py | 18 +++++++------- 4 files changed, 75 insertions(+), 31 deletions(-) diff --git a/osa/cli/deploy.py b/osa/cli/deploy.py index a4edded..0856d59 100644 --- a/osa/cli/deploy.py +++ b/osa/cli/deploy.py @@ -510,16 +510,21 @@ def build_manifest(conv: ConventionInfo) -> ConventionManifest: def bind_releases( - manifest: ConventionManifest, releases: dict[str, ComponentRelease] + manifest: ConventionManifest, + hook_releases: dict[str, ComponentRelease], + ingester_release: ComponentRelease | None = None, ) -> ConventionManifest: - """Return a copy of the manifest with each built component's release bound by - name — uniform across hooks and the ingester. ``osa deploy`` uses it; ``osa - manifest`` skips it (releases stay ``None``, omitted at serialization).""" + """Return a copy of the manifest with each built component's release bound. + + Hooks are matched by name; the (single) ingester takes its release directly. + Keeping the two separate means a hook and the ingester sharing a name cannot + collide in a shared keyspace. ``osa deploy`` uses this; ``osa manifest`` + skips it (releases stay ``None``, omitted at serialization).""" bound = manifest.model_copy(deep=True) for hook in bound.hooks: - hook.release = releases.get(hook.name) + hook.release = hook_releases.get(hook.name) if bound.ingester is not None: - bound.ingester.release = releases.get(bound.ingester.name) + bound.ingester.release = ingester_release return bound @@ -753,25 +758,26 @@ def deploy( with ui.phase("Registering conventions", count=len(_conventions)) as reg_phase: for conv in _conventions: - releases: dict[str, ComponentRelease] = {} + hook_releases: dict[str, ComponentRelease] = {} for h in conv.hooks: built = hook_images.get(h.__name__) if built is not None: image, digest = built - releases[h.__name__] = ComponentRelease( + hook_releases[h.__name__] = ComponentRelease( image=image, digest=digest, source_ref=source_ref ) + ingester_release: ComponentRelease | None = None if conv.ingester_info is not None: built = ingester_images.get(conv.ingester_info.name) if built is not None: image, digest = built - releases[conv.ingester_info.name] = ComponentRelease( + ingester_release = ComponentRelease( image=image, digest=digest, source_ref=source_ref ) - payload = bind_releases(build_manifest(conv), releases).model_dump( - by_alias=True, exclude_none=True - ) + payload = bind_releases( + build_manifest(conv), hook_releases, ingester_release + ).model_dump(by_alias=True, exclude_none=True) with reg_phase.task(conv.title) as task: result = _register_convention(conv, payload, server, token) diff --git a/osa/cli/main.py b/osa/cli/main.py index 153c14e..12a3e78 100644 --- a/osa/cli/main.py +++ b/osa/cli/main.py @@ -101,7 +101,7 @@ def manifest( ui = _ui(ctx) for ep in importlib.metadata.entry_points(group="osa.conventions"): - importlib.import_module(ep.value) + ep.load() # ep.load() handles both module- and object-form entry points if not _conventions: ui.error( @@ -226,7 +226,7 @@ def deploy( ui = _ui(ctx) for ep in importlib.metadata.entry_points(group="osa.conventions"): - importlib.import_module(ep.value) + ep.load() # ep.load() handles both module- and object-form entry points server_url, server_source = resolve_server_with_source(flag=server) resolved_token = token @@ -440,7 +440,7 @@ def test_cmd( ui = _ui(ctx) for ep in importlib.metadata.entry_points(group="osa.conventions"): - importlib.import_module(ep.value) + ep.load() # ep.load() handles both module- and object-form entry points candidates = [c for c in _conventions if c.ingester_info is not None] diff --git a/tests/test_deploy.py b/tests/test_deploy.py index 6a3e023..ecc9784 100644 --- a/tests/test_deploy.py +++ b/tests/test_deploy.py @@ -384,10 +384,8 @@ def test_bind_sets_release_on_hook_and_ingester(self) -> None: m = self._manifest() bound = bind_releases( m, - { - "detect": ComponentRelease(image="i", digest="d", source_ref="g"), - "ingest": ComponentRelease(image="ii", digest="dd", source_ref="g"), - }, + {"detect": ComponentRelease(image="i", digest="d", source_ref="g")}, + ComponentRelease(image="ii", digest="dd", source_ref="g"), ) assert bound.hooks[0].release.image == "i" assert bound.ingester.release.image == "ii" @@ -401,10 +399,50 @@ def test_bind_missing_release_stays_none(self) -> None: bound = bind_releases( self._manifest(), {"detect": ComponentRelease(image="i", digest="d", source_ref="g")}, + None, ) assert bound.hooks[0].release is not None # only detect was built assert bound.ingester.release is None + def test_same_name_hook_and_ingester_do_not_collide(self) -> None: + # A hook and the ingester sharing a name must not clobber each other in a + # shared keyspace (regression: they used to share one name-keyed map). + from osa.cli.deploy import ( + ComponentRelease, + ConventionDocs, + ConventionManifest, + Feature, + Hook, + Ingester, + SchemaRef, + bind_releases, + ) + from osa.types.ingester import Limits + + m = ConventionManifest( + title="T", + description="d", + schema=SchemaRef(id="s", version="1.0.0", fields=[]), + file_requirements={"min_count": 0}, + hooks=[ + Hook( + name="shared", + config={}, + limits=Limits(), + feature=Feature(cardinality="many", columns=[]), + ) + ], + ingester=Ingester(name="shared", config={}, limits=Limits()), + docs=ConventionDocs(purpose="p", example_questions=[], examples=[]), + ) + bound = bind_releases( + m, + {"shared": ComponentRelease(image="hook-img", digest="hd", source_ref="g")}, + ComponentRelease(image="ing-img", digest="id", source_ref="g"), + ) + assert bound.hooks[0].release.image == "hook-img" + assert bound.ingester.release.image == "ing-img" + # --- edge serialization --------------------------------------------------- def test_release_less_wire_omits_release_and_has_no_nulls(self) -> None: @@ -423,10 +461,8 @@ def test_bound_wire_nests_release_on_both_symmetrically(self) -> None: wire = bind_releases( self._manifest(), - { - "detect": ComponentRelease(image="i", digest="d", source_ref="g"), - "ingest": ComponentRelease(image="ii", digest="dd", source_ref="g"), - }, + {"detect": ComponentRelease(image="i", digest="d", source_ref="g")}, + ComponentRelease(image="ii", digest="dd", source_ref="g"), ).model_dump(by_alias=True, exclude_none=True) assert wire["hooks"][0]["release"] == { "image": "i", diff --git a/tests/test_deploy_v2.py b/tests/test_deploy_v2.py index 7aa1cc0..9a6d009 100644 --- a/tests/test_deploy_v2.py +++ b/tests/test_deploy_v2.py @@ -127,13 +127,12 @@ def test_includes_ingester_definition(self) -> None: # config/limits/name stay on the ingester). wire = bind_releases( manifest, - { - "test-ingester": ComponentRelease( - image="osa-hooks-ingesters/test-ingester:latest", - digest="sha256:abc123", - source_ref="git:x", - ) - }, + {}, + ComponentRelease( + image="osa-hooks-ingesters/test-ingester:latest", + digest="sha256:abc123", + source_ref="git:x", + ), ).model_dump(by_alias=True, exclude_none=True) assert wire["ingester"]["release"] == { "image": "osa-hooks-ingesters/test-ingester:latest", @@ -180,7 +179,9 @@ def test_manifest_includes_ingester_without_a_build(self) -> None: manifest = build_manifest(conv) assert manifest.ingester is not None # Release-less wire (no releases bound): present, named, no release. - wire = bind_releases(manifest, {}).model_dump(by_alias=True, exclude_none=True) + wire = bind_releases(manifest, {}, None).model_dump( + by_alias=True, exclude_none=True + ) assert wire["ingester"]["name"] == "test-ingester" assert "release" not in wire["ingester"] @@ -248,6 +249,7 @@ def test_includes_hook_definitions(self) -> None: source_ref="git:abc1234", ) }, + None, ).model_dump(by_alias=True, exclude_none=True) assert wire["hooks"][0]["release"] == { "image": "osa-hooks/detect_pockets:latest",