From 6bc3d2c59a3cd3b53b4131e7bcedb95f0d7ba069 Mon Sep 17 00:00:00 2001 From: AngeloDanducci Date: Wed, 30 Sep 2026 10:42:45 -0400 Subject: [PATCH 1/2] remove deprecated adapter shims Signed-off-by: AngeloDanducci --- docs/docs/integrations/openai.md | 3 +- docs/docs/reference/glossary.md | 8 +- .../tutorials/07-custom-adapter-function.md | 4 +- .../granite-switch/manual_adapter_loading.py | 3 +- mellea/backends/adapters/__init__.py | 4 - mellea/backends/adapters/_core.py | 14 +- mellea/backends/adapters/adapter.py | 1086 +++++----------- mellea/backends/adapters/io_contracts.py | 12 +- mellea/backends/huggingface.py | 327 +---- mellea/backends/ollama.py | 2 - mellea/backends/openai.py | 102 +- mellea/stdlib/components/intrinsic/_util.py | 2 +- .../stdlib/components/intrinsic/intrinsic.py | 4 +- test/backends/test_adapters/test_adapter.py | 62 - .../test_adapters/test_embedded_adapter.py | 392 +++--- .../test_embedded_integration.py | 27 +- .../test_adapters/test_io_contracts.py | 2 +- .../test_adapters/test_local_file_e2e.py | 2 +- test/backends/test_adapters/test_shims.py | 1099 ----------------- test/backends/test_huggingface.py | 25 +- test/backends/test_huggingface_unit.py | 468 ++++--- test/backends/test_openai_intrinsics_unit.py | 37 +- test/package/test_dependency_isolation.py | 2 +- .../components/intrinsic/test_intrinsic.py | 5 +- 24 files changed, 893 insertions(+), 2799 deletions(-) delete mode 100644 test/backends/test_adapters/test_adapter.py delete mode 100644 test/backends/test_adapters/test_shims.py diff --git a/docs/docs/integrations/openai.md b/docs/docs/integrations/openai.md index cc59421409..182ae4d31d 100644 --- a/docs/docs/integrations/openai.md +++ b/docs/docs/integrations/openai.md @@ -361,8 +361,7 @@ backend = OpenAIBackend( ) # 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. +# this composes an Adapter (Identity + IOContract + EmbeddedBinding). backend.register_embedded_adapter_model( IBM_GRANITE_SWITCH_4_1_3B_PREVIEW.hf_model_name, intrinsic_name="answerability", diff --git a/docs/docs/reference/glossary.md b/docs/docs/reference/glossary.md index b3fb46f718..5cda53ece2 100644 --- a/docs/docs/reference/glossary.md +++ b/docs/docs/reference/glossary.md @@ -572,10 +572,10 @@ runtime or uses adapter functions embedded in a local [Granite Switch](#granite- checkpoint with `load_embedded_adapters=True`. `OpenAIBackend` also supports embedded adapter functions when backed by a Granite Switch deployment. -> **Note:** The Python symbol `Intrinsic` (and related classes such as `IntrinsicAdapter`) -> will be renamed to `AdapterFunction` / `Adapter` in a future phase of Epic #929 -> (#1136). Prefer the term **adapter function** in prose and documentation; use `Intrinsic` -> only when referencing the current Python API directly. +> **Note:** The Python symbol `Intrinsic` will be renamed to `AdapterFunction` +> in a future phase of Epic #929 (#1136). Prefer the term **adapter function** +> in prose and documentation; use `Intrinsic` only when referencing the current +> Python API directly. See: [Adapter Functions](../advanced/intrinsics.md) diff --git a/docs/docs/tutorials/07-custom-adapter-function.md b/docs/docs/tutorials/07-custom-adapter-function.md index b96e4505ec..8e3a329682 100644 --- a/docs/docs/tutorials/07-custom-adapter-function.md +++ b/docs/docs/tutorials/07-custom-adapter-function.md @@ -6,7 +6,7 @@ description: "Compose your own trained LoRA/aLoRA adapter into Mellea without a 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). +removed `CustomIntrinsicAdapter` shim (Epic #929, issue #1144). By the end you will have covered: @@ -137,7 +137,7 @@ result = m.instruct( ## What replaced the old approach -The deprecated `CustomIntrinsicAdapter` shim did two things under the hood +The removed `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 diff --git a/docs/examples/granite-switch/manual_adapter_loading.py b/docs/examples/granite-switch/manual_adapter_loading.py index 13c5a1bc0c..0b5c136c68 100644 --- a/docs/examples/granite-switch/manual_adapter_loading.py +++ b/docs/examples/granite-switch/manual_adapter_loading.py @@ -6,8 +6,7 @@ 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 `register_embedded_adapter_model(intrinsic_name=...)`, which composes an -`Adapter` (Identity + IOContract + EmbeddedBinding) instead of the deprecated -`EmbeddedIntrinsicAdapter` shim. +`Adapter` (Identity + IOContract + EmbeddedBinding). This is useful when: - You only need a subset of the model's embedded adapters. diff --git a/mellea/backends/adapters/__init__.py b/mellea/backends/adapters/__init__.py index f9dc2aaa9b..fa507d5d4a 100644 --- a/mellea/backends/adapters/__init__.py +++ b/mellea/backends/adapters/__init__.py @@ -19,8 +19,6 @@ AdapterInput, AdapterMixin, AdapterType, - EmbeddedIntrinsicAdapter, - IntrinsicAdapter, LocalHFAdapter, fetch_intrinsic_metadata, get_adapter_for_intrinsic, @@ -39,10 +37,8 @@ "AloraActivationError", "EmbeddedActivationRequest", "EmbeddedBinding", - "EmbeddedIntrinsicAdapter", "IOContract", "Identity", - "IntrinsicAdapter", "LocalFileBinding", "LocalHFAdapter", "ServerMediatedBinding", diff --git a/mellea/backends/adapters/_core.py b/mellea/backends/adapters/_core.py index d05ae259bc..ed2ceacdc4 100644 --- a/mellea/backends/adapters/_core.py +++ b/mellea/backends/adapters/_core.py @@ -27,7 +27,7 @@ The existing :class:`~mellea.backends.adapters.adapter.Adapter` ABC in `adapter.py` is not modified here. This module introduces a new `Adapter` *dataclass* that is re-exported from - `mellea.backends.adapters`. Both coexist until shim removal in 4.1. + `mellea.backends.adapters`. Both coexist for now. The old ABC is not part of the public `__init__.py` surface, so there is no namespace collision on the public API. """ @@ -1129,11 +1129,7 @@ class Adapter: # (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. + # is enforced not here but in `LocalHFBackend.add_adapter`, at registration + # time, so a construction that never reaches a backend isn't rejected. + # `EmbeddedBinding` has no `name`/`adapter_type` of its own, so the + # Embedded/Granite Switch reality has nothing to cross-check against. diff --git a/mellea/backends/adapters/adapter.py b/mellea/backends/adapters/adapter.py index e753a377cb..f433969e38 100644 --- a/mellea/backends/adapters/adapter.py +++ b/mellea/backends/adapters/adapter.py @@ -8,23 +8,22 @@ (look up a registered adapter). :class:`AdapterMixin` is mixed into backends that support runtime adapter loading and unloading. -`LocalHFAdapter`, `IntrinsicAdapter`, and `EmbeddedIntrinsicAdapter` are -**deprecation shims** retained for backwards compatibility. They satisfy -`isinstance(x, _core.Adapter)` but delegate all behaviour to the new dataclass. -`get_adapter_for_intrinsic` is similarly deprecated; prefer `resolve_adapter`. +`Adapter` and `LocalHFAdapter` are the legacy adapter ABCs retained for +third-party backends; the composed `Adapter` dataclass in `._core` is the +current adapter surface. `get_adapter_for_intrinsic` is deprecated; prefer +`resolve_adapter`. """ import abc import contextlib import hashlib import pathlib -import re import shutil import tempfile import time -import warnings from collections.abc import Callable, Sequence -from typing import ClassVar, Literal, TypeAlias, TypeVar, cast +from typing import Literal, TypeAlias, TypeVar, cast + import yaml @@ -100,227 +99,334 @@ def get_local_hf_path(self, base_model_name: str) -> str: ... -class _ShimWeightsBinding(WeightsBinding): - """Placeholder weights binding for the deprecated IntrinsicAdapter shims. - - All lifecycle verbs raise NotImplementedError; it exists only so the - shims can satisfy the Adapter protocol. - """ - - def prepare(self) -> None: - raise NotImplementedError("WeightsBinding not yet implemented") - - def activate(self) -> None: - raise NotImplementedError("WeightsBinding not yet implemented") +T = TypeVar("T") - def deactivate(self) -> None: - raise NotImplementedError("WeightsBinding not yet implemented") - def release(self) -> None: - raise NotImplementedError("WeightsBinding not yet implemented") +def _composed_adapter_key(adapter: "_AdapterCore") -> str: + """Return the registry key for a composed `Adapter`, mirroring `qualified_name`. + A composed `Adapter` 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. -class IntrinsicAdapter(LocalHFAdapter, _AdapterCore): - """Deprecated shim for adapters that implement adapter functions. + Args: + adapter: The composed adapter to key. - Deprecated: - Use :class:`~mellea.backends.adapters.Adapter` directly. - `IntrinsicAdapter` will be removed in a future release (Epic #929, - issue #1144). + Returns: + str: `"_"`. + """ + return f"{adapter.identity.name}_{adapter.identity.adapter_type}" - Subtype of :class:`Adapter` for models that: - * implement adapter functions - * are packaged as LoRA or aLoRA adapters on top of a base model - * use the shared model loading code in `mellea.formatters.granite.intrinsics` - * use the shared input and output processing code in - `mellea.formatters.granite.intrinsics` +def _embedded_adapter_from_entry( + intrinsic_name: str, config: dict, technology: str +) -> "_AdapterCore": + """Build a composed embedded `Adapter` from one `adapter_index.json` entry. Args: - intrinsic_name (str): Name of the adapter function (e.g. `"answerability"`); - the adapter's `qualified_name` will be derived from this. - adapter_type (AdapterType): Enum describing the adapter type; defaults to - `AdapterType.ALORA`. - config_file (str | pathlib.Path | None): Path to a YAML config file defining - the adapter function's I/O transformations; mutually exclusive with - `config_dict`. - config_dict (dict | None): Dict defining the adapter function's I/O - transformations; mutually exclusive with `config_file`. - base_model_name (str | None): Base model name used to look up the I/O - processing config when neither `config_file` nor `config_dict` are - provided. + intrinsic_name (str): Adapter function name from the index entry + (e.g. `"answerability"`). + config (dict): Parsed `io.yaml` transformation configuration. + technology (str): Adapter technology in the switch model — `"lora"` or + `"alora"`. Determines the `Identity.adapter_type`. - Attributes: - intrinsic_name (str): Name of the adapter function this adapter implements. - intrinsic_metadata (IntrinsicsCatalogEntry): Catalog metadata for the adapter function. - base_model_name (str | None): Base model name provided at construction, if any. - adapter_type (AdapterType): The adapter type (`LORA` or `ALORA`). - config (dict): Parsed I/O transformation configuration for the adapter function. - - Note: - `identity`, `io_contract`, and `weights` are internal scaffolding populated - in `__init__` to satisfy the :class:`~mellea.backends.adapters.Adapter` - protocol; they are not meaningful consumer-facing attributes. `io_contract` - is the real, declared contract for `intrinsic_name` (issue #1516); `weights` - remains the Phase 1 `_ShimWeightsBinding` placeholder and raises - `NotImplementedError` until Phase 2 (issue #1141) replaces it. + Returns: + _AdapterCore: A composed `Adapter` whose `weights` is an + `EmbeddedBinding` (activation runs through the served model's chat + template), carrying the declared I/O contract for `intrinsic_name`. + + Raises: + ValueError: If `technology` is neither `"lora"` nor `"alora"`. """ + if technology not in ("lora", "alora"): + raise ValueError(f"technology must be 'lora' or 'alora', got '{technology}'") + capability = intrinsic_name + if intrinsic_name in known_intrinsic_names(): + capability = fetch_intrinsic_metadata(intrinsic_name).effective_capability + return _AdapterCore( + identity=Identity( + name=intrinsic_name, + adapter_type=cast(Literal["lora", "alora"], technology), + capability=capability, + ), + io_contract=get_io_contract(intrinsic_name), + weights=EmbeddedBinding(), + ) - def __setattr__(self, name: str, value: object) -> None: - """Allow mutation; bypasses the frozen restriction on _AdapterCore.""" - object.__setattr__(self, name, value) - def __delattr__(self, name: str) -> None: - """Allow deletion; bypasses the frozen restriction on _AdapterCore.""" - object.__delattr__(self, name) +def _embedded_adapters_from_model_directory( + model_path: str | pathlib.Path, intrinsic_name: str | None = None +) -> list[tuple["_AdapterCore", dict]]: + """Load composed embedded adapters from a Granite Switch model directory. - def __init__( - self, - intrinsic_name: str, - adapter_type: AdapterType = AdapterType.ALORA, - config_file: str | pathlib.Path | None = None, - config_dict: dict | None = None, - base_model_name: str | None = None, - ): - """Initialize IntrinsicAdapter for the named adapter function, loading its I/O configuration.""" - warnings.warn( - "IntrinsicAdapter is deprecated; use Adapter directly (Epic #929, issue #1144).", - DeprecationWarning, - stacklevel=2, - ) - super().__init__(intrinsic_name, adapter_type) + Reads `adapter_index.json` and the corresponding `io_configs/*/io.yaml` + files from the model directory, returning one composed `Adapter` (paired + with its raw `io.yaml` config) per index entry. - self.intrinsic_name = intrinsic_name - self.intrinsic_metadata = fetch_intrinsic_metadata(intrinsic_name) - self.base_model_name = base_model_name + Args: + model_path (str | pathlib.Path): Path to a Granite Switch model + directory that contains `adapter_index.json` and `io_configs/`. + intrinsic_name (str | None): If provided, only load the adapter + matching this adapter function name. `None` loads all adapters. - if adapter_type not in self.intrinsic_metadata.adapter_types: - raise ValueError( - f"Adapter function '{intrinsic_name}' not available as an adapter of type " - f"'{adapter_type}. Available types are " - f"{self.intrinsic_metadata.adapter_types}." - ) - self.adapter_type = adapter_type + Returns: + list[tuple[_AdapterCore, dict]]: One `(adapter, io_yaml_config)` pair + per entry in the index. - # If any of the optional params are specified, attempt to set up the - # config for the adapter function here. - if config_file and config_dict: + Raises: + FileNotFoundError: If `adapter_index.json` is missing. + ValueError: If an `io.yaml` file listed in the index cannot be found, + if an `io_config` path escapes the model directory, if an entry's + `technology` is not `"lora"`/`"alora"`, or if no adapters are found. + """ + import json as _json + + model_path = pathlib.Path(model_path) + index_path = model_path / "adapter_index.json" + if not index_path.exists(): + raise FileNotFoundError(f"No adapter_index.json found at {index_path}") + + with open(index_path, encoding="utf-8") as f: + index = _json.load(f) + + adapters: list[tuple[_AdapterCore, dict]] = [] + for entry in index.get("adapters", []): + entry_name = entry.get("adapter_name") + if entry_name is None: + continue + if intrinsic_name is not None and entry_name != intrinsic_name: + continue + io_config_rel = entry.get("io_config") + if io_config_rel is None: + continue + + io_config_path = model_path / io_config_rel + try: + io_config_path = io_config_path.resolve(strict=True) + except (FileNotFoundError, OSError): raise ValueError( - f"Conflicting values for config_file and config_dict " - f"parameters provided. Values were {config_file=} " - f"and {config_dict=}" + f"io.yaml for adapter function '{entry_name}' " + f"not found at {model_path / io_config_rel}" ) - if config_file is None and config_dict is None and self.base_model_name is None: + if not io_config_path.is_relative_to(model_path.resolve()): raise ValueError( - "At least one of [config_file, config_dict, base_model_name] " - "must be provided." + f"io_config path for adapter function '{entry_name}' " + f"escapes the model directory: {io_config_path}" ) - if config_file is None and config_dict is None: - assert self.base_model_name is not None, ( - "must provide `base_model_name` if not providing a `config_file` or `config_dict`" - ) - # We're converting the adapter type to a boolean flag here. - assert adapter_type in (AdapterType.ALORA, AdapterType.LORA), ( - f"{adapter_type} not supported" - ) - is_alora = self.adapter_type == AdapterType.ALORA - config_file = intrinsics.obtain_io_yaml( - self.intrinsic_name, - self.base_model_name, - self.intrinsic_metadata.repo_id, - revision=self.intrinsic_metadata.revision, - alora=is_alora, - ) - if config_file: - with open(config_file, encoding="utf-8") as f: - config_dict = yaml.safe_load(f) - if not isinstance(config_dict, dict): - raise ValueError( - f"YAML file {config_file} does not evaluate to a " - f"dictionary when parsed." - ) - assert config_dict is not None # Code above should initialize this variable - self.config: dict = config_dict - - # Populate the new Adapter triple so isinstance(self, _AdapterCore) holds. - # io_contract comes from the same registry resolve_adapter() consults - # (see issue #1516), not a placeholder. weights stays the Phase 2 - # _ShimWeightsBinding placeholder; that axis is #1141/#1142. - _AdapterCore.__init__( - self, - identity=Identity( - name=intrinsic_name, - adapter_type="alora" - if self.adapter_type == AdapterType.ALORA - else "lora", - capability=self.intrinsic_metadata.effective_capability, - ), - io_contract=get_io_contract(intrinsic_name), - weights=_ShimWeightsBinding(), + + with open(io_config_path, encoding="utf-8") as f: + config_dict = yaml.safe_load(f) + + adapter = _embedded_adapter_from_entry( + entry_name, config_dict, entry.get("technology", "lora") ) + adapters.append((adapter, config_dict)) - def get_local_hf_path(self, base_model_name: str) -> str: - """Return the local filesystem path from which adapter weights should be loaded. + if not adapters: + if intrinsic_name is not None: + raise ValueError( + f"No adapter found for adapter function '{intrinsic_name}' in {model_path}" + ) + raise ValueError(f"No adapters found in {model_path}") - Downloads the adapter weights if they are not already cached locally. + return adapters - Args: - base_model_name (str): The base model name; typically the last component - of the Hugging Face model ID (e.g. `"granite-3.3-8b-instruct"`). - Returns: - str: Filesystem path to the downloaded adapter weights directory. - """ - return self.download_and_get_path(base_model_name) +def _embedded_adapters_from_hub( + repo_id: str, + revision: str = "main", + cache_dir: str | None = None, + intrinsic_name: str | None = None, +) -> list[tuple["_AdapterCore", dict]]: + """Load composed embedded adapters from a Granite Switch model on the Hub. + + Downloads `adapter_index.json` and the `io_configs/` directory into a + persistent self-contained local directory, then delegates to + `_embedded_adapters_from_model_directory`. + + `huggingface_hub.snapshot_download`'s default cache-backed snapshot + directory populates `io_configs/` with symlinks that resolve into a + sibling `blobs/` directory *outside* the snapshot root. That breaks the + contract `_embedded_adapters_from_model_directory` expects (a + self-contained model directory) and trips its path-escape check. To satisfy + that contract, the downloaded snapshot is materialised under the Hugging + Face cache into a self-contained directory keyed by its immutable revision, + so `io_configs/` contains real files rather than symlinks escaping the + directory. This preserves standard Hugging Face Hub cache reuse and offline + loading while preventing stale files from a mutable revision. - def download_and_get_path(self, base_model_name: str) -> str: - """Download the required adapter function files if necessary and return the path to them. + Args: + repo_id (str): Hugging Face Hub repository ID + (e.g. `"ibm-granite/granite-switch-micro"`). + revision (str): Git revision to download from. + cache_dir (str | None): Local cache directory; `None` for the default. + intrinsic_name (str | None): If provided, only load the adapter + matching this adapter function name. `None` loads all adapters. - Args: - base_model_name: the base model; typically the last part of the Hugging Face - model id like "granite-3.3-8b-instruct" + Returns: + list[tuple[_AdapterCore, dict]]: One `(adapter, io_yaml_config)` pair + per entry in the index. - Returns: - a path to the files - """ - is_alora = self.adapter_type == AdapterType.ALORA - return str( - intrinsics.obtain_lora( - self.intrinsic_name, - base_model_name, - self.intrinsic_metadata.repo_id, - revision=self.intrinsic_metadata.revision, - alora=is_alora, + Raises: + ImportError: If `huggingface_hub` is not installed. + PermissionError: If the repository is private or gated and the current + Hugging Face credentials do not grant access. + FileNotFoundError: If the downloaded snapshot has no + `adapter_index.json` (wrong repo/revision, not a Granite Switch + model, or a stale cache). + ValueError: If no adapters are found (delegated from + `_embedded_adapters_from_model_directory`). + """ + try: + import huggingface_hub + from huggingface_hub.constants import HF_HUB_CACHE + from huggingface_hub.errors import GatedRepoError, RepositoryNotFoundError + except ImportError as e: + raise ImportError( + "huggingface_hub is required to download embedded adapter configs from " + 'Hugging Face Hub. Please install it with: pip install "mellea[switch]"' + ) from e + + try: + snapshot_root = pathlib.Path( + huggingface_hub.snapshot_download( + repo_id=repo_id, + allow_patterns=["adapter_index.json", "io_configs/**"], + cache_dir=cache_dir, + revision=revision, ) ) + except (GatedRepoError, RepositoryNotFoundError) as e: + auth_hint = ( + f"Could not access '{repo_id}' on Hugging Face Hub. If this is a " + "private or gated repository, authenticate first (run " + "`huggingface-cli login` or set the HF_TOKEN environment variable) " + "and confirm your account has been granted access to the repository. " + "Otherwise, the repository ID may be misspelled." + ) + raise PermissionError(auth_hint) from e + cache_root = pathlib.Path(cache_dir or HF_HUB_CACHE) + cache_key = hashlib.sha256(f"{repo_id}\0{snapshot_root.name}".encode()).hexdigest() + local_root = cache_root / "mellea" / "embedded-adapter-configs" / cache_key -T = TypeVar("T") + try: + # Validate the cache by content, not just directory existence: a + # prior run that crashed or lost a `temporary_dir.replace()` race + # (see below) can leave `local_root` as a directory missing + # `adapter_index.json`, which `is_dir()` alone would trust forever. + if not (local_root / "adapter_index.json").is_file(): + local_root.parent.mkdir(parents=True, exist_ok=True) + if local_root.is_dir(): + # Invalid leftover from a prior partial run -- clear it so + # `temporary_dir.replace(local_root)` below doesn't fail + # trying to rename onto a non-empty stale directory. + shutil.rmtree(local_root) + temporary_dir = pathlib.Path( + tempfile.mkdtemp(dir=local_root.parent, prefix=f"{cache_key}-") + ) + try: + import json as _json + + index_path = snapshot_root / "adapter_index.json" + with open(index_path, encoding="utf-8") as f: + index = _json.load(f) + shutil.copyfile(index_path, temporary_dir / "adapter_index.json") + + snapshot_cache_root = snapshot_root.parent.parent.resolve() + for entry in index.get("adapters", []): + io_config_rel = entry.get("io_config") + if io_config_rel is None: + continue + io_config_path = (snapshot_root / io_config_rel).resolve( + strict=True + ) + if not io_config_path.is_relative_to(snapshot_cache_root): + raise ValueError( + f"io_config path '{io_config_rel}' escapes " + "the downloaded Hugging Face snapshot" + ) + destination = temporary_dir / io_config_rel + destination.parent.mkdir(parents=True, exist_ok=True) + shutil.copyfile(io_config_path, destination) + adapters = _embedded_adapters_from_model_directory( + temporary_dir, intrinsic_name=intrinsic_name + ) + try: + temporary_dir.replace(local_root) + except OSError: + if not local_root.is_dir(): + raise + else: + return adapters + finally: + if temporary_dir.exists(): + shutil.rmtree(temporary_dir) -def _composed_adapter_key(adapter: "_AdapterCore") -> str: - """Return the registry key for a composed `Adapter`, mirroring `qualified_name`. + return _embedded_adapters_from_model_directory( + local_root, intrinsic_name=intrinsic_name + ) + except FileNotFoundError as e: + # snapshot_download succeeded but the index is absent: wrong + # repo/revision, a repo that isn't a Granite Switch model, or a + # stale cache. Replace the cryptic snapshot-cache path with a + # repo-scoped message that names authentication as one possible + # cause without asserting it. + raise FileNotFoundError( + f"adapter_index.json was not found in the downloaded snapshot of " + f"'{repo_id}'. Verify it is a Granite Switch model and that the " + f"revision '{revision}' is correct; if the repository is private " + "or gated, confirm you are authenticated (run " + "`huggingface-cli login` or set the HF_TOKEN environment variable)." + ) from e + except ValueError as e: + if intrinsic_name is not None: + raise ValueError( + f"No adapter found for adapter function '{intrinsic_name}' in {repo_id}" + ) from e + raise ValueError(f"No adapters found in {repo_id}") from e - 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. + +def _embedded_adapters_from_source( + source: str, + revision: str = "main", + cache_dir: str | None = None, + intrinsic_name: str | None = None, +) -> list[tuple["_AdapterCore", dict]]: + """Load composed embedded adapters from a local directory or Hugging Face Hub. + + Automatically detects whether `source` is a local filesystem path or a + Hugging Face Hub repo ID, and delegates accordingly. Args: - adapter: The composed adapter to key. + 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: - str: `"_"`. + list[tuple[_AdapterCore, dict]]: One `(adapter, io_yaml_config)` pair + per entry in the index. """ - return f"{adapter.identity.name}_{adapter.identity.adapter_type}" + if pathlib.Path(source).is_dir(): + return _embedded_adapters_from_model_directory( + source, intrinsic_name=intrinsic_name + ) + return _embedded_adapters_from_hub( + source, revision=revision, cache_dir=cache_dir, intrinsic_name=intrinsic_name + ) def _discover_embedded_adapters( @@ -332,19 +438,15 @@ def _discover_embedded_adapters( ) -> 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. + Thin keyword-only wrapper over `_embedded_adapters_from_source`, kept as + the stable name both backends import. Returns composed `Adapter` instances + paired with their raw `io.yaml` config. 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`. + a composed `Adapter` has no field for it 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 @@ -358,25 +460,9 @@ def _discover_embedded_adapters( 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 - ] + return _embedded_adapters_from_source( + source, revision=revision, cache_dir=cache_dir, intrinsic_name=intrinsic_name + ) def get_adapter_for_intrinsic( @@ -577,21 +663,6 @@ 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 @@ -766,9 +837,9 @@ def _adapter_activation_lock( 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 + `LocalHFBackend`, `_generate_composed_local_file_with_adapter_scope` holds `_generation_lock` for the whole generation, and the - `_IntrinsicPeftBinding` verbs it drives through `adapter_scope()` + `LocalFileBinding` verbs it drives through `adapter_scope()` 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 @@ -817,42 +888,6 @@ def _adapter_resolve_lock(self) -> contextlib.AbstractContextManager[bool | None """ 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. @@ -893,9 +928,8 @@ 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_resolve_lock()` closes both: a no-op by default, + # _added_adapters, which races under concurrent first-time resolves for + # the same name. `_adapter_resolve_lock()` closes it: 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 @@ -904,9 +938,7 @@ def resolve_adapter(self, name: str) -> _AdapterCore: # `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_resolve_lock(), warnings.catch_warnings(): + with self._adapter_resolve_lock(): # 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 @@ -915,71 +947,48 @@ def resolve_adapter(self, name: str) -> _AdapterCore: if found is not None: return found - warnings.simplefilter("ignore", DeprecationWarning) if getattr(self, "_uses_embedded_adapters", False): repo_id = ( getattr(self, "_adapter_source", None) or getattr(self, "_model_id", None) or base ) - # 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). + # Register composed Adapters discovered from the Granite Switch + # source. 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). + # register-then-separately-cache sequence could. for a, config in _discover_embedded_adapters( repo_id, intrinsic_name=name ): - self._add_embedded_adapter_compat(a, config) + self.add_adapter(a, config=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. 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 - ) + 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, + ), ) + ) found = self._find_adapter(name) if found is not None: @@ -987,9 +996,9 @@ def resolve_adapter(self, name: str) -> _AdapterCore: # `_find_adapter` only matches `_AdapterCore` entries. If registration # above silently failed because a `LocalFileBinding` already claims a - # colliding qualified name (they share `f"{name}_{type}"` with - # `IntrinsicAdapter`), say so — the alternative is an opaque KeyError - # that gives no hint the two registration paths collided. + # colliding qualified name (both registration paths share the + # `f"{name}_{type}"` key space), say so — the alternative is an opaque + # KeyError that gives no hint the two registration paths collided. # list(...): same concurrent-mutation hazard as `_find_adapter` — snapshot # before iterating rather than holding a live view over `_added_adapters`. added = list(getattr(self, "_added_adapters", {}).items()) @@ -1057,7 +1066,8 @@ def adapter_scope(self, adapter: "_AdapterCore | None"): # type: ignore[type-ar that work runs on the shared event-loop thread while this thread holds the lock — a same-thread `RLock` doesn't help across threads. - `LocalHFBackend._generate_intrinsic_with_adapter_scope` is the reference + `LocalHFBackend._generate_composed_local_file_with_adapter_scope` is the + reference example of a caller that *does* close the gap for its own call site: it holds `_generation_lock` around the entire scope, which is safe there only because the scope *body* is fully synchronous end to end and @@ -1215,11 +1225,9 @@ def _find_adapter( # # The snapshot also means this lookup can still see an entry that # `remove_adapter()` just popped — harmless today because a qualified - # name is held by either a `LocalFileBinding` or an `IntrinsicAdapter` - # shim (never both) and the generation path consumes only shims. - # `remove_adapter()` is public, though, so any registered entry — shim - # or binding — can be popped: re-check that invariant when #1465 moves - # generation inside `adapter_scope`. + # name is held by a single registered entry. `remove_adapter()` is + # public, though, so any registered entry 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 @@ -1250,423 +1258,3 @@ def _find_adapter( ): return a return None - - -class EmbeddedIntrinsicAdapter(_AdapterCore): - """Deprecated shim for adapter functions embedded in a Granite Switch model. - - Deprecated: - Use :class:`~mellea.backends.adapters.Adapter` directly. - `EmbeddedIntrinsicAdapter` will be removed in a future release - (Epic #929, issue #1144). - - Unlike PEFT-based adapters that are loaded into the model at runtime, - embedded adapters are already baked into the model weights and activated - via control tokens injected by the model's chat template. Only the I/O - transformation config (`io.yaml`) is needed; no adapter weights are - downloaded or loaded. - - Args: - intrinsic_name (str): Name of the adapter function (e.g. `"answerability"`). - config (dict): Parsed I/O transformation configuration (from `io.yaml`). - technology (str): Adapter technology in the switch model — `"lora"` or - `"alora"`. Determines where the control token is placed in the - chat template (beginning of sequence for LoRA, before generation - prompt for aLoRA). - - Attributes: - intrinsic_name (str): Name of the adapter function this adapter implements. - config (dict): Parsed I/O transformation configuration. - technology (str): `"lora"` or `"alora"`. - - Note: - `identity`, `io_contract`, and `weights` are internal scaffolding - populated in `__init__` to satisfy the `Adapter` protocol; they are - not meaningful consumer-facing attributes. - - - `identity`: always a real value. - - - `io_contract`: the real, declared contract for `intrinsic_name` - (issue #1516); no longer a placeholder. - - - `weights`: a real `EmbeddedBinding`; activation runs through it. - """ - - def __setattr__(self, name: str, value: object) -> None: - """Allow mutation; bypasses the frozen restriction on _AdapterCore.""" - object.__setattr__(self, name, value) - - def __delattr__(self, name: str) -> None: - """Allow deletion; bypasses the frozen restriction on _AdapterCore.""" - object.__delattr__(self, name) - - def __init__(self, intrinsic_name: str, config: dict, technology: str = "lora"): - """Initialize an embedded adapter function with its I/O config.""" - if technology not in ("lora", "alora"): - raise ValueError( - f"technology must be 'lora' or 'alora', got '{technology}'" - ) - warnings.warn( - "EmbeddedIntrinsicAdapter is deprecated; use Adapter directly (Epic #929, issue #1144).", - DeprecationWarning, - stacklevel=2, - ) - adapter_type = AdapterType.ALORA if technology == "alora" else AdapterType.LORA - - # Old-style Adapter fields — set manually since we no longer inherit from the - # legacy Adapter ABC. Preserved for backward compatibility until Phase 4. - self.name = intrinsic_name - self.adapter_type = adapter_type - self.qualified_name = intrinsic_name + "_" + adapter_type.value - self.backend: Backend | None = None - self.path: str | None = None - - self.intrinsic_name = intrinsic_name - self.config = config - self.technology = technology - capability = intrinsic_name - if intrinsic_name in known_intrinsic_names(): - capability = fetch_intrinsic_metadata(intrinsic_name).effective_capability - - # Populate the new Adapter triple so isinstance(self, _AdapterCore) holds. - # technology is validated above; cast to the Literal type mypy expects. - identity = Identity( - name=intrinsic_name, - adapter_type=cast(Literal["lora", "alora"], technology), - capability=capability, - ) - - io_contract = get_io_contract(intrinsic_name) - - weights = EmbeddedBinding() - - _AdapterCore.__init__( - self, identity=identity, io_contract=io_contract, weights=weights - ) - - @staticmethod - def from_model_directory( - model_path: str | pathlib.Path, intrinsic_name: str | None = None - ) -> list["EmbeddedIntrinsicAdapter"]: - """Load embedded adapters from a Granite Switch model directory. - - Reads `adapter_index.json` and the corresponding `io_configs/*/io.yaml` - files from the model directory. - - Args: - model_path (str | pathlib.Path): Path to a Granite Switch model - directory that contains `adapter_index.json` and `io_configs/`. - intrinsic_name (str | None): If provided, only load the adapter - matching this adapter function name. `None` loads all adapters. - - Returns: - list[EmbeddedIntrinsicAdapter]: One adapter per entry in the index. - - Raises: - FileNotFoundError: If `adapter_index.json` is missing. - ValueError: If an `io.yaml` file listed in the index cannot be found - or if no adapters are found. - """ - import json as _json - - model_path = pathlib.Path(model_path) - index_path = model_path / "adapter_index.json" - if not index_path.exists(): - raise FileNotFoundError(f"No adapter_index.json found at {index_path}") - - with open(index_path, encoding="utf-8") as f: - index = _json.load(f) - - adapters: list[EmbeddedIntrinsicAdapter] = [] - for entry in index.get("adapters", []): - entry_name = entry.get("adapter_name") - if entry_name is None: - continue - if intrinsic_name is not None and entry_name != intrinsic_name: - continue - io_config_rel = entry.get("io_config") - if io_config_rel is None: - continue - - io_config_path = model_path / io_config_rel - try: - io_config_path = io_config_path.resolve(strict=True) - except (FileNotFoundError, OSError): - raise ValueError( - f"io.yaml for adapter function '{entry_name}' " - f"not found at {model_path / io_config_rel}" - ) - if not io_config_path.is_relative_to(model_path.resolve()): - raise ValueError( - f"io_config path for adapter function '{entry_name}' " - f"escapes the model directory: {io_config_path}" - ) - - with open(io_config_path, encoding="utf-8") as f: - config_dict = yaml.safe_load(f) - - adapters.append( - EmbeddedIntrinsicAdapter( - intrinsic_name=entry_name, - config=config_dict, - technology=entry.get("technology", "lora"), - ) - ) - - if not adapters: - if intrinsic_name is not None: - raise ValueError( - f"No adapter found for adapter function '{intrinsic_name}' in {model_path}" - ) - raise ValueError(f"No adapters found in {model_path}") - - return adapters - - @staticmethod - def from_hub( - repo_id: str, - revision: str = "main", - cache_dir: str | None = None, - intrinsic_name: str | None = None, - ) -> list["EmbeddedIntrinsicAdapter"]: - """Load embedded adapters from a Granite Switch model on Hugging Face Hub. - - Downloads `adapter_index.json` and the `io_configs/` directory into a - persistent self-contained local directory, then delegates to - `from_model_directory`. - - `huggingface_hub.snapshot_download`'s default cache-backed snapshot - directory populates `io_configs/` with symlinks that resolve into a - sibling `blobs/` directory *outside* the snapshot root. That breaks the - contract `from_model_directory` expects (a self-contained model - directory) and trips its path-escape check. To satisfy that contract, - the downloaded snapshot is materialised under the Hugging Face cache - into a self-contained directory keyed by its immutable revision, so - `io_configs/` contains real files rather than symlinks escaping the - directory. This preserves standard Hugging Face Hub cache reuse and - offline loading while preventing stale files from a mutable revision. - - Args: - repo_id (str): Hugging Face Hub repository ID - (e.g. `"ibm-granite/granite-switch-micro"`). - revision (str): Git revision to download from. - cache_dir (str | None): Local cache directory; `None` for the default. - intrinsic_name (str | None): If provided, only load the adapter - matching this adapter function name. `None` loads all adapters. - - Returns: - list[EmbeddedIntrinsicAdapter]: One adapter per entry in the index. - - Raises: - ImportError: If `huggingface_hub` is not installed. - PermissionError: If the repository is private or gated and the - current Hugging Face credentials do not grant access. - FileNotFoundError: If the downloaded snapshot has no - `adapter_index.json` (wrong repo/revision, not a Granite Switch - model, or a stale cache). - ValueError: If no adapters are found (delegated from - `from_model_directory`). - """ - try: - import huggingface_hub - from huggingface_hub.constants import HF_HUB_CACHE - from huggingface_hub.errors import GatedRepoError, RepositoryNotFoundError - except ImportError as e: - raise ImportError( - "huggingface_hub is required to download embedded adapter configs from " - 'Hugging Face Hub. Please install it with: pip install "mellea[switch]"' - ) from e - - try: - snapshot_root = pathlib.Path( - huggingface_hub.snapshot_download( - repo_id=repo_id, - allow_patterns=["adapter_index.json", "io_configs/**"], - cache_dir=cache_dir, - revision=revision, - ) - ) - except (GatedRepoError, RepositoryNotFoundError) as e: - auth_hint = ( - f"Could not access '{repo_id}' on Hugging Face Hub. If this is a " - "private or gated repository, authenticate first (run " - "`huggingface-cli login` or set the HF_TOKEN environment variable) " - "and confirm your account has been granted access to the repository. " - "Otherwise, the repository ID may be misspelled." - ) - raise PermissionError(auth_hint) from e - - cache_root = pathlib.Path(cache_dir or HF_HUB_CACHE) - cache_key = hashlib.sha256( - f"{repo_id}\0{snapshot_root.name}".encode() - ).hexdigest() - local_root = cache_root / "mellea" / "embedded-adapter-configs" / cache_key - - try: - # Validate the cache by content, not just directory existence: a - # prior run that crashed or lost a `temporary_dir.replace()` race - # (see below) can leave `local_root` as a directory missing - # `adapter_index.json`, which `is_dir()` alone would trust forever. - if not (local_root / "adapter_index.json").is_file(): - local_root.parent.mkdir(parents=True, exist_ok=True) - if local_root.is_dir(): - # Invalid leftover from a prior partial run -- clear it so - # `temporary_dir.replace(local_root)` below doesn't fail - # trying to rename onto a non-empty stale directory. - shutil.rmtree(local_root) - temporary_dir = pathlib.Path( - tempfile.mkdtemp(dir=local_root.parent, prefix=f"{cache_key}-") - ) - try: - import json as _json - - index_path = snapshot_root / "adapter_index.json" - with open(index_path, encoding="utf-8") as f: - index = _json.load(f) - shutil.copyfile(index_path, temporary_dir / "adapter_index.json") - - snapshot_cache_root = snapshot_root.parent.parent.resolve() - for entry in index.get("adapters", []): - io_config_rel = entry.get("io_config") - if io_config_rel is None: - continue - io_config_path = (snapshot_root / io_config_rel).resolve( - strict=True - ) - if not io_config_path.is_relative_to(snapshot_cache_root): - raise ValueError( - f"io_config path '{io_config_rel}' escapes " - "the downloaded Hugging Face snapshot" - ) - destination = temporary_dir / io_config_rel - destination.parent.mkdir(parents=True, exist_ok=True) - shutil.copyfile(io_config_path, destination) - - adapters = EmbeddedIntrinsicAdapter.from_model_directory( - temporary_dir, intrinsic_name=intrinsic_name - ) - try: - temporary_dir.replace(local_root) - except OSError: - if not local_root.is_dir(): - raise - else: - return adapters - finally: - if temporary_dir.exists(): - shutil.rmtree(temporary_dir) - - return EmbeddedIntrinsicAdapter.from_model_directory( - local_root, intrinsic_name=intrinsic_name - ) - except FileNotFoundError as e: - # snapshot_download succeeded but the index is absent: wrong - # repo/revision, a repo that isn't a Granite Switch model, or a - # stale cache. Replace the cryptic snapshot-cache path with a - # repo-scoped message that names authentication as one possible - # cause without asserting it. - raise FileNotFoundError( - f"adapter_index.json was not found in the downloaded snapshot of " - f"'{repo_id}'. Verify it is a Granite Switch model and that the " - f"revision '{revision}' is correct; if the repository is private " - "or gated, confirm you are authenticated (run " - "`huggingface-cli login` or set the HF_TOKEN environment variable)." - ) from e - except ValueError as e: - if intrinsic_name is not None: - raise ValueError( - f"No adapter found for adapter function '{intrinsic_name}' in {repo_id}" - ) from e - raise ValueError(f"No adapters found in {repo_id}") from e - - @staticmethod - def from_source( - source: str, - revision: str = "main", - cache_dir: str | None = None, - intrinsic_name: str | None = None, - ) -> list["EmbeddedIntrinsicAdapter"]: - """Load embedded adapters from a local directory or Hugging Face Hub. - - Automatically detects whether `source` is a local filesystem path - or a Hugging Face Hub repo ID, and delegates accordingly. - - 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[EmbeddedIntrinsicAdapter]: One adapter per entry in the index. - """ - if pathlib.Path(source).is_dir(): - return EmbeddedIntrinsicAdapter.from_model_directory( - source, intrinsic_name=intrinsic_name - ) - return EmbeddedIntrinsicAdapter.from_hub( - source, - revision=revision, - cache_dir=cache_dir, - intrinsic_name=intrinsic_name, - ) - - -class CustomIntrinsicAdapter(IntrinsicAdapter): - """Deprecated shim for user-defined custom adapter functions. - - .. deprecated:: - Use :class:`~mellea.backends.adapters.Adapter` directly. - `CustomIntrinsicAdapter` will be removed in a future release - (Epic #929, issue #1144). - - This class has the same functionality as `IntrinsicAdapter`, except that - its constructor monkey-patches Mellea global variables to enable the backend - to load the user's adapter. - - Args: - model_id (str): The Hugging Face model ID used for downloading model weights; - expected format is `"/"`. - intrinsic_name (str | None): Catalog name for the adapter function; defaults to the - repository name portion of `model_id` if not provided. - base_model_name (str): The short name of the base model (NOT its repo ID). - """ - - def __init__( - self, *, model_id: str, intrinsic_name: str | None = None, base_model_name: str - ): - """Initialize CustomIntrinsicAdapter and patch the global adapter function catalog if needed.""" - warnings.warn( - "CustomIntrinsicAdapter is deprecated; use Adapter directly (Epic #929, issue #1144).", - DeprecationWarning, - stacklevel=2, - ) - assert re.match(".*/.*", model_id), ( - "expected a Hugging Face model id with format /" - ) - intrinsic_name = ( - intrinsic_name if intrinsic_name is not None else model_id.split("/")[1] - ) - - # patch the catalog. TODO this is a temporary hack until we re-org adapters. - from mellea.backends.adapters import catalog - - if intrinsic_name not in catalog._INTRINSICS_CATALOG: - catalog._INTRINSICS_CATALOG_ENTRIES.append( - catalog.IntrinsicsCatalogEntry( - name=intrinsic_name, repo_id=model_id, revision="main" - ) - ) - catalog._INTRINSICS_CATALOG = { - e.name: e for e in catalog._INTRINSICS_CATALOG_ENTRIES - } - - # Suppress DeprecationWarning from the IntrinsicAdapter shim: the warning we - # emitted above is already correctly attributed to the caller's frame. - with warnings.catch_warnings(): - warnings.simplefilter("ignore", DeprecationWarning) - IntrinsicAdapter.__init__( - self, intrinsic_name=intrinsic_name, base_model_name=base_model_name - ) diff --git a/mellea/backends/adapters/io_contracts.py b/mellea/backends/adapters/io_contracts.py index 244d86afa0..f8724c47e9 100644 --- a/mellea/backends/adapters/io_contracts.py +++ b/mellea/backends/adapters/io_contracts.py @@ -13,14 +13,14 @@ :data:`_INTRINSIC_IO_CONTRACTS` is the single source of truth instead: it is keyed by the adapter function's catalog name (:attr:`~mellea.backends.adapters.catalog.IntrinsicsCatalogEntry.name`, e.g. `"guardian-core"` — the same string passed to `call_intrinsic` and -`resolve_adapter`). The shim adapters in `adapter.py` read from it at construction -(via :func:`get_io_contract`); the high-level helpers reach it indirectly, through -the adapter `resolve_adapter` returns. Declaring a capability's contract anywhere +`resolve_adapter`). The composed adapters `resolve_adapter` builds read from it at +construction (via :func:`get_io_contract`); the high-level helpers reach it indirectly, +through the adapter `resolve_adapter` returns. Declaring a capability's contract anywhere else reintroduces the parallel-argument problem this module exists to close. An adapter a user registers with a backend takes precedence over the registry for its own contract: `call_intrinsic` parses with whatever contract the resolved adapter -carries, and only adapters Mellea constructs itself (the lazy shims) get theirs from here. +carries, and only adapters Mellea constructs itself get theirs from here. """ import json @@ -244,7 +244,7 @@ def parse(self, raw: str) -> dict[str, object]: } # Canonical output contract for every built-in adapter function, keyed by its # catalog `name` (see module docstring). Capture the built-in names before -# `CustomIntrinsicAdapter` can extend the global catalogue at runtime, then keep +# runtime catalogue registration can extend the global catalogue, then keep # the registry exhaustive over that fixed set at import time — mirroring the # duplicate-`effective_capability` check in catalog.py. _BUILTIN_INTRINSIC_NAMES = frozenset(known_intrinsic_names()) @@ -271,7 +271,7 @@ def get_io_contract(name: str) -> IOContract: Returns: IOContract: The contract declared in :data:`_INTRINSIC_IO_CONTRACTS` for `name`. Adapter functions outside the catalog (e.g. one registered - through the deprecated `CustomIntrinsicAdapter`) fall back to a dict + at runtime) fall back to a dict contract with no required keys — permissive about *which* keys are present, but still requires the parsed JSON to be an object; a top-level array or scalar still raises `ValueError`, since Mellea has diff --git a/mellea/backends/huggingface.py b/mellea/backends/huggingface.py index 8308926170..e2029d9f7d 100644 --- a/mellea/backends/huggingface.py +++ b/mellea/backends/huggingface.py @@ -21,7 +21,7 @@ from collections import OrderedDict from collections.abc import Callable, Coroutine, Sequence from importlib import metadata -from typing import Any, ClassVar, TypeVar, cast +from typing import Any, TypeVar, cast import jinja2 import jinja2.meta @@ -94,21 +94,17 @@ EmbeddedActivationRequest, EmbeddedBinding, Identity, - IntrinsicAdapter, LocalHFAdapter, ) from .adapters._core import ( Adapter as _AdapterCore, AloraActivationError, - IOContract, LocalFileBinding, - WeightsBinding, _await_embedded_generation, _fire_embedded_invocation_complete, ) from .adapters.adapter import ( AdapterInput, - EmbeddedIntrinsicAdapter, _alora_invocation_sequence_present, _composed_adapter_key, _discover_embedded_adapters, @@ -637,7 +633,6 @@ class LocalHFBackend(FormatterBackend, AdapterMixin): """ _cached_blocks: dict[str, DynamicCache] = dict() - _supports_composed_adapters = True def __init__( self, @@ -754,9 +749,7 @@ def __init__( ) # Adapters can be made known to the backend (added) and loaded. - self._added_adapters: dict[ - str, LocalHFAdapter | LocalFileBinding | EmbeddedIntrinsicAdapter - ] = {} + self._added_adapters: dict[str, LocalHFAdapter | LocalFileBinding] = {} self._loaded_adapters: dict[str, LocalHFAdapter | LocalFileBinding] = {} # Composed Adapter instances (Epic #929, issue #1144), keyed by # _composed_adapter_key(). LocalFile/PEFT composed adapters also @@ -767,9 +760,9 @@ def __init__( # 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). + # way. A composed Embedded adapter carries no config of its own, 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 @@ -779,7 +772,7 @@ def __init__( activation (`_adapter_activation_lock()` returns this same lock). Reentrant (`threading.RLock`, not `threading.Lock`) because - `_generate_intrinsic_with_adapter_scope` holds it for a whole + `_generate_composed_local_file_with_adapter_scope` holds it for a whole prepare -> activate -> generate -> deactivate critical section, and `adapter_scope()`'s `activate()`/`deactivate()` re-acquire it from inside that section, on the same thread, via `_adapter_activation_lock()`. @@ -983,8 +976,9 @@ def _generate_with_adapter_lock(self, generate_func: Callable, *args, **kwargs): Standard (non-intrinsic) generation runs without adapters: any active adapter is deactivated before the model call, and the model's active state is asserted before and after. Adapter-active generation goes - elsewhere — `_generate_intrinsic_with_adapter_scope` for runtime PEFT - adapters (routed through `adapter_scope`, with its lifecycle hooks) and + elsewhere — `_generate_composed_local_file_with_adapter_scope` for + runtime PEFT adapters (routed through `adapter_scope`, with its + lifecycle hooks) and `_generate_embedded_with_generation_lock` for Granite Switch adapters, which are activated while rendering the chat template. @@ -1027,79 +1021,6 @@ def _generate_embedded_with_generation_lock( _assert_correct_adapters("", self._model) return generate_func(*args, **kwargs) - def _generate_intrinsic_with_adapter_scope( - self, - adapter: IntrinsicAdapter, - generate_func: Callable[..., _T], - *args: Any, - **kwargs: Any, - ) -> _T: - """Runs `generate_func` with `adapter` active, inside `AdapterMixin.adapter_scope`. - - The model call now runs inside `adapter_scope`, so activation and - deactivation fire the `ADAPTER_FUNCTION_PHASE_COMPLETE`/ - `ADAPTER_FUNCTION_INVOCATION_COMPLETE` hooks (`phase="activate"` and - `"deactivate"` only — `"generate"`/`"parse"` are not emitted; adding - those is out of scope here, see #1466), and deactivation is guaranteed - even if `generate_func` raises. Those hooks dispatch while - `_generation_lock` is held below: the deadlock and stall consequences - in `adapter_scope()`'s hook-dispatch paragraph apply here in full, - since this backend's `_generation_lock` is what the dispatching thread - holds, and the `RLock` cannot rescue a subscriber that re-enters from - the event-loop thread — reentrance only helps the owning thread. - `ADAPTER_FUNCTION_*` subscribers must therefore be non-blocking and - must never re-enter this backend; a merely slow one stalls all - generation on this backend for that dispatch's duration. - - `IntrinsicAdapter` (the legacy shim `adapter` is expected to be) carries - an inert `_ShimWeightsBinding` that raises on every verb, so it can't be - handed to `adapter_scope()` directly. This method builds a fresh - `_AdapterCore` around `adapter`'s own `identity` (not a rebuilt one — - rebuilding it would re-run `Identity.__post_init__`'s capability-registry - check on every call) wrapping a `_IntrinsicPeftBinding`, which drives - this backend's own `load_peft_adapter`/`activate_peft_adapter`/ - `deactivate_peft_adapter` verbs — the same verbs - `_generate_with_adapter_lock` used to call directly. - - Holds `_generation_lock` for the *entire* prepare -> activate -> - generate -> deactivate section, not just around activation, so a - concurrent caller's `activate()` can never interleave with this call's - generation — the atomicity gap `adapter_scope()`'s own docstring flags - as unresolved for a caller that doesn't do this. Safe here because each - invocation of this method runs entirely on one thread (in production it - is only ever invoked via a single `asyncio.to_thread` call — see its - call site in `_generate_from_intrinsic`; the tests call it directly, - one invocation per thread) and never re-enters generation on - another thread: the inner re-acquisition of `_generation_lock` inside - `adapter_scope()`'s `activate()`/`deactivate()` (via - `_adapter_activation_lock()`) is therefore same-thread and reentrant. - Concurrent invocations land on different `asyncio.to_thread` pool - threads and simply serialise on the lock. - - Args: - adapter: The (legacy) `IntrinsicAdapter` to activate for this call. - 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. - """ - weights = _IntrinsicPeftBinding( - self, adapter.qualified_name, adapter.intrinsic_metadata.revision - ) - scope_adapter = _AdapterCore( - identity=adapter.identity, io_contract=_UnusedIOContract(), weights=weights - ) - with self._generation_lock: - weights.prepare() - with self.adapter_scope(scope_adapter): - _assert_correct_adapters(adapter.qualified_name, self._model) - out = generate_func(*args, **kwargs) - _assert_correct_adapters(adapter.qualified_name, self._model) - return out - def _generate_composed_local_file_with_adapter_scope( self, adapter: _AdapterCore, @@ -1109,11 +1030,10 @@ def _generate_composed_local_file_with_adapter_scope( ) -> _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. + Drives intrinsic generation for a composed `Adapter` (Epic #929, issue + #1144). `adapter.weights` here is already a real, registered + `LocalFileBinding` (see `add_adapter`), 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 @@ -1153,19 +1073,18 @@ def _generate_composed_local_file_with_adapter_scope( return out def _intrinsic_adapter_name_and_config( - self, adapter: IntrinsicAdapter | EmbeddedIntrinsicAdapter | _AdapterCore + self, adapter: _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 + A composed `Adapter` carries neither directly — `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 @@ -1185,8 +1104,6 @@ def _intrinsic_adapter_name_and_config( 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) @@ -1203,7 +1120,7 @@ def _intrinsic_adapter_name_and_config( ) def _obtain_local_file_io_yaml_config(self, binding: LocalFileBinding) -> dict: - """Download and parse `binding`'s io.yaml, mirroring `IntrinsicAdapter.__init__`. + """Download and parse `binding`'s io.yaml. Called synchronously from `add_adapter`'s composed-`LocalFileBinding` branch, before `binding.prepare()`'s (larger) weights download runs @@ -1275,11 +1192,8 @@ async def _generate_from_intrinsic( 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. + TypeError: If the adapter is not a composed `Adapter`, or if a + composed `Adapter`'s `weights` is an unsupported binding type. AloraActivationError: If the resolved adapter is an aLoRA whose declared activation sequence is absent from the assembled prompt. @@ -1344,12 +1258,10 @@ 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, _AdapterCore) - ): + if not isinstance(adapter, _AdapterCore): raise TypeError( - "LocalHFBackend only supports IntrinsicAdapter, EmbeddedIntrinsicAdapter, " - f"or a composed Adapter, got: {type(adapter).__name__}" + "LocalHFBackend only supports a composed Adapter, got: " + f"{type(adapter).__name__}" ) adapter_name, intrinsic_config = self._intrinsic_adapter_name_and_config( @@ -1388,17 +1300,6 @@ async def _generate_from_intrinsic( # us having specific caching for each Component/Message. rewritten_request = rewritten.model_dump() - # 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", {}) @@ -1572,17 +1473,6 @@ 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, @@ -1610,8 +1500,8 @@ async def granite_formatters_processing( input_ids, embedded_identity: Identity | None, ): - # embedded_identity is set only for EmbeddedIntrinsicAdapter calls - # (the legacy PEFT path already gets this signal via adapter_scope). + # embedded_identity is set only for embedded-adapter calls + # (the LocalFile/PEFT path already gets this signal via adapter_scope). # Kept in one try so any failure below — not just from transform # itself — fires outcome="error" instead of reporting success and # then raising, the exact bug #1559 reverted. @@ -3044,8 +2934,8 @@ def add_adapter(self, adapter: AdapterInput, *, config: dict | None = None) -> N Args: adapter (AdapterInput): The adapter to register. Must be a - `LocalHFAdapter`, `LocalFileBinding`, `EmbeddedIntrinsicAdapter`, - or a composed `Adapter`; other adapter realities are rejected. + `LocalHFAdapter`, `LocalFileBinding`, 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 @@ -3065,23 +2955,17 @@ def add_adapter(self, adapter: AdapterInput, *, config: dict | None = None) -> N its cause. Exception: If `adapter` has already been added to a different backend. """ - if not isinstance( - adapter, - (LocalHFAdapter, LocalFileBinding, EmbeddedIntrinsicAdapter, _AdapterCore), - ): + if not isinstance(adapter, (LocalHFAdapter, LocalFileBinding, _AdapterCore)): raise TypeError( "LocalHFBackend requires a LocalHFAdapter, LocalFileBinding, " - "EmbeddedIntrinsicAdapter, or a composed Adapter; got " + "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) - ): + # A composed Adapter: dispatch on its weights binding instead of the + # legacy LocalHFAdapter branches below, which mutate attributes a + # composed Adapter's frozen dataclass doesn't have. + if isinstance(adapter, _AdapterCore): # 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. @@ -3238,22 +3122,11 @@ def add_adapter(self, adapter: AdapterInput, *, config: dict | None = None) -> N f"but {adapter.qualified_name!r} is already registered as a " f"{type(existing).__name__} on {self.__class__.__name__}. The backend is " "refusing to do this, because adapter loading is not idempotent. " - "LocalFileBinding and IntrinsicAdapter/resolve_adapter() registrations " + "LocalFileBinding and resolve_adapter() registrations " "share this qualified-name key space and cannot both claim the same name." ) return None - if isinstance(adapter, EmbeddedIntrinsicAdapter): - if not isinstance(adapter.weights, EmbeddedBinding): - raise TypeError( - "EmbeddedIntrinsicAdapter.weights must be an EmbeddedBinding; " - f"got {type(adapter.weights).__name__}." - ) - adapter.backend = self - adapter.weights.source = self.base_model_name - self._added_adapters[adapter.qualified_name] = adapter - return - adapter.path = adapter.get_local_hf_path(self.base_model_name) adapter.backend = self self._added_adapters[adapter.qualified_name] = adapter @@ -3288,14 +3161,12 @@ def register_embedded_adapter_model( """ # 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 + # to any other thread): this method registers embedded adapters on a + # live backend, so a caller here can race another thread's + # `add_adapter`/`resolve_adapter` — `add_adapter`'s read-then-write + # across `_composed_adapters` needs 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 @@ -3360,11 +3231,6 @@ def load_peft_adapter(self, adapter_qualified_name: str) -> None: raise ValueError( f"could not load adapter {adapter_qualified_name} for backend {self}: adapter was not previously added" ) - if isinstance(adapter, EmbeddedIntrinsicAdapter): - raise TypeError( - f"cannot load embedded adapter {adapter_qualified_name} through PEFT; " - "it is activated by the chat template" - ) try: # self._model is a HF PreTrainedModel — .load_adapter() here is @@ -3544,7 +3410,7 @@ def _adapter_activation_lock( """Reuse `_generation_lock` for exclusivity around adapter activation. `activate_peft_adapter`/`deactivate_peft_adapter` document "must be called - while holding `_generation_lock`" as a precondition, and they have three + while holding `_generation_lock`" as a precondition, and they have two callers: 1. `_generate_with_adapter_lock` (the standard, non-intrinsic @@ -3553,16 +3419,14 @@ def _adapter_activation_lock( because standard generation always runs against the base model. 2. `LocalFileBinding.activate()`/`.deactivate()` (driven by `AdapterMixin.adapter_scope()`), which holds no lock of its own. - 3. `_IntrinsicPeftBinding.activate()`/`.deactivate()` (driven by - `_generate_intrinsic_with_adapter_scope` via - `AdapterMixin.adapter_scope()`), the intrinsic-path counterpart of - caller 2's `LocalFileBinding` — whose driver also holds - `_generation_lock` itself for the whole prepare -> activate -> - generate -> deactivate section (#1465). + This is also the intrinsic-generation path: intrinsic generation + for a composed `Adapter` drives `adapter_scope()` on its + `LocalFileBinding` while holding `_generation_lock` itself for the + whole prepare -> activate -> generate -> deactivate section (#1465). - This method exists for callers 2 and 3 — so it is not a duplicate of - 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. + This method exists for caller 2 — so it is not a duplicate of the lock + caller 1 takes, it is the only thing satisfying the precondition on + that path. A fourth consumer, unrelated to that precondition: `add_adapter()`'s composed-`Adapter` branches also take this lock, briefly, around @@ -3572,7 +3436,7 @@ def _adapter_activation_lock( Caller 3's verb acquisitions therefore nest inside its driver's `_generation_lock` hold on the same thread (see - `_generate_intrinsic_with_adapter_scope`'s docstring), which is + `_generate_composed_local_file_with_adapter_scope`'s docstring), which is exactly why `_generation_lock` is a `threading.RLock`: a plain `threading.Lock` would deadlock on that same-thread re-acquisition (this was #1465's known lock-reentrancy issue). @@ -3600,7 +3464,7 @@ def list_adapters(self) -> list[str]: Returns: 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 + been loaded via `load_peft_adapter`. For a bare `LocalFileBinding`, this is `adapter.qualified_name`; for a composed `Adapter`, it's the equivalent `_composed_adapter_key()` format (the two formats agree). @@ -3641,84 +3505,3 @@ def _assert_correct_adapters(expected_state: str, model: PreTrainedModel): ) else: raise e - - -class _UnusedIOContract(IOContract): - """Placeholder `IOContract` for `_generate_intrinsic_with_adapter_scope`'s `_AdapterCore`. - - `_generate_from_intrinsic` builds its own I/O handling directly (the - rewriter/result-processor pipeline driven by `adapter.config`) and only - needs `adapter_scope()` for its activate/deactivate lifecycle hooks — - `adapter_scope()` never reads `Adapter.io_contract`, so this slot is filled - only to satisfy the dataclass's required field. - """ - - def build_prompt(self, **kwargs: object) -> Component: - raise NotImplementedError( - "not used on the intrinsic generation path: " - "_generate_from_intrinsic builds prompts via IntrinsicsRewriter." - ) - - def parse(self, raw: str) -> dict[str, object]: - raise NotImplementedError( - "not used on the intrinsic generation path: " - "_generate_from_intrinsic parses output via IntrinsicsResultProcessor." - ) - - -class _IntrinsicPeftBinding(WeightsBinding): - """Drives already-registered PEFT verbs for one intrinsic-generation call. - - `IntrinsicAdapter` (the legacy shim, see `mellea.backends.adapters.adapter`) - carries an inert `_ShimWeightsBinding` that raises on every verb, so it - can't be handed to `adapter_scope()` directly. - `_generate_intrinsic_with_adapter_scope` builds one of these fresh per call - instead, and it drives the backend's own `load_peft_adapter`/ - `activate_peft_adapter`/`deactivate_peft_adapter` verbs — the same verbs - `_generate_with_adapter_lock` used to call directly before generation was - routed through `adapter_scope`. - - `binding_type` is `"local_file"`, not a distinct value: this binding loads - LoRA/aLoRA weights downloaded to local disk via PEFT — the same reality - `LocalFileBinding` reports — so it must share that value rather than - fragment the weights-reality metric dimension. A distinct class exists - rather than reusing `LocalFileBinding` directly because `LocalFileBinding.prepare()` - calls `add_adapter()`, which would collide with the `IntrinsicAdapter` - already registered under this `qualified_name`. - - `prepare()` loads the weights (mirrors `LocalFileBinding.prepare()`'s - phase boundary: the load belongs to `"prepare"`, not `"activate"`, so - `phase="activate"` duration stays comparable across bindings). - `release()` is a no-op: registration happened in `add_adapter()` when the - intrinsic adapter was resolved (see `AdapterMixin.resolve_adapter`), and - this binding doesn't own that lifecycle — it only exists for the duration - of one generate call. `revision` — the value `adapter_scope()` reports to - the `ADAPTER_FUNCTION_INVOCATION_COMPLETE` hook — is set explicitly rather - than left for `adapter_scope()`'s `getattr(..., None)` fallback, because - `IntrinsicAdapter`s are always pinned to a catalogue SHA; reporting - `None` would mislabel every intrinsic invocation as unpinned. - """ - - binding_type: ClassVar[str] = "local_file" - - def __init__( - self, backend: LocalHFBackend, qualified_name: str, revision: str | None - ) -> None: - self._backend = backend - self._qualified_name = qualified_name - self.revision = revision - - def prepare(self) -> None: - with self._backend._adapter_activation_lock(): - self._backend.load_peft_adapter(self._qualified_name) - - def activate(self) -> None: - with self._backend._adapter_activation_lock(): - self._backend.activate_peft_adapter(self._qualified_name) - - def deactivate(self) -> None: - with self._backend._adapter_activation_lock(): - self._backend.deactivate_peft_adapter(self._qualified_name) - - def release(self) -> None: - return diff --git a/mellea/backends/ollama.py b/mellea/backends/ollama.py index af289b246f..3a6dca0529 100644 --- a/mellea/backends/ollama.py +++ b/mellea/backends/ollama.py @@ -237,8 +237,6 @@ class OllamaModelBackend(FormatterBackend, AdapterMixin): OSError: If the model cannot be pulled from the Ollama library. """ - _supports_composed_adapters = True - def __init__( self, model_id: str | ModelIdentifier = model_ids.IBM_GRANITE_4_2_3B, diff --git a/mellea/backends/openai.py b/mellea/backends/openai.py index 0935ad32d2..2289333ea1 100644 --- a/mellea/backends/openai.py +++ b/mellea/backends/openai.py @@ -66,7 +66,6 @@ from .adapters.adapter import ( AdapterInput, AdapterMixin, - EmbeddedIntrinsicAdapter, _composed_adapter_key, _discover_embedded_adapters, ) @@ -132,8 +131,6 @@ 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, @@ -305,12 +302,10 @@ def __init__( 2, aclose=lambda client: client.close() ) - # 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 + # `.config` field, so its io.yaml is cached here; see # _intrinsic_adapter_name_and_config. self._composed_adapter_configs: dict[str, dict] = {} self._adapter_lock = threading.RLock() @@ -352,46 +347,25 @@ def add_adapter(self, adapter: AdapterInput, *, config: dict | None = None) -> N """Register an adapter with this backend. Accepts the full `AdapterInput` union to honour the mixin contract, but - currently only `EmbeddedIntrinsicAdapter` (the Embedded/Granite Switch - reality) is supported; other realities are rejected at runtime. As a - side effect, an `EmbeddedBinding` weights handler is stamped with this - backend's `base_model_name` in its `source` field. Refuses a name - already registered instead of silently overwriting it. + currently only a composed `Adapter` whose `weights` is an + `EmbeddedBinding` (the Embedded/Granite Switch reality) is supported; + other realities are rejected at runtime. As a side effect, the + `EmbeddedBinding` weights handler is stamped with this backend's + `base_model_name` in its `source` field. Refuses a name already + registered instead of silently overwriting it. Args: - adapter (AdapterInput): The adapter to register. Must be an - `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`. + adapter (AdapterInput): The adapter to register. Must be a composed + `Adapter` whose `weights` is an `EmbeddedBinding`. + config (dict | None): Raw io.yaml config for the composed `Adapter`. + Required (its config cannot be cheaply re-derived later). Raises: - 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. + TypeError: If `adapter` is not a supported Embedded adapter. + ValueError: If `config` is not given — registering the adapter + without one would make it discoverable but permanently unable + to generate. """ - 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 - if isinstance(adapter, _AdapterCore): if not isinstance(adapter.weights, EmbeddedBinding): raise TypeError( @@ -431,8 +405,9 @@ def add_adapter(self, adapter: AdapterInput, *, config: dict | None = None) -> N return raise TypeError( - "OpenAIBackend currently only supports EmbeddedIntrinsicAdapter or a " - f"composed Adapter. Got: {type(adapter).__name__}" + "OpenAIBackend currently only supports a composed Adapter whose " + "weights is an EmbeddedBinding. " + f"Got: {type(adapter).__name__}" ) def list_adapters(self) -> list[str]: @@ -463,10 +438,6 @@ def remove_adapter(self, adapter_qualified_name: str) -> None: f"could not remove adapter {adapter_qualified_name} for backend " f"{self}: adapter was not registered" ) - return - - if isinstance(adapter, EmbeddedIntrinsicAdapter): - adapter.backend = None def _adapter_activation_lock( self, @@ -489,13 +460,12 @@ def _adapter_resolve_lock(self) -> contextlib.AbstractContextManager[bool | None # ------------------------------------------------------------------ def _intrinsic_adapter_name_and_config( - self, adapter: "EmbeddedIntrinsicAdapter | _AdapterCore" + self, adapter: "_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, + A composed `Adapter` carries neither directly — `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 @@ -512,8 +482,6 @@ def _intrinsic_adapter_name_and_config( 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: @@ -554,8 +522,9 @@ def register_embedded_adapter_model( # 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 + # post-construction way to load embedded adapters from a Hub repo or + # model directory 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 @@ -1040,10 +1009,8 @@ async def _generate_from_intrinsic( ValueError: If no embedded adapter is registered for the requested 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). + TypeError: If the adapter is not a composed `Adapter`, or its + `weights` isn't an `EmbeddedBinding`. """ if not ctx.is_chat_context: raise NotImplementedError("Intrinsics require a chat context.") @@ -1065,12 +1032,10 @@ async def _generate_from_intrinsic( # TODO: OpenAIBackend only supports EmbeddedAdapters. # It should be refactored into a specific adapter.transform() function. - # 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( - "OpenAIBackend only supports EmbeddedIntrinsicAdapter or a composed " - f"Adapter, got: {type(adapter).__name__}" + "OpenAIBackend only supports a composed Adapter, " + f"got: {type(adapter).__name__}" ) adapter_name, intrinsic_config = self._intrinsic_adapter_name_and_config( @@ -1189,14 +1154,13 @@ async def _generate_from_intrinsic( # Embedded adapters activate via control tokens in the chat template; # the binding owns the final request edit so callers cannot override - # the adapter selected for this intrinsic. `adapter.weights` is always - # an EmbeddedBinding here — EmbeddedIntrinsicAdapter.__init__ - # constructs one unconditionally — but the shim permits attribute - # mutation, so a caller reassigning `.weights` must fail loudly here - # rather than silently skip activation and send an unactivated request. + # the adapter selected for this intrinsic. A composed embedded Adapter + # always carries an EmbeddedBinding, but guard against a mismatched + # `.weights` (e.g. a wrongly-constructed adapter) so activation fails + # loudly here rather than silently sending an unactivated request. if not isinstance(adapter.weights, EmbeddedBinding): raise TypeError( - f"EmbeddedIntrinsicAdapter.weights must be an EmbeddedBinding; " + f"embedded adapter weights must be an EmbeddedBinding; " f"got {type(adapter.weights).__name__}. Activation cannot proceed." ) activation_request = EmbeddedActivationRequest( diff --git a/mellea/stdlib/components/intrinsic/_util.py b/mellea/stdlib/components/intrinsic/_util.py index 4f65c4c54e..d6f743bc7c 100644 --- a/mellea/stdlib/components/intrinsic/_util.py +++ b/mellea/stdlib/components/intrinsic/_util.py @@ -224,7 +224,7 @@ def call_intrinsic( # Adapter activation is the backend's responsibility — the HF backend acquires # its generation lock and activates the adapter inside adapter_scope(), driven - # from _generate_intrinsic_with_adapter_scope immediately before generation. + # from _generate_composed_local_file_with_adapter_scope immediately before generation. # Activating here (outside that lock) would race with concurrent async requests. intrinsic = Intrinsic( intrinsic_name, diff --git a/mellea/stdlib/components/intrinsic/intrinsic.py b/mellea/stdlib/components/intrinsic/intrinsic.py index f52fad1927..d21f3fb7e0 100644 --- a/mellea/stdlib/components/intrinsic/intrinsic.py +++ b/mellea/stdlib/components/intrinsic/intrinsic.py @@ -27,8 +27,8 @@ class Intrinsic(Component[str]): 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). + catalog entry, rather than through a 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 diff --git a/test/backends/test_adapters/test_adapter.py b/test/backends/test_adapters/test_adapter.py deleted file mode 100644 index 142ea4fdd4..0000000000 --- a/test/backends/test_adapters/test_adapter.py +++ /dev/null @@ -1,62 +0,0 @@ -# Copyright IBM Corp. All Rights Reserved. -# SPDX-License-Identifier: Apache-2.0 - -import pathlib -from unittest.mock import patch - -import pytest - -from mellea.backends.adapters import IntrinsicAdapter -from mellea.backends.adapters.catalog import fetch_intrinsic_metadata - - -# The backend tests handle most of the adapter testing. Do a basic test here -# to make sure init and config loading work. -def test_adapter_init(): - dir_file = pathlib.Path(__file__).parent.joinpath("intrinsics-data") - answerability_file = f"{dir_file}/answerability.yaml" - - adapter = IntrinsicAdapter("answerability", config_file=answerability_file) - - assert adapter.config is not None - assert adapter.config["parameters"]["max_completion_tokens"] == 6 - - -def test_init_forwards_pinned_revision_to_obtain_io_yaml(): - """Regression guard (issue #1141): when no config_file/config_dict is given, - __init__ must forward the catalog's pinned revision to obtain_io_yaml, not the - default `"main"`. - """ - pinned_revision = fetch_intrinsic_metadata("answerability").revision - - with patch( - "mellea.formatters.granite.intrinsics.obtain_io_yaml" - ) as mock_obtain_io_yaml: - mock_obtain_io_yaml.return_value = ( - pathlib.Path(__file__).parent / "intrinsics-data" / "answerability.yaml" - ) - IntrinsicAdapter("answerability", base_model_name="granite-3.3-8b-instruct") - - assert mock_obtain_io_yaml.call_args.kwargs["revision"] == pinned_revision - assert pinned_revision != "main" - - -def test_download_and_get_path_forwards_pinned_revision_to_obtain_lora(): - """Regression guard (issue #1141): download_and_get_path must forward the - catalog's pinned revision to obtain_lora, not the default `"main"`. - """ - dir_file = pathlib.Path(__file__).parent.joinpath("intrinsics-data") - answerability_file = f"{dir_file}/answerability.yaml" - adapter = IntrinsicAdapter("answerability", config_file=answerability_file) - pinned_revision = adapter.intrinsic_metadata.revision - - with patch("mellea.formatters.granite.intrinsics.obtain_lora") as mock_obtain_lora: - mock_obtain_lora.return_value = pathlib.Path("/fake/adapter/path") - adapter.download_and_get_path("granite-3.3-8b-instruct") - - assert mock_obtain_lora.call_args.kwargs["revision"] == pinned_revision - assert pinned_revision != "main" - - -if __name__ == "__main__": - pytest.main([__file__]) diff --git a/test/backends/test_adapters/test_embedded_adapter.py b/test/backends/test_adapters/test_embedded_adapter.py index d84e9de007..ec2d4e6434 100644 --- a/test/backends/test_adapters/test_embedded_adapter.py +++ b/test/backends/test_adapters/test_embedded_adapter.py @@ -1,7 +1,13 @@ # Copyright IBM Corp. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 -"""Tests for EmbeddedIntrinsicAdapter and OpenAI backend integration.""" +"""Tests for embedded-adapter discovery helpers and OpenAI backend integration. + +The `_embedded_adapters_from_*` module-level helpers replace the deprecated +`EmbeddedIntrinsicAdapter.from_*` static methods (Epic #929, issue #1144): they +read a Granite Switch model directory (or Hub snapshot) and return composed +`(Adapter, io_yaml_config)` pairs directly, with no shim intermediary. +""" import json import os @@ -12,7 +18,12 @@ import pytest import yaml -from mellea.backends.adapters.adapter import EmbeddedIntrinsicAdapter +from mellea.backends.adapters._core import Adapter, EmbeddedBinding +from mellea.backends.adapters.adapter import ( + _embedded_adapters_from_hub, + _embedded_adapters_from_model_directory, + _embedded_adapters_from_source, +) from mellea.backends.adapters.catalog import AdapterType _TEST_DIR = pathlib.Path(__file__).parent @@ -88,81 +99,44 @@ def hub_cache_dir(tmp_path, monkeypatch): return cache_dir -# ---- EmbeddedIntrinsicAdapter.__init__ ---- - +def _by_name(pairs: list[tuple[Adapter, dict]]) -> dict[str, tuple[Adapter, dict]]: + """Index discovered `(Adapter, config)` pairs by capability name.""" + return {adapter.identity.name: (adapter, config) for adapter, config in pairs} -class TestEmbeddedIntrinsicAdapterInit: - def test_alora_technology(self): - adapter = EmbeddedIntrinsicAdapter( - intrinsic_name="answerability", - config=_ANSWERABILITY_CONFIG, - technology="alora", - ) - assert adapter.intrinsic_name == "answerability" - assert adapter.name == "answerability" - assert adapter.technology == "alora" - assert adapter.adapter_type == AdapterType.ALORA - assert adapter.qualified_name == "answerability_alora" - assert adapter.config is _ANSWERABILITY_CONFIG - - def test_lora_technology(self): - adapter = EmbeddedIntrinsicAdapter( - intrinsic_name="citations", config=_CITATIONS_CONFIG, technology="lora" - ) - assert adapter.adapter_type == AdapterType.LORA - assert adapter.qualified_name == "citations_lora" - def test_default_technology_is_lora(self): - adapter = EmbeddedIntrinsicAdapter( - intrinsic_name="test", config={"model": None} - ) - assert adapter.technology == "lora" - assert adapter.adapter_type == AdapterType.LORA - - def test_invalid_technology_raises(self): - with pytest.raises(ValueError, match="must be 'lora' or 'alora'"): - EmbeddedIntrinsicAdapter( - intrinsic_name="test", config={"model": None}, technology="qlora" - ) - - def test_inherited_adapter_defaults(self): - adapter = EmbeddedIntrinsicAdapter( - intrinsic_name="test", config={"model": None} - ) - assert adapter.backend is None - assert adapter.path is None - - -# ---- EmbeddedIntrinsicAdapter.from_model_directory ---- +# ---- _embedded_adapters_from_model_directory ---- class TestFromModelDirectory: def test_loads_all_adapters(self, model_dir): - adapters = EmbeddedIntrinsicAdapter.from_model_directory(model_dir) + pairs = _embedded_adapters_from_model_directory(model_dir) - assert len(adapters) == 2 - names = {a.intrinsic_name for a in adapters} - assert names == {"answerability", "citations"} + assert len(pairs) == 2 + by_name = _by_name(pairs) + assert set(by_name) == {"answerability", "citations"} - ans = next(a for a in adapters if a.intrinsic_name == "answerability") - assert ans.technology == "alora" - assert ans.config["parameters"]["max_completion_tokens"] == 6 + # Discovered adapters are composed Adapters carrying an EmbeddedBinding. + ans, ans_config = by_name["answerability"] + assert isinstance(ans, Adapter) + assert isinstance(ans.weights, EmbeddedBinding) + assert ans.identity.adapter_type == "alora" + assert ans_config["parameters"]["max_completion_tokens"] == 6 - cit = next(a for a in adapters if a.intrinsic_name == "citations") - assert cit.technology == "lora" + cit, _ = by_name["citations"] + assert cit.identity.adapter_type == "lora" def test_accepts_string_path(self, model_dir): - adapters = EmbeddedIntrinsicAdapter.from_model_directory(str(model_dir)) - assert len(adapters) == 2 + pairs = _embedded_adapters_from_model_directory(str(model_dir)) + assert len(pairs) == 2 def test_missing_adapter_index(self, tmp_path): with pytest.raises(FileNotFoundError, match=r"adapter_index\.json"): - EmbeddedIntrinsicAdapter.from_model_directory(tmp_path) + _embedded_adapters_from_model_directory(tmp_path) def test_missing_io_yaml(self, tmp_path): (tmp_path / "adapter_index.json").write_text(json.dumps(_SAMPLE_ADAPTER_INDEX)) with pytest.raises(ValueError, match=r"io\.yaml.*not found"): - EmbeddedIntrinsicAdapter.from_model_directory(tmp_path) + _embedded_adapters_from_model_directory(tmp_path) def test_skips_entry_without_io_config(self, tmp_path): """Entries with io_config=None are silently skipped.""" @@ -181,9 +155,9 @@ def test_skips_entry_without_io_config(self, tmp_path): cfg_dir.mkdir(parents=True) (cfg_dir / "io.yaml").write_text(yaml.dump({"model": None})) - adapters = EmbeddedIntrinsicAdapter.from_model_directory(tmp_path) - assert len(adapters) == 1 - assert adapters[0].intrinsic_name == "has_config" + pairs = _embedded_adapters_from_model_directory(tmp_path) + assert len(pairs) == 1 + assert pairs[0][0].identity.name == "has_config" def test_defaults_technology_to_lora(self, tmp_path): """Entries without a 'technology' key default to lora.""" @@ -201,36 +175,55 @@ def test_defaults_technology_to_lora(self, tmp_path): cfg_dir.mkdir(parents=True) (cfg_dir / "io.yaml").write_text(yaml.dump({"model": None})) - adapters = EmbeddedIntrinsicAdapter.from_model_directory(tmp_path) - assert len(adapters) == 1 - assert adapters[0].technology == "lora" + pairs = _embedded_adapters_from_model_directory(tmp_path) + assert len(pairs) == 1 + assert pairs[0][0].identity.adapter_type == "lora" def test_empty_adapters_list(self, tmp_path): (tmp_path / "adapter_index.json").write_text(json.dumps({"adapters": []})) with pytest.raises(ValueError, match="No adapters found"): - EmbeddedIntrinsicAdapter.from_model_directory(tmp_path) + _embedded_adapters_from_model_directory(tmp_path) def test_no_adapters_key(self, tmp_path): """Index with no 'adapters' key raises ValueError.""" (tmp_path / "adapter_index.json").write_text(json.dumps({})) with pytest.raises(ValueError, match="No adapters found"): - EmbeddedIntrinsicAdapter.from_model_directory(tmp_path) + _embedded_adapters_from_model_directory(tmp_path) def test_filter_single_intrinsic(self, model_dir): - adapters = EmbeddedIntrinsicAdapter.from_model_directory( + pairs = _embedded_adapters_from_model_directory( model_dir, intrinsic_name="answerability" ) - assert len(adapters) == 1 - assert adapters[0].intrinsic_name == "answerability" + assert len(pairs) == 1 + assert pairs[0][0].identity.name == "answerability" def test_filter_nonexistent_intrinsic(self, model_dir): with pytest.raises( ValueError, match="No adapter found for adapter function 'nonexistent'" ): - EmbeddedIntrinsicAdapter.from_model_directory( + _embedded_adapters_from_model_directory( model_dir, intrinsic_name="nonexistent" ) + def test_invalid_technology_raises(self, tmp_path): + """An unsupported technology in the index is rejected.""" + index = { + "adapters": [ + { + "adapter_name": "test", + "technology": "qlora", + "io_config": "io_configs/test/io.yaml", + } + ] + } + (tmp_path / "adapter_index.json").write_text(json.dumps(index)) + cfg_dir = tmp_path / "io_configs" / "test" + cfg_dir.mkdir(parents=True) + (cfg_dir / "io.yaml").write_text(yaml.dump({"model": None})) + + with pytest.raises(ValueError, match="must be 'lora' or 'alora'"): + _embedded_adapters_from_model_directory(tmp_path) + def test_path_traversal_in_io_config_raises(self, tmp_path): """io_config paths with ../ traversal that escape model_path are rejected.""" outside = tmp_path / "outside.yaml" @@ -250,7 +243,7 @@ def test_path_traversal_in_io_config_raises(self, tmp_path): (model_dir / "adapter_index.json").write_text(json.dumps(index)) with pytest.raises(ValueError, match="escapes the model directory"): - EmbeddedIntrinsicAdapter.from_model_directory(model_dir) + _embedded_adapters_from_model_directory(model_dir) def test_symlink_escape_in_io_config_raises(self, tmp_path): """io_config paths that resolve via symlink outside model_path are rejected.""" @@ -275,7 +268,7 @@ def test_symlink_escape_in_io_config_raises(self, tmp_path): (model_dir / "adapter_index.json").write_text(json.dumps(index)) with pytest.raises(ValueError, match="escapes the model directory"): - EmbeddedIntrinsicAdapter.from_model_directory(model_dir) + _embedded_adapters_from_model_directory(model_dir) def test_adapter_name_key(self, tmp_path): """Index with 'adapter_name' key is read correctly.""" @@ -293,12 +286,12 @@ def test_adapter_name_key(self, tmp_path): cfg_dir.mkdir(parents=True) (cfg_dir / "io.yaml").write_text(yaml.dump(_ANSWERABILITY_CONFIG)) - adapters = EmbeddedIntrinsicAdapter.from_model_directory(tmp_path) - assert len(adapters) == 1 - assert adapters[0].intrinsic_name == "answerability" + pairs = _embedded_adapters_from_model_directory(tmp_path) + assert len(pairs) == 1 + assert pairs[0][0].identity.name == "answerability" -# ---- EmbeddedIntrinsicAdapter.from_hub ---- +# ---- _embedded_adapters_from_hub ---- class TestFromHub: @@ -308,7 +301,7 @@ def test_downloads_and_delegates(self, model_dir): with patch( "huggingface_hub.snapshot_download", return_value=str(model_dir) ) as mock_dl: - adapters = EmbeddedIntrinsicAdapter.from_hub( + pairs = _embedded_adapters_from_hub( "ibm-granite/granite-switch-micro", revision="test-rev", cache_dir=str(cache_dir), @@ -320,14 +313,14 @@ def test_downloads_and_delegates(self, model_dir): cache_dir=str(cache_dir), revision="test-rev", ) - assert len(adapters) == 2 + assert len(pairs) == 2 def test_filter_single_intrinsic(self, model_dir): cache_dir = model_dir.parent / "cache" with patch( "huggingface_hub.snapshot_download", return_value=str(model_dir) ) as mock_dl: - adapters = EmbeddedIntrinsicAdapter.from_hub( + pairs = _embedded_adapters_from_hub( "ibm-granite/granite-switch-micro", cache_dir=str(cache_dir), intrinsic_name="citations", @@ -339,8 +332,8 @@ def test_filter_single_intrinsic(self, model_dir): cache_dir=str(cache_dir), revision="main", ) - assert len(adapters) == 1 - assert adapters[0].intrinsic_name == "citations" + assert len(pairs) == 1 + assert pairs[0][0].identity.name == "citations" def test_from_hub_materialises_hub_snapshot(self, model_dir, tmp_path): """from_hub materialises Hub blob symlinks into its persistent local directory.""" @@ -368,11 +361,11 @@ def snapshot_download(**_): return str(snapshot_dir) with patch("huggingface_hub.snapshot_download", side_effect=snapshot_download): - adapters = EmbeddedIntrinsicAdapter.from_hub( + pairs = _embedded_adapters_from_hub( "ibm-granite/granite-switch-micro", cache_dir=str(tmp_path / "cache") ) - assert {adapter.intrinsic_name for adapter in adapters} == { + assert {adapter.identity.name for adapter, _ in pairs} == { "answerability", "citations", } @@ -403,13 +396,13 @@ def make_snapshot(name): "huggingface_hub.snapshot_download", side_effect=[str(main_snapshot), str(main_snapshot), str(v2_snapshot)], ) as mock_dl: - EmbeddedIntrinsicAdapter.from_hub( + _embedded_adapters_from_hub( "ibm-granite/granite-switch-micro", cache_dir=str(cache_dir) ) - EmbeddedIntrinsicAdapter.from_hub( + _embedded_adapters_from_hub( "ibm-granite/granite-switch-micro", cache_dir=str(cache_dir) ) - EmbeddedIntrinsicAdapter.from_hub( + _embedded_adapters_from_hub( "ibm-granite/granite-switch-micro", cache_dir=str(cache_dir), revision="v2", @@ -424,7 +417,7 @@ def make_snapshot(name): def test_missing_huggingface_hub_raises(self): with patch.dict("sys.modules", {"huggingface_hub": None}): with pytest.raises(ImportError, match="huggingface_hub is required"): - EmbeddedIntrinsicAdapter.from_hub("some/repo") + _embedded_adapters_from_hub("some/repo") @pytest.mark.parametrize( "error_name", ["GatedRepoError", "RepositoryNotFoundError"] @@ -447,7 +440,7 @@ class _FakeResponse: with patch("huggingface_hub.snapshot_download", side_effect=hf_error): with pytest.raises(PermissionError, match="huggingface-cli login") as exc: - EmbeddedIntrinsicAdapter.from_hub("ibm-granite/private-switch") + _embedded_adapters_from_hub("ibm-granite/private-switch") # Original HF error is chained for debugging. assert exc.value.__cause__ is hf_error @@ -466,7 +459,7 @@ def test_missing_index_after_download_raises_clear_file_not_found( with pytest.raises( FileNotFoundError, match=r"ibm-granite/some-switch" ) as exc: - EmbeddedIntrinsicAdapter.from_hub("ibm-granite/some-switch") + _embedded_adapters_from_hub("ibm-granite/some-switch") # Not the raw cache-path error, and auth is offered as a possibility. assert "huggingface-cli login" in str(exc.value) @@ -474,40 +467,38 @@ def test_missing_index_after_download_raises_clear_file_not_found( assert isinstance(exc.value.__cause__, FileNotFoundError) -# ---- EmbeddedIntrinsicAdapter.from_source ---- +# ---- _embedded_adapters_from_source ---- class TestFromSource: def test_local_directory(self, model_dir): """Local path routes to from_model_directory.""" - adapters = EmbeddedIntrinsicAdapter.from_source(str(model_dir)) - assert len(adapters) == 2 + pairs = _embedded_adapters_from_source(str(model_dir)) + assert len(pairs) == 2 def test_local_directory_with_filter(self, model_dir): """Local path with intrinsic_name filter.""" - adapters = EmbeddedIntrinsicAdapter.from_source( + pairs = _embedded_adapters_from_source( str(model_dir), intrinsic_name="answerability" ) - assert len(adapters) == 1 - assert adapters[0].intrinsic_name == "answerability" + assert len(pairs) == 1 + assert pairs[0][0].identity.name == "answerability" def test_hub_repo_id(self, model_dir, hub_cache_dir): """Non-local string routes to from_hub.""" with patch( "huggingface_hub.snapshot_download", return_value=str(model_dir) ) as mock_dl: - adapters = EmbeddedIntrinsicAdapter.from_source( - "ibm-granite/granite-switch-micro" - ) + pairs = _embedded_adapters_from_source("ibm-granite/granite-switch-micro") mock_dl.assert_called_once() - assert len(adapters) == 2 + assert len(pairs) == 2 def test_hub_passes_revision_and_cache(self, model_dir): """revision and cache_dir are forwarded to from_hub.""" with patch( "huggingface_hub.snapshot_download", return_value=str(model_dir) ) as mock_dl: - EmbeddedIntrinsicAdapter.from_source( + _embedded_adapters_from_source( "ibm-granite/granite-switch-micro", revision="v2", cache_dir="/tmp/cache", @@ -520,10 +511,14 @@ def test_hub_passes_revision_and_cache(self, model_dir): ) -# ---- OpenAIBackend adapter integration ---- +# ---- OpenAIBackend adapter integration (composed Adapter) ---- + +class TestOpenAIBackendComposedAdapterRegistration: + """`OpenAIBackend.add_adapter` accepts a composed `Adapter` whose `weights` + is an `EmbeddedBinding` (Epic #929, issue #1144). + """ -class TestOpenAIBackendRegistration: @pytest.fixture def backend(self): os.environ.setdefault("OPENAI_API_KEY", "test-key") @@ -533,32 +528,68 @@ def backend(self): model_id="granite-switch", base_url="http://localhost:8000/v1" ) - def test_add_adapter(self, backend): - adapter = EmbeddedIntrinsicAdapter( - intrinsic_name="answerability", - config=_ANSWERABILITY_CONFIG, - technology="alora", + @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(), ) - backend.add_adapter(adapter) + + 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.backend is backend + assert adapter.weights.source == backend.base_model_name # type: ignore[union-attr] + assert backend._composed_adapter_configs["answerability_alora"] == { + "parameters": {} + } - def test_add_non_embedded_adapter_raises(self, backend): + 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_non_adapter_raises(self, backend): mock_adapter = MagicMock(spec=[]) - with pytest.raises(TypeError, match="only supports EmbeddedIntrinsicAdapter"): + with pytest.raises(TypeError, match="composed Adapter"): backend.add_adapter(mock_adapter) - def test_list_adapters(self, backend): - backend.add_adapter( - EmbeddedIntrinsicAdapter( - "answerability", config=_ANSWERABILITY_CONFIG, technology="alora" - ) - ) + 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" - ) + self._make_composed_adapter(name="citations", adapter_type="lora"), + config=_CITATIONS_CONFIG, ) assert set(backend.list_adapters()) == {"answerability_alora", "citations_lora"} @@ -580,27 +611,8 @@ def test_register_from_local_directory(self, backend, model_dir): assert set(names) == {"answerability", "citations"} assert len(backend._added_adapters) == 2 - def test_add_adapter_refuses_duplicate_name(self, backend): - """A second add_adapter() for an already-registered name is refused, not applied. - - Matches LocalHFBackend's own duplicate-registration guard (issue #1562): - the first registration wins, the second is a no-op. - """ - config1 = {"model": None, "parameters": {"max_completion_tokens": 10}} - config2 = {"model": None, "parameters": {"max_completion_tokens": 20}} - - backend.add_adapter(EmbeddedIntrinsicAdapter("test", config=config1)) - backend.add_adapter(EmbeddedIntrinsicAdapter("test", config=config2)) - - assert ( - backend._added_adapters["test_lora"].config["parameters"][ - "max_completion_tokens" - ] - == 10 - ) - def test_embedded_adapters_flag_loads_from_model_id(self, model_dir, hub_cache_dir): - """embedded_adapters=True auto-registers adapters using model_id as source.""" + """load_embedded_adapters=True auto-registers adapters using model_id as source.""" from mellea.backends.openai import OpenAIBackend os.environ.setdefault("OPENAI_API_KEY", "test-key") @@ -646,81 +658,39 @@ 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 +class TestShimsRemoved: + """The deprecated adapter shims are gone (issue #1621, PR2 of #1144). - def test_add_composed_adapter_rejects_non_embedded_weights(self, backend): - from mellea.backends.adapters._core import Adapter, Identity, LocalFileBinding + `IntrinsicAdapter`, `EmbeddedIntrinsicAdapter`, and `CustomIntrinsicAdapter` + were removed once the composed `Adapter` became their working replacement. + Importing any of them must now fail, not resolve to a lingering alias. + """ - 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) + @pytest.mark.parametrize( + "name", + ["IntrinsicAdapter", "EmbeddedIntrinsicAdapter", "CustomIntrinsicAdapter"], + ) + def test_shim_not_importable_from_package(self, name): + import mellea.backends.adapters as adapters_pkg - 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 + assert not hasattr(adapters_pkg, name) + assert name not in adapters_pkg.__all__ + with pytest.raises(ImportError): + exec(f"from mellea.backends.adapters import {name}") - 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 + @pytest.mark.parametrize( + "name", + [ + "IntrinsicAdapter", + "EmbeddedIntrinsicAdapter", + "CustomIntrinsicAdapter", + "_ShimWeightsBinding", + ], + ) + def test_shim_not_importable_from_adapter_module(self, name): + import mellea.backends.adapters.adapter as adapter_mod - 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"} + assert not hasattr(adapter_mod, name) if __name__ == "__main__": diff --git a/test/backends/test_adapters/test_embedded_integration.py b/test/backends/test_adapters/test_embedded_integration.py index 9bff4a3e68..4046f74ea9 100644 --- a/test/backends/test_adapters/test_embedded_integration.py +++ b/test/backends/test_adapters/test_embedded_integration.py @@ -11,6 +11,7 @@ the GPU-backed e2e counterpart. """ +import dataclasses from unittest.mock import AsyncMock, MagicMock, PropertyMock, patch import pytest @@ -19,7 +20,7 @@ from openai.types.completion_usage import CompletionUsage from mellea.backends.adapters import EmbeddedBinding, ServerMediatedBinding -from mellea.backends.adapters.adapter import EmbeddedIntrinsicAdapter +from mellea.backends.adapters.adapter import _embedded_adapter_from_entry from mellea.backends.openai import OpenAIBackend from mellea.formatters.granite import IntrinsicsResultProcessor from mellea.plugins.types import HookType @@ -63,9 +64,10 @@ def _backend_with_adapter(technology: str) -> OpenAIBackend: base_url="http://localhost:9999/v1", ) backend.add_adapter( - EmbeddedIntrinsicAdapter( + _embedded_adapter_from_entry( intrinsic_name="answerability", config=_SIMPLE_CONFIG, technology=technology - ) + ), + config=_SIMPLE_CONFIG, ) return backend @@ -299,16 +301,19 @@ async def test_invocation_complete_fires_error_on_empty_choices(): async def test_reassigned_weights_fail_loudly(): - """Reassigning `.weights` off the EmbeddedBinding must fail at generation. + """Reassigning `.weights` off the EmbeddedBinding must be impossible. - The shim permits attribute mutation, so a caller reassigning `.weights` - after construction must hit the explicit TypeError rather than silently - skipping activation and sending an unactivated request (issue #1142). + The composed `Adapter` is a frozen dataclass, so a caller cannot swap + `.weights` after construction to send an unactivated request — the + assignment itself raises rather than silently skipping activation + (issue #1142). This is the composed-Adapter guarantee that replaces the + deprecated shim's mutable-attribute-then-fail-at-generation path. """ backend = _backend_with_adapter("alora") adapter = backend._added_adapters["answerability_alora"] - adapter.weights = ServerMediatedBinding() - ctx = ChatContext().add(Message("user", "What is the square root of 4?")) - with pytest.raises(TypeError, match=r"weights must be an EmbeddedBinding"): - await mfuncs.aact(Intrinsic("answerability"), ctx, backend, strategy=None) + with pytest.raises(dataclasses.FrozenInstanceError): + adapter.weights = ServerMediatedBinding() + + # The registered adapter still activates correctly — nothing was mutated. + assert isinstance(adapter.weights, EmbeddedBinding) diff --git a/test/backends/test_adapters/test_io_contracts.py b/test/backends/test_adapters/test_io_contracts.py index b02a694418..c90bb07d34 100644 --- a/test/backends/test_adapters/test_io_contracts.py +++ b/test/backends/test_adapters/test_io_contracts.py @@ -82,7 +82,7 @@ def test_registry_invariant_ignores_runtime_custom_catalogue_entry(monkeypatch): def test_get_io_contract_falls_back_permissively_for_unknown_names(): - """A name outside the catalog (e.g. a `CustomIntrinsicAdapter`) must not raise.""" + """A name outside the catalog (a custom, non-catalog adapter) must not raise.""" contract = get_io_contract("some-custom-user-adapter") result = contract.parse(json.dumps({"anything": "goes"})) assert result == {"anything": "goes"} diff --git a/test/backends/test_adapters/test_local_file_e2e.py b/test/backends/test_adapters/test_local_file_e2e.py index 2f9985bce4..b5f3f3d35e 100644 --- a/test/backends/test_adapters/test_local_file_e2e.py +++ b/test/backends/test_adapters/test_local_file_e2e.py @@ -94,7 +94,7 @@ async def test_local_file_binding_full_lifecycle_against_real_model(backend): assert binding.backend is backend assert binding.qualified_name in backend.list_adapters() - # `_generate_intrinsic_with_adapter_scope()` holds this lock for its whole + # `_generate_composed_local_file_with_adapter_scope()` holds this lock for its whole # prepare -> activate -> generate -> deactivate critical section. # `adapter_scope()` reacquires it during activation/deactivation, so this # real-model test also proves that same-thread reentrancy works in practice. diff --git a/test/backends/test_adapters/test_shims.py b/test/backends/test_adapters/test_shims.py deleted file mode 100644 index b781bd7c58..0000000000 --- a/test/backends/test_adapters/test_shims.py +++ /dev/null @@ -1,1099 +0,0 @@ -# Copyright IBM Corp. All Rights Reserved. -# SPDX-License-Identifier: Apache-2.0 - -"""Unit tests for adapter shim classes (Epic #929 Phase 1, issue #1136). - -Verifies that IntrinsicAdapter, EmbeddedIntrinsicAdapter, and CustomIntrinsicAdapter: - - emit DeprecationWarning on construction - - are instances of both their own class and the new Adapter dataclass - - expose a well-formed Identity (name, adapter_type, capability) - - leave AdapterMixin.resolve_adapter and AdapterMixin.adapter_scope callable -""" - -import threading -import time -import warnings -from unittest.mock import MagicMock, mock_open, patch - -import pytest - -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, _composed_adapter_key -from mellea.backends.adapters.catalog import AdapterType, IntrinsicsCatalogEntry - -# --------------------------------------------------------------------------- -# Helpers -# --------------------------------------------------------------------------- - -_MOCK_CATALOG_ENTRY = IntrinsicsCatalogEntry( - name="answerability", - repo_id="ibm-granite/granitelib-rag-r1.0", - revision="abc123deadbeef", - adapter_types=(AdapterType.ALORA, AdapterType.LORA), -) - - -def _make_intrinsic_adapter(intrinsic_name: str = "answerability") -> IntrinsicAdapter: - """Construct IntrinsicAdapter with mocked catalog + config (no HF downloads).""" - with ( - patch( - "mellea.backends.adapters.adapter.fetch_intrinsic_metadata", - return_value=IntrinsicsCatalogEntry( - name=intrinsic_name, - repo_id="ibm-granite/granitelib-rag-r1.0", - revision="abc123", - adapter_types=(AdapterType.ALORA, AdapterType.LORA), - ), - ), - warnings.catch_warnings(), - ): - warnings.simplefilter("ignore", DeprecationWarning) - return IntrinsicAdapter( - intrinsic_name, - adapter_type=AdapterType.ALORA, - config_dict={"dummy": "config"}, - ) - - -# --------------------------------------------------------------------------- -# EmbeddedIntrinsicAdapter shim tests (no mock needed — no catalog access) -# --------------------------------------------------------------------------- - - -def test_embedded_emits_deprecation_warning(): - with pytest.warns( - DeprecationWarning, match="EmbeddedIntrinsicAdapter is deprecated" - ): - EmbeddedIntrinsicAdapter("answerability", config={}, technology="alora") - - -def test_embedded_is_instance_of_new_adapter(): - with warnings.catch_warnings(): - warnings.simplefilter("ignore", DeprecationWarning) - adapter = EmbeddedIntrinsicAdapter( - "answerability", config={}, technology="alora" - ) - assert isinstance(adapter, Adapter), ( - "shim must be instance of new Adapter dataclass" - ) - assert isinstance(adapter, EmbeddedIntrinsicAdapter), ( - "shim must remain its own type" - ) - - -def test_embedded_identity_populated(): - with warnings.catch_warnings(): - warnings.simplefilter("ignore", DeprecationWarning) - adapter = EmbeddedIntrinsicAdapter( - "answerability", config={}, technology="alora" - ) - assert isinstance(adapter.identity, Identity) - assert adapter.identity.name == "answerability" - assert adapter.identity.capability == "answerability" - assert adapter.identity.adapter_type == "alora" - - -def test_embedded_carries_the_declared_io_contract_not_a_shim(): - """resolve_adapter()'s shim path must carry a real contract (issue #1516). - - Regression guard: without this, reverting `get_io_contract(intrinsic_name)` - back to the Phase 1 `_ShimIOContract()` placeholder would pass every other - test in this file, since none of them inspect `.io_contract`. - """ - with warnings.catch_warnings(): - warnings.simplefilter("ignore", DeprecationWarning) - adapter = EmbeddedIntrinsicAdapter( - "answerability", config={}, technology="alora" - ) - assert adapter.io_contract is get_io_contract("answerability") - - -def test_embedded_identity_lora_technology(): - with warnings.catch_warnings(): - warnings.simplefilter("ignore", DeprecationWarning) - adapter = EmbeddedIntrinsicAdapter( - "answerability", config={}, technology="lora" - ) - assert adapter.identity.adapter_type == "lora" - - -def test_embedded_catalog_capability_avoids_spurious_registry_warning(): - """Regression (#1563): catalog aliases use their effective capability. - - Before this fix, the hyphenated catalog name emitted a KNOWN_CAPABILITIES - warning even though the adapter function was registered. - """ - with warnings.catch_warnings(record=True) as caught: - warnings.simplefilter("always") - adapter = EmbeddedIntrinsicAdapter( - "guardian-core", config={}, technology="alora" - ) - - assert adapter.identity.capability == "guardian_core" - assert not any( - warning.category is UserWarning and "KNOWN_CAPABILITIES" in str(warning.message) - for warning in caught - ) - - -def test_embedded_custom_capability_still_emits_registry_warning(): - """Custom embedded adapters retain their user-provided capability token.""" - with warnings.catch_warnings(record=True) as caught: - warnings.simplefilter("always") - adapter = EmbeddedIntrinsicAdapter("custom-adapter", config={}) - - assert adapter.identity.capability == "custom-adapter" - assert any( - warning.category is UserWarning and "KNOWN_CAPABILITIES" in str(warning.message) - for warning in caught - ) - - -def test_embedded_legacy_attributes_preserved(): - with warnings.catch_warnings(): - warnings.simplefilter("ignore", DeprecationWarning) - adapter = EmbeddedIntrinsicAdapter( - "answerability", config={"k": 1}, technology="alora" - ) - assert adapter.intrinsic_name == "answerability" - assert adapter.config == {"k": 1} - assert adapter.technology == "alora" - assert adapter.qualified_name == "answerability_alora" - assert adapter.backend is None - - -def test_embedded_backend_mutable(): - """Shim must allow setting backend after construction (frozen bypass).""" - with warnings.catch_warnings(): - warnings.simplefilter("ignore", DeprecationWarning) - adapter = EmbeddedIntrinsicAdapter( - "answerability", config={}, technology="alora" - ) - sentinel = object() - adapter.backend = sentinel # type: ignore[assignment] - assert adapter.backend is sentinel - - -def test_embedded_invalid_technology(): - # Validation runs before the deprecation warning, so no DeprecationWarning fires. - with warnings.catch_warnings(): - warnings.simplefilter("error", DeprecationWarning) - with pytest.raises(ValueError, match="technology must be"): - EmbeddedIntrinsicAdapter("answerability", config={}, technology="qlora") - - -# --------------------------------------------------------------------------- -# IntrinsicAdapter shim tests (uses patch to avoid catalog / HF access) -# --------------------------------------------------------------------------- - - -def test_intrinsic_emits_deprecation_warning(): - with ( - patch( - "mellea.backends.adapters.adapter.fetch_intrinsic_metadata", - return_value=_MOCK_CATALOG_ENTRY, - ), - pytest.warns(DeprecationWarning, match="IntrinsicAdapter is deprecated"), - ): - IntrinsicAdapter( - "answerability", - adapter_type=AdapterType.ALORA, - config_dict={"dummy": "config"}, - ) - - -def test_intrinsic_is_instance_of_new_adapter(): - adapter = _make_intrinsic_adapter("answerability") - assert isinstance(adapter, Adapter), ( - "shim must be instance of new Adapter dataclass" - ) - assert isinstance(adapter, IntrinsicAdapter), "shim must remain its own type" - - -def test_intrinsic_identity_populated(): - adapter = _make_intrinsic_adapter("answerability") - assert isinstance(adapter.identity, Identity) - assert adapter.identity.name == "answerability" - assert adapter.identity.capability == "answerability" - assert adapter.identity.adapter_type == "alora" - - -def test_intrinsic_carries_the_declared_io_contract_not_a_shim(): - """resolve_adapter()'s shim path must carry a real contract (issue #1516). - - Regression guard: without this, reverting `get_io_contract(intrinsic_name)` - back to the Phase 1 `_ShimIOContract()` placeholder would pass every other - test in this file, since none of them inspect `.io_contract`. - """ - adapter = _make_intrinsic_adapter("answerability") - assert adapter.io_contract is get_io_contract("answerability") - - -def test_intrinsic_identity_lora_adapter_type(): - with ( - patch( - "mellea.backends.adapters.adapter.fetch_intrinsic_metadata", - return_value=IntrinsicsCatalogEntry( - name="answerability", - repo_id="ibm-granite/granitelib-rag-r1.0", - revision="abc123", - adapter_types=(AdapterType.LORA,), - ), - ), - warnings.catch_warnings(), - ): - warnings.simplefilter("ignore", DeprecationWarning) - adapter = IntrinsicAdapter( - "answerability", - adapter_type=AdapterType.LORA, - config_dict={"dummy": "config"}, - ) - assert adapter.identity.adapter_type == "lora" - - -def test_intrinsic_catalog_capability_avoids_spurious_registry_warning(): - """Regression (#1563): catalog aliases use their effective capability. - - Before this fix, the hyphenated catalog name emitted a KNOWN_CAPABILITIES - warning even though the adapter function was registered. - """ - with warnings.catch_warnings(record=True) as caught: - warnings.simplefilter("always") - adapter = IntrinsicAdapter( - "guardian-core", - adapter_type=AdapterType.ALORA, - config_dict={"dummy": "config"}, - ) - - assert adapter.identity.capability == "guardian_core" - assert not any( - warning.category is UserWarning and "KNOWN_CAPABILITIES" in str(warning.message) - for warning in caught - ) - - -def test_intrinsic_legacy_attributes_preserved(): - adapter = _make_intrinsic_adapter("answerability") - assert adapter.intrinsic_name == "answerability" - assert adapter.config == {"dummy": "config"} - assert adapter.qualified_name == "answerability_alora" - assert adapter.backend is None - - -def test_intrinsic_backend_mutable(): - """Shim must allow setting backend after construction (frozen bypass).""" - adapter = _make_intrinsic_adapter() - sentinel = object() - adapter.backend = sentinel # type: ignore[assignment] - assert adapter.backend is sentinel - - -# --------------------------------------------------------------------------- -# AdapterMixin stub methods -# --------------------------------------------------------------------------- - - -def test_adapter_mixin_has_resolve_adapter(): - assert callable(getattr(AdapterMixin, "resolve_adapter", None)) - - -def test_adapter_mixin_has_adapter_scope(): - assert callable(getattr(AdapterMixin, "adapter_scope", None)) - - -def test_adapter_scope_is_noop(): - """adapter_scope must work as a no-op context manager via the mixin default.""" - mock_backend = MagicMock(spec=AdapterMixin) - # Call the real implementation via the class (bypasses mock's own spec) - with AdapterMixin.adapter_scope(mock_backend, None): - pass # must not raise - - -def test_adapter_scope_raises_for_a_shim_backed_adapter(): - """adapter_scope now activates real weights, so a shim-backed adapter raises. - - Deliberate behaviour change from Phase 1 (issue #1140), where `adapter_scope` - was `yield` unconditionally regardless of `adapter.weights`. `resolve_adapter()` - still returns `IntrinsicAdapter`/`LocalHFAdapter` shims carrying - `_ShimWeightsBinding`, whose `.activate()` raises `NotImplementedError` — so - `with backend.adapter_scope(backend.resolve_adapter(name)):` goes from a - no-op to a hard failure for every adapter the public API currently hands - out. Nothing in the codebase calls `adapter_scope` with a resolved adapter - yet (#1465 is the tracked cutover), but this pins the change as - deliberate rather than incidental — if #1465 needs `adapter_scope` to - tolerate shim/unprepared bindings instead, that decision should update - this test, not silently contradict it. - """ - mock_backend = MagicMock(spec=AdapterMixin) - adapter = _make_intrinsic_adapter("answerability") - - with pytest.raises(NotImplementedError, match="WeightsBinding not yet implemented"): - with AdapterMixin.adapter_scope(mock_backend, adapter): - pytest.fail("body must not run when the shim's activate() raises") - - -def test_adapter_scope_rejects_an_embedded_binding(): - """adapter_scope drives the WeightsBinding lifecycle; EmbeddedBinding has none. - - Deliberate behaviour change from #1142: `adapter_scope` used to raise - `NotImplementedError` from `_ShimWeightsBinding.activate()` for an - `EmbeddedIntrinsicAdapter` and fire `invocation_complete(outcome="error")`; - it now raises `TypeError` before entering the try/finally and fires - nothing. This pins the new guard (`adapter.py`'s `isinstance(adapter.weights, - WeightsBinding)` check) rather than leaving it to surface as an - `AttributeError` from `adapter.weights.activate()` the next time someone - wires embedded generation through this scope. - """ - mock_backend = MagicMock(spec=AdapterMixin) - with warnings.catch_warnings(): - warnings.simplefilter("ignore", DeprecationWarning) - adapter = EmbeddedIntrinsicAdapter( - "answerability", config={}, technology="alora" - ) - - with pytest.raises(TypeError, match=r"have no activate\(\)/deactivate\(\)"): - with AdapterMixin.adapter_scope(mock_backend, adapter): - pytest.fail("body must not run for a binding with no lifecycle") - - -def test_resolve_adapter_returns_existing_by_capability(): - """resolve_adapter must return an already-registered adapter without creating a new one.""" - existing = _make_intrinsic_adapter("answerability") - mock_backend = MagicMock(spec=AdapterMixin) - mock_backend._added_adapters = {existing.qualified_name: existing} - # Route _find_adapter through the real implementation so the _added_adapters search runs. - mock_backend._find_adapter.side_effect = lambda cap, types=None: ( - AdapterMixin._find_adapter(mock_backend, cap, types) - ) - result = AdapterMixin.resolve_adapter(mock_backend, "answerability") - assert result is existing - mock_backend.add_adapter.assert_not_called() - - -def test_find_adapter_honours_type_preference_order(): - """_find_adapter must return the highest-priority type, not the insertion-order winner.""" - with warnings.catch_warnings(): - warnings.simplefilter("ignore", DeprecationWarning) - lora = EmbeddedIntrinsicAdapter("answerability", config={}, technology="lora") - alora = EmbeddedIntrinsicAdapter("answerability", config={}, technology="alora") - - mock_backend = MagicMock(spec=AdapterMixin) - # Register lora first so insertion order would return it without preference logic. - mock_backend._added_adapters = { - lora.qualified_name: lora, - alora.qualified_name: alora, - } - - result = AdapterMixin._find_adapter( - mock_backend, "answerability", ("alora", "lora") - ) - assert result is alora, "alora must win over lora regardless of insertion order" - - -class _MutatingCapability: - """Capability sentinel whose `__eq__` deletes an entry from the registry - it is compared against, simulating a concurrent `release()` mutating - `_added_adapters` mid-iteration. - - `_find_adapter` compares `a.identity.capability == capability` for each - registered adapter in turn; a real `str.__eq__` against this sentinel - returns `NotImplemented`, so Python falls back to this class's reflected - `__eq__` — firing the side effect from inside the loop, deterministically, - with no actual threading required. - """ - - def __init__(self, registry: dict, key_to_remove: str) -> None: - self._registry = registry - self._key_to_remove = key_to_remove - self.fired = False - - def __eq__(self, other: object) -> bool: - if not self.fired: - self.fired = True - self._registry.pop(self._key_to_remove, None) - return False - - def __hash__(self) -> int: - return hash("mutating-capability-sentinel") - - -class _MutatingName: - """Capability-name sentinel whose `__str__` pops an entry from the - registry, simulating a concurrent `release()` mutating - `_added_adapters` while `resolve_adapter`'s collision scan builds - `f"{name}_"` on every iteration. - - Companion to `_MutatingCapability` (which fires from a reflected - `str.__eq__` inside `_find_adapter`): the collision scan never compares - `name` with `==`, so the side effect hooks `__str__` instead — the - f-string build fires it from inside the scan, deterministically, with no - threading. `__add__` answers without firing: `Adapter.__init__` builds - `qualified_name` as `name + "_" + ...` during the lazy-registration - step, and the mutation must land in the scan, not in registration. - """ - - def __init__(self, registry: dict, key_to_remove: str, prefix: str) -> None: - self._registry = registry - self._key_to_remove = key_to_remove - self._prefix = prefix - self.fired = False - - def __add__(self, other: str) -> str: - return self._prefix + other - - def __str__(self) -> str: - if not self.fired: - self.fired = True - self._registry.pop(self._key_to_remove, None) - return self._prefix - - def __repr__(self) -> str: - return f"_MutatingName({self._prefix!r})" - - -def test_find_adapter_survives_concurrent_removal_during_iteration(): - """`_find_adapter` must not iterate a live view over `_added_adapters`. - - `_added_adapters` was insert-only until `remove_adapter()` (#1528) added - the first runtime deletion from it. A concurrent `release()` mutating the - dict while `_find_adapter` holds a live `.values()` view raises - `RuntimeError: dictionary changed size during iteration`. `_find_adapter` - must snapshot into a list first. - """ - with warnings.catch_warnings(): - warnings.simplefilter("ignore", DeprecationWarning) - first = EmbeddedIntrinsicAdapter("answerability", config={}, technology="lora") - second = EmbeddedIntrinsicAdapter("uncertainty", config={}, technology="lora") - - mock_backend = MagicMock(spec=AdapterMixin) - registry = {first.qualified_name: first, second.qualified_name: second} - mock_backend._added_adapters = registry - - capability = _MutatingCapability(registry, second.qualified_name) - result = AdapterMixin._find_adapter(mock_backend, capability) # must not raise - - assert capability.fired, "the mutation must have fired during iteration" - assert result is None - assert second.qualified_name not in registry - - -def test_resolve_adapter_survives_concurrent_removal_during_iteration(): - """`resolve_adapter`'s collision scan must not iterate a live view of `_added_adapters`. - - Guards the `list(...)` snapshot with the same deterministic sentinel - pattern as - `test_find_adapter_survives_concurrent_removal_during_iteration`: the - pop fires from the scan's `f"{name}_"` build on the first (non-matching) - entry, so a live `.items()` view raises - `RuntimeError: dictionary changed size during iteration` when the scan - then advances, while the snapshot iterates on. - """ - binding = LocalFileBinding(name="answerability", adapter_type=AdapterType.LORA) - with warnings.catch_warnings(): - warnings.simplefilter("ignore", DeprecationWarning) - distractor = EmbeddedIntrinsicAdapter( - "uncertainty", config={}, technology="lora" - ) - - mock_backend = MagicMock(spec=AdapterMixin) - mock_backend.base_model_name = "ibm-granite/granite-4.1-3b" - mock_backend._uses_embedded_adapters = False - registry = {distractor.qualified_name: distractor, binding.qualified_name: binding} - mock_backend._added_adapters = registry - mock_backend._find_adapter.side_effect = lambda cap, types=None: ( - AdapterMixin._find_adapter(mock_backend, cap, types) - ) - # Nothing new lands in the registry: the binding keeps the name, exactly - # as the real duplicate-key guard would. - mock_backend.add_adapter.side_effect = lambda a: None - - name = _MutatingName(registry, distractor.qualified_name, "answerability") - 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 pytest.raises(KeyError, match=r"LocalFileBinding.*answerability_lora"): - AdapterMixin.resolve_adapter(mock_backend, name) - - assert name.fired, "the mutation must have fired during the collision scan" - assert distractor.qualified_name not in registry - assert binding.qualified_name in registry - - -def test_resolve_adapter_names_the_conflict_when_a_binding_blocks_registration(): - """resolve_adapter's KeyError should name the collision, not just say "not found". - - Regression guard: a `LocalFileBinding` registered under the same - qualified-name key space `resolve_adapter` auto-registers into silently - blocks the new `IntrinsicAdapter` (the backend's duplicate-key guard - refuses it). `_find_adapter` can't see the `LocalFileBinding` either (not - an `_AdapterCore`), so without this check the failure surfaced as a bare - "Adapter 'answerability' not found after registration" with no hint of - what actually occupied the name. - """ - binding = LocalFileBinding(name="answerability", adapter_type=AdapterType.LORA) - mock_backend = MagicMock(spec=AdapterMixin) - mock_backend.base_model_name = "ibm-granite/granite-4.1-3b" - mock_backend._uses_embedded_adapters = False - mock_backend._added_adapters = {binding.qualified_name: binding} - mock_backend._find_adapter.side_effect = lambda cap, types=None: ( - AdapterMixin._find_adapter(mock_backend, cap, types) - ) - # Simulates the backend's real duplicate-key guard refusing the new - # IntrinsicAdapter: registration is attempted but nothing new lands in - # `_added_adapters`. - mock_backend.add_adapter.side_effect = lambda a: None - - 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 pytest.raises(KeyError, match=r"LocalFileBinding.*answerability_lora"): - AdapterMixin.resolve_adapter(mock_backend, "answerability") - - -def test_resolve_adapter_raises_without_base_model(): - """resolve_adapter must raise ValueError when the backend has no model ID.""" - mock_backend = MagicMock(spec=AdapterMixin) - mock_backend._added_adapters = {} - mock_backend.base_model_name = None - # _find_adapter returns None so resolve_adapter proceeds to the base-model check. - mock_backend._find_adapter.return_value = None - with pytest.raises(ValueError, match="no model ID"): - AdapterMixin.resolve_adapter(mock_backend, "answerability") - - -def test_resolve_adapter_lazy_creates_and_returns(): - """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", - revision="abc123", - adapter_types=(AdapterType.ALORA, AdapterType.LORA), - ) - mock_backend = MagicMock(spec=AdapterMixin) - mock_backend.base_model_name = "ibm-granite/granite-4.1-3b" - mock_backend._uses_embedded_adapters = False - - created_adapters: list = [] - - def fake_add_adapter(a): - created_adapters.append(a) - mock_backend._added_adapters[_composed_adapter_key(a)] = a - - mock_backend._added_adapters = {} - 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, - ): - result = AdapterMixin.resolve_adapter(mock_backend, "answerability") - - assert mock_backend.add_adapter.called, ( - "add_adapter must be called for a new capability" - ) - assert len(created_adapters) == 1 - 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] - - -def test_resolve_adapter_catalog_alias_returns_registered_adapter(): - """Regression (#1563): lookup accepts the catalog's public alias. - - The shim stores `guardian_core` as its canonical capability, while callers - resolve the adapter via the catalog name `guardian-core`, including when - they constrain the adapter type. - """ - mock_catalog_entry = IntrinsicsCatalogEntry( - name="guardian-core", - capability="guardian_core", - repo_id="ibm-granite/granitelib-guardian-r1.0", - revision="abc123", - adapter_types=(AdapterType.LORA,), - ) - mock_backend = MagicMock(spec=AdapterMixin) - mock_backend.base_model_name = "ibm-granite/granite-4.1-3b" - mock_backend._uses_embedded_adapters = False - mock_backend._added_adapters = {} - mock_backend.add_adapter.side_effect = lambda 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, - ): - result = AdapterMixin.resolve_adapter(mock_backend, "guardian-core") - - assert result.identity.name == "guardian-core" - assert result.identity.capability == "guardian_core" - assert ( - AdapterMixin._find_adapter(mock_backend, "guardian-core", ("lora",)) is result - ) - - -class _TrackingLock: - """Real lock that records how many times it was entered/exited. - - Used in place of a bare `MagicMock()` context manager so a test can - assert the lock was acquired exactly once around the whole registration - critical section (not once per `add_adapter` call in the embedded loop), - and that `warnings.catch_warnings()` has already restored the filter - state by the time this lock releases (`filter_restored_at_exit`) — that - ordering is what closes the pre-existing filter-restoration race - alongside the registration race; the two context managers must nest with - the lock outermost so `catch_warnings().__exit__()` runs before this - lock's `__exit__()`. - """ - - def __init__(self) -> None: - self._lock = threading.RLock() - self.enter_count = 0 - self.exit_count = 0 - self.max_depth = 0 - self._baseline_filters = list(warnings.filters) - self.filter_restored_at_exit: list[bool] = [] - - def __enter__(self): - self._lock.acquire() - self.enter_count += 1 - self.max_depth = max(self.max_depth, self.enter_count - self.exit_count) - return None - - def __exit__(self, *exc_info): - # `simplefilter("ignore", DeprecationWarning)` happens to add an entry - # identical to one Python's own default filters already carry, so - # scanning `warnings.filters` for that entry can't tell "restored" - # apart from "never mutated" — checking against the exact pre-test - # snapshot (order included) is what actually distinguishes them. - self.filter_restored_at_exit.append( - list(warnings.filters) == self._baseline_filters - ) - self.exit_count += 1 - self._lock.release() - return False - - -def test_resolve_adapter_survives_reentrant_activation_lock(): - """resolve_adapter() must not deadlock when called while the lock is already held. - - `_adapter_activation_lock()`'s docstring documents a reentrancy contract: - an override must return a reentrant lock because a caller can already - hold it on the same thread. Nothing production calls today puts - `resolve_adapter()` on that path (`call_intrinsic` resolves before - `mfuncs.act` acquires the lock), so a regression to a non-reentrant - override — a real `threading.RLock` swapped for a plain `threading.Lock` - — would only surface as a hang the first time some future caller does - reenter, not as a test failure today. Simulate that caller directly: - hold the real lock this mock backend's `_adapter_activation_lock()` - returns, then call `resolve_adapter()` from inside that hold. - """ - mock_catalog_entry = IntrinsicsCatalogEntry( - name="answerability", - repo_id="ibm-granite/granitelib-rag-r1.0", - revision="abc123", - adapter_types=(AdapterType.ALORA, AdapterType.LORA), - ) - real_lock = threading.RLock() - mock_backend = MagicMock(spec=AdapterMixin) - mock_backend.base_model_name = "ibm-granite/granite-4.1-3b" - mock_backend._uses_embedded_adapters = False - 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__(_composed_adapter_key(a), a) - ) - 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, - ), - real_lock, # simulate an already-in-progress caller holding the lock - ): - result = AdapterMixin.resolve_adapter(mock_backend, "answerability") - - assert result is not None - assert result.identity.name == "answerability" - - -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 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", - repo_id="ibm-granite/granitelib-rag-r1.0", - revision="abc123", - adapter_types=(AdapterType.ALORA, AdapterType.LORA), - ) - mock_backend = MagicMock(spec=AdapterMixin) - mock_backend.base_model_name = "ibm-granite/granite-4.1-3b" - mock_backend._uses_embedded_adapters = False - mock_backend._added_adapters = {} - - tracking_lock = _TrackingLock() - 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 resolve lock is held" - ) - 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, - ): - 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 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_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). `_adapter_resolve_lock()`, not - `_adapter_activation_lock()` — see the lock-order note on the latter. - """ - with warnings.catch_warnings(): - warnings.simplefilter("ignore", DeprecationWarning) - embedded_adapters = [ - EmbeddedIntrinsicAdapter("answerability", config={}, technology="alora"), - EmbeddedIntrinsicAdapter("answerability", config={}, technology="lora"), - ] - - mock_backend = MagicMock(spec=AdapterMixin) - mock_backend.base_model_name = "ibm-granite/granite-4.1-3b" - mock_backend._uses_embedded_adapters = True - mock_backend._adapter_source = "ibm-granite/granite-switch-micro" - mock_backend._added_adapters = {} - - tracking_lock = _TrackingLock() - mock_backend._adapter_resolve_lock.return_value = tracking_lock - mock_backend._supports_composed_adapters = True - - 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", - return_value=embedded_adapters, - ): - AdapterMixin.resolve_adapter(mock_backend, "answerability") - - assert mock_backend.add_adapter.call_count == 2, ( - "both embedded adapters must have been registered" - ) - assert tracking_lock.enter_count == 1, ( - "the lock must be acquired once for the whole loop, not per adapter" - ) - assert tracking_lock.exit_count == 1 - - -def test_resolve_adapter_concurrent_first_use_does_not_double_register(): - """Two concurrent first-time resolves for the same name must not race (issue #1562). - - Mirrors the real `LocalHFBackend.add_adapter` duplicate-registration - 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_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. - """ - mock_catalog_entry = IntrinsicsCatalogEntry( - name="answerability", - repo_id="ibm-granite/granitelib-rag-r1.0", - revision="abc123", - adapter_types=(AdapterType.ALORA, AdapterType.LORA), - ) - - registry: dict = {} - real_lock = threading.RLock() - registrations: list = [] - - mock_backend = MagicMock(spec=AdapterMixin) - 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_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): - 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[key] = adapter - registrations.append(adapter) - - mock_backend.add_adapter.side_effect = racy_add_adapter - - results: list = [None, None] - errors: list = [] - - def call(index: int) -> None: - try: - results[index] = AdapterMixin.resolve_adapter(mock_backend, "answerability") - except Exception as e: # pragma: no cover - surfaced via errors list - errors.append(e) - - # `unittest.mock.patch`'s save/restore of the patched attribute is not - # thread-safe: two threads independently entering/exiting a `patch(...)` - # on the same target race on save-then-restore and can leave the target - # (e.g. `builtins.open`) permanently monkey-patched for the rest of the - # process. Install the patches once here, from the main thread, before - # spawning the workers — only `resolve_adapter`'s own registration path - # runs concurrently, which is what this test targets. - 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")), - ): - t1 = threading.Thread(target=call, args=(0,), daemon=True) - t2 = threading.Thread(target=call, args=(1,), daemon=True) - t1.start() - t2.start() - t1.join(timeout=5) - t2.join(timeout=5) - # A still-alive thread here would still be running resolve_adapter() - # against the patches this `with` block is about to tear down on exit - # — exactly the patch-torn-down-while-in-use hazard the comment above - # documents. Assert liveness before leaving the patch context, not - # after, so a hang surfaces as this failure instead of a flaky - # downstream corruption. - assert not t1.is_alive(), "t1 did not finish within the join timeout" - assert not t2.is_alive(), "t2 did not finish within the join timeout" - - assert not errors, f"resolve_adapter raised under concurrency: {errors}" - assert len(registrations) == 1, ( - "exactly one adapter must be registered under the never-before-seen name" - ) - # `racy_add_adapter`'s own no-op guard (`if existing is not None: return`) - # would keep the assertion above green even without resolve_adapter's - # in-lock re-check of `_find_adapter(name)` — the loser would just call - # `add_adapter` a second time and no-op there instead. Pin the actual - # optimisation directly: the loser must never call `add_adapter` at all. - mock_backend.add_adapter.assert_called_once() - 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.py b/test/backends/test_huggingface.py index c0b5b885df..edb652fb44 100644 --- a/test/backends/test_huggingface.py +++ b/test/backends/test_huggingface.py @@ -34,7 +34,6 @@ from mellea import MelleaSession from mellea.backends import ModelOption, model_ids -from mellea.backends.adapters import IntrinsicAdapter from mellea.backends.cache import SimpleLRUCache from mellea.backends.huggingface import LocalHFBackend, _assert_correct_adapters from mellea.core import ( @@ -65,14 +64,8 @@ def backend(): backend = LocalHFBackend( model_id=model_ids.IBM_GRANITE_4_1_3B, cache=SimpleLRUCache(5) ) - backend.add_adapter( - IntrinsicAdapter( - "requirement-check", base_model_name=backend.base_model_name - ) - ) - backend.add_adapter( - IntrinsicAdapter("answerability", base_model_name=backend.base_model_name) - ) + backend.resolve_adapter("requirement-check") + backend.resolve_adapter("answerability") yield backend from test.conftest import cleanup_gpu_backend @@ -372,10 +365,10 @@ async def test_generate_with_lock(backend) -> None: b._model = model b._added_adapters = {} b._loaded_adapters = {} - b.add_adapter( - IntrinsicAdapter("requirement-check", base_model_name=b.base_model_name) - ) - b.add_adapter(IntrinsicAdapter("answerability", base_model_name=b.base_model_name)) + b._composed_adapters = {} + b._composed_adapter_configs = {} + b.resolve_adapter("requirement-check") + b.resolve_adapter("answerability") memoized: dict[torch.Tensor, str] = dict() # type: ignore[name-defined] gen_func = model.generate @@ -563,9 +556,9 @@ async def test_error_during_generate_with_lock(backend) -> None: raise b._added_adapters = {} b._loaded_adapters = {} - b.add_adapter( - IntrinsicAdapter("requirement-check", base_model_name=b.base_model_name) - ) + b._composed_adapters = {} + b._composed_adapter_configs = {} + b.resolve_adapter("requirement-check") regular_generate = b._model.generate diff --git a/test/backends/test_huggingface_unit.py b/test/backends/test_huggingface_unit.py index 8ba5e68eb2..3e18ba3d94 100644 --- a/test/backends/test_huggingface_unit.py +++ b/test/backends/test_huggingface_unit.py @@ -38,15 +38,16 @@ AdapterMixin, AdapterType, EmbeddedBinding, - IntrinsicAdapter, + LocalFileBinding, ServerMediatedBinding, ) -from mellea.backends.adapters._core import Identity +from mellea.backends.adapters._core import Adapter as _AdapterCore, Identity from mellea.backends.adapters.adapter import ( - EmbeddedIntrinsicAdapter, - _ShimWeightsBinding, + _composed_adapter_key, + _embedded_adapter_from_entry, ) from mellea.backends.adapters.catalog import IntrinsicsCatalogEntry +from mellea.backends.adapters.io_contracts import get_io_contract from mellea.backends.huggingface import LocalHFBackend from mellea.core import ModelOutputThunk from mellea.formatters.granite.base.util import ( @@ -222,40 +223,69 @@ def _call(stub, opts): return LocalHFBackend._make_backend_specific_and_remove(stub, opts) -def _make_intrinsic_adapter_stub(): - adapter = IntrinsicAdapter.__new__(IntrinsicAdapter) - adapter.name = "answerability" - adapter.qualified_name = "answerability_alora" - adapter.config = {} - # Required for the capability-based lookup introduced in Epic #929 Phase 1. - # __new__ bypasses __init__; use object.__setattr__ to set frozen-dataclass fields. - object.__setattr__( - adapter, - "identity", - Identity( +def _make_intrinsic_adapter_stub() -> _AdapterCore: + """A composed LocalFile/PEFT `Adapter` for the legacy (non-embedded) path.""" + return _AdapterCore( + identity=Identity( name="answerability", adapter_type="alora", capability="answerability" ), + io_contract=get_io_contract("answerability"), + weights=LocalFileBinding( + name="answerability", + adapter_type=AdapterType.ALORA, + repo_id="fake/repo", + revision="fake0000000000000000000000000000000000000", + ), ) - object.__setattr__(adapter, "weights", _ShimWeightsBinding()) - return adapter -def _make_embedded_adapter_stub() -> EmbeddedIntrinsicAdapter: - """Build an embedded adapter without exposing its deprecation warning in tests.""" - with warnings.catch_warnings(): - warnings.simplefilter("ignore", DeprecationWarning) - return EmbeddedIntrinsicAdapter( - intrinsic_name="answerability", - config={ - "model": None, - "response_format": None, - "transformations": None, - "instruction": None, - "parameters": {"max_completion_tokens": 8}, - "sentence_boundaries": None, - }, - technology="alora", - ) +def _make_local_file_binding_stub() -> LocalFileBinding: + """A bare `LocalFileBinding` for PEFT-lifecycle tests. + + The composed `Adapter` (see `_make_intrinsic_adapter_stub`) is frozen, so + tests that exercise the register/load/remove lifecycle — mutating `.backend`, + `.path`, `._loaded`, and stubbing `.get_local_hf_path` to skip the Hub + download — drive the binding directly. `add_adapter` accepts a bare binding + and drives its lifecycle just as it does a composed LocalFile adapter's. + """ + binding = LocalFileBinding( + name="answerability", adapter_type=AdapterType.ALORA, repo_id="fake/repo" + ) + binding.get_local_hf_path = lambda base_model_name: "/fake/path" # type: ignore[method-assign] + return binding + + +_EMBEDDED_STUB_CONFIG = { + "model": None, + "response_format": None, + "transformations": None, + "instruction": None, + "parameters": {"max_completion_tokens": 8}, + "sentence_boundaries": None, +} + + +def _make_embedded_adapter_stub() -> _AdapterCore: + """A composed embedded `Adapter` (EmbeddedBinding) for the Granite Switch path.""" + return _embedded_adapter_from_entry( + intrinsic_name="answerability", config=_EMBEDDED_STUB_CONFIG, technology="alora" + ) + + +def _register_adapter_stub(backend, adapter: _AdapterCore) -> str: + """Register a composed adapter stub into a backend the way `add_adapter` would. + + A composed `Adapter` carries no `.qualified_name` and its config lives in the + backend's `_composed_adapter_configs`, not on the adapter — so tests that used + to do `backend._added_adapters = {adapter.qualified_name: adapter}` must both + key by `_composed_adapter_key(adapter)` and seed the config cache. Returns the + composed key so callers can assert against it. + """ + key = _composed_adapter_key(adapter) + backend._added_adapters = {key: adapter} + backend._composed_adapters[key] = adapter + backend._composed_adapter_configs[key] = _EMBEDDED_STUB_CONFIG + return key def _make_intrinsic_backend_stub(stub_backend): @@ -280,12 +310,6 @@ def _make_intrinsic_backend_stub(stub_backend): LocalHFBackend._make_backend_specific_and_remove(stub_backend, opts) ) stub_backend.post_processing = lambda *args, **kwargs: None - # Bypasses locking/adapter_scope entirely — these tests exercise option-merging - # and logits capture, not activation semantics (see - # test_generate_intrinsic_with_adapter_scope_* below for that). - stub_backend._generate_intrinsic_with_adapter_scope = ( - lambda adapter, generate_func, *args, **kwargs: generate_func(*args, **kwargs) - ) stub_backend._generate_embedded_with_generation_lock = ( lambda generate_func, *args, **kwargs: generate_func(*args, **kwargs) ) @@ -295,9 +319,9 @@ def _make_intrinsic_backend_stub(stub_backend): 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). + # Bypasses locking/adapter_scope entirely — these tests exercise + # option-merging and logits capture, not activation semantics (see the + # test_generate_composed_local_file_with_adapter_scope_* tests for that). stub_backend._generate_composed_local_file_with_adapter_scope = ( lambda adapter, generate_func, *args, **kwargs: generate_func(*args, **kwargs) ) @@ -331,9 +355,10 @@ def test_load_embedded_adapters_registers_checkpoint_adapters(): with ( patch("mellea.backends.huggingface.llguidance") as mock_llg, - patch.object( - EmbeddedIntrinsicAdapter, "from_source", return_value=[adapter] - ) as mock_from_source, + patch( + "mellea.backends.huggingface._discover_embedded_adapters", + return_value=[(adapter, _EMBEDDED_STUB_CONFIG)], + ) as mock_discover, ): mock_llg.hf.from_tokenizer.return_value = MagicMock(vocab_size=32000) backend = LocalHFBackend( @@ -343,21 +368,19 @@ def test_load_embedded_adapters_registers_checkpoint_adapters(): adapter_source="/tmp/switch-checkpoint", ) - mock_from_source.assert_called_once_with( + mock_discover.assert_called_once_with( "/tmp/switch-checkpoint", revision="main", cache_dir=None, intrinsic_name=None ) assert backend.list_adapters() == ["answerability_alora"] - # 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. + # register_embedded_adapter_model discovers via the _discover_embedded_adapters + # factory (Epic #929, issue #1144) and registers the composed Adapter it + # returns into _composed_adapters. 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. + # add_adapter stamps the registration-time base-model name onto the shared + # EmbeddedBinding. assert adapter.weights.source == backend.base_model_name backend._model.load_adapter.assert_not_called() @@ -509,9 +532,9 @@ async def test_embedded_intrinsic_activates_template_without_peft(stub_backend): """Embedded calls select the template control token without PEFT lifecycle work.""" backend = _make_intrinsic_backend_stub(stub_backend) adapter = _make_embedded_adapter_stub() - backend._added_adapters = {adapter.qualified_name: adapter} + _register_adapter_stub(backend, adapter) peft_scope = MagicMock() - backend._generate_intrinsic_with_adapter_scope = peft_scope + backend._generate_composed_local_file_with_adapter_scope = peft_scope captured: dict[str, object] = {} def fake_transformers_inputs(request, tokenizer, model, ll_tokenizer=None): @@ -576,7 +599,7 @@ async def _run_embedded_intrinsic_and_collect_invocation_payloads( backend = _make_intrinsic_backend_stub(stub_backend) backend.processing = AsyncMock(return_value=None) adapter = _make_embedded_adapter_stub() - backend._added_adapters = {adapter.qualified_name: adapter} + _register_adapter_stub(backend, adapter) def fake_transformers_inputs(request, tokenizer, model, ll_tokenizer=None): return {"input_tokens": object()}, {} @@ -717,7 +740,7 @@ async def test_embedded_intrinsic_invocation_complete_fires_error_on_generation_ backend = _make_intrinsic_backend_stub(stub_backend) backend.processing = AsyncMock(return_value=None) adapter = _make_embedded_adapter_stub() - backend._added_adapters = {adapter.qualified_name: adapter} + _register_adapter_stub(backend, adapter) def fake_transformers_inputs(request, tokenizer, model, ll_tokenizer=None): return {"input_tokens": object()}, {} @@ -780,7 +803,7 @@ async def test_legacy_peft_intrinsic_never_fires_embedded_invocation_complete( backend = _make_intrinsic_backend_stub(stub_backend) backend.processing = AsyncMock(return_value=None) adapter = _make_intrinsic_adapter_stub() - backend._added_adapters = {adapter.qualified_name: adapter} + _register_adapter_stub(backend, adapter) def fake_transformers_inputs(request, tokenizer, model, ll_tokenizer=None): return {"input_tokens": object()}, {} @@ -838,8 +861,8 @@ def transform(self, chunk, rewritten): @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 + """A composed `Adapter` 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). """ @@ -1402,10 +1425,10 @@ def test_composed_local_file_config_is_derived_once_at_registration_not_on_the_l 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`. + rather than once synchronously at registration. 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, @@ -1470,27 +1493,44 @@ def test_generate_embedded_with_generation_lock_deactivates_peft_state(): mock_deactivate.assert_called_once_with("") -def test_add_embedded_adapter_rejects_mutated_weights_without_binding_backend(): - """A malformed shim must not retain this backend after registration fails.""" +def test_add_adapter_rejects_unsupported_weights_reality(): + """A composed Adapter whose weights the backend cannot stage must be + refused, and must leave no registration behind. + + `LocalHFBackend` supports only `LocalFileBinding` (PEFT) and + `EmbeddedBinding` (Granite Switch) weights realities. A composed Adapter + carrying a `ServerMediatedBinding` reaches `add_adapter` intact — the + frozen dataclass admits it at construction — so the backend rejects it at + registration rather than partially wiring it up. + """ + from mellea.backends.adapters._core import Adapter as _AdapterCore, Identity + from mellea.backends.adapters.io_contracts import get_io_contract + backend = _make_backend() - adapter = _make_embedded_adapter_stub() - adapter.weights = ServerMediatedBinding() + adapter = _AdapterCore( + identity=Identity( + name="answerability", adapter_type="alora", capability="answerability" + ), + io_contract=get_io_contract("answerability"), + weights=ServerMediatedBinding(), + ) - with pytest.raises(TypeError, match="must be an EmbeddedBinding"): + with pytest.raises(TypeError, match="ServerMediatedBinding weights reality"): backend.add_adapter(adapter) - assert adapter.backend is None - assert adapter.qualified_name not in backend._added_adapters + assert "answerability_alora" not in backend._added_adapters + assert "answerability_alora" not in backend._composed_adapters def test_load_peft_adapter_rejects_embedded_adapter(): """Embedded adapters are selected by the chat template, not loaded by PEFT.""" backend = _make_backend() adapter = _make_embedded_adapter_stub() - backend.add_adapter(adapter) + backend.add_adapter(adapter, config=_EMBEDDED_STUB_CONFIG) + key = _composed_adapter_key(adapter) with pytest.raises(TypeError, match="through PEFT"): - backend.load_peft_adapter(adapter.qualified_name) + backend.load_peft_adapter(key) backend._model.load_adapter.assert_not_called() @@ -1503,7 +1543,8 @@ def test_generate_with_adapter_lock_deactivates_and_calls_generate_func(): `_model.set_adapter` directly, Epic #929 Phase 2 / issue #1141), never touches the activation verbs or `load_peft_adapter`, and forwards to `generate_func` (its return value is the method's). Since #1465 routed - intrinsic generation through `_generate_intrinsic_with_adapter_scope`, no + intrinsic generation through + `_generate_composed_local_file_with_adapter_scope`, no production caller passes it an adapter to activate — which is why the method takes no adapter name at all. The method's deactivate-then-generate ordering is fixed by its body, not observable from these patched verbs. @@ -1578,7 +1619,8 @@ def test_adapter_activation_lock_is_the_generation_lock(): def test_generation_lock_is_reentrant(): """`_generation_lock` must be a `threading.RLock`, not a plain `threading.Lock`. - `_generate_intrinsic_with_adapter_scope` (issue #1465) holds `_generation_lock` + `_generate_composed_local_file_with_adapter_scope` (issue #1465) holds + `_generation_lock` for the whole prepare -> activate -> generate -> deactivate critical section on one thread, and the binding's verb calls (prepare/activate/deactivate) re-acquire the same lock (via `_adapter_activation_lock()`) from inside that @@ -1603,7 +1645,8 @@ def test_generation_lock_reentrant_activation_does_not_deadlock(): `activate()`/`deactivate()` (driven by `adapter_scope()`) acquire `_adapter_activation_lock()`, which is `_generation_lock`. Intrinsic generation holds `_generation_lock` for its whole critical section (see - `_generate_intrinsic_with_adapter_scope`), so that inner acquisition happens + `_generate_composed_local_file_with_adapter_scope`), so that inner acquisition + happens on the same thread while the outer one is still held. Runs on a background thread with a bounded join so a regression to a non-reentrant lock fails fast instead of hanging the suite. @@ -1626,49 +1669,36 @@ def nested_acquire(): ) -def _make_fake_intrinsic_adapter( - qualified_name: str, revision: str = "fake0000000000000000000000000000000000000" -) -> IntrinsicAdapter: - """Builds a minimal `IntrinsicAdapter` stand-in, bypassing `__init__` (which - downloads the adapter's `io.yaml`), exposing the attribute set - `_generate_intrinsic_with_adapter_scope` reads — `.identity` (reused - directly, not rebuilt, so this must already be the real `Identity` - `IntrinsicAdapter.__init__` would have built), `.qualified_name`, and - `.intrinsic_metadata.revision` — plus `.name`/`.adapter_type` for realism - (the method reaches those via `identity`, not the stand-in's own fields). - The stand-in pins `revision` to a catalogue SHA, mirroring the real - `IntrinsicsCatalogEntry.revision` (a required, non-optional `str`) rather - than allowing `None`. +def _make_composed_local_file_adapter( + backend: LocalHFBackend, + qualified_name: str, + revision: str = "fake0000000000000000000000000000000000000", +) -> _AdapterCore: + """Build a composed LocalFile `Adapter` with its binding wired to `backend`. + + `_generate_composed_local_file_with_adapter_scope` drives `adapter_scope` + directly on a binding that `add_adapter`/`resolve_adapter` already prepared, + so — like `test_generate_composed_local_file_with_adapter_scope_*` above — + this pre-wires the `LocalFileBinding` (`.backend`, `.path`, `._loaded`) + instead of relying on a `load_peft_adapter` call inside the scope. The + `revision` is pinned to a catalogue-shaped SHA, mirroring a real + `LocalFileBinding.revision`. """ name, _, adapter_type_str = qualified_name.rpartition("_") adapter_type = ( AdapterType.ALORA if adapter_type_str == "alora" else AdapterType.LORA ) - adapter = IntrinsicAdapter.__new__(IntrinsicAdapter) - adapter.name = name - adapter.qualified_name = qualified_name - adapter.adapter_type = adapter_type - adapter.intrinsic_metadata = IntrinsicsCatalogEntry( - name=name, repo_id="fake/repo", revision=revision + binding = LocalFileBinding( + name=name, adapter_type=adapter_type, repo_id="fake/repo", revision=revision ) - object.__setattr__( - adapter, - "identity", - Identity(name=name, adapter_type=adapter_type.value, capability=name), + binding.backend = backend + binding.path = "/fake/path" + binding._loaded = True + return _AdapterCore( + identity=Identity(name=name, adapter_type=adapter_type.value, capability=name), + io_contract=get_io_contract(name), + weights=binding, ) - return adapter - - -def _register_fake_adapter( - backend: LocalHFBackend, qualified_name: str, path: str -) -> None: - """Registers a minimal `IntrinsicAdapter` stand-in under `_added_adapters`, - satisfying what `load_peft_adapter` reads (`.path`, `.qualified_name`). - """ - fake = IntrinsicAdapter.__new__(IntrinsicAdapter) - fake.path = path - fake.qualified_name = qualified_name - backend._added_adapters[qualified_name] = fake def _wire_fake_peft_model(backend: LocalHFBackend) -> None: @@ -1698,7 +1728,7 @@ def test_generate_composed_local_file_with_adapter_scope_activates_during_genera 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. + logic would otherwise have no direct coverage of its own. """ from mellea.backends.adapters._core import ( Adapter as _AdapterCore, @@ -1742,51 +1772,26 @@ def fake_generate(): 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 - smoke-tested by checking generation merely succeeds. - """ - backend = _make_backend() - _wire_fake_peft_model(backend) - adapter = _make_fake_intrinsic_adapter("answerability_alora") - _register_fake_adapter(backend, adapter.qualified_name, "/fake/path") - - seen_during_generation = [] - - def fake_generate(): - seen_during_generation.append(backend._model.active_adapters()) - return "output" - - out = backend._generate_intrinsic_with_adapter_scope(adapter, fake_generate) - - assert out == "output" - assert seen_during_generation == [[adapter.qualified_name]] - assert backend._model.active_adapters() == [] - # Asserts the real verb ran deactivation, not just that _wire_fake_peft_model's - # `active_adapters()` mock still happens to read back empty. - backend._model.set_adapter.assert_any_call([]) # type: ignore[attr-defined] - - -def test_generate_intrinsic_with_adapter_scope_fires_hooks_with_correct_payload(): - """The hooks `_generate_intrinsic_with_adapter_scope`'s docstring claims to - enable must actually fire, with a payload that matches reality — not just - smoke-tested by checking that *some* hooks fire. +def test_generate_composed_local_file_with_adapter_scope_fires_hooks_with_correct_payload(): + """The hooks `_generate_composed_local_file_with_adapter_scope` enables must + actually fire, with a payload that matches reality — not just smoke-tested by + checking that *some* hooks fire. Regression coverage for two bugs caught in review: `revision` reported as `None` (mislabelled "unpinned") despite the adapter being pinned, and - `binding_type` set to an invented `"intrinsic_legacy"` value instead of the - `"local_file"` reality this binding actually is. + `binding_type` set to an invented value instead of the `"local_file"` + reality this binding actually is. """ backend = _make_backend() _wire_fake_peft_model(backend) - adapter = _make_fake_intrinsic_adapter( - "answerability_alora", revision="deadbeef00000000000000000000000000000000" + adapter = _make_composed_local_file_adapter( + backend, "answerability_alora", revision="deadbeef" + "0" * 32 ) - _register_fake_adapter(backend, adapter.qualified_name, "/fake/path") with capture_adapter_hooks() as mock_invoke: - out = backend._generate_intrinsic_with_adapter_scope(adapter, lambda: "output") + out = backend._generate_composed_local_file_with_adapter_scope( + adapter, lambda: "output" + ) assert out == "output" payloads = hook_payloads(mock_invoke) @@ -1801,48 +1806,50 @@ def test_generate_intrinsic_with_adapter_scope_fires_hooks_with_correct_payload( assert invocation.name == "answerability" assert invocation.adapter_type == "alora" assert invocation.binding_type == "local_file" - assert invocation.revision == "deadbeef00000000000000000000000000000000" + assert invocation.revision == "deadbeef" + "0" * 32 -def test_generate_intrinsic_with_adapter_scope_deactivates_on_error(): - """`adapter_scope()`'s deactivate-in-finally guarantee holds on the intrinsic path.""" +def test_generate_composed_local_file_with_adapter_scope_deactivates_on_error(): + """`adapter_scope()`'s deactivate-in-finally guarantee holds on the composed path.""" backend = _make_backend() _wire_fake_peft_model(backend) - adapter = _make_fake_intrinsic_adapter("answerability_alora") - _register_fake_adapter(backend, adapter.qualified_name, "/fake/path") + adapter = _make_composed_local_file_adapter(backend, "answerability_alora") def failing_generate(): raise RuntimeError("boom") with pytest.raises(RuntimeError, match="boom"): - backend._generate_intrinsic_with_adapter_scope(adapter, failing_generate) + backend._generate_composed_local_file_with_adapter_scope( + adapter, failing_generate + ) assert backend._model.active_adapters() == [] backend._model.set_adapter.assert_any_call([]) # type: ignore[attr-defined] -def test_generate_intrinsic_with_adapter_scope_reports_error_outcome_in_hook_payload(): - """A failed intrinsic generation reports `outcome="error"` with both phase events. +def test_generate_composed_local_file_with_adapter_scope_reports_error_outcome_in_hook_payload(): + """A failed composed generation reports `outcome="error"` with both phase events. The `AdapterMixin.adapter_scope` contract for a failing body is pinned generically (against a `LocalFileBinding`) in `test_adapter_scope.py`; this - pins it with the intrinsic payload — `name`, `adapter_type`, `binding_type` - and the pinned `revision` — so a regression can't silently mislabel - intrinsic failures while the generic coverage keeps passing. + pins it with the composed-adapter payload — `name`, `adapter_type`, + `binding_type` and the pinned `revision` — so a regression can't silently + mislabel these failures while the generic coverage keeps passing. """ backend = _make_backend() _wire_fake_peft_model(backend) - adapter = _make_fake_intrinsic_adapter( - "answerability_alora", revision="deadbeef00000000000000000000000000000000" + adapter = _make_composed_local_file_adapter( + backend, "answerability_alora", revision="deadbeef" + "0" * 32 ) - _register_fake_adapter(backend, adapter.qualified_name, "/fake/path") def failing_generate(): raise RuntimeError("boom") with capture_adapter_hooks() as mock_invoke: with pytest.raises(RuntimeError, match="boom"): - backend._generate_intrinsic_with_adapter_scope(adapter, failing_generate) + backend._generate_composed_local_file_with_adapter_scope( + adapter, failing_generate + ) payloads = hook_payloads(mock_invoke) phases = [p.phase for p in payloads if hasattr(p, "phase")] @@ -1856,36 +1863,8 @@ def failing_generate(): assert invocation.name == "answerability" assert invocation.adapter_type == "alora" assert invocation.binding_type == "local_file" - assert invocation.revision == "deadbeef00000000000000000000000000000000" - - assert backend._model.active_adapters() == [] - - -def test_generate_intrinsic_with_adapter_scope_fires_no_hooks_on_prepare_failure(): - """A `load_peft_adapter` failure happens before `adapter_scope()` is entered. - - So no adapter hooks fire at all, activation state is unchanged, and the - next call still succeeds — a failed load must not poison later calls. - """ - backend = _make_backend() - _wire_fake_peft_model(backend) - adapter = _make_fake_intrinsic_adapter("answerability_alora") - _register_fake_adapter(backend, adapter.qualified_name, "/fake/path") - - with ( - capture_adapter_hooks() as mock_invoke, - patch.object( - backend, "load_peft_adapter", side_effect=RuntimeError("load failed") - ), - ): - with pytest.raises(RuntimeError, match="load failed"): - backend._generate_intrinsic_with_adapter_scope(adapter, lambda: "output") + assert invocation.revision == "deadbeef" + "0" * 32 - assert hook_payloads(mock_invoke) == [] - assert backend._model.active_adapters() == [] - - out = backend._generate_intrinsic_with_adapter_scope(adapter, lambda: "output") - assert out == "output" assert backend._model.active_adapters() == [] @@ -1902,14 +1881,12 @@ def test_concurrent_intrinsic_calls_cannot_observe_each_others_adapter(): """ backend = _make_backend() _wire_fake_peft_model(backend) - _register_fake_adapter(backend, "answerability_lora", "/fake/a") - _register_fake_adapter(backend, "uncertainty_lora", "/fake/b") mismatches: list[tuple[str, list[str]]] = [] errors: list[BaseException] = [] def run(qualified_name: str): - adapter = _make_fake_intrinsic_adapter(qualified_name) + adapter = _make_composed_local_file_adapter(backend, qualified_name) def fake_generate(): time.sleep(0.05) @@ -1919,7 +1896,9 @@ def fake_generate(): return "ok" try: - backend._generate_intrinsic_with_adapter_scope(adapter, fake_generate) + backend._generate_composed_local_file_with_adapter_scope( + adapter, fake_generate + ) except Exception as exc: # surfaced via `errors`, not swallowed errors.append(exc) @@ -1934,7 +1913,8 @@ def fake_generate(): assert not any(t.is_alive() for t in threads) # A `Thread` swallows exceptions from its target by default, so without this - # check a totally broken `_generate_intrinsic_with_adapter_scope` (e.g. an + # check a totally broken + # `_generate_composed_local_file_with_adapter_scope` (e.g. an # AttributeError before `fake_generate` ever runs) would leave `mismatches` # empty and this test would pass vacuously. assert errors == [] @@ -1946,14 +1926,13 @@ def test_list_adapters_reflects_registration_not_just_loading(): if they've never been loaded (aligns HF's semantics with OpenAI's). """ backend = _make_backend() - adapter = _make_intrinsic_adapter_stub() - adapter.backend = None - adapter.get_local_hf_path = lambda base_model_name: "/fake/path" + adapter = _make_embedded_adapter_stub() + key = _composed_adapter_key(adapter) - backend.add_adapter(adapter) + backend.add_adapter(adapter, config=_EMBEDDED_STUB_CONFIG) - assert adapter.qualified_name not in backend._loaded_adapters - assert adapter.qualified_name in backend.list_adapters() + assert key not in backend._loaded_adapters + assert key in backend.list_adapters() def test_add_non_local_hf_adapter_raises(): @@ -1968,16 +1947,15 @@ def test_add_non_local_hf_adapter_raises(): def test_remove_adapter_removes_from_added_adapters(): """remove_adapter() is the inverse of add_adapter() (#1528).""" 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) - assert adapter.qualified_name in backend.list_adapters() + adapter = _make_embedded_adapter_stub() + key = _composed_adapter_key(adapter) + backend.add_adapter(adapter, config=_EMBEDDED_STUB_CONFIG) + assert key in backend.list_adapters() - backend.remove_adapter(adapter.qualified_name) + backend.remove_adapter(key) - assert adapter.qualified_name not in backend.list_adapters() - assert adapter.qualified_name not in backend._added_adapters + assert key not in backend.list_adapters() + assert key not in backend._composed_adapters def test_remove_adapter_unregistered_name_is_noop(): @@ -2138,15 +2116,15 @@ def test_add_adapter_rejects_composed_adapter_with_mismatched_identity_and_weigh 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. +def test_add_adapter_bare_binding_refuses_name_already_claimed_by_composed_adapter(): + """A bare add_adapter path must see composed-adapter registrations too. - Regression: the duplicate-name guard on the shim path only checked + Regression: the duplicate-name guard on the bare-binding 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. + registered under a qualified name did not stop a later 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, @@ -2166,14 +2144,15 @@ def test_add_adapter_shim_refuses_name_already_claimed_by_composed_adapter(): 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") + 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(shim) + backend.add_adapter(binding) - assert shim.backend is None - assert "answerability_alora" not in backend._added_adapters + assert binding.backend is None + assert backend._added_adapters.get("answerability_alora") is not binding def test_register_embedded_adapter_model_refused_duplicate_does_not_clobber_cached_config(): @@ -2345,14 +2324,11 @@ def test_add_adapter_after_remove_adapter_allows_a_fresh_registration(): backend's lifetime. """ backend = _make_backend() - first = _make_intrinsic_adapter_stub() - first.backend = None - first.get_local_hf_path = lambda base_model_name: "/fake/path" + first = _make_local_file_binding_stub() backend.add_adapter(first) backend.remove_adapter(first.qualified_name) - second = _make_intrinsic_adapter_stub() - second.backend = None + second = _make_local_file_binding_stub() second.get_local_hf_path = lambda base_model_name: "/fake/path-2" backend.add_adapter(second) @@ -2371,9 +2347,7 @@ def test_remove_adapter_clears_backend_and_path_references(): re-registration anywhere (see the next test). """ backend = _make_backend() - adapter = _make_intrinsic_adapter_stub() - adapter.backend = None - adapter.get_local_hf_path = lambda base_model_name: "/fake/path" + adapter = _make_local_file_binding_stub() backend.add_adapter(adapter) assert adapter.backend is backend assert adapter.path == "/fake/path" @@ -2392,9 +2366,7 @@ def test_add_adapter_after_remove_adapter_allows_reregistering_the_same_object() silent no-op, never re-registered, with no exception raised. """ backend = _make_backend() - adapter = _make_intrinsic_adapter_stub() - adapter.backend = None - adapter.get_local_hf_path = lambda base_model_name: "/fake/path" + adapter = _make_local_file_binding_stub() backend.add_adapter(adapter) backend.remove_adapter(adapter.qualified_name) @@ -2417,9 +2389,7 @@ def test_remove_adapter_raises_if_still_loaded(): can succeed. """ backend = _make_backend() - adapter = _make_intrinsic_adapter_stub() - adapter.backend = None - adapter.get_local_hf_path = lambda base_model_name: "/fake/path" + adapter = _make_local_file_binding_stub() backend.add_adapter(adapter) backend.load_peft_adapter(adapter.qualified_name) assert adapter.qualified_name in backend._loaded_adapters @@ -2451,9 +2421,7 @@ def test_load_peft_adapter_records_loaded_state_before_set_adapter_failure(): 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" + adapter = _make_local_file_binding_stub() backend.add_adapter(adapter) backend._model.set_adapter.side_effect = RuntimeError("PEFT set_adapter failure") # type: ignore[union-attr] @@ -2552,7 +2520,7 @@ def test_seed_sentinel_is_stripped(stub_backend): async def test_intrinsic_seed_with_zero_temperature_keeps_greedy(stub_backend): """The intrinsic path must not let seed override explicit temperature=0.""" backend = _make_intrinsic_backend_stub(stub_backend) - adapter = _make_intrinsic_adapter_stub() + adapter = _make_embedded_adapter_stub() captured = {} def fake_transformers_inputs(rewritten, tokenizer, model, ll_tokenizer=None): @@ -2565,7 +2533,7 @@ def fake_generate_with_transformers(tokenizer, model, generate_input, other_inpu return object() # Pre-populate the adapter so the capability-based lookup finds it. - backend._added_adapters = {adapter.qualified_name: adapter} + _register_adapter_stub(backend, adapter) with ( patch( @@ -2993,8 +2961,8 @@ async def test_intrinsic_logits_populated_when_option_set(stub_backend): backend._tokenizer = MagicMock(eos_token_id=0) backend.model_id = "stub-model" - adapter = _make_intrinsic_adapter_stub() - backend._added_adapters = {adapter.qualified_name: adapter} + adapter = _make_embedded_adapter_stub() + _register_adapter_stub(backend, adapter) class _FakeChatCompletionResponse: class _Choice: @@ -3142,8 +3110,8 @@ async def test_intrinsic_closure_cell_and_kv_cache_released_after_post_processin if t is not None ] - adapter = _make_intrinsic_adapter_stub() - backend._added_adapters = {adapter.qualified_name: adapter} + adapter = _make_embedded_adapter_stub() + _register_adapter_stub(backend, adapter) class _FakeChatCompletionResponse: class _Choice: @@ -3420,8 +3388,8 @@ async def test_multimodal_blocks_in_intrinsic_ctx_raise_error( this test exercises that branch directly. """ backend = _make_intrinsic_backend_stub(stub_backend) - adapter = _make_intrinsic_adapter_stub() - backend._added_adapters = {adapter.qualified_name: adapter} + adapter = _make_embedded_adapter_stub() + _register_adapter_stub(backend, adapter) ctx = ChatContext().add(Message("user", "Hello", images=images, audio=audio)) with pytest.raises(ValueError, match="LocalHFBackend does not support"): diff --git a/test/backends/test_openai_intrinsics_unit.py b/test/backends/test_openai_intrinsics_unit.py index 6d488b4237..b92d89a06b 100644 --- a/test/backends/test_openai_intrinsics_unit.py +++ b/test/backends/test_openai_intrinsics_unit.py @@ -31,7 +31,7 @@ from openai.types.completion_usage import CompletionUsage from mellea.backends import ModelOption -from mellea.backends.adapters.adapter import EmbeddedIntrinsicAdapter +from mellea.backends.adapters.adapter import _embedded_adapter_from_entry from mellea.backends.openai import OpenAIBackend from mellea.core import ModelOutputThunk, Requirement from mellea.stdlib import functional as mfuncs @@ -168,10 +168,10 @@ def _make_backend_with_adapter( base_url="http://localhost:9999/v1", model_options=model_options, ) - adapter = EmbeddedIntrinsicAdapter( + adapter = _embedded_adapter_from_entry( intrinsic_name="answerability", config=config, technology="alora" ) - backend.add_adapter(adapter) + backend.add_adapter(adapter, config=config) return backend @@ -181,12 +181,12 @@ def _make_context() -> ChatContext: def _make_backend_with_composed_adapter(config: dict) -> OpenAIBackend: - """Return an OpenAIBackend with a composed `Adapter` registered directly. + """Return an OpenAIBackend with a composed `Adapter` built by hand. - 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. + Builds the composed `Adapter` directly (rather than through + `_embedded_adapter_from_entry`, as `_make_backend_with_adapter` does) so + the hand-constructed identity/io_contract/weights path is exercised too + (Epic #929, issue #1144). """ from mellea.backends.adapters._core import ( Adapter as _AdapterCore, @@ -245,9 +245,8 @@ async def test_chat_template_kwargs_set(): 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).""" + """A hand-constructed composed `Adapter` drives the full generation + path — activation, name/config resolution (Epic #929, issue #1144).""" backend = _make_backend_with_composed_adapter(_SIMPLE_CONFIG) ctx = _make_context() mock_create = AsyncMock(return_value=_simple_chat_completion()) @@ -385,28 +384,25 @@ async def test_no_adapter_raises_valueerror(): def test_remove_adapter_frees_name_for_reuse(): - """Removing a shim adapter frees its name and clears its backend reference.""" + """Removing a composed adapter frees its name for a fresh registration.""" backend = _make_backend_with_adapter(_SIMPLE_CONFIG) qualified_name = "answerability_alora" first = backend._added_adapters[qualified_name] - replacement = EmbeddedIntrinsicAdapter( + replacement = _embedded_adapter_from_entry( intrinsic_name="answerability", config=deepcopy(_SIMPLE_CONFIG), technology="alora", ) # While the name is occupied, the registration is refused. - backend.add_adapter(replacement) + backend.add_adapter(replacement, config=deepcopy(_SIMPLE_CONFIG)) assert backend._added_adapters[qualified_name] is first - assert replacement.backend is None backend.remove_adapter(qualified_name) assert qualified_name not in backend.list_adapters() - assert isinstance(first, EmbeddedIntrinsicAdapter) - assert first.backend is None - backend.add_adapter(replacement) + backend.add_adapter(replacement, config=deepcopy(_SIMPLE_CONFIG)) assert backend._added_adapters[qualified_name] is replacement @@ -916,11 +912,12 @@ def _make_backend_with_requirement_adapter() -> OpenAIBackend: base_url="http://localhost:9999/v1", ) backend.add_adapter( - EmbeddedIntrinsicAdapter( + _embedded_adapter_from_entry( intrinsic_name="requirement-check", config=deepcopy(_SIMPLE_CONFIG), technology="alora", - ) + ), + config=deepcopy(_SIMPLE_CONFIG), ) return backend diff --git a/test/package/test_dependency_isolation.py b/test/package/test_dependency_isolation.py index f0e2c3fb58..c8fd558567 100644 --- a/test/package/test_dependency_isolation.py +++ b/test/package/test_dependency_isolation.py @@ -49,7 +49,7 @@ "from mellea.backends import Backend, ModelIdentifier, ModelOption", "from mellea.backends.ollama import OllamaModelBackend", "from mellea.backends.openai import OpenAIBackend", - "from mellea.backends.adapters.adapter import EmbeddedIntrinsicAdapter", + "from mellea.backends.adapters.adapter import AdapterMixin", "from mellea.core import Backend", ], "hf": ["from mellea.backends.huggingface import LocalHFBackend"], diff --git a/test/stdlib/components/intrinsic/test_intrinsic.py b/test/stdlib/components/intrinsic/test_intrinsic.py index 3d9ef4cf3f..fe38dccc0f 100644 --- a/test/stdlib/components/intrinsic/test_intrinsic.py +++ b/test/stdlib/components/intrinsic/test_intrinsic.py @@ -12,9 +12,8 @@ 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`. + 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): From 7b72102ba5379e6db0ba94fa3bfe82c5f82e15f5 Mon Sep 17 00:00:00 2001 From: AngeloDanducci Date: Fri, 23 Oct 2026 15:55:32 -0400 Subject: [PATCH 2/2] rebase and review feedback Signed-off-by: AngeloDanducci --- mellea/backends/adapters/_core.py | 24 +- mellea/backends/adapters/adapter.py | 6 +- mellea/backends/huggingface.py | 38 +- mellea/backends/openai.py | 6 +- .../test_adapters/test_activation_guard.py | 36 +- .../test_embedded_integration.py | 2 +- .../test_adapters/test_local_file_binding.py | 55 ++ .../test_adapters/test_resolve_locking.py | 626 ++++++++++++++++++ test/backends/test_huggingface.py | 46 +- test/backends/test_huggingface_unit.py | 67 +- test/backends/test_openai_intrinsics_unit.py | 10 +- 11 files changed, 769 insertions(+), 147 deletions(-) create mode 100644 test/backends/test_adapters/test_resolve_locking.py diff --git a/mellea/backends/adapters/_core.py b/mellea/backends/adapters/_core.py index ed2ceacdc4..a3b08163af 100644 --- a/mellea/backends/adapters/_core.py +++ b/mellea/backends/adapters/_core.py @@ -691,6 +691,16 @@ def hold_prepared(self, backend: "AdapterMixin") -> Iterator[None]: def activate(self) -> None: """Selects already-loaded adapter weights for generation. + Resilient to the weights having been dropped from the backend's PEFT + model since `prepare()`: `prepare()` injects the weights once, but an + `unload_peft_adapter()` call (e.g. to free GPU memory, or as a test + teardown) deletes them from the PEFT model while leaving this binding's + registration intact. Re-injecting them here — rather than treating + `prepare()` as a one-time guarantee — lets a composed `Adapter` be + unloaded and later re-used on the same backend without a hard failure + (the resilience the removed `IntrinsicAdapter` shim gave by re-loading + on demand). + Raises: RuntimeError: `prepare()` was not called first, or called but did not complete (registered with the backend but the weights @@ -706,7 +716,19 @@ def activate(self) -> None: raise RuntimeError( "LocalFileBinding.activate() requires prepare() to be called first." ) - backend.activate_peft_adapter(self.qualified_name) + try: + backend.activate_peft_adapter(self.qualified_name) + except ValueError: + # The weights were dropped from the PEFT model since prepare() + # (an intervening unload_peft_adapter), so set_adapter() cannot + # find the name. The registration in `_added_adapters` survives + # an unload, so load_peft_adapter() re-injects it; retry once. + # Both verbs mutate PEFT model state the activation lock already + # held here guards, so no new lock is taken — re-loading under + # `_lifecycle_lock` (as prepare() does) would invert the adapter + # lock order against a concurrent release(). + backend.load_peft_adapter(self.qualified_name) + backend.activate_peft_adapter(self.qualified_name) self._active = True def deactivate(self) -> None: diff --git a/mellea/backends/adapters/adapter.py b/mellea/backends/adapters/adapter.py index f433969e38..95d42a3d32 100644 --- a/mellea/backends/adapters/adapter.py +++ b/mellea/backends/adapters/adapter.py @@ -24,7 +24,6 @@ from collections.abc import Callable, Sequence from typing import Literal, TypeAlias, TypeVar, cast - import yaml from ...core import Backend, MelleaLogger @@ -128,14 +127,13 @@ def _composed_adapter_key(adapter: "_AdapterCore") -> str: def _embedded_adapter_from_entry( - intrinsic_name: str, config: dict, technology: str + intrinsic_name: str, technology: str ) -> "_AdapterCore": """Build a composed embedded `Adapter` from one `adapter_index.json` entry. Args: intrinsic_name (str): Adapter function name from the index entry (e.g. `"answerability"`). - config (dict): Parsed `io.yaml` transformation configuration. technology (str): Adapter technology in the switch model — `"lora"` or `"alora"`. Determines the `Identity.adapter_type`. @@ -227,7 +225,7 @@ def _embedded_adapters_from_model_directory( config_dict = yaml.safe_load(f) adapter = _embedded_adapter_from_entry( - entry_name, config_dict, entry.get("technology", "lora") + entry_name, entry.get("technology", "lora") ) adapters.append((adapter, config_dict)) diff --git a/mellea/backends/huggingface.py b/mellea/backends/huggingface.py index e2029d9f7d..ea5ed66dac 100644 --- a/mellea/backends/huggingface.py +++ b/mellea/backends/huggingface.py @@ -442,48 +442,31 @@ def _check_no_multimodal_blocks(action: Span | None, ctx: Context | None) -> Non ) -def _alora_qualified_name_for_activation_check( - adapter: IntrinsicAdapter | EmbeddedIntrinsicAdapter | _AdapterCore, -) -> str | None: +def _alora_qualified_name_for_activation_check(adapter: _AdapterCore) -> str | None: """Return `adapter`'s PEFT-loaded qualified name, iff it is a LocalFile/PEFT aLoRA. `None` for anything the activation guard does not apply to: an - `EmbeddedIntrinsicAdapter` (activated by the chat template, not PEFT - `set_adapter`, so `peft_config` never carries it; whether that reality + embedded adapter (an `EmbeddedBinding`, activated by the chat template, not + PEFT `set_adapter`, so `peft_config` never carries it; whether that reality needs its own check is still open), or a LoRA adapter (no invocation sequence to check). Returning a qualified name here is necessary but not sufficient for the guard to actually see the adapter: the caller (`_generate_from_intrinsic`) checks `peft_config` for it immediately, before the weights are - necessarily loaded. That is fine for the default composed-`Adapter` path + necessarily loaded. That is fine for the composed-`Adapter` path (`resolve_adapter()` calls `binding.prepare()`, and therefore - `load_peft_adapter`, at registration time, well before generation), but - the deprecated `IntrinsicAdapter` shim only loads inside - `_generate_intrinsic_with_adapter_scope`, which runs *after* this check - — so the guard silently no-ops on a shim adapter's first call. Not fixed - here: the shim is scheduled for removal (#1621), and - forcing an early load would mean triggering a Hub round trip from this - check rather than from generation. + `load_peft_adapter`, at registration time, well before generation). Args: - adapter: The resolved adapter about to generate. + adapter: The resolved composed `Adapter` about to generate. Returns: str | None: The qualified name PEFT loaded this adapter's weights under, or `None` if the activation guard does not apply. """ - if isinstance(adapter, EmbeddedIntrinsicAdapter): - return None - # `.identity.adapter_type` (a `Literal["lora", "alora"]` string), not - # `.adapter_type` (an `AdapterType` enum member only `IntrinsicAdapter` - # carries): every adapter shape this method accepts — the shim and the - # composed `_AdapterCore` alike — sets `.identity`, so checking it once - # here covers both without a shape-specific attribute read. if adapter.identity.adapter_type != AdapterType.ALORA.value: return None - if isinstance(adapter, IntrinsicAdapter): - return adapter.qualified_name # Composed Adapter: LocalFile/PEFT reality only # — an EmbeddedBinding has no qualified_name in this sense (see the # docstring). @@ -1370,15 +1353,10 @@ async def _generate_from_intrinsic( # error signal, for exactly the scenario this guard exists # to catch. Mirrors `adapter_scope`'s own # revision/binding_type derivation (`adapter.py`) so the - # two emit consistent fields for the same adapter. The - # legacy shim's own `weights` is an inert placeholder, so - # report what its per-call `_IntrinsicPeftBinding` would. + # two emit consistent fields for the same adapter. revision: str | None binding_type = adapter.weights.binding_type - if isinstance(adapter, IntrinsicAdapter): - revision = adapter.intrinsic_metadata.revision - binding_type = _IntrinsicPeftBinding.binding_type - elif isinstance(adapter.weights, LocalFileBinding): + if isinstance(adapter.weights, LocalFileBinding): try: revision = adapter.weights.resolved_revision() except Exception: diff --git a/mellea/backends/openai.py b/mellea/backends/openai.py index 2289333ea1..e2c4d62165 100644 --- a/mellea/backends/openai.py +++ b/mellea/backends/openai.py @@ -524,10 +524,8 @@ def register_embedded_adapter_model( # to any other thread): this method is now also the documented # post-construction way to load embedded adapters from a Hub repo or # model directory 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 + # thread's `add_adapter`/`resolve_adapter` — `add_adapter`'s + # read-then-write across `_added_adapters` needs 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 diff --git a/test/backends/test_adapters/test_activation_guard.py b/test/backends/test_adapters/test_activation_guard.py index c40b30d634..78776cba91 100644 --- a/test/backends/test_adapters/test_activation_guard.py +++ b/test/backends/test_adapters/test_activation_guard.py @@ -39,11 +39,7 @@ Identity, LocalFileBinding, ) -from mellea.backends.adapters.adapter import ( - EmbeddedIntrinsicAdapter, - IntrinsicAdapter, - _alora_invocation_sequence_present, -) +from mellea.backends.adapters.adapter import _alora_invocation_sequence_present from mellea.backends.adapters.catalog import AdapterType # The requirement-check aLoRA's real declared invocation sequence (#1679): @@ -96,31 +92,13 @@ def _identity(adapter_type: str) -> Identity: class TestQualifiedNameForActivationCheck: - """`_alora_qualified_name_for_activation_check` gates which adapter shapes are checked.""" - - def test_intrinsic_adapter_alora_returns_qualified_name(self): - adapter = IntrinsicAdapter.__new__(IntrinsicAdapter) - adapter.qualified_name = "requirement-check_alora" - object.__setattr__(adapter, "identity", _identity("alora")) - - result = huggingface._alora_qualified_name_for_activation_check(adapter) - assert result == "requirement-check_alora" - - def test_intrinsic_adapter_lora_is_skipped(self): - adapter = IntrinsicAdapter.__new__(IntrinsicAdapter) - adapter.qualified_name = "requirement-check_lora" - object.__setattr__(adapter, "identity", _identity("lora")) + """`_alora_qualified_name_for_activation_check` gates which adapter shapes are checked. - assert huggingface._alora_qualified_name_for_activation_check(adapter) is None - - def test_embedded_adapter_is_always_skipped(self): - with pytest.warns(DeprecationWarning): - adapter = EmbeddedIntrinsicAdapter( - intrinsic_name="requirement-check", - config={"parameters": {}}, - technology="alora", - ) - assert huggingface._alora_qualified_name_for_activation_check(adapter) is None + The guard now accepts only the composed `Adapter` (`_AdapterCore`) that + `_find_adapter` returns — the deprecated `IntrinsicAdapter` / + `EmbeddedIntrinsicAdapter` shim shapes it also handled were removed with the + shims in PR #1700, so the cases below cover every shape the guard can see. + """ def test_composed_adapter_alora_with_local_file_binding(self): binding = LocalFileBinding( diff --git a/test/backends/test_adapters/test_embedded_integration.py b/test/backends/test_adapters/test_embedded_integration.py index 4046f74ea9..3ca61011f5 100644 --- a/test/backends/test_adapters/test_embedded_integration.py +++ b/test/backends/test_adapters/test_embedded_integration.py @@ -65,7 +65,7 @@ def _backend_with_adapter(technology: str) -> OpenAIBackend: ) backend.add_adapter( _embedded_adapter_from_entry( - intrinsic_name="answerability", config=_SIMPLE_CONFIG, technology=technology + intrinsic_name="answerability", technology=technology ), config=_SIMPLE_CONFIG, ) diff --git a/test/backends/test_adapters/test_local_file_binding.py b/test/backends/test_adapters/test_local_file_binding.py index eec7832e8e..62f985f0da 100644 --- a/test/backends/test_adapters/test_local_file_binding.py +++ b/test/backends/test_adapters/test_local_file_binding.py @@ -337,6 +337,61 @@ def test_activate_delegates_to_backend_verb(): backend.activate_peft_adapter.assert_called_once_with(binding.qualified_name) +def test_activate_reinjects_weights_dropped_since_prepare(): + """`activate()` re-loads when the PEFT model dropped the weights after prepare(). + + An `unload_peft_adapter()` between `prepare()` and `activate()` deletes the + weights from the PEFT model while leaving the registration intact, so the + first `activate_peft_adapter()` (PEFT's `set_adapter`) raises `ValueError`. + `activate()` must re-load via `load_peft_adapter()` and retry, rather than + treat `prepare()`'s injection as a one-time guarantee — the unload/reuse + resilience the removed `IntrinsicAdapter` shim gave (PR #1700 follow-up). + """ + backend = _fake_backend() + binding = LocalFileBinding(name="answerability") + binding.bind_backend(backend) + binding.prepare() + backend.load_peft_adapter.reset_mock() + + # First set_adapter fails (weights dropped); the post-reload retry succeeds. + backend.activate_peft_adapter.side_effect = [ + ValueError( + f"Adapter with name {binding.qualified_name} not found. " + "Please pass the correct adapter name among []" + ), + None, + ] + + binding.activate() + + backend.load_peft_adapter.assert_called_once_with(binding.qualified_name) + assert backend.activate_peft_adapter.call_count == 2 + assert binding._active + + +def test_activate_propagates_when_reload_does_not_recover(): + """A `ValueError` that re-loading can't fix still propagates (no masking/loop). + + The retry re-loads once and re-activates once; if `set_adapter` still raises + (e.g. the name was never registered, not merely unloaded), the second + failure surfaces rather than being swallowed or retried forever. + """ + backend = _fake_backend() + binding = LocalFileBinding(name="answerability") + binding.bind_backend(backend) + binding.prepare() + backend.load_peft_adapter.reset_mock() + + backend.activate_peft_adapter.side_effect = ValueError("still not found") + + with pytest.raises(ValueError, match="still not found"): + binding.activate() + + backend.load_peft_adapter.assert_called_once_with(binding.qualified_name) + assert backend.activate_peft_adapter.call_count == 2 + assert not binding._active + + def test_deactivate_delegates_to_backend_verb(): backend = _fake_backend() binding = LocalFileBinding(name="answerability") diff --git a/test/backends/test_adapters/test_resolve_locking.py b/test/backends/test_adapters/test_resolve_locking.py new file mode 100644 index 0000000000..bbc70ff4d4 --- /dev/null +++ b/test/backends/test_adapters/test_resolve_locking.py @@ -0,0 +1,626 @@ +# Copyright IBM Corp. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Locking, race-safety, and resolution tests for `AdapterMixin` (Epic #929). + +These exercise production code in `mellea/backends/adapters/adapter.py` that is +untouched by the shim removal (PR #1700): `resolve_adapter`'s collision scan and +lazy-registration, `_find_adapter`'s snapshot-before-iterate guard, the +`_adapter_resolve_lock()` critical section, and type-preference ordering. They +were ported from the deleted `test_shims.py`, which used the removed +`IntrinsicAdapter`/`EmbeddedIntrinsicAdapter` shims only as convenient fixtures; +here the same paths are driven with composed `Adapter` (`_AdapterCore`) stubs +carrying `LocalFileBinding`/`EmbeddedBinding`, matching the fixture pattern in +`test_huggingface_unit.py`. +""" + +import threading +import time +import warnings +from unittest.mock import MagicMock, mock_open, patch + +import pytest + +from mellea.backends.adapters import Adapter, EmbeddedBinding, get_io_contract +from mellea.backends.adapters._core import ( + Adapter as _AdapterCore, + Identity, + LocalFileBinding, +) +from mellea.backends.adapters.adapter import AdapterMixin, _composed_adapter_key +from mellea.backends.adapters.catalog import AdapterType, IntrinsicsCatalogEntry + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +_MOCK_CATALOG_ENTRY = IntrinsicsCatalogEntry( + name="answerability", + repo_id="ibm-granite/granitelib-rag-r1.0", + revision="abc123deadbeef", + adapter_types=(AdapterType.ALORA, AdapterType.LORA), +) + + +def _make_composed_adapter( + intrinsic_name: str = "answerability", + technology: str = "alora", + capability: str | None = None, +) -> _AdapterCore: + """A composed embedded `Adapter` (EmbeddedBinding) stub. + + Replaces the deleted `EmbeddedIntrinsicAdapter`/`IntrinsicAdapter` shim + fixtures: carries the same `Identity` (name, type, capability) the resolver + and `_find_adapter` match on, without any deprecated shim machinery. + """ + return _AdapterCore( + identity=Identity( + name=intrinsic_name, + adapter_type=technology, # type: ignore[arg-type] + capability=capability or intrinsic_name, + ), + io_contract=get_io_contract(intrinsic_name), + weights=EmbeddedBinding(), + ) + + +# --------------------------------------------------------------------------- +# resolve_adapter / _find_adapter — basic resolution +# --------------------------------------------------------------------------- + + +def test_resolve_adapter_returns_existing_by_capability(): + """resolve_adapter must return an already-registered adapter without creating a new one.""" + existing = _make_composed_adapter("answerability") + mock_backend = MagicMock(spec=AdapterMixin) + mock_backend._added_adapters = {_composed_adapter_key(existing): existing} + # Route _find_adapter through the real implementation so the _added_adapters search runs. + mock_backend._find_adapter.side_effect = lambda cap, types=None: ( + AdapterMixin._find_adapter(mock_backend, cap, types) + ) + result = AdapterMixin.resolve_adapter(mock_backend, "answerability") + assert result is existing + mock_backend.add_adapter.assert_not_called() + + +def test_find_adapter_honours_type_preference_order(): + """_find_adapter must return the highest-priority type, not the insertion-order winner.""" + lora = _make_composed_adapter("answerability", technology="lora") + alora = _make_composed_adapter("answerability", technology="alora") + + mock_backend = MagicMock(spec=AdapterMixin) + # Register lora first so insertion order would return it without preference logic. + mock_backend._added_adapters = { + _composed_adapter_key(lora): lora, + _composed_adapter_key(alora): alora, + } + + result = AdapterMixin._find_adapter( + mock_backend, "answerability", ("alora", "lora") + ) + assert result is alora, "alora must win over lora regardless of insertion order" + + +def test_resolve_adapter_raises_without_base_model(): + """resolve_adapter must raise ValueError when the backend has no model ID.""" + mock_backend = MagicMock(spec=AdapterMixin) + mock_backend._added_adapters = {} + mock_backend.base_model_name = None + # _find_adapter returns None so resolve_adapter proceeds to the base-model check. + mock_backend._find_adapter.return_value = None + with pytest.raises(ValueError, match="no model ID"): + AdapterMixin.resolve_adapter(mock_backend, "answerability") + + +def test_resolve_adapter_lazy_creates_and_returns(): + """resolve_adapter must create a composed Adapter when none is registered. + + Epic #929, issue #1144: resolve_adapter's construction site builds a + composed `Adapter(identity, io_contract, LocalFileBinding)`, not the + deprecated `IntrinsicAdapter` shim. The default type is LORA (every catalog + entry supports it; Phase 2 will select from real catalog availability). + """ + mock_catalog_entry = IntrinsicsCatalogEntry( + name="answerability", + repo_id="ibm-granite/granitelib-rag-r1.0", + revision="abc123", + adapter_types=(AdapterType.ALORA, AdapterType.LORA), + ) + mock_backend = MagicMock(spec=AdapterMixin) + mock_backend.base_model_name = "ibm-granite/granite-4.1-3b" + mock_backend._uses_embedded_adapters = False + + created_adapters: list = [] + + def fake_add_adapter(a): + created_adapters.append(a) + mock_backend._added_adapters[_composed_adapter_key(a)] = a + + mock_backend._added_adapters = {} + 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, + ): + result = AdapterMixin.resolve_adapter(mock_backend, "answerability") + + assert mock_backend.add_adapter.called, ( + "add_adapter must be called for a new capability" + ) + assert len(created_adapters) == 1 + assert isinstance(created_adapters[0], Adapter) + assert isinstance(created_adapters[0].weights, LocalFileBinding) + assert created_adapters[0].weights.adapter_type == AdapterType.LORA + assert result is created_adapters[0] + + +def test_resolve_adapter_catalog_alias_returns_registered_adapter(): + """Regression (#1563): lookup accepts the catalog's public alias. + + The adapter stores `guardian_core` as its canonical capability, while + callers resolve it via the catalog name `guardian-core`, including when + they constrain the adapter type. + """ + mock_catalog_entry = IntrinsicsCatalogEntry( + name="guardian-core", + capability="guardian_core", + repo_id="ibm-granite/granitelib-guardian-r1.0", + revision="abc123", + adapter_types=(AdapterType.LORA,), + ) + mock_backend = MagicMock(spec=AdapterMixin) + mock_backend.base_model_name = "ibm-granite/granite-4.1-3b" + mock_backend._uses_embedded_adapters = False + mock_backend._added_adapters = {} + mock_backend.add_adapter.side_effect = lambda 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, + ): + result = AdapterMixin.resolve_adapter(mock_backend, "guardian-core") + + assert result.identity.name == "guardian-core" + assert result.identity.capability == "guardian_core" + assert ( + AdapterMixin._find_adapter(mock_backend, "guardian-core", ("lora",)) is result + ) + + +# --------------------------------------------------------------------------- +# Concurrent-mutation-during-iteration guards +# --------------------------------------------------------------------------- + + +class _MutatingCapability: + """Capability-name sentinel whose reflected `str.__eq__` pops an entry from + the registry, simulating a concurrent `release()` mutating `_added_adapters` + while `_find_adapter` iterates it. A live `.values()` view would raise + `RuntimeError: dictionary changed size during iteration`; the snapshot + `_find_adapter` takes must iterate on regardless. + """ + + def __init__(self, registry: dict, key_to_remove: str) -> None: + self._registry = registry + self._key_to_remove = key_to_remove + self.fired = False + + def __eq__(self, other: object) -> bool: + if not self.fired: + self.fired = True + self._registry.pop(self._key_to_remove, None) + return False + + def __hash__(self) -> int: + return hash("mutating-capability-sentinel") + + +class _MutatingName: + """Capability-name sentinel whose `__str__` pops an entry from the registry, + simulating a concurrent `release()` mutating `_added_adapters` while + `resolve_adapter`'s collision scan builds `f"{name}_"` on every iteration. + + `__add__` answers without firing: `Identity`/key construction may build + `name + "_" + ...`, and the mutation must land in the scan, not in + registration. + """ + + def __init__(self, registry: dict, key_to_remove: str, prefix: str) -> None: + self._registry = registry + self._key_to_remove = key_to_remove + self._prefix = prefix + self.fired = False + + def __add__(self, other: str) -> str: + return self._prefix + other + + def __str__(self) -> str: + if not self.fired: + self.fired = True + self._registry.pop(self._key_to_remove, None) + return self._prefix + + def __repr__(self) -> str: + return f"_MutatingName({self._prefix!r})" + + +def test_find_adapter_survives_concurrent_removal_during_iteration(): + """`_find_adapter` must not iterate a live view over `_added_adapters`. + + `_added_adapters` was insert-only until `remove_adapter()` (#1528) added + the first runtime deletion from it. A concurrent `release()` mutating the + dict while `_find_adapter` holds a live `.values()` view raises + `RuntimeError: dictionary changed size during iteration`. `_find_adapter` + must snapshot into a list first. + """ + first = _make_composed_adapter("answerability") + second = _make_composed_adapter("uncertainty") + + mock_backend = MagicMock(spec=AdapterMixin) + registry = { + _composed_adapter_key(first): first, + _composed_adapter_key(second): second, + } + mock_backend._added_adapters = registry + + capability = _MutatingCapability(registry, _composed_adapter_key(second)) + result = AdapterMixin._find_adapter(mock_backend, capability) # must not raise + + assert capability.fired, "the mutation must have fired during iteration" + assert result is None + assert _composed_adapter_key(second) not in registry + + +def test_resolve_adapter_survives_concurrent_removal_during_iteration(): + """`resolve_adapter`'s collision scan must not iterate a live view of `_added_adapters`. + + Guards the `list(...)` snapshot with the same deterministic sentinel + pattern as `test_find_adapter_survives_concurrent_removal_during_iteration`: + the pop fires from the scan's `f"{name}_"` build on the first (non-matching) + entry, so a live `.items()` view raises `RuntimeError: dictionary changed + size during iteration` when the scan then advances, while the snapshot + iterates on. + """ + # resolve_adapter registers under the LORA default, so the colliding + # binding must claim `answerability_lora` to actually block registration. + binding = LocalFileBinding(name="answerability", adapter_type=AdapterType.LORA) + distractor = _make_composed_adapter("uncertainty") + + mock_backend = MagicMock(spec=AdapterMixin) + mock_backend.base_model_name = "ibm-granite/granite-4.1-3b" + mock_backend._uses_embedded_adapters = False + registry = { + _composed_adapter_key(distractor): distractor, + binding.qualified_name: binding, + } + mock_backend._added_adapters = registry + mock_backend._find_adapter.side_effect = lambda cap, types=None: ( + AdapterMixin._find_adapter(mock_backend, cap, types) + ) + # Nothing new lands in the registry: the binding keeps the name, exactly + # as the real duplicate-key guard would. + mock_backend.add_adapter.side_effect = lambda a: None + + name = _MutatingName(registry, _composed_adapter_key(distractor), "answerability") + 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 pytest.raises(KeyError, match=r"LocalFileBinding.*answerability_lora"): + AdapterMixin.resolve_adapter(mock_backend, name) + + assert name.fired, "the mutation must have fired during the collision scan" + assert _composed_adapter_key(distractor) not in registry + assert binding.qualified_name in registry + + +def test_resolve_adapter_names_the_conflict_when_a_binding_blocks_registration(): + """resolve_adapter's KeyError should name the collision, not just say "not found". + + Regression guard: a `LocalFileBinding` registered under the same + qualified-name key space `resolve_adapter` auto-registers into silently + blocks the new composed `Adapter` (the backend's duplicate-key guard + refuses it). `_find_adapter` can't see the `LocalFileBinding` either (not + an `_AdapterCore`), so without this check the failure surfaced as a bare + "Adapter 'answerability' not found after registration" with no hint of + what actually occupied the name. The resolver registers under the LORA + default, so the colliding key is `answerability_lora`. + """ + binding = LocalFileBinding(name="answerability", adapter_type=AdapterType.LORA) + mock_backend = MagicMock(spec=AdapterMixin) + mock_backend.base_model_name = "ibm-granite/granite-4.1-3b" + mock_backend._uses_embedded_adapters = False + mock_backend._added_adapters = {binding.qualified_name: binding} + mock_backend._find_adapter.side_effect = lambda cap, types=None: ( + AdapterMixin._find_adapter(mock_backend, cap, types) + ) + # Simulates the backend's real duplicate-key guard refusing the new + # composed Adapter: registration is attempted but nothing new lands in + # `_added_adapters`. + mock_backend.add_adapter.side_effect = lambda a: None + + 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 pytest.raises(KeyError, match=r"LocalFileBinding.*answerability_lora"): + AdapterMixin.resolve_adapter(mock_backend, "answerability") + + +# --------------------------------------------------------------------------- +# _adapter_resolve_lock() critical section +# --------------------------------------------------------------------------- + + +class _TrackingLock: + """Real lock that records how many times it was entered/exited. + + Used in place of a bare `MagicMock()` context manager so a test can assert + the lock was acquired exactly once around the whole registration critical + section (not once per `add_adapter` call in the embedded loop). + """ + + def __init__(self) -> None: + self._lock = threading.RLock() + self.enter_count = 0 + self.exit_count = 0 + self.max_depth = 0 + + def __enter__(self): + self._lock.acquire() + self.enter_count += 1 + self.max_depth = max(self.max_depth, self.enter_count - self.exit_count) + return None + + def __exit__(self, *exc_info): + self.exit_count += 1 + self._lock.release() + return False + + +def test_resolve_adapter_survives_reentrant_activation_lock(): + """resolve_adapter() must not deadlock when called while the lock is already held. + + `_adapter_activation_lock()`'s docstring documents a reentrancy contract: + an override must return a reentrant lock because a caller can already hold + it on the same thread. Simulate that caller directly: hold the real lock + this mock backend's `_adapter_activation_lock()` returns, then call + `resolve_adapter()` from inside that hold. + """ + mock_catalog_entry = IntrinsicsCatalogEntry( + name="answerability", + repo_id="ibm-granite/granitelib-rag-r1.0", + revision="abc123", + adapter_types=(AdapterType.ALORA, AdapterType.LORA), + ) + real_lock = threading.RLock() + mock_backend = MagicMock(spec=AdapterMixin) + mock_backend.base_model_name = "ibm-granite/granite-4.1-3b" + mock_backend._uses_embedded_adapters = False + 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__(_composed_adapter_key(a), a) + ) + 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, + ), + real_lock, # simulate an already-in-progress caller holding the lock + ): + result = AdapterMixin.resolve_adapter(mock_backend, "answerability") + + assert result is not None + assert result.identity.name == "answerability" + + +def test_resolve_adapter_holds_resolve_lock_during_registration(): + """resolve_adapter's single-adapter 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 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", + repo_id="ibm-granite/granitelib-rag-r1.0", + revision="abc123", + adapter_types=(AdapterType.ALORA, AdapterType.LORA), + ) + mock_backend = MagicMock(spec=AdapterMixin) + mock_backend.base_model_name = "ibm-granite/granite-4.1-3b" + mock_backend._uses_embedded_adapters = False + mock_backend._added_adapters = {} + + tracking_lock = _TrackingLock() + 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 resolve lock is held" + ) + 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, + ): + AdapterMixin.resolve_adapter(mock_backend, "answerability") + + assert tracking_lock.enter_count == 1 + assert tracking_lock.exit_count == 1 + + +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 + `_discover_embedded_adapters()`. The lock must be acquired once for the + whole loop, not re-acquired per adapter (that would reopen the race between + iterations). `_adapter_resolve_lock()`, not `_adapter_activation_lock()` — + see the lock-order note on the latter. + """ + embedded_adapters = [ + (_make_composed_adapter("answerability", technology="alora"), {"k": "v"}), + (_make_composed_adapter("answerability", technology="lora"), {"k": "v"}), + ] + + mock_backend = MagicMock(spec=AdapterMixin) + mock_backend.base_model_name = "ibm-granite/granite-4.1-3b" + mock_backend._uses_embedded_adapters = True + mock_backend._adapter_source = "ibm-granite/granite-switch-micro" + mock_backend._added_adapters = {} + + tracking_lock = _TrackingLock() + mock_backend._adapter_resolve_lock.return_value = tracking_lock + + 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) + ) + + with patch( + "mellea.backends.adapters.adapter._discover_embedded_adapters", + return_value=embedded_adapters, + ): + AdapterMixin.resolve_adapter(mock_backend, "answerability") + + assert mock_backend.add_adapter.call_count == 2, ( + "both embedded adapters must have been registered" + ) + assert tracking_lock.enter_count == 1, ( + "the lock must be acquired once for the whole loop, not per adapter" + ) + assert tracking_lock.exit_count == 1 + + +def test_resolve_adapter_concurrent_first_use_does_not_double_register(): + """Two concurrent first-time resolves for the same name must not race (issue #1562). + + Mirrors the real `add_adapter` duplicate-registration check (`existing = + registry.get(key); ...; registry[key] = adapter`) with an injected delay + between the read and the write, which makes the unguarded race + deterministic: without `_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 reusing + the other's registration. + """ + mock_catalog_entry = IntrinsicsCatalogEntry( + name="answerability", + repo_id="ibm-granite/granitelib-rag-r1.0", + revision="abc123", + adapter_types=(AdapterType.ALORA, AdapterType.LORA), + ) + + registry: dict = {} + real_lock = threading.RLock() + registrations: list = [] + + mock_backend = MagicMock(spec=AdapterMixin) + 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_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): + 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[key] = adapter + registrations.append(adapter) + + mock_backend.add_adapter.side_effect = racy_add_adapter + + results: list = [None, None] + errors: list = [] + + def call(index: int) -> None: + try: + results[index] = AdapterMixin.resolve_adapter(mock_backend, "answerability") + except Exception as e: # pragma: no cover - surfaced via errors list + errors.append(e) + + # `unittest.mock.patch`'s save/restore of the patched attribute is not + # thread-safe: two threads independently entering/exiting a `patch(...)` on + # the same target race on save-then-restore. Install the patch once here, + # from the main thread, before spawning the workers — only + # `resolve_adapter`'s own registration path runs concurrently. + with patch( + "mellea.backends.adapters.adapter.fetch_intrinsic_metadata", + return_value=mock_catalog_entry, + ): + t1 = threading.Thread(target=call, args=(0,), daemon=True) + t2 = threading.Thread(target=call, args=(1,), daemon=True) + t1.start() + t2.start() + t1.join(timeout=5) + t2.join(timeout=5) + # A still-alive thread here would still be running resolve_adapter() + # against the patch this `with` block is about to tear down. Assert + # liveness before leaving the patch context, not after. + assert not t1.is_alive(), "t1 did not finish within the join timeout" + assert not t2.is_alive(), "t2 did not finish within the join timeout" + + assert not errors, f"resolve_adapter raised under concurrency: {errors}" + assert len(registrations) == 1, ( + "exactly one adapter must be registered under the never-before-seen name" + ) + # `racy_add_adapter`'s own no-op guard (`if existing is not None: return`) + # would keep the assertion above green even without resolve_adapter's + # in-lock re-check of `_find_adapter(name)` — the loser would just call + # `add_adapter` a second time and no-op there instead. Pin the actual + # optimisation directly: the loser must never call `add_adapter` at all. + mock_backend.add_adapter.assert_called_once() + assert results[0] is results[1] is registrations[0], ( + "both concurrent callers must resolve to the single registered adapter" + ) diff --git a/test/backends/test_huggingface.py b/test/backends/test_huggingface.py index edb652fb44..a2e2ab18d8 100644 --- a/test/backends/test_huggingface.py +++ b/test/backends/test_huggingface.py @@ -34,6 +34,13 @@ from mellea import MelleaSession from mellea.backends import ModelOption, model_ids +from mellea.backends.adapters._core import ( + Adapter as _AdapterCore, + Identity, + LocalFileBinding, +) +from mellea.backends.adapters.catalog import AdapterType, fetch_intrinsic_metadata +from mellea.backends.adapters.io_contracts import get_io_contract from mellea.backends.cache import SimpleLRUCache from mellea.backends.huggingface import LocalHFBackend, _assert_correct_adapters from mellea.core import ( @@ -53,6 +60,35 @@ from test.conftest import hf_skip +def _add_alora_intrinsic(backend: LocalHFBackend, name: str) -> None: + """Register an intrinsic's aLoRA on `backend` as a composed `Adapter`. + + `resolve_adapter()` registers the LoRA by default (adapter.py), but the + constraint-checking path and `test_adapters` need the aLoRA (keyed + `"_alora"`) — the behaviour the removed `IntrinsicAdapter` shim gave + by defaulting to `AdapterType.ALORA`. Build the composed aLoRA `Adapter` + directly and register it, rather than changing `resolve_adapter`'s default + (which would wrongly promote LoRA-only intrinsics like `uncertainty`). + """ + metadata = fetch_intrinsic_metadata(name) + backend.add_adapter( + _AdapterCore( + identity=Identity( + name=name, + adapter_type="alora", + capability=metadata.effective_capability, + ), + io_contract=get_io_contract(name), + weights=LocalFileBinding( + name=name, + adapter_type=AdapterType.ALORA, + repo_id=metadata.repo_id, + revision=metadata.revision, + ), + ) + ) + + @pytest.fixture(scope="module") def backend(): """Shared HuggingFace backend for all tests in this module. @@ -64,8 +100,8 @@ def backend(): backend = LocalHFBackend( model_id=model_ids.IBM_GRANITE_4_1_3B, cache=SimpleLRUCache(5) ) - backend.resolve_adapter("requirement-check") - backend.resolve_adapter("answerability") + _add_alora_intrinsic(backend, "requirement-check") + _add_alora_intrinsic(backend, "answerability") yield backend from test.conftest import cleanup_gpu_backend @@ -367,8 +403,8 @@ async def test_generate_with_lock(backend) -> None: b._loaded_adapters = {} b._composed_adapters = {} b._composed_adapter_configs = {} - b.resolve_adapter("requirement-check") - b.resolve_adapter("answerability") + _add_alora_intrinsic(b, "requirement-check") + _add_alora_intrinsic(b, "answerability") memoized: dict[torch.Tensor, str] = dict() # type: ignore[name-defined] gen_func = model.generate @@ -558,7 +594,7 @@ async def test_error_during_generate_with_lock(backend) -> None: b._loaded_adapters = {} b._composed_adapters = {} b._composed_adapter_configs = {} - b.resolve_adapter("requirement-check") + _add_alora_intrinsic(b, "requirement-check") regular_generate = b._model.generate diff --git a/test/backends/test_huggingface_unit.py b/test/backends/test_huggingface_unit.py index 3e18ba3d94..e21466a758 100644 --- a/test/backends/test_huggingface_unit.py +++ b/test/backends/test_huggingface_unit.py @@ -268,7 +268,7 @@ def _make_local_file_binding_stub() -> LocalFileBinding: def _make_embedded_adapter_stub() -> _AdapterCore: """A composed embedded `Adapter` (EmbeddedBinding) for the Granite Switch path.""" return _embedded_adapter_from_entry( - intrinsic_name="answerability", config=_EMBEDDED_STUB_CONFIG, technology="alora" + intrinsic_name="answerability", technology="alora" ) @@ -1230,71 +1230,6 @@ def fake_generate_with_transformers(tokenizer, model, generate_input, other_inpu assert payload.error is exc_info.value -@pytest.mark.asyncio -async def test_alora_activation_guard_abort_hook_reports_shim_as_pinned_local_file(): - """A guard abort on the legacy `IntrinsicAdapter` shim reports what a success would. - - The shim's own `weights` is an inert `_ShimWeightsBinding` (`binding_type` - `"unknown"`, no `revision`); a successful call reports through the per-call - `_IntrinsicPeftBinding` instead (`"local_file"` plus the catalogue SHA). The - abort payload must match that, not the placeholder. - """ - from mellea.backends.adapters._core import AloraActivationError - from test.backends.test_adapters._hook_capture import ( - capture_adapter_hooks, - invocation_payloads, - ) - - stub_backend = SimpleNamespace( - from_mellea_model_opts_map={ - ModelOption.MAX_NEW_TOKENS: "max_new_tokens", - ModelOption.STOP_SEQUENCES: "stop_strings", - } - ) - backend = _setup_mismatched_alora_stub(stub_backend) - shim = _make_intrinsic_adapter_stub() - shim.intrinsic_metadata = SimpleNamespace(revision="def456") - shim.config = {"parameters": {}} - backend._composed_adapters = {} - backend._composed_adapter_configs = {} - backend._added_adapters = {shim.qualified_name: shim} - - with ( - capture_adapter_hooks() as mock_invoke, - 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_with_mismatched_prompt, - ), - 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": {}}), - ): - with pytest.raises(AloraActivationError): - await LocalHFBackend._generate_from_intrinsic( - backend, - Intrinsic("answerability"), - ChatContext().add(Message("user", "Is the sky blue?")), - model_options={}, - ) - - payloads = invocation_payloads(mock_invoke) - assert len(payloads) == 1, payloads - assert payloads[0].binding_type == "local_file" - assert payloads[0].revision == "def456" - assert payloads[0].outcome == "error" - - @pytest.mark.asyncio @pytest.mark.parametrize("explicit_alora", [False, True], ids=["plain", "explicit"]) async def test_requirement_reroute_on_activation_failure(caplog, explicit_alora): diff --git a/test/backends/test_openai_intrinsics_unit.py b/test/backends/test_openai_intrinsics_unit.py index b92d89a06b..47ecaf822f 100644 --- a/test/backends/test_openai_intrinsics_unit.py +++ b/test/backends/test_openai_intrinsics_unit.py @@ -169,7 +169,7 @@ def _make_backend_with_adapter( model_options=model_options, ) adapter = _embedded_adapter_from_entry( - intrinsic_name="answerability", config=config, technology="alora" + intrinsic_name="answerability", technology="alora" ) backend.add_adapter(adapter, config=config) return backend @@ -389,9 +389,7 @@ def test_remove_adapter_frees_name_for_reuse(): qualified_name = "answerability_alora" first = backend._added_adapters[qualified_name] replacement = _embedded_adapter_from_entry( - intrinsic_name="answerability", - config=deepcopy(_SIMPLE_CONFIG), - technology="alora", + intrinsic_name="answerability", technology="alora" ) # While the name is occupied, the registration is refused. @@ -913,9 +911,7 @@ def _make_backend_with_requirement_adapter() -> OpenAIBackend: ) backend.add_adapter( _embedded_adapter_from_entry( - intrinsic_name="requirement-check", - config=deepcopy(_SIMPLE_CONFIG), - technology="alora", + intrinsic_name="requirement-check", technology="alora" ), config=deepcopy(_SIMPLE_CONFIG), )