fix(backends): release() deregisters via a new remove_adapter() inverse verb - #1554
Merged
Conversation
…se verb Settles generative-computing#1528's contract question: release() stays terminal for the binding instance itself (bind_backend()/prepare() still reject reuse), but a released qualified_name no longer stays claimed for the backend's lifetime. add_adapter() previously had no inverse, so unload_peft_adapter reversed the load but left the registration in LocalHFBackend's _added_adapters — burning the name for any later binding. Adds remove_adapter() as a reality-specific AdapterMixin verb (mirrors load_peft_adapter/unload_peft_adapter: default NotImplementedError, overridden by LocalHFBackend to pop the entry from _added_adapters). LocalFileBinding.release() now calls it alongside unload_peft_adapter, so a fresh LocalFileBinding for the same capability can register under the same qualified_name once the original is released. list_adapters() and the strong reference to the released binding both correct themselves as a result, with no change to list_adapters()'s own contract. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
…ontract Independent review of PR generative-computing#1554 (3 reviewers) found remove_adapter() was an incomplete inverse of add_adapter() and unsafe against still-loaded names: - remove_adapter() only popped the registry entry, leaving the removed object's .backend/.path pointing at the old backend. Re-adding the same object silently no-op'd via add_adapter()'s "already added" guard. Fix: pop and clear both. - remove_adapter() never checked _loaded_adapters. Freeing a name that was still loaded let a later load_peft_adapter() call hit PEFT's swallowed "already exists" error and silently keep running on the old adapter's weights under a new binding's identity. Fix: raise ValueError if still loaded; release() already unloads first, so the sanctioned path is unaffected. - _added_adapters was insert-only before remove_adapter() added the first runtime deletion from it. Two generation-path sites (_find_adapter, resolve_adapter) iterated it via a live dict view outside the lock release() holds while deleting, racing to "dictionary changed size during iteration". Fix: snapshot into a list before iterating at both sites. - release() called remove_adapter() unconditionally; a PEFT-reality backend that hasn't overridden it would have release() raise NotImplementedError mid-teardown and strand the binding permanently. Fix: catch and log, degrading to the pre-generative-computing#1528 behaviour (name stays claimed) instead of breaking release(). Also: corrected the reworded prepare() error (a blocking adapter is only known to be registered, not "active"); strengthened the test double in test_local_file_binding.py to mirror add_adapter()'s "already added" guard, not just the duplicate-name one; added remove_adapter() assertions to the idempotent-release and never-prepared-noop tests; fixed the AdapterMixin docstring's stale reality-specific-verb count. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
…ents No behavioural change. Docstrings/comments referenced "generative-computing#1554 review finding N" — a self-reference to the PR they live in, redundant now that the fixes are just part of this PR's own history rather than a response to someone else's review. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
Fold the independent review of the generative-computing#1528 registry work into the PR: - prepare(): drop "and has not been released" from the refusal message — false on the degradation path, where release() ran but the backend predates remove_adapter() - release(): the degradation warning now states the observable fact (remove_adapter() raised NotImplementedError) instead of asserting the backend lacks the verb — a real implementation can raise it internally - prepare(): note the lock mismatch between add_adapter()'s duplicate check (binding lifecycle lock only) and release()'s pop (generation lock) — a transient, retryable refusal - _find_adapter(): record the snapshot-lookup invariant (one entry type per qualified name; generation consumes only shims; remove_adapter() is public) for generative-computing#1465 - LocalHFBackend.remove_adapter(): docstring points binding users at binding.release() - tests: deterministic regression test for the resolve_adapter snapshot (companion to the _find_adapter one); drop drifting line-number citations; correct the fake-backend fidelity claim and the degradation-test history sentence Assisted-by: Zed Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
release() is terminal, so a later activate()/deactivate() does not "keep working" — both paths raise the same prepare()-required error. The docstring now states the actual contrast: release() keeps the binding's lifecycle state coherent with the registry, while a direct call leaves the binding in a state where the prepare()-required error hides the real cause (an external remove_adapter()); prepare() self-heals that state. Assisted-by: Zed Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
AngeloDanducci
approved these changes
Aug 19, 2026
AngeloDanducci
left a comment
AngeloDanducci
left a comment
Contributor
There was a problem hiding this comment.
One nit from claude, otherwise LGTM
mellea/backends/adapters/_core.py line 551–552 —
release() catches only NotImplementedError from remove_adapter().
LocalHFBackend.remove_adapter() can also raise ValueError (still-loaded).
On the normal path unload_peft_adapter() runs first
so this can't fire, but if a subclass's unload_peft_adapter()
were ever a no-op while weights remained loaded, that ValueError
would propagate mid-teardown and leave _released False
(the same failure mode the NotImplementedError catch was added to prevent).
This is defensible as-is — the current code is correct under the documented invariant.
Contributor
Author
Thanks for the feedback. I think it makes sense to leave as is |
Merged
via the queue into
generative-computing:main
with commit Aug 24, 2026
0aeae49
13 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Issue
Fixes #1528.
Description
LocalFileBinding.release()unloaded adapter weights but left the adapter registered with the backend. That permanently reserved its qualified name, so a fresh binding for the same capability could not be prepared later in the same process.This PR adds
remove_adapter()as the inverse of registration and calls it duringLocalFileBinding.release(). A released binding remains terminal; a newly created binding can now register the released name safely.Where this fits
This is Phase 2 work for Epic #929. It resolves the lifecycle decision needed by #1465 and is independent of the embedded-adapter work in #1142 and telemetry work in #1466. Shim removal remains with #1144.
What changed
AdapterMixin.remove_adapter()backend verb and theLocalHFBackendimplementation.Caveat
Backends that do not implement
remove_adapter()retain the previous release behaviour and emit a warning rather than failing teardown.Testing
Focused adapter and Hugging Face tests, the non-qualitative suite, Ruff, mypy, and the documentation quality gate pass. Required GitHub checks pass.
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.