fix(backends): fire adapter_function_invocation_complete for embedded adapter calls - #1609
Merged
Merged
Conversation
… adapter calls EmbeddedBinding.apply_activation only mutates the outgoing request, so it cannot know a call's real generate+parse outcome — firing the invocation hook there previously meant guessing "success" (issue generative-computing#1142/PR generative-computing#1559). Both OpenAIBackend and LocalHFBackend resolve the real outcome once result_processor.transform() runs inside their granite_formatters_processing closures. Fire adapter_function_invocation_complete there instead, once generation and parsing actually resolve, classifying malformed/unparsable output as schema_error and any other failure as error. The legacy PEFT path already gets this signal via adapter_scope and is left untouched. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
…st parse failure Address review findings on the embedded invocation_complete wiring: - Fire outcome="error" when the generation call itself fails (network error, timeout, provider error), not only from a parse-time failure inside granite_formatters_processing. ModelOutputThunk.avalue() raises a queue-carried exception before that closure ever runs, so this path previously fired nothing at all. Adds _await_embedded_generation, which wraps the generation coroutine in both backends; the two fire sites are mutually exclusive. - Move the outcome="success" fire to after all closure work that can still raise (indexing into a malformed response, the tail self.processing() call in HF), not before it — otherwise a call that goes on to fail after transform succeeds still records success, the exact shape generative-computing#1559 reverted. - Narrow docstrings/comments that overclaimed full outcome coverage: contract-level IOContract mismatches (as opposed to malformed JSON) still record success, since that validation runs later in call_intrinsic; documented as a residual gap, not fixed here. - Add regression tests for the generation-failure path on both backends, a guard pinning the PEFT path never double-fires through the embedded helper, and a guard pinning that a failing hook dispatch doesn't mask the real outcome. Tighten the HF test helper to capture (not swallow) the exception process() raises, so a silently-swallowed raise would fail the test. - Naming/import cleanup: rename the new helper to _fire_embedded_invocation_complete to match its private sibling helpers; import Identity from the public mellea.backends.adapters surface in both backends. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
…iring Several comments duplicated docstring reasoning nearly verbatim (the per-call-site closure comments in openai.py/huggingface.py, and part of _fire_embedded_invocation_complete's docstring). Shorten to what the reader needs at that call site. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
… classification Address a review finding (WARNING, 3/3 panel consensus) on PR generative-computing#1609 (issue generative-computing#1560): the OpenAI closure's `await self.processing(mot, chunk)` and `chunk.model_dump()` ran outside the try that fires adapter_function_invocation_complete, unlike the HF twin which already covers this. OpenAIBackend.processing() indexes chunk.choices[0].message unguarded, so an empty-choices response (content-filter rejection, some proxy errors) raised IndexError before any fire site — the call silently dropped out of both invocations and parse_failures. Moves both lines inside the try. Adds a regression test with an empty-choices ChatCompletion, verified to fail on the pre-fix code (zero payloads fired) and pass after the move. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
… refs at generative-computing#1611 Further pass on comment length across the embedded invocation_complete wiring (docstrings in _core.py/adapter_function.py, test docstrings and per-test comments that duplicated context already stated elsewhere). Also update the two "tracked as follow-up (issue generative-computing#1560)" references to point at the now-filed generative-computing#1611, since generative-computing#1560 is this issue. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
planetf1
marked this pull request as ready for review
September 2, 2026 17:42
planetf1
requested review from
ajbozarth,
jakelorocco and
markstur
and removed request for
markstur
September 2, 2026 17:42
ajbozarth
approved these changes
Sep 2, 2026
ajbozarth
left a comment
ajbozarth
left a comment
Contributor
There was a problem hiding this comment.
Some feedback from Claude:
Traced the fire-once path end-to-end and reviewed the full diff (control-flow restructuring, tests, docs, backward-compat) — correct, well-tested, no regressions. Two minor non-blocking notes inline. Didn't independently run the BlueVela GPU e2e; taking those results as reported. Approve.
jakelorocco
approved these changes
Sep 2, 2026
…xception Addresses a non-blocking review comment on PR generative-computing#1609: documents that catching BaseException (not Exception) is intentional, so a cancellation is recorded as outcome="error" too, matching adapter.py's local_file sibling helper. No behaviour change. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
planetf1
enabled auto-merge
September 3, 2026 05:46
planetf1
disabled auto-merge
September 3, 2026 05:46
planetf1
enabled auto-merge
September 3, 2026 05:47
Merged
via the queue into
generative-computing:main
with commit Sep 3, 2026
e772c88
16 of 17 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
3 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 #1560
Description
If you call an Embedded/Granite Switch adapter function (e.g.
answerability) onOpenAIBackendorLocalHFBackend, it doesn't show upin
mellea.adapter_function.invocations/.parse_failures— those metricsjust sit at zero, whether the calls succeed or fail. Nothing crashes; the
telemetry is quietly missing, so a real failure rate (e.g. the model
returning malformed JSON) is invisible on a dashboard.
That's because the code that activates an Embedded adapter only edits the
outgoing request, before the call happens — it genuinely can't know yet
whether the call will succeed. An earlier fix guessed
successanyway(#1142/PR #1559) and got reverted for marking broken calls as successful,
and the right fix — firing once the outcome is actually known — never got
done.
This PR does that: it fires once each backend has a real answer, after the
response comes back and is parsed, recording
success,schema_error(invalid JSON), or
error(anything else, including the call never gettinga response at all).
Impact:
binding_type="embedded"invocation/parse-failure metrics nowwork for both backends. No change for
local_file/PEFT, which already hadthis signal.
Epic position
Epic #929, Phase 2, depends on #1465 (merged). Follows #1142 (PR #1559),
which built
apply_activationbut deferred this signal. Scope widened2026-09-02 to also cover
LocalHFBackend's embedded path (#1018/PR #1593).Testing
New regression tests on both backends cover all three outcomes, plus guards
against double-firing on the legacy PEFT path and a failing hook dispatch
masking the real outcome:
test/backends/test_adapters/test_embedded_integration.py(OpenAIBackend)test/backends/test_adapters/test_embedded_binding.pytest/backends/test_huggingface_unit.py(LocalHFBackend)Locally:
uv run pytest test/backends/ test/plugins/ test/telemetry/ -m "not qualitative"— all passing.
ruff format/checkandmypy .clean.GPU e2e on BlueVela against the real
ibm-granite/granite-switch-4.1-3b-previewcheckpoint (both suites skipped in CI):
test/backends/test_huggingface_embedded.py(LocalHFBackend, real H100) — 1 passed.test/backends/test_openai_intrinsics.py(OpenAIBackend+ real vLLM) —21 passed, 1 unrelated failure (a server config gap that rejects the
request before this PR's code runs).
Caveats
One known gap, tracked separately: contract-level
IOContractmismatchesstill record
success— see #1611.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.