Replies: 1 comment 4 replies
|
What's the intended scope of the Adapters API? Is it lifecycle management, or are we also intending it to handle request formatting? |
4 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
The problem
We pass adapters around as labels instead of as adapters. To load one, you glue its name and type into a label, hand the backend the label, and the backend looks that up in a dictionary to get the adapter back:
We build that label in four separate places. Nothing checks they agree, and when they don't the lookup quietly returns nothing.
Every adapter has to pretend it has a lifecycle. Bindings must implement
prepare → activate → deactivate → release. That fits an adapter whose weights are files we download and switch on. It means nothing for one already inside the model, where all you do is set a field on the outgoing request — and we ship one of those. Our two stub bindings satisfy the ABC by raisingNotImplementedErroreight times between them.Both are cheap to fix now:
from_catalog,bind_backend,prepare()andadapter_scopehave zero callers inmellea/, so nothing depends on either yet. Once #1142 builds on them it's a migration.The proposal
Two changes, independent of each other.
Verbs take the adapter
Every adapter verb takes
adapter_qualified_name: str. Give them the object:One name-to-adapter lookup survives, because a name does arrive from outside — an
Intrinsicknows it wants "answerability", not which object that is. Past that point, objects.Identityis already a frozen dataclass over three hashable fields, so it can be the dictionary key as-is.This is also the answer to @jakelorocco's question on #1454 — why does
LocalFileBindingcarryadapter_typewhenIdentityalready has it? Because it needs name and type to build the label. Take the label away and the copy has nothing left to do.Related: #1422 looked at a generic
AdapterMixin[A], decided against it as too invasive, and recorded it as the direction for the laterAdapterhierarchy consolidation. This arrives at roughly the same place from the verbs instead.Two kinds of binding
WeightsBindingis an ABC with four abstract methods, so everything declares all four. But the API is already doing two jobs — lifecycle management for weights we load ourselves, and request formatting for weights already in the model. Make each verb set optional, so a binding declares which job it's in:A binding implements one.
render_controlsandset_request_adapterboth becometransform; for Granite Switch that body writeschat_template_kwargs["adapter_name"].A runtime with nothing to do implements neither. Ollama fuses a LoRA into the model at build time — there's no adapter parameter on
/api/generateor/api/chat; you pick an adapter by picking the model. Today that can only be written as four raises.Why
openai.py:607-608, from feat: add embedded adapters (granite switch) to openai backend #881:# TODO: OpenAIBackend only supports EmbeddedAdapters. It should be refactored into a specific adapter.transform() function.render_controls(name, active) -> Nonehas nowhere to put a rendered control. OnOpenAIBackendit's adebuglog with zero production callers; the real activation is one line writing a template kwarg.render_controlsonLocalHFBackendto injectchat_template_kwargs["adapter_name"]".answerability_lorawhile the identity says aLoRA. Two parts of one object disagree, and the test passes.activate(ctx); the ABC declaresactivate(self). Whoever picks it up must either change the verbs feat(backends): LocalFileBinding implements verbs (PEFT/aLoRA path) + from_catalog() + OTel spans (Epic #929 Phase 2) #1454 just added or quietly diverge.Verbs stop taking strings and two collapse into one. Four label-building sites go to none, and
AdapterInputstops being a union of three types.Happy to be wrong on any of this, especially the binding split — @jakelorocco you've had a clearer view since April, and if the answer is "just add
transformand move on", that's much smaller than what I'm proposing. The other options I looked at are below.The two kinds of binding, in detail
The useful question isn't "where do the weights live" but who works out the point at which the adapter turns on. Two answers ship today:
LocalHFBackend— shippingOpenAIBackend(Granite Switch) — shipping. Also howllama-serverexposes aLoRA (merged) — no Mellea backend for that oneThe first is a real state toggle: stage it, switch on, switch off, take it away. The four verbs describe it exactly.
The second has no client-side state and nothing to toggle. Activation is an edit to the outgoing request, and only the field varies —
chat_template_kwargs["adapter_name"]for Switch,lora: [{"id": 0, "scale": 1.0}]forllama-server.Then the case with no verbs at all: Ollama, where the adapter is fused in when the model is built — the
ModelfileADAPTERdirective, oradaptersonPOST /api/create. In the whole ofdocs/api.md, "adapter" appears once, on model creation. Nothing to stage, nothing to toggle, nothing to put on a request.The two kinds are already close enough to confuse.
LocalHFBackend._generate_from_intrinsicclaims it "injectsintrinsic_nameintochat_template_kwargsso that the Granite Switch chat template activates the correct adapter" (huggingface.py:586). It doesn't — that's the only occurrence ofchat_template_kwargsin the whole HF backend, it's inside the docstring, and it's copied word for word fromopenai.py:564-565where it's true. The HF path goes through the PEFT lifecycle. Needs fixing whatever we decide.Identity and types, in detail
The label is
name + "_" + adapter_type.value, built atadapter.py:75,:338,:831and_core.py:290. Nothing parses it — nosplit("_"), noendswith("_lora"), no regex anywhere inmellea/orcli/. It's built, then used as an opaque dictionary key. So the risk isn't mis-parsing; it's the four builders drifting, and two lookups that disagree about which field identifies an adapter._added_adapters[adapter.qualified_name], then.get(adapter_qualified_name)(huggingface.py:2047, 2087)nameandadapter_typeresolve_adapter→_find_adapter(adapter.py:757).values()fora.identity.capability == capability, ignoring the keycapabilityNothing requires those two fields equal, and both lookups return
Noneon a miss — so a drift surfaces as "adapter not found" a long way from the mistake. Nothing can be wrong at construction time.The label also keeps the Phase 0
Adapterout of its own API: noqualified_nameto be keyed by,frozen=Trueso theadapter.backend = selfboth backends do would raise, and excluded by both registry annotations (openai.py:224,huggingface.py:448). Theadd_adapterisinstanceguards are the outermost of three walls, not a policy. Coming the other way,resolve_adapteris annotated-> _AdapterCore, the deprecated shim, because that's all_find_adaptercan yield.And it's why one fact has six spellings:
Identity.adapter_typeLiteral["lora","alora"]LocalFileBinding.adapter_typeAdapterTypeenumIntrinsic.adapter_typeslist[AdapterType], converted to a tuple ofstrat the call site_find_adapterEmbeddedIntrinsicAdapter.technologystr, mapped ontoAdapterTypeatadapter.py:825EmbeddedBindingEmbeddedBindinghaving no such field is the tell — it never builds a label, so it never needed one.LocalFileBindinggenuinely needs both facts (get_local_hf_pathpassesself.nameandalora=self.adapter_type is AdapterType.ALORA); it just shouldn't keep its own copy. Hand it the identity when the adapter is composed, or pass identity into each verb — either works, andIdentityalready validates itself in__post_init__.EmbeddedIntrinsicAdapter.technologyis the odd one out. Its only job is deciding where the control token goes — start of sequence for LoRA, before the generation prompt for aLoRA (adapter.py:788). A parameter of a request transform, on an adapter class, spelled differently from every other adapter-type field.Deletable either way:
get_adapter_for_intrinsic, exported and deprecated with zero call sites, rebuilds the label by interpolation and returns the first allowed adapter type — the same first-of-two rule behind the bug above.Options I considered
Document it and move on. Write down that the four verbs are the file-based lifecycle and that per-request adapters use
render_controls. Costs nothing now, and defers both problems into #1142 and #1018 where they become migrations.Add a fifth verb. Bindings gain
transform(request) -> requestalongside the lifecycle four; per-request bindings no-op the four. Smallest diff that fixes the verb complaint. But five verbs of which you use one or four is the shape @jakelorocco already considered and didn't like, and the labels stay.Cross-check only.
Adapter.__post_init__assertingidentity.adapter_type == weights.adapter_type. Catches @AngeloDanducci's class of bug at construction time rather than on a silent lookup miss. I'd take it as a stopgap either way.Both changes — what I'm proposing.
Rejected: putting the activation edit in the I/O contract, since
IOContract.build_prompt()(_core.py:111-120) already owns input construction. The same contract — answerability, say — is valid for both kinds of binding, while the activation mechanism varies by target.build_promptbuilds the semantic prompt;transformadapts it to the runtime.Impact on work in flight
Sketch only — worth doing properly once we've agreed the shape.
LocalFileBindingverbs stay; they're the lifecycle case and they're right. They lose their string parameter.EmbeddedBindingimplementstransformrather than routingactivatethroughrender_controls, and theactivate(ctx)/activate(self)collision stops mattering.LocalHFBackendthe first backend holding both kinds at once. That's @jakelorocco's original question about one backend supporting several adapter types, arriving for real.huggingface.py:586needs correcting either way.Does the lifecycle hold up for aLoRA today?
Yes — because PEFT does the hard part.
An aLoRA has to start applying at a specific point in the input: its invocation tokens. We store those at training time in the adapter config (
cli/alora/train.py:295-306), and PEFT reads them and scopes the adapter itself. At inferencemellea/has no invocation-token handling at all — zero references, in any form. All we do is switch the adapter on for the request, which the four verbs describe exactly.But that's a division of labour, not a property of aLoRA.
llama-serverdoes the same job inside the server, exposing only a per-requestlorafield (tools/server/README.md); a Granite Switch server does it in its chat template. Same adapter concept, three different owners — so "aLoRA" tells you nothing about which shape you're in.The risk is that the division moves again. Per-token scoping is still open upstream: llama.cpp#15327 notes under
## TODOthat the correct approach is to "identify the invocation tokens within an input request and only use the adapter for tokens starting with the invocation sequence", needing "adapter scaling on a per-token basis rather than on a per computation basis." A whole-request on/off switch can't express "on from token N onward". We don't have to today because PEFT hides it — which is why the abstraction looks fine until a second runtime turns up. (That PR's own headline benefit is cache preservation, not per-token scoping.)All reactions