feat(backends): LocalFileBinding implements verbs (PEFT/aLoRA path) + from_catalog() + OTel spans (Epic #929 Phase 2) - #1454
Conversation
… adapter-function OTel spans Epic generative-computing#929 Phase 2 (Wave 4). Makes LocalFileBinding real: prepare/activate/ deactivate/release now drive the PEFT/aLoRA path through two new AdapterMixin verbs (activate_peft_adapter/deactivate_peft_adapter, extracted from LocalHFBackend._generate_with_adapter_lock's inline set_adapter calls), and from_catalog() builds a binding from the pinned catalogue revision. AdapterMixin.adapter_scope() is wired to really activate/deactivate around the caller's block, guaranteeing deactivate runs even on exception, and emits the adapter_function parent span plus activate/deactivate child spans and phase-complete/invocation-complete metrics hooks. Also fixes IntrinsicAdapter's two revision TODOs so the catalogue's pinned SHA reaches the HF download calls instead of always resolving "main". Adds a new AdapterMixin._adapter_activation_lock() hook (default no-op, overridden by LocalHFBackend to reuse _generation_lock) so activate/ deactivate hold the same exclusivity their docstrings already required. The production generation path (IntrinsicAdapter / resolve_adapter / _generate_from_intrinsic) is intentionally left unrewired in this PR -- cutting it over to adapter_scope is Phase 4.1's job, per the issue's own "Blocks: 4.1". _generate_with_adapter_lock still deactivates any adapter unconditionally before its own generate call, so adapter_scope's generation-through-the-adapter path is not yet exercised by production code; the new e2e test documents this and only asserts the real PEFT model's active-adapter set, not generation output. Assisted-by: Claude Code Assisted-by: IBM Bob Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
| finish_adapter_function_phase_span(call_id, "prepare", exception=exc) | ||
| raise | ||
| finish_adapter_function_phase_span(call_id, "prepare") | ||
| self._fire_phase_complete("prepare", time.monotonic() - started_at) |
There was a problem hiding this comment.
@ajbozarth, can you please weigh in on the telemetry changes in this PR? I believe we've moved to all telemetry being done through hooks. If that's the case, I think we should potentially re-evaluate here and move to the same approach.
There was a problem hiding this comment.
I agree, I'll post a deep dive review later this afternoon for @planetf1 to address tomorrow
| payload = AdapterFunctionPhaseCompletePayload( | ||
| name=name, phase=phase, duration_ms=(time.monotonic() - started_at) * 1000.0 | ||
| ) | ||
| _run_async_in_thread(invoke_hook(HookType.ADAPTER_FUNCTION_PHASE_COMPLETE, payload)) |
There was a problem hiding this comment.
Should we guard this one with a try-catch like the other one added in this PR?
There was a problem hiding this comment.
Agreed — and note there's a second unguarded site: _fire_invocation_complete (line 411) does the same bare _run_async_in_thread(invoke_hook(...)). Both should match _core.py's _fire_phase_complete:
| _run_async_in_thread(invoke_hook(HookType.ADAPTER_FUNCTION_PHASE_COMPLETE, payload)) | |
| hook_coro = invoke_hook(HookType.ADAPTER_FUNCTION_PHASE_COMPLETE, payload) | |
| try: | |
| _run_async_in_thread(hook_coro) | |
| except BaseException: | |
| hook_coro.close() | |
| raise |
| with self._generation_lock: | ||
| if adapter_name != "": | ||
| self.load_peft_adapter(adapter_name) | ||
| self._model.set_adapter(adapter_name) | ||
| self.activate_peft_adapter(adapter_name) |
There was a problem hiding this comment.
Since activate and deactivate are already called under this generation_lock, why do they need to manually request inside their own function bodies?
| pass | ||
| else: | ||
| raise e | ||
| self.deactivate_peft_adapter(adapter_name) |
There was a problem hiding this comment.
Apologies if I've already mentioned this: by using these verbs that are specific to adapter type here, we lock ourselves into supporting peft adapters (ie granite switch becomes harder to support).
What is the plan for future adapter types supported by the same backend?
There was a problem hiding this comment.
@jakelorocco this came up on #1422, and I think where we got to still holds.
A backend only implements the verbs for the adapter types it supports. The rest
raise NotImplementedError if anything calls them. Nothing is obliged to
implement the PEFT ones.
Granite Switch already has its own verb from #1140 — render_controls, at
adapter.py:569. That's what #1142's activate() calls. It never goes near the
PEFT verbs.
So each binding reaches for the verbs it needs, and the backend never has to
branch on adapter type.
On the same backend specifically — nothing stops one backend implementing more
than one set. HF is the case in point: #1018 gives it embedded adapters alongside
the PEFT ones, and EmbeddedBinding from #1142 calls render_controls on it
without touching the rest.
That said, this is the first PR that calls into any of this, rather than just
defining it. PEFT names at a call site read a lot worse than the same names in a
list of definitions. I'll add a comment there.
We also left the other half of your #1422 question open — subclassing, or a
generic AdapterMixin[A]. Nothing was opened for it, so the only record is a
paragraph in that PR description. It touches #1142, #1018, and eventually
server-mediated adapters, which bring a third verb set of their own
(set_request_adapter, adapter.py:591).
Are you still comfortable with the direction #1140 settled on, or do you think the
approach needs a wider look before Phase 2 goes further? Better to know now than
after #1142. If a wider look isn't needed, I'll raise an issue for the subclassing
question and plan the hierarchy work alongside it. Either way I'll wait for your
reply before filing anything.
There was a problem hiding this comment.
Okay, I think seeing the implementation made me doubt a little bit. I think the alternative to the distinct verbs per adapter type would be to force all adapters to implement (or no-op) a common set of verbs. There would be common interfaces for prepare/load/activate/deactivate/unload/render/etc... and adapters would have to implement/no-op all of them. That seems potentially problematic as well.
I think we can continue with the current approach (sorry for continually raising this concern, it just seems like one of the larger sources of potential future pain). Since backends check for adapter type when adding them, it seems unlikely that there will be any verb issues / incompatibilities. In order to always force compatibility (or at least keep it in the mental model when writing adapters / backends), I wonder if it's worth rewriting the add_adapter function for AdapterMixins (or restructure the calls) such that backends have to explicitly declare what adapter types they support and we check during add_adapter against that list?
There was a problem hiding this comment.
I'll think through some options tomorrow and update suggestions - it's better we get this right now.
| def activate(self) -> None: | ||
| """Loads the adapter weights into the backend for generation. | ||
|
|
||
| Raises: | ||
| RuntimeError: `prepare()` was not called first. | ||
| """ | ||
| if self.backend is None: | ||
| raise RuntimeError( | ||
| "LocalFileBinding.activate() requires prepare() to be called first." | ||
| ) | ||
| with self.backend._adapter_activation_lock(): | ||
| self.backend.activate_peft_adapter(self.qualified_name) | ||
|
|
||
| def deactivate(self) -> None: | ||
| raise NotImplementedError( | ||
| _PHASE_2_NOT_IMPLEMENTED.format(cls="LocalFileBinding") | ||
| ) | ||
| """Unloads the adapter weights from the backend. | ||
|
|
||
| Raises: | ||
| RuntimeError: `prepare()` was not called first. | ||
| """ | ||
| if self.backend is None: | ||
| raise RuntimeError( | ||
| "LocalFileBinding.deactivate() requires prepare() to be called first." | ||
| ) | ||
| with self.backend._adapter_activation_lock(): | ||
| self.backend.deactivate_peft_adapter(self.qualified_name) |
There was a problem hiding this comment.
I left a comment in the backend itself, but I think this is duplicate code. Doesn't the backend already handle requesting a lock during the activation / deactivation of adapters?
| def __init__( | ||
| self, | ||
| name: str = "", | ||
| adapter_type: AdapterType = AdapterType.LORA, |
There was a problem hiding this comment.
Identity.adapter_type is Literal["lora","alora"]. Should this match that?
I may also be misremembering, but an Adapter is defined by an identity, an io contract, and a weights binding. Why does the weights binding defined here redefine a field in the identity portion of the adapter?
| model). | ||
|
|
||
| Args: | ||
| adapter_qualified_name (str): The ``adapter.qualified_name`` of the |
There was a problem hiding this comment.
Some double-` sprinkled throughout the PR.
AngeloDanducci
left a comment
AngeloDanducci
left a comment
There was a problem hiding this comment.
A few non-blocking notes on top of @jakelorocco's threads, which already cover the main design questions plus the adapter.py:373 guard and double-backticks nits. I've replied on the 373 thread with a related second site, and left two inline notes below: a contradictory adapter_type between identity and binding in the two new tests, and a prepare() idempotency edge. For context, the diff looks correct relative to #1141, the scope boundary (production hot path deferred to 4.1) is documented honestly, and the revision-forwarding fix has good regression coverage.
| async def test_local_file_binding_full_lifecycle_against_real_model(backend): | ||
| binding = LocalFileBinding.from_catalog("answerability") | ||
| identity = Identity( | ||
| name="answerability", adapter_type="alora", capability="answerability" |
There was a problem hiding this comment.
Distinct from the design question on the adapter_type field itself: here the identity and the binding are set to contradictory values. from_catalog("answerability") builds binding from metadata.adapter_types[0], and answerability uses the catalog default (LORA, ALORA) — so binding is LoRA (qualified_name == "answerability_lora"), while this identity says aLoRA. The test passes because activation is driven entirely by binding.qualified_name and the identity's adapter_type is never asserted — but the span attribute mellea.adapter_function.adapter_type (read from adapter.identity.adapter_type in adapter_scope) would report "alora" for a binding that loaded the "lora" weights. Reads like a copy-paste of the old IntrinsicAdapter aLoRA default; align it with the binding.
| name="answerability", adapter_type="alora", capability="answerability" | |
| name="answerability", adapter_type=binding.adapter_type.value, capability="answerability" |
|
|
||
| def _make_adapter(binding: LocalFileBinding) -> Adapter: | ||
| identity = Identity( | ||
| name="answerability", adapter_type="alora", capability="answerability" |
There was a problem hiding this comment.
Same contradictory adapter_type as in test_local_file_e2e.py: binding is LoRA (from adapter_types[0]) but this identity says aLoRA. Since _make_adapter already receives binding, align them:
| name="answerability", adapter_type="alora", capability="answerability" | |
| name="answerability", adapter_type=binding.adapter_type.value, capability="answerability" |
| Raises: | ||
| RuntimeError: `bind_backend()` was not called first. | ||
| """ | ||
| if self.backend is not None: |
There was a problem hiding this comment.
prepare()'s idempotency guard relies on add_adapter having set self.backend. But LocalHFBackend.add_adapter has two early-return warning paths (already-added-to-this-backend; duplicate qualified name) that return without setting .backend. If either fires, prepare() continues to load_peft_adapter(...) while self.backend stays None, and a later activate() then wrongly raises "requires prepare() to be called first" despite prepare() having run. Narrow (duplicate registration) and mostly self-limiting, and the fake-backend unit tests can't catch it because their add_adapter double unconditionally sets .backend. Flagging as an observation, not a required change.
ajbozarth
left a comment
ajbozarth
left a comment
There was a problem hiding this comment.
Clean scoping, the 4.1 boundary is well-documented, and the revision-forwarding fix is a good catch with regression coverage. One blocking item on the span telemetry — two equally-fine ways to resolve it below — plus two small independent notes.
Blocker: adapter-function spans are emitted inline instead of through the tracing-plugin pattern
Spans here are opened/closed by direct start_*_span/finish_*_span calls in adapter_scope/_run_adapter_phase/prepare/release. Everywhere else in Mellea, spans are emitted by a *TracingPlugin in mellea/telemetry/tracing_plugins.py subscribing to lifecycle hooks, and core code never imports the span helpers. Inline emission is only used where code is synchronous and can't fire paired hooks (the session lifecycle spans); adapter_scope is async-capable (the e2e test awaits generate_from_context inside it), so it falls under the plugin pattern.
The underlying reason it had to be inline: the ADAPTER_FUNCTION_*_COMPLETE hooks are completion-only. This is the one hook family in HookType without a pre/start sibling — every other family (generation, component, tool, streaming, sampling, validation, session) has paired members. A span needs an opener to anchor on, so completion-only hooks can feed a metric but can't drive span open/close, which forced the inline workaround. (The metrics half of this PR is fine — it consumes those hooks correctly, the same as the other *MetricsPlugins.)
Two ways to resolve, both fine by me:
Option A — do it here, properly. Add pre/post (or start/end) hook points to the adapter-function contract and move span emission into an AdapterFunctionTracingPlugin. Metrics then read the post payload (which already carries duration/outcome) instead of a separate *_COMPLETE event. Two gaps to close while you're in there:
- The phase enum advertises
generateandparse, but nothing fires them (no span, no hook, no call site). Their real production sites are the model call in_generate_from_intrinsic(huggingface.py:575) andaction.parse(result)(huggingface.py:1776) — place the hook sites there so the contract doesn't ship dead phases. adapter_scope's phase and invocation hooks are currently unasserted (onlyprepare's is tested) — the new plugin + paired hooks should get coverage like the existing pairs have.
Option B — pull span telemetry out, follow up separately. Keep the LocalFileBinding lifecycle, from_catalog, verb extraction, and the revision fix (all self-standing), drop the inline spans, and open a follow-up issue to add adapter-function tracing through the plugin pattern. Reasonable if the hook-contract redesign is more than you want in this PR — and it lines up naturally with the 4.1 cutover, which is what would fire generate/parse anyway. Whether the metrics half stays or goes with it is your call.
Small note (file not in the diff, so can't inline it)
mellea/telemetry/metrics_plugins.py — the AdapterFunctionMetricsPlugin docstring still says "No production call site fires these hooks yet." This PR adds them; please update or remove it, depending on which option above you take.
| if "No adapter loaded" not in str(e): | ||
| raise e | ||
|
|
||
| def _adapter_activation_lock( |
There was a problem hiding this comment.
Forward-looking, not a bug today: _generation_lock is a non-reentrant threading.Lock, and nothing calls adapter_scope under it yet. But when 4.1 wires the generation path onto adapter_scope, entering the scope while _generate_with_adapter_lock holds this lock will deadlock when activate() re-acquires it. Worth a # TODO(4.1) here so the cutover makes this reentrant or restructures the nesting.
|
@ajbozarth thanks — you're right, and the mechanism is more wrong than the diff makes it look. Summary The spans in this PR are produced the wrong way — inline in How we got here Until #1181 in June, calling the span helpers directly from library code was What it didn't do was write the new rule down. The bit I'd most like fixed: even knowing the plugin rule, I couldn't have In this PR
Follow-ups under #929
One correction before anyone builds on it: the Shout if you'd sequence any of this differently. |
|
Thanks for the comments so far. Will wait to see if the proposed approach is in the right direction on otel (@ajbozarth ) - i read this first, and more generally on the abstraction (@jakelorocco ) - thread - (since this is more than a in-pr review comment to be handled) before addressing the various other detailed comments which need changes & closing off conversation threads etc. |
4 participants
Pull Request
Issue
Fixes #1141
Summary
Epic #929 Phase 2, Wave 4. Makes
LocalFileBinding(the PEFT/aLoRA weights binding) real — replacing its fourNotImplementedErrorstubs with workingprepare/activate/deactivate/releaseverbs, afrom_catalog()constructor, and real OTel spans + metrics hooks onAdapterMixin.adapter_scope(). Also fixes a pre-existing revision-forwarding bug inIntrinsicAdapter.The production generation path is deliberately not rewired onto this new machinery in this PR —
LocalFileBinding/adapter_scopeare fully real and independently tested, but nothing in production calls them yet. That cutover is Phase 4.1's job (see "Relation to the epic" below).Things for reviewers to check
_adapter_activation_lock()design (newAdapterMixinhook,mellea/backends/adapters/adapter.py) — is a narrow lock scoped to just the activate/deactivate verb calls the right shape, or should this be redesigned as part of 4.1's broader generation-exclusivity cutover? Deliberate "fix the documented precondition now, defer the bigger design question" choice.adapter_scope'sfinallyblock ordering — span-finish now runs before theADAPTER_FUNCTION_INVOCATION_COMPLETEhook fires (matches the existing_run_adapter_phasepattern) so a hook exception can't leak an unclosed span.test_local_file_e2e.pyasserts against the real model'sactive_adapters()inside the scope, but thegenerate_from_contextcall after it does not run through the adapter (see scope boundary in Details). Worth double-checking the module docstring and inline comments explain this clearly enough for a future reader.test/package/test_dependency_isolation.py— 3 pre-existing failures (test_hooks,test_telemetry,test_telemetry_plugins_register) reproduce identically on a cleanupstream/maincheckout with no changes at all; confirmed unrelated to this diff (likely a localcpex/ContextForge package resolution issue in isolateduv run --withenvironments). Not fixed here — flagging in case it's a known/tracked issue already.Relation to the epic
adapter_functionspan/metric naming conventions and theAdapterFunctionPhaseCompletePayload/hook contract it established.IntrinsicAdapter's generation path onto this new machinery) — see the scope boundary below.Details
What's implemented:
LocalFileBinding.prepare/activate/deactivate/release— a real lifecycle:prepare()downloads the adapter via the existing HF path and registers with the backend (session-scoped, idempotent);activate()/deactivate()are call-scoped and delegate to two newAdapterMixinverbs;release()unregisters everything and is a no-op on a second call.AdapterMixin.activate_peft_adapter/deactivate_peft_adapter— new verbs on the mixin (NotImplementedErrordefault, real override onLocalHFBackend), extracted from the inlineset_adapter(...)calls that used to live only insideLocalHFBackend._generate_with_adapter_lock. That method is refactored to call the new verbs instead of inlining PEFT calls — behaviour-preserving, existing tests pass unmodified.LocalFileBinding.from_catalog(name)— builds a binding from the adapter-function catalogue's pinned repo/revision/type.AdapterMixin.adapter_scope()becomes real — activates on entry, deactivates in afinally(so deactivate always runs, even on exception), and emits:adapter_functionparent span plusadapter_function.activate/adapter_function.deactivatechild spans (new helpers inmellea/telemetry/tracing.py, following the existingstart_action_span/finish_action_span_*pattern)ADAPTER_FUNCTION_PHASE_COMPLETE/ADAPTER_FUNCTION_INVOCATION_COMPLETEhook dispatch via the existinghas_plugins/invoke_hookguard-then-dispatch idiom (no directrecord_*calls)IntrinsicAdapterrevision-forwarding fix — the twoTODO(phase-2.2)spots inadapter.pynow forwardself.intrinsic_metadata.revisiontoobtain_io_yaml/obtain_lorainstead of implicitly resolving"main". Independent one-line bug fix, added regression coverage intest_adapter.py.AdapterMixin._adapter_activation_lock()hook — defaultcontextlib.nullcontext(), overridden byLocalHFBackendto returnself._generation_lock.LocalFileBinding.activate()/.deactivate()now hold it, so the two new verbs get the same exclusivity their docstrings already documented as a precondition (previously only enforced end-to-end inside_generate_with_adapter_lock, not on the individual verb calls).Deviations from the issue's own text (verified against current
upstream/mainbefore implementing, corrections folded into the design rather than followed literally):adapter_function/adapter_function.activate/adapter_function.deactivate(matching the naming already used by sibling Phase 2 issue refactor(backends): AdapterMixin verb rename/narrow + resolve_model_options + IntrinsicMetricsPlugin (Epic #929 Phase 2) #1140 / PR refactor(backends)!: narrow AdapterMixin verbs, centralize option resolution, add AdapterFunctionMetricsPlugin skeleton #1422 anddocs/dev/adapter_observability.md), notintrinsic.call/intrinsic.prepareas the issue text suggested.MELLEA_TRACES_CONTENTenv var (already implemented for OTel omissions/gaps: chat content events, gen_ai.conversation.id, prompt templates, error status #1035/PR feat(telemetry): close five OTel GenAI semantic convention emission gaps (#1035) #1036), notMELLEA_TRACE_CONTENT.docs/docs/advanced/intrinsics.mdanddocs/examples/intrinsics/are not touched — the user-facing construction API doesn't change in this PR (see next point), so there's nothing new to document there yet.Explicit scope boundary (by design, not an oversight): the production generation path (
IntrinsicAdapter/resolve_adapter/_generate_from_intrinsic) is not rewired ontoadapter_scopein this PR.LocalHFBackend._generate_from_context_standard's dispatch to_generate_with_adapter_lock("", self._model.generate, ...)still unconditionally deactivates any adapter before its own generate call — including one activated viaadapter_scope. That cutover is Phase 4.1's job (this issue lists "Blocks: 4.1").LocalFileBinding+adapter_scopeare fully real and independently tested here, but nothing in production calls them yet. The new e2e test's docstring and assertions are written to reflect this honestly: it provesadapter_scopereally flips the real PEFT model's active-adapter set, but does not claim generation runs through the adapter.Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.