fix(backends): route intrinsic generation through adapter_scope, fix lock reentrancy - #1555
Merged
Merged
Conversation
…adapter_scope routing Add tests for the intrinsic-generation lock-reentrancy deadlock, atomicity against concurrent callers, and activated-adapter assertions (rather than a generation-succeeded smoke test) ahead of routing _generate_from_intrinsic through adapter_scope. Also drop test_local_file_e2e.py's deferral note and have it generate directly against the real model while the adapter is active, instead of through the standard path, which always deactivates adapters first. These tests target a _generate_intrinsic_with_adapter_scope() helper that does not exist yet; the implementation follows in the next commit. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
test_concurrent_intrinsic_calls_cannot_observe_each_others_adapter spawned worker threads without capturing what they raised. threading.Thread swallows exceptions from its target by default, so a totally broken _generate_intrinsic_with_adapter_scope (e.g. missing before this issue's fix) left `mismatches` empty and the test passed vacuously — confirmed by running it against the pre-fix code, where it passed despite every worker thread raising AttributeError. Capture and assert on thread exceptions so the test actually fails when the call raises. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
…rative-computing#1465) _generate_from_intrinsic called _generate_with_adapter_lock directly, bypassing AdapterMixin.adapter_scope entirely -- the model call ran outside the scope whose activate()/deactivate() lifecycle generative-computing#1141 built for this. Add _generate_intrinsic_with_adapter_scope, which builds an ephemeral Adapter around a new _IntrinsicPeftBinding (driving this backend's own load_peft_adapter/activate_peft_adapter/deactivate_peft_adapter -- the same verbs the old code called directly) and runs the generate call inside adapter_scope(), so activation and deactivation now fire the ADAPTER_FUNCTION_PHASE_COMPLETE/ADAPTER_FUNCTION_INVOCATION_COMPLETE hooks, and deactivation is guaranteed even if generation raises. This reactivates the lock-reentrancy deadlock generative-computing#1454's review flagged and _adapter_activation_lock()'s docstring tracked as a TODO(generative-computing#1465): adapter_scope()'s activate()/deactivate() re-acquire _generation_lock (via _adapter_activation_lock()) from inside the same critical section _generate_intrinsic_with_adapter_scope holds it for. Fix by making _generation_lock a threading.RLock -- reentrant acquisition on the same thread succeeds instead of deadlocking, since this method only ever runs inside a single asyncio.to_thread worker thread. Holding the lock for the whole activate -> generate -> deactivate section (not just around activation) also closes the atomicity gap adapter_scope()'s own docstring flagged as latent: a concurrent caller's activate() can no longer interleave with this call's generation. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
… that hooks fire _generate_intrinsic_with_adapter_scope's own docstring claims activation now fires ADAPTER_FUNCTION_PHASE_COMPLETE/ADAPTER_FUNCTION_INVOCATION_COMPLETE, but nothing asserted on the payload -- only on lock/activation state. Add a test using the existing capture_adapter_hooks()/hook_payloads() helpers (already used by test_local_file_integration.py for the sibling binding) to pin phases, outcome, name, adapter_type, binding_type, and revision. This fails against the current code: binding_type is the invented "intrinsic_legacy" instead of the "local_file" reality this binding actually is, and revision is unresolved to None (mislabelled "unpinned") despite the adapter being pinned. Both fixed in the next commit. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
…ope path Address code-review findings on the generative-computing#1465 adapter_scope routing: - Thread the catalogue revision through _IntrinsicPeftBinding instead of leaving it unset. adapter_scope() fell back to getattr(..., None) for revision, so every intrinsic invocation was reported as revision=None ("unpinned") despite IntrinsicAdapter.intrinsic_metadata.revision always being a pinned catalogue SHA. - Use "local_file" for binding_type instead of inventing "intrinsic_legacy". This binding loads LoRA/aLoRA weights from local disk via PEFT -- the same reality LocalFileBinding reports -- so it must share that value rather than fragment the weights-reality metric dimension into two series for one reality. - Reuse adapter.identity instead of rebuilding an Identity per call. Rebuilding it re-ran Identity.__post_init__'s KNOWN_CAPABILITIES check on every generate call, firing a UserWarning on every call for hyphenated catalogue names (including "requirement-check", the auto-routed ALoraRequirement adapter) instead of once at registration. - Split load_peft_adapter into prepare() and leave only activate_peft_adapter in activate(), mirroring LocalFileBinding's phase boundary. Previously the weights load (can take seconds on first use) was folded into the "activate" phase metric, making its duration bimodal and incomparable across the two bindings that feed the same metric. Also reconciles adapter_scope()'s docstring, which still claimed "no caller combines concurrent adapter_scope() calls today" and that generative-computing#1465 (this PR) "has to solve the atomicity and threading interaction" -- both false once _generate_intrinsic_with_adapter_scope exists. Documents that ADAPTER_FUNCTION_* hook dispatch is synchronous and runs under _generation_lock, and that phase="generate"/"parse" are not emitted (deferred to generative-computing#1466). Drops the docstring's imprecise "safe because of a single asyncio.to_thread worker thread" framing (to_thread uses a pool with many threads; the safety property is that each invocation is wholly synchronous on whichever thread it lands on). Renames _NoIntrinsicIOContract to _UnusedIOContract and drops embedded issue numbers from runtime exception text (kept in docstrings instead). Types _generate_intrinsic_with_adapter_scope's generate_func/return via a TypeVar. The new test from the previous commit (asserting on the actual hook payload) now passes. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
- test_local_file_e2e.py: restore a generate_from_context() call after adapter_scope exits as a composition smoke test (the direct model.generate() edit proved activation but dropped the file's only coverage of mellea's own generation path composing with a scoped-and-released adapter). Also drops the now-tautological isinstance(value, str) assertion (value now always comes from tokenizer.decode(), which is always str) in favour of asserting non-empty content. - test_huggingface_unit.py: except BaseException -> except Exception in the concurrency test, so a real KeyboardInterrupt/SystemExit during the test isn't captured as a spurious "error". Tighten the two deactivation assertions to also check the actual set_adapter([]) call, not just the mock's derived active_adapters() state, so a change to the deactivation verb's call shape would break the test loudly instead of the mock silently going stale. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
…andard generation path _generate_with_adapter_lock (the standard, non-intrinsic path) always deactivates any adapter before generating, so a caller composing generate_from_context() with adapter_scope() silently generates against the base model regardless. Pre-existing gap, not introduced by the intrinsic routing this PR adds -- documented here since the original note recording it (in test_local_file_e2e.py's module docstring) was removed when that file's own coverage moved to a direct model.generate() call. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
Hook dispatch runs the hook coroutines on the shared _EventLoopHandler event-loop thread while the calling thread blocks; the old wording ("runs synchronously on the calling thread") hid that, along with the hazard it creates: a subscriber that re-enters a _generation_lock-holding path deadlocks the backend, which an RLock cannot prevent because reentrance only helps the owning thread. Also name _IntrinsicPeftBinding as the intrinsic-path verb acquirer in the _adapter_activation_lock() caller list (it is not LocalFileBinding's path), and qualify the to_thread claim to production invocations.
Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
Assisted-by: opencode
The generic adapter_scope failure contract is pinned in test_adapter_scope.py, but the intrinsic payload on an error outcome (name, adapter_type, binding_type, revision) and the prepare-failure semantics (no hooks fire, activation state unchanged, next call still succeeds) were untested despite this PR establishing them. Signed-off-by: Nigel Jones <jonesn@uk.ibm.com> Assisted-by: opencode
model.generate returns the prompt plus completion, so the decode always contained the prompt text and the strip() assertion could never fail for the reason it purported to guard. The real proof is the active-adapter assertion straddling the raw generate call. Signed-off-by: Nigel Jones <jonesn@uk.ibm.com> Assisted-by: opencode
… surrounding text
Second-pass fixes after re-review: the new hook-dispatch paragraph contradicted the untouched reference-example sentence ("never re-enters the event loop") two paragraphs away, so qualify it to the scope body and name the hook dispatches as the only loop traffic; state the hook-dispatch mechanism once in adapter_scope() and cross-reference it from the HF method; say the nesting fact once in the _adapter_activation_lock() docstring; and include prepare in the section name everywhere, matching the code (the lock wraps weights.prepare()).
Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
Assisted-by: opencode
Match the family scheme (test_generate_intrinsic_with_adapter_scope_<behaviour>): _error_hook_payload and _prepare_failure_no_hooks name observations, not behaviours. Signed-off-by: Nigel Jones <jonesn@uk.ibm.com> Assisted-by: opencode
This was referenced Aug 20, 2026
Third-round fixes: fold the unearned Attributes: section out of _IntrinsicPeftBinding (AGENTS.md allows it only for transformed/computed storage); let the HF method docstring cross-reference the consequences instead of restating them; correct the over-claim that the unit tests are 'also single-threaded' (the two-thread test runs one invocation per thread); add prepare to the lock-section naming in the two test docstrings that predate the 1dfe7fa alignment; reword the concurrent test away from the now-merged 'latent until generative-computing#1465' framing; and state precisely which attributes _make_fake_intrinsic_adapter exposes for the method versus for realism. Signed-off-by: Nigel Jones <jonesn@uk.ibm.com> Assisted-by: opencode
…d-path contract With intrinsic generation routed through adapter_scope, no production caller still passed an adapter name to this method (all three standard call sites passed ""); the load/activate branch survived only as test-covered residue advertising a capability the standard path does not have. The method now takes no adapter name: deactivate-any-adapter, assert base model, generate, assert. The two unit tests that pinned the removed branch collapse into one pinning the remaining contract. Granite Switch's embedded activation is a separate mechanism (EmbeddedBinding.apply_activation, generative-computing#1018) and does not use this method; the orphan was orphaned by this PR, so the cleanup lands with it. Signed-off-by: Nigel Jones <jonesn@uk.ibm.com> Assisted-by: opencode
…enerative-computing#1142 future work; pin the shrunken method's forwarding Delta-review fixes: the new docstring cited apply_activation as a present route but the verb does not exist in-tree (AGENTS.md: verify transcribed identifiers against source), so it is now framed as the designed generative-computing#1018 mechanism landing via generative-computing#1142; the method gains the Args/Returns blocks its sibling lock-holding helper documents; and the shrunken-contract test now captures and asserts the generate_func return so its name matches what it pins (the deactivate-then-generate ordering stays a body fact, stated as such in the docstring). Signed-off-by: Nigel Jones <jonesn@uk.ibm.com> Assisted-by: opencode
jakelorocco
reviewed
Aug 24, 2026
jakelorocco
left a comment
jakelorocco
left a comment
Contributor
There was a problem hiding this comment.
I think this looks good, but is there a test that does real generation with a real adapter while holding the generation lock? I think the only one has the async / generation patched out?
Assisted-by: Codex Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
Contributor
Author
|
Added in 8540e69. The real-model test now holds |
jakelorocco
approved these changes
Aug 25, 2026
Merged
via the queue into
generative-computing:main
with commit Aug 25, 2026
9c923e1
11 checks passed
This was referenced Sep 2, 2026
planetf1
added a commit
to planetf1/mellea
that referenced
this pull request
Sep 3, 2026
…uting (generative-computing#1608) * test(backends): add real-model GPU e2e for intrinsic adapter_scope routing Closes generative-computing#1574. LocalHFBackend's intrinsic LoRA/aLoRA generation path (_generate_intrinsic_with_adapter_scope / _IntrinsicPeftBinding, from generative-computing#1555) was covered only by MagicMock unit tests and by an e2e that exercises LocalFileBinding directly, never the intrinsic path itself. This adds a structural GPU e2e that drives core.check_certainty end to end against a real Granite model and the real "uncertainty" adapter, asserting on ADAPTER_FUNCTION_* hook payloads and real PEFT adapter activation state (never on the generated score, per test/README.md's e2e rules and the flakiness class documented in generative-computing#1291). A second call proves load_peft_adapter's already-loaded ValueError tolerance against a real PeftModel, a path the mocks cannot exercise. Verified: 2 consecutive local runs on Apple Silicon MPS, 2 consecutive runs on BlueVela LSF against a real CUDA GPU, all green. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com> * test(backends): tighten intrinsic adapter_scope e2e per review Address review findings on PR generative-computing#1608: - Assert invocation.adapter_type == "lora" (resolve_adapter's lazy registration always registers AdapterType.LORA on this path, per its own "pre-Phase-1 default" comment) — the payload defaults to "unknown", so a regression reporting that would have passed silently. - Note the residual dependency on a real parse-valid response in the module docstring: check_certainty still requires contract-conformant JSON, so a real parse failure surfaces as an exception from the call itself, not from any assertion this test makes. - Comment the 12GB VRAM floor against test_core.py's precedent for the same model, distinguishing it from test_local_file_e2e.py's 20GB. - Broaden _assert_single_success_invocation's docstring to match what it actually checks. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com> * test(backends): annotate _assert_single_success_invocation per review Address Bob's review note on PR generative-computing#1608: the helper lacked a type annotation on its mock_invoke parameter, inconsistent with _hook_capture.py's own convention of typing every function that accepts the mock. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com> * test(backends): trim history/PR references from test comments Rewrite the module docstring and inline comments to describe what the test verifies rather than the PR/issue history behind it, and drop the now-redundant rationale comment on the adapter_type assertion. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com> * test(backends): reduce cost and hf_skip overreach per review - Module-scope the backend fixture (halves the 3B model load cost across the two tests, matching test_core.py's convention). - Replace the live-chat context fixture with a static ChatContext, removing a redundant real generation call and its dependency on the backend fixture, and matching the minimal-context shape used elsewhere in the intrinsic test suite. - Narrow hf_skip() off the idempotency test's second check_certainty call: the adapter is already loaded by then, so that call does no network I/O, and leaving it wrapped in hf_skip could misreport a real inference failure as a skip. - Add a docstring to the first test for symmetry with the second. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com> --------- Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
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 #1465.
Description
Intrinsic generation previously occurred outside
adapter_scope(), so the adapter lifecycle was disconnected from the model call it was meant to control. Moving generation into that scope exposed a lock re-entrancy deadlock and an atomicity gap between activation and generation.This PR routes intrinsic generation through
adapter_scope(), uses a re-entrant generation lock for the full lifecycle, and records the correct lifecycle-hook metadata. The active adapter is now demonstrably the one used for generation, and cleanup occurs when generation fails.Where this fits
This is Phase 2 work for Epic #929 and follows #1141 / PR #1454. It enables the
generateandparsespan work in #1466. Embedded adapters (#1142 / #1018), output-contract resolution (#1516), and shim removal (#1144) remain separate.What changed
LocalHFBackendintrinsic calls through an adapter-scoped binding.Caveat
The hardware-backed end-to-end test is updated but was not run locally.
generateandparsespans are intentionally deferred to #1466.Testing
Focused backend 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.