feat: normailze intinsics interfaces by akihikokuroda · Pull Request #1028 · generative-computing/mellea

akihikokuroda · 2026-05-06T17:18:23Z

Misc PR

Type of PR

Bug Fix
New Feature
Documentation
Other

Description

Link to Issue: fix: fix: intrinsic function signatures #1003

Testing

Tests added to the respective file if code was changed
New code has 100% coverage if code as added
Ensure existing tests and github automation passes (a maintainer will kick off the github automation when the rest of the PR is populated)

Attribution

AI coding assistants used: claude

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

github-actions · 2026-05-06T17:20:48Z

The PR description has been updated. Please fill out the template for your PR to be reviewed.

planetf1

threading looks good, and catching the float → str return type mismatch was worth doing. Two things I'd want resolved before merging: the from_context tests won't actually exercise the code path they're meant to cover, and the documents positional change breaks existing callers without any mention in the issue or PR. The latter is easy to fix without the break — see inline.

planetf1 · 2026-05-07T09:58:37Z

+    # Add documents to the last assistant message
+    last_turn = context.last_turn()
+    assert last_turn is not None and last_turn.output is not None
+    assert last_turn.output.value is not None


These assertions will never pass as written. _read_guardian_input builds context via context.add(Message(...)), which means the last node is a Message, not a ModelOutputThunk — so last_turn() returns ContextTurn(last_element, None) (base.py:919). .output is always None here. The test aborts before reaching the code it's supposed to cover, so the documents=None branch in factuality_detection is entirely uncovered.

The fix is to use the same extraction path that guardian.py itself falls back to:

last_turn = context.last_turn() assert last_turn is not None if last_turn.output is not None and last_turn.output.value is not None: content = last_turn.output.value else: assert isinstance(last_turn.model_input, Message) content = last_turn.model_input.content rewound = context.previous_node assert rewound is not None context_with_docs: ChatContext = rewound.add( # type: ignore[assignment] Message("assistant", content, documents=documents) )

Same issue at lines 220–221 in test_factuality_correction_from_context.

planetf1 · 2026-05-07T09:58:37Z

-def factuality_detection(context: ChatContext, backend: AdapterMixin) -> float:
-    """Determine is the last response is factually incorrect.
+def factuality_detection(
+    documents: collections.abc.Iterable[str | Document] | None,


Making documents the new first positional arg is a silent break — existing callers doing factuality_detection(ctx, backend) now pass ctx as documents and get a confusing error rather than a helpful one. Neither #1003 nor this PR mentions it as a breaking change, and our policy is to avoid that without explicit callout.

The good news is the issue just asks for parity with the other intrinsics — we can get there without the break by making documents keyword-only:

def factuality_detection( context: ChatContext, backend: AdapterMixin, *, documents: collections.abc.Iterable[str | Document] | None = None, model_options: dict | None = None, ) -> str:

Existing call sites continue to work, new callers pass documents= explicitly, and it's consistent with how model_options is handled everywhere else in this PR. Worth also updating the AGENTS.md Section 13 table with the new signature.

planetf1 · 2026-05-07T09:58:37Z


+    # If documents are provided, add them to the last assistant message
+    if documents is not None:
+        # Get the last turn and add documents to it


This block extracting the assistant content, rewinding the context, and reattaching documents is identical in factuality_correction (~lines 303–336). If either of the fixes above lands here, it'll need to land in both places. Would be cleaner to pull it into a private helper — something like _reattach_documents(context, documents) -> ChatContext — which also gives us one place to sort out the cast() vs type: ignore question below.

planetf1 · 2026-05-07T09:58:37Z

+
+        # Extract assistant content from either output (if generated) or model_input (if manually added)
+        if last_turn.output is not None and last_turn.output.value is not None:
+            assistant_content = last_turn.output.value


Small edge case: if last_turn.output is not None but .value is None (uncomputed thunk, failed generation), the code falls through to model_input and ends up wrapping the user's question as the assistant's response. That'll produce a factuality score on the wrong text with no indication anything went wrong. Raising explicitly there rather than falling through would be much easier to debug.

planetf1 · 2026-05-07T09:58:37Z

+            raise ValueError("Cannot rewind context past the root node")
+        # Type ignore because rewound is Context but we know it's ChatContext
+        context = rewound.add(  # type: ignore[assignment]
+            Message(


# type: ignore[assignment] is hiding a real mismatch: previous_node.add(...) returns Context, not ChatContext. Worth using cast(ChatContext, rewound.add(...)) with a short note explaining why, rather than silencing mypy here — if the type hierarchy shifts this will fail at runtime instead of at type-check time. As above, extracting the helper means one fix instead of two.

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

jakelorocco · 2026-05-08T16:30:18Z

+def _reattach_documents(
+    context: ChatContext, documents: collections.abc.Iterable[str | Document]
+) -> ChatContext:
+    """Rewind context and re-add the last message with documents attached.
+
+    Extracts the assistant's response from the last turn (handling both generated
+    output and manually-added messages), then rewinds the context to before that
+    message and re-adds it with the provided documents.
+
+    Args:
+        context: Context containing the conversation.
+        documents: Documents to attach to the last assistant message.
+
+    Returns:
+        New context with documents attached to the last assistant message.
+
+    Raises:
+        ValueError: If context cannot be rewound or assistant content cannot be extracted.
+    """
+    last_turn = context.last_turn()
+    if last_turn is None:
+        raise ValueError("Cannot reattach documents: context has no last turn")
+
+    # Extract assistant content, preferring generated output over input
+    if last_turn.output is not None and last_turn.output.value is not None:
+        assistant_content = last_turn.output.value
+    elif last_turn.output is not None and last_turn.output.value is None:
+        # Uncomputed thunk — avoid silent fallthrough to model_input
+        raise ValueError(
+            "Cannot reattach documents: last turn output is uncomputed (thunk with no value)"
+        )
+    elif last_turn.model_input is not None and isinstance(
+        last_turn.model_input, Message
+    ):
+        assistant_content = last_turn.model_input.content
+    else:
+        raise ValueError(
+            "Cannot reattach documents: cannot extract assistant content from last turn"
+        )
+
+    # Rewind and re-add with documents
+    rewound = context.previous_node
+    if rewound is None:
+        raise ValueError("Cannot rewind context past the root node")
+
+    return cast(
+        ChatContext,
+        rewound.add(
+            Message(
+                "assistant",
+                assistant_content,
+                documents=_coerce_to_documents(documents),
+            )
+        ),
+    )


We actually have two helper functions already (_resolve_question and _resolve_response) utilized by the other intrinsics. Could we utilize that function? I believe it serves the same purpose.

I'm still working on this.

Now it's fixed.

jakelorocco · 2026-05-08T16:31:20Z

+def factuality_detection(
+    context: ChatContext,
+    backend: AdapterMixin,
+    *,
+    documents: collections.abc.Iterable[str | Document] | None = None,
+    model_options: dict | None = None,


I think to match the function signature of the other intrinsics, this should actually accept a response parameter that's of type str | None. We only extract / modify the last message if response isn't provided.

jakelorocco · 2026-05-08T16:31:34Z

+def factuality_correction(
+    context: ChatContext,
+    backend: AdapterMixin,
+    *,
+    documents: collections.abc.Iterable[str | Document] | None = None,
+    model_options: dict | None = None,
+) -> str:


Same as the function signature comment above.

jakelorocco · 2026-05-08T16:32:59Z

+        context = context.add(Message(m["role"], m["content"]))

-    result = guardian.factuality_detection(context, call_intrinsic_backend)
+    result = guardian.factuality_detection(docs, context, call_intrinsic_backend)


Please make sure the tests run. I might be misreading things but shouldn't this throw an error given the function signature define above?

jakelorocco · 2026-05-08T16:34:00Z

+    # Extract assistant content using the same logic as _reattach_documents
+    last_turn = context.last_turn()
+    assert last_turn is not None
+    if last_turn.output is not None and last_turn.output.value is not None:
+        content = last_turn.output.value
+    else:
+        assert isinstance(last_turn.model_input, Message)
+        content = last_turn.model_input.content
+
+    rewound = context.previous_node
+    assert rewound is not None
+    context_with_docs: ChatContext = rewound.add(  # type: ignore[assignment]
+        Message("assistant", content, documents=documents)
+    )

-    result = guardian.factuality_correction(context, backend)
+    # Test with documents=None (should extract from context)
+    result = guardian.factuality_correction(context_with_docs, backend)


Isn't this replicating the logic from the helper function? Should we be able to use that here? I might be missing what this is testing.

jakelorocco · 2026-05-08T16:38:03Z

 def guardian_check(
    context: ChatContext,
    backend: AdapterMixin,
    criteria: str,
    target_role: str = "assistant",
+    model_options: dict | None = None,


I think we should add the same document handling to this one as well. One of the modes for this is groundedness which allows for providing context documents: https://huggingface.co/ibm-granite/granitelib-guardian-r1.0/blob/main/guardian-core/README.md#intended-use

You can also take a look at Example 3 in docs/examples/intrinsics/guardian_core.py. I think it would be worth adding additional commentary in the function signature describing that documents should primarily be used for this purpose / that specific criteria.

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

normailze intinsics interfaces

c0221b1

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

akihikokuroda requested a review from a team as a code owner May 6, 2026 17:18

akihikokuroda requested review from avinash2692 and planetf1 May 6, 2026 17:18

github-actions Bot added the enhancement New feature or request label May 6, 2026

planetf1 mentioned this pull request May 7, 2026

Epic: Fix Intrinsic Adapter Lifecycle & Consistency in Mellea #929

Open

planetf1 requested changes May 7, 2026

View reviewed changes

akihikokuroda added 2 commits May 7, 2026 09:20

review comments

4ddb641

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

review comments

6733711

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

akihikokuroda requested a review from planetf1 May 7, 2026 14:13

psschwei requested a review from jakelorocco May 8, 2026 15:08

jakelorocco reviewed May 8, 2026

View reviewed changes

akihikokuroda requested a review from jakelorocco May 8, 2026 22:51

review comments

b6cc1e6

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

Conversation

akihikokuroda commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Misc PR

Type of PR

Description

Testing

Attribution

Uh oh!

github-actions Bot commented May 6, 2026

Uh oh!

planetf1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

akihikokuroda commented May 6, 2026 •

edited

Loading