diff --git a/cli/alora/README_TEMPLATE.jinja b/cli/alora/README_TEMPLATE.jinja index d2a01c5bd9..35c8659500 100644 --- a/cli/alora/README_TEMPLATE.jinja +++ b/cli/alora/README_TEMPLATE.jinja @@ -27,11 +27,19 @@ tags: ## How to Use -To use this intrinsic adapter in your application, you need to define an adapter class -and an intrinsic component. The adapter connects to the HuggingFace model repository +To use this intrinsic adapter in your application, you need to compose an adapter +(a factory function returning it, rather than a dedicated shim class) and an intrinsic +component. The adapter connects to the HuggingFace model repository where the trained weights are stored, and the intrinsic wraps it as a composable Mellea component that can be used with any compatible backend. +> **Repo layout:** the code below only works if `{{ userid }}/{{ intrinsic_name }}` +> was uploaded with `m alora upload ... --intrinsic --io-yaml ./io.yaml` — that +> command places weights at `{{ intrinsic_name }}//alora/`, which is +> where `LocalFileBinding` looks for them. The plain `m alora upload` (without +> `--intrinsic`) pushes weights to the repo root instead, and this code will +> fail with `ValueError: Intrinsic '{{ intrinsic_name }}' ... not found`. + Make sure you have Mellea installed (`uv pip install mellea`), then use the code below to integrate this intrinsic into your application. @@ -41,26 +49,49 @@ from mellea.core import Context from mellea.backends import Backend from mellea.stdlib.components.simple import SimpleComponent from mellea.backends.adapters import AdapterMixin -from mellea.backends.adapters.adapter import CustomIntrinsicAdapter +from mellea.backends.adapters import Adapter, Identity, LocalFileBinding, get_io_contract +from mellea.backends.adapters.catalog import AdapterType from mellea.stdlib.components.intrinsic import Intrinsic _INTRINSIC_MODEL_ID = "{{ userid }}/{{ intrinsic_name }}" _INTRINSIC_ADAPTER_NAME = "{{ intrinsic_name }}" - - -class {{ intrinsic_name_camelcase }}Adapter(CustomIntrinsicAdapter): - def __init__(self, base_model_name: str): - super().__init__( - model_id=_INTRINSIC_MODEL_ID, - intrinsic_name=_INTRINSIC_ADAPTER_NAME, - base_model_name=base_model_name, - ) +_INTRINSIC_QUALIFIED_NAME = f"{_INTRINSIC_ADAPTER_NAME}_{AdapterType.ALORA.value}" + + +def _{{ intrinsic_name }}_adapter() -> Adapter: + """Compose this trained adapter (LoRA/aLoRA weights at `_INTRINSIC_MODEL_ID`) + from its Identity, output contract, and weights binding, rather than a + dedicated shim class.""" + return Adapter( + identity=Identity(name=_INTRINSIC_ADAPTER_NAME, adapter_type="alora"), + io_contract=get_io_contract(_INTRINSIC_ADAPTER_NAME), + weights=LocalFileBinding( + name=_INTRINSIC_ADAPTER_NAME, + adapter_type=AdapterType.ALORA, + repo_id=_INTRINSIC_MODEL_ID, + # A custom, non-catalog adapter has no catalog entry to resolve + # revision=None from — pin it explicitly. + revision="main", + ), + ) class {{ intrinsic_name_camelcase }}Intrinsic(Intrinsic, SimpleComponent): def __init__(self, {{ arglist }}): - Intrinsic.__init__(self, intrinsic_name=_INTRINSIC_ADAPTER_NAME) + # adapter_types is required here: "{{ intrinsic_name }}" is outside + # Mellea's intrinsics catalog, so Intrinsic has no catalog entry to + # fall back to. + Intrinsic.__init__( + self, + intrinsic_name=_INTRINSIC_ADAPTER_NAME, + adapter_types=(AdapterType.ALORA,), + # `_generate_from_intrinsic` never adds `action` to the context — + # intrinsics reach the model only through their rewriter, driven + # by `intrinsic_kwargs` — so these arguments must be passed here, + # not just stored on the SimpleComponent side below. + intrinsic_kwargs=dict({{ arglist_as_kwargs }}), + ) SimpleComponent.__init__(self, {{ arglist_as_kwargs }}) def format_for_llm(self): @@ -68,18 +99,17 @@ class {{ intrinsic_name_camelcase }}Intrinsic(Intrinsic, SimpleComponent): async def async_{{ intrinsic_name }}({{ arglist }}, ctx: Context, backend: Backend | AdapterMixin): # Backend.add_adapter should be idempotent, but we'll go ahead and check just in case. - if adapter.qualified_name not in backend.list_adapters(): - backend.add_adapter({{ intrinsic_name_camelcase }}Adapter(backend.base_model_name)) - action = {{ intrinsic_name_camelcase }}Intrinsic("{{ intrinsic_name }}", {{ arglist_without_type_annotations }}) - mot = await backend.generate_from_context(action, ctx) - return mot + if _INTRINSIC_QUALIFIED_NAME not in backend.list_adapters(): + backend.add_adapter(_{{ intrinsic_name }}_adapter()) + action = {{ intrinsic_name_camelcase }}Intrinsic({{ arglist_without_type_annotations }}) + mot, ctx = await backend.generate_from_context(action, ctx) + return mot, ctx def {{ intrinsic_name }}({{ arglist }}, ctx: Context, backend: Backend | AdapterMixin): # Backend.add_adapter should be idempotent, but we'll go ahead and check just in case. - adapter = {{ intrinsic_name_camelcase }}Adapter(backend.base_model_name) - if adapter.qualified_name not in backend.list_adapters(): - backend.add_adapter(adapter) + if _INTRINSIC_QUALIFIED_NAME not in backend.list_adapters(): + backend.add_adapter(_{{ intrinsic_name }}_adapter()) action = {{ intrinsic_name_camelcase }}Intrinsic({{ arglist_without_type_annotations }}) return mfuncs.act(action, ctx, backend) diff --git a/docs/docs/advanced/intrinsics.md b/docs/docs/advanced/intrinsics.md index 298beb27dc..5147123653 100644 --- a/docs/docs/advanced/intrinsics.md +++ b/docs/docs/advanced/intrinsics.md @@ -214,24 +214,41 @@ print(result) ## Direct adapter function usage -> **Advanced:** For custom adapter tasks, use the `Intrinsic` component and -> `CustomIntrinsicAdapter` directly. +> **Advanced:** For custom adapter tasks, compose an `Adapter` directly from +> an `Identity`, an output contract, and a weights binding. ```python # Requires: mellea[hf] # Returns: dict import mellea.stdlib.functional as mfuncs -from mellea.backends.adapters.adapter import CustomIntrinsicAdapter +from mellea.backends.adapters import Adapter, Identity, LocalFileBinding, get_io_contract +from mellea.backends.adapters.catalog import AdapterType, fetch_intrinsic_metadata from mellea.backends.huggingface import LocalHFBackend from mellea.stdlib.components import Intrinsic, Message from mellea.stdlib.context import ChatContext backend = LocalHFBackend(model_id="ibm-granite/granite-4.1-3b") -# Register an adapter by task name -req_adapter = CustomIntrinsicAdapter( - "requirement-check", - base_model_name=backend.base_model_name, +# Compose an adapter by task name — get_io_contract returns the catalog's +# declared contract (or a permissive fallback for a name outside it). +# requirement-check's catalog entry lists LoRA before aLoRA, so +# LocalFileBinding.from_catalog would pick LoRA — build the binding +# explicitly instead when you want the aLoRA variant specifically; identity +# and weights must agree, since nothing currently cross-checks them. +_metadata = fetch_intrinsic_metadata("requirement-check") +req_adapter = Adapter( + identity=Identity( + name="requirement-check", + adapter_type="alora", + capability=_metadata.effective_capability, + ), + io_contract=get_io_contract("requirement-check"), + weights=LocalFileBinding( + name="requirement-check", + adapter_type=AdapterType.ALORA, + repo_id=_metadata.repo_id, + revision=_metadata.revision, + ), ) backend.add_adapter(req_adapter) @@ -255,80 +272,82 @@ For OpenAI backends with Granite Switch, adapters are loaded from the model's Hugging Face repository configuration instead of the adapter function catalog. Output format is task-specific — `requirement-check` returns `{"requirement_check": {"score": }}`. +For a fully custom, non-catalog adapter — your own trained LoRA/aLoRA weights, +not one of the built-in adapter functions — see +[Adding a custom adapter function in 20 lines](../tutorials/07-custom-adapter-function.md). + ## Composable adapter construction (advanced) -> **Advanced:** `Adapter` composes an `Identity`, an `IOContract`, and a -> weights binding into a single, inspectable object. It's scaffolding for a -> future backend-integration surface (Epic #929) — today, neither backend -> accepts a composed `Adapter` directly: `LocalHFBackend.add_adapter` takes a -> `LocalFileBinding` or the `LocalHFAdapter` shim, while -> `OpenAIBackend.add_adapter` takes only the deprecated -> `EmbeddedIntrinsicAdapter` shim, which builds an `EmbeddedBinding` -> internally. The construction below is illustrative of the binding shapes; -> write a new backend integration against the bindings themselves. +`Adapter` composes an `Identity`, an output contract (`IOContract`), and a +weights binding into a single, inspectable object. Both `LocalHFBackend` and +`OpenAIBackend` accept a composed `Adapter` directly via `add_adapter` — +dispatching on the weights binding's reality (`LocalFileBinding` for +LocalFile/PEFT, `EmbeddedBinding` for Embedded/Granite Switch) — alongside +the deprecated shim classes, which remain functional for now (Epic #929, +issue #1144). An `EmbeddedBinding` adapter additionally requires `add_adapter`'s +`config=` argument (the raw io.yaml mapping) — `add_adapter` raises `ValueError` +without it, since that reality's config cannot be cheaply re-derived later; see +below. Each weights binding models how its deployment turns an adapter on. `LocalFileBinding` downloads and loads LoRA/aLoRA weights, so it exposes a -`prepare`/`activate`/`deactivate`/`release` lifecycle: +`prepare`/`activate`/`deactivate`/`release` lifecycle — driven automatically +by `add_adapter`, so a caller need not call `prepare()` itself: ```python # Requires: mellea[hf] -from mellea.backends.adapters import Adapter, EmbeddedBinding, Identity, IOContract, LocalFileBinding +from mellea.backends.adapters import Adapter, Identity, LocalFileBinding, get_io_contract from mellea.backends.huggingface import LocalHFBackend -from mellea.backends.openai import OpenAIBackend -from mellea.core import Component - - -class AnswerabilityContract(IOContract): - def build_prompt(self, **kwargs: object) -> Component: - raise NotImplementedError # request formatting lands with #1516 - - def parse(self, raw: str) -> dict[str, object]: - import json - - return json.loads(raw) - # LocalFile/PEFT reality — LocalHFBackend downloads and loads the weights. hf_backend = LocalHFBackend(model_id="ibm-granite/granite-4.1-3b") -hf_binding = LocalFileBinding.from_catalog("answerability") -hf_binding.bind_backend(hf_backend) -# hf_binding.prepare() downloads the weights and loads them into hf_backend. -# adapter_type must match the binding — from_catalog loads the first -# catalog-listed adapter type, which is LoRA for answerability. hf_adapter = Adapter( identity=Identity(name="answerability", adapter_type="lora"), - io_contract=AnswerabilityContract(), - weights=hf_binding, + io_contract=get_io_contract("answerability"), + weights=LocalFileBinding.from_catalog("answerability"), ) +hf_backend.add_adapter(hf_adapter) # downloads and loads the weights ``` `EmbeddedBinding` has no weights to manage — the adapter is already part of the served base model — so it exposes a single method, `apply_activation`, -that edits the outgoing request instead of a lifecycle: +that edits the outgoing request instead of a lifecycle. Its `io.yaml` config +comes from the served checkpoint's `adapter_index.json`, not from anything +you can construct by hand, so registration goes through +`register_embedded_adapter_model` (or `resolve_adapter`) rather than a bare +`add_adapter(adapter)` call with no `config=`: ```python +from mellea.backends.openai import OpenAIBackend +from mellea.backends.model_ids import IBM_GRANITE_SWITCH_4_1_3B_PREVIEW + switch_backend = OpenAIBackend( - model_id="granite-switch", + model_id=IBM_GRANITE_SWITCH_4_1_3B_PREVIEW.hf_model_name, api_key="EMPTY", base_url="http://localhost:8000/v1", + load_embedded_adapters=False, ) -switch_adapter = Adapter( - identity=Identity(name="answerability", adapter_type="alora"), - io_contract=AnswerabilityContract(), - weights=EmbeddedBinding.from_base_model(switch_backend), +# Discovers "answerability" from the model's Hugging Face repo and composes +# an Adapter (Identity + IOContract + EmbeddedBinding) for it, including the +# io.yaml config a bare Adapter(weights=EmbeddedBinding.from_base_model(...)) +# construction has no way to supply. +switch_backend.register_embedded_adapter_model( + IBM_GRANITE_SWITCH_4_1_3B_PREVIEW.hf_model_name, intrinsic_name="answerability" ) ``` -Weights-binding support by backend today — this tracks the binding -implementations, not whether a composed `Adapter` can be registered directly: +Weights-binding support by backend today: | Backend | `LocalFileBinding` (LocalFile/PEFT) | `EmbeddedBinding` (Embedded/Granite Switch) | `ServerMediatedBinding` | | --- | --- | --- | --- | -| `LocalHFBackend` | ✅ shipping — `add_adapter` accepts a `LocalFileBinding` directly | ✅ shipping — `load_embedded_adapters=True`, via the deprecated `EmbeddedIntrinsicAdapter` shim | — | -| `OpenAIBackend` | — | ✅ shipping, via the deprecated `EmbeddedIntrinsicAdapter` shim above, which builds an `EmbeddedBinding` internally | — | +| `LocalHFBackend` | ✅ shipping — `add_adapter` accepts a composed `Adapter` or a bare `LocalFileBinding` directly | ✅ shipping — `load_embedded_adapters=True`, or `add_adapter(adapter, config=...)`/`register_embedded_adapter_model` with a composed `Adapter` | — | +| `OpenAIBackend` | — | ✅ shipping — `load_embedded_adapters=True`, or `add_adapter(adapter, config=...)`/`register_embedded_adapter_model` with a composed `Adapter` | — | `ServerMediatedBinding` has no backend implementation yet — see discussion #1486. +Discovering *multiple* embedded adapters from a Granite Switch checkpoint or +Hub repo (rather than one already-known name) still goes through +`register_embedded_adapter_model`, which builds the composed `Adapter` +instances for you. --- @@ -338,3 +357,8 @@ Safety and factuality checks use a separate set of Guardian-specific adapter fun `guardian_check()`, `policy_guardrails()`, `factuality_detection()`, and `factuality_correction()`. These are documented in the [Safety Guardrails](../how-to/safety-guardrails) how-to guide. + +**See also:** +[Adding a custom adapter function in 20 lines](../tutorials/07-custom-adapter-function.md) | +[Handling a breaking adapter schema change](../tutorials/08-adapter-schema-migrations.md) | +[Adapter function metrics](../observability/metrics.md#adapter-function-metrics) diff --git a/docs/docs/advanced/lora-and-alora-adapters.md b/docs/docs/advanced/lora-and-alora-adapters.md index b6198cdb23..f52b69fefd 100644 --- a/docs/docs/advanced/lora-and-alora-adapters.md +++ b/docs/docs/advanced/lora-and-alora-adapters.md @@ -117,34 +117,41 @@ m alora upload ./checkpoints/my_adapter \ ## Use the adapter in Mellea -Load the trained adapter into a `LocalHFBackend` using `CustomIntrinsicAdapter`. - -> **Note:** `CustomIntrinsicAdapter` is deprecated in favor of the `Adapter` / -> `WeightsBinding` model, but is still the only working way to load a -> locally-trained custom adapter — `Adapter`'s weight-loading is not yet -> implemented. This example will move to `Adapter` once that lands; see #1144. +Load the trained adapter into a `LocalHFBackend` by composing an `Adapter` +directly — `LocalFileBinding` accepts an arbitrary `repo_id`, so a locally +trained, non-catalog adapter does not need a dedicated shim class. ```python from mellea.backends.huggingface import LocalHFBackend -from mellea.backends.adapters.adapter import CustomIntrinsicAdapter +from mellea.backends.adapters import Adapter, Identity, LocalFileBinding, get_io_contract +from mellea.backends.adapters.catalog import AdapterType from mellea.stdlib.context import ChatContext from mellea import MelleaSession from mellea.stdlib.requirements import ALoraRequirement backend = LocalHFBackend(model_id="ibm-granite/granite-3.2-8b-instruct") -adapter = CustomIntrinsicAdapter( - model_id="your-org/my-adapter", # HF repo ID or local checkpoint path - base_model_name="granite-3.2-8b-instruct", - intrinsic_name="custom-failure-check", +adapter = Adapter( + identity=Identity(name="custom-failure-check", adapter_type="alora"), + # get_io_contract falls back to a permissive dict contract for names + # outside the built-in catalog, so this works for a custom adapter too. + io_contract=get_io_contract("custom-failure-check"), + weights=LocalFileBinding( + name="custom-failure-check", + adapter_type=AdapterType.ALORA, + repo_id="your-org/my-adapter", # HF repo ID or local checkpoint path + revision="main", # a custom adapter has no catalog entry to resolve this from + ), ) -backend.add_adapter(adapter) +backend.add_adapter(adapter) # downloads and loads the weights m = MelleaSession(backend, ctx=ChatContext()) failure_check = ALoraRequirement( "The failure mode must not be 'no_failure'.", intrinsic_name="custom-failure-check", + # Required for a name outside the intrinsics catalog — see Intrinsic. + adapter_types=(AdapterType.ALORA,), ) result = m.instruct( "Write a triage summary based on this technician note: {{note}}", @@ -155,17 +162,13 @@ print(str(result)) # Output will vary — LLM responses depend on model and temperature. ``` -> **Note:** `CustomIntrinsicAdapter` emits an advisory `UserWarning` because -> custom capability names are not part of Mellea's built-in capability registry. -> The adapter is still registered for routing. - `ALoraRequirement` routes validation through the adapter with the matching -`intrinsic_name`. Create `CustomIntrinsicAdapter` before the requirement: its -compatibility shim registers the custom name, which lets `ALoraRequirement` -resolve it. The adapter runs at the `check_requirement` prompt position. Its -`io.yaml` must transform the output into the -`{"requirement_check": {"score": }}` response schema; label-only -adapter output is not compatible with `ALoraRequirement`. +`intrinsic_name`. Call `backend.add_adapter(adapter)` before the requirement +runs — it registers the custom name so `ALoraRequirement` can resolve it. The +adapter runs at the `check_requirement` prompt position. Its `io.yaml` must +transform the output into the `{"requirement_check": {"score": }}` +response schema; label-only adapter output is not compatible with +`ALoraRequirement`. ## How automatic routing works diff --git a/docs/docs/integrations/openai.md b/docs/docs/integrations/openai.md index 24fcdece1a..4be1163a95 100644 --- a/docs/docs/integrations/openai.md +++ b/docs/docs/integrations/openai.md @@ -348,7 +348,6 @@ The high-level adapter function wrappers (`rag.check_answerability`, For more control, load adapters manually with `load_embedded_adapters=False`: ```python -from mellea.backends.adapters.adapter import EmbeddedIntrinsicAdapter from mellea.backends.openai import OpenAIBackend from mellea.backends.model_ids import IBM_GRANITE_SWITCH_4_1_3B_PREVIEW from mellea.formatters import TemplateFormatter @@ -361,13 +360,13 @@ backend = OpenAIBackend( load_embedded_adapters=False, ) -# Load a single adapter from the model's Hugging Face repo -adapters = EmbeddedIntrinsicAdapter.from_hub( +# Discover and register a single adapter from the model's Hugging Face repo — +# this composes an Adapter (Identity + IOContract + EmbeddedBinding) rather +# than returning the deprecated EmbeddedIntrinsicAdapter shim. +backend.register_embedded_adapter_model( IBM_GRANITE_SWITCH_4_1_3B_PREVIEW.hf_model_name, intrinsic_name="answerability", ) -for adapter in adapters: - backend.add_adapter(adapter) ``` ## Troubleshooting diff --git a/docs/docs/observability/metrics.md b/docs/docs/observability/metrics.md index cff637b63e..adbd933c0b 100644 --- a/docs/docs/observability/metrics.md +++ b/docs/docs/observability/metrics.md @@ -297,6 +297,101 @@ All sampling metrics include: | `gen_ai.tool.name` | Name of the invoked tool | `"search"`, `"calculator"` | | `status` | Execution outcome | `success`, `failure` | +## Adapter function metrics + +Mellea records metrics for adapter function calls (`core.requirement_check()`, +`rag.check_answerability()`, `Intrinsic`/`ALoraRequirement`, and the +deprecated shim classes alike) — no code changes are required beyond enabling +metrics as above. They are recorded by `AdapterFunctionMetricsPlugin` +(`mellea/telemetry/metrics_plugins.py`), which subscribes to the +`adapter_function_invocation_complete` and `adapter_function_phase_complete` +hooks fired by `AdapterMixin.adapter_scope()` and +`EmbeddedBinding.apply_activation()`. + +### Adapter function instruments + +| Metric Name | Type | Attributes | Description | +| ----------- | ---- | ---------- | ----------- | +| `mellea.adapter_function.invocations` | Counter | `name`, `revision`, `binding_type`, `adapter_type`, `outcome` | Calls, split by outcome (`success`, `schema_error`, `error`) | +| `mellea.adapter_function.phase_duration` | Histogram | `name`, `phase` | Lifecycle phase duration, in seconds | +| `mellea.adapter_function.parse_failures` | Counter | `name`, `revision` | LocalFile/PEFT: calls that raised `AdapterSchemaMismatchError` *while still inside* `adapter_scope()` — see the known gap below. Embedded/Granite Switch: not subject to that gap — a malformed (non-JSON) response fires `outcome="schema_error"` directly, with no `adapter_scope()` involved | + +`revision` reports the *resolved* revision actually used, not the raw value +passed in: for a `LocalFileBinding` constructed with `revision=None` (the +default for a catalog adapter), `adapter_scope()` resolves it to the +catalog's pinned SHA before recording the metric, so a catalog adapter used +without an explicit revision still reports that SHA rather than `"unpinned"`. +The `"unpinned"` label only appears when no revision could be resolved at +all — for a `LocalFileBinding`, that means resolution itself failed (a +non-catalog adapter with no explicit `revision` — see +[Adding a custom adapter function](../tutorials/07-custom-adapter-function.md)). +It is not a general filter for "running without a pinned revision." The +Embedded/Granite Switch reality is the exception: weights are already baked +into the served model, so there is no revision to resolve at all, and every +Embedded invocation reports `revision="unpinned"` regardless of resolution +success. + +`phase` is one of `"prepare"`, `"activate"`, `"generate"`, `"parse"`, or +`"deactivate"` — though as of this writing only three are actually emitted: +`"prepare"` once per `LocalFileBinding` (fired from `add_adapter`/ +`binding.prepare()` — the download-and-load cost), then +`"activate"`/`"deactivate"` per call from `adapter_scope()` for the +LocalFile/PEFT reality, or `"activate"` alone per call for the Embedded +reality (`EmbeddedBinding.apply_activation()`, which has no lifecycle to +deactivate). `"generate"`/`"parse"` phase timing is tracked as future work in +issue #1466. + +> **Known gap (LocalFile/PEFT only):** every production caller +> (`core.requirement_check()`, `call_intrinsic()`, `ALoraRequirement`) parses +> the model's output *after* `adapter_scope()` has already exited and fired +> `outcome="success"` — so a real `AdapterSchemaMismatchError` from one of +> those call paths is correctly raised to your code, but is not yet reflected +> in `parse_failures` or in `invocations{outcome="schema_error"}`. Tracked in +> issue #1611. Until that's fixed, catch `AdapterSchemaMismatchError` in +> application code (see +> [Adapter schema migrations](../tutorials/08-adapter-schema-migrations.md)) +> rather than relying on these metrics to surface schema drift for that +> reality. +> +> This gap does not apply to the Embedded/Granite Switch reality: a malformed +> response there fires `outcome="schema_error"` (and increments +> `parse_failures`) directly from the response-processing code, with no +> `adapter_scope()` in that path to have already exited. + +### Adapter function dashboard queries + +**Success rate** — `mellea.adapter_function.invocations`, grouped by `name` +and `outcome`: + +```promql +sum by (name, outcome) (rate(mellea_adapter_function_invocations_total[5m])) +``` + +Watch `outcome="error"` and `outcome="schema_error"` separately — an error +means the call itself failed (network, generation); a schema error means the +call succeeded but its output didn't parse against the declared contract +(subject to the known gap above). + +**Phase latency** — `mellea.adapter_function.phase_duration`, grouped by +`name` and `phase`. A p95 activate-phase latency that jumps for one adapter +but not others usually means its weights just got evicted from whatever +cache your deployment relies on (HF Hub's local cache, or your own). + +**Parse-failure rate** — `mellea.adapter_function.parse_failures` divided by +`mellea.adapter_function.invocations` for the same `name`. In principle this +is the leading indicator for the schema-drift scenario in +[Adapter schema migrations](../tutorials/08-adapter-schema-migrations.md): a +sudden rise, correlated with a `revision` change in your own deploy history, +would be exactly the signal to re-pin or roll back — subject to the known gap +above. + +> **Adapter function tracing:** Mellea does not currently open OpenTelemetry +> **spans** for adapter function calls — only these metrics, driven by hooks, +> exist today. Span-level tracing is tracked separately in issue #1466. If +> your dashboard needs request-level tracing today, correlate the metrics +> above with your own application-level spans around the +> `mfuncs.act()`/`m.instruct()` call site instead. + ## Metrics export configuration Mellea supports multiple metrics exporters that can be used independently or diff --git a/docs/docs/tutorials/07-custom-adapter-function.md b/docs/docs/tutorials/07-custom-adapter-function.md new file mode 100644 index 0000000000..b96e4505ec --- /dev/null +++ b/docs/docs/tutorials/07-custom-adapter-function.md @@ -0,0 +1,154 @@ +--- +title: "Tutorial: Adding a Custom Adapter Function" +description: "Compose your own trained LoRA/aLoRA adapter into Mellea without a shim class, in about 20 lines." +# diataxis: tutorial +--- + +This tutorial walks through registering a custom, non-catalog adapter +function by composing an `Adapter` directly — the replacement for the +deprecated `CustomIntrinsicAdapter` shim (Epic #929, issue #1144). + +By the end you will have covered: + +- Composing an `Adapter` from an `Identity`, an output contract, and a + `LocalFileBinding` +- Why a custom adapter needs an explicit `revision` +- Registering it with a backend and invoking it through `Intrinsic` +- Validating output with `ALoraRequirement` against a custom adapter name + +**Prerequisites:** `pip install "mellea[hf]"`, a trained LoRA/aLoRA adapter +uploaded to Hugging Face Hub with +`m alora upload ... --intrinsic --io-yaml ./io.yaml` (see +[LoRA and aLoRA adapters](../advanced/lora-and-alora-adapters.md) for +training and uploading), and a GPU or Apple Silicon Mac. + +--- + +## Step 1: Compose the adapter + +Mellea's built-in adapter functions (`answerability`, `requirement-check`, +etc.) are looked up from a catalog that supplies their `repo_id` and +`revision`. A custom adapter has no catalog entry, so you supply those +fields yourself: + +> **Repo layout matters:** `LocalFileBinding` only finds weights at +> `///` inside `repo_id` — the same layout +> Mellea's own built-in adapter functions use, and the one `m alora upload +> --intrinsic --io-yaml ./io.yaml` produces. The plain `m alora upload` +> (no `--intrinsic`) pushes weights to the repo root instead, which +> `LocalFileBinding` cannot find — if you used that command, re-upload with +> `--intrinsic` before continuing. + +```python +# Requires: mellea[hf] +from mellea.backends.adapters import Adapter, Identity, LocalFileBinding, get_io_contract +from mellea.backends.adapters.catalog import AdapterType + +def custom_failure_check_adapter() -> Adapter: + return Adapter( + identity=Identity(name="custom-failure-check", adapter_type="alora"), + # get_io_contract falls back to a permissive dict contract for names + # outside the built-in catalog — sufficient here, since the real + # output shape is enforced by the adapter's own io.yaml, not by + # IOContract (which doesn't yet drive request/response handling). + io_contract=get_io_contract("custom-failure-check"), + weights=LocalFileBinding( + name="custom-failure-check", + adapter_type=AdapterType.ALORA, + repo_id="your-org/my-adapter", # your Hugging Face repo + # A LocalFileBinding with revision=None resolves it from the + # catalog — which has no entry for a custom name. Pin it + # explicitly (a branch, tag, or commit SHA). + revision="main", + ), + ) +``` + +> **Trap:** Omitting `revision` here raises `ValueError: Unknown intrinsic +> name '...'` from deep inside `LocalFileBinding.resolved_revision()` — easy +> to misread as a registration failure rather than what it actually is: no +> catalog entry to fall back to. + +## Step 2: Register it with a backend + +```python +from mellea.backends.huggingface import LocalHFBackend + +backend = LocalHFBackend(model_id="ibm-granite/granite-3.2-8b-instruct") +backend.add_adapter(custom_failure_check_adapter()) # downloads and loads the weights +``` + +`add_adapter` refuses a second registration under the same qualified name, so +guard repeated calls (e.g. inside a helper function) the same way the +built-in adapters do: + +```python +qualified_name = "custom-failure-check_alora" +if qualified_name not in backend.list_adapters(): + backend.add_adapter(custom_failure_check_adapter()) +``` + +## Step 3: Invoke it + +There are two ways to invoke a registered adapter, depending on whether you +need raw output or a pass/fail validation. + +### Directly, for raw output + +```python +from mellea.backends.adapters.catalog import AdapterType +from mellea.stdlib.components.intrinsic import Intrinsic +import mellea.stdlib.functional as mfuncs +from mellea.stdlib.components import Message +from mellea.stdlib.context import ChatContext + +# adapter_types is required for a name outside the catalog — Intrinsic has +# no catalog entry to look up adapter_types from otherwise. +action = Intrinsic("custom-failure-check", adapter_types=(AdapterType.ALORA,)) +ctx = ChatContext().add(Message("user", "Observed black soot on intake.")) +out, _ = mfuncs.act(action, ctx, backend) +print(out) +``` + +### As a validator, with `ALoraRequirement` + +If your adapter's `io.yaml` transforms its output into +`{"requirement_check": {"score": }}`, use it as a drop-in requirement +validator instead: + +```python +from mellea import MelleaSession +from mellea.stdlib.requirements import ALoraRequirement + +m = MelleaSession(backend, ctx=ChatContext()) + +failure_check = ALoraRequirement( + "The failure mode must not be 'no_failure'.", + intrinsic_name="custom-failure-check", + # Required for the same reason as Intrinsic's adapter_types above. + adapter_types=(AdapterType.ALORA,), +) +result = m.instruct( + "Summarize this technician note: {{note}}", + user_variables={"note": "High vibration at 3100 RPM."}, + requirements=[failure_check], +) +``` + +## What replaced the old approach + +The deprecated `CustomIntrinsicAdapter` shim did two things under the hood +that this tutorial's composed `Adapter` replaces without a class of its own: + +1. **Adapter registration** — `LocalFileBinding` already accepts an arbitrary + `repo_id`, so no shim was ever needed for this half. +2. **Making the name usable by `Intrinsic`/`ALoraRequirement`** — + `CustomIntrinsicAdapter` monkey-patched Mellea's global intrinsics catalog + to add the custom name before constructing anything. `Intrinsic` and + `ALoraRequirement` now accept `adapter_types` explicitly instead, so no + catalog mutation is needed — only the caller's own process is affected, + and there is no shared global state to leak between callers. + +**See also:** [Adapter functions](../advanced/intrinsics.md) | +[LoRA and aLoRA adapters](../advanced/lora-and-alora-adapters.md) | +[Adapter function metrics](../observability/metrics.md#adapter-function-metrics) diff --git a/docs/docs/tutorials/08-adapter-schema-migrations.md b/docs/docs/tutorials/08-adapter-schema-migrations.md new file mode 100644 index 0000000000..94aaf10ade --- /dev/null +++ b/docs/docs/tutorials/08-adapter-schema-migrations.md @@ -0,0 +1,157 @@ +--- +title: "Tutorial: Adapter Schema Migrations" +description: "Pin an adapter function's Hugging Face revision and handle AdapterSchemaMismatchError so a breaking schema change doesn't take your program down." +# diataxis: tutorial +--- + +An adapter function's output schema is defined by its `io.yaml`, hosted +alongside its weights on Hugging Face Hub. If the maintainer changes that +schema — adding, renaming, or removing a field the adapter's `IOContract` +requires — any program still pointed at `main` (or another mutable branch) +picks up the new schema on its next download, with no warning. This +tutorial covers the two mechanisms that keep that from breaking your +program: revision pinning, and handling the exception a mismatch raises. + +By the end you will have covered: + +- Why `LocalFileBinding`/`IntrinsicsCatalogEntry` pin `revision` to a commit + SHA rather than tracking `main` +- How to pin (or intentionally un-pin) a composed adapter's revision +- Catching `AdapterSchemaMismatchError` instead of letting it propagate +- Reading `ValidationResult.error` to distinguish "requirement not met" from + "output unparsable" when using `ALoraRequirement` + +**Prerequisites:** `pip install "mellea[hf]"`, a GPU or Apple Silicon Mac. + +--- + +## Step 1: Understand what `revision` actually pins + +`LocalFileBinding.revision` (and the built-in catalog's own +`IntrinsicsCatalogEntry.revision`) is a Hugging Face revision — a branch +name, tag, or commit SHA — passed straight through to the Hub download call. +Mellea's built-in catalog entries pin to commit SHAs by convention, precisely +so that a maintainer publishing a new `io.yaml` on `main` doesn't silently +change what your already-deployed program downloads: + +```python +# Requires: mellea[hf] +from mellea.backends.adapters.catalog import fetch_intrinsic_metadata + +metadata = fetch_intrinsic_metadata("requirement-check") +print(metadata.repo_id, metadata.revision) +# ibm-granite/granitelib-core-r1.0 d0a2a96a4cd07e96f0fe7ca29a42bfe088299d43 +``` + +`LocalFileBinding.resolved_revision()` uses this pinned value whenever you +construct a binding with `revision=None` (the default for +`LocalFileBinding.from_catalog(name)`) — you get the reproducibility for +free for a catalog adapter. A **custom** adapter has no catalog entry, so +`revision=None` there raises instead (see +[Adding a custom adapter function](./07-custom-adapter-function.md)) — +pinning is not optional for a custom adapter, only automatic for a catalog one. + +## Step 2: Pin explicitly when you need a specific version + +If a maintainer ships a breaking `io.yaml` change on a new branch or tag +before promoting it to the pinned revision Mellea's catalog tracks, or if +you're testing against an upcoming version ahead of a Mellea release that +updates the pin, construct the binding with an explicit `revision` instead +of `from_catalog`: + +```python +from mellea.backends.adapters import Adapter, Identity, LocalFileBinding, get_io_contract +from mellea.backends.adapters.catalog import AdapterType, fetch_intrinsic_metadata +from mellea.backends.huggingface import LocalHFBackend + +metadata = fetch_intrinsic_metadata("requirement-check") +backend = LocalHFBackend(model_id="ibm-granite/granite-3.2-8b-instruct") + +adapter = Adapter( + identity=Identity( + name="requirement-check", + adapter_type="alora", + capability=metadata.effective_capability, + ), + io_contract=get_io_contract("requirement-check"), + weights=LocalFileBinding( + name="requirement-check", + adapter_type=AdapterType.ALORA, + repo_id=metadata.repo_id, + # Explicit revision — the "v2" candidate you're validating against, + # not the catalog's currently-pinned "v1" SHA above. + revision="some-future-commit-sha-or-branch-name", + ), +) +backend.add_adapter(adapter) +``` + +Registering two revisions of the same adapter name on one backend at once +isn't supported — `add_adapter` keys the registry on +`f"{name}_{adapter_type}"`, so the second registration is refused. Use two +backend instances (or two processes) if you need to compare v1 and v2 output +side by side. + +## Step 3: Handle a schema mismatch gracefully + +Whether the mismatch comes from an unpinned adapter drifting underneath you, +or from deliberately testing a new revision, the failure mode is the same: +`IOContract.parse` raises `AdapterSchemaMismatchError` when the model's JSON +output doesn't satisfy the declared contract. + +`mfuncs.act(Intrinsic(...), ...)` on its own only returns the model's raw +output — it does not parse or validate it, so it never raises this error. +The high-level adapter function wrappers (`core.requirement_check`, +`core.check_certainty`, the `rag` module's functions) do both: they call +`mfuncs.act()` internally, then parse the result against the adapter's +contract. Catch the exception there: + +```python +from mellea.backends.adapters import AdapterSchemaMismatchError +from mellea.stdlib.components.intrinsic import core +from mellea.stdlib.components import Message +from mellea.stdlib.context import ChatContext + +context = ( + ChatContext() + .add(Message("user", "Can you help me plan a trip?")) + .add(Message("assistant", "Sure — where are you headed?")) +) + +try: + score = core.requirement_check(context, backend, "The assistant is helpful.") +except AdapterSchemaMismatchError as e: + print(f"Schema mismatch: expected {e.expected_keys}, observed {e.observed_keys}") + if e.reason: + print(f"Reason: {e.reason}") +``` + +`ALoraRequirement` handles this differently: it surfaces the error on +`ValidationResult.error` rather than raising, and fails the check closed +(`bool(result)` is `False`) so a schema mismatch is never silently treated +as "requirement not met": + +```python +from mellea import MelleaSession +from mellea.stdlib.requirements import ALoraRequirement + +m = MelleaSession(backend, ctx=context) +requirement = ALoraRequirement("The assistant is helpful.") + +val_res = m.validate([requirement]) +for result in val_res: + if result.error is not None: + print(f"Adapter output was unparsable: {result.error}") + elif not bool(result): + print("Requirement genuinely not met") +``` + +This distinction matters for monitoring: alert on `result.error is not None` +separately from your ordinary pass/fail rate — a spike in schema-mismatch +errors after an adapter update is a signal to re-pin or roll back, not +evidence your model got worse at the task. + +**See also:** +[Adapter functions](../advanced/intrinsics.md) | +[Adding a custom adapter function in 20 lines](./07-custom-adapter-function.md) | +[Adapter function metrics](../observability/metrics.md#adapter-function-metrics) diff --git a/docs/examples/aLora/101_example.py b/docs/examples/aLora/101_example.py index 822903cf20..1840f21f3b 100644 --- a/docs/examples/aLora/101_example.py +++ b/docs/examples/aLora/101_example.py @@ -1,10 +1,10 @@ # pytest: huggingface, e2e, slow """How `ALoraRequirement` routes validation through a fast adapter. -Uses the catalog-native `requirement-check` adapter (registered here via -`IntrinsicAdapter`, still the only working way to drive `_generate_from_intrinsic` — -see #1144) to compare aLoRA-backed validation against full LLM-as-judge generation -for the same requirement. `LLMaJRequirement` is used for the comparison because +Uses the catalog-native `requirement-check` adapter, registered here as a +composed `Adapter` (Identity + IOContract + LocalFileBinding), to compare +aLoRA-backed validation against full LLM-as-judge generation for the same +requirement. `LLMaJRequirement` is used for the comparison because `ALoraRequirement` always routes through the registered adapter regardless of `backend.default_to_constraint_checking_alora`. @@ -18,8 +18,13 @@ import time from mellea import MelleaSession, model_ids -from mellea.backends.adapters import AdapterType -from mellea.backends.adapters.adapter import IntrinsicAdapter +from mellea.backends.adapters import ( + Adapter, + Identity, + LocalFileBinding, + get_io_contract, +) +from mellea.backends.adapters.catalog import AdapterType, fetch_intrinsic_metadata from mellea.backends.huggingface import LocalHFBackend from mellea.core import GenerateLog, ValidationResult from mellea.stdlib.context import ChatContext @@ -40,11 +45,21 @@ # returning an ordinary failed check or propagating the exception. The ALORA # type is also load-bearing here: routing only looks up ("alora",), so # registering the LORA variant instead would hit the same failure. +_requirement_check_metadata = fetch_intrinsic_metadata("requirement-check") backend.add_adapter( - IntrinsicAdapter( # emits a DeprecationWarning — see module docstring, #1144 - "requirement-check", - adapter_type=AdapterType.ALORA, - base_model_name=backend.base_model_name, + Adapter( + identity=Identity( + name="requirement-check", + adapter_type="alora", + capability=_requirement_check_metadata.effective_capability, + ), + io_contract=get_io_contract("requirement-check"), + weights=LocalFileBinding( + name="requirement-check", + adapter_type=AdapterType.ALORA, + repo_id=_requirement_check_metadata.repo_id, + revision=_requirement_check_metadata.revision, + ), ) ) diff --git a/docs/examples/aLora/README.md b/docs/examples/aLora/README.md index 60588d6969..3bc46b9b99 100644 --- a/docs/examples/aLora/README.md +++ b/docs/examples/aLora/README.md @@ -88,21 +88,31 @@ The generator will display the README and ask for confirmation before uploading ## Using Adapter Functions -You can now create a new adapter class for this model somewhere in your python project: +Compose the adapter directly from an `Identity`, an output contract, and a +weights binding — no dedicated adapter class needed: ```python -from mellea.backends.adapters.adapter import CustomIntrinsicAdapter - -class StemboltAdapter(CustomIntrinsicAdapter): - def __init__(self, base_model_name:str="granite-4.1-3b"): - super().__init__( - model_id="$USERNAME/stembolts", # REPLACE $USERNAME WITH YOUR HUGGING FACE USERNAME - intrinsic_name="stembolts", - base_model_name=base_model_name, - ) +from mellea.backends.adapters import Adapter, Identity, LocalFileBinding, get_io_contract +from mellea.backends.adapters.catalog import AdapterType + +def stembolt_adapter(base_model_name: str = "granite-4.1-3b") -> Adapter: + return Adapter( + identity=Identity(name="stembolts", adapter_type="alora"), + # get_io_contract falls back to a permissive dict contract for names + # outside the built-in catalog. + io_contract=get_io_contract("stembolts"), + weights=LocalFileBinding( + name="stembolts", + adapter_type=AdapterType.ALORA, + repo_id="$USERNAME/stembolts", # REPLACE $USERNAME WITH YOUR HUGGING FACE USERNAME + # A custom, non-catalog adapter has no catalog entry to resolve + # revision=None from — pin it explicitly. + revision="main", + ), + ) ``` -Using this adapter requires adding it to a backend: +Registering it with a backend downloads and loads the weights: ```python from mellea.backends.cache import SimpleLRUCache @@ -112,7 +122,7 @@ backend = LocalHFBackend( model_id="ibm-granite/granite-4.1-3b", cache=SimpleLRUCache(5) ) -backend.add_adapter(StemboltAdapter(base_model_name="granite-4.1-3b")) +backend.add_adapter(stembolt_adapter(base_model_name="granite-4.1-3b")) ``` ## Example Files @@ -151,7 +161,7 @@ Helper module for loading and calling the fully custom `stembolts` adapter (no standalone entry point — `102_example.py` consumes it). Demonstrates: -- Custom adapter class definition (`StemboltAdapter`) +- Custom adapter composition (`_stembolt_adapter()`) and intrinsic definition (`StemboltIntrinsic`) - Registering the adapter on a backend, guarded by qualified name - Calling the intrinsic directly via `mfuncs.act` diff --git a/docs/examples/aLora/stembolts_intrinsic.py b/docs/examples/aLora/stembolts_intrinsic.py index da3a5d5f86..26f0bf86e4 100644 --- a/docs/examples/aLora/stembolts_intrinsic.py +++ b/docs/examples/aLora/stembolts_intrinsic.py @@ -12,37 +12,65 @@ import mellea.stdlib.functional as mfuncs from mellea.backends import Backend -from mellea.backends.adapters import AdapterMixin -from mellea.backends.adapters.adapter import CustomIntrinsicAdapter +from mellea.backends.adapters import ( + Adapter, + AdapterMixin, + Identity, + LocalFileBinding, + get_io_contract, +) +from mellea.backends.adapters.catalog import AdapterType from mellea.core import Context from mellea.stdlib.components import Message from mellea.stdlib.components.intrinsic import Intrinsic _INTRINSIC_MODEL_ID = "nfulton/stembolts" _INTRINSIC_ADAPTER_NAME = "stembolts" +_INTRINSIC_QUALIFIED_NAME = f"{_INTRINSIC_ADAPTER_NAME}_{AdapterType.ALORA.value}" -class StemboltAdapter(CustomIntrinsicAdapter): - def __init__(self, base_model_name: str): - super().__init__( - model_id=_INTRINSIC_MODEL_ID, - intrinsic_name=_INTRINSIC_ADAPTER_NAME, - base_model_name=base_model_name, - ) +def _stembolt_adapter() -> Adapter: + """Compose the stembolts adapter from its Identity, output contract, and + weights binding, rather than a dedicated shim class. + + `get_io_contract` falls back to a permissive dict contract for names + outside the built-in catalog — sufficient here, since the adapter's real + output shape (`{"defective_part": str, "diag_likelihood": float}`) is + enforced by its own `io.yaml` on the Hugging Face repo, not by `IOContract`. + """ + return Adapter( + identity=Identity(name=_INTRINSIC_ADAPTER_NAME, adapter_type="alora"), + io_contract=get_io_contract(_INTRINSIC_ADAPTER_NAME), + weights=LocalFileBinding( + name=_INTRINSIC_ADAPTER_NAME, + adapter_type=AdapterType.ALORA, + repo_id=_INTRINSIC_MODEL_ID, + # A LocalFileBinding resolves revision=None via the adapter + # function catalog, which has no entry for a custom, non-catalog + # name like this one — pin explicitly instead. + revision="main", + ), + ) class StemboltIntrinsic(Intrinsic): def __init__(self): - Intrinsic.__init__(self, intrinsic_name=_INTRINSIC_ADAPTER_NAME) + # adapter_types is required here: "stembolts" is outside Mellea's + # intrinsics catalog, so Intrinsic has no catalog entry to fall + # back to (Epic #929, issue #1144). + Intrinsic.__init__( + self, + intrinsic_name=_INTRINSIC_ADAPTER_NAME, + adapter_types=(AdapterType.ALORA,), + ) async def async_stembolt_failure_analysis( notes: str, ctx: Context, backend: Backend | AdapterMixin ): # add_adapter() refuses a duplicate qualified name with a warning, so guard first. - adapter = StemboltAdapter(backend.base_model_name) - if adapter.qualified_name not in backend.list_adapters(): - backend.add_adapter(adapter) + if _INTRINSIC_QUALIFIED_NAME not in backend.list_adapters(): + backend.add_adapter(_stembolt_adapter()) ctx = ctx.add(Message("user", content=notes)) @@ -55,9 +83,8 @@ def stembolt_failure_analysis( notes: str, ctx: Context, backend: Backend | AdapterMixin ): # add_adapter() refuses a duplicate qualified name with a warning, so guard first. - adapter = StemboltAdapter(backend.base_model_name) - if adapter.qualified_name not in backend.list_adapters(): - backend.add_adapter(adapter) + if _INTRINSIC_QUALIFIED_NAME not in backend.list_adapters(): + backend.add_adapter(_stembolt_adapter()) ctx = ctx.add(Message("user", content=notes)) diff --git a/docs/examples/granite-switch/README.md b/docs/examples/granite-switch/README.md index 1453fd1a9d..f35f46c5f2 100644 --- a/docs/examples/granite-switch/README.md +++ b/docs/examples/granite-switch/README.md @@ -58,7 +58,7 @@ Demonstrates `rag.flag_hallucinated_content()` using `OpenAIBackend` with ### manual_adapter_loading.py Shows how to manually load embedded adapters using -`EmbeddedIntrinsicAdapter.from_hub()` and `backend.add_adapter()`. Useful when +`backend.register_embedded_adapter_model(intrinsic_name=...)`. Useful when you only need a subset of adapters or want more control over adapter registration. diff --git a/docs/examples/granite-switch/manual_adapter_loading.py b/docs/examples/granite-switch/manual_adapter_loading.py index 8174c8b427..13c5a1bc0c 100644 --- a/docs/examples/granite-switch/manual_adapter_loading.py +++ b/docs/examples/granite-switch/manual_adapter_loading.py @@ -5,7 +5,9 @@ Instead of using `load_embedded_adapters=True` (which loads all adapters from the model repo at init), this example shows how to create an OpenAIBackend with `load_embedded_adapters=False` and then manually load individual adapters using -`EmbeddedIntrinsicAdapter.from_hub()` or `from_model_directory()`. +`register_embedded_adapter_model(intrinsic_name=...)`, which composes an +`Adapter` (Identity + IOContract + EmbeddedBinding) instead of the deprecated +`EmbeddedIntrinsicAdapter` shim. This is useful when: - You only need a subset of the model's embedded adapters. @@ -35,7 +37,6 @@ print(f"Skipped: vLLM server not reachable at {VLLM_BASE_URL}", file=sys.stderr) raise SystemExit(1) -from mellea.backends.adapters.adapter import EmbeddedIntrinsicAdapter from mellea.backends.model_ids import IBM_GRANITE_SWITCH_4_1_3B_PREVIEW from mellea.backends.openai import OpenAIBackend from mellea.formatters import TemplateFormatter @@ -56,19 +57,13 @@ ) # --- Option A: Load a single adapter from Hugging Face Hub --- -adapters = EmbeddedIntrinsicAdapter.from_hub( - SWITCH_MODEL_ID, intrinsic_name="answerability" -) -for adapter in adapters: - backend.add_adapter(adapter) +backend.register_embedded_adapter_model(SWITCH_MODEL_ID, intrinsic_name="answerability") # --- Option B (alternative): Load from a local model directory --- -# adapters = EmbeddedIntrinsicAdapter.from_model_directory( +# backend.register_embedded_adapter_model( # "/path/to/local/granite-switch-model", # intrinsic_name="answerability", # ) -# for adapter in adapters: -# backend.add_adapter(adapter) print(f"Registered adapters: {backend.list_adapters()}") diff --git a/docs/examples/intrinsics/intrinsics.py b/docs/examples/intrinsics/intrinsics.py index a2f6bc1b36..2a499a8014 100644 --- a/docs/examples/intrinsics/intrinsics.py +++ b/docs/examples/intrinsics/intrinsics.py @@ -2,7 +2,13 @@ import mellea.stdlib.functional as mfuncs from mellea.backends import model_ids -from mellea.backends.adapters.adapter import AdapterType, IntrinsicAdapter +from mellea.backends.adapters import ( + Adapter, + Identity, + LocalFileBinding, + get_io_contract, +) +from mellea.backends.adapters.catalog import AdapterType, fetch_intrinsic_metadata from mellea.backends.huggingface import LocalHFBackend from mellea.stdlib.components import Intrinsic, Message from mellea.stdlib.context import ChatContext @@ -23,9 +29,23 @@ # ) # --- End alternative --- -# Create the Adapter. IntrinsicAdapter's default to ALORAs. -req_adapter = IntrinsicAdapter( - "requirement-check", base_model_name=backend.base_model_name +# Compose the Adapter. requirement-check's catalog entry lists LoRA before +# aLoRA, so pin adapter_type explicitly rather than using +# LocalFileBinding.from_catalog (which would pick LoRA). +_requirement_check_metadata = fetch_intrinsic_metadata("requirement-check") +req_adapter = Adapter( + identity=Identity( + name="requirement-check", + adapter_type="alora", + capability=_requirement_check_metadata.effective_capability, + ), + io_contract=get_io_contract("requirement-check"), + weights=LocalFileBinding( + name="requirement-check", + adapter_type=AdapterType.ALORA, + repo_id=_requirement_check_metadata.repo_id, + revision=_requirement_check_metadata.revision, + ), ) # Add the adapter to the backend. @@ -36,7 +56,7 @@ ctx = ctx.add(Message("assistant", "Hello; yes! What can I help with?")) # Generate from an intrinsic with the same name as the adapter. By default, it will look for -# ALORA and then LORA adapters. +# LORA and then ALORA adapters. out, new_ctx = mfuncs.act( Intrinsic( "requirement-check", diff --git a/docs/sidebars.ts b/docs/sidebars.ts index fd02bfa1dc..3e71ba73aa 100644 --- a/docs/sidebars.ts +++ b/docs/sidebars.ts @@ -20,6 +20,8 @@ const sidebars: SidebarsConfig = { 'tutorials/making-agents-reliable', 'tutorials/mifying-legacy-code', 'tutorials/streaming-validation', + 'tutorials/custom-adapter-function', + 'tutorials/adapter-schema-migrations', ], }, { diff --git a/mellea/backends/adapters/_core.py b/mellea/backends/adapters/_core.py index 6131107eff..40e45ae3cf 100644 --- a/mellea/backends/adapters/_core.py +++ b/mellea/backends/adapters/_core.py @@ -33,10 +33,12 @@ """ import abc +import contextlib import json import threading import time import warnings +from collections.abc import Iterator from dataclasses import dataclass from typing import TYPE_CHECKING, Any, ClassVar, Literal @@ -486,6 +488,7 @@ def prepare(self) -> None: the registration. """ started_at = time.monotonic() + registered_here = False with self._lifecycle_lock: if self._released: raise RuntimeError( @@ -529,16 +532,102 @@ def prepare(self) -> None: "the backend's warning log — another adapter is already registered " "under this qualified name." ) - # `load_peft_adapter` mutates the backend's underlying PEFT model, the - # same shared state `activate_peft_adapter`/`deactivate_peft_adapter` - # document "must be called while holding `_generation_lock`" for. - # `prepare()`/`release()` aren't driven through `adapter_scope`, so - # nothing else takes this lock on their behalf. - with self.backend._adapter_activation_lock(): - self.backend.load_peft_adapter(self.qualified_name) + registered_here = True + try: + # `load_peft_adapter` mutates the backend's underlying PEFT model, the + # same shared state `activate_peft_adapter`/`deactivate_peft_adapter` + # document "must be called while holding `_generation_lock`" for. + # `prepare()`/`release()` aren't driven through `adapter_scope`, so + # nothing else takes this lock on their behalf. + with self.backend._adapter_activation_lock(): + self.backend.load_peft_adapter(self.qualified_name) + except BaseException: + # Keeps prepare() all-or-nothing for a call that did its own + # registration above: without this, a load failure leaves a + # registered binding with no way for the caller to distinguish + # it from a fully-prepared one, and the qualified name stays + # claimed — permanently refusing a *different* binding for + # the same capability (exactly what a fresh resolve_adapter() + # call constructs on retry). A call that instead retried an + # *already*-registered binding (registered_here False) isn't + # this call's registration to undo. + if registered_here: + self._rollback_registration() + raise self._loaded = True self._fire_phase_complete("prepare", time.monotonic() - started_at) + def _rollback_registration(self) -> None: + """Undoes this call's own `add_adapter` after the weights load failed. + + Called from `prepare()` with `_lifecycle_lock` already held, on the + failure path right after `self._staged_backend.add_adapter(self)` + succeeded but `load_peft_adapter` then raised. Takes only the + activation lock — re-entering `_lifecycle_lock` here would + self-deadlock, since it is a plain `threading.Lock`, not an `RLock` + (this is also why `release()`, which acquires `_lifecycle_lock` + itself, cannot be reused here). + """ + backend = self.backend + assert backend is not None + try: + with backend._adapter_activation_lock(): + # Unload before removing, mirroring release()'s own + # unload-then-remove sequence: `remove_adapter()` refuses a + # still-loaded name, and `load_peft_adapter()` now records the + # load before its own `set_adapter([])` call, so a + # `set_adapter([])` failure there leaves a real PEFT-level + # load for this to undo. Already a no-op (logs and returns) + # when nothing was actually loaded — the ordinary case, where + # `load_adapter()` itself is what failed. + backend.unload_peft_adapter(self.qualified_name) + backend.remove_adapter(self.qualified_name) + except NotImplementedError: + # State the observable fact, not the presumed cause: a real + # implementation can raise NotImplementedError internally for + # reasons unrelated to this rollback. + MelleaLogger.get_logger().warning( + f"{type(backend).__name__}.remove_adapter() raised " + f"NotImplementedError; {self.qualified_name!r} stays registered " + "after a failed load. Retrying prepare() on this same binding " + "still works; a different binding for the same capability " + "will be refused." + ) + return + self.backend = None + self.path = None + + @contextlib.contextmanager + def hold_prepared(self, backend: "AdapterMixin") -> Iterator[None]: + """Holds this binding's lifecycle lock while a caller publishes it. + + Used by a composed `Adapter`'s registration (`LocalHFBackend.add_adapter`) + to commit its registry entries only while this binding is still + genuinely prepared for `backend`. Blocks `prepare()`/`release()` for + the duration, so a composed `Adapter` can never be published wrapping + a binding that a concurrent `release()` — running in the gap after + `prepare()` returned but before the caller's commit — has already + made terminal. + + Args: + backend: The backend the caller is about to publish this binding + as registered with. Must match `self.backend`. + + Raises: + RuntimeError: This binding is no longer prepared for `backend` + (released, unloaded, or reassigned since the caller's own + `prepare()` call returned). + """ + with self._lifecycle_lock: + if self._released or not self._loaded or self.backend is not backend: + raise RuntimeError( + f"LocalFileBinding {self.qualified_name!r} is no longer " + f"prepared for {type(backend).__name__} (concurrent " + "release()?); refusing to publish a composed Adapter " + "around it." + ) + yield + def activate(self) -> None: """Selects already-loaded adapter weights for generation. @@ -974,13 +1063,17 @@ class Adapter: io_contract: IOContract weights: WeightsBinding | EmbeddedBinding - # NOTE(#1516): a construction-time cross-check that `weights.adapter_type` - # agrees with `identity.adapter_type` was tried here and backed out. It is the - # right invariant — the two feed different lookup paths (registration and the - # verbs key on the binding's `qualified_name`; `_find_adapter` scans on the - # identity) and both return `None` on a miss, so a disagreement surfaces as - # "adapter not found" far from its cause. But it cannot be enforced yet: - # the deprecated shims carry a `_ShimWeightsBinding` with no `adapter_type` - # to compare at all (their identity tracks the configured type). Enforce - # the check once those constructions carry real, typed bindings (the shims - # retire in #1144). + # NOTE(#1516): a construction-time cross-check that `weights` (`name` and + # `adapter_type`) agrees with `identity` was tried here and backed out. It + # is the right invariant — the two feed different lookup paths + # (registration and the verbs key on the binding's `qualified_name`; + # `_find_adapter` scans on the identity) and both return `None` on a miss, + # so a disagreement surfaces as "adapter not found" far from its cause. It + # could not be enforced here because the deprecated shims carry a + # `_ShimWeightsBinding` with no `name`/`adapter_type` to compare at all + # (their identity tracks the configured name/type) — that no longer + # blocks the LocalFile/PEFT reality now that the shims retire in #1144, so + # `LocalHFBackend.add_adapter` enforces it there instead, at registration + # time. `EmbeddedBinding` has no `name`/`adapter_type` of its own, so the + # Embedded/Granite Switch reality still has nothing to cross-check + # against. diff --git a/mellea/backends/adapters/adapter.py b/mellea/backends/adapters/adapter.py index 990f999970..935daf1467 100644 --- a/mellea/backends/adapters/adapter.py +++ b/mellea/backends/adapters/adapter.py @@ -24,7 +24,7 @@ import time import warnings from collections.abc import Callable -from typing import Literal, TypeAlias, TypeVar, cast +from typing import ClassVar, Literal, TypeAlias, TypeVar, cast import yaml @@ -298,6 +298,87 @@ def download_and_get_path(self, base_model_name: str) -> str: T = TypeVar("T") +def _composed_adapter_key(adapter: "_AdapterCore") -> str: + """Return the registry key for a composed `Adapter`, mirroring `qualified_name`. + + A composed `Adapter` (unlike the deprecated shims) has no `qualified_name` + of its own; backends key their registries on this instead. For a + LocalFile/PEFT composed adapter, this must produce the same string as + `adapter.weights.qualified_name` (`LocalFileBinding`'s own key) so a + backend's `_added_adapters` (keyed on the binding) and `_composed_adapters` + (keyed on this function) agree — which holds because `Identity.adapter_type` + is the `Literal["lora", "alora"]` string and `AdapterType.LORA.value`/ + `AdapterType.ALORA.value` are those same two strings. `LocalHFBackend.add_adapter` + enforces this at registration time for a `LocalFileBinding` (see the + `NOTE(#1516)` on the `Adapter` dataclass); a composed `Adapter` whose + `identity.adapter_type` disagrees with its own `weights.adapter_type` + otherwise produces two different keys instead of one. + + Args: + adapter: The composed adapter to key. + + Returns: + str: `"_"`. + """ + return f"{adapter.identity.name}_{adapter.identity.adapter_type}" + + +def _discover_embedded_adapters( + source: str, + *, + revision: str = "main", + cache_dir: str | None = None, + intrinsic_name: str | None = None, +) -> list[tuple["_AdapterCore", dict]]: + """Discover embedded adapter functions from a Granite Switch source. + + Non-shim equivalent of `EmbeddedIntrinsicAdapter.from_source()`: returns + composed `Adapter` instances instead of the deprecated shim (Epic #929, + issue #1144). Reuses the shim's discovery, `adapter_index.json`/`io.yaml` + parsing, and hub-snapshot materialization internally — including its + path-escape guard — rather than duplicating that logic, and lifts out the + already-correct `identity`/`io_contract`/`weights` triple each shim + instance built. + + The raw parsed `io.yaml` config is returned alongside each adapter because + a composed `Adapter` has no field for it (that's shim-only state) and it + cannot be cheaply re-derived later — callers that need it at generation + time (see `LocalHFBackend`/`OpenAIBackend`'s `_generate_from_intrinsic`) + must cache it themselves, keyed by `_composed_adapter_key`. + + Args: + source (str): Local path to a model directory, or a Hugging Face Hub + repo ID (e.g. `"ibm-granite/granite-switch-micro"`). + revision (str): Git revision (only used for Hub downloads). + cache_dir (str | None): Cache directory (only used for Hub downloads). + intrinsic_name (str | None): If provided, only load the adapter + matching this adapter function name. `None` loads all adapters. + + Returns: + list[tuple[_AdapterCore, dict]]: One `(adapter, io_yaml_config)` pair + per entry in the index. + """ + with warnings.catch_warnings(): + warnings.simplefilter("ignore", DeprecationWarning) + shims = EmbeddedIntrinsicAdapter.from_source( + source, + revision=revision, + cache_dir=cache_dir, + intrinsic_name=intrinsic_name, + ) + return [ + ( + _AdapterCore( + identity=shim.identity, + io_contract=shim.io_contract, + weights=shim.weights, + ), + shim.config, + ) + for shim in shims + ] + + def get_adapter_for_intrinsic( intrinsic_name: str, intrinsic_adapter_types: list[AdapterType] | tuple[AdapterType, ...], @@ -447,6 +528,21 @@ class AdapterMixin(Backend, abc.ABC): `"ibm-granite/granite-3.3-8b-instruct"`). """ + _supports_composed_adapters: ClassVar[bool] = False + """Whether this backend's `add_adapter` understands a composed `Adapter`. + + Opt-in, defaulting to `False`: `_uses_embedded_adapters` predates the + composed `Adapter` contract (Epic #929, issue #1144), so a third-party + subclass can already be reachable from `resolve_adapter()`'s embedded + branch while its `add_adapter` only knows the deprecated + `EmbeddedIntrinsicAdapter` shim's attribute shape. Signature inspection + cannot distinguish "modern" from "legacy" here — a `**kwargs` catch-all + present for unrelated reasons, or a pre-existing `config` parameter + meaning something else entirely, both read as "accepts config=". Only an + explicit declaration can carry that fact; `LocalHFBackend` and + `OpenAIBackend` both set this to `True`. + """ + # ---- Universal verbs (every adapter reality) ---- @property @@ -459,7 +555,7 @@ def base_model_name(self) -> str: """ @abc.abstractmethod - def add_adapter(self, adapter: AdapterInput) -> None: + def add_adapter(self, adapter: AdapterInput, *, config: dict | None = None) -> None: """Register an adapter with this backend so it can be loaded later. The adapter must not already have been added to a different backend. @@ -468,12 +564,25 @@ def add_adapter(self, adapter: AdapterInput) -> None: backend rejects an embedded adapter), so a statically valid call may still be rejected at runtime. + `config` is the raw io.yaml mapping for a composed `Adapter`/ + `_AdapterCore` whose `weights` is an `EmbeddedBinding` — that reality's + config cannot be cheaply re-derived later, so it must be supplied here + (see `_discover_embedded_adapters`/`resolve_adapter`), rather than + being fetched lazily the way a `LocalFileBinding`'s io.yaml is. + Args: adapter (AdapterInput): The adapter to register with this backend. + config (dict | None): Raw io.yaml config for a composed + `EmbeddedBinding` adapter. Ignored (and rejected) for every + other adapter reality. Raises: TypeError: If `adapter` belongs to a reality this backend does not - support. + support, or `config` is given for a reality other than a + composed `EmbeddedBinding` adapter. + ValueError: If `adapter.weights` is an `EmbeddedBinding` and + `config` is not given — registering it without a config would + make it discoverable but permanently unable to generate. """ @abc.abstractmethod @@ -588,27 +697,111 @@ def deactivate_peft_adapter(self, adapter_qualified_name: str) -> None: def _adapter_activation_lock( self, ) -> contextlib.AbstractContextManager[bool | None]: - """Exclusivity lock to hold while calling activate/deactivate and registration verbs. + """Exclusivity lock to hold while calling activate/deactivate and dict writes. Default is a no-op (`contextlib.nullcontext()`). Backends whose activation verbs mutate shared, non-thread-safe state (e.g. `LocalHFBackend`'s underlying PEFT model) override this to return their own lock, so callers like `LocalFileBinding.activate()` get the same exclusivity `_generate_with_adapter_lock` relies on. - `resolve_adapter()` also holds it for its own registration path. + `add_adapter()`'s registration-dict writes also hold it, briefly. + + Innermost in the adapter lock order + (`_adapter_resolve_lock` -> `binding._lifecycle_lock` -> + `_adapter_activation_lock`). Never hold this lock across a + `WeightsBinding` lifecycle verb (`prepare`/`activate`/`deactivate`/ + `release`) or across any I/O — those take `_lifecycle_lock`, and + holding activation across them inverts the order and can deadlock + against a concurrent lifecycle call on the same binding. A code path already holding this lock can re-enter it: on `LocalHFBackend`, `_generate_intrinsic_with_adapter_scope` holds `_generation_lock` for the whole generation, and the `_IntrinsicPeftBinding` verbs it drives through `adapter_scope()` - take `_adapter_activation_lock()` again on the same thread. (No - production caller currently reaches `resolve_adapter()` this way, but - nothing prevents one.) An override must therefore return a reentrant - lock (`threading.RLock`, as `LocalHFBackend` does) — a plain - `threading.Lock` here is a real deadlock hazard. + take `_adapter_activation_lock()` again on the same thread. An + override must therefore return a reentrant lock (`threading.RLock`, + as `LocalHFBackend` does) — a plain `threading.Lock` here is a real + deadlock hazard. """ return contextlib.nullcontext() + def _adapter_resolve_lock(self) -> contextlib.AbstractContextManager[bool | None]: + """Exclusivity lock for discovery-plus-registration orchestration. + + Default is a no-op (`contextlib.nullcontext()`). Held by + `resolve_adapter()` and `register_embedded_adapter_model()` across + their catalogue/Hugging Face Hub discovery I/O and their + `add_adapter()` calls. + + Outermost in the adapter lock order + (`_adapter_resolve_lock` -> `binding._lifecycle_lock` -> + `_adapter_activation_lock`). Deliberately a separate lock from + `_adapter_activation_lock`, not a reuse of it: the activation lock is + taken *inside* every `WeightsBinding` lifecycle verb, so holding it + across `add_adapter()` — which can call those verbs — would invert + the order and deadlock against a concurrent `prepare()`/`release()` + on the same binding. + + Must not be acquired while already holding + `_adapter_activation_lock()` — that reverses the order. No + production caller does: `resolve_adapter()` runs before the backend + takes its generation lock, not after (see + `mellea/stdlib/components/intrinsic/_util.py`). + + An override must return a reentrant lock (`threading.RLock`) for the + same same-thread-reentry reason `_adapter_activation_lock()` + documents. + + Known limitation: a `HookType.ADAPTER_FUNCTION_PHASE_COMPLETE` + subscriber must not synchronously call `resolve_adapter()`, + `register_embedded_adapter_model()`, or `add_adapter()` with a + composed `Adapter` on this same backend. `LocalFileBinding.prepare()` + fires that hook via `_run_async_in_thread`, which blocks the calling + thread on a dedicated event-loop thread's result — if this lock is + held at that point (as it is for the whole body of the three methods + above) and the hook's handler re-enters one of them on the same + backend, the event-loop thread blocks on this lock while the original + thread blocks waiting for the event-loop thread: deadlock. No shipped + caller does this today. + """ + return contextlib.nullcontext() + + def _add_embedded_adapter_compat( + self, adapter: "_AdapterCore", config: dict + ) -> None: + """Register a composed embedded Adapter, tolerating a pre-#1144 `add_adapter`. + + `AdapterMixin.add_adapter` gained a `config` keyword-only parameter in + Epic #929, issue #1144 (needed because a composed `Adapter` has no + field to carry a shim's `.config`). `_uses_embedded_adapters` predates + that: a third-party `AdapterMixin` subclass written before this + parameter existed, but already supporting the Embedded reality, is + reachable here and would raise + `TypeError: add_adapter() got an unexpected keyword argument 'config'` + — or worse, silently misbehave, if it happens to already accept a + `config=`/`**kwargs` for unrelated reasons and gets handed a composed + `Adapter` its body doesn't know how to read (it has no + `.qualified_name`/`.config`/`.technology`, unlike the shim). Gated on + `_supports_composed_adapters`, not signature inspection, for exactly + that reason: a parameter name can't tell you whether the method body + understands the new object shape. + + Args: + adapter: The composed `Adapter` to register. + config: Raw io.yaml config for `adapter`. + """ + if self._supports_composed_adapters: + self.add_adapter(adapter, config=config) + return + # Legacy subclass predating the composed-Adapter contract: it only + # knows the deprecated EmbeddedIntrinsicAdapter shim, which carries + # its own .config and needs no config= parameter. + self.add_adapter( + EmbeddedIntrinsicAdapter( + adapter.identity.name, config, technology=adapter.identity.adapter_type + ) + ) + def resolve_adapter(self, name: str) -> _AdapterCore: """Find or lazily register an adapter by capability name. @@ -627,14 +820,16 @@ def resolve_adapter(self, name: str) -> _AdapterCore: KeyError: If the adapter cannot be found after registration. Note: - On backends with a real `_adapter_activation_lock()` override - (e.g. `LocalHFBackend`), a cold first resolve holds that lock - across any Hugging Face Hub download it triggers, so it can stall - every other caller of the same lock (activation, generation, - other resolves) — including a resolve on the asyncio event-loop - thread inside a gathered set of coroutines. This is a deliberate - trade-off (reusing the existing lock rather than adding a second - one); a warm cache after the first resolve avoids the download. + A cold first resolve holds `_adapter_resolve_lock()` across any + Hugging Face Hub download it triggers, so it can stall every + other caller of *that* lock (other resolves, + `register_embedded_adapter_model()`) — including a resolve on + the asyncio event-loop thread inside a gathered set of + coroutines. It does **not** stall generation or activation: + `_adapter_resolve_lock` is a separate lock from + `_adapter_activation_lock`, deliberately (see the lock-order note + on `_adapter_activation_lock()`). A warm cache after the first + resolve avoids the download either way. """ found = self._find_adapter(name) if found is not None: @@ -649,14 +844,18 @@ def resolve_adapter(self, name: str) -> _AdapterCore: # add_adapter()'s own duplicate check is an unguarded read-then-write on # _added_adapters, and catch_warnings() below mutates thread-unsafe global # filter state — both race under concurrent first-time resolves for the - # same name. `_adapter_activation_lock()` closes both: a no-op by default, - # LocalHFBackend's reentrant `_generation_lock` and OpenAIBackend's own - # lock otherwise. Not closed here: LocalFileBinding.prepare() (own - # `_lifecycle_lock`) and register_embedded_adapter_model() (no lock, but - # constructor-only today, so not currently reachable concurrently). + # same name. `_adapter_resolve_lock()` closes both: a no-op by default, + # and each concrete backend's reentrant lock otherwise. Deliberately not + # `_adapter_activation_lock()`: that lock is taken *inside* + # `add_adapter()`'s composed-LocalFileBinding branch around the dict + # writes only, and inside every `WeightsBinding` lifecycle verb — holding + # it here, across the whole discover-and-register loop below (which calls + # `add_adapter()`, which can call `binding.prepare()`), would invert the + # adapter lock order and deadlock against a concurrent + # `prepare()`/`release()` on the same binding. # Suppress DeprecationWarning: the shim constructors warn user-facing code, # not internal registration paths. - with self._adapter_activation_lock(), warnings.catch_warnings(): + with self._adapter_resolve_lock(), warnings.catch_warnings(): # Re-check now the lock is held: a concurrent resolve may have already # registered this name. Without this, the loser redundantly re-fetches # and then hits the backend's own duplicate guard, which logs a @@ -672,23 +871,64 @@ def resolve_adapter(self, name: str) -> _AdapterCore: or getattr(self, "_model_id", None) or base ) - for a in EmbeddedIntrinsicAdapter.from_source( + # Composed Adapter, not the deprecated EmbeddedIntrinsicAdapter + # shim (Epic #929, issue #1144). Valid only for backends whose + # add_adapter supports the Embedded/Granite Switch reality + # (currently OpenAIBackend and LocalHFBackend when configured + # with load_embedded_adapters=True). + # + # Passing config= lets add_adapter() cache it atomically with + # registration, gated behind its own duplicate-key guard — a + # refused duplicate (a different object already holds the + # key) therefore never reaches the config write, so this + # can't clobber a live adapter's cached config the way a + # register-then-separately-cache sequence could. Routed + # through _add_embedded_adapter_compat, not called directly, + # for a third-party AdapterMixin subclass whose add_adapter + # predates config= (see that method's docstring). + for a, config in _discover_embedded_adapters( repo_id, intrinsic_name=name ): - # EmbeddedIntrinsicAdapter is valid only for backends whose - # add_adapter supports the Embedded/Granite Switch reality - # (currently OpenAIBackend and LocalHFBackend when configured - # with load_embedded_adapters=True). - self.add_adapter(a) + self._add_embedded_adapter_compat(a, config) else: # AdapterType.LORA is the pre-Phase-1 default (mirrors old _util.py). # Every current catalog entry supports LORA. Phase 2 (see epic #929) # will select the type from catalog availability instead of hardcoding. - self.add_adapter( - IntrinsicAdapter( - name, adapter_type=AdapterType.LORA, base_model_name=base + metadata = fetch_intrinsic_metadata(name) + if self._supports_composed_adapters: + # Composed Adapter, not the deprecated IntrinsicAdapter shim + # (Epic #929, issue #1144). + self.add_adapter( + _AdapterCore( + identity=Identity( + name=name, + adapter_type="lora", + capability=metadata.effective_capability, + ), + io_contract=get_io_contract(name), + weights=LocalFileBinding( + name=name, + adapter_type=AdapterType.LORA, + repo_id=metadata.repo_id, + revision=metadata.revision, + ), + ) + ) + else: + # Legacy subclass predating the composed-Adapter contract + # (same reasoning as _add_embedded_adapter_compat): its + # add_adapter only knows the deprecated IntrinsicAdapter + # shim's attribute shape, not a composed Adapter's. This + # is exactly what this branch passed before Epic #929, + # issue #1144 — IntrinsicAdapter is dual-shaped (both + # LocalHFAdapter, for `.qualified_name`/`.backend`/`.path`, + # and _AdapterCore, so `_find_adapter` below and this + # method's own `-> _AdapterCore` return type still hold). + self.add_adapter( + IntrinsicAdapter( + name, adapter_type=AdapterType.LORA, base_model_name=base + ) ) - ) found = self._find_adapter(name) if found is not None: @@ -930,6 +1170,16 @@ def _find_adapter( # or binding — can be popped: re-check that invariant when #1465 moves # generation inside `adapter_scope`. adapters = list(getattr(self, "_added_adapters", {}).values()) + # LocalFile/PEFT composed Adapters (Epic #929, issue #1144) don't live + # in _added_adapters — that dict holds their LocalFileBinding, keyed + # for the PEFT lifecycle (see LocalHFBackend.add_adapter); only + # _composed_adapters carries the _AdapterCore object this method + # matches on. Embedded composed adapters may live in either, per + # backend (LocalHFBackend uses _composed_adapters; OpenAIBackend + # stores them directly in _added_adapters), so checking both here + # keeps this method backend-agnostic without either backend needing + # to override it. + adapters += list(getattr(self, "_composed_adapters", {}).values()) if adapter_types is None: for a in adapters: if isinstance(a, _AdapterCore) and ( diff --git a/mellea/backends/huggingface.py b/mellea/backends/huggingface.py index 9f30cb963e..8fe5c6d708 100644 --- a/mellea/backends/huggingface.py +++ b/mellea/backends/huggingface.py @@ -25,6 +25,7 @@ import jinja2 import jinja2.meta +import yaml from packaging.requirements import Requirement as PackagingRequirement from packaging.version import InvalidVersion, Version @@ -73,6 +74,7 @@ ) from ..core.base import AbstractMelleaTool from ..formatters import ChatFormatter, TemplateFormatter, granite as granite_formatters +from ..formatters.granite import intrinsics as intrinsics from ..formatters.granite.base.util import ( _LLGUIDANCE_GRAMMAR_DEFAULTS, _GuidanceLogitsProcessor, @@ -103,7 +105,13 @@ _await_embedded_generation, _fire_embedded_invocation_complete, ) -from .adapters.adapter import AdapterInput, EmbeddedIntrinsicAdapter +from .adapters.adapter import ( + AdapterInput, + EmbeddedIntrinsicAdapter, + _composed_adapter_key, + _discover_embedded_adapters, +) +from .adapters.catalog import AdapterType from .backend import FormatterBackend from .cache import Cache, SimpleLRUCache from .model_ids import ModelIdentifier @@ -424,6 +432,7 @@ class LocalHFBackend(FormatterBackend, AdapterMixin): """ _cached_blocks: dict[str, DynamicCache] = dict() + _supports_composed_adapters = True def __init__( self, @@ -544,6 +553,19 @@ def __init__( str, LocalHFAdapter | LocalFileBinding | EmbeddedIntrinsicAdapter ] = {} self._loaded_adapters: dict[str, LocalHFAdapter | LocalFileBinding] = {} + # Composed Adapter instances (Epic #929, issue #1144), keyed by + # _composed_adapter_key(). LocalFile/PEFT composed adapters also + # register their LocalFileBinding into _added_adapters under the + # same key (see add_adapter) so the existing PEFT lifecycle keeps + # working unchanged; this dict is what makes _find_adapter() see the + # composed Adapter object itself (identity/io_contract), which the + # bare binding doesn't carry. + self._composed_adapters: dict[str, _AdapterCore] = {} + # Raw io.yaml config for composed Embedded adapters, keyed the same + # way. Embedded composed adapters have no `.config` field (that's + # shim-only state) and it can't be cheaply re-derived at generation + # time, unlike the LocalFile case (see _intrinsic_adapter_name_and_config). + self._composed_adapter_configs: dict[str, dict] = {} self._adapter_source = adapter_source self._uses_embedded_adapters = load_embedded_adapters @@ -559,6 +581,13 @@ def __init__( A plain `Lock` deadlocks on that same-thread re-acquisition (#1465). """ + self._adapter_resolve_lock_obj = threading.RLock() + """Backs `_adapter_resolve_lock()`. Deliberately a separate lock from + `_generation_lock`/`_adapter_activation_lock()` — see that method's + docstring for the deadlock this avoids. Reentrant for the same + same-thread-reentry reason `_generation_lock` is. + """ + if load_embedded_adapters: self.register_embedded_adapter_model(self._adapter_source or self._model_id) @@ -630,8 +659,18 @@ async def _generate_from_context( alora_action = ALoraRequirement(action.description, adapter_name) # Check if a requirement-check (or AloraRequirement specified) adapter - # exists. - alora_req_adapter = self._find_adapter(adapter_name, ("alora",)) + # exists. An explicit adapter_types override (Epic #929, issue + # #1144) is honoured here — e.g. a custom, LoRA-only adapter + # would never be found by the ("alora",)-only default search, + # silently falling back to regular generation regardless of + # what the caller asked for. + explicit_types = getattr(alora_action, "_adapter_types", None) + search_types = ( + tuple(t.value for t in explicit_types) + if explicit_types + else ("alora",) + ) + alora_req_adapter = self._find_adapter(adapter_name, search_types) if alora_req_adapter is None: # Log a warning if using an AloraRequirement but no adapter fit. if reroute_to_alora and isinstance(action, ALoraRequirement): @@ -792,6 +831,144 @@ def _generate_intrinsic_with_adapter_scope( _assert_correct_adapters(adapter.qualified_name, self._model) return out + def _generate_composed_local_file_with_adapter_scope( + self, + adapter: _AdapterCore, + generate_func: Callable[..., _T], + *args: Any, + **kwargs: Any, + ) -> _T: + """Runs `generate_func` with a composed `Adapter`'s `LocalFileBinding` active. + + Composed-Adapter counterpart of `_generate_intrinsic_with_adapter_scope` + (Epic #929, issue #1144). Unlike the deprecated `IntrinsicAdapter` shim, + `adapter.weights` here is already a real, registered `LocalFileBinding` + (see `add_adapter`) — there is no throwaway `_IntrinsicPeftBinding` to + build, so this drives `adapter_scope` directly on `adapter` itself. + + Does not call `binding.prepare()`: `add_adapter`/`resolve_adapter` + already guarantee it before this method's caller can reach a + registered adapter, and calling it again here would acquire + `binding._lifecycle_lock` while already holding `_generation_lock` — + the reverse of the order `LocalFileBinding.release()` acquires them + in (`_lifecycle_lock` then, via `_adapter_activation_lock()`, + `_generation_lock`), a lock-order inversion that can deadlock a + `release()` racing an in-flight generation on the same binding. + + Args: + adapter: The composed `Adapter` to activate for this call; its + `weights` must be a `LocalFileBinding` already registered with + this backend. + generate_func: The synchronous generation callable to invoke while + `adapter` is active. + *args: Positional arguments forwarded to `generate_func`. + **kwargs: Keyword arguments forwarded to `generate_func`. + + Returns: + Whatever `generate_func` returns. + + Raises: + TypeError: `adapter.weights` is not a `LocalFileBinding`. + """ + binding = adapter.weights + if not isinstance(binding, LocalFileBinding): + raise TypeError( + "LocalHFBackend's composed local-file generation path requires " + f"a LocalFileBinding; got {type(binding).__name__}." + ) + with self._generation_lock: + with self.adapter_scope(adapter): + _assert_correct_adapters(binding.qualified_name, self._model) + out = generate_func(*args, **kwargs) + _assert_correct_adapters(binding.qualified_name, self._model) + return out + + def _intrinsic_adapter_name_and_config( + self, adapter: IntrinsicAdapter | EmbeddedIntrinsicAdapter | _AdapterCore + ) -> tuple[str, dict]: + """Return the adapter-function name and raw io.yaml config for an adapter. + + The shims (`IntrinsicAdapter`/`EmbeddedIntrinsicAdapter`) carry both + directly (`.name`/`.config`). A composed `Adapter` carries neither — + `io_contract` does not yet drive `IntrinsicsRewriter`/ + `IntrinsicsResultProcessor` (Epic #929, issue #1144) — so this is a + pure cache read from `_composed_adapter_configs` for both realities. + The LocalFile/PEFT reality is cached by `add_adapter`'s composed + branch (via `_obtain_local_file_io_yaml_config`), alongside the + weights download `binding.prepare()` already does there; the + Embedded/Granite Switch reality is cached at registration time (see + `add_adapter`/`register_embedded_adapter_model`). Deliberately not + fetched here: this method runs inline inside the async + `_generate_from_intrinsic`, and `obtain_io_yaml`'s Hugging Face Hub + round trip must not block the event loop. + + Args: + adapter: The adapter to resolve a name and config for. + + Returns: + tuple[str, dict]: The adapter-function name and its parsed + io.yaml config. + + Raises: + ValueError: A composed LocalFile or Embedded adapter has no + cached config (never registered via + `add_adapter`/`register_embedded_adapter_model`). + TypeError: A composed Adapter's `weights` is neither a + `LocalFileBinding` nor an `EmbeddedBinding`. + """ + if isinstance(adapter, (IntrinsicAdapter, EmbeddedIntrinsicAdapter)): + return adapter.name, adapter.config + if isinstance(adapter.weights, (LocalFileBinding, EmbeddedBinding)): + key = _composed_adapter_key(adapter) + config = self._composed_adapter_configs.get(key) + if config is None: + raise ValueError( + f"No io.yaml config cached for composed adapter {key!r}; " + "register it via add_adapter(), " + "register_embedded_adapter_model(), or resolve_adapter()." + ) + return adapter.identity.name, config + raise TypeError( + "Unsupported weights binding for composed Adapter: " + f"{type(adapter.weights).__name__}" + ) + + def _obtain_local_file_io_yaml_config(self, binding: LocalFileBinding) -> dict: + """Download and parse `binding`'s io.yaml, mirroring `IntrinsicAdapter.__init__`. + + Called synchronously from `add_adapter`'s composed-`LocalFileBinding` + branch, before `binding.prepare()`'s (larger) weights download runs + there — so the Hugging Face Hub round trip this performs never runs + on the event loop inside `_generate_from_intrinsic`, and a failure + here leaves nothing registered (retryable), rather than leaving a + real, prepared `LocalFileBinding` with no matching composed-adapter + entry. + + Args: + binding: The `LocalFileBinding` to load `io.yaml` for. + + Returns: + dict: The parsed `io.yaml` mapping. + + Raises: + ValueError: `io.yaml` did not parse to a mapping. + """ + io_yaml_path = intrinsics.obtain_io_yaml( + binding.name, + self.base_model_name, + binding.repo_id, + revision=binding.resolved_revision(), + alora=binding.adapter_type is AdapterType.ALORA, + ) + with open(io_yaml_path, encoding="utf-8") as f: + loaded = yaml.safe_load(f) + if not isinstance(loaded, dict): + raise ValueError( + f"io.yaml for adapter {binding.name!r} at {io_yaml_path} " + f"did not parse to a mapping (got {type(loaded).__name__})." + ) + return loaded + async def _generate_from_intrinsic( self, action: Intrinsic, @@ -825,11 +1002,15 @@ async def _generate_from_intrinsic( Raises: ValueError: If no adapter is registered for the requested intrinsic, - or if the context contains images or audio; `LocalHFBackend` - does not support multimodal inputs. - TypeError: If the adapter is neither an `IntrinsicAdapter` nor an - `EmbeddedIntrinsicAdapter`, or if an embedded adapter does not - have an `EmbeddedBinding`. + the context contains images or audio (`LocalHFBackend` does + not support multimodal inputs), or a composed Embedded + adapter has no io.yaml config cached (see + `_intrinsic_adapter_name_and_config`). + TypeError: If the adapter is neither an `IntrinsicAdapter`, an + `EmbeddedIntrinsicAdapter`, nor a composed `Adapter`; if an + `EmbeddedIntrinsicAdapter`'s `weights` is not an + `EmbeddedBinding`; or if a composed `Adapter`'s `weights` is + an unsupported binding type. """ if not ctx.is_chat_context: raise Exception("Does not yet support non-chat contexts.") @@ -891,17 +1072,20 @@ async def _generate_from_intrinsic( # TODO: Code below this point is mostly specific to RagIntrinsics # It should be refactored into a specific adapter.transform() function. - if not isinstance(adapter, (IntrinsicAdapter, EmbeddedIntrinsicAdapter)): + if not isinstance( + adapter, (IntrinsicAdapter, EmbeddedIntrinsicAdapter, _AdapterCore) + ): raise TypeError( - "LocalHFBackend only supports IntrinsicAdapter or " - f"EmbeddedIntrinsicAdapter, got: {type(adapter).__name__}" + "LocalHFBackend only supports IntrinsicAdapter, EmbeddedIntrinsicAdapter, " + f"or a composed Adapter, got: {type(adapter).__name__}" ) - intrinsic_config = adapter.config - assert intrinsic_config is not None + adapter_name, intrinsic_config = self._intrinsic_adapter_name_and_config( + adapter + ) rewriter = granite_formatters.IntrinsicsRewriter( - config_dict=intrinsic_config, model_name=adapter.name + config_dict=intrinsic_config, model_name=adapter_name ) result_processor = granite_formatters.IntrinsicsResultProcessor( config_dict=intrinsic_config @@ -932,23 +1116,34 @@ async def _generate_from_intrinsic( # us having specific caching for each Component/Message. rewritten_request = rewritten.model_dump() - if isinstance(adapter, EmbeddedIntrinsicAdapter): - if not isinstance(adapter.weights, EmbeddedBinding): - raise TypeError( - "EmbeddedIntrinsicAdapter.weights must be an EmbeddedBinding; " - f"got {type(adapter.weights).__name__}. Activation cannot proceed." - ) + # A caller can reassign `.weights` on the mutable EmbeddedIntrinsicAdapter + # shim after construction; a composed Adapter's `.weights` is frozen and + # therefore always agrees with its own type, so this guard only ever + # fires for the shim. + if isinstance(adapter, EmbeddedIntrinsicAdapter) and not isinstance( + adapter.weights, EmbeddedBinding + ): + raise TypeError( + "EmbeddedIntrinsicAdapter.weights must be an EmbeddedBinding; " + f"got {type(adapter.weights).__name__}. Activation cannot proceed." + ) + adapter_weights = adapter.weights + if isinstance(adapter_weights, EmbeddedBinding): extra_body = rewritten_request.setdefault("extra_body", {}) if not isinstance(extra_body, dict): raise TypeError( "Embedded adapter generation requires extra_body to be a dict." ) - await adapter.weights.apply_activation( + await adapter_weights.apply_activation( EmbeddedActivationRequest( extra_body=extra_body, api_params=rewritten_request ), adapter.identity, ) + # Reused below (generation-path dispatch, embedded_identity for + # post-processing) — computed once here rather than re-checked, since + # unlike the `if` above this isn't gating a narrowing-sensitive block. + adapter_is_embedded = isinstance(adapter_weights, EmbeddedBinding) generate_input, other_input = ( granite_formatters.base.util.chat_completion_request_to_transformers_inputs( # type: ignore @@ -1012,18 +1207,7 @@ def __getattr__(self_proxy, name: str) -> Any: model_arg = _CapturingModelProxy() # type: ignore[assignment] - if isinstance(adapter, IntrinsicAdapter): - chat_response = asyncio.to_thread( - self._generate_intrinsic_with_adapter_scope, - adapter, - granite_formatters.base.util.generate_with_transformers, # type: ignore - # Passed as args/kwargs to generate. - self._tokenizer, - model_arg, - generate_input, - other_input, - ) - else: + if adapter_is_embedded: chat_response = _await_embedded_generation( asyncio.to_thread( self._generate_embedded_with_generation_lock, @@ -1036,6 +1220,28 @@ def __getattr__(self_proxy, name: str) -> Any: ), adapter.identity, ) + elif isinstance(adapter, IntrinsicAdapter): + chat_response = asyncio.to_thread( + self._generate_intrinsic_with_adapter_scope, + adapter, + granite_formatters.base.util.generate_with_transformers, # type: ignore + # Passed as args/kwargs to generate. + self._tokenizer, + model_arg, + generate_input, + other_input, + ) + else: + chat_response = asyncio.to_thread( + self._generate_composed_local_file_with_adapter_scope, + adapter, + granite_formatters.base.util.generate_with_transformers, # type: ignore + # Passed as args/kwargs to generate. + self._tokenizer, + model_arg, + generate_input, + other_input, + ) output = ModelOutputThunk(None) output._gen.start = datetime.datetime.now() @@ -1105,11 +1311,7 @@ async def granite_formatters_processing( rewritten=rewritten, result_processor=result_processor, input_ids=generate_input["input_tokens"], - embedded_identity=( - adapter.identity - if isinstance(adapter, EmbeddedIntrinsicAdapter) - else None - ), + embedded_identity=(adapter.identity if adapter_is_embedded else None), ) # TODO: Post-processing should release the lock for this generation. @@ -2406,7 +2608,7 @@ def base_model_name(self): """Returns the base_model_id of the model used by the backend. For example, `granite-3.3-8b-instruct` for `ibm-granite/granite-3.3-8b-instruct`.""" return pathlib.PurePath(self._model_id).name - def add_adapter(self, adapter: AdapterInput) -> None: + def add_adapter(self, adapter: AdapterInput, *, config: dict | None = None) -> None: """Register an adapter function with this backend. Downloads the adapter weights (via `adapter.get_local_hf_path`) and records @@ -2420,21 +2622,180 @@ def add_adapter(self, adapter: AdapterInput) -> None: Args: adapter (AdapterInput): The adapter to register. Must be a - `LocalHFAdapter`, `LocalFileBinding`, or - `EmbeddedIntrinsicAdapter`; other adapter realities are - rejected. + `LocalHFAdapter`, `LocalFileBinding`, `EmbeddedIntrinsicAdapter`, + or a composed `Adapter`; other adapter realities are rejected. + config (dict | None): Raw io.yaml config for a composed `Adapter` + whose `weights` is an `EmbeddedBinding`. Required for that + shape (its config cannot be cheaply re-derived later, unlike a + `LocalFileBinding`'s); rejected for every other shape. Raises: - TypeError: If `adapter` is not a supported local or embedded adapter. + TypeError: If `adapter` is not a supported local or embedded + adapter, or `config` is given for a shape other than a + composed `EmbeddedBinding` adapter. + ValueError: If `adapter` is a composed `Adapter` whose `weights` is + an `EmbeddedBinding` and `config` is not given — registering it + without one would make it discoverable but permanently unable + to generate. Also raised if a composed `Adapter`'s + `identity.adapter_type` disagrees with its own + `weights.adapter_type` — the two feed different lookup paths and + a mismatch surfaces as a spurious "adapter not found" far from + its cause. Exception: If `adapter` has already been added to a different backend. """ if not isinstance( - adapter, (LocalHFAdapter, LocalFileBinding, EmbeddedIntrinsicAdapter) + adapter, + (LocalHFAdapter, LocalFileBinding, EmbeddedIntrinsicAdapter, _AdapterCore), ): raise TypeError( - "LocalHFBackend requires a LocalHFAdapter, LocalFileBinding, or " - f"EmbeddedIntrinsicAdapter; got {type(adapter).__name__}." + "LocalHFBackend requires a LocalHFAdapter, LocalFileBinding, " + "EmbeddedIntrinsicAdapter, or a composed Adapter; got " + f"{type(adapter).__name__}." ) + + # A composed Adapter that isn't one of the shim subclasses (both of + # which are also _AdapterCore instances): dispatch on its weights + # binding instead of the shim/legacy branches below, which mutate + # attributes a composed Adapter's frozen dataclass doesn't have. + if isinstance(adapter, _AdapterCore) and not isinstance( + adapter, (IntrinsicAdapter, EmbeddedIntrinsicAdapter) + ): + # Validated ahead of the duplicate-registration guard below: a + # malformed config= argument must raise even when the call would + # otherwise be a silently-refused duplicate. + if isinstance(adapter.weights, EmbeddedBinding): + if config is None: + raise ValueError( + f"No io.yaml config given for composed embedded adapter " + f"{adapter.identity.name!r}; registering it without one " + "would leave it discoverable but unable to generate. " + "Pass config=, or register it via " + "register_embedded_adapter_model() or resolve_adapter()." + ) + elif config is not None: + raise TypeError( + "config= is only accepted for a composed Adapter whose " + f"weights is an EmbeddedBinding; got " + f"{type(adapter.weights).__name__}, which derives its " + "config lazily from io.yaml on first use." + ) + + key = _composed_adapter_key(adapter) + + # Held across the whole branch, including the unlocked-looking + # I/O and lifecycle-verb calls below: `_adapter_resolve_lock()` is + # outermost in the adapter lock order + # (resolve -> lifecycle -> activation), so a caller here may + # freely acquire `binding._lifecycle_lock` (via bind_backend()/ + # prepare()) and, inside that, the activation lock, without + # inverting anything. A concurrent caller for the same key — + # whether a direct add_adapter() call or resolve_adapter()'s own + # construction of a fresh composed Adapter — now simply blocks + # here until this registration finishes, rather than racing a + # partially-visible state (this replaces an earlier + # `_composed_registrations_in_flight` claim-set design, which + # made a concurrent resolve_adapter() see the claim as a + # permanent refusal and raise a spurious KeyError instead of + # waiting). Reentrant, so resolve_adapter()'s own hold of this + # same lock is a same-thread no-op, not double-blocking. + with self._adapter_resolve_lock(): + with self._adapter_activation_lock(): + # A LocalFileBinding registered standalone (bare + # add_adapter(binding)) lands in _added_adapters under + # this same key — that's not a collision if it's the very + # binding this composed Adapter wraps, only if some + # *other* object already claimed the name. + existing_added = self._added_adapters.get(key) + if ( + existing_added is not None + and existing_added is not adapter.weights + ) or key in self._composed_adapters: + MelleaLogger.get_logger().warning( + f"Client code attempted to add {adapter.identity.name} " + f"with type {adapter.identity.adapter_type} but {key!r} " + f"is already registered on {self.__class__.__name__}. " + "The backend is refusing to do this, because adapter " + "loading is not idempotent." + ) + return + if isinstance(adapter.weights, EmbeddedBinding): + assert config is not None # validated above + adapter.weights.source = self.base_model_name + self._composed_adapters[key] = adapter + self._composed_adapter_configs[key] = config + return # atomic: no lifecycle verb, no I/O + if not isinstance(adapter.weights, LocalFileBinding): + raise TypeError( + "LocalHFBackend does not support the " + f"{type(adapter.weights).__name__} weights reality " + "for a composed Adapter." + ) + binding = adapter.weights + if ( + adapter.identity.name != binding.name + or adapter.identity.adapter_type != binding.adapter_type.value + ): + raise ValueError( + f"Composed Adapter has identity.name={adapter.identity.name!r}/" + f"adapter_type={adapter.identity.adapter_type!r} but " + f"weights.name={binding.name!r}/" + f"adapter_type={binding.adapter_type.value!r}; the two " + "must agree on both, or registration and lookup key on " + "different strings for the same adapter (NOTE(#1516))." + ) + + # Slow work, still under the resolve lock but not the + # activation lock: released above, so bind_backend()/ + # prepare()'s own lifecycle-then-activation acquisitions + # below don't nest inside it. + # + # Fetched before bind_backend()/prepare(), not after: a + # failed fetch here leaves nothing registered, so the same + # composed Adapter can simply be retried. Fetching it after + # prepare() (which this used to do) left a real, registered + # LocalFileBinding with no matching _composed_adapters entry + # on failure — a later resolve_adapter()/add_adapter() for + # the same name then hit the duplicate-key guard above and + # permanently refused to (re)register the capability. + # `prepare()` itself closes the equivalent gap for its own + # weights-load failure (see its docstring's rollback note) — + # this ordering only has to cover the fetch this method adds + # ahead of it. + io_yaml_config = self._obtain_local_file_io_yaml_config(binding) + # bind_backend() itself raises if binding.backend is already + # a *different* backend, and no-ops if it's already self — + # calling it unconditionally (rather than guarding on + # `binding.backend is None`) is what makes that raise + # reachable; the guard used to skip straight to prepare() (a + # silent no-op once loaded), registering this adapter on + # `self` while the binding's weights stayed activated against + # whatever backend it was really bound to. + binding.bind_backend(self) + # Recurses into self.add_adapter(binding) below (the + # bare-LocalFileBinding branch further down this method) to + # populate `_added_adapters` for this binding — see + # `LocalFileBinding.prepare()`'s own docstring/comments for + # why that call is there. On a weights-load failure inside + # `prepare()`, that same docstring's rollback note applies: + # the binding un-registers itself, leaving nothing here to + # clean up beyond letting the exception propagate (this + # resolve-lock hold releases normally on the way out). + binding.prepare() + + # Commit under the binding's own lifecycle lock, then the + # activation lock (`hold_prepared` takes lifecycle; + # resolve -> lifecycle -> activation is still the order) — + # not just the activation lock alone: prepare() above + # released `_lifecycle_lock` on return, and without + # re-acquiring it here, a concurrent `binding.release()` + # could run in the gap between prepare() returning and this + # commit, releasing (terminal, weights unloaded) the exact + # binding this is about to publish as registered. + with binding.hold_prepared(self), self._adapter_activation_lock(): + self._composed_adapter_configs[key] = io_yaml_config + self._composed_adapters[key] = adapter + return + if adapter.backend is not None: if adapter.backend is self: MelleaLogger.get_logger().warning( @@ -2446,7 +2807,9 @@ def add_adapter(self, adapter: AdapterInput) -> None: f"adapter {adapter.name} with type {adapter.adapter_type} has already been added to backend {adapter.backend}" ) - existing = self._added_adapters.get(adapter.qualified_name) + existing = self._added_adapters.get( + adapter.qualified_name + ) or self._composed_adapters.get(adapter.qualified_name) if existing is not None: MelleaLogger.get_logger().warning( f"Client code attempted to add {adapter.name} with type {adapter.adapter_type} " @@ -2474,7 +2837,12 @@ def add_adapter(self, adapter: AdapterInput) -> None: self._added_adapters[adapter.qualified_name] = adapter def register_embedded_adapter_model( - self, source: str, *, revision: str = "main", cache_dir: str | None = None + self, + source: str, + *, + revision: str = "main", + cache_dir: str | None = None, + intrinsic_name: str | None = None, ) -> list[str]: """Register embedded adapter functions from a Granite Switch checkpoint. @@ -2482,6 +2850,9 @@ def register_embedded_adapter_model( source: Local checkpoint directory or Hugging Face Hub repository ID. revision: Git revision when loading from the Hub. cache_dir: Cache directory for Hub downloads. + intrinsic_name: If provided, register only the adapter matching + this adapter function name. `None` registers all adapters + found in `source`. Returns: Names of the registered adapter functions. @@ -2493,15 +2864,49 @@ def register_embedded_adapter_model( ValueError: If the source has no matching embedded adapter functions. TypeError: If an adapter from the source has an unsupported binding. """ - # No lock here: both call sites (here, and this class's __init__) run - # single-threaded during construction, before the backend is exposed to - # any other thread — see the matching note on OpenAIBackend's twin. - adapters = EmbeddedIntrinsicAdapter.from_source( - source, revision=revision, cache_dir=cache_dir - ) - for adapter in adapters: - self.add_adapter(adapter) - return [adapter.intrinsic_name for adapter in adapters] + # Locked (unlike __init__'s call to this, which still runs + # single-threaded during construction, before the backend is exposed + # to any other thread): this method is now also the documented + # post-construction replacement for `EmbeddedIntrinsicAdapter.from_hub()` + # on a live backend, so a caller here can race another thread's + # `add_adapter`/`resolve_adapter` — both `add_adapter`'s + # read-then-write across `_composed_adapters` and + # `_discover_embedded_adapters`'s mutation of global `warnings` filter + # state need the same lock `resolve_adapter` already takes around its + # own `add_adapter` calls. `_adapter_resolve_lock()`, not + # `_adapter_activation_lock()`: see the lock-order note on the latter + # — this method's own I/O (this Hub discovery call) must not run + # under the lock `add_adapter()`'s composed-LocalFileBinding branch + # briefly takes around its dict writes. + with self._adapter_resolve_lock(): + discovered = _discover_embedded_adapters( + source, + revision=revision, + cache_dir=cache_dir, + intrinsic_name=intrinsic_name, + ) + names = [] + for adapter, config in discovered: + key = _composed_adapter_key(adapter) + # add_adapter() caches config atomically with registration now, so + # a refused duplicate (a different object already holding `key`) + # never reaches that write — no separate clobber guard needed for + # the config. The identity check below is still required, though: + # add_adapter() returns None on both success and silent refusal, + # so this is the only way to know whether *this* adapter is the + # one that actually ended up registered, for the `names` result. + self.add_adapter(adapter, config=config) + # Under the activation lock, not just the resolve lock this + # whole method holds: a concurrent release() pops + # _composed_adapters without taking the resolve lock (it isn't + # a resolve-orchestration caller), so this read needs its own + # protection against racing that pop. + with self._adapter_activation_lock(): + registered = self._composed_adapters.get(key) is adapter + if not registered: + continue + names.append(adapter.identity.name) + return names def load_peft_adapter(self, adapter_qualified_name: str) -> None: """Load a previously registered adapter into the underlying Hugging Face model. @@ -2521,6 +2926,15 @@ def load_peft_adapter(self, adapter_qualified_name: str) -> None: """ adapter = self._added_adapters.get(adapter_qualified_name, None) if adapter is None: + # A composed Embedded adapter never lands in `_added_adapters` (see + # `add_adapter`'s composed-Adapter branch) — check there before + # concluding the name was never added at all. + composed = self._composed_adapters.get(adapter_qualified_name, None) + if composed is not None and isinstance(composed.weights, EmbeddedBinding): + raise TypeError( + f"cannot load embedded adapter {adapter_qualified_name} through PEFT; " + "it is activated by the chat template" + ) raise ValueError( f"could not load adapter {adapter_qualified_name} for backend {self}: adapter was not previously added" ) @@ -2546,12 +2960,21 @@ def load_peft_adapter(self, adapter_qualified_name: str) -> None: ): raise e + # Recorded before set_adapter([]) below, not after: from this point on + # PEFT holds this adapter's weights loaded, so remove_adapter()'s + # still-loaded guard must be able to see that — otherwise a + # set_adapter([]) failure here leaves PEFT holding weights that + # _loaded_adapters never records, bypassing that guard and letting a + # later same-name registration silently serve these stale weights + # under the new registration's identity (PEFT's own "already exists" + # error at the `self._model.load_adapter` call above is swallowed). + self._loaded_adapters[adapter.qualified_name] = adapter + # Loading an adapter activates it. We disable adapters immediately after. # Prefer this over `.disable_adapters()`; the disable function doesn't always # seem to work. self._model.set_adapter([]) # self._model.disable_adapters() - self._loaded_adapters[adapter.qualified_name] = adapter def unload_peft_adapter(self, adapter_qualified_name: str) -> None: """Unload a previously loaded adapter from the underlying Hugging Face model. @@ -2566,9 +2989,22 @@ def unload_peft_adapter(self, adapter_qualified_name: str) -> None: # Check if the backend knows about this adapter. adapter = self._loaded_adapters.get(adapter_qualified_name, None) if adapter is None: - MelleaLogger.get_logger().info( - f"could not unload adapter {adapter_qualified_name} for backend {self}: adapter is not loaded" - ) + # `_loaded_adapters` is only written after `self._model.load_adapter()` + # returns (see `load_peft_adapter`), but PEFT injects the adapter's + # modules into the model *before* it finishes loading weights — + # `inject_adapter_in_model()` runs, then the checkpoint state dict is + # applied. A failure in the latter step (e.g. a corrupt/incomplete + # checkpoint) leaves PEFT holding a real, injected-but-unloaded + # adapter under this name, with nothing in our own bookkeeping to + # say so. Best-effort direct cleanup covers that gap: harmless if + # PEFT never registered the name either, in which case its own + # `delete_adapter` raises and this logs instead of propagating. + try: + self._model.delete_adapter(adapter_qualified_name) + except ValueError: + MelleaLogger.get_logger().info( + f"could not unload adapter {adapter_qualified_name} for backend {self}: adapter is not loaded" + ) return self._model.delete_adapter(adapter.qualified_name) @@ -2621,6 +3057,14 @@ def remove_adapter(self, adapter_qualified_name: str) -> None: adapter = self._added_adapters.pop(adapter_qualified_name, None) if adapter is None: + # A composed Embedded adapter (see add_adapter) lives only in + # _composed_adapters — it never has a bare-binding entry in + # _added_adapters to mutate .backend/.path on — so it must be + # checked here rather than falling through to the "not + # registered" log below. + if self._composed_adapters.pop(adapter_qualified_name, None) is not None: + self._composed_adapter_configs.pop(adapter_qualified_name, None) + return MelleaLogger.get_logger().info( f"could not remove adapter {adapter_qualified_name} for backend {self}: " "adapter was not registered" @@ -2629,6 +3073,11 @@ def remove_adapter(self, adapter_qualified_name: str) -> None: adapter.backend = None adapter.path = None + # A composed LocalFile Adapter registered here (see add_adapter) + # shares this qualified name; drop its entry too so a stale one + # doesn't outlive the binding it wraps. + self._composed_adapters.pop(adapter_qualified_name, None) + self._composed_adapter_configs.pop(adapter_qualified_name, None) def activate_peft_adapter(self, adapter_qualified_name: str) -> None: """Switch a previously loaded PEFT adapter on for subsequent generation. @@ -2693,8 +3142,11 @@ def _adapter_activation_lock( the lock caller 1 (and, on the outer level, caller 3's driver) takes, it is the only thing satisfying the precondition on those paths. - A fourth consumer, unrelated to that precondition: `resolve_adapter()` - (base class) also takes this lock, to serialize registration. + A fourth consumer, unrelated to that precondition: `add_adapter()`'s + composed-`Adapter` branches also take this lock, briefly, around + their registration-dict writes — never across a `WeightsBinding` + lifecycle verb or I/O (see `_adapter_resolve_lock()` below and the + lock-order note on the base class's `_adapter_activation_lock()`). Caller 3's verb acquisitions therefore nest inside its driver's `_generation_lock` hold on the same thread (see @@ -2705,15 +3157,36 @@ def _adapter_activation_lock( """ return self._generation_lock + def _adapter_resolve_lock(self) -> contextlib.AbstractContextManager[bool | None]: + """A separate, reentrant lock for `resolve_adapter()`/registration orchestration. + + Not `_generation_lock` (the lock `_adapter_activation_lock()` + returns): that lock is taken *inside* `add_adapter()`'s + composed-`LocalFileBinding` branch and inside every `LocalFileBinding` + lifecycle verb, so holding it across `resolve_adapter()`'s whole + discover-and-register loop — which calls `add_adapter()`, which can + call `binding.prepare()` — would invert the adapter lock order + (`_adapter_resolve_lock` -> `binding._lifecycle_lock` -> + `_adapter_activation_lock`) and deadlock against a concurrent + `prepare()`/`release()` on the same binding. + """ + return self._adapter_resolve_lock_obj + def list_adapters(self) -> list[str]: """List the qualified names of all adapters registered with this backend. Returns: - list[str]: Qualified adapter names (i.e. `adapter.qualified_name`) for - all adapters that have been registered via `add_adapter`, whether - or not they have also been loaded via `load_peft_adapter`. + list[str]: Qualified adapter names for all adapters that have been + registered via `add_adapter`, whether or not they have also + been loaded via `load_peft_adapter`. For a shim or a bare + `LocalFileBinding`, this is `adapter.qualified_name`; for a + composed `Adapter`, it's the equivalent + `_composed_adapter_key()` format (the two formats agree). """ - return list(self._added_adapters.keys()) + # A composed Embedded adapter lives only in _composed_adapters (see + # add_adapter); a composed LocalFile adapter's binding is also in + # _added_adapters under the same key, so dict.fromkeys dedupes it. + return list(dict.fromkeys([*self._added_adapters, *self._composed_adapters])) def _assert_correct_adapters(expected_state: str, model: PreTrainedModel): diff --git a/mellea/backends/openai.py b/mellea/backends/openai.py index be73f513a5..1f684326d4 100644 --- a/mellea/backends/openai.py +++ b/mellea/backends/openai.py @@ -57,10 +57,17 @@ from ._options import resolve_model_options from .adapters import EmbeddedActivationRequest, EmbeddedBinding, Identity from .adapters._core import ( + Adapter as _AdapterCore, _await_embedded_generation, _fire_embedded_invocation_complete, ) -from .adapters.adapter import AdapterInput, AdapterMixin, EmbeddedIntrinsicAdapter +from .adapters.adapter import ( + AdapterInput, + AdapterMixin, + EmbeddedIntrinsicAdapter, + _composed_adapter_key, + _discover_embedded_adapters, +) from .backend import FormatterBackend from .model_options import ModelOption from .tools import ( @@ -123,6 +130,8 @@ class OpenAIBackend(FormatterBackend, AdapterMixin): `OPENAI_API_KEY` is set, or if `model_id` is a `ModelIdentifier` with no `openai_name` set. """ + _supports_composed_adapters = True + def __init__( self, model_id: str | ModelIdentifier = model_ids.OPENAI_GPT_5_1, @@ -292,8 +301,19 @@ def __init__( self._client_cache = ClientCache(2) - self._added_adapters: dict[str, EmbeddedIntrinsicAdapter] = {} + # EmbeddedIntrinsicAdapter is itself an _AdapterCore subclass, so this + # single type covers both the shim and composed-Adapter realities. + self._added_adapters: dict[str, _AdapterCore] = {} + # Raw io.yaml config for composed Adapter instances (Epic #929, issue + # #1144), keyed by _composed_adapter_key(). A composed Adapter has no + # `.config` field (that's shim-only state); see + # _intrinsic_adapter_name_and_config. + self._composed_adapter_configs: dict[str, dict] = {} self._adapter_lock = threading.RLock() + # Backs _adapter_resolve_lock() — deliberately separate from + # _adapter_lock (`_adapter_activation_lock()`); see that method's + # docstring for the deadlock a shared lock would risk. + self._adapter_resolve_lock_obj = threading.RLock() # Call once to create an async_client and populate the cache. _ = self._async_client @@ -324,7 +344,7 @@ def __str__(self) -> str: # AdapterMixin implementation # ------------------------------------------------------------------ - def add_adapter(self, adapter: AdapterInput) -> None: + def add_adapter(self, adapter: AdapterInput, *, config: dict | None = None) -> None: """Register an adapter with this backend. Accepts the full `AdapterInput` union to honour the mixin contract, but @@ -336,26 +356,80 @@ def add_adapter(self, adapter: AdapterInput) -> None: Args: adapter (AdapterInput): The adapter to register. Must be an - `EmbeddedIntrinsicAdapter`. + `EmbeddedIntrinsicAdapter` or a composed `Adapter` whose + `weights` is an `EmbeddedBinding`. + config (dict | None): Raw io.yaml config for a composed `Adapter`. + Required for that shape (its config cannot be cheaply + re-derived later); rejected for `EmbeddedIntrinsicAdapter`, + which already carries its own `.config`. Raises: - TypeError: If `adapter` is not an `EmbeddedIntrinsicAdapter`. + TypeError: If `adapter` is not a supported Embedded adapter, or + `config` is given for an `EmbeddedIntrinsicAdapter`. + ValueError: If `adapter` is a composed `Adapter` and `config` is + not given — registering it without one would make it + discoverable but permanently unable to generate. """ - if not isinstance(adapter, EmbeddedIntrinsicAdapter): - raise TypeError( - f"OpenAIBackend currently only supports EmbeddedIntrinsicAdapter. " - f"Got: {type(adapter).__name__}" - ) - if adapter.qualified_name in self._added_adapters: - MelleaLogger.get_logger().warning( - f"attempted to add adapter {adapter.qualified_name!r} but it is " - "already registered; refusing to overwrite it." - ) + if isinstance(adapter, EmbeddedIntrinsicAdapter): + if config is not None: + raise TypeError( + "config= is not accepted for an EmbeddedIntrinsicAdapter; " + "it already carries its own .config." + ) + if adapter.qualified_name in self._added_adapters: + MelleaLogger.get_logger().warning( + f"attempted to add adapter {adapter.qualified_name!r} but it is " + "already registered; refusing to overwrite it." + ) + return + adapter.backend = self + if isinstance(adapter.weights, EmbeddedBinding): + adapter.weights.source = self.base_model_name + self._added_adapters[adapter.qualified_name] = adapter return - adapter.backend = self - if isinstance(adapter.weights, EmbeddedBinding): - adapter.weights.source = self.base_model_name - self._added_adapters[adapter.qualified_name] = adapter + + if isinstance(adapter, _AdapterCore): + if not isinstance(adapter.weights, EmbeddedBinding): + raise TypeError( + "OpenAIBackend only supports the Embedded/Granite Switch " + f"reality for a composed Adapter; got {type(adapter.weights).__name__}." + ) + # Validated ahead of the duplicate-registration guard below: a + # malformed config= argument must raise even when the call would + # otherwise be a silently-refused duplicate. + if config is None: + raise ValueError( + f"No io.yaml config given for composed embedded adapter " + f"{adapter.identity.name!r}; registering it without one " + "would leave it discoverable but unable to generate. " + "Pass config=, or register it via " + "register_embedded_adapter_model() or resolve_adapter()." + ) + key = _composed_adapter_key(adapter) + # Locked: a direct add_adapter() call (unlike resolve_adapter(), + # which already holds this lock around its own add_adapter() + # calls) is otherwise an unguarded check-then-write across + # _added_adapters/_composed_adapter_configs — two concurrent + # callers registering under the same key could each pass the + # duplicate check below before either writes, then pair one + # call's adapter with the other's config. Reentrant, so a caller + # already holding it (resolve_adapter()) is unaffected. + with self._adapter_activation_lock(): + if key in self._added_adapters: + MelleaLogger.get_logger().warning( + f"attempted to add adapter {key!r} but it is already " + "registered; refusing to overwrite it." + ) + return + adapter.weights.source = self.base_model_name + self._added_adapters[key] = adapter + self._composed_adapter_configs[key] = config + return + + raise TypeError( + "OpenAIBackend currently only supports EmbeddedIntrinsicAdapter or a " + f"composed Adapter. Got: {type(adapter).__name__}" + ) def list_adapters(self) -> list[str]: """Return qualified names of all registered adapters. @@ -368,39 +442,123 @@ def list_adapters(self) -> list[str]: def _adapter_activation_lock( self, ) -> contextlib.AbstractContextManager[bool | None]: - """Serialize `add_adapter()` / registration for this backend.""" + """Serialize `add_adapter()`'s registration-dict writes for this backend.""" return self._adapter_lock + def _adapter_resolve_lock(self) -> contextlib.AbstractContextManager[bool | None]: + """A separate lock for `resolve_adapter()`/registration orchestration. + + Not `_adapter_lock` (the lock `_adapter_activation_lock()` returns): + see that method's docstring, and the base class's + `_adapter_resolve_lock()` docstring, for the deadlock a shared lock + would risk. + """ + return self._adapter_resolve_lock_obj + # ------------------------------------------------------------------ # Convenience registration helpers # ------------------------------------------------------------------ + def _intrinsic_adapter_name_and_config( + self, adapter: "EmbeddedIntrinsicAdapter | _AdapterCore" + ) -> tuple[str, dict]: + """Return the adapter-function name and raw io.yaml config for an adapter. + + `EmbeddedIntrinsicAdapter` carries both directly (`.name`/`.config`). + A composed `Adapter` carries neither — `io_contract` does not yet + drive `IntrinsicsRewriter`/`IntrinsicsResultProcessor` (Epic #929, + issue #1144) — so its config is looked up from + `_composed_adapter_configs`, cached at registration time (see + `add_adapter`/`register_embedded_adapter_model`) since it comes from + `adapter_index.json`/`io.yaml` and cannot be cheaply re-derived. + + Args: + adapter: The adapter to resolve a name and config for. + + Returns: + tuple[str, dict]: The adapter-function name and its parsed + io.yaml config. + + Raises: + ValueError: A composed adapter has no cached config (never + registered via `add_adapter`/`register_embedded_adapter_model`). + """ + if isinstance(adapter, EmbeddedIntrinsicAdapter): + return adapter.name, adapter.config + key = _composed_adapter_key(adapter) + config = self._composed_adapter_configs.get(key) + if config is None: + raise ValueError( + f"No io.yaml config cached for composed adapter {key!r}; register " + "it via register_embedded_adapter_model() or resolve_adapter()." + ) + return adapter.identity.name, config + def register_embedded_adapter_model( - self, source: str, *, revision: str = "main", cache_dir: str | None = None + self, + source: str, + *, + revision: str = "main", + cache_dir: str | None = None, + intrinsic_name: str | None = None, ) -> list[str]: - """Register all embedded adapters from an Embedded Adapter model. + """Register embedded adapters from an Embedded Adapter model. Args: source (str): A local model directory path or Hugging Face Hub repo ID. revision (str): Git revision when loading from Hugging Face Hub. cache_dir (str | None): Cache directory for HF downloads. + intrinsic_name (str | None): If provided, register only the adapter + matching this adapter function name. `None` registers all + adapters found in `source`. Returns: list[str]: Names of the registered intrinsics. - """ - import os - - # No lock here: both call sites (here, and this class's __init__) run - # single-threaded during construction, before the backend is exposed to - # any other thread — see the matching note on LocalHFBackend's twin. - adapters = EmbeddedIntrinsicAdapter.from_source( - source, revision=revision, cache_dir=cache_dir - ) - - for adapter in adapters: - self.add_adapter(adapter) - return [a.intrinsic_name for a in adapters] + Raises: + ImportError: If Hugging Face Hub support is not installed. + PermissionError: If the model repository is private or gated. + FileNotFoundError: If the source has no adapter index. + ValueError: If the source has no matching embedded adapter functions. + TypeError: If an adapter from the source has an unsupported binding. + """ + # Locked (unlike __init__'s call to this, which still runs + # single-threaded during construction, before the backend is exposed + # to any other thread): this method is now also the documented + # post-construction replacement for `EmbeddedIntrinsicAdapter.from_hub()` + # on a live backend, so a caller here can race another thread's + # `add_adapter`/`resolve_adapter` — both `add_adapter`'s + # read-then-write across `_added_adapters` and `_discover_embedded_adapters`' + # mutation of global `warnings` filter state need the same lock + # `add_adapter` itself would take via `resolve_adapter`. + # `_adapter_resolve_lock()`, not `_adapter_activation_lock()`: see the + # lock-order note on the latter — this method's own I/O (this Hub + # discovery call) must not run under the lock `add_adapter()` briefly + # takes around its dict writes. + with self._adapter_resolve_lock(): + discovered = _discover_embedded_adapters( + source, + revision=revision, + cache_dir=cache_dir, + intrinsic_name=intrinsic_name, + ) + names = [] + for adapter, config in discovered: + key = _composed_adapter_key(adapter) + # add_adapter() caches config atomically with registration now, so + # a refused duplicate (a different object already holding `key`) + # never reaches that write — no separate clobber guard needed for + # the config. The identity check below is still required, though: + # add_adapter() returns None on both success and silent refusal, + # so this is the only way to know whether *this* adapter is the + # one that actually ended up registered, for the `names` result. + self.add_adapter(adapter, config=config) + with self._adapter_activation_lock(): + registered = self._added_adapters.get(key) is adapter + if not registered: + continue + names.append(adapter.identity.name) + return names @property def _async_client(self) -> openai.AsyncOpenAI: @@ -705,7 +863,18 @@ async def _generate_from_context( ) alora_action = ALoraRequirement(action.description, adapter_name) - alora_req_adapter = self._find_adapter(adapter_name, ("alora",)) + # An explicit adapter_types override (Epic #929, issue #1144) + # is honoured here — e.g. a custom, LoRA-only adapter would + # never be found by the ("alora",)-only default search, + # silently falling back to regular generation regardless of + # what the caller asked for. + explicit_types = getattr(alora_action, "_adapter_types", None) + search_types = ( + tuple(t.value for t in explicit_types) + if explicit_types + else ("alora",) + ) + alora_req_adapter = self._find_adapter(adapter_name, search_types) if alora_req_adapter is None: if reroute_to_alora and isinstance(action, ALoraRequirement): MelleaLogger.get_logger().warning( @@ -779,9 +948,11 @@ async def _generate_from_intrinsic( streaming is requested (intrinsic post-processing requires the complete response). ValueError: If no embedded adapter is registered for the requested - intrinsic. - TypeError: If the adapter isn't an EmbeddedIntrinsicAdapter, or its - `weights` isn't an EmbeddedBinding (only reachable if a caller + intrinsic, or a composed `Adapter` has no io.yaml config + cached (see `_intrinsic_adapter_name_and_config`). + TypeError: If the adapter is neither an `EmbeddedIntrinsicAdapter` + nor a composed `Adapter`, or its `weights` isn't an + `EmbeddedBinding` (for the shim, only reachable if a caller reassigns `.weights` after construction). """ if not ctx.is_chat_context: @@ -804,16 +975,20 @@ async def _generate_from_intrinsic( # TODO: OpenAIBackend only supports EmbeddedAdapters. # It should be refactored into a specific adapter.transform() function. - if not isinstance(adapter, EmbeddedIntrinsicAdapter): + # EmbeddedIntrinsicAdapter is itself an _AdapterCore subclass, so checking + # for _AdapterCore alone covers both the shim and composed-Adapter realities. + if not isinstance(adapter, _AdapterCore): raise TypeError( - f"OpenAIBackend only supports EmbeddedIntrinsicAdapter, got: {type(adapter).__name__}" + "OpenAIBackend only supports EmbeddedIntrinsicAdapter or a composed " + f"Adapter, got: {type(adapter).__name__}" ) - intrinsic_config = adapter.config - assert intrinsic_config is not None + adapter_name, intrinsic_config = self._intrinsic_adapter_name_and_config( + adapter + ) rewriter = granite_formatters.IntrinsicsRewriter( - config_dict=intrinsic_config, model_name=adapter.name + config_dict=intrinsic_config, model_name=adapter_name ) result_processor = granite_formatters.IntrinsicsResultProcessor( config_dict=intrinsic_config diff --git a/mellea/plugins/hooks/adapter_function.py b/mellea/plugins/hooks/adapter_function.py index 896917e3a4..b70de03f84 100644 --- a/mellea/plugins/hooks/adapter_function.py +++ b/mellea/plugins/hooks/adapter_function.py @@ -57,6 +57,18 @@ class AdapterFunctionPhaseCompletePayload(MelleaBasePayload): phase: Lifecycle phase (`"prepare"`, `"activate"`, `"generate"`, `"parse"`, or `"deactivate"`). duration_ms: Wall-clock duration of the phase in milliseconds. + + Warning: + A subscriber to this hook must not synchronously call + `resolve_adapter()`, `register_embedded_adapter_model()`, or + `add_adapter()` with a composed `Adapter` on the same backend that + fired it. `LocalFileBinding.prepare()` dispatches this hook via a + blocking call onto a dedicated event-loop thread; those three + methods hold a backend-level lock across their whole body, so a + handler that re-enters one of them from that thread deadlocks + against the thread that's still waiting for the hook to return. See + `AdapterMixin._adapter_resolve_lock()`'s docstring for the full + mechanism. """ name: str diff --git a/mellea/stdlib/components/intrinsic/intrinsic.py b/mellea/stdlib/components/intrinsic/intrinsic.py index c73b59d1cd..f52fad1927 100644 --- a/mellea/stdlib/components/intrinsic/intrinsic.py +++ b/mellea/stdlib/components/intrinsic/intrinsic.py @@ -6,11 +6,12 @@ An `Intrinsic` component references a named adapter from Mellea's intrinsic catalog and transforms a chat completion request — typically by injecting new messages, modifying model parameters, or applying structured output constraints. It must be -paired with a backend that supports adapter loading (e.g. `LocalHFBackend` with an -attached `IntrinsicAdapter`). +paired with a backend that supports adapter loading (e.g. `LocalHFBackend` with a +composed `Adapter` — see `mellea.backends.adapters` — attached via `add_adapter`). """ from ....backends.adapters import AdapterType, fetch_intrinsic_metadata +from ....backends.adapters.catalog import IntrinsicsCatalogEntry from ....core import Component, ModelOutputThunk, Span, TemplateRepresentation @@ -22,17 +23,35 @@ class Intrinsic(Component[str]): Must be paired with a backend that supports adapter loading. Args: - intrinsic_name (str): The user-visible name of the intrinsic; must match - a known name in Mellea's intrinsics catalog. + intrinsic_name (str): The user-visible name of the intrinsic. For a + name outside Mellea's intrinsics catalog (a custom, non-catalog + adapter function), `adapter_types` must be provided explicitly — + this is what lets a custom `Intrinsic` construct without a + catalog entry, replacing the deprecated `CustomIntrinsicAdapter` + shim's catalog monkey-patch (Epic #929, issue #1144). intrinsic_kwargs (dict | None): Optional keyword arguments required by the intrinsic at invocation time. + adapter_types (tuple[AdapterType, ...] | None): Adapter types this + intrinsic supports. Required for a name outside the catalog; + optional (defaults to the catalog entry's own types) otherwise. Attributes: - metadata: The intrinsic metadata fetched from the catalog. + metadata: The intrinsic metadata fetched from the catalog, or a + synthetic entry for a name outside it (see `adapter_types` above). + For that synthetic entry, `metadata.repo_id` is `""` and + `metadata.revision` is `"not-a-catalog-entry"` — this class never + reads either field itself, but code that does (e.g. after + reaching in via `intrinsic.metadata.repo_id`) should treat an + empty `repo_id` as "no catalog entry", not as a real value. intrinsic_kwargs (dict): Keyword arguments passed to the intrinsic. intrinsic_name (str): User-visible name of this intrinsic (property). adapter_types (tuple[AdapterType, ...]): Available adapter types that implement this intrinsic (property). Defaults to values in self.metadata. + + Raises: + ValueError: `intrinsic_name` is not in the catalog and `adapter_types` + was not provided — there is no way to know which adapter + realities to look up without either. """ def __init__( @@ -42,7 +61,27 @@ def __init__( adapter_types: tuple[AdapterType, ...] | None = None, ) -> None: """Initialize Intrinsic by fetching metadata for the named intrinsic from the catalog.""" - self.metadata = fetch_intrinsic_metadata(intrinsic_name) + try: + self.metadata = fetch_intrinsic_metadata(intrinsic_name) + except ValueError: + if adapter_types is None: + raise + # A custom, non-catalog adapter function: the caller has already + # told us which adapter types to look up, so a catalog entry + # (whose repo_id/revision this class never reads — see + # `adapter_types` and `intrinsic_name` properties below) isn't + # needed to construct a usable Intrinsic. "not-a-catalog-entry" + # (rather than "unpinned") avoids colliding with the + # `mellea.adapter_function.*` telemetry sentinel, which already + # uses the string "unpinned" to mean "revision is None" for a + # real, catalog-resolvable adapter — a different situation from + # this synthetic entry having no revision concept at all. + self.metadata = IntrinsicsCatalogEntry( + name=intrinsic_name, + repo_id="", + revision="not-a-catalog-entry", + adapter_types=adapter_types, + ) if intrinsic_kwargs is None: intrinsic_kwargs = {} self.intrinsic_kwargs = intrinsic_kwargs diff --git a/mellea/stdlib/requirements/requirement.py b/mellea/stdlib/requirements/requirement.py index 3c992e0c28..2e33fae678 100644 --- a/mellea/stdlib/requirements/requirement.py +++ b/mellea/stdlib/requirements/requirement.py @@ -6,7 +6,7 @@ from collections.abc import Callable from typing import Any, cast, overload -from ...backends.adapters import get_io_contract +from ...backends.adapters import AdapterType, get_io_contract from ...core import ( CBlock, Context, @@ -82,13 +82,28 @@ class ALoraRequirement(Requirement, Intrinsic): description (str): Human-readable requirement description. intrinsic_name (str | None): Name of the ALoRA intrinsic to use. Defaults to `"requirement-check"`. + adapter_types (tuple[AdapterType, ...] | None): Adapter types to look + up for `intrinsic_name`. Required when `intrinsic_name` is a + custom, non-catalog adapter function (see `Intrinsic`). Optional + for a catalog name, which otherwise supplies its own — but an + explicit value here overrides the catalog's, it is not ignored. Attributes: use_aloras (bool): Always `True`; this class always attempts to use ALoRA adapters for validation. + + Raises: + ValueError: `intrinsic_name` is a custom, non-catalog adapter + function and `adapter_types` was not provided. """ - def __init__(self, description: str, intrinsic_name: str | None = None): + def __init__( + self, + description: str, + intrinsic_name: str | None = None, + *, + adapter_types: tuple[AdapterType, ...] | None = None, + ): """Initialize ALoraRequirement with a description and optional intrinsic adapter name.""" # TODO: We may want to actually do the validation_fn here so that we can set the score. super().__init__( @@ -104,6 +119,7 @@ def __init__(self, description: str, intrinsic_name: str | None = None): self, intrinsic_name=intrinsic_name, intrinsic_kwargs={"requirement": f"{self.description}"}, + adapter_types=adapter_types, ) diff --git a/test/backends/test_adapters/test_embedded_adapter.py b/test/backends/test_adapters/test_embedded_adapter.py index 6fc5367a12..d84e9de007 100644 --- a/test/backends/test_adapters/test_embedded_adapter.py +++ b/test/backends/test_adapters/test_embedded_adapter.py @@ -6,6 +6,7 @@ import json import os import pathlib +from typing import Literal from unittest.mock import MagicMock, patch import pytest @@ -645,5 +646,82 @@ def test_adapter_source_defaults_to_model_id(self, model_dir, hub_cache_dir): assert len(backend._added_adapters) == 2 +class TestOpenAIBackendComposedAdapterRegistration: + """Composed-`Adapter` counterpart of `TestOpenAIBackendRegistration` (Epic + #929, issue #1144): `add_adapter` must accept a composed `Adapter` whose + `weights` is an `EmbeddedBinding` directly, alongside the deprecated shim. + """ + + @pytest.fixture + def backend(self): + os.environ.setdefault("OPENAI_API_KEY", "test-key") + from mellea.backends.openai import OpenAIBackend + + return OpenAIBackend( + model_id="granite-switch", base_url="http://localhost:8000/v1" + ) + + @staticmethod + def _make_composed_adapter( + name: str = "answerability", adapter_type: Literal["lora", "alora"] = "alora" + ): + from mellea.backends.adapters._core import Adapter, EmbeddedBinding, Identity + from mellea.backends.adapters.io_contracts import get_io_contract + + return Adapter( + identity=Identity(name=name, adapter_type=adapter_type, capability=name), + io_contract=get_io_contract(name), + weights=EmbeddedBinding(), + ) + + def test_add_composed_adapter(self, backend): + adapter = self._make_composed_adapter() + backend.add_adapter(adapter, config={"parameters": {}}) + assert "answerability_alora" in backend._added_adapters + assert backend._added_adapters["answerability_alora"] is adapter + assert adapter.weights.source == backend.base_model_name # type: ignore[union-attr] + assert backend._composed_adapter_configs["answerability_alora"] == { + "parameters": {} + } + + def test_add_composed_adapter_without_config_raises(self, backend): + adapter = self._make_composed_adapter() + with pytest.raises(ValueError, match=r"No io\.yaml config given"): + backend.add_adapter(adapter) + assert "answerability_alora" not in backend._added_adapters + + def test_add_composed_adapter_rejects_non_embedded_weights(self, backend): + from mellea.backends.adapters._core import Adapter, Identity, LocalFileBinding + + adapter = Adapter( + identity=Identity(name="answerability", adapter_type="lora"), + io_contract=self._make_composed_adapter().io_contract, + weights=LocalFileBinding.from_catalog("answerability"), + ) + with pytest.raises(TypeError, match="Embedded/Granite Switch"): + backend.add_adapter(adapter) + + def test_add_composed_adapter_refuses_duplicate_name(self, backend): + first = self._make_composed_adapter() + second = self._make_composed_adapter() + backend.add_adapter(first, config={"parameters": {}}) + backend.add_adapter(second, config={"parameters": {}}) + assert backend._added_adapters["answerability_alora"] is first + + def test_composed_adapter_discoverable_by_capability(self, backend): + adapter = self._make_composed_adapter() + backend.add_adapter(adapter, config={"parameters": {}}) + assert backend._find_adapter("answerability") is adapter + + def test_list_adapters_includes_composed_adapter(self, backend): + backend.add_adapter(self._make_composed_adapter(), config={"parameters": {}}) + backend.add_adapter( + EmbeddedIntrinsicAdapter( + "citations", config=_CITATIONS_CONFIG, technology="lora" + ) + ) + assert set(backend.list_adapters()) == {"answerability_alora", "citations_lora"} + + if __name__ == "__main__": pytest.main([__file__, "-v", "-s"]) diff --git a/test/backends/test_adapters/test_local_file_binding.py b/test/backends/test_adapters/test_local_file_binding.py index ef7bf25720..eec7832e8e 100644 --- a/test/backends/test_adapters/test_local_file_binding.py +++ b/test/backends/test_adapters/test_local_file_binding.py @@ -240,15 +240,26 @@ def test_prepare_ignores_phase_hook_dispatch_failure(): backend.activate_peft_adapter.assert_called_once_with(binding.qualified_name) -def test_prepare_retries_only_the_load_after_a_load_failure(): - """A failed load must be retryable without re-registering. - - Regression guard: `add_adapter` sets `.backend` (registration) before - `prepare()` calls `load_peft_adapter` (the load). If the load raised, - `.backend` was already non-None, so the old idempotency guard - (`if self.backend is not None: return`) made every retry a silent no-op — - the caller got no error and no adapter, forever. The fix tracks the load - separately from registration so a retry redoes only the failed step. +def test_prepare_rolls_back_registration_after_a_load_failure(): + """A failed load must fully undo this call's own registration, then be retryable. + + Regression guard, in two parts: + + 1. `add_adapter` sets `.backend` (registration) before `prepare()` calls + `load_peft_adapter` (the load). If the load raised, `.backend` was + already non-None, so the old idempotency guard + (`if self.backend is not None: return`) made every retry a silent + no-op — the caller got no error and no adapter, forever. + 2. An earlier fix tracked the load separately from registration so a + retry redid only the failed step — but that left a load failure's + *first* attempt as a registered binding with `._loaded` still False + forever if the caller gave up rather than retried, and — worse, for a + composed `Adapter` wrapping this binding — no way for + `LocalHFBackend.add_adapter`'s composed branch to tell "registered but + not usable" from "not registered", permanently orphaning the + qualified name. `prepare()` now rolls its own registration back on a + load failure, so a binding that never successfully prepares is never + left half-registered, and a retry re-registers cleanly. """ backend = _fake_backend() backend.load_peft_adapter.side_effect = [ @@ -261,15 +272,16 @@ def test_prepare_retries_only_the_load_after_a_load_failure(): with pytest.raises(RuntimeError, match="transient load failure"): binding.prepare() - # Registration succeeded (that's why .backend is set); the load did not. - # A binding in this state must not look "already prepared". - assert binding.backend is backend + # The failed load's own registration was rolled back: no half-registered + # state survives for a caller to mistake as usable. + assert binding.backend is None + backend.remove_adapter.assert_called_once_with(binding.qualified_name) with pytest.raises(RuntimeError, match="prepare"): binding.activate() - binding.prepare() # retry: must not re-register, must retry the load + binding.prepare() # retry: re-registers (rolled back above) and retries the load - backend.add_adapter.assert_called_once() + assert backend.add_adapter.call_count == 2 assert backend.load_peft_adapter.call_count == 2 binding.activate() backend.activate_peft_adapter.assert_called_once_with(binding.qualified_name) @@ -619,5 +631,39 @@ def test_release_does_not_fire_phase_complete_metric(): mock_run.assert_not_called() +def test_prepare_rollback_unloads_peft_before_removing(): + """`_rollback_registration` must unload PEFT state before removing the + registration, mirroring `release()`'s own unload-then-remove order. + + Regression: rollback previously called only `remove_adapter()`, never + `unload_peft_adapter()`. If PEFT itself ends up holding an adapter's + weights loaded (`load_adapter()` succeeded) even though the load as a + whole failed (a later step, e.g. `set_adapter([])`, raised), that left + PEFT holding weights under a name Mellea's own bookkeeping no longer + tracks — invisible until a later same-name registration's own load call + interacted with whatever state PEFT was actually in. + """ + backend = _fake_backend() + backend.load_peft_adapter.side_effect = RuntimeError("load-adjacent failure") + + calls: list[tuple[str, str]] = [] + backend.unload_peft_adapter.side_effect = lambda name: calls.append( + ("unload", name) + ) + backend.remove_adapter.side_effect = lambda name: calls.append(("remove", name)) + + binding = LocalFileBinding(name="answerability") + binding.bind_backend(backend) + + with pytest.raises(RuntimeError, match="load-adjacent failure"): + binding.prepare() + + assert calls == [ + ("unload", binding.qualified_name), + ("remove", binding.qualified_name), + ], "rollback must unload before removing, not remove alone" + assert binding.backend is None + + if __name__ == "__main__": pytest.main([__file__]) diff --git a/test/backends/test_adapters/test_local_file_integration.py b/test/backends/test_adapters/test_local_file_integration.py index a369ffad72..161a827bba 100644 --- a/test/backends/test_adapters/test_local_file_integration.py +++ b/test/backends/test_adapters/test_local_file_integration.py @@ -13,7 +13,8 @@ the spans from a plugin). """ -from unittest.mock import MagicMock, patch +import threading +from unittest.mock import MagicMock, mock_open, patch import pytest @@ -161,5 +162,260 @@ def test_deactivate_runs_even_when_generation_body_raises(): assert isinstance(invocations[0].error, RuntimeError) +def test_add_adapter_registers_composed_adapter_via_backend(): + """`backend.add_adapter(composed_adapter)` drives the binding lifecycle + itself (Epic #929, issue #1144) — a caller no longer has to call + `binding.bind_backend()`/`binding.prepare()` separately before the + composed `Adapter` becomes resolvable by capability name. + """ + backend = _make_backend() + binding = _make_binding() + adapter = _make_adapter(binding) + + with ( + patch( + "mellea.formatters.granite.intrinsics.obtain_lora", + return_value="/fake/local/adapter/path", + ), + patch( + "mellea.formatters.granite.intrinsics.obtain_io_yaml", + return_value="/fake/adapter.yaml", + ), + patch("builtins.open", mock_open(read_data="key: value")), + patch("yaml.safe_load", return_value={"parameters": {}}), + ): + backend.add_adapter(adapter) + + assert binding.backend is backend + assert binding.qualified_name in backend.list_adapters() + # add_adapter also makes the composed Adapter itself discoverable by + # capability, unlike registering a bare LocalFileBinding directly. + found = backend._find_adapter("answerability") + assert found is adapter + + with capture_adapter_hooks() as mock_invoke: + with backend.adapter_scope(adapter): + backend._model.set_adapter.assert_called_with(binding.qualified_name) # type: ignore[union-attr] + backend._model.set_adapter.assert_called_with([]) # type: ignore[union-attr] + + invocations = [p for p in hook_payloads(mock_invoke) if hasattr(p, "outcome")] + assert len(invocations) == 1 + assert invocations[0].outcome == "success" + + +def test_add_adapter_rejects_second_composed_registration_for_same_capability(): + """A second `add_adapter` for the same capability is refused, not silently + overwritten — mirrors the shim's duplicate-registration guard.""" + backend = _make_backend() + binding = _make_binding() + adapter = _make_adapter(binding) + other_binding = _make_binding() + other_adapter = _make_adapter(other_binding) + + with ( + patch( + "mellea.formatters.granite.intrinsics.obtain_lora", + return_value="/fake/local/adapter/path", + ), + patch( + "mellea.formatters.granite.intrinsics.obtain_io_yaml", + return_value="/fake/adapter.yaml", + ), + patch("builtins.open", mock_open(read_data="key: value")), + patch("yaml.safe_load", return_value={"parameters": {}}), + ): + backend.add_adapter(adapter) + backend.add_adapter(other_adapter) + + assert backend._find_adapter("answerability") is adapter + assert other_binding.backend is None + + +def test_add_adapter_rejects_binding_already_bound_to_a_different_backend(): + """Registering a composed Adapter whose LocalFileBinding is already bound + to a *different* backend must raise, not silently misroute. + + Regression: `add_adapter`'s composed-LocalFileBinding branch used to skip + `bind_backend()` — the only call that raises for this — whenever + `binding.backend` was already set, then called the now-no-op `prepare()` + and registered the adapter on the *new* backend anyway. A later + `adapter_scope()`/`activate()` on the new backend would then activate + PEFT state on the *original* backend's model instead, with no error at + any point. + """ + backend_a = _make_backend() + backend_b = _make_backend() + binding = _make_binding() + adapter = _make_adapter(binding) + + with ( + patch( + "mellea.formatters.granite.intrinsics.obtain_lora", + return_value="/fake/local/adapter/path", + ), + patch( + "mellea.formatters.granite.intrinsics.obtain_io_yaml", + return_value="/fake/adapter.yaml", + ), + patch("builtins.open", mock_open(read_data="key: value")), + patch("yaml.safe_load", return_value={"parameters": {}}), + ): + backend_a.add_adapter(adapter) + assert binding.backend is backend_a + + with pytest.raises(RuntimeError, match="cannot change the backend"): + backend_b.add_adapter(adapter) + + # The failed registration attempt must not have touched backend_a's claim. + assert binding.backend is backend_a + assert backend_b._find_adapter("answerability") is None + + +def test_add_adapter_does_not_hold_activation_lock_during_prepare(): + """`add_adapter`'s composed-`LocalFileBinding` branch must not hold the + activation lock while `binding.prepare()` runs. + + Regression: `prepare()` acquires `binding._lifecycle_lock` first, then + (internally, around the weights load) the backend's activation lock — + lifecycle-then-activation. If `add_adapter` held the activation lock + across the whole call to `prepare()`, that's activation-then-lifecycle, + the opposite order — a classic AB-BA deadlock against a concurrent + `prepare()`/`release()` call on the same binding from another thread. + """ + backend = _make_backend() + binding = _make_binding() + adapter = _make_adapter(binding) + + lock_held_during_prepare = [] + orig_prepare = LocalFileBinding.prepare + + def spying_prepare(self): + lock_held_during_prepare.append(backend._generation_lock._is_owned()) + return orig_prepare(self) + + with ( + patch( + "mellea.formatters.granite.intrinsics.obtain_lora", + return_value="/fake/local/adapter/path", + ), + patch( + "mellea.formatters.granite.intrinsics.obtain_io_yaml", + return_value="/fake/adapter.yaml", + ), + patch("builtins.open", mock_open(read_data="key: value")), + patch("yaml.safe_load", return_value={"parameters": {}}), + patch.object(LocalFileBinding, "prepare", spying_prepare), + ): + backend.add_adapter(adapter) + + assert lock_held_during_prepare == [False], ( + "add_adapter must not hold the activation lock while prepare() runs" + ) + # The registration itself must still have succeeded normally. + assert backend._find_adapter("answerability") is adapter + + +def test_concurrent_resolve_waits_for_in_progress_direct_registration(): + """A concurrent `resolve_adapter()` racing an in-progress direct + `add_adapter()` for the same capability must wait for it, not fail. + + Regression: an earlier design used a `_composed_registrations_in_flight` + claim-set to guard the unlocked window between claiming a key and + committing it. A concurrent caller for the same key saw the claim as a + permanent refusal — `add_adapter` returned silently, and the caller + (`resolve_adapter`, checking `_find_adapter` afterwards) raised + `KeyError`, even though the in-progress registration succeeded a moment + later and would have satisfied the request. Holding + `_adapter_resolve_lock()` across the whole registration (not just a + claim-set) makes a concurrent caller block until it commits, instead of + observing a "claimed but not yet found" state at all. + """ + backend = _make_backend() + binding = _make_binding() + adapter = _make_adapter(binding) + + reached_slow_section = threading.Event() + release_thread_a = threading.Event() + + def slow_obtain_io_yaml(*args, **kwargs): + reached_slow_section.set() + assert release_thread_a.wait(timeout=5), "thread B never released thread A" + return "/fake/adapter.yaml" + + errors: list[tuple[str, Exception]] = [] + resolved: dict[str, object] = {} + + def thread_a_direct_add_adapter(): + try: + with ( + patch( + "mellea.formatters.granite.intrinsics.obtain_lora", + return_value="/fake/local/adapter/path", + ), + patch( + "mellea.formatters.granite.intrinsics.obtain_io_yaml", + side_effect=slow_obtain_io_yaml, + ), + patch("builtins.open", mock_open(read_data="key: value")), + patch("yaml.safe_load", return_value={"parameters": {}}), + ): + backend.add_adapter(adapter) + except Exception as e: # pragma: no cover - surfaced via errors list + errors.append(("A", e)) + + def thread_b_resolve(): + assert reached_slow_section.wait(timeout=5), ( + "thread A never reached the locked slow section" + ) + release_thread_a.set() + try: + resolved["b"] = backend.resolve_adapter("answerability") + except Exception as e: # pragma: no cover - surfaced via errors list + errors.append(("B", e)) + + t_a = threading.Thread(target=thread_a_direct_add_adapter, daemon=True) + t_b = threading.Thread(target=thread_b_resolve, daemon=True) + t_a.start() + t_b.start() + t_a.join(timeout=10) + t_b.join(timeout=10) + + assert not t_a.is_alive(), "thread A did not finish within the join timeout" + assert not t_b.is_alive(), "thread B did not finish within the join timeout" + assert not errors, f"resolve_adapter/add_adapter raised under concurrency: {errors}" + assert resolved["b"] is adapter, ( + "the concurrent resolve must return the in-progress registration's " + "own adapter, not fail or construct a competing one" + ) + + +def test_hold_prepared_raises_if_binding_released_before_publish(): + """`LocalFileBinding.hold_prepared()` must refuse to let a caller publish + a composed Adapter around a binding a concurrent `release()` has already + made terminal. + + Regression: `add_adapter`'s composed-`LocalFileBinding` branch used to + have an unguarded window between `prepare()` returning and the commit of + `_composed_adapters[key] = adapter` — nothing prevented a concurrent + `binding.release()` from running in that gap. `_find_adapter()` would + then report the published adapter as registered, but `activate()` on it + would raise, since the binding backing it was already released. + """ + backend = _make_backend() + binding = _make_binding() + + with patch( + "mellea.formatters.granite.intrinsics.obtain_lora", + return_value="/fake/local/adapter/path", + ): + binding.bind_backend(backend) + binding.prepare() + binding.release() + + with pytest.raises(RuntimeError, match="no longer prepared"): + with binding.hold_prepared(backend): + pytest.fail("hold_prepared must raise before yielding") + + if __name__ == "__main__": pytest.main([__file__]) diff --git a/test/backends/test_adapters/test_shims.py b/test/backends/test_adapters/test_shims.py index ff7bd67d41..b781bd7c58 100644 --- a/test/backends/test_adapters/test_shims.py +++ b/test/backends/test_adapters/test_shims.py @@ -19,12 +19,13 @@ from mellea.backends.adapters import ( Adapter, + EmbeddedBinding, EmbeddedIntrinsicAdapter, IntrinsicAdapter, get_io_contract, ) from mellea.backends.adapters._core import Identity, LocalFileBinding -from mellea.backends.adapters.adapter import AdapterMixin +from mellea.backends.adapters.adapter import AdapterMixin, _composed_adapter_key from mellea.backends.adapters.catalog import AdapterType, IntrinsicsCatalogEntry # --------------------------------------------------------------------------- @@ -584,7 +585,12 @@ def test_resolve_adapter_raises_without_base_model(): def test_resolve_adapter_lazy_creates_and_returns(): - """resolve_adapter must create an IntrinsicAdapter when none is registered.""" + """resolve_adapter must create a composed Adapter when none is registered. + + Epic #929, issue #1144: resolve_adapter's default (LORA) construction site + builds a composed `Adapter(identity, io_contract, LocalFileBinding)`, not + the deprecated `IntrinsicAdapter` shim. + """ mock_catalog_entry = IntrinsicsCatalogEntry( name="answerability", repo_id="ibm-granite/granitelib-rag-r1.0", @@ -599,7 +605,7 @@ def test_resolve_adapter_lazy_creates_and_returns(): def fake_add_adapter(a): created_adapters.append(a) - mock_backend._added_adapters[a.qualified_name] = a + mock_backend._added_adapters[_composed_adapter_key(a)] = a mock_backend._added_adapters = {} mock_backend.add_adapter.side_effect = fake_add_adapter @@ -607,16 +613,9 @@ def fake_add_adapter(a): AdapterMixin._find_adapter(mock_backend, cap, types) ) - with ( - patch( - "mellea.backends.adapters.adapter.fetch_intrinsic_metadata", - return_value=mock_catalog_entry, - ), - patch( - "mellea.backends.adapters.adapter.intrinsics.obtain_io_yaml", - return_value="/fake/adapter.yaml", - ), - patch("builtins.open", mock_open(read_data="key: value")), + with patch( + "mellea.backends.adapters.adapter.fetch_intrinsic_metadata", + return_value=mock_catalog_entry, ): result = AdapterMixin.resolve_adapter(mock_backend, "answerability") @@ -624,8 +623,10 @@ def fake_add_adapter(a): "add_adapter must be called for a new capability" ) assert len(created_adapters) == 1 - assert isinstance(created_adapters[0], IntrinsicAdapter) - assert created_adapters[0].adapter_type == AdapterType.LORA + assert isinstance(created_adapters[0], Adapter) + assert not isinstance(created_adapters[0], IntrinsicAdapter) + assert isinstance(created_adapters[0].weights, LocalFileBinding) + assert created_adapters[0].weights.adapter_type == AdapterType.LORA assert result is created_adapters[0] @@ -648,22 +649,17 @@ def test_resolve_adapter_catalog_alias_returns_registered_adapter(): mock_backend._uses_embedded_adapters = False mock_backend._added_adapters = {} mock_backend.add_adapter.side_effect = lambda adapter: ( - mock_backend._added_adapters.__setitem__(adapter.qualified_name, adapter) + mock_backend._added_adapters.__setitem__( + _composed_adapter_key(adapter), adapter + ) ) mock_backend._find_adapter.side_effect = lambda cap, types=None: ( AdapterMixin._find_adapter(mock_backend, cap, types) ) - with ( - patch( - "mellea.backends.adapters.adapter.fetch_intrinsic_metadata", - return_value=mock_catalog_entry, - ), - patch( - "mellea.backends.adapters.adapter.intrinsics.obtain_io_yaml", - return_value="/fake/adapter.yaml", - ), - patch("builtins.open", mock_open(read_data="key: value")), + with patch( + "mellea.backends.adapters.adapter.fetch_intrinsic_metadata", + return_value=mock_catalog_entry, ): result = AdapterMixin.resolve_adapter(mock_backend, "guardian-core") @@ -743,7 +739,7 @@ def test_resolve_adapter_survives_reentrant_activation_lock(): mock_backend._added_adapters = {} mock_backend._adapter_activation_lock.return_value = real_lock mock_backend.add_adapter.side_effect = lambda a: ( - mock_backend._added_adapters.__setitem__(a.qualified_name, a) + mock_backend._added_adapters.__setitem__(_composed_adapter_key(a), a) ) mock_backend._find_adapter.side_effect = lambda cap, types=None: ( AdapterMixin._find_adapter(mock_backend, cap, types) @@ -754,11 +750,6 @@ def test_resolve_adapter_survives_reentrant_activation_lock(): "mellea.backends.adapters.adapter.fetch_intrinsic_metadata", return_value=mock_catalog_entry, ), - patch( - "mellea.backends.adapters.adapter.intrinsics.obtain_io_yaml", - return_value="/fake/adapter.yaml", - ), - patch("builtins.open", mock_open(read_data="key: value")), real_lock, # simulate an already-in-progress caller holding the lock ): result = AdapterMixin.resolve_adapter(mock_backend, "answerability") @@ -767,12 +758,19 @@ def test_resolve_adapter_survives_reentrant_activation_lock(): assert result.identity.name == "answerability" -def test_resolve_adapter_holds_activation_lock_during_lora_registration(): - """resolve_adapter's single-adapter (LORA) path must run inside `_adapter_activation_lock()`. +def test_resolve_adapter_holds_resolve_lock_during_lora_registration(): + """resolve_adapter's single-adapter (LORA) path must run inside `_adapter_resolve_lock()`. Issue #1562: `add_adapter()` is an unguarded read-then-write on - `_added_adapters`; every other verb that touches it already holds this - lock (#1465). Pin the lock's use here so it can't regress silently. + `_added_adapters`; every other verb that touches it already holds a lock + (#1465). Pin the lock's use here so it can't regress silently. + + Not `_adapter_activation_lock()` (Epic #929, issue #1144): that lock is + taken *inside* `add_adapter()`'s composed-`LocalFileBinding` branch and + inside every `LocalFileBinding` lifecycle verb, so holding it across this + whole discover-and-register call would invert the adapter lock order and + risk deadlocking against a concurrent `prepare()`/`release()`. See + `_adapter_resolve_lock()`'s docstring for the full lock order. """ mock_catalog_entry = IntrinsicsCatalogEntry( name="answerability", @@ -786,49 +784,43 @@ def test_resolve_adapter_holds_activation_lock_during_lora_registration(): mock_backend._added_adapters = {} tracking_lock = _TrackingLock() - mock_backend._adapter_activation_lock.return_value = tracking_lock + mock_backend._adapter_resolve_lock.return_value = tracking_lock def fake_add_adapter(a): assert tracking_lock.enter_count == 1 and tracking_lock.exit_count == 0, ( - "add_adapter must run while the activation lock is held" + "add_adapter must run while the resolve lock is held" ) - mock_backend._added_adapters[a.qualified_name] = a + mock_backend._added_adapters[_composed_adapter_key(a)] = a mock_backend.add_adapter.side_effect = fake_add_adapter mock_backend._find_adapter.side_effect = lambda cap, types=None: ( AdapterMixin._find_adapter(mock_backend, cap, types) ) - with ( - patch( - "mellea.backends.adapters.adapter.fetch_intrinsic_metadata", - return_value=mock_catalog_entry, - ), - patch( - "mellea.backends.adapters.adapter.intrinsics.obtain_io_yaml", - return_value="/fake/adapter.yaml", - ), - patch("builtins.open", mock_open(read_data="key: value")), + with patch( + "mellea.backends.adapters.adapter.fetch_intrinsic_metadata", + return_value=mock_catalog_entry, ): AdapterMixin.resolve_adapter(mock_backend, "answerability") assert tracking_lock.enter_count == 1 assert tracking_lock.exit_count == 1 assert tracking_lock.filter_restored_at_exit == [True], ( - "warnings.catch_warnings() must be nested inside the activation lock " + "warnings.catch_warnings() must be nested inside the resolve lock " "(lock outermost) so its filter restoration runs before the lock " "releases — swapping the nesting order reopens the pre-existing " "filter-restoration race between concurrent first-time resolves" ) -def test_resolve_adapter_holds_activation_lock_once_across_embedded_loop(): - """The embedded-adapter loop (#1018) must hold the lock across all iterations. +def test_resolve_adapter_holds_resolve_lock_once_across_embedded_loop(): + """The embedded-adapter loop (#1018) must hold the resolve lock across all iterations. `resolve_adapter()` calls `add_adapter()` once per adapter discovered by `EmbeddedIntrinsicAdapter.from_source()`. The lock must be acquired once for the whole loop, not re-acquired per adapter (that would reopen the - race between iterations). + race between iterations). `_adapter_resolve_lock()`, not + `_adapter_activation_lock()` — see the lock-order note on the latter. """ with warnings.catch_warnings(): warnings.simplefilter("ignore", DeprecationWarning) @@ -844,15 +836,23 @@ def test_resolve_adapter_holds_activation_lock_once_across_embedded_loop(): mock_backend._added_adapters = {} tracking_lock = _TrackingLock() - mock_backend._adapter_activation_lock.return_value = tracking_lock + mock_backend._adapter_resolve_lock.return_value = tracking_lock + mock_backend._supports_composed_adapters = True - def fake_add_adapter(a): - mock_backend._added_adapters[a.qualified_name] = a + def fake_add_adapter(a, config=None): + mock_backend._added_adapters[_composed_adapter_key(a)] = a mock_backend.add_adapter.side_effect = fake_add_adapter mock_backend._find_adapter.side_effect = lambda cap, types=None: ( AdapterMixin._find_adapter(mock_backend, cap, types) ) + # resolve_adapter() routes through this compat wrapper, not add_adapter() + # directly (Epic #929, issue #1144) — a MagicMock(spec=...) auto-mocks it + # too, so it must be wired to the real implementation to actually reach + # fake_add_adapter above. + mock_backend._add_embedded_adapter_compat.side_effect = lambda a, config: ( + AdapterMixin._add_embedded_adapter_compat(mock_backend, a, config) + ) with patch( "mellea.backends.adapters.adapter.EmbeddedIntrinsicAdapter.from_source", @@ -876,7 +876,7 @@ def test_resolve_adapter_concurrent_first_use_does_not_double_register(): check (`existing = registry.get(qualified_name); ...; registry[key] = adapter`) with an injected delay between the read and the write, which makes the unguarded race deterministic: without - `_adapter_activation_lock()` serializing the two threads, both would + `_adapter_resolve_lock()` serializing the two threads, both would read `existing is None` before either writes, and both would overwrite the registry entry independently rather than one of them reusing the other's registration. @@ -896,17 +896,18 @@ def test_resolve_adapter_concurrent_first_use_does_not_double_register(): mock_backend.base_model_name = "ibm-granite/granite-4.1-3b" mock_backend._uses_embedded_adapters = False mock_backend._added_adapters = registry - mock_backend._adapter_activation_lock.return_value = real_lock + mock_backend._adapter_resolve_lock.return_value = real_lock mock_backend._find_adapter.side_effect = lambda cap, types=None: ( AdapterMixin._find_adapter(mock_backend, cap, types) ) def racy_add_adapter(adapter): - existing = registry.get(adapter.qualified_name) + key = _composed_adapter_key(adapter) + existing = registry.get(key) if existing is not None: return time.sleep(0.02) # widen the read-then-write window - registry[adapter.qualified_name] = adapter + registry[key] = adapter registrations.append(adapter) mock_backend.add_adapter.side_effect = racy_add_adapter @@ -966,3 +967,133 @@ def call(index: int) -> None: assert results[0] is results[1] is registrations[0], ( "both concurrent callers must resolve to the single registered adapter" ) + + +def test_add_embedded_adapter_compat_falls_back_to_shim_when_not_supported(): + """A third-party `AdapterMixin` subclass predating composed Adapters + (Epic #929, issue #1144) must not break when `resolve_adapter()`'s + embedded branch tries to register a discovered adapter. + + Regression: `_uses_embedded_adapters` predates the composed-`Adapter` + contract — a legacy `add_adapter` only knows the shim's attribute shape + (`.qualified_name`/`.config`/`.technology`), not a composed `Adapter`'s + (`.identity`/`.io_contract`/`.weights`). The compat method is gated on + the explicit `_supports_composed_adapters` opt-in, not on whether + `add_adapter` merely *accepts* a `config=` keyword — signature inspection + can't tell "accepts the keyword" apart from "understands the object + shape", and a subclass with an unrelated `**kwargs` catch-all would + otherwise be handed a composed `Adapter` its body can't read, raising + `AttributeError` rather than the more diagnosable `TypeError` a bare + signature mismatch would. + """ + registered = [] + + def legacy_add_adapter(adapter, **kwargs): + # Even a **kwargs catch-all (unrelated to config=) must not fool the + # compat method into thinking this body understands a composed + # Adapter — only _supports_composed_adapters governs that. + registered.append(adapter) + + mock_backend = MagicMock(spec=AdapterMixin) + mock_backend._supports_composed_adapters = False + mock_backend.add_adapter = legacy_add_adapter + + composed = Adapter( + identity=Identity(name="answerability", adapter_type="alora"), + io_contract=get_io_contract("answerability"), + weights=EmbeddedBinding(), + ) + + with warnings.catch_warnings(): + warnings.simplefilter("ignore", DeprecationWarning) + AdapterMixin._add_embedded_adapter_compat( + mock_backend, composed, {"parameters": {}} + ) + + assert len(registered) == 1 + shim = registered[0] + assert isinstance(shim, EmbeddedIntrinsicAdapter) + assert shim.intrinsic_name == "answerability" + assert shim.technology == "alora" + assert shim.config == {"parameters": {}} + + +def test_add_embedded_adapter_compat_passes_composed_adapter_when_supported(): + """A backend that declares `_supports_composed_adapters = True` gets the + composed `Adapter` directly — the deprecated-shim fallback is only for a + subclass that hasn't opted in.""" + calls = [] + + def modern_add_adapter(adapter, *, config=None): + calls.append((adapter, config)) + + mock_backend = MagicMock(spec=AdapterMixin) + mock_backend._supports_composed_adapters = True + mock_backend.add_adapter = modern_add_adapter + + composed = Adapter( + identity=Identity(name="answerability", adapter_type="alora"), + io_contract=get_io_contract("answerability"), + weights=EmbeddedBinding(), + ) + + AdapterMixin._add_embedded_adapter_compat( + mock_backend, composed, {"parameters": {}} + ) + + assert calls == [(composed, {"parameters": {}})] + + +def test_resolve_adapter_lora_branch_falls_back_to_shim_when_not_supported(): + """resolve_adapter's LocalFile/LORA branch (the default, non-embedded + path — hit on every ordinary catalog resolution) must gate on + `_supports_composed_adapters` too, not just the embedded branch. + + Regression: only `_add_embedded_adapter_compat` (the embedded branch) + checked `_supports_composed_adapters`. The LORA branch — the common + case, since it's what runs whenever `_uses_embedded_adapters` is unset — + called `self.add_adapter(_AdapterCore(...))` unconditionally. A + third-party AdapterMixin subclass predating composed Adapters, whose + add_adapter reads `.qualified_name`, breaks on essentially every + resolve_adapter() call, not just embedded discovery. + """ + mock_catalog_entry = IntrinsicsCatalogEntry( + name="answerability", + repo_id="ibm-granite/granitelib-rag-r1.0", + revision="abc123", + adapter_types=(AdapterType.ALORA, AdapterType.LORA), + ) + registered = [] + + def legacy_add_adapter(adapter): + registered.append(adapter) + adapter.backend = mock_backend + mock_backend._added_adapters[adapter.qualified_name] = adapter + + mock_backend = MagicMock(spec=AdapterMixin) + mock_backend.base_model_name = "ibm-granite/granite-4.1-3b" + mock_backend._uses_embedded_adapters = False + mock_backend._supports_composed_adapters = False + mock_backend._added_adapters = {} + mock_backend.add_adapter = legacy_add_adapter + mock_backend._find_adapter.side_effect = lambda cap, types=None: ( + AdapterMixin._find_adapter(mock_backend, cap, types) + ) + + with ( + patch( + "mellea.backends.adapters.adapter.fetch_intrinsic_metadata", + return_value=mock_catalog_entry, + ), + patch( + "mellea.backends.adapters.adapter.intrinsics.obtain_io_yaml", + return_value="/fake/adapter.yaml", + ), + patch("builtins.open", mock_open(read_data="key: value")), + ): + result = AdapterMixin.resolve_adapter(mock_backend, "answerability") + + assert len(registered) == 1 + assert isinstance(registered[0], IntrinsicAdapter) + assert registered[0].qualified_name == "answerability_lora" + assert result is registered[0] diff --git a/test/backends/test_huggingface_unit.py b/test/backends/test_huggingface_unit.py index 7a79c22673..ba25932bba 100644 --- a/test/backends/test_huggingface_unit.py +++ b/test/backends/test_huggingface_unit.py @@ -10,7 +10,7 @@ import warnings from types import SimpleNamespace from typing import Any, cast -from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import AsyncMock, MagicMock, mock_open, patch import pytest @@ -42,7 +42,10 @@ ServerMediatedBinding, ) from mellea.backends.adapters._core import Identity -from mellea.backends.adapters.adapter import EmbeddedIntrinsicAdapter +from mellea.backends.adapters.adapter import ( + EmbeddedIntrinsicAdapter, + _ShimWeightsBinding, +) from mellea.backends.adapters.catalog import IntrinsicsCatalogEntry from mellea.backends.huggingface import LocalHFBackend from mellea.core import ModelOutputThunk @@ -233,6 +236,7 @@ def _make_intrinsic_adapter_stub(): name="answerability", adapter_type="alora", capability="answerability" ), ) + object.__setattr__(adapter, "weights", _ShimWeightsBinding()) return adapter @@ -259,6 +263,8 @@ def _make_intrinsic_backend_stub(stub_backend): to_chat_messages=lambda linearized_ctx: [Message("user", "Is the sky blue?")] ) stub_backend._added_adapters = {} + stub_backend._composed_adapters = {} + stub_backend._composed_adapter_configs = {} stub_backend._tokenizer = object() stub_backend._model = object() stub_backend._llguidance_tokenizer = object() @@ -280,6 +286,15 @@ def _make_intrinsic_backend_stub(stub_backend): stub_backend._find_adapter = lambda cap, types=None: AdapterMixin._find_adapter( stub_backend, cap, types ) + stub_backend._intrinsic_adapter_name_and_config = lambda adapter: ( + LocalHFBackend._intrinsic_adapter_name_and_config(stub_backend, adapter) + ) + # Composed-Adapter counterpart of _generate_intrinsic_with_adapter_scope + # above — same bypass, for the same reason (these tests exercise + # option-merging/logits capture, not activation semantics). + stub_backend._generate_composed_local_file_with_adapter_scope = ( + lambda adapter, generate_func, *args, **kwargs: generate_func(*args, **kwargs) + ) return stub_backend @@ -323,12 +338,20 @@ def test_load_embedded_adapters_registers_checkpoint_adapters(): ) mock_from_source.assert_called_once_with( - "/tmp/switch-checkpoint", revision="main", cache_dir=None + "/tmp/switch-checkpoint", revision="main", cache_dir=None, intrinsic_name=None ) assert backend.list_adapters() == ["answerability_alora"] - assert backend._added_adapters["answerability_alora"] is adapter - assert adapter.backend is backend + # register_embedded_adapter_model discovers via the non-shim + # _discover_embedded_adapters factory (Epic #929, issue #1144), which + # lifts the shim's identity/io_contract/weights into a composed Adapter + # stored in _composed_adapters, not the shim instance itself. + registered = backend._composed_adapters["answerability_alora"] + assert registered.identity == adapter.identity + assert registered.weights is adapter.weights assert isinstance(adapter.weights, EmbeddedBinding) + # The composed Adapter is registered, not the shim instance, so the + # shim's own `.backend` is never touched; the binding it shares with the + # composed Adapter (asserted above) is what actually gets stamped. assert adapter.weights.source == backend.base_model_name backend._model.load_adapter.assert_not_called() @@ -807,6 +830,157 @@ def transform(self, chunk, rewritten): assert HookType.ADAPTER_FUNCTION_INVOCATION_COMPLETE not in fired_hook_types +@pytest.mark.asyncio +async def test_composed_adapter_drives_generate_from_intrinsic(stub_backend): + """A composed `Adapter` (not the `IntrinsicAdapter` shim) drives the full + `_generate_from_intrinsic` path — name/config resolution, the composed + `_generate_composed_local_file_with_adapter_scope` dispatch, and normal + post-processing — end to end (Epic #929, issue #1144). + """ + from mellea.backends.adapters._core import ( + Adapter as _AdapterCore, + Identity, + LocalFileBinding, + ) + from mellea.backends.adapters.catalog import AdapterType + from mellea.backends.adapters.io_contracts import get_io_contract + + backend = _make_intrinsic_backend_stub(stub_backend) + backend.processing = AsyncMock(return_value=None) + binding = LocalFileBinding( + name="answerability", + adapter_type=AdapterType.ALORA, + repo_id="ibm-granite/granitelib-rag-r1.0", + revision="abc123", + ) + composed = _AdapterCore( + identity=Identity( + name="answerability", adapter_type="alora", capability="answerability" + ), + io_contract=get_io_contract("answerability"), + weights=binding, + ) + backend._added_adapters = {} + backend._composed_adapters = {"answerability_alora": composed} + backend._composed_adapter_configs = {"answerability_alora": {"parameters": {}}} + backend.base_model_name = "granite-4.1-3b" + + def fake_transformers_inputs(request, tokenizer, model, ll_tokenizer=None): + return {"input_tokens": object()}, {} + + def fake_generate_with_transformers(tokenizer, model, generate_input, other_input): + return _FakeChatCompletionResponseWithContent('{"result": "ok"}') + + class _PassthroughResultProcessor: + def __init__(self, *args, **kwargs): + pass + + def transform(self, chunk, rewritten): + return chunk + + with ( + patch( + "mellea.backends.huggingface.granite_formatters.IntrinsicsRewriter", + _FakeRewriter, + ), + patch( + "mellea.backends.huggingface.granite_formatters.IntrinsicsResultProcessor", + _PassthroughResultProcessor, + ), + patch( + "mellea.formatters.granite.base.util.chat_completion_request_to_transformers_inputs", + side_effect=fake_transformers_inputs, + ), + patch( + "mellea.formatters.granite.base.util.generate_with_transformers", + side_effect=fake_generate_with_transformers, + ), + patch( + "mellea.formatters.granite.intrinsics.obtain_io_yaml", + return_value="/fake/adapter.yaml", + ), + patch("builtins.open", mock_open(read_data="key: value")), + patch("yaml.safe_load", return_value={"parameters": {}}), + ): + output = await LocalHFBackend._generate_from_intrinsic( + backend, + Intrinsic("answerability"), + ChatContext().add(Message("user", "Is the sky blue?")), + model_options={}, + ) + assert output._gen.generate is not None + await output._gen.generate + + assert output._gen.process is not None + processed = False + while not output._gen.queue.empty(): + item = output._gen.queue.get_nowait() + if item is not None: + await output._gen.process(output, item) + processed = True + assert processed, "the composed local-file path must produce a response" + + +def test_composed_local_file_config_is_derived_once_at_registration_not_on_the_loop(): + """A composed LocalFile adapter's io.yaml must be fetched once, synchronously, + during `add_adapter` — never lazily from `_intrinsic_adapter_name_and_config`. + + Regression: an earlier version derived io.yaml lazily, inline inside the + async `_generate_from_intrinsic` (via `_intrinsic_adapter_name_and_config`), + unlike the shim it replaces (`IntrinsicAdapter.__init__`, which loads once + at sync construction time). That put a blocking Hugging Face Hub round + trip on the asyncio event loop on first use of every composed-LocalFile + intrinsic — including the default path via `resolve_adapter`. + """ + from mellea.backends.adapters._core import ( + Adapter as _AdapterCore, + Identity, + LocalFileBinding, + ) + from mellea.backends.adapters.catalog import AdapterType + from mellea.backends.adapters.io_contracts import get_io_contract + + backend = _make_backend() + binding = LocalFileBinding( + name="answerability", + adapter_type=AdapterType.ALORA, + repo_id="ibm-granite/granitelib-rag-r1.0", + revision="abc123", + ) + composed = _AdapterCore( + identity=Identity( + name="answerability", adapter_type="alora", capability="answerability" + ), + io_contract=get_io_contract("answerability"), + weights=binding, + ) + + with ( + patch( + "mellea.formatters.granite.intrinsics.obtain_lora", + return_value="/fake/local/adapter/path", + ), + patch( + "mellea.formatters.granite.intrinsics.obtain_io_yaml", + return_value="/fake/adapter.yaml", + ) as mock_obtain, + patch("builtins.open", mock_open(read_data="key: value")), + patch("yaml.safe_load", return_value={"parameters": {}}), + ): + backend.add_adapter(composed) + + mock_obtain.assert_called_once() + + # A pure cache read from here on: no further obtain_io_yaml calls, + # even off the event loop. + name1, config1 = backend._intrinsic_adapter_name_and_config(composed) + name2, config2 = backend._intrinsic_adapter_name_and_config(composed) + + mock_obtain.assert_called_once() + assert name1 == name2 == "answerability" + assert config1 is config2 + + def test_generate_embedded_with_generation_lock_deactivates_peft_state(): """Embedded generation clears stale PEFT state before running the checkpoint.""" backend = _make_backend() @@ -1041,6 +1215,58 @@ def fake_set_adapter(name_or_names): backend._model.active_adapters.side_effect = lambda: list(active) # type: ignore[attr-defined] +def test_generate_composed_local_file_with_adapter_scope_activates_during_generation(): + """Composed-adapter counterpart of the test above, exercising the real + `_generate_composed_local_file_with_adapter_scope` method directly. + + Coverage gap noted in review: the only other test reaching this method + goes through `stub_backend`, whose fixture replaces it with a + pass-through bypassing the lock hold, `adapter_scope` drive, and both + `_assert_correct_adapters` calls entirely — so the method that owns that + logic had no direct coverage of its own, unlike its shim sibling above. + """ + from mellea.backends.adapters._core import ( + Adapter as _AdapterCore, + LocalFileBinding as _LocalFileBinding, + ) + from mellea.backends.adapters.io_contracts import get_io_contract + + backend = _make_backend() + _wire_fake_peft_model(backend) + + binding = _LocalFileBinding( + name="answerability", + adapter_type=AdapterType.ALORA, + repo_id="fake/repo", + revision="fake0000000000000000000000000000000000000", + ) + binding.backend = backend + binding.path = "/fake/path" + binding._loaded = True + composed = _AdapterCore( + identity=Identity( + name="answerability", adapter_type="alora", capability="answerability" + ), + io_contract=get_io_contract("answerability"), + weights=binding, + ) + + seen_during_generation = [] + + def fake_generate(): + seen_during_generation.append(backend._model.active_adapters()) + return "output" + + out = backend._generate_composed_local_file_with_adapter_scope( + composed, fake_generate + ) + + assert out == "output" + assert seen_during_generation == [[binding.qualified_name]] + assert backend._model.active_adapters() == [] + backend._model.set_adapter.assert_any_call([]) # type: ignore[attr-defined] + + def test_generate_intrinsic_with_adapter_scope_activates_during_generation(): """Generation demonstrably runs with the adapter active — asserted from inside the generate callback via the real (mocked) PEFT model state, not @@ -1285,6 +1511,359 @@ def test_remove_adapter_unregistered_name_is_noop(): backend.remove_adapter("never_registered_lora") # must not raise +def test_remove_adapter_deregisters_composed_embedded_adapter(): + """remove_adapter() must deregister a composed Embedded adapter too. + + Regression: a composed Embedded adapter lives only in _composed_adapters + (see add_adapter) — it never has a bare-binding entry in _added_adapters + to mutate .backend/.path on. remove_adapter's original early return on an + _added_adapters miss meant this case never reached the + _composed_adapters/_composed_adapter_configs cleanup below it. + """ + from mellea.backends.adapters._core import ( + Adapter as _AdapterCore, + EmbeddedBinding, + Identity, + ) + from mellea.backends.adapters.io_contracts import get_io_contract + + backend = _make_backend() + composed = _AdapterCore( + identity=Identity( + name="answerability", adapter_type="alora", capability="answerability" + ), + io_contract=get_io_contract("answerability"), + weights=EmbeddedBinding(), + ) + backend.add_adapter(composed, config={"parameters": {}}) + key = "answerability_alora" + assert key in backend.list_adapters() + + backend.remove_adapter(key) + + assert key not in backend.list_adapters() + assert key not in backend._composed_adapters + assert key not in backend._composed_adapter_configs + + +def test_load_peft_adapter_on_composed_embedded_adapter_raises_type_error(): + """load_peft_adapter() must reject a composed Embedded adapter with TypeError. + + Regression: a composed Embedded adapter lives only in _composed_adapters, + never _added_adapters (see add_adapter's composed-Adapter branch). Without + a check there, load_peft_adapter's `_added_adapters.get(...)` miss raised + ValueError("was not previously added") instead of the documented + TypeError explaining it's activated by the chat template, not PEFT — + misleading, since the adapter genuinely was added. + """ + from mellea.backends.adapters._core import ( + Adapter as _AdapterCore, + EmbeddedBinding, + Identity, + ) + from mellea.backends.adapters.io_contracts import get_io_contract + + backend = _make_backend() + composed = _AdapterCore( + identity=Identity( + name="answerability", adapter_type="alora", capability="answerability" + ), + io_contract=get_io_contract("answerability"), + weights=EmbeddedBinding(), + ) + backend.add_adapter(composed, config={"parameters": {}}) + + with pytest.raises(TypeError, match="cannot load embedded adapter"): + backend.load_peft_adapter("answerability_alora") + + +def test_add_adapter_allows_composed_wrapper_of_a_standalone_registered_binding(): + """A composed Adapter wrapping an already-standalone-registered LocalFileBinding + must register into _composed_adapters, not be silently refused. + + Regression: the duplicate-key guard on the composed branch checked + `key in self._added_adapters` unconditionally — a LocalFileBinding + registered standalone (bare `add_adapter(binding)`) lands there under the + same key, so a subsequent `add_adapter(composed_wrapping_that_binding)` + hit the guard and was silently dropped: the binding's weights stayed + loaded and functional, but the composed Adapter carrying `identity`/ + `io_contract` never made it into `_composed_adapters`, so + `_find_adapter(capability)` could never find this capability. + """ + from mellea.backends.adapters._core import ( + Adapter as _AdapterCore, + LocalFileBinding as _LocalFileBinding, + ) + from mellea.backends.adapters.io_contracts import get_io_contract + + backend = _make_backend() + binding = _LocalFileBinding( + name="answerability", adapter_type=AdapterType.ALORA, repo_id="fake/repo" + ) + binding.get_local_hf_path = lambda base_model_name: "/fake/path" + backend.add_adapter(binding) + assert backend._added_adapters["answerability_alora"] is binding + + composed = _AdapterCore( + identity=Identity( + name="answerability", adapter_type="alora", capability="answerability" + ), + io_contract=get_io_contract("answerability"), + weights=binding, + ) + with ( + patch( + "mellea.formatters.granite.intrinsics.obtain_io_yaml", + return_value="/fake/adapter.yaml", + ), + patch("builtins.open", mock_open(read_data="key: value")), + patch("yaml.safe_load", return_value={"parameters": {}}), + ): + backend.add_adapter(composed) + + assert backend._composed_adapters["answerability_alora"] is composed + assert backend._find_adapter("answerability") is composed + + +def test_add_adapter_rejects_composed_adapter_with_mismatched_identity_and_weights_type(): + """A composed Adapter whose `identity.adapter_type` disagrees with its + own `weights.adapter_type` must be refused at registration, not silently + accepted (NOTE(#1516)). + + Regression: `_composed_adapter_key` (registration/lookup) keys on + `identity.adapter_type`, but the PEFT lifecycle keys on + `weights.qualified_name` (`weights.adapter_type`). A mismatch used to + register a real binding whose weights don't match the adapter variant a + caller asked for by name — silent wrong-variant activation, a duplicate + `list_adapters()` entry, and a registry leak on `remove_adapter`. + """ + from mellea.backends.adapters._core import ( + Adapter as _AdapterCore, + Identity, + LocalFileBinding, + ) + from mellea.backends.adapters.io_contracts import get_io_contract + + backend = _make_backend() + binding = LocalFileBinding( + name="answerability", adapter_type=AdapterType.LORA, repo_id="fake/repo" + ) + mismatched = _AdapterCore( + identity=Identity( + name="answerability", adapter_type="alora", capability="answerability" + ), + io_contract=get_io_contract("answerability"), + weights=binding, + ) + + with pytest.raises(ValueError, match="must agree"): + backend.add_adapter(mismatched) + + assert backend._find_adapter("answerability") is None + assert binding.backend is None + + +def test_add_adapter_shim_refuses_name_already_claimed_by_composed_adapter(): + """The legacy/shim add_adapter path must see composed-adapter registrations too. + + Regression: the duplicate-name guard on the shim path only checked + `_added_adapters`, not `_composed_adapters` — a composed Adapter already + registered under a qualified name did not stop a later shim + `EmbeddedIntrinsicAdapter` (or bare `LocalFileBinding`) from silently + claiming the same name, defeating the "adapter loading is not + idempotent" invariant the warning message itself describes. + """ + from mellea.backends.adapters._core import ( + Adapter as _AdapterCore, + EmbeddedBinding as _EmbeddedBinding, + Identity as _Identity, + ) + from mellea.backends.adapters.io_contracts import get_io_contract + + backend = _make_backend() + composed = _AdapterCore( + identity=_Identity( + name="answerability", adapter_type="alora", capability="answerability" + ), + io_contract=get_io_contract("answerability"), + weights=_EmbeddedBinding(), + ) + backend.add_adapter(composed, config={"parameters": {}}) + assert "answerability_alora" in backend._composed_adapters + + with warnings.catch_warnings(): + warnings.simplefilter("ignore", DeprecationWarning) + shim = EmbeddedIntrinsicAdapter("answerability", config={}, technology="alora") + + backend.add_adapter(shim) + + assert shim.backend is None + assert "answerability_alora" not in backend._added_adapters + + +def test_register_embedded_adapter_model_refused_duplicate_does_not_clobber_cached_config(): + """register_embedded_adapter_model() must not overwrite a live adapter's cached + config, or falsely report it as (re-)registered, when add_adapter() refuses + a duplicate name. + + Regression: add_adapter() silently refuses (logs a warning, returns) rather + than raising for an already-registered qualified name. The loop here used to + write `discovered`'s config into `_composed_adapter_configs` and append the + name unconditionally, so a second discovery call for the same adapter name + would overwrite the first, still-registered adapter's config with whatever + the second (possibly different) discovery produced, and falsely list it as + registered in the returned names. + """ + from mellea.backends.adapters._core import ( + Adapter as _AdapterCore, + EmbeddedBinding as _EmbeddedBinding, + Identity as _Identity, + ) + from mellea.backends.adapters.io_contracts import get_io_contract + + backend = _make_backend() + + def _make_composed(): + return _AdapterCore( + identity=_Identity( + name="answerability", adapter_type="alora", capability="answerability" + ), + io_contract=get_io_contract("answerability"), + weights=_EmbeddedBinding(), + ) + + first_config = {"version": "first"} + with patch( + "mellea.backends.huggingface._discover_embedded_adapters", + return_value=[(_make_composed(), first_config)], + ): + names = backend.register_embedded_adapter_model( + "some/repo", intrinsic_name="answerability" + ) + assert names == ["answerability"] + assert backend._composed_adapter_configs["answerability_alora"] is first_config + + second_config = {"version": "second"} + with patch( + "mellea.backends.huggingface._discover_embedded_adapters", + return_value=[(_make_composed(), second_config)], + ): + names = backend.register_embedded_adapter_model( + "some/other-repo", intrinsic_name="answerability" + ) + + assert names == [] + assert backend._composed_adapter_configs["answerability_alora"] is first_config + + +def test_remove_adapter_deregisters_composed_local_file_adapter(): + """remove_adapter() must clean up a composed LocalFile adapter's entries too. + + Coverage gap noted in review: only the composed-Embedded path (above) had + a regression test for this cleanup; the composed-LocalFile branch at the + same call site (which additionally clears the bare binding's + `.backend`/`.path`) had none. + """ + from mellea.backends.adapters._core import ( + Adapter as _AdapterCore, + LocalFileBinding as _LocalFileBinding, + ) + from mellea.backends.adapters.io_contracts import get_io_contract + + backend = _make_backend() + binding = _LocalFileBinding( + name="answerability", + adapter_type=AdapterType.ALORA, + repo_id="fake/repo", + revision="fake0000000000000000000000000000000000000", + ) + binding.backend = backend + binding.path = "/fake/path" + binding._loaded = True + composed = _AdapterCore( + identity=Identity( + name="answerability", adapter_type="alora", capability="answerability" + ), + io_contract=get_io_contract("answerability"), + weights=binding, + ) + key = "answerability_alora" + # Simulates the post-add_adapter() state directly, bypassing the real + # PEFT-loading side effects add_adapter() would otherwise trigger. + backend._added_adapters[key] = binding + backend._composed_adapters[key] = composed + backend._composed_adapter_configs[key] = {"parameters": {}} + + backend.remove_adapter(key) + + assert key not in backend.list_adapters() + assert key not in backend._added_adapters + assert key not in backend._composed_adapters + assert key not in backend._composed_adapter_configs + assert binding.backend is None + assert binding.path is None + + +def test_add_adapter_raises_for_composed_embedded_adapter_without_config(): + """add_adapter() must reject a composed Embedded adapter with no config. + + Registering one without a config would make it discoverable + (list_adapters()/_find_adapter()) but permanently unable to generate — + add_adapter() is now the only call site that registers this shape, so it + must refuse rather than defer the failure to generation time. + """ + from mellea.backends.adapters._core import ( + Adapter as _AdapterCore, + EmbeddedBinding as _EmbeddedBinding, + ) + from mellea.backends.adapters.io_contracts import get_io_contract + + backend = _make_backend() + composed = _AdapterCore( + identity=Identity( + name="answerability", adapter_type="alora", capability="answerability" + ), + io_contract=get_io_contract("answerability"), + weights=_EmbeddedBinding(), + ) + + with pytest.raises(ValueError, match=r"No io\.yaml config given"): + backend.add_adapter(composed) + + assert "answerability_alora" not in backend._composed_adapters + assert "answerability_alora" not in backend._composed_adapter_configs + + +def test_intrinsic_adapter_name_and_config_raises_for_uncached_composed_embedded(): + """`_intrinsic_adapter_name_and_config` must raise its documented ValueError + when a composed Embedded adapter has no cached config. + + Simulates the post-add_adapter() state directly rather than through the + public API: add_adapter() itself now refuses to register this shape + without a config (see the test above), so the only way to reach this + branch is a registry that got into this state some other way — this + keeps the branch covered without depending on that being possible. + """ + from mellea.backends.adapters._core import ( + Adapter as _AdapterCore, + EmbeddedBinding as _EmbeddedBinding, + ) + from mellea.backends.adapters.io_contracts import get_io_contract + + backend = _make_backend() + composed = _AdapterCore( + identity=Identity( + name="answerability", adapter_type="alora", capability="answerability" + ), + io_contract=get_io_contract("answerability"), + weights=_EmbeddedBinding(), + ) + backend._composed_adapters["answerability_alora"] = composed + + with pytest.raises(ValueError, match=r"No io\.yaml config cached"): + backend._intrinsic_adapter_name_and_config(composed) + + def test_add_adapter_after_remove_adapter_allows_a_fresh_registration(): """#1528: removing an adapter frees its qualified_name for a different adapter object to register under — the name is no longer burned for the @@ -1381,6 +1960,78 @@ def test_remove_adapter_raises_if_still_loaded(): assert adapter.qualified_name not in backend._added_adapters +def test_load_peft_adapter_records_loaded_state_before_set_adapter_failure(): + """`_loaded_adapters` must be recorded before `set_adapter([])`, not after. + + Regression: `_loaded_adapters[...] = adapter` used to run *after* + `self._model.set_adapter([])`. If `self._model.load_adapter(...)` + succeeded (PEFT now holds the weights) but the subsequent + `set_adapter([])` call raised, `_loaded_adapters` was never updated — + which meant `remove_adapter()`'s still-loaded guard couldn't see the + load, `_rollback_registration()` could remove the registration cleanly, + and PEFT was left holding the adapter's weights under a name Mellea's + own bookkeeping no longer tracked. The *next* registration under that + name then hit PEFT's own "already exists" error — silently swallowed by + `load_peft_adapter()` — and appeared to succeed while actually serving + the old, orphaned weights. + """ + backend = _make_backend() + adapter = _make_intrinsic_adapter_stub() + adapter.backend = None + adapter.get_local_hf_path = lambda base_model_name: "/fake/path" + backend.add_adapter(adapter) + backend._model.set_adapter.side_effect = RuntimeError("PEFT set_adapter failure") # type: ignore[union-attr] + + with pytest.raises(RuntimeError, match="PEFT set_adapter failure"): + backend.load_peft_adapter(adapter.qualified_name) + + # The load itself succeeded (load_adapter was called); the bookkeeping + # must reflect that even though the call as a whole raised, so a later + # remove_adapter() correctly refuses to free a name PEFT still holds. + assert adapter.qualified_name in backend._loaded_adapters + with pytest.raises(ValueError, match="still loaded"): + backend.remove_adapter(adapter.qualified_name) + + +def test_unload_peft_adapter_cleans_up_partial_injection_not_in_bookkeeping(): + """`unload_peft_adapter` must attempt real PEFT-level cleanup even when + `_loaded_adapters` has no record of the name. + + Regression: PEFT's own `load_adapter()` injects an adapter's modules into + the model (`inject_adapter_in_model()`) *before* it finishes applying the + checkpoint's weights. A failure partway through that call (e.g. a + corrupt/incomplete checkpoint) leaves PEFT holding a real, injected + adapter under this name — but `_loaded_adapters` is only written after + `self._model.load_adapter(...)` returns successfully, so + `_rollback_registration()`'s call to `unload_peft_adapter()` used to see + "not loaded" and skip cleanup entirely (a pure bookkeeping-gated + no-op) — leaving the injected adapter in place. A later registration + under the same name then hit PEFT's own "already exists" error, which + `load_peft_adapter()` swallows, silently serving the corrupt weights + under the new registration's identity. + """ + backend = _make_backend() + + # No entry in `_loaded_adapters` — this is the "load_adapter() itself + # raised before returning" case, not the "set_adapter([]) failed after + # load_adapter() returned" case the sibling test above covers. + assert "answerability_alora" not in backend._loaded_adapters + + backend.unload_peft_adapter("answerability_alora") + + backend._model.delete_adapter.assert_called_once_with("answerability_alora") # type: ignore[union-attr] + + +def test_unload_peft_adapter_tolerates_peft_also_having_nothing_registered(): + """The best-effort direct cleanup must not raise when PEFT genuinely + never registered the name either (the ordinary, common case: nothing to + clean up at all).""" + backend = _make_backend() + backend._model.delete_adapter.side_effect = ValueError("adapter not found") # type: ignore[union-attr] + + backend.unload_peft_adapter("never-registered_alora") # must not raise + + def test_seed_forces_do_sample_true(stub_backend): """Issue #40: a seed alone must flip do_sample=True so it isn't ignored.""" out = _call(stub_backend, {ModelOption.SEED: 42}) @@ -2879,3 +3530,43 @@ async def test_generate_from_raw_raw_response_none_for_beam_outputs(): assert result.raw.response is None, ( f"item {item_idx}: raw.response must stay None for beam-search outputs" ) + + +@pytest.mark.asyncio +async def test_alora_requirement_dispatch_honors_explicit_adapter_types_override(): + """A custom `ALoraRequirement`'s `adapter_types` override must reach + `_find_adapter`, not the hardcoded `("alora",)` default (Epic #929, + issue #1144). + + Regression: the requirement-check dispatcher in `_generate_from_context` + hardcoded `self._find_adapter(adapter_name, ("alora",))` regardless of + what `adapter_types` the caller passed to `ALoraRequirement` — a custom, + LoRA-only adapter could never be found this way, silently falling back + to regular generation. + """ + from mellea.stdlib.requirements.requirement import ALoraRequirement + + backend = _make_backend() + action = ALoraRequirement( + "custom check", "my-custom-check", adapter_types=(AdapterType.LORA,) + ) + ctx = ChatContext().add(Message("user", "hi")) + + captured: dict[str, object] = {} + + def fake_find_adapter(name, adapter_types=None): + captured["name"] = name + captured["adapter_types"] = adapter_types + return None # not registered; falls back to regular generation below + + with ( + patch.object(backend, "_find_adapter", side_effect=fake_find_adapter), + patch.object( + LocalHFBackend, "_generate_from_context_standard", new_callable=AsyncMock + ) as mock_standard, + ): + mock_standard.return_value = (MagicMock(), ctx) + await backend._generate_from_context(action, ctx, model_options={}) + + assert captured["name"] == "my-custom-check" + assert captured["adapter_types"] == ("lora",) diff --git a/test/backends/test_openai_intrinsics.py b/test/backends/test_openai_intrinsics.py index f6fa354591..df11c6e509 100644 --- a/test/backends/test_openai_intrinsics.py +++ b/test/backends/test_openai_intrinsics.py @@ -223,7 +223,7 @@ def _registered_intrinsic_names(backend: OpenAIBackend) -> set[str]: """Return the set of intrinsic names that have registered adapters.""" names = set() for adapter in backend._added_adapters.values(): - names.add(adapter.intrinsic_name) + names.add(adapter.identity.name) return names diff --git a/test/backends/test_openai_intrinsics_unit.py b/test/backends/test_openai_intrinsics_unit.py index bcca66b16f..4a6e06a9ca 100644 --- a/test/backends/test_openai_intrinsics_unit.py +++ b/test/backends/test_openai_intrinsics_unit.py @@ -175,6 +175,37 @@ def _make_context() -> ChatContext: return ChatContext().add(Message("user", "What is the square root of 4?")) +def _make_backend_with_composed_adapter(config: dict) -> OpenAIBackend: + """Return an OpenAIBackend with a composed `Adapter` registered directly. + + Composed-Adapter counterpart of `_make_backend_with_adapter` (Epic #929, + issue #1144): `add_adapter` and `_generate_from_intrinsic` must drive the + same generation path for a composed `Adapter` as for the deprecated + `EmbeddedIntrinsicAdapter` shim. + """ + from mellea.backends.adapters._core import ( + Adapter as _AdapterCore, + EmbeddedBinding, + Identity, + ) + from mellea.backends.adapters.io_contracts import get_io_contract + + backend = OpenAIBackend( + model_id="granite-switch", + api_key="fake-key", + base_url="http://localhost:9999/v1", + ) + adapter = _AdapterCore( + identity=Identity( + name="answerability", adapter_type="alora", capability="answerability" + ), + io_contract=get_io_contract("answerability"), + weights=EmbeddedBinding(), + ) + backend.add_adapter(adapter, config=config) + return backend + + # --------------------------------------------------------------------------- # Tests # --------------------------------------------------------------------------- @@ -208,6 +239,36 @@ async def test_chat_template_kwargs_set(): assert extra_body["chat_template_kwargs"]["adapter_name"] == "answerability" +async def test_composed_adapter_drives_generate_from_intrinsic(): + """A composed `Adapter` (not the `EmbeddedIntrinsicAdapter` shim) drives + the same generation path — activation, name/config resolution — as the + shim (Epic #929, issue #1144).""" + backend = _make_backend_with_composed_adapter(_SIMPLE_CONFIG) + ctx = _make_context() + mock_create = AsyncMock(return_value=_simple_chat_completion()) + + mock_client = MagicMock() + mock_client.chat.completions.create = mock_create + + with patch.object( + OpenAIBackend, + "_async_client", + new_callable=PropertyMock, + return_value=mock_client, + ): + mot, _ = await mfuncs.aact( + Intrinsic("answerability"), ctx, backend, strategy=None + ) + await mot.avalue() + + mock_create.assert_called_once() + call_kwargs = mock_create.call_args + extra_body = call_kwargs.kwargs.get("extra_body", {}) + + assert "chat_template_kwargs" in extra_body + assert extra_body["chat_template_kwargs"]["adapter_name"] == "answerability" + + async def test_construction_time_extra_body_default_survives_intrinsic_call(): """A construction-time extra_body default must survive an intrinsic call that supplies its own unrelated per-call extra_body, alongside the @@ -787,3 +848,59 @@ def get_temperature(location: str) -> int: assert tools is not None assert len(tools) == 1 assert tools[0]["function"]["name"] == "get_temperature" + + +def test_register_embedded_adapter_model_refused_duplicate_does_not_clobber_cached_config(): + """register_embedded_adapter_model() must not overwrite a live adapter's cached + config, or falsely report it as (re-)registered, when add_adapter() refuses + a duplicate name. + + Regression: add_adapter() silently refuses (logs a warning, returns) rather + than raising for an already-registered qualified name. The loop here used to + write `discovered`'s config into `_composed_adapter_configs` and append the + name unconditionally — mirrors the identical bug fixed on LocalHFBackend. + """ + from mellea.backends.adapters._core import ( + Adapter as _AdapterCore, + EmbeddedBinding as _EmbeddedBinding, + Identity as _Identity, + ) + from mellea.backends.adapters.io_contracts import get_io_contract + + backend = OpenAIBackend( + model_id="granite-switch", + api_key="fake-key", + base_url="http://localhost:9999/v1", + ) + + def _make_composed(): + return _AdapterCore( + identity=_Identity( + name="answerability", adapter_type="alora", capability="answerability" + ), + io_contract=get_io_contract("answerability"), + weights=_EmbeddedBinding(), + ) + + first_config = {"version": "first"} + with patch( + "mellea.backends.openai._discover_embedded_adapters", + return_value=[(_make_composed(), first_config)], + ): + names = backend.register_embedded_adapter_model( + "some/repo", intrinsic_name="answerability" + ) + assert names == ["answerability"] + assert backend._composed_adapter_configs["answerability_alora"] is first_config + + second_config = {"version": "second"} + with patch( + "mellea.backends.openai._discover_embedded_adapters", + return_value=[(_make_composed(), second_config)], + ): + names = backend.register_embedded_adapter_model( + "some/other-repo", intrinsic_name="answerability" + ) + + assert names == [] + assert backend._composed_adapter_configs["answerability_alora"] is first_config diff --git a/test/cli/test_alora_readme_template.py b/test/cli/test_alora_readme_template.py new file mode 100644 index 0000000000..9e9e1b0564 --- /dev/null +++ b/test/cli/test_alora_readme_template.py @@ -0,0 +1,139 @@ +# Copyright IBM Corp. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Regression tests for the generated `cli/alora/README_TEMPLATE.jinja` code sample. + +Nothing renders or executes this template today outside the LLM-assisted +`readme_generator.py` flow, so a construction bug in it (e.g. a missing +required argument) ships silently to every user who trains a custom aLoRA +adapter. These tests render it with representative variables, extract the +fenced Python sample, and execute the adapter/intrinsic construction it +generates for a non-catalog adapter name. +""" + +import os +import re +from unittest.mock import AsyncMock, MagicMock + +import pytest + +pytest.importorskip("jinja2", reason="jinja2 not installed") + +_TEMPLATE_VARS = { + "base_model": "ibm-granite/granite-4.1-3b", + "adapter_name": "Stembolts", + "high_level_description": "Detects defective stembolts from inspection notes.", + "dataset_description": "JSONL rows of inspection notes and defect labels.", + "samples": [{"input": "note text", "output": "defective"}], + "userid": "acme", + "intrinsic_name": "stembolts", + "intrinsic_name_camelcase": "Stembolts", + "arglist": "description: str", + "arglist_without_type_annotations": "description", + "arglist_as_kwargs": "description=description", + "example_call_kwargs": "description='a cracked stembolt'", +} + + +def _render_template() -> str: + from jinja2 import Environment, FileSystemLoader + + template_dir = os.path.join(os.path.dirname(__file__), "..", "..", "cli", "alora") + env = Environment(loader=FileSystemLoader(template_dir)) + template = env.get_template("README_TEMPLATE.jinja") + return template.render(**_TEMPLATE_VARS) + + +def _extract_python_sample(readme_content: str) -> str: + match = re.search(r"```python\n(.*?)\n```", readme_content, re.DOTALL) + assert match is not None, "README template has no fenced python code sample" + code = match.group(1) + # The `if __name__ == "__main__":` block calls the generated function against + # a real LocalHFBackend — exercise only the adapter/intrinsic construction, + # not model loading or generation. + return code.split('if __name__ == "__main__":')[0] + + +@pytest.mark.unit +def test_readme_template_renders_and_python_sample_compiles(): + """The generated README's fenced Python sample must be syntactically valid.""" + code = _extract_python_sample(_render_template()) + compile(code, "", "exec") + + +@pytest.mark.unit +def test_readme_template_constructs_composed_adapter_for_non_catalog_name(): + """The generated composed `Adapter`/`Intrinsic` must construct for a non-catalog name. + + Regression guard: the generated `LocalFileBinding(...)` previously omitted + `revision=`, and the generated `Intrinsic.__init__(...)` previously omitted + `adapter_types=`. Since a trained user adapter is never in Mellea's + intrinsics catalog, both omissions raised + `ValueError: Unknown intrinsic name '...'` the first time a user followed + this exact generated code. + """ + code = _extract_python_sample(_render_template()) + namespace: dict = {} + exec(code, namespace) + + adapter = namespace["_stembolts_adapter"]() + assert adapter.weights.revision is not None + + intrinsic = namespace["StemboltsIntrinsic"](description="a cracked stembolt") + assert intrinsic.metadata.name == "stembolts" + + +@pytest.mark.unit +def test_readme_template_generated_intrinsic_forwards_args_as_intrinsic_kwargs(): + """The generated `Intrinsic` subclass must pass its constructor arguments + through as `intrinsic_kwargs`, not only store them on the `SimpleComponent` + side. + + Regression guard: `_generate_from_intrinsic` explicitly never adds the + action itself to the context ("Intrinsics modify the context through + their rewriters") — the *only* path from an `Intrinsic` action's own data + to the rewritten request is `rewriter.transform(request_json, + **action.intrinsic_kwargs)`. The generated class used to call + `Intrinsic.__init__` with no `intrinsic_kwargs=` at all and store the + caller's arguments only via `SimpleComponent.__init__`, whose + `format_for_llm()` is never invoked for an `Intrinsic` — every + user-supplied argument to a generated custom adapter was silently + dropped before reaching the model. + """ + code = _extract_python_sample(_render_template()) + namespace: dict = {} + exec(code, namespace) + + intrinsic = namespace["StemboltsIntrinsic"](description="a cracked stembolt") + assert intrinsic.intrinsic_kwargs == {"description": "a cracked stembolt"} + + +@pytest.mark.unit +async def test_readme_template_async_wrapper_constructs_intrinsic_correctly(): + """The generated `async_` wrapper must construct its `Intrinsic` subclass + with only the arglist, not an extra positional intrinsic-name argument. + + Regression guard: `async_stembolts(...)` previously called + `StemboltsIntrinsic("stembolts", description)` — an extra leading + positional the sync `stembolts(...)` wrapper (right below it) never had + — raising `TypeError: __init__() takes 2 positional arguments but 3 were + given` for every user who called the generated async wrapper. + """ + code = _extract_python_sample(_render_template()) + namespace: dict = {} + exec(code, namespace) + + backend = MagicMock() + backend.list_adapters.return_value = ["stembolts_alora"] + fake_ctx = MagicMock() + backend.generate_from_context = AsyncMock(return_value=("mocked output", fake_ctx)) + + result, result_ctx = await namespace["async_stembolts"]( + description="a cracked stembolt", ctx=MagicMock(), backend=backend + ) + + assert result == "mocked output" + assert result_ctx is fake_ctx + backend.generate_from_context.assert_awaited_once() + action = backend.generate_from_context.await_args.args[0] + assert isinstance(action, namespace["StemboltsIntrinsic"]) diff --git a/test/stdlib/components/intrinsic/test_intrinsic.py b/test/stdlib/components/intrinsic/test_intrinsic.py index 6a4309cff1..3d9ef4cf3f 100644 --- a/test/stdlib/components/intrinsic/test_intrinsic.py +++ b/test/stdlib/components/intrinsic/test_intrinsic.py @@ -3,10 +3,39 @@ """Unit tests for the Intrinsic component.""" +import pytest + from mellea.backends.adapters import AdapterType from mellea.stdlib.components import Intrinsic +class TestCustomNonCatalogName: + """A name outside the intrinsics catalog (Epic #929, issue #1144). + + Replaces the deprecated `CustomIntrinsicAdapter` shim's catalog + monkey-patch: a custom adapter function can construct an `Intrinsic` + without any catalog mutation, as long as it supplies `adapter_types`. + """ + + def test_custom_name_without_adapter_types_raises(self): + with pytest.raises(ValueError, match="Unknown intrinsic name"): + Intrinsic("totally-made-up-name") + + def test_custom_name_with_adapter_types_constructs(self): + intrinsic = Intrinsic( + "totally-made-up-name", adapter_types=(AdapterType.ALORA,) + ) + assert intrinsic.intrinsic_name == "totally-made-up-name" + assert intrinsic.adapter_types == (AdapterType.ALORA,) + assert intrinsic.metadata.name == "totally-made-up-name" + + def test_known_name_ignores_synthetic_fallback(self): + """A real catalog name never hits the synthetic-entry path, even + without adapter_types.""" + intrinsic = Intrinsic("answerability") + assert intrinsic.metadata.repo_id != "" + + class TestAdapterTypesOverride: """Verify the adapter_types constructor parameter.""" diff --git a/test/stdlib/requirements/test_requirement.py b/test/stdlib/requirements/test_requirement.py index c51d14726b..3e9cca8753 100644 --- a/test/stdlib/requirements/test_requirement.py +++ b/test/stdlib/requirements/test_requirement.py @@ -6,7 +6,7 @@ import pytest -from mellea.backends.adapters import AdapterSchemaMismatchError +from mellea.backends.adapters import AdapterSchemaMismatchError, AdapterType from mellea.core import ModelOutputThunk, Requirement from mellea.stdlib.context import ChatContext from mellea.stdlib.requirements import LLMaJRequirement, simple_validate @@ -227,6 +227,7 @@ def test_alora_requirement_default_intrinsic(mock_intrinsic_init): r, intrinsic_name="requirement-check", intrinsic_kwargs={"requirement": "must be valid"}, + adapter_types=None, ) @@ -239,6 +240,26 @@ def test_alora_requirement_custom_intrinsic(mock_intrinsic_init): r, intrinsic_name="custom_check", intrinsic_kwargs={"requirement": "must be valid"}, + adapter_types=None, + ) + + +@patch("mellea.stdlib.requirements.requirement.Intrinsic.__init__") +def test_alora_requirement_forwards_adapter_types(mock_intrinsic_init): + """adapter_types is required to use a custom, non-catalog intrinsic_name + (Epic #929, issue #1144) — ALoraRequirement must forward it to Intrinsic. + """ + mock_intrinsic_init.return_value = None + r = ALoraRequirement( + "must be valid", + intrinsic_name="custom_check", + adapter_types=(AdapterType.ALORA,), + ) + mock_intrinsic_init.assert_called_once_with( + r, + intrinsic_name="custom_check", + intrinsic_kwargs={"requirement": "must be valid"}, + adapter_types=(AdapterType.ALORA,), )