From bf0f45e6875e74795bbd10d3f2efabb2dc556d2b Mon Sep 17 00:00:00 2001 From: Nigel Jones Date: Thu, 3 Sep 2026 11:48:36 +0100 Subject: [PATCH 01/10] fix(hf): parse tags into mot.thinking on LocalHFBackend LocalHFBackend never populated ModelOutputThunk.thinking; raw HF completions kept Granite's ... block inline in mot.value, unlike every other backend which gets reasoning pre-separated by its SDK/provider response. Adds _split_think_tags(), gated on the chat template actually exposing a thinking variable (avoids corrupting answers that merely mention "") and skipped for streaming generations (splitting would shrink mot.value after ModelOutputThunk.astream() has already captured a delta offset against the longer raw string, corrupting the final streamed delta). Fixes #1610. Assisted-by: Claude Code Signed-off-by: Nigel Jones --- mellea/backends/huggingface.py | 45 +++- mellea/backends/utils.py | 5 +- test/backends/test_huggingface_thinking.py | 199 ++++++++++++++++++ .../backends/test_huggingface_thinking_e2e.py | 95 +++++++++ 4 files changed, 339 insertions(+), 5 deletions(-) create mode 100644 test/backends/test_huggingface_thinking.py create mode 100644 test/backends/test_huggingface_thinking_e2e.py diff --git a/mellea/backends/huggingface.py b/mellea/backends/huggingface.py index 9f30cb963e..2a8ef7abf8 100644 --- a/mellea/backends/huggingface.py +++ b/mellea/backends/huggingface.py @@ -274,6 +274,24 @@ def _cleanup_kv_cache(cache_info: HFAloraCacheInfo) -> None: _CHAT_TEMPLATE_THINKING_VARS: tuple[str, ...] = ("think", "thinking", "enable_thinking") +_THINK_OPEN_TAG: str = "" +_THINK_CLOSE_TAG: str = "" + + +def _split_think_tags(text: str) -> tuple[str | None, str]: + """Split raw HF output into (thinking, answer) on the closing tag. + + Splits on alone (not a ... pair) because some chat + templates (e.g. granite-4.2 with enable_thinking) bake the opening tag into + the prompt, so it never appears in the model's own output. A fixed pattern + match for Granite's convention, not a general reasoning-parser; other + delimiters pass through unchanged. See #1604 for generalizing this. + """ + if _THINK_CLOSE_TAG not in text: + return None, text + reasoning, _, answer = text.partition(_THINK_CLOSE_TAG) + return (reasoning.strip().removeprefix(_THINK_OPEN_TAG).strip(), answer.strip()) + def _compute_generate_kwargs_allowlist() -> frozenset[str]: """Names that `transformers`' `model.generate` accepts as keyword arguments. @@ -1817,6 +1835,29 @@ class used during generation, if any. OrderedDict.__delitem__(hf_output, "logits") hf_output.logits = None + # Capture the raw text before any split below, for the stop-string check further down. + raw_value = mot.value + + # Gate on the template exposing a thinking var (not ModelOption.THINKING) since some + # models think by default; otherwise an answer that just mentions "" gets split. + # Skip for streaming: astream() assumes mot.value only grows, and shrinking it here + # would corrupt the final delta (see #1604 for proper incremental splitting later). + thinking_allowlist: frozenset[str] = getattr( + self, "_chat_template_allowlist", frozenset() + ) + if not mot.generation.streaming and thinking_allowlist.intersection( + _CHAT_TEMPLATE_THINKING_VARS + ): + thinking, answer = _split_think_tags(raw_value) + if thinking is not None: + mot.thinking = thinking + mot.value = answer + MelleaLogger.get_logger().debug( + "Split %d chars of thinking out of HF completion for %s.", + len(thinking), + self._model_id, + ) + # Only scan for tools if we are not doing structured output and tool calls were provided to the model. if _format is None and tool_calls: mot.tool_calls = to_tool_calls(tools, mot.value) @@ -1867,8 +1908,8 @@ class used during generation, if any. stop_strings = ( mot._call.model_options.get(ModelOption.STOP_SEQUENCES) or [] ) - ends_with_stop_string = isinstance(mot.value, str) and any( - mot.value.endswith(s) for s in stop_strings + ends_with_stop_string = isinstance(raw_value, str) and any( + raw_value.endswith(s) for s in stop_strings ) if last_token in eos_set or ends_with_stop_string: mot.generation.finish_reasons = ["stop"] diff --git a/mellea/backends/utils.py b/mellea/backends/utils.py index 401e9ed79a..4a3ad874a1 100644 --- a/mellea/backends/utils.py +++ b/mellea/backends/utils.py @@ -100,9 +100,8 @@ def to_chat( # NOTE: `self.formatter.to_chat_messages` explicitly skips `Message` objects. However, we need # to print `Message`s to correctly serialize any documents with the message. Do the printing here. # NOTE: reasoning is never replayed on the HF chat path — we serialize only `content` and never - # consult `should_replay_reasoning` (unlike the OpenAI/LiteLLM/Watsonx/Ollama chat paths). This is - # acceptable today because HF has a capture gap (per #1201) and never populates `Message.thinking` - # to begin with; when that gap is closed, replay must be wired in here. + # consult `should_replay_reasoning` (unlike OpenAI/LiteLLM/Watsonx/Ollama). `Message.thinking` + # is now captured for HF's Granite convention, but replay isn't wired in yet; #1604. ctx_as_conversation: list = [] for m in ctx_as_message_list: msg_dict: dict = {"role": m.role, "content": formatter.print(m)} diff --git a/test/backends/test_huggingface_thinking.py b/test/backends/test_huggingface_thinking.py new file mode 100644 index 0000000000..85f4a40f4b --- /dev/null +++ b/test/backends/test_huggingface_thinking.py @@ -0,0 +1,199 @@ +# Copyright IBM Corp. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Unit tests for LocalHFBackend's ... tag splitting. + +No GPU or real model is needed — _split_think_tags is a pure string function. + +torch must be importable because importing huggingface.py triggers the top-level +`import torch`. Install mellea[hf] to satisfy this requirement. +""" + +import pytest + +torch = pytest.importorskip("torch", reason="torch not installed — install mellea[hf]") + +import mellea.backends.huggingface as hf_backend +from mellea.backends.huggingface import LocalHFBackend, _split_think_tags +from mellea.core.base import CBlock, ModelOutputThunk + + +def test_split_think_tags_with_both_tags() -> None: + """Both tags present: reasoning and answer are split and stripped.""" + thinking, answer = _split_think_tags("reasoning herethe answer") + assert thinking == "reasoning here" + assert answer == "the answer" + + +def test_split_think_tags_missing_opening_tag() -> None: + """Only the closing tag present (e.g. granite-4.2's prompt-baked opening tag).""" + thinking, answer = _split_think_tags("reasoning herethe answer") + assert thinking == "reasoning here" + assert answer == "the answer" + + +def test_split_think_tags_no_closing_tag() -> None: + """No anywhere: text is returned unchanged, thinking is None.""" + thinking, answer = _split_think_tags("just a plain answer, no tags") + assert thinking is None + assert answer == "just a plain answer, no tags" + + +def test_split_think_tags_strips_surrounding_whitespace() -> None: + """Whitespace/newlines around tags (matches granite's \\n form) are stripped.""" + thinking, answer = _split_think_tags( + "\n reasoning here \n\n the answer \n" + ) + assert thinking == "reasoning here" + assert answer == "the answer" + + +def test_split_think_tags_empty_reasoning() -> None: + """Empty think block (e.g. granite's thinking-disabled form).""" + thinking, answer = _split_think_tags("the answer") + assert thinking == "" + assert answer == "the answer" + + +def test_split_think_tags_leading_whitespace_before_open_tag() -> None: + """A newline/whitespace before must not defeat removeprefix. + + Regression test: reasoning.removeprefix(_THINK_OPEN_TAG) only strips the tag + when it is the literal first character(s) of the string. Without stripping + first, "\n\n..." still starts with "\n", removeprefix is a no-op, and + the literal "" tag leaks into mot.thinking. + """ + thinking, answer = _split_think_tags("\n\nreasoning\n\nanswer") + assert thinking == "reasoning" + assert answer == "answer" + + +def test_split_think_tags_multiple_close_tags_uses_first() -> None: + """Multiple occurrences: only the first is treated as the boundary. + + Matches the first-match-wins convention used by transformers' own serving + utilities (cli/serving/utils.py) for the same start/end tag pattern. + """ + thinking, answer = _split_think_tags("abc") + assert thinking == "a" + assert answer == "bc" + + +def _make_backend(*, thinking_template_var: str | None = "think") -> LocalHFBackend: + """Return a LocalHFBackend with __init__ bypassed, wired with the minimum + state post_processing needs when there is no real GenerateDecoderOnlyOutput + (i.e. every isinstance(hf_output, GenerateDecoderOnlyOutput) branch is skipped). + + Args: + thinking_template_var: name of a thinking-related Jinja variable to bake + into the fake chat template (gates the think-split in post_processing), + or None for a template that does not reference any of them. + """ + b: LocalHFBackend = LocalHFBackend.__new__(LocalHFBackend) + b._model_id = "test-org/test-model" + b.model_id = "test-org/test-model" + b._provider = "huggingface" + b._use_caches = False + + template = ( + f"{{{{ {thinking_template_var} }}}}" + if thinking_template_var + else "{{ messages }}" + ) + + class _FakeTokenizer: + chat_template = template + + object.__setattr__(b, "_tokenizer", _FakeTokenizer()) + return b + + +async def test_post_processing_splits_thinking_before_tool_scan(monkeypatch) -> None: + """post_processing must strip ... into mot.thinking and pass + only the post-split answer text to the tool-call scan, not the raw combined + output — verifies the ordering the inline comment in post_processing promises. + """ + recorded_text: list[str] = [] + + def fake_to_tool_calls(tools, text): + recorded_text.append(text) + return None + + monkeypatch.setattr(hf_backend, "to_tool_calls", fake_to_tool_calls) + + backend = _make_backend() + mot = ModelOutputThunk( + value="reasoning herecall get_weather(city='Boston')" + ) + mot._call.action = CBlock("What's the weather?") + mot._call.model_options = {} + + await backend.post_processing( + mot, + conversation=[], + _format=None, + tool_calls=True, + tools={}, + seed=None, + input_ids=None, + ) + + assert mot.thinking == "reasoning here" + assert mot.value == "call get_weather(city='Boston')" + assert recorded_text == ["call get_weather(city='Boston')"] + + +async def test_post_processing_skips_split_when_streaming() -> None: + """Streaming generations must not have mot.value shrunk by post_processing. + + Regression test for a real bug: ModelOutputThunk.astream() (mellea/core/base.py) + computes each delta from an offset captured before post_processing runs, assuming + mot._underlying_value only ever grows during streaming. If post_processing + replaces mot.value with the shorter post-split answer, that offset goes stale and + the final astream() delta is corrupted (truncated or empty) — verified separately + by simulating astream()'s delta math against this exact before/after state. + """ + backend = _make_backend() + mot = ModelOutputThunk(value="reasoning herethe answer") + mot.generation.streaming = True + mot._call.action = CBlock("test") + mot._call.model_options = {} + + await backend.post_processing( + mot, + conversation=[], + _format=None, + tool_calls=False, + tools={}, + seed=None, + input_ids=None, + ) + + assert mot.thinking is None + assert mot.value == "reasoning herethe answer" + + +async def test_post_processing_does_not_split_without_thinking_template_var() -> None: + """A chat template with no thinking-related variable must not trigger the split. + + Regression test: without this gate, any answer that happens to contain the + literal substring "" (e.g. a question about chat templates) would have + legitimate answer text incorrectly moved into mot.thinking. + """ + backend = _make_backend(thinking_template_var=None) + mot = ModelOutputThunk(value="Use the tag to close a reasoning block.") + mot._call.action = CBlock("How do reasoning tags work?") + mot._call.model_options = {} + + await backend.post_processing( + mot, + conversation=[], + _format=None, + tool_calls=False, + tools={}, + seed=None, + input_ids=None, + ) + + assert mot.thinking is None + assert mot.value == "Use the tag to close a reasoning block." diff --git a/test/backends/test_huggingface_thinking_e2e.py b/test/backends/test_huggingface_thinking_e2e.py new file mode 100644 index 0000000000..2a2163c256 --- /dev/null +++ b/test/backends/test_huggingface_thinking_e2e.py @@ -0,0 +1,95 @@ +# Copyright IBM Corp. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""End-to-end tests for LocalHFBackend's ... tag splitting. + +Unlike test_huggingface_thinking.py (pure unit tests against synthetic strings), +these tests run real generation against granite-4.2-3b to verify the raw text +produced by `model.generate()` + `tokenizer.decode(skip_special_tokens=True)` +actually has the shape _split_think_tags assumes. This matters because +and are registered as *non-special* added tokens in granite-4.2's +tokenizer (special: false), so skip_special_tokens=True does not strip them — +but that is a property of this specific model/tokenizer, not something a +synthetic-string test can verify. +""" + +import os + +import pytest + +from test.predicates import require_gpu + +torch = pytest.importorskip("torch", reason="torch not installed — install mellea[hf]") + +pytestmark = [ + pytest.mark.huggingface, + pytest.mark.e2e, + pytest.mark.qualitative, + require_gpu(min_vram_gb=8), + pytest.mark.skipif( + int(os.environ.get("CICD", 0)) == 1, + reason="Skipping HuggingFace thinking e2e tests in CI - qualitative test", + ), +] + +import mellea.backends.model_ids as model_ids +from mellea import MelleaSession +from mellea.backends import ModelOption +from mellea.backends.cache import SimpleLRUCache +from mellea.backends.huggingface import LocalHFBackend +from mellea.stdlib.context import ChatContext +from test.conftest import hf_skip + + +@pytest.fixture(scope="module") +def backend(): + """Shared granite-4.2-3b HuggingFace backend for all tests in this module.""" + with hf_skip(): + backend = LocalHFBackend( + model_id=model_ids.IBM_GRANITE_4_2_3B, cache=SimpleLRUCache(5) + ) + yield backend + + from test.conftest import cleanup_gpu_backend + + cleanup_gpu_backend(backend, "huggingface-thinking") + + +@pytest.fixture(scope="function") +def session(backend): + """Fresh HuggingFace session for each test.""" + session = MelleaSession(backend, ctx=ChatContext()) + yield session + session.reset() + + +def test_thinking_enabled_populates_mot_thinking(session): + """ModelOption.THINKING=True: mot.thinking holds the real reasoning trace, + mot.value is the clean answer with no leftover tag, and the answer + itself still contains the expected content (not just an empty string that + would vacuously satisfy the tag-absence checks alone).""" + output = session.instruct( + "What is 2 + 2? Answer with just the number.", + model_options={ModelOption.THINKING: True, ModelOption.MAX_NEW_TOKENS: 400}, + ) + assert output.thinking, ( + f"Expected a non-empty reasoning trace, got: {output.thinking!r}" + ) + # granite-4.2's chat template bakes the opening into the generation + # prompt itself (see module docstring / _split_think_tags), so the model's own + # output never contains it — checking for its absence here would be vacuous. + # is the tag that actually appears in raw output and must be split out. + assert "" not in output.value + assert "4" in output.value + + +def test_thinking_disabled_leaves_mot_thinking_none(session): + """ModelOption.THINKING=False: no think block is generated, so mot.thinking + is falsy (None or empty string — both mean "no reasoning trace captured").""" + output = session.instruct( + "What is 2 + 2? Answer with just the number.", + model_options={ModelOption.THINKING: False, ModelOption.MAX_NEW_TOKENS: 100}, + ) + assert not output.thinking, f"Expected no reasoning trace, got: {output.thinking!r}" + assert "" not in output.value + assert "4" in output.value From 74f5d6d7f5fce78071b00b7af97d13182d9a3d48 Mon Sep 17 00:00:00 2001 From: Nigel Jones Date: Fri, 4 Sep 2026 09:52:53 +0100 Subject: [PATCH 02/10] docs: add design proposal for HF backend reasoning handling Issue #1610 asked LocalHFBackend to split Granite's block into mot.thinking; PR #1616 implemented it, and review surfaced cache-key timing, to_chat round-tripping, gating truthiness, and token-boundary questions the original issue didn't anticipate. Writes up a full RFC at docs/dev/proposals/1604-hf-output-parsing.md resolving those four findings plus several more (streaming, intrinsic input changes, replay policy layering), verified directly against the code and Granite's own chat template, with open questions for further review. Assisted-by: Claude Code Signed-off-by: Nigel Jones --- docs/dev/proposals/1604-hf-output-parsing.md | 767 +++++++++++++++++++ 1 file changed, 767 insertions(+) create mode 100644 docs/dev/proposals/1604-hf-output-parsing.md diff --git a/docs/dev/proposals/1604-hf-output-parsing.md b/docs/dev/proposals/1604-hf-output-parsing.md new file mode 100644 index 0000000000..c723c94d3b --- /dev/null +++ b/docs/dev/proposals/1604-hf-output-parsing.md @@ -0,0 +1,767 @@ +# HF backend reasoning (`` tag) handling — design proposal + +> **Status:** Draft proposal, not agreed. Do not implement beyond the +> narrow slice already merging in PR [#1616](https://github.com/generative-computing/mellea/pull/1616) +> until the decisions in Part I §5 are settled. +> +> **Addresses:** [#1604](https://github.com/generative-computing/mellea/issues/1604) +> (umbrella: "implement better hugging face output parsing" — this doc is the +> design work that issue asked for) and the four review comments on PR #1616, +> which implements the narrower [#1610](https://github.com/generative-computing/mellea/issues/1610) +> ("add think tag parsing for granite models to hugging face backend"). +> +> **Structure:** Part I is the ask — read it alone and you can say yes/no to +> the shape. Part II is supporting detail: current-state analysis, the +> upstream `transformers` mechanism, the Granite/Qwen3 template mechanism, +> and a full finding list. Appendix indexes referenced issues/PRs and the +> verification trail. +> +> **Terminology stance:** this doc uses *reasoning* and *thinking* +> interchangeably (both appear in code and in HF/Granite/Qwen naming); *think +> tags* refers specifically to the ``/`` textual delimiters. +> Full glossary in Part II §7. + +## Part I — Summary for agreement + +### §1 Problem + +**The reported symptom.** `LocalHFBackend` never populated +`ModelOutputThunk.thinking`. Every other backend Mellea supports — Ollama, +LiteLLM, OpenAI, WatsonX — gets reasoning pre-separated from the answer by its +SDK: the transport delivers `thinking`/`reasoning_content` as a field distinct +from `content`. HF's `transformers.generate()` returns one flat token +sequence; decoding it (`mellea/backends/huggingface.py:1701-1710`, +`skip_special_tokens=True`) leaves Granite's `...` block glued +inline to `mot.value`, unparsed, visible to end users, and liable to confuse +anything scanning the answer text (tool-call detection, requirement checks). +Granite's ``/`` tokens are registered `special: false` in its +tokenizer, so `skip_special_tokens=True` does not strip them — they survive +into the decoded string as literal text. + +PR #1616 fixes this for the common case: a `_split_think_tags()` helper, +wired into `post_processing()`, partitions the decoded text on the first +`` when the active chat template declares a thinking variable. + +**The deeper framing.** HF is the *only* backend where Mellea, not the +provider SDK, owns the reasoning/answer split. That makes this a genuine +backend-local policy decision — where the split happens, what "raw" means +once it has, and how a split value round-trips back into history — not +transport plumbing. Mellea has never written that policy down, and issue +#1610 asked only for the split itself, not for the policy around it. + +**The scope framing.** The reviewer who wrote #1610 (jakelorocco) left four +inline comments on PR #1616 and said directly: *"I think I did not realize +how many aspects of the hf backend this would impact when I created the +issue. I think there's actually a fair bit of design work that might be +required to address these concerns."* Investigation for this doc confirms +all four comments are real, and surfaces five more gaps the four comments +didn't cover. The table below shows why a 45-line diff touches this much +surface: + +| Subsystem touched | How | +|---|---| +| LRU cache (`_cache`/`cache_get`/`cache_put`) | Cache key computed from the pre-split string's object identity | +| History serialization (`to_chat`) | Reasoning is dropped on replay, and — new finding — the model's own template reacts to that absence | +| Chat-template introspection (`_chat_template_allowlist`) | Gate only checks which variable *names* a template declares, never resolved values | +| Tool-call scanning, stop-string/finish-reason derivation | Correctly read different values (split vs. raw) — confirmed right, but undocumented as intentional | +| `GenerateLog` / observability | Reasoning is absent from the trace | +| Intrinsic adapter functions (RAG/core aLoRAs) | Now receive reasoning-free response text where they previously didn't — a real, untested behaviour change | +| Streaming (`astream()`) | Split is skipped entirely; `m serve` still shows raw tags | + +### §2 Goals / non-goals + +**Goals:** +- One written policy for *where* the reasoning/answer split happens, *what + value* each downstream consumer (cache, tool-call scanner, `to_chat`, + intrinsics, logging) should read, and *how* a split value round-trips + through multi-turn history. +- Resolve the four PR #1616 review comments with a stated position each, not + just a restatement of the problem. +- Name what stays deliberately out of scope, and why. + +**Non-goals:** +- Incremental (streaming-safe) splitting. Already deferred to #1604 by PR + #1616's own comments; this doc keeps that deferral but insists it be named + prominently (§6), not buried, since it is the gap most visible to `m + serve` users. +- A general multi-convention reasoning parser for models Mellea doesn't ship + against (channel-based conventions like gpt-oss, bracket conventions like + `[THINK]`). See Part II §13 (Generality). +- Revisiting `should_replay_reasoning`'s existing cross-backend consensus + rule (reasoning replays only on an assistant turn that issued a tool call) + — from prior work tracked in #1201, referenced in + `mellea/helpers/openai_compatible_helpers.py:321-354`. This doc asks + whether to *apply* that rule to HF, not whether to change it. + +### §3 Key terms + +- **Capture** — reasoning text becoming `mot.thinking` (separate from + `mot.value`), for the turn just generated. +- **Replay** — a *previous* turn's `Message.thinking` being sent back to the + model as part of the next turn's input. +- **Template-declared thinking variable** — a Jinja variable name + (`enable_thinking`, `think`, `thinking`) that a model's chat template + references, detected today by static AST introspection + (`_chat_template_allowlist`, Part II §8), independent of what value was + actually passed for it. +- Full glossary: Part II §7. + +### §4 Decisions + +Each decision states the recommended position and the alternative rejected. +Corresponding open questions (where the position isn't fully settled) are in +§5. + +**D1 — Where the split happens, and what "raw" means.** *Shipped.* +Recommended, and now implemented: keep exactly one split point, in +`post_processing()`, and treat the pre-split decoded string (captured +locally as `raw_value` before the split) as the canonical "raw" text for +that turn. Every consumer either reads before this point (raw) or after +(split) — no third copy. The executable sequence, as shipped: (1) build +`cache_info` from `hf_output`'s KV-cache/scores fields and clear those +fields from `hf_output`, but do **not** cache-key or cache-put yet; (2) +capture `raw_value = mot.value`; (3) run the resolved-value-gated split +(D3), reassigning `mot.value`; (4) only now compute `cache_key = id(mot.value)` +and `cache_put()` (D6) — on the post-split object, not the pre-split one; +(5) the stop-string check reads `raw_value` exclusively, never `mot.value`. +Rejected alternative: adding a new public `raw`-text field to +`ModelOutputThunk` speculatively, before any consumer outside this backend +needs one (see D6 / Q6). + +**D2 — Boundary detection: token check, string match, or declared schema.** +Recommended: prefer `transformers`' own `PreTrainedTokenizerBase.parse_response()` +(reads a per-tokenizer `response_schema`) when a tokenizer declares one; +fall back to `_split_think_tags()`'s string partition otherwise, and treat +the fallback explicitly as a fallback in its docstring (currently it reads +as the primary mechanism). See Part II §10 for why upstream's own +`parse_response()` is *also* text-level, not token-level — the reviewer's +"look at the tokens" suggestion (PR comment on `huggingface.py:278`) does +not resolve the ambiguity for Granite, because Granite's real `` +token is itself non-special (Part II §10). Rejected alternative: building a +token-ID-based disambiguator for Granite specifically — verified not to +work for this model family (Part II §10), so building it would be dead +code. + +**D3 — Gate on declared variable name alone, or also on resolved value.** +*Shipped.* Recommended, and now implemented: gate on both — the template +must declare a thinking variable *and* the resolved per-call value +(`mot._call.model_options.get(ModelOption.THINKING)` — the same dict +`_filter_for_chat_template` reads `ModelOption.THINKING` from when +resolving the pre-generation template kwargs, so this mirrors what the +model was actually asked to do) must not be explicitly `False`. A +`None`/unset value still allows the split, since both Granite 4.2 and +Qwen3 default thinking to `True` (`chat_template.jinja:13`: +`enable_thinking if ... is defined else True`). Rejected alternative: the +prior behaviour (declared-name only) — confirmed to false-positive +whenever a template declares the variable regardless of its resolved +value (Part II §9, F3). + +**D4 — Replay wire format for HF's `to_chat`.** *Interim forward shipped; +final policy still open (Q3).* The interim fix now attaches +`reasoning_content` to the assistant wire dict in `to_chat()` +**unconditionally** (whenever `Message.thinking` is non-empty), not yet +gated by `should_replay_reasoning()`. This was necessary sooner than this +doc's full agreement: shipping D1's capture fix alone, without any replay +forward, silently changed multi-turn HF Granite prompt content (§6, §9 F2) +— every assistant turn without `reasoning_content` gets an empty +`` prepended by Granite's own template +(`chat_template.jinja:89-90`), so a plain capture fix drops reasoning the +model previously saw. The unconditional attach restores the pre-#1610 +parity (raw tags were inline on every turn before) without deciding D5's +real question. **The final, gated design this doc still recommends** — +attach `reasoning_content` only when `should_replay_reasoning()` says yes +(`openai_compatible_helpers.py:321-354`), matching OpenAI/LiteLLM/WatsonX +(`openai_compatible_helpers.py:471`: `result["reasoning_content"] = msg.thinking`) +— is Q3's open question, not yet implemented. Rejected alternative (the +original reviewer's suggestion at `huggingface.py:1853`): saving a raw +unsplit copy and re-inlining `...` directly into `content` +on replay — contradicts the wire-format convention every other backend +follows, and is unnecessary now that `reasoning_content` is confirmed to +be a real, consumed template variable (Part II §11). + +**D5 — Whose replay policy governs: Mellea's or the template's own.** +*Open — this is what the interim D4 forward deliberately left undecided.* +Recommended: once D4 moves off the unconditional interim, apply +`should_replay_reasoning()` first (keeps HF consistent with every other +backend's replay rule), and *document* — not fight — the fact that +Granite's template applies a second, looser gate of its own +(`truncate_history_thinking`, defaulting to `True`, dropping reasoning on +turns before the last user message — `chat_template.jinja:18,99`). A +consequence worth naming explicitly rather than letting D4/D5 quietly +absorb it: applying `should_replay_reasoning()` means a **plain assistant +turn that issued no tool call permanently loses replayed reasoning** on +every subsequent turn, even though the interim unconditional forward +currently preserves it. That is a deliberate policy choice inherited from +the #1201 consensus rule, not an oversight — see Q3 for the explicit +decision this doc asks for. Rejected alternative: skipping Mellea's policy +and relying solely on the template's own truncation, which would make HF's +replay behaviour diverge from OpenAI/LiteLLM/WatsonX for no reason tied to +HF's actual constraints. + +**D6 — Cache-key fix timing.** *Shipped.* +Recommended, and now implemented: the split runs before +`cache_key = id(mot.value)` is computed (D1's sequence), so the key is +derived from the same string object `mot.value` holds afterward. +`cache_get()` had zero call sites anywhere in `mellea/` or `test/` before +this fix — this was a latent correctness fix for a not-yet-built consumer, +not a live-bug fix, and a test now exists (Part II §15 item 9) asserting +the key is retrievable, making it the first in-tree `cache_get()` caller. +Rejected alternative: leaving it, on the grounds that nothing reads the cache today +— rejected because a dead-path bug keyed on Python object identity is +invisible to whoever wires up the first reader, and the fix is one line. + +### §5 Open questions + +Numbered; every question here also appears, unrestated, in Part II §17 (a +back-reference, not a repeated version) with its full context. These are the decisions this doc is *not* making unilaterally. + +1. **Will Granite tokenizers ever ship a `response_schema`?** (Cross-team: + Granite model/tokenizer team.) D2 recommends preferring + `transformers.PreTrainedTokenizerBase.parse_response()` when a tokenizer + declares `response_schema`. No Granite tokenizer publishes one today + (Part II §10), so today's fallback string-parser is the *only* path. If + the Granite team plans to add one, this doc's fallback parser is + transitional and should be structured as a fallback from day one (already + D2's recommendation); if not, it's the permanent mechanism and deserves + more structure than a single private helper function. +2. **First or last ``?** Granite's own template keeps only the text + *after the last* closing tag on replay (`chat_template.jinja:107`: + `c.split('')[-1]`); `_split_think_tags()` partitions on the + *first* occurrence, and PR #1616's own unit test + (`test/backends/test_huggingface_thinking.py:77-79`) pins that. Match the + model's own convention (last), or keep first-occurrence and accept the + documented divergence? This changes an already-merging test assertion. +3. **Replay-policy layering (D5), and the plain-turn consequence it commits to.** + Confirm applying `should_replay_reasoning()` before the template's own + `truncate_history_thinking` gate, per D5's recommendation, rather than + relying on the template alone or keeping the current unconditional + interim forward (D4) permanently. This has a concrete, permanent + consequence that must be decided explicitly rather than absorbed as a + side effect of "resolving" D4/D5: gating on `should_replay_reasoning()` + means a plain assistant turn that issued no tool call **permanently + loses replayed reasoning** on every later turn, even though today's + interim unconditional forward preserves it. Accept that consequence (it + matches the #1201 consensus rule every other backend already follows), + or decide HF should replay reasoning on plain turns too — a genuine, + named divergence from that consensus, not an oversight? +4. **Silent key drop on templates without `reasoning_content`.** Granite + 4.1, 4.0-micro, granite-switch-4.1-3b-preview, and other non-4.2 model + templates don't declare `reasoning_content` at all. `apply_chat_template` + silently drops unknown keys (documented in `merge_provider_fields`'s + docstring), so attaching `reasoning_content` on those templates is a + silent no-op — reasoning is requested to replay but nothing renders. + Accept the silent no-op, add a one-time debug log gated by + `_chat_template_allowlist`, or refuse to attach the key at all unless the + active template declares it? +5. **Is the raw/batch generation path in scope?** `_generate_from_raw` (the + non-chat completion path) never splits at all — it has no chat template + to introspect, so the whole gating mechanism doesn't apply. Leave this + path unsplit and documented as out of scope, or is there demand for + reasoning-splitting on raw completions too? +6. **Promote the local `raw_value` var to a public field?** D1 keeps "raw" + as the existing local variable at `huggingface.py:1839`. Should this + become a real field on `ModelOutputThunk` (a public API addition scoped + to one backend's benefit) so external code — not just HF-internal + consumers — can reach the pre-split text? No consumer outside this + backend needs it today. +7. **Does the intrinsic-input behaviour change (Part II §12, G4) need a + re-baseline?** Adapter functions (`mellea/stdlib/components/intrinsic/`) + read prior assistant response text via `turn.output.value` + (`mellea/stdlib/components/intrinsic/_util.py:104-109,249`). On HF, that + text previously included raw `` blocks and now (post PR #1616) + doesn't. This is a correctness improvement, but it changes the literal + input intrinsic aLoRAs see on HF. Is this a documented bug fix, or does + it warrant re-running intrinsic evals before merge to confirm no + regression in adapter accuracy? +8. **Can PR #1616 merge independently of this doc?** Recommended: yes, once + narrowed to D3/D6 plus the observability and docstring items in Part II + §16 — all uncontroversial, no open design tradeoff. Confirm this framing. +9. **Doc placement and numbering.** This file is placed at + `docs/dev/proposals/1604-hf-output-parsing.md`, numbered under #1604 (the + umbrella issue) rather than #1610 (the narrow bug), because #1604 is what + this design actually resolves. Confirm, or rename/renumber to #1610. +10. **Prose dialect.** Written in UK spelling per the author's standing + convention; the repository states no dialect preference in + `AGENTS.md`/`CLAUDE.md`. Confirm, or switch to US spelling for + consistency with the rest of the docs tree. + +### §6 Impact and blast radius + +**API surface.** No new public fields proposed by the recommended positions +(D1 rejects a new `ModelOutputThunk.raw_value` field pending Q6; D4 uses the +existing `Message.thinking`/wire-dict mechanism other backends already use). +If Q6 resolves toward "yes, promote it," that is the one public API change +this doc could produce. + +**User-archetype impact:** + +| User | Before PR #1616 | After PR #1616 (narrowed, per §16) | After this doc's full recommendations | +|---|---|---|---| +| Non-streaming HF chat user | Raw `` block visible in the answer | Clean split; `mot.thinking` populated | Same, plus correct gating on explicit `THINKING=False` | +| `m serve` (streaming) user | Raw `` block visible | **Unchanged — still raw** (streaming split deferred, §6/G1) | Unchanged until #1604's incremental-splitting work | +| Multi-turn HF chat, tool-call turns | Reasoning silently dropped from history (pre-existing gap, not introduced by #1610/#1616) | Still dropped | Reasoning replays via `reasoning_content` per D4/D5 | +| Intrinsic adapter (RAG/core aLoRA) caller on HF | Adapter input included raw `` text | Adapter input is reasoning-free (Q7) | Same, with the change explicitly documented | +| Anything reading `GenerateLog` for HF | Trace includes full raw text | Trace has post-split text only, reasoning absent (§16) | Reasoning added back to the trace | + +**Code reach:** confined to `mellea/backends/huggingface.py` and +`mellea/backends/utils.py`; touches `mellea/helpers/openai_compatible_helpers.py` +only by reuse (no changes needed there — `should_replay_reasoning()` and +`message_to_openai_message()`'s `reasoning_content` pattern are reused +as-is). No changes to `mellea/core/base.py`'s `GenFields` hook contract — +confirmed there is no shared cross-backend `post_processing()` signature to +respect (Part II §8), so this stays backend-local. + +**Release planning.** Target release: minor version, exact number TBD — +depends on when Q1–Q9 settle. + +**Risk register:** +- Splitting on the *first* `` (current behaviour, Q2 open) risks + truncating an answer if the model ever emits a genuine second use of the + literal text `` inside its answer — low probability, unverified + frequency. +- The `to_chat` regression this doc identifies (D4's motivation): PR #1616 + as merged, *before* D4 ships, changes multi-turn prompt content for HF + Granite conversations (Granite's template re-inlines an empty + `` onto any assistant turn lacking `reasoning_content` — + `chat_template.jinja:89-90`). This is a behaviour change introduced by the + capture fix, not a pre-existing gap, and should be called out in PR + #1616's own description regardless of this doc's timeline. +- Silent no-op risk (Q4) if `reasoning_content` ships before its no-op + behaviour on older Granite templates is decided. + +**Blocking / unblocking.** This doc blocks D4/D5/D2's full resolution (F2, +F4 in Part II §9) from being implemented in PR #1616. It does not block the +narrower fixes in §16, which ship independently per Q8. + +--- + +## Part II — Supporting detail + +### §7 Full glossary + +| Term | Meaning | +|---|---| +| Capture | Reasoning text becoming `mot.thinking`, separate from `mot.value`, for the turn just generated | +| Replay | A previous turn's `Message.thinking` being sent back to the model in the next turn's input | +| Wire message | The `dict` built for a provider's chat API / `apply_chat_template` call — `{"role": ..., "content": ..., ...}` | +| Template-declared thinking variable | A Jinja variable name (`think`/`thinking`/`enable_thinking`) a chat template's source references, per static AST introspection | +| Resolved thinking value | The actual boolean forwarded for that variable on a specific generation call, derived from `ModelOption.THINKING` | +| Response schema | `transformers`' declarative per-tokenizer metadata describing how to parse structured content (e.g. reasoning) out of generated text — see §10 | +| Raw (this doc) | The fully-decoded text for a generation, before `_split_think_tags()` runs | +| Split / answer | The post-`_split_think_tags()` value assigned to `mot.value` | + +### §8 Current-state analysis, per code path + +**Chat path — `post_processing()`** (`huggingface.py:1756-1930`): +1. KV-cache metadata captured, keyed `cache_key = id(mot.value)` (line 1820) + — **before** any split. +2. `raw_value = mot.value` captured locally (line 1839) — this is the "raw" + text D1 recommends canonicalising. +3. Gate check (lines 1841-1850): `thinking_allowlist.intersection(_CHAT_TEMPLATE_THINKING_VARS)` + — declared-name check only, no value check (F3, D3). +4. `_split_think_tags(raw_value)` (line 1851) — first-occurrence string + partition (D2, Q2). +5. Tool-call scan (`to_tool_calls`) runs on the now-split `mot.value` — + correct: reasoning must not be scanned for tool calls, and Granite emits + `` after content, so a false-positive split at worst truncates + an answer prefix, not a tool call (Part II §12, G6 — no fix needed, just + documented as intentional). +6. Stop-string / finish-reason derivation (lines 1908-1924) reads `raw_value` + (the *pre-split* text), not `mot.value` — correct as written, since a + stop string could itself be ``-adjacent; this local var is + effectively already D1's "raw" concept, just not yet named as + canonical or documented as intentional (G5). + +**Raw/batch path — `_generate_from_raw`:** no chat template, no gating +mechanism, no split. Text returned as-is (Q5 — in/out of scope). + +**`to_chat()`** (`mellea/backends/utils.py:74-134`) — *state as of this +doc's first draft, before D4's interim forward shipped:* built each wire +message from `{"role": m.role, "content": formatter.print(m)}` only — +`m.thinking` was never read. A pre-existing comment already named this gap, +referencing #1201 (the capture-gap issue this fix closes), not #1604 as an +earlier draft of this doc mistakenly said. As of D4's interim forward +(§4, §9 F2), `to_chat()` now attaches `reasoning_content` unconditionally; +the comment there now cites this doc and #1604 for the remaining, +still-open gated-replay design (D5/Q3). + +**`Message._parse()`** (`mellea/stdlib/components/chat.py:207-292`): the HF +fallback branch (there is no HF-specific provider branch — HF's raw +response has no role/content structure to parse, so it falls through to a +generic path) *does* correctly carry `thinking` forward: `Message(role="assistant", +content=computed.value, thinking=computed.thinking)` (lines 264, 291). The +drop is specifically and only inside `to_chat()`, not here. + +**Two unrelated caches, worth distinguishing explicitly** (reviewers will +conflate them): the `_cache`/`cache_get`/`cache_put`/`SimpleLRUCache` +mechanism this doc's D6 fixes is *not* the multi-turn KV-cache-continuation +mechanism (`_make_merged_kv_cache`, `self._cached_blocks: dict[str, DynamicCache]`), +which is keyed by literal `CBlock` content strings and is unaffected by +anything in this doc. + +**Streaming (`astream()`):** the gate explicitly excludes streaming +generations (`if not mot.generation.streaming and ...`, line 1848) — the +split never runs during/after a stream. `astream()` itself is a pure +text-length diff (`beginning_length = len(str(mot._underlying_value))`) with +no notion of think tags at all. + +### §9 Finding-by-finding resolution + +**F1 — Cache key computed pre-split** (PR comment, `huggingface.py:1820`). +Confirmed: `cache_key = id(mot.value)` is Python object-identity of the +pre-split string; `_split_think_tags()` later reassigns `mot.value` to a new +string object, changing its `id()`. Confirmed dead path today: `cache_get()` +(`huggingface.py:2247`ish) has zero call sites in `mellea/` or `test/`. +Resolution: D6 — fix now regardless of doc outcome (one-line reorder). + +**F2 — `to_chat` round-trip** (PR comment, `huggingface.py:1853`) — live +bug, bigger than the original comment suggested. Confirmed: `to_chat()` +drops `m.thinking` unconditionally. New finding beyond the PR comment: +Granite's template re-inlines an *empty* `` onto any +assistant turn lacking `reasoning_content` (`chat_template.jinja:89-90`), +so PR #1616's capture fix — by finally putting reasoning into a field +`to_chat()` doesn't forward — silently changes multi-turn prompt content +for HF Granite conversations that previously carried the inline block +straight through. This is a regression the capture fix introduces, not +merely a missing round-trip feature — and because it changes model input +before any replay mitigation ships, it was treated as blocking rather than +deferrable. *Mitigated:* `to_chat()` now attaches `reasoning_content` +unconditionally (D4's interim forward), restoring parity with the pre-fix +behavior. *Still open:* the final, gated design (attach only per +`should_replay_reasoning()`, D5) remains an open question (Q3), including +the plain-turn consequence named there explicitly. + +**F3 — Gating checks declared name, not resolved value** (PR comment, +`huggingface.py:1850`). Confirmed via direct read of the gate logic +(§8 step 3) and the template (`chat_template.jinja:13`, default `True`). +The existing e2e test for "thinking disabled" +(`test/backends/test_huggingface_thinking_e2e.py`, +`test_thinking_disabled_leaves_mot_thinking_none`) passes for an unrelated +reason: Granite genuinely emits no `` when thinking is off, not +because the gate itself checks the resolved value. Resolution: D3. + +**F4 — Token vs. text boundary detection** (PR comment, +`huggingface.py:278`) — confirmed, and the reviewer's own suggested +direction ("look at the tokens") does not resolve it for this model family. +See §10 for the full mechanism trace. Resolution: D2 (prefer +`response_schema`/`parse_response` when available; otherwise the existing +string fallback, explicitly labelled as a fallback, combined with D3's +resolved-value gate to reduce — not eliminate — false-positive risk). + +Beyond the four review comments, this investigation found: + +**G1 — Streaming still shows raw tags.** Already named in the PR's own +comments as deferred to #1604. This doc keeps that deferral (non-goal, §2) +but insists §6's impact table name it explicitly, since it's the gap most +visible to interactive users. + +**G2 — Raw/batch path never splits.** §8, Q5. + +**G3 — `GenerateLog` drops reasoning from the trace.** Other backends' +`GenerateLog` equivalents record the full provider response (which includes +reasoning); HF's records only the post-split `mot.value`. Resolution: +include reasoning in the log or its `extra` field — cheap, no design +tradeoff, part of §16's narrow slice. + +**G4 — Intrinsic adapter functions see different input text than before.** +`mellea/stdlib/components/intrinsic/_util.py:104-109,249` reads +`turn.output.value` to build adapter input. HF and OpenAI are the only two +`AdapterMixin`-capable backends today. Before PR #1616, HF intrinsic input +on a thinking-enabled turn included the raw `` block; after, it +doesn't. No existing test pins either the old or the new behaviour. This is +almost certainly a correctness improvement (adapters shouldn't see +reasoning noise) but is an unannounced behaviour change with real +consequences for anyone running intrinsic evals against HF. See Q7. + +**G5 — Stop-string check correctly reads `raw_value`.** Confirmed correct +as written (§8); flagged here only so a future refactor doesn't +"helpfully" switch it to `mot.value` and break stop-string detection when a +stop string happens to be near a think-tag boundary. Resolution: add a +one-line comment pinning this, part of §16. + +**G6 — Tool-call scan correctly reads post-split `mot.value`.** Confirmed +correct (§8); no fix needed, included here so reviewers don't re-litigate +it as a bug. + +**G7 — Two layered replay policies.** Mellea's `should_replay_reasoning()` +(tool-call turns only) and Granite's own `truncate_history_thinking` +(all turns at/after the last user message) are both real and both would +apply if D4 ships. Resolution: D5. + +**G8 — `Message._parse`'s HF fallback already carries `thinking` correctly.** +Confirmed (§8); included so no one "fixes" `_parse()` — the bug is +downstream, in `to_chat()`, only. + +**G9 — No shared `post_processing()` contract across backends.** Confirmed: +every backend defines its own signature; the only shared contract is +`GenFields.process`/`post_process` coroutine hook slots in +`mellea/core/base.py`, invoked generically by `ModelOutputThunk` without +knowledge of each backend's internals. Included as a scope-limiter: nothing +in D1–D6 requires a cross-backend architecture change — it's all local to +`huggingface.py`/`utils.py`. + +### §10 The upstream mechanism (`transformers.parse_response`) + +Per this skill's "verify upstream before inventing a parallel concept" +rule: `transformers` (as vendored in this environment) already ships +`PreTrainedTokenizerBase.parse_response(response, schema=None)`, which +reads `tokenizer.response_schema` and, when present, runs a +`recursive_parse` over the *decoded text* to split out structured content +(including reasoning) per the schema. When no schema is set, it raises +`AttributeError`. + +Two things follow directly from reading this mechanism, both load-bearing +for D2 and F4: + +1. **No tokenizer available to Mellea today — including every Granite + variant checked — declares a `response_schema`.** So `parse_response()` + is not usable today; `_split_think_tags()`'s string fallback is the only + working path, exactly as #1604 anticipated when it asked for "better + output parsing." +2. **Even `parse_response()` operates on decoded text, not raw token + IDs.** It calls `self.decode()` internally before parsing. This means + the reviewer's "look at the tokens" suggestion (PR comment, + `huggingface.py:278`) is not how upstream itself solves this class of + problem either — upstream's answer to "don't confuse literal text with + a control delimiter" is a *declared schema*, not token inspection. + Verified separately (§8, §10): Granite's actual `` token is + itself registered `special: false` in its tokenizer, so even a + token-identity check (had `_split_think_tags` been given token access) + would not distinguish "the model emitted the genuine end-of-reasoning + token" from "the model wrote the four characters `` as prose" — + both decode to the identical string, and the real token carries no + distinguishing flag either way. + +Conclusion: `_split_think_tags()` should be documented explicitly as a +fallback for `parse_response()`/`response_schema`, not as the primary +mechanism it currently reads as. If/when Granite tokenizers gain a +`response_schema` (Q1, a cross-team ask), Mellea should prefer that path. + +### §11 The model's own template as source of truth + +Verified directly against the on-disk artefact (not inferred): +`~/.cache/huggingface/hub/models--ibm-granite--granite-4.2-3b/snapshots/.../chat_template.jinja`. + +- Line 13: `{%- set enable_thinking = enable_thinking if enable_thinking is defined else True %}` + — thinking defaults **on**. +- Line 18: `{%- set truncate_history_thinking = truncate_history_thinking if truncate_history_thinking is defined else True %}` + — history truncation of reasoning also defaults **on**. +- Lines 83-84: `{%- if message.reasoning_content is defined and message.reasoning_content is string and ... %}` + → `{%- set content = "\n" ~ message.reasoning_content ~ "\n\n" ~ (message.content | default('', true)) %}` + — **the template already has a working mechanism for accepting prior + reasoning via a `reasoning_content` key and re-inlining it.** This is the + mechanism D4 recommends using. +- Lines 89-90: `{%- if '' not in content and '' not in content -%}{%- set content = "" ~ content -%}` — + the source of F2's newly-found regression: any assistant turn arriving + *without* think tags (i.e., without `reasoning_content` set) gets an empty + `` pair silently prepended. +- Lines 99, 105-112: history-truncation logic — turns before the last user + message get reasoning dropped (if `truncate_history_thinking`), and + turns that keep reasoning split on the **last** `` occurrence + (`c.split('')[-1]`) — the opposite occurrence rule from + `_split_think_tags()`'s first-occurrence partition (Q2). +- Lines 137-145: a second truncation branch with the same last-occurrence + rule (`c.split('')[-1]`), confirming this is the template's + consistent convention, not a one-off. +- Lines 179-182: generation-prompt construction opens `\n` (or the + already-closed `` when `enable_thinking` is false) — this + confirms the opening tag is baked into the *prompt*, matching + `_split_think_tags()`'s docstring rationale for splitting on `` + alone. + +Qwen3's chat template (checked for generality, not reproduced verbatim +here) uses the same `reasoning_content` key and a `preserve_thinking`-style +gate with equivalent semantics — this is a cross-model Jinja convention +among reasoning models with think-tag delimiters, not a Granite-only +mechanism. Named as such in §13. + +Round-trip sequence (proposed, D4/D5): + +```mermaid +sequenceDiagram + participant U as User turn N + participant M as Model (assistant turn N) + participant H as post_processing() + participant C as to_chat() (turn N replay, building turn N+1 request) + U->>M: prompt (template opens \n) + M->>H: raw decoded text, "...reasoning...answer" + H->>H: split_think_tags -> mot.thinking, mot.value + Note over H: D3: only if resolved THINKING != False + C->>C: should_replay_reasoning(turn N) ? + alt turn N issued a tool call + C->>M: wire dict: {role: assistant, content: answer, reasoning_content: thinking} + M->>M: template re-inlines ...reasoning...answer + else no tool call + C->>M: wire dict: {role: assistant, content: answer} + M->>M: template prepends empty (F2) + end +``` + +### §12 Consumer matrix + +| Consumer | Reads today | Should read (proposed) | Notes | +|---|---|---|---| +| End user (`mot.value`) | Split answer (post PR #1616) | Same | Correct already | +| Tool-call scan (`to_tool_calls`) | Split `mot.value` | Same | G6 — correct, document as intentional | +| Stop-string / finish-reason | Pre-split `raw_value` | Same | G5 — correct, document as intentional | +| LRU cache key (`cache_key = id(mot.value)`) | Pre-split object identity | Post-split, or a stable key | F1/D6 | +| `to_chat()` wire message | `mot.value` only | `mot.value` + `reasoning_content` (gated) | F2/D4/D5 | +| `GenerateLog` | Post-split `mot.value` only | Include reasoning | G3 | +| Intrinsic adapter input (`_extract_last_response`) | Split, reasoning-free text (post PR #1616) | Same (this is the improvement) | G4 — behaviour change, needs Q7 | +| `astream()` deltas | Raw, unsplit, growing string | Unchanged (non-goal, §2) | G1 | +| `_generate_from_raw` completions | Raw, unsplit | Unchanged unless Q5 says otherwise | G2 | + +### §13 Generality + +Is this design Granite-specific? Partially, and this doc should say so +plainly rather than imply universality: + +- **Generalises:** the declared-thinking-variable gate (D3), the + `reasoning_content` replay key (D4), and the ``/`` literal + boundary all generalise cleanly to Qwen3 (confirmed, §11) and to any + future Granite-family model following the same template convention. +- **Does not generalise:** models using channel-based reasoning conventions + (e.g. gpt-oss's channel markers) or bracket conventions (`[THINK]...[/THINK]`) + would need a different boundary detector entirely — `_split_think_tags()` + is scoped to the ``/`` textual convention by design + (its own docstring already says this), and this doc doesn't propose + changing that scope. +- If a second convention arrives, the natural extension point is a small + per-model-family convention table keyed by the same + `_chat_template_allowlist` introspection this doc already relies on — not + designed here, since no second model needing it exists yet. + +### §14 Observability + +- `GenerateLog` should carry reasoning (G3) so debug/eval traces aren't + missing it — matches what OpenAI/LiteLLM/WatsonX backends already log. +- A debug log line already exists on successful split + (`huggingface.py`, `MelleaLogger.get_logger().debug(...)` after the split) + — recommend a matching debug line when the gate *would* have split but + the resolved-value check (D3) suppressed it, to make Q4's silent-no-op + risk debuggable. + +### §15 Docs and tests + +All items below currently have zero coverage — required, not optional, +before the design's full recommendations (D4/D2/D5) ship: + +1. Gate honours explicit `ModelOption.THINKING=False` with synthetic text + containing a literal `` — asserts no split (the case F3/D3 fixes). +2. Gate still splits when `THINKING` is unset (`None`) on a template that + thinks by default — guards against over-correcting D3. +3. `to_chat()` with a non-empty `Message.thinking`: asserts `reasoning_content` + present on the assistant wire dict for a tool-call turn, absent for a + plain turn (existing `test/backends/test_utils.py` `to_chat` tests don't + set `.thinking` at all today). +4. HF rows added to `test/backends/test_reasoning_replay.py` (currently + covers OpenAI/Ollama/WatsonX only). +5. A rendered-prompt assertion: run `apply_chat_template` over a two-turn + conversation and assert reasoning appears in the rendered string — the + only test that catches Q4's silent-key-drop failure mode. +6. Truncated-reasoning case: thinking on, generation hits `max_new_tokens` + before any `` appears — assert whatever behaviour this doc's + resolution of D2/D3 settles on (today: `_split_think_tags` returns + `(None, text)`, silently surfacing the entire reasoning block as the + answer). +7. Occurrence-rule test updated or explicitly justified if Q2 resolves + toward last-occurrence (currently pinned first-occurrence at + `test/backends/test_huggingface_thinking.py:77-79`). +8. An intrinsic-level test pinning that adapter functions receive + reasoning-free response text on HF (G4/Q7). +9. A cache test: assert the key computed in `post_processing()` is + retrievable via `cache_get()` after the split (F1/D6) — would be the + first in-tree caller of `cache_get()`. +10. A guard test confirming a model whose template declares no thinking + variable never attempts a split. + +### §16 Migration / sequencing + +**Shipped in PR #1616, independent of this doc's remaining open questions +(Q8):** D6 (cache-key reorder, with the test at §15 item 9 — the first +in-tree `cache_get()` caller), D3 (resolved-value gate, with the tests at +§15 items 1-2), G3 (`GenerateLog` reasoning), G5 (pin the `raw_value` +intent with a comment), relabelling `_split_think_tags()`'s docstring as a +`response_schema` fallback (§10), and — moved up from "waits for D5" +because leaving it unaddressed would have shipped a silent multi-turn +prompt regression (F2) — D4's **interim, unconditional** `reasoning_content` +forward in `to_chat()`, with the round-trip tests at §15 item 3. PR #1616's +description flags G4 (Q7) so reviewers running intrinsic evals know input +text changed, and discloses the F2 regression this interim forward +mitigates. + +**Waits for this doc's decisions (D4's final gated form, D5, D2/Q2, Q4, Q5, Q6):** +everything else. Sequencing once agreed: D4's move from unconditional to +`should_replay_reasoning()`-gated and D5 (replay policy layering) ship +together as one PR (they're the same code path, and D5's plain-turn +consequence per §5 Q3 must be decided first); D2's +`response_schema` preference ships whenever Q1 resolves (likely later, +gated on an external team); Q5 (raw path) and Q6 (public field promotion) +are independent follow-ups if their answers are "yes." + +### §17 Open questions (full list) + +Back-reference only — each item below is the same decision as its Part I +§5 counterpart, not a restated version. See §5 for the question text. + +1. Q1 → Part I §5.1 (`response_schema` availability; cross-team, Granite tokenizer team). +2. Q2 → Part I §5.2 (first- vs. last-`` occurrence; see §11 for the template evidence). +3. Q3 → Part I §5.3 (replay-policy layering, D5). +4. Q4 → Part I §5.4 (silent `reasoning_content` key drop on older templates). +5. Q5 → Part I §5.5 (raw/batch path in scope?). +6. Q6 → Part I §5.6 (promote `raw_value` to a public field?). +7. Q7 → Part I §5.7 (intrinsic-input behaviour change; re-baseline needed?). +8. Q8 → Part I §5.8 (can PR #1616 merge independently of this doc?). +9. Q9 → Part I §5.9 (doc placement and numbering). +10. Q10 → Part I §5.10 (prose dialect). + +--- + +## Appendix + +### Tracking items + +| Ref | Relation to this doc | +|---|---| +| [#1604](https://github.com/generative-computing/mellea/issues/1604) | Umbrella issue this doc resolves; this doc's numbering | +| [#1610](https://github.com/generative-computing/mellea/issues/1610) | The narrow bug PR #1616 fixes; this doc's proximate trigger | +| [#1201](https://github.com/generative-computing/mellea/issues/1201) (referenced, not re-opened) | Prior cross-backend consensus on `should_replay_reasoning`; this doc asks whether to apply it to HF, not to change it | +| PR [#1616](https://github.com/generative-computing/mellea/pull/1616) | Draft implementation under review; stays open per this doc's Part I §5 Q8 | + +### History and rework evidence + +- PR #1616, single commit `bf0f45e6`. Four inline review comments from + `jakelorocco` at `huggingface.py:278`, `:1820`, `:1850`, `:1853` + (verbatim quotes reproduced in Part I §1 and Part II §9). +- Top-level review comment from `jakelorocco`: *"I think I did not realize + how many aspects of the hf backend this would impact when I created the + issue. I think there's actually a fair bit of design work that might be + required to address these concerns."* +- Pre-existing acknowledgement of the `to_chat` gap already in the codebase + before this PR, at `mellea/backends/utils.py:100-104`, referencing **#1201** + (corrected from an earlier draft of this doc, which incorrectly cited #1604) — + evidence the gap was known before #1610 was filed. + +### Related in-flight work + +- Streaming-safe incremental splitting (deferred non-goal, §2/G1) — tracked + under #1604, not designed in this doc. + +### Verification trail + +- Cache-key, `to_chat` drop, gating logic, and token-boundary claims: + traced against `mellea/backends/huggingface.py`, `mellea/backends/utils.py`, + `mellea/helpers/openai_compatible_helpers.py`, and + `mellea/stdlib/components/chat.py` at the commit checked out in this + worktree (`bf0f45e6` head of `issue-1610`); line numbers re-verified + directly (not solely from prior research notes) immediately before this + doc was written. +- Granite 4.2 template mechanism (`reasoning_content`, last-occurrence + split, default-thinking-`True`): verified directly against + `~/.cache/huggingface/hub/models--ibm-granite--granite-4.2-3b/snapshots/b7e947307dd2efb3ad3b853b0e8a7e75f8ad4ac2/chat_template.jinja` + lines 13, 18, 83-118, 137-145, 179-182 on disk. +- `transformers.parse_response`/`response_schema` mechanism (§10): verified + against the vendored `transformers` source in this environment; confirmed + no Granite tokenizer checked declares a `response_schema`. +- Intrinsic adapter consumption (G4): verified against + `mellea/stdlib/components/intrinsic/_util.py:104-109,249`. From c0394e03c69535e232b63f8e73a862064748bf40 Mon Sep 17 00:00:00 2001 From: Nigel Jones Date: Fri, 4 Sep 2026 09:53:22 +0100 Subject: [PATCH 03/10] fix(hf): resolve cache-key timing, gating truthiness, and reasoning replay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback on PR #1616 (see docs/dev/proposals/1604-hf-output-parsing.md): - Reorder the KV-cache key computation to run after the split, not before, so a future cache_get() call can actually find the entry by the same key mot.value holds afterward. - Gate the split on the resolved per-call ModelOption.THINKING value, not just on the chat template declaring a thinking variable — a template declaring the variable is not proof thinking was requested on this call. - Attach reasoning captured in mot.thinking to the assistant wire message in to_chat() as reasoning_content (Granite/Qwen3 templates already consume this key). This is an interim, unconditional forward, not yet gated by should_replay_reasoning() like other backends — needed now because the capture fix alone silently dropped reasoning on multi-turn HF Granite replay, since Granite's template re-inlines an empty onto any assistant turn missing reasoning_content. - Include captured reasoning in GenerateLog, matching other backends. - Fix a fabricated citation in a test docstring and move a qualitative marker from module to function level per test/README.md. Assisted-by: Claude Code Signed-off-by: Nigel Jones --- mellea/backends/huggingface.py | 57 +++++++-- mellea/backends/utils.py | 16 ++- test/backends/test_huggingface_thinking.py | 111 +++++++++++++++++- .../backends/test_huggingface_thinking_e2e.py | 5 +- test/backends/test_utils.py | 41 +++++++ 5 files changed, 211 insertions(+), 19 deletions(-) diff --git a/mellea/backends/huggingface.py b/mellea/backends/huggingface.py index 2a8ef7abf8..b383474191 100644 --- a/mellea/backends/huggingface.py +++ b/mellea/backends/huggingface.py @@ -279,13 +279,26 @@ def _cleanup_kv_cache(cache_info: HFAloraCacheInfo) -> None: def _split_think_tags(text: str) -> tuple[str | None, str]: - """Split raw HF output into (thinking, answer) on the closing tag. + r"""Split raw HF output into (thinking, answer) on the closing tag. + + A string-level fallback for `transformers.PreTrainedTokenizerBase.parse_response()` + (schema-driven, token-decode-then-parse), used because no tokenizer available to + this backend today declares a `response_schema` — see the design proposal at + docs/dev/proposals/1604-hf-output-parsing.md §10 for why even that upstream + mechanism is text-level, not token-level, and can't disambiguate a genuine + end-of-reasoning token from literal `` text for Granite either. Splits on alone (not a ... pair) because some chat templates (e.g. granite-4.2 with enable_thinking) bake the opening tag into the prompt, so it never appears in the model's own output. A fixed pattern match for Granite's convention, not a general reasoning-parser; other delimiters pass through unchanged. See #1604 for generalizing this. + + Deliberately strips leading/trailing whitespace from both `thinking` and + `answer`: the whitespace immediately around the tags is delimiter framing + (Granite's template emits `\n...\n\n`), not meaningful + content, so the user-visible completion loses that framing whitespace as + part of this split, not incidentally. """ if _THINK_CLOSE_TAG not in text: return None, text @@ -1797,6 +1810,10 @@ class used during generation, if any. if isinstance(hf_output, GenerateDecoderOnlyOutput) and mot._call.model_options: self._surface_logits(mot, hf_output) + # Built here (before the split below) because it needs `hf_output`'s KV cache/scores + # fields, which are cleared immediately after; cached under a key derived from + # `mot.value` further down, once the split has settled on the final string object. + cache_info: HFAloraCacheInfo | None = None if ( self._use_caches and isinstance(hf_output, GenerateDecoderOnlyOutput) @@ -1817,9 +1834,6 @@ class used during generation, if any. scores=hf_output.scores, ) - cache_key = id(mot.value) - self.cache_put(cache_key, cache_info) - # Clear KV cache and scores from HF output; retained via LRU cache above. # `ModelOutput` (`OrderedDict` subclass) does not sync `None` writes back # to the mapping, so plain attribute assignment leaves the dict entry — and @@ -1835,18 +1849,36 @@ class used during generation, if any. OrderedDict.__delitem__(hf_output, "logits") hf_output.logits = None - # Capture the raw text before any split below, for the stop-string check further down. + # Capture the raw text before any split below. Used for the stop-string check + # further down (a stop string could itself be think-tag-adjacent, so that check + # must stay on the pre-split text) and as the input to the split itself. Do not + # repoint this at `mot.value` after the split runs. raw_value = mot.value - # Gate on the template exposing a thinking var (not ModelOption.THINKING) since some - # models think by default; otherwise an answer that just mentions "" gets split. + # Gate on the template exposing a thinking var (some models think by default, so + # declaring the var isn't itself proof thinking is on) AND the resolved per-call + # value not being explicitly False (an answer that merely mentions "" on a + # template with thinking off must not be split). `None`/unset must still allow the + # split: Granite and Qwen3 both default `enable_thinking` to True in their own + # template source, so treating "unset" as "off" would under-split for the common case. + # Read from `mot._call.model_options` directly (not a value already filtered for the + # template) — it's the same dict `_filter_for_chat_template` resolves + # `ModelOption.THINKING` from when building the generation-time template kwargs, so + # this mirrors what the model was actually asked to do on this call. # Skip for streaming: astream() assumes mot.value only grows, and shrinking it here # would corrupt the final delta (see #1604 for proper incremental splitting later). thinking_allowlist: frozenset[str] = getattr( self, "_chat_template_allowlist", frozenset() ) - if not mot.generation.streaming and thinking_allowlist.intersection( - _CHAT_TEMPLATE_THINKING_VARS + resolved_thinking = ( + mot._call.model_options.get(ModelOption.THINKING) + if mot._call.model_options + else None + ) + if ( + not mot.generation.streaming + and resolved_thinking is not False + and thinking_allowlist.intersection(_CHAT_TEMPLATE_THINKING_VARS) ): thinking, answer = _split_think_tags(raw_value) if thinking is not None: @@ -1858,6 +1890,12 @@ class used during generation, if any. self._model_id, ) + if cache_info is not None: + # Keyed after the split above has settled on the final `mot.value` object, so a + # later lookup by the same key against the same (now-split) thunk can find it. + cache_key = id(mot.value) + self.cache_put(cache_key, cache_info) + # Only scan for tools if we are not doing structured output and tool calls were provided to the model. if _format is None and tool_calls: mot.tool_calls = to_tool_calls(tools, mot.value) @@ -1965,6 +2003,7 @@ class used during generation, if any. "tools_available": tools, "tools_called": mot.tool_calls, "seed": seed, + "thinking": mot.thinking, } generate_log.action = mot._call.action generate_log.result = mot diff --git a/mellea/backends/utils.py b/mellea/backends/utils.py index 4a3ad874a1..c288bd1f7f 100644 --- a/mellea/backends/utils.py +++ b/mellea/backends/utils.py @@ -99,9 +99,17 @@ def to_chat( # NOTE: `self.formatter.to_chat_messages` explicitly skips `Message` objects. However, we need # to print `Message`s to correctly serialize any documents with the message. Do the printing here. - # NOTE: reasoning is never replayed on the HF chat path — we serialize only `content` and never - # consult `should_replay_reasoning` (unlike OpenAI/LiteLLM/Watsonx/Ollama). `Message.thinking` - # is now captured for HF's Granite convention, but replay isn't wired in yet; #1604. + # NOTE: reasoning replay here is an interim, unconditional forward — every non-empty + # `Message.thinking` is attached as `reasoning_content` regardless of `should_replay_reasoning` + # (unlike OpenAI/LiteLLM/Watsonx/Ollama, which gate replay on that policy). This exists to + # restore the parity the HF think-tag capture fix (#1610) removed: before that fix, raw + # `...` text sat inline in `content` on every turn, so Granite's chat template + # (chat_template.jinja:83-90) always saw prior reasoning; after the fix, `content` is tag-free + # and the template silently prepends an empty `` instead, dropping reasoning the + # model previously saw. Attaching `reasoning_content` unconditionally restores that prior + # behavior without deciding the real design question (whether HF should follow the + # tool-call-only consensus rule from #1201 on plain turns too) — see the design proposal at + # docs/dev/proposals/1604-hf-output-parsing.md, D5/Q3, for the full replay-policy decision. ctx_as_conversation: list = [] for m in ctx_as_message_list: msg_dict: dict = {"role": m.role, "content": formatter.print(m)} @@ -112,6 +120,8 @@ def to_chat( msg_dict["tool_calls"] = m.tool_calls if m.tool_call_id: msg_dict["tool_call_id"] = m.tool_call_id + if m.thinking: + msg_dict["reasoning_content"] = m.thinking # Merge any author-declared provider fields (Mellea's known fields win; # a mismatched target raises). Must run after the known fields are set. msg_dict = merge_provider_fields(msg_dict, m.provider_fields, "huggingface") diff --git a/test/backends/test_huggingface_thinking.py b/test/backends/test_huggingface_thinking.py index 85f4a40f4b..0fec2795d5 100644 --- a/test/backends/test_huggingface_thinking.py +++ b/test/backends/test_huggingface_thinking.py @@ -14,6 +14,7 @@ torch = pytest.importorskip("torch", reason="torch not installed — install mellea[hf]") import mellea.backends.huggingface as hf_backend +from mellea.backends import ModelOption from mellea.backends.huggingface import LocalHFBackend, _split_think_tags from mellea.core.base import CBlock, ModelOutputThunk @@ -71,29 +72,42 @@ def test_split_think_tags_leading_whitespace_before_open_tag() -> None: def test_split_think_tags_multiple_close_tags_uses_first() -> None: """Multiple occurrences: only the first is treated as the boundary. - Matches the first-match-wins convention used by transformers' own serving - utilities (cli/serving/utils.py) for the same start/end tag pattern. + Pins current behavior, not a settled design choice: Granite's own chat + template splits replayed history on the *last* occurrence + (chat_template.jinja, e.g. `c.split('')[-1]`), the opposite rule. + Whether to match the model's own convention is an open question — see + docs/dev/proposals/1604-hf-output-parsing.md, Q2. """ thinking, answer = _split_think_tags("abc") assert thinking == "a" assert answer == "bc" -def _make_backend(*, thinking_template_var: str | None = "think") -> LocalHFBackend: +def _make_backend( + *, thinking_template_var: str | None = "think", use_caches: bool = False +) -> LocalHFBackend: """Return a LocalHFBackend with __init__ bypassed, wired with the minimum state post_processing needs when there is no real GenerateDecoderOnlyOutput - (i.e. every isinstance(hf_output, GenerateDecoderOnlyOutput) branch is skipped). + (i.e. every isinstance(hf_output, GenerateDecoderOnlyOutput) branch is skipped + unless use_caches=True and the caller also sets mot.raw.response). Args: thinking_template_var: name of a thinking-related Jinja variable to bake into the fake chat template (gates the think-split in post_processing), or None for a template that does not reference any of them. + use_caches: whether to wire up a real `SimpleLRUCache` so the KV-cache + branch in post_processing actually runs. """ b: LocalHFBackend = LocalHFBackend.__new__(LocalHFBackend) b._model_id = "test-org/test-model" b.model_id = "test-org/test-model" b._provider = "huggingface" - b._use_caches = False + b._use_caches = use_caches + if use_caches: + from mellea.backends.cache import SimpleLRUCache + + object.__setattr__(b, "_cache", SimpleLRUCache(5)) + object.__setattr__(b, "_device", torch.device("cpu")) template = ( f"{{{{ {thinking_template_var} }}}}" @@ -197,3 +211,90 @@ async def test_post_processing_does_not_split_without_thinking_template_var() -> assert mot.thinking is None assert mot.value == "Use the tag to close a reasoning block." + + +async def test_post_processing_does_not_split_when_thinking_explicitly_false() -> None: + """Regression test: a template declaring a thinking var is not proof thinking + was requested on this specific call. With ModelOption.THINKING explicitly + False, a literal "" in the answer must survive untouched, even though + the template declares "think" (the gate must check the resolved per-call value, + not just whether the template mentions the variable name at all). + """ + backend = _make_backend(thinking_template_var="think") + mot = ModelOutputThunk(value="Use the tag to close a reasoning block.") + mot._call.action = CBlock("How do reasoning tags work?") + mot._call.model_options = {ModelOption.THINKING: False} + + await backend.post_processing( + mot, + conversation=[], + _format=None, + tool_calls=False, + tools={}, + seed=None, + input_ids=None, + ) + + assert mot.thinking is None + assert mot.value == "Use the tag to close a reasoning block." + + +async def test_post_processing_splits_when_thinking_unset() -> None: + """Regression test: an unset/None ModelOption.THINKING must still allow the + split when the template declares a thinking var, since Granite and Qwen3 both + default thinking to True in their own template source — treating "unset" as + "off" would under-split the common case. + """ + backend = _make_backend(thinking_template_var="think") + mot = ModelOutputThunk(value="reasoning herethe answer") + mot._call.action = CBlock("test") + mot._call.model_options = {} + + await backend.post_processing( + mot, + conversation=[], + _format=None, + tool_calls=False, + tools={}, + seed=None, + input_ids=None, + ) + + assert mot.thinking == "reasoning here" + assert mot.value == "the answer" + + +async def test_post_processing_cache_key_findable_after_split() -> None: + """Regression test: the LRU cache key computed in post_processing must be + derived from mot.value *after* the split has run, so a later cache_get() call + against the same (now-split) thunk's value can find the entry. Before the + fix, the key was computed from the pre-split string's object identity, then + mot.value was reassigned to a new string a few lines later — orphaning the + cache entry under a key nothing would ever look up again. + """ + from transformers.generation.utils import GenerateDecoderOnlyOutput + + backend = _make_backend(thinking_template_var="think", use_caches=True) + sequences = torch.tensor([[1, 2, 3, 4]]) + hf_output = GenerateDecoderOnlyOutput( + sequences=sequences, scores=(torch.zeros(1, 10),) + ) + mot = ModelOutputThunk(value="reasoning herethe answer") + mot.raw.response = hf_output + mot._call.action = CBlock("test") + mot._call.model_options = {} + + await backend.post_processing( + mot, + conversation=[], + _format=None, + tool_calls=False, + tools={}, + seed=None, + input_ids=torch.tensor([[1, 2]]), + ) + + assert mot.thinking == "reasoning here" + assert mot.value == "the answer" + cached = backend.cache_get(id(mot.value)) + assert cached is not None diff --git a/test/backends/test_huggingface_thinking_e2e.py b/test/backends/test_huggingface_thinking_e2e.py index 2a2163c256..7cd8fe63ed 100644 --- a/test/backends/test_huggingface_thinking_e2e.py +++ b/test/backends/test_huggingface_thinking_e2e.py @@ -24,7 +24,6 @@ pytestmark = [ pytest.mark.huggingface, pytest.mark.e2e, - pytest.mark.qualitative, require_gpu(min_vram_gb=8), pytest.mark.skipif( int(os.environ.get("CICD", 0)) == 1, @@ -63,6 +62,7 @@ def session(backend): session.reset() +@pytest.mark.qualitative def test_thinking_enabled_populates_mot_thinking(session): """ModelOption.THINKING=True: mot.thinking holds the real reasoning trace, mot.value is the clean answer with no leftover tag, and the answer @@ -83,7 +83,8 @@ def test_thinking_enabled_populates_mot_thinking(session): assert "4" in output.value -def test_thinking_disabled_leaves_mot_thinking_none(session): +@pytest.mark.qualitative +def test_thinking_disabled_leaves_mot_thinking_falsy(session): """ModelOption.THINKING=False: no think block is generated, so mot.thinking is falsy (None or empty string — both mean "no reasoning trace captured").""" output = session.instruct( diff --git a/test/backends/test_utils.py b/test/backends/test_utils.py index b226766bb3..4b467472eb 100644 --- a/test/backends/test_utils.py +++ b/test/backends/test_utils.py @@ -151,6 +151,47 @@ def test_to_chat_basic_message(): assert result[1]["content"] == "next question" +def test_to_chat_attaches_reasoning_content_for_assistant_thinking(): + """Regression test: an assistant Message carrying `.thinking` must have it + forwarded as `reasoning_content` on the wire dict. Before this fix, `to_chat` + read only `m.role`/`m.content` and silently dropped `.thinking` on every + HF replay — see docs/dev/proposals/1604-hf-output-parsing.md, D4. + """ + from mellea.backends.utils import to_chat + from mellea.formatters.template_formatter import TemplateFormatter as ChatFormatter + from mellea.stdlib.components import Message + from mellea.stdlib.context import ChatContext + + ctx = ChatContext() + ctx = ctx.add(Message("user", "hello")) + ctx = ctx.add(Message("assistant", "the answer", thinking="reasoning trace")) + action = Message("user", "next question") + formatter = ChatFormatter(model_id="test") + + result = to_chat(action, ctx, formatter, system_prompt=None) + assistant_msg = next(m for m in result if m["role"] == "assistant") + assert assistant_msg["reasoning_content"] == "reasoning trace" + + +def test_to_chat_omits_reasoning_content_when_no_thinking(): + """An assistant Message with no captured reasoning must not get a + `reasoning_content` key at all (not even an empty string).""" + from mellea.backends.utils import to_chat + from mellea.formatters.template_formatter import TemplateFormatter as ChatFormatter + from mellea.stdlib.components import Message + from mellea.stdlib.context import ChatContext + + ctx = ChatContext() + ctx = ctx.add(Message("user", "hello")) + ctx = ctx.add(Message("assistant", "the answer")) + action = Message("user", "next question") + formatter = ChatFormatter(model_id="test") + + result = to_chat(action, ctx, formatter, system_prompt=None) + assistant_msg = next(m for m in result if m["role"] == "assistant") + assert "reasoning_content" not in assistant_msg + + def test_to_chat_with_system_prompt(): from mellea.backends.utils import to_chat from mellea.formatters.template_formatter import TemplateFormatter as ChatFormatter From 2f2a2fe2c53648780dfea85d80b5dc58279ef0f8 Mon Sep 17 00:00:00 2001 From: Nigel Jones Date: Fri, 4 Sep 2026 10:06:55 +0100 Subject: [PATCH 04/10] test(hf): add collision test for reasoning_content vs provider_fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the last outstanding item from qwen's review of the design proposal and PR #1616: document and test the conflict policy when a Message.provider_fields entry also targets reasoning_content for the huggingface provider. to_chat() already set reasoning_content before merge_provider_fields() runs (same known-fields-first ordering as tool_calls/tool_call_id), so Mellea's own value silently wins; this pins that behavior with a test and records it as D4's conflict policy in the design proposal. Also fixes stale line-number citations and status markers in the proposal (Message._parse range, §15/§16 shipped-item tracking, header banner) to reflect the code fixes committed in c0394e03. Assisted-by: Claude Code Signed-off-by: Nigel Jones --- docs/dev/proposals/1604-hf-output-parsing.md | 74 ++++++++++++++------ test/backends/test_utils.py | 30 ++++++++ 2 files changed, 81 insertions(+), 23 deletions(-) diff --git a/docs/dev/proposals/1604-hf-output-parsing.md b/docs/dev/proposals/1604-hf-output-parsing.md index c723c94d3b..ff543e3bc5 100644 --- a/docs/dev/proposals/1604-hf-output-parsing.md +++ b/docs/dev/proposals/1604-hf-output-parsing.md @@ -1,8 +1,10 @@ # HF backend reasoning (`` tag) handling — design proposal -> **Status:** Draft proposal, not agreed. Do not implement beyond the -> narrow slice already merging in PR [#1616](https://github.com/generative-computing/mellea/pull/1616) -> until the decisions in Part I §5 are settled. +> **Status:** Draft proposal, not agreed. D1, D3, D6, and D4's *interim +> unconditional* forward are implemented in PR +> [#1616](https://github.com/generative-computing/mellea/pull/1616) +> (still open as a draft). D2, D4's final gated form, and D5 remain +> unimplemented pending the decisions in Part I §5. > > **Addresses:** [#1604](https://github.com/generative-computing/mellea/issues/1604) > (umbrella: "implement better hugging face output parsing" — this doc is the @@ -177,7 +179,16 @@ original reviewer's suggestion at `huggingface.py:1853`): saving a raw unsplit copy and re-inlining `...` directly into `content` on replay — contradicts the wire-format convention every other backend follows, and is unnecessary now that `reasoning_content` is confirmed to -be a real, consumed template variable (Part II §11). +be a real, consumed template variable (Part II §11). **Conflict policy +(shipped):** `to_chat()` sets `reasoning_content` on the wire dict before +`merge_provider_fields()` runs — the same known-fields-first ordering +`tool_calls`/`tool_call_id` already use — so an author-declared +`Message.provider_fields` entry that also targets `reasoning_content` for +`"huggingface"` is silently dropped in favour of Mellea's own value (debug- +logged by `merge_provider_fields`, not raised), rather than either +colliding unpredictably or raising. Covered by +`test_to_chat_known_reasoning_content_wins_over_provider_fields` +(Part II §15 item 11). **D5 — Whose replay policy governs: Mellea's or the template's own.** *Open — this is what the interim D4 forward deliberately left undecided.* @@ -385,7 +396,7 @@ earlier draft of this doc mistakenly said. As of D4's interim forward the comment there now cites this doc and #1604 for the remaining, still-open gated-replay design (D5/Q3). -**`Message._parse()`** (`mellea/stdlib/components/chat.py:207-292`): the HF +**`Message._parse()`** (`mellea/stdlib/components/chat.py:212-292`): the HF fallback branch (there is no HF-specific provider branch — HF's raw response has no role/content structure to parse, so it falls through to a generic path) *does* correctly carry `thinking` forward: `Message(role="assistant", @@ -643,19 +654,26 @@ plainly rather than imply universality: ### §15 Docs and tests -All items below currently have zero coverage — required, not optional, -before the design's full recommendations (D4/D2/D5) ship: - -1. Gate honours explicit `ModelOption.THINKING=False` with synthetic text - containing a literal `` — asserts no split (the case F3/D3 fixes). -2. Gate still splits when `THINKING` is unset (`None`) on a template that - thinks by default — guards against over-correcting D3. -3. `to_chat()` with a non-empty `Message.thinking`: asserts `reasoning_content` - present on the assistant wire dict for a tool-call turn, absent for a - plain turn (existing `test/backends/test_utils.py` `to_chat` tests don't - set `.thinking` at all today). +Items 1, 2, 3, 9, and 11 are shipped alongside the code fixes in §16; the +rest wait on this doc's remaining open questions (D4's final gated form, +D2, Q2, Q4, Q5, Q7): + +1. **Shipped.** Gate honours explicit `ModelOption.THINKING=False` with + synthetic text containing a literal `` — asserts no split (F3/D3). + `test_post_processing_does_not_split_when_thinking_explicitly_false`. +2. **Shipped.** Gate still splits when `THINKING` is unset (`None`) on a + template that thinks by default — guards against over-correcting D3. + `test_post_processing_splits_when_thinking_unset`. +3. **Shipped, for the interim unconditional forward only.** `to_chat()` + with a non-empty `Message.thinking`: asserts `reasoning_content` present + on the assistant wire dict (`test_to_chat_attaches_reasoning_content_for_assistant_thinking`) + and absent when there is no captured reasoning + (`test_to_chat_omits_reasoning_content_when_no_thinking`). **Still + needed once D5 ships:** a version of this test asserting presence only + on a tool-call turn and absence on a plain turn, replacing the + unconditional assertion above. 4. HF rows added to `test/backends/test_reasoning_replay.py` (currently - covers OpenAI/Ollama/WatsonX only). + covers OpenAI/Ollama/WatsonX only) — waits on D5. 5. A rendered-prompt assertion: run `apply_chat_template` over a two-turn conversation and assert reasoning appears in the rendered string — the only test that catches Q4's silent-key-drop failure mode. @@ -666,21 +684,31 @@ before the design's full recommendations (D4/D2/D5) ship: answer). 7. Occurrence-rule test updated or explicitly justified if Q2 resolves toward last-occurrence (currently pinned first-occurrence at - `test/backends/test_huggingface_thinking.py:77-79`). + `test/backends/test_huggingface_thinking.py`, unchanged by this round). 8. An intrinsic-level test pinning that adapter functions receive reasoning-free response text on HF (G4/Q7). -9. A cache test: assert the key computed in `post_processing()` is - retrievable via `cache_get()` after the split (F1/D6) — would be the - first in-tree caller of `cache_get()`. +9. **Shipped.** A cache test: the key computed in `post_processing()` is + retrievable via `cache_get()` after the split (F1/D6) — the first + in-tree caller of `cache_get()`. + `test_post_processing_cache_key_findable_after_split`. 10. A guard test confirming a model whose template declares no thinking - variable never attempts a split. + variable never attempts a split — pre-existing, unchanged by this round + (`test_post_processing_does_not_split_without_thinking_template_var`). +11. **Shipped.** Collision test, per D4's conflict policy: a + `Message.provider_fields` entry author-declaring `reasoning_content` + for `"huggingface"` is silently overridden by Mellea's own value, + confirming `to_chat()` sets `reasoning_content` before + `merge_provider_fields` runs, the same known-fields-first ordering + `tool_calls`/`tool_call_id` already use. + `test_to_chat_known_reasoning_content_wins_over_provider_fields`. ### §16 Migration / sequencing **Shipped in PR #1616, independent of this doc's remaining open questions (Q8):** D6 (cache-key reorder, with the test at §15 item 9 — the first in-tree `cache_get()` caller), D3 (resolved-value gate, with the tests at -§15 items 1-2), G3 (`GenerateLog` reasoning), G5 (pin the `raw_value` +§15 items 1-2), D4's conflict policy against `merge_provider_fields` +(§15 item 11), G3 (`GenerateLog` reasoning), G5 (pin the `raw_value` intent with a comment), relabelling `_split_think_tags()`'s docstring as a `response_schema` fallback (§10), and — moved up from "waits for D5" because leaving it unaddressed would have shipped a silent multi-turn diff --git a/test/backends/test_utils.py b/test/backends/test_utils.py index 4b467472eb..1fd27b2f4a 100644 --- a/test/backends/test_utils.py +++ b/test/backends/test_utils.py @@ -173,6 +173,36 @@ def test_to_chat_attaches_reasoning_content_for_assistant_thinking(): assert assistant_msg["reasoning_content"] == "reasoning trace" +def test_to_chat_known_reasoning_content_wins_over_provider_fields(): + """Regression/contract test: `reasoning_content` is set from `Message.thinking` + before `merge_provider_fields` runs (same known-fields-first pattern as + `tool_calls`/`tool_call_id`), so an author-declared `provider_fields` + collision on the same key is silently dropped (debug-logged, not raised) — + Mellea's own value always wins. See docs/dev/proposals/1604-hf-output-parsing.md, D4. + """ + from mellea.backends.utils import to_chat + from mellea.formatters.template_formatter import TemplateFormatter as ChatFormatter + from mellea.stdlib.components import Message + from mellea.stdlib.context import ChatContext + + ctx = ChatContext() + ctx = ctx.add(Message("user", "hello")) + ctx = ctx.add( + Message( + "assistant", + "the answer", + thinking="reasoning trace", + provider_fields={"huggingface": {"reasoning_content": "author override"}}, + ) + ) + action = Message("user", "next question") + formatter = ChatFormatter(model_id="test") + + result = to_chat(action, ctx, formatter, system_prompt=None) + assistant_msg = next(m for m in result if m["role"] == "assistant") + assert assistant_msg["reasoning_content"] == "reasoning trace" + + def test_to_chat_omits_reasoning_content_when_no_thinking(): """An assistant Message with no captured reasoning must not get a `reasoning_content` key at all (not even an empty string).""" From 5c9f27632b1d58d0a97a99ecfa40c37e3dc7880f Mon Sep 17 00:00:00 2001 From: Nigel Jones Date: Fri, 4 Sep 2026 12:45:53 +0100 Subject: [PATCH 05/10] fix(hf): correct multi-turn reasoning replay disclosure, add missing coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 review found that the reasoning_content forward in to_chat() only restores prior parity for tool-call turns, not plain multi-turn: Granite's chat template strips reasoning whenever the serialized content carries both and , which the reconstructed form always does but the pre-fix inline form (closing tag only) never did. Corrects the utils.py comment and PR description to state this precisely instead of claiming full parity, and adds two tests against the real granite-4.2-3b template pinning both shapes. Also: - Gate the reasoning_content forward on role == "assistant". - Add a seam test proving Message._parse() -> to_chat() actually carries .thinking through for HF in both the tool-call and plain branches, not just by inspection. - Tighten an e2e assertion ("4" in output.value could match "40"/"14"). - Correct the _split_think_tags docstring to name the generation-prompt path (not the replay path) as the source of its whitespace framing. Removes the design-proposal doc used to plan this work — it was workflow scaffolding, not a deliverable. Its substance (behaviour change, open questions) now lives in the PR description. Assisted-by: Claude Code Signed-off-by: Nigel Jones --- docs/dev/proposals/1604-hf-output-parsing.md | 795 ------------------ mellea/backends/huggingface.py | 19 +- mellea/backends/utils.py | 29 +- test/backends/test_huggingface_thinking.py | 134 ++- .../backends/test_huggingface_thinking_e2e.py | 9 +- test/backends/test_utils.py | 24 +- 6 files changed, 191 insertions(+), 819 deletions(-) delete mode 100644 docs/dev/proposals/1604-hf-output-parsing.md diff --git a/docs/dev/proposals/1604-hf-output-parsing.md b/docs/dev/proposals/1604-hf-output-parsing.md deleted file mode 100644 index ff543e3bc5..0000000000 --- a/docs/dev/proposals/1604-hf-output-parsing.md +++ /dev/null @@ -1,795 +0,0 @@ -# HF backend reasoning (`` tag) handling — design proposal - -> **Status:** Draft proposal, not agreed. D1, D3, D6, and D4's *interim -> unconditional* forward are implemented in PR -> [#1616](https://github.com/generative-computing/mellea/pull/1616) -> (still open as a draft). D2, D4's final gated form, and D5 remain -> unimplemented pending the decisions in Part I §5. -> -> **Addresses:** [#1604](https://github.com/generative-computing/mellea/issues/1604) -> (umbrella: "implement better hugging face output parsing" — this doc is the -> design work that issue asked for) and the four review comments on PR #1616, -> which implements the narrower [#1610](https://github.com/generative-computing/mellea/issues/1610) -> ("add think tag parsing for granite models to hugging face backend"). -> -> **Structure:** Part I is the ask — read it alone and you can say yes/no to -> the shape. Part II is supporting detail: current-state analysis, the -> upstream `transformers` mechanism, the Granite/Qwen3 template mechanism, -> and a full finding list. Appendix indexes referenced issues/PRs and the -> verification trail. -> -> **Terminology stance:** this doc uses *reasoning* and *thinking* -> interchangeably (both appear in code and in HF/Granite/Qwen naming); *think -> tags* refers specifically to the ``/`` textual delimiters. -> Full glossary in Part II §7. - -## Part I — Summary for agreement - -### §1 Problem - -**The reported symptom.** `LocalHFBackend` never populated -`ModelOutputThunk.thinking`. Every other backend Mellea supports — Ollama, -LiteLLM, OpenAI, WatsonX — gets reasoning pre-separated from the answer by its -SDK: the transport delivers `thinking`/`reasoning_content` as a field distinct -from `content`. HF's `transformers.generate()` returns one flat token -sequence; decoding it (`mellea/backends/huggingface.py:1701-1710`, -`skip_special_tokens=True`) leaves Granite's `...` block glued -inline to `mot.value`, unparsed, visible to end users, and liable to confuse -anything scanning the answer text (tool-call detection, requirement checks). -Granite's ``/`` tokens are registered `special: false` in its -tokenizer, so `skip_special_tokens=True` does not strip them — they survive -into the decoded string as literal text. - -PR #1616 fixes this for the common case: a `_split_think_tags()` helper, -wired into `post_processing()`, partitions the decoded text on the first -`` when the active chat template declares a thinking variable. - -**The deeper framing.** HF is the *only* backend where Mellea, not the -provider SDK, owns the reasoning/answer split. That makes this a genuine -backend-local policy decision — where the split happens, what "raw" means -once it has, and how a split value round-trips back into history — not -transport plumbing. Mellea has never written that policy down, and issue -#1610 asked only for the split itself, not for the policy around it. - -**The scope framing.** The reviewer who wrote #1610 (jakelorocco) left four -inline comments on PR #1616 and said directly: *"I think I did not realize -how many aspects of the hf backend this would impact when I created the -issue. I think there's actually a fair bit of design work that might be -required to address these concerns."* Investigation for this doc confirms -all four comments are real, and surfaces five more gaps the four comments -didn't cover. The table below shows why a 45-line diff touches this much -surface: - -| Subsystem touched | How | -|---|---| -| LRU cache (`_cache`/`cache_get`/`cache_put`) | Cache key computed from the pre-split string's object identity | -| History serialization (`to_chat`) | Reasoning is dropped on replay, and — new finding — the model's own template reacts to that absence | -| Chat-template introspection (`_chat_template_allowlist`) | Gate only checks which variable *names* a template declares, never resolved values | -| Tool-call scanning, stop-string/finish-reason derivation | Correctly read different values (split vs. raw) — confirmed right, but undocumented as intentional | -| `GenerateLog` / observability | Reasoning is absent from the trace | -| Intrinsic adapter functions (RAG/core aLoRAs) | Now receive reasoning-free response text where they previously didn't — a real, untested behaviour change | -| Streaming (`astream()`) | Split is skipped entirely; `m serve` still shows raw tags | - -### §2 Goals / non-goals - -**Goals:** -- One written policy for *where* the reasoning/answer split happens, *what - value* each downstream consumer (cache, tool-call scanner, `to_chat`, - intrinsics, logging) should read, and *how* a split value round-trips - through multi-turn history. -- Resolve the four PR #1616 review comments with a stated position each, not - just a restatement of the problem. -- Name what stays deliberately out of scope, and why. - -**Non-goals:** -- Incremental (streaming-safe) splitting. Already deferred to #1604 by PR - #1616's own comments; this doc keeps that deferral but insists it be named - prominently (§6), not buried, since it is the gap most visible to `m - serve` users. -- A general multi-convention reasoning parser for models Mellea doesn't ship - against (channel-based conventions like gpt-oss, bracket conventions like - `[THINK]`). See Part II §13 (Generality). -- Revisiting `should_replay_reasoning`'s existing cross-backend consensus - rule (reasoning replays only on an assistant turn that issued a tool call) - — from prior work tracked in #1201, referenced in - `mellea/helpers/openai_compatible_helpers.py:321-354`. This doc asks - whether to *apply* that rule to HF, not whether to change it. - -### §3 Key terms - -- **Capture** — reasoning text becoming `mot.thinking` (separate from - `mot.value`), for the turn just generated. -- **Replay** — a *previous* turn's `Message.thinking` being sent back to the - model as part of the next turn's input. -- **Template-declared thinking variable** — a Jinja variable name - (`enable_thinking`, `think`, `thinking`) that a model's chat template - references, detected today by static AST introspection - (`_chat_template_allowlist`, Part II §8), independent of what value was - actually passed for it. -- Full glossary: Part II §7. - -### §4 Decisions - -Each decision states the recommended position and the alternative rejected. -Corresponding open questions (where the position isn't fully settled) are in -§5. - -**D1 — Where the split happens, and what "raw" means.** *Shipped.* -Recommended, and now implemented: keep exactly one split point, in -`post_processing()`, and treat the pre-split decoded string (captured -locally as `raw_value` before the split) as the canonical "raw" text for -that turn. Every consumer either reads before this point (raw) or after -(split) — no third copy. The executable sequence, as shipped: (1) build -`cache_info` from `hf_output`'s KV-cache/scores fields and clear those -fields from `hf_output`, but do **not** cache-key or cache-put yet; (2) -capture `raw_value = mot.value`; (3) run the resolved-value-gated split -(D3), reassigning `mot.value`; (4) only now compute `cache_key = id(mot.value)` -and `cache_put()` (D6) — on the post-split object, not the pre-split one; -(5) the stop-string check reads `raw_value` exclusively, never `mot.value`. -Rejected alternative: adding a new public `raw`-text field to -`ModelOutputThunk` speculatively, before any consumer outside this backend -needs one (see D6 / Q6). - -**D2 — Boundary detection: token check, string match, or declared schema.** -Recommended: prefer `transformers`' own `PreTrainedTokenizerBase.parse_response()` -(reads a per-tokenizer `response_schema`) when a tokenizer declares one; -fall back to `_split_think_tags()`'s string partition otherwise, and treat -the fallback explicitly as a fallback in its docstring (currently it reads -as the primary mechanism). See Part II §10 for why upstream's own -`parse_response()` is *also* text-level, not token-level — the reviewer's -"look at the tokens" suggestion (PR comment on `huggingface.py:278`) does -not resolve the ambiguity for Granite, because Granite's real `` -token is itself non-special (Part II §10). Rejected alternative: building a -token-ID-based disambiguator for Granite specifically — verified not to -work for this model family (Part II §10), so building it would be dead -code. - -**D3 — Gate on declared variable name alone, or also on resolved value.** -*Shipped.* Recommended, and now implemented: gate on both — the template -must declare a thinking variable *and* the resolved per-call value -(`mot._call.model_options.get(ModelOption.THINKING)` — the same dict -`_filter_for_chat_template` reads `ModelOption.THINKING` from when -resolving the pre-generation template kwargs, so this mirrors what the -model was actually asked to do) must not be explicitly `False`. A -`None`/unset value still allows the split, since both Granite 4.2 and -Qwen3 default thinking to `True` (`chat_template.jinja:13`: -`enable_thinking if ... is defined else True`). Rejected alternative: the -prior behaviour (declared-name only) — confirmed to false-positive -whenever a template declares the variable regardless of its resolved -value (Part II §9, F3). - -**D4 — Replay wire format for HF's `to_chat`.** *Interim forward shipped; -final policy still open (Q3).* The interim fix now attaches -`reasoning_content` to the assistant wire dict in `to_chat()` -**unconditionally** (whenever `Message.thinking` is non-empty), not yet -gated by `should_replay_reasoning()`. This was necessary sooner than this -doc's full agreement: shipping D1's capture fix alone, without any replay -forward, silently changed multi-turn HF Granite prompt content (§6, §9 F2) -— every assistant turn without `reasoning_content` gets an empty -`` prepended by Granite's own template -(`chat_template.jinja:89-90`), so a plain capture fix drops reasoning the -model previously saw. The unconditional attach restores the pre-#1610 -parity (raw tags were inline on every turn before) without deciding D5's -real question. **The final, gated design this doc still recommends** — -attach `reasoning_content` only when `should_replay_reasoning()` says yes -(`openai_compatible_helpers.py:321-354`), matching OpenAI/LiteLLM/WatsonX -(`openai_compatible_helpers.py:471`: `result["reasoning_content"] = msg.thinking`) -— is Q3's open question, not yet implemented. Rejected alternative (the -original reviewer's suggestion at `huggingface.py:1853`): saving a raw -unsplit copy and re-inlining `...` directly into `content` -on replay — contradicts the wire-format convention every other backend -follows, and is unnecessary now that `reasoning_content` is confirmed to -be a real, consumed template variable (Part II §11). **Conflict policy -(shipped):** `to_chat()` sets `reasoning_content` on the wire dict before -`merge_provider_fields()` runs — the same known-fields-first ordering -`tool_calls`/`tool_call_id` already use — so an author-declared -`Message.provider_fields` entry that also targets `reasoning_content` for -`"huggingface"` is silently dropped in favour of Mellea's own value (debug- -logged by `merge_provider_fields`, not raised), rather than either -colliding unpredictably or raising. Covered by -`test_to_chat_known_reasoning_content_wins_over_provider_fields` -(Part II §15 item 11). - -**D5 — Whose replay policy governs: Mellea's or the template's own.** -*Open — this is what the interim D4 forward deliberately left undecided.* -Recommended: once D4 moves off the unconditional interim, apply -`should_replay_reasoning()` first (keeps HF consistent with every other -backend's replay rule), and *document* — not fight — the fact that -Granite's template applies a second, looser gate of its own -(`truncate_history_thinking`, defaulting to `True`, dropping reasoning on -turns before the last user message — `chat_template.jinja:18,99`). A -consequence worth naming explicitly rather than letting D4/D5 quietly -absorb it: applying `should_replay_reasoning()` means a **plain assistant -turn that issued no tool call permanently loses replayed reasoning** on -every subsequent turn, even though the interim unconditional forward -currently preserves it. That is a deliberate policy choice inherited from -the #1201 consensus rule, not an oversight — see Q3 for the explicit -decision this doc asks for. Rejected alternative: skipping Mellea's policy -and relying solely on the template's own truncation, which would make HF's -replay behaviour diverge from OpenAI/LiteLLM/WatsonX for no reason tied to -HF's actual constraints. - -**D6 — Cache-key fix timing.** *Shipped.* -Recommended, and now implemented: the split runs before -`cache_key = id(mot.value)` is computed (D1's sequence), so the key is -derived from the same string object `mot.value` holds afterward. -`cache_get()` had zero call sites anywhere in `mellea/` or `test/` before -this fix — this was a latent correctness fix for a not-yet-built consumer, -not a live-bug fix, and a test now exists (Part II §15 item 9) asserting -the key is retrievable, making it the first in-tree `cache_get()` caller. -Rejected alternative: leaving it, on the grounds that nothing reads the cache today -— rejected because a dead-path bug keyed on Python object identity is -invisible to whoever wires up the first reader, and the fix is one line. - -### §5 Open questions - -Numbered; every question here also appears, unrestated, in Part II §17 (a -back-reference, not a repeated version) with its full context. These are the decisions this doc is *not* making unilaterally. - -1. **Will Granite tokenizers ever ship a `response_schema`?** (Cross-team: - Granite model/tokenizer team.) D2 recommends preferring - `transformers.PreTrainedTokenizerBase.parse_response()` when a tokenizer - declares `response_schema`. No Granite tokenizer publishes one today - (Part II §10), so today's fallback string-parser is the *only* path. If - the Granite team plans to add one, this doc's fallback parser is - transitional and should be structured as a fallback from day one (already - D2's recommendation); if not, it's the permanent mechanism and deserves - more structure than a single private helper function. -2. **First or last ``?** Granite's own template keeps only the text - *after the last* closing tag on replay (`chat_template.jinja:107`: - `c.split('')[-1]`); `_split_think_tags()` partitions on the - *first* occurrence, and PR #1616's own unit test - (`test/backends/test_huggingface_thinking.py:77-79`) pins that. Match the - model's own convention (last), or keep first-occurrence and accept the - documented divergence? This changes an already-merging test assertion. -3. **Replay-policy layering (D5), and the plain-turn consequence it commits to.** - Confirm applying `should_replay_reasoning()` before the template's own - `truncate_history_thinking` gate, per D5's recommendation, rather than - relying on the template alone or keeping the current unconditional - interim forward (D4) permanently. This has a concrete, permanent - consequence that must be decided explicitly rather than absorbed as a - side effect of "resolving" D4/D5: gating on `should_replay_reasoning()` - means a plain assistant turn that issued no tool call **permanently - loses replayed reasoning** on every later turn, even though today's - interim unconditional forward preserves it. Accept that consequence (it - matches the #1201 consensus rule every other backend already follows), - or decide HF should replay reasoning on plain turns too — a genuine, - named divergence from that consensus, not an oversight? -4. **Silent key drop on templates without `reasoning_content`.** Granite - 4.1, 4.0-micro, granite-switch-4.1-3b-preview, and other non-4.2 model - templates don't declare `reasoning_content` at all. `apply_chat_template` - silently drops unknown keys (documented in `merge_provider_fields`'s - docstring), so attaching `reasoning_content` on those templates is a - silent no-op — reasoning is requested to replay but nothing renders. - Accept the silent no-op, add a one-time debug log gated by - `_chat_template_allowlist`, or refuse to attach the key at all unless the - active template declares it? -5. **Is the raw/batch generation path in scope?** `_generate_from_raw` (the - non-chat completion path) never splits at all — it has no chat template - to introspect, so the whole gating mechanism doesn't apply. Leave this - path unsplit and documented as out of scope, or is there demand for - reasoning-splitting on raw completions too? -6. **Promote the local `raw_value` var to a public field?** D1 keeps "raw" - as the existing local variable at `huggingface.py:1839`. Should this - become a real field on `ModelOutputThunk` (a public API addition scoped - to one backend's benefit) so external code — not just HF-internal - consumers — can reach the pre-split text? No consumer outside this - backend needs it today. -7. **Does the intrinsic-input behaviour change (Part II §12, G4) need a - re-baseline?** Adapter functions (`mellea/stdlib/components/intrinsic/`) - read prior assistant response text via `turn.output.value` - (`mellea/stdlib/components/intrinsic/_util.py:104-109,249`). On HF, that - text previously included raw `` blocks and now (post PR #1616) - doesn't. This is a correctness improvement, but it changes the literal - input intrinsic aLoRAs see on HF. Is this a documented bug fix, or does - it warrant re-running intrinsic evals before merge to confirm no - regression in adapter accuracy? -8. **Can PR #1616 merge independently of this doc?** Recommended: yes, once - narrowed to D3/D6 plus the observability and docstring items in Part II - §16 — all uncontroversial, no open design tradeoff. Confirm this framing. -9. **Doc placement and numbering.** This file is placed at - `docs/dev/proposals/1604-hf-output-parsing.md`, numbered under #1604 (the - umbrella issue) rather than #1610 (the narrow bug), because #1604 is what - this design actually resolves. Confirm, or rename/renumber to #1610. -10. **Prose dialect.** Written in UK spelling per the author's standing - convention; the repository states no dialect preference in - `AGENTS.md`/`CLAUDE.md`. Confirm, or switch to US spelling for - consistency with the rest of the docs tree. - -### §6 Impact and blast radius - -**API surface.** No new public fields proposed by the recommended positions -(D1 rejects a new `ModelOutputThunk.raw_value` field pending Q6; D4 uses the -existing `Message.thinking`/wire-dict mechanism other backends already use). -If Q6 resolves toward "yes, promote it," that is the one public API change -this doc could produce. - -**User-archetype impact:** - -| User | Before PR #1616 | After PR #1616 (narrowed, per §16) | After this doc's full recommendations | -|---|---|---|---| -| Non-streaming HF chat user | Raw `` block visible in the answer | Clean split; `mot.thinking` populated | Same, plus correct gating on explicit `THINKING=False` | -| `m serve` (streaming) user | Raw `` block visible | **Unchanged — still raw** (streaming split deferred, §6/G1) | Unchanged until #1604's incremental-splitting work | -| Multi-turn HF chat, tool-call turns | Reasoning silently dropped from history (pre-existing gap, not introduced by #1610/#1616) | Still dropped | Reasoning replays via `reasoning_content` per D4/D5 | -| Intrinsic adapter (RAG/core aLoRA) caller on HF | Adapter input included raw `` text | Adapter input is reasoning-free (Q7) | Same, with the change explicitly documented | -| Anything reading `GenerateLog` for HF | Trace includes full raw text | Trace has post-split text only, reasoning absent (§16) | Reasoning added back to the trace | - -**Code reach:** confined to `mellea/backends/huggingface.py` and -`mellea/backends/utils.py`; touches `mellea/helpers/openai_compatible_helpers.py` -only by reuse (no changes needed there — `should_replay_reasoning()` and -`message_to_openai_message()`'s `reasoning_content` pattern are reused -as-is). No changes to `mellea/core/base.py`'s `GenFields` hook contract — -confirmed there is no shared cross-backend `post_processing()` signature to -respect (Part II §8), so this stays backend-local. - -**Release planning.** Target release: minor version, exact number TBD — -depends on when Q1–Q9 settle. - -**Risk register:** -- Splitting on the *first* `` (current behaviour, Q2 open) risks - truncating an answer if the model ever emits a genuine second use of the - literal text `` inside its answer — low probability, unverified - frequency. -- The `to_chat` regression this doc identifies (D4's motivation): PR #1616 - as merged, *before* D4 ships, changes multi-turn prompt content for HF - Granite conversations (Granite's template re-inlines an empty - `` onto any assistant turn lacking `reasoning_content` — - `chat_template.jinja:89-90`). This is a behaviour change introduced by the - capture fix, not a pre-existing gap, and should be called out in PR - #1616's own description regardless of this doc's timeline. -- Silent no-op risk (Q4) if `reasoning_content` ships before its no-op - behaviour on older Granite templates is decided. - -**Blocking / unblocking.** This doc blocks D4/D5/D2's full resolution (F2, -F4 in Part II §9) from being implemented in PR #1616. It does not block the -narrower fixes in §16, which ship independently per Q8. - ---- - -## Part II — Supporting detail - -### §7 Full glossary - -| Term | Meaning | -|---|---| -| Capture | Reasoning text becoming `mot.thinking`, separate from `mot.value`, for the turn just generated | -| Replay | A previous turn's `Message.thinking` being sent back to the model in the next turn's input | -| Wire message | The `dict` built for a provider's chat API / `apply_chat_template` call — `{"role": ..., "content": ..., ...}` | -| Template-declared thinking variable | A Jinja variable name (`think`/`thinking`/`enable_thinking`) a chat template's source references, per static AST introspection | -| Resolved thinking value | The actual boolean forwarded for that variable on a specific generation call, derived from `ModelOption.THINKING` | -| Response schema | `transformers`' declarative per-tokenizer metadata describing how to parse structured content (e.g. reasoning) out of generated text — see §10 | -| Raw (this doc) | The fully-decoded text for a generation, before `_split_think_tags()` runs | -| Split / answer | The post-`_split_think_tags()` value assigned to `mot.value` | - -### §8 Current-state analysis, per code path - -**Chat path — `post_processing()`** (`huggingface.py:1756-1930`): -1. KV-cache metadata captured, keyed `cache_key = id(mot.value)` (line 1820) - — **before** any split. -2. `raw_value = mot.value` captured locally (line 1839) — this is the "raw" - text D1 recommends canonicalising. -3. Gate check (lines 1841-1850): `thinking_allowlist.intersection(_CHAT_TEMPLATE_THINKING_VARS)` - — declared-name check only, no value check (F3, D3). -4. `_split_think_tags(raw_value)` (line 1851) — first-occurrence string - partition (D2, Q2). -5. Tool-call scan (`to_tool_calls`) runs on the now-split `mot.value` — - correct: reasoning must not be scanned for tool calls, and Granite emits - `` after content, so a false-positive split at worst truncates - an answer prefix, not a tool call (Part II §12, G6 — no fix needed, just - documented as intentional). -6. Stop-string / finish-reason derivation (lines 1908-1924) reads `raw_value` - (the *pre-split* text), not `mot.value` — correct as written, since a - stop string could itself be ``-adjacent; this local var is - effectively already D1's "raw" concept, just not yet named as - canonical or documented as intentional (G5). - -**Raw/batch path — `_generate_from_raw`:** no chat template, no gating -mechanism, no split. Text returned as-is (Q5 — in/out of scope). - -**`to_chat()`** (`mellea/backends/utils.py:74-134`) — *state as of this -doc's first draft, before D4's interim forward shipped:* built each wire -message from `{"role": m.role, "content": formatter.print(m)}` only — -`m.thinking` was never read. A pre-existing comment already named this gap, -referencing #1201 (the capture-gap issue this fix closes), not #1604 as an -earlier draft of this doc mistakenly said. As of D4's interim forward -(§4, §9 F2), `to_chat()` now attaches `reasoning_content` unconditionally; -the comment there now cites this doc and #1604 for the remaining, -still-open gated-replay design (D5/Q3). - -**`Message._parse()`** (`mellea/stdlib/components/chat.py:212-292`): the HF -fallback branch (there is no HF-specific provider branch — HF's raw -response has no role/content structure to parse, so it falls through to a -generic path) *does* correctly carry `thinking` forward: `Message(role="assistant", -content=computed.value, thinking=computed.thinking)` (lines 264, 291). The -drop is specifically and only inside `to_chat()`, not here. - -**Two unrelated caches, worth distinguishing explicitly** (reviewers will -conflate them): the `_cache`/`cache_get`/`cache_put`/`SimpleLRUCache` -mechanism this doc's D6 fixes is *not* the multi-turn KV-cache-continuation -mechanism (`_make_merged_kv_cache`, `self._cached_blocks: dict[str, DynamicCache]`), -which is keyed by literal `CBlock` content strings and is unaffected by -anything in this doc. - -**Streaming (`astream()`):** the gate explicitly excludes streaming -generations (`if not mot.generation.streaming and ...`, line 1848) — the -split never runs during/after a stream. `astream()` itself is a pure -text-length diff (`beginning_length = len(str(mot._underlying_value))`) with -no notion of think tags at all. - -### §9 Finding-by-finding resolution - -**F1 — Cache key computed pre-split** (PR comment, `huggingface.py:1820`). -Confirmed: `cache_key = id(mot.value)` is Python object-identity of the -pre-split string; `_split_think_tags()` later reassigns `mot.value` to a new -string object, changing its `id()`. Confirmed dead path today: `cache_get()` -(`huggingface.py:2247`ish) has zero call sites in `mellea/` or `test/`. -Resolution: D6 — fix now regardless of doc outcome (one-line reorder). - -**F2 — `to_chat` round-trip** (PR comment, `huggingface.py:1853`) — live -bug, bigger than the original comment suggested. Confirmed: `to_chat()` -drops `m.thinking` unconditionally. New finding beyond the PR comment: -Granite's template re-inlines an *empty* `` onto any -assistant turn lacking `reasoning_content` (`chat_template.jinja:89-90`), -so PR #1616's capture fix — by finally putting reasoning into a field -`to_chat()` doesn't forward — silently changes multi-turn prompt content -for HF Granite conversations that previously carried the inline block -straight through. This is a regression the capture fix introduces, not -merely a missing round-trip feature — and because it changes model input -before any replay mitigation ships, it was treated as blocking rather than -deferrable. *Mitigated:* `to_chat()` now attaches `reasoning_content` -unconditionally (D4's interim forward), restoring parity with the pre-fix -behavior. *Still open:* the final, gated design (attach only per -`should_replay_reasoning()`, D5) remains an open question (Q3), including -the plain-turn consequence named there explicitly. - -**F3 — Gating checks declared name, not resolved value** (PR comment, -`huggingface.py:1850`). Confirmed via direct read of the gate logic -(§8 step 3) and the template (`chat_template.jinja:13`, default `True`). -The existing e2e test for "thinking disabled" -(`test/backends/test_huggingface_thinking_e2e.py`, -`test_thinking_disabled_leaves_mot_thinking_none`) passes for an unrelated -reason: Granite genuinely emits no `` when thinking is off, not -because the gate itself checks the resolved value. Resolution: D3. - -**F4 — Token vs. text boundary detection** (PR comment, -`huggingface.py:278`) — confirmed, and the reviewer's own suggested -direction ("look at the tokens") does not resolve it for this model family. -See §10 for the full mechanism trace. Resolution: D2 (prefer -`response_schema`/`parse_response` when available; otherwise the existing -string fallback, explicitly labelled as a fallback, combined with D3's -resolved-value gate to reduce — not eliminate — false-positive risk). - -Beyond the four review comments, this investigation found: - -**G1 — Streaming still shows raw tags.** Already named in the PR's own -comments as deferred to #1604. This doc keeps that deferral (non-goal, §2) -but insists §6's impact table name it explicitly, since it's the gap most -visible to interactive users. - -**G2 — Raw/batch path never splits.** §8, Q5. - -**G3 — `GenerateLog` drops reasoning from the trace.** Other backends' -`GenerateLog` equivalents record the full provider response (which includes -reasoning); HF's records only the post-split `mot.value`. Resolution: -include reasoning in the log or its `extra` field — cheap, no design -tradeoff, part of §16's narrow slice. - -**G4 — Intrinsic adapter functions see different input text than before.** -`mellea/stdlib/components/intrinsic/_util.py:104-109,249` reads -`turn.output.value` to build adapter input. HF and OpenAI are the only two -`AdapterMixin`-capable backends today. Before PR #1616, HF intrinsic input -on a thinking-enabled turn included the raw `` block; after, it -doesn't. No existing test pins either the old or the new behaviour. This is -almost certainly a correctness improvement (adapters shouldn't see -reasoning noise) but is an unannounced behaviour change with real -consequences for anyone running intrinsic evals against HF. See Q7. - -**G5 — Stop-string check correctly reads `raw_value`.** Confirmed correct -as written (§8); flagged here only so a future refactor doesn't -"helpfully" switch it to `mot.value` and break stop-string detection when a -stop string happens to be near a think-tag boundary. Resolution: add a -one-line comment pinning this, part of §16. - -**G6 — Tool-call scan correctly reads post-split `mot.value`.** Confirmed -correct (§8); no fix needed, included here so reviewers don't re-litigate -it as a bug. - -**G7 — Two layered replay policies.** Mellea's `should_replay_reasoning()` -(tool-call turns only) and Granite's own `truncate_history_thinking` -(all turns at/after the last user message) are both real and both would -apply if D4 ships. Resolution: D5. - -**G8 — `Message._parse`'s HF fallback already carries `thinking` correctly.** -Confirmed (§8); included so no one "fixes" `_parse()` — the bug is -downstream, in `to_chat()`, only. - -**G9 — No shared `post_processing()` contract across backends.** Confirmed: -every backend defines its own signature; the only shared contract is -`GenFields.process`/`post_process` coroutine hook slots in -`mellea/core/base.py`, invoked generically by `ModelOutputThunk` without -knowledge of each backend's internals. Included as a scope-limiter: nothing -in D1–D6 requires a cross-backend architecture change — it's all local to -`huggingface.py`/`utils.py`. - -### §10 The upstream mechanism (`transformers.parse_response`) - -Per this skill's "verify upstream before inventing a parallel concept" -rule: `transformers` (as vendored in this environment) already ships -`PreTrainedTokenizerBase.parse_response(response, schema=None)`, which -reads `tokenizer.response_schema` and, when present, runs a -`recursive_parse` over the *decoded text* to split out structured content -(including reasoning) per the schema. When no schema is set, it raises -`AttributeError`. - -Two things follow directly from reading this mechanism, both load-bearing -for D2 and F4: - -1. **No tokenizer available to Mellea today — including every Granite - variant checked — declares a `response_schema`.** So `parse_response()` - is not usable today; `_split_think_tags()`'s string fallback is the only - working path, exactly as #1604 anticipated when it asked for "better - output parsing." -2. **Even `parse_response()` operates on decoded text, not raw token - IDs.** It calls `self.decode()` internally before parsing. This means - the reviewer's "look at the tokens" suggestion (PR comment, - `huggingface.py:278`) is not how upstream itself solves this class of - problem either — upstream's answer to "don't confuse literal text with - a control delimiter" is a *declared schema*, not token inspection. - Verified separately (§8, §10): Granite's actual `` token is - itself registered `special: false` in its tokenizer, so even a - token-identity check (had `_split_think_tags` been given token access) - would not distinguish "the model emitted the genuine end-of-reasoning - token" from "the model wrote the four characters `` as prose" — - both decode to the identical string, and the real token carries no - distinguishing flag either way. - -Conclusion: `_split_think_tags()` should be documented explicitly as a -fallback for `parse_response()`/`response_schema`, not as the primary -mechanism it currently reads as. If/when Granite tokenizers gain a -`response_schema` (Q1, a cross-team ask), Mellea should prefer that path. - -### §11 The model's own template as source of truth - -Verified directly against the on-disk artefact (not inferred): -`~/.cache/huggingface/hub/models--ibm-granite--granite-4.2-3b/snapshots/.../chat_template.jinja`. - -- Line 13: `{%- set enable_thinking = enable_thinking if enable_thinking is defined else True %}` - — thinking defaults **on**. -- Line 18: `{%- set truncate_history_thinking = truncate_history_thinking if truncate_history_thinking is defined else True %}` - — history truncation of reasoning also defaults **on**. -- Lines 83-84: `{%- if message.reasoning_content is defined and message.reasoning_content is string and ... %}` - → `{%- set content = "\n" ~ message.reasoning_content ~ "\n\n" ~ (message.content | default('', true)) %}` - — **the template already has a working mechanism for accepting prior - reasoning via a `reasoning_content` key and re-inlining it.** This is the - mechanism D4 recommends using. -- Lines 89-90: `{%- if '' not in content and '' not in content -%}{%- set content = "" ~ content -%}` — - the source of F2's newly-found regression: any assistant turn arriving - *without* think tags (i.e., without `reasoning_content` set) gets an empty - `` pair silently prepended. -- Lines 99, 105-112: history-truncation logic — turns before the last user - message get reasoning dropped (if `truncate_history_thinking`), and - turns that keep reasoning split on the **last** `` occurrence - (`c.split('')[-1]`) — the opposite occurrence rule from - `_split_think_tags()`'s first-occurrence partition (Q2). -- Lines 137-145: a second truncation branch with the same last-occurrence - rule (`c.split('')[-1]`), confirming this is the template's - consistent convention, not a one-off. -- Lines 179-182: generation-prompt construction opens `\n` (or the - already-closed `` when `enable_thinking` is false) — this - confirms the opening tag is baked into the *prompt*, matching - `_split_think_tags()`'s docstring rationale for splitting on `` - alone. - -Qwen3's chat template (checked for generality, not reproduced verbatim -here) uses the same `reasoning_content` key and a `preserve_thinking`-style -gate with equivalent semantics — this is a cross-model Jinja convention -among reasoning models with think-tag delimiters, not a Granite-only -mechanism. Named as such in §13. - -Round-trip sequence (proposed, D4/D5): - -```mermaid -sequenceDiagram - participant U as User turn N - participant M as Model (assistant turn N) - participant H as post_processing() - participant C as to_chat() (turn N replay, building turn N+1 request) - U->>M: prompt (template opens \n) - M->>H: raw decoded text, "...reasoning...answer" - H->>H: split_think_tags -> mot.thinking, mot.value - Note over H: D3: only if resolved THINKING != False - C->>C: should_replay_reasoning(turn N) ? - alt turn N issued a tool call - C->>M: wire dict: {role: assistant, content: answer, reasoning_content: thinking} - M->>M: template re-inlines ...reasoning...answer - else no tool call - C->>M: wire dict: {role: assistant, content: answer} - M->>M: template prepends empty (F2) - end -``` - -### §12 Consumer matrix - -| Consumer | Reads today | Should read (proposed) | Notes | -|---|---|---|---| -| End user (`mot.value`) | Split answer (post PR #1616) | Same | Correct already | -| Tool-call scan (`to_tool_calls`) | Split `mot.value` | Same | G6 — correct, document as intentional | -| Stop-string / finish-reason | Pre-split `raw_value` | Same | G5 — correct, document as intentional | -| LRU cache key (`cache_key = id(mot.value)`) | Pre-split object identity | Post-split, or a stable key | F1/D6 | -| `to_chat()` wire message | `mot.value` only | `mot.value` + `reasoning_content` (gated) | F2/D4/D5 | -| `GenerateLog` | Post-split `mot.value` only | Include reasoning | G3 | -| Intrinsic adapter input (`_extract_last_response`) | Split, reasoning-free text (post PR #1616) | Same (this is the improvement) | G4 — behaviour change, needs Q7 | -| `astream()` deltas | Raw, unsplit, growing string | Unchanged (non-goal, §2) | G1 | -| `_generate_from_raw` completions | Raw, unsplit | Unchanged unless Q5 says otherwise | G2 | - -### §13 Generality - -Is this design Granite-specific? Partially, and this doc should say so -plainly rather than imply universality: - -- **Generalises:** the declared-thinking-variable gate (D3), the - `reasoning_content` replay key (D4), and the ``/`` literal - boundary all generalise cleanly to Qwen3 (confirmed, §11) and to any - future Granite-family model following the same template convention. -- **Does not generalise:** models using channel-based reasoning conventions - (e.g. gpt-oss's channel markers) or bracket conventions (`[THINK]...[/THINK]`) - would need a different boundary detector entirely — `_split_think_tags()` - is scoped to the ``/`` textual convention by design - (its own docstring already says this), and this doc doesn't propose - changing that scope. -- If a second convention arrives, the natural extension point is a small - per-model-family convention table keyed by the same - `_chat_template_allowlist` introspection this doc already relies on — not - designed here, since no second model needing it exists yet. - -### §14 Observability - -- `GenerateLog` should carry reasoning (G3) so debug/eval traces aren't - missing it — matches what OpenAI/LiteLLM/WatsonX backends already log. -- A debug log line already exists on successful split - (`huggingface.py`, `MelleaLogger.get_logger().debug(...)` after the split) - — recommend a matching debug line when the gate *would* have split but - the resolved-value check (D3) suppressed it, to make Q4's silent-no-op - risk debuggable. - -### §15 Docs and tests - -Items 1, 2, 3, 9, and 11 are shipped alongside the code fixes in §16; the -rest wait on this doc's remaining open questions (D4's final gated form, -D2, Q2, Q4, Q5, Q7): - -1. **Shipped.** Gate honours explicit `ModelOption.THINKING=False` with - synthetic text containing a literal `` — asserts no split (F3/D3). - `test_post_processing_does_not_split_when_thinking_explicitly_false`. -2. **Shipped.** Gate still splits when `THINKING` is unset (`None`) on a - template that thinks by default — guards against over-correcting D3. - `test_post_processing_splits_when_thinking_unset`. -3. **Shipped, for the interim unconditional forward only.** `to_chat()` - with a non-empty `Message.thinking`: asserts `reasoning_content` present - on the assistant wire dict (`test_to_chat_attaches_reasoning_content_for_assistant_thinking`) - and absent when there is no captured reasoning - (`test_to_chat_omits_reasoning_content_when_no_thinking`). **Still - needed once D5 ships:** a version of this test asserting presence only - on a tool-call turn and absence on a plain turn, replacing the - unconditional assertion above. -4. HF rows added to `test/backends/test_reasoning_replay.py` (currently - covers OpenAI/Ollama/WatsonX only) — waits on D5. -5. A rendered-prompt assertion: run `apply_chat_template` over a two-turn - conversation and assert reasoning appears in the rendered string — the - only test that catches Q4's silent-key-drop failure mode. -6. Truncated-reasoning case: thinking on, generation hits `max_new_tokens` - before any `` appears — assert whatever behaviour this doc's - resolution of D2/D3 settles on (today: `_split_think_tags` returns - `(None, text)`, silently surfacing the entire reasoning block as the - answer). -7. Occurrence-rule test updated or explicitly justified if Q2 resolves - toward last-occurrence (currently pinned first-occurrence at - `test/backends/test_huggingface_thinking.py`, unchanged by this round). -8. An intrinsic-level test pinning that adapter functions receive - reasoning-free response text on HF (G4/Q7). -9. **Shipped.** A cache test: the key computed in `post_processing()` is - retrievable via `cache_get()` after the split (F1/D6) — the first - in-tree caller of `cache_get()`. - `test_post_processing_cache_key_findable_after_split`. -10. A guard test confirming a model whose template declares no thinking - variable never attempts a split — pre-existing, unchanged by this round - (`test_post_processing_does_not_split_without_thinking_template_var`). -11. **Shipped.** Collision test, per D4's conflict policy: a - `Message.provider_fields` entry author-declaring `reasoning_content` - for `"huggingface"` is silently overridden by Mellea's own value, - confirming `to_chat()` sets `reasoning_content` before - `merge_provider_fields` runs, the same known-fields-first ordering - `tool_calls`/`tool_call_id` already use. - `test_to_chat_known_reasoning_content_wins_over_provider_fields`. - -### §16 Migration / sequencing - -**Shipped in PR #1616, independent of this doc's remaining open questions -(Q8):** D6 (cache-key reorder, with the test at §15 item 9 — the first -in-tree `cache_get()` caller), D3 (resolved-value gate, with the tests at -§15 items 1-2), D4's conflict policy against `merge_provider_fields` -(§15 item 11), G3 (`GenerateLog` reasoning), G5 (pin the `raw_value` -intent with a comment), relabelling `_split_think_tags()`'s docstring as a -`response_schema` fallback (§10), and — moved up from "waits for D5" -because leaving it unaddressed would have shipped a silent multi-turn -prompt regression (F2) — D4's **interim, unconditional** `reasoning_content` -forward in `to_chat()`, with the round-trip tests at §15 item 3. PR #1616's -description flags G4 (Q7) so reviewers running intrinsic evals know input -text changed, and discloses the F2 regression this interim forward -mitigates. - -**Waits for this doc's decisions (D4's final gated form, D5, D2/Q2, Q4, Q5, Q6):** -everything else. Sequencing once agreed: D4's move from unconditional to -`should_replay_reasoning()`-gated and D5 (replay policy layering) ship -together as one PR (they're the same code path, and D5's plain-turn -consequence per §5 Q3 must be decided first); D2's -`response_schema` preference ships whenever Q1 resolves (likely later, -gated on an external team); Q5 (raw path) and Q6 (public field promotion) -are independent follow-ups if their answers are "yes." - -### §17 Open questions (full list) - -Back-reference only — each item below is the same decision as its Part I -§5 counterpart, not a restated version. See §5 for the question text. - -1. Q1 → Part I §5.1 (`response_schema` availability; cross-team, Granite tokenizer team). -2. Q2 → Part I §5.2 (first- vs. last-`` occurrence; see §11 for the template evidence). -3. Q3 → Part I §5.3 (replay-policy layering, D5). -4. Q4 → Part I §5.4 (silent `reasoning_content` key drop on older templates). -5. Q5 → Part I §5.5 (raw/batch path in scope?). -6. Q6 → Part I §5.6 (promote `raw_value` to a public field?). -7. Q7 → Part I §5.7 (intrinsic-input behaviour change; re-baseline needed?). -8. Q8 → Part I §5.8 (can PR #1616 merge independently of this doc?). -9. Q9 → Part I §5.9 (doc placement and numbering). -10. Q10 → Part I §5.10 (prose dialect). - ---- - -## Appendix - -### Tracking items - -| Ref | Relation to this doc | -|---|---| -| [#1604](https://github.com/generative-computing/mellea/issues/1604) | Umbrella issue this doc resolves; this doc's numbering | -| [#1610](https://github.com/generative-computing/mellea/issues/1610) | The narrow bug PR #1616 fixes; this doc's proximate trigger | -| [#1201](https://github.com/generative-computing/mellea/issues/1201) (referenced, not re-opened) | Prior cross-backend consensus on `should_replay_reasoning`; this doc asks whether to apply it to HF, not to change it | -| PR [#1616](https://github.com/generative-computing/mellea/pull/1616) | Draft implementation under review; stays open per this doc's Part I §5 Q8 | - -### History and rework evidence - -- PR #1616, single commit `bf0f45e6`. Four inline review comments from - `jakelorocco` at `huggingface.py:278`, `:1820`, `:1850`, `:1853` - (verbatim quotes reproduced in Part I §1 and Part II §9). -- Top-level review comment from `jakelorocco`: *"I think I did not realize - how many aspects of the hf backend this would impact when I created the - issue. I think there's actually a fair bit of design work that might be - required to address these concerns."* -- Pre-existing acknowledgement of the `to_chat` gap already in the codebase - before this PR, at `mellea/backends/utils.py:100-104`, referencing **#1201** - (corrected from an earlier draft of this doc, which incorrectly cited #1604) — - evidence the gap was known before #1610 was filed. - -### Related in-flight work - -- Streaming-safe incremental splitting (deferred non-goal, §2/G1) — tracked - under #1604, not designed in this doc. - -### Verification trail - -- Cache-key, `to_chat` drop, gating logic, and token-boundary claims: - traced against `mellea/backends/huggingface.py`, `mellea/backends/utils.py`, - `mellea/helpers/openai_compatible_helpers.py`, and - `mellea/stdlib/components/chat.py` at the commit checked out in this - worktree (`bf0f45e6` head of `issue-1610`); line numbers re-verified - directly (not solely from prior research notes) immediately before this - doc was written. -- Granite 4.2 template mechanism (`reasoning_content`, last-occurrence - split, default-thinking-`True`): verified directly against - `~/.cache/huggingface/hub/models--ibm-granite--granite-4.2-3b/snapshots/b7e947307dd2efb3ad3b853b0e8a7e75f8ad4ac2/chat_template.jinja` - lines 13, 18, 83-118, 137-145, 179-182 on disk. -- `transformers.parse_response`/`response_schema` mechanism (§10): verified - against the vendored `transformers` source in this environment; confirmed - no Granite tokenizer checked declares a `response_schema`. -- Intrinsic adapter consumption (G4): verified against - `mellea/stdlib/components/intrinsic/_util.py:104-109,249`. diff --git a/mellea/backends/huggingface.py b/mellea/backends/huggingface.py index b383474191..5a87e01277 100644 --- a/mellea/backends/huggingface.py +++ b/mellea/backends/huggingface.py @@ -283,10 +283,10 @@ def _split_think_tags(text: str) -> tuple[str | None, str]: A string-level fallback for `transformers.PreTrainedTokenizerBase.parse_response()` (schema-driven, token-decode-then-parse), used because no tokenizer available to - this backend today declares a `response_schema` — see the design proposal at - docs/dev/proposals/1604-hf-output-parsing.md §10 for why even that upstream - mechanism is text-level, not token-level, and can't disambiguate a genuine - end-of-reasoning token from literal `` text for Granite either. + this backend today declares a `response_schema`. Even that upstream mechanism is + text-level, not token-level, and can't disambiguate a genuine end-of-reasoning + token from literal `` text for Granite either — its own `` token + is registered non-special. Splits on alone (not a ... pair) because some chat templates (e.g. granite-4.2 with enable_thinking) bake the opening tag into @@ -295,10 +295,13 @@ def _split_think_tags(text: str) -> tuple[str | None, str]: delimiters pass through unchanged. See #1604 for generalizing this. Deliberately strips leading/trailing whitespace from both `thinking` and - `answer`: the whitespace immediately around the tags is delimiter framing - (Granite's template emits `\n...\n\n`), not meaningful - content, so the user-visible completion loses that framing whitespace as - part of this split, not incidentally. + `answer`: the whitespace immediately around the tags is delimiter framing, + not meaningful content, so the user-visible completion loses that framing + whitespace as part of this split, not incidentally. This framing comes + from the *generation-prompt* path (chat_template.jinja:178-182, e.g. + `<|im_start|>assistant\n\n`), not the replay-reconstruction path + (chat_template.jinja:83-84) — this function only ever sees freshly + generated text, never a replayed history turn. """ if _THINK_CLOSE_TAG not in text: return None, text diff --git a/mellea/backends/utils.py b/mellea/backends/utils.py index c288bd1f7f..7a39f0c39b 100644 --- a/mellea/backends/utils.py +++ b/mellea/backends/utils.py @@ -101,15 +101,24 @@ def to_chat( # to print `Message`s to correctly serialize any documents with the message. Do the printing here. # NOTE: reasoning replay here is an interim, unconditional forward — every non-empty # `Message.thinking` is attached as `reasoning_content` regardless of `should_replay_reasoning` - # (unlike OpenAI/LiteLLM/Watsonx/Ollama, which gate replay on that policy). This exists to - # restore the parity the HF think-tag capture fix (#1610) removed: before that fix, raw - # `...` text sat inline in `content` on every turn, so Granite's chat template - # (chat_template.jinja:83-90) always saw prior reasoning; after the fix, `content` is tag-free - # and the template silently prepends an empty `` instead, dropping reasoning the - # model previously saw. Attaching `reasoning_content` unconditionally restores that prior - # behavior without deciding the real design question (whether HF should follow the - # tool-call-only consensus rule from #1201 on plain turns too) — see the design proposal at - # docs/dev/proposals/1604-hf-output-parsing.md, D5/Q3, for the full replay-policy decision. + # (unlike OpenAI/LiteLLM/Watsonx/Ollama, which gate replay on that policy). This exists because + # the HF think-tag capture fix (#1610) alone removed reasoning that Granite's chat template + # previously saw on every turn: before that fix, raw `...` text sat inline in + # `content`, and after it, `content` is tag-free and the template prepends an empty + # `` instead (chat_template.jinja:83-90). + # + # Restores parity ONLY for tool-call turns (a turn after the last user message, or one the + # template's `truncate_history_thinking` gate otherwise doesn't touch) — those match the + # #1201 cross-backend consensus (replay on tool-call turns only) and the reconstructed + # `...` survives unmodified. Does NOT restore parity for plain multi-turn + # (an assistant turn with no tool call, before the last user message): the template's + # truncation branch strips reasoning whenever content carries BOTH tags + # (chat_template.jinja:99,137-145), and the reconstructed form always does — whereas the + # pre-#1610 inline form only ever carried the closing tag (the opening tag is prompt-baked, + # never present in `mot.value`), so it was invisible to that same gate and passed through by + # accident. This is a real, measured functional gap: whether HF should replay reasoning on + # plain turns too (diverging from the #1201 consensus) is open — see PR #1616 and the two + # rendered-prompt tests in test/backends/test_huggingface_thinking.py that pin both shapes. ctx_as_conversation: list = [] for m in ctx_as_message_list: msg_dict: dict = {"role": m.role, "content": formatter.print(m)} @@ -120,7 +129,7 @@ def to_chat( msg_dict["tool_calls"] = m.tool_calls if m.tool_call_id: msg_dict["tool_call_id"] = m.tool_call_id - if m.thinking: + if m.role == "assistant" and m.thinking: msg_dict["reasoning_content"] = m.thinking # Merge any author-declared provider fields (Mellea's known fields win; # a mismatched target raises). Must run after the known fields are set. diff --git a/test/backends/test_huggingface_thinking.py b/test/backends/test_huggingface_thinking.py index 0fec2795d5..f78fd1c786 100644 --- a/test/backends/test_huggingface_thinking.py +++ b/test/backends/test_huggingface_thinking.py @@ -17,6 +17,10 @@ from mellea.backends import ModelOption from mellea.backends.huggingface import LocalHFBackend, _split_think_tags from mellea.core.base import CBlock, ModelOutputThunk +from test.backends.test_huggingface_filter_options import ( + _GRANITE_THINKING_MODEL_ID, + _try_load_granite_tokenizer, +) def test_split_think_tags_with_both_tags() -> None: @@ -75,8 +79,7 @@ def test_split_think_tags_multiple_close_tags_uses_first() -> None: Pins current behavior, not a settled design choice: Granite's own chat template splits replayed history on the *last* occurrence (chat_template.jinja, e.g. `c.split('')[-1]`), the opposite rule. - Whether to match the model's own convention is an open question — see - docs/dev/proposals/1604-hf-output-parsing.md, Q2. + Whether to match the model's own convention is an open question — see PR #1616. """ thinking, answer = _split_think_tags("abc") assert thinking == "a" @@ -298,3 +301,130 @@ async def test_post_processing_cache_key_findable_after_split() -> None: assert mot.value == "the answer" cached = backend.cache_get(id(mot.value)) assert cached is not None + + +def _render_history(messages: list) -> str: + """Render a message list through the real granite-4.2-3b chat template. + + Loads the template only (no GPU, no model weights) via + `_try_load_granite_tokenizer`; skips if not locally cached. + """ + tok = _try_load_granite_tokenizer(_GRANITE_THINKING_MODEL_ID) + if tok is None: + pytest.skip( + f"{_GRANITE_THINKING_MODEL_ID} not in local HF cache — " + "run qualitative tests first" + ) + return tok.apply_chat_template( + messages, tokenize=False, add_generation_prompt=False + ) + + +@pytest.mark.integration +@pytest.mark.huggingface +def test_rendered_prompt_preserves_reasoning_on_tool_call_turn() -> None: + """Regression/contract test against the real template (not a synthetic one): + on a tool-call turn, the unconditional `reasoning_content` forward in + `to_chat()` restores reasoning to the rendered prompt, matching pre-#1610 + behavior for this shape. The other shape (plain multi-turn, next test) is + NOT restored by the same fix — see PR #1616. + """ + messages = [ + {"role": "user", "content": "What's the weather in Boston?"}, + { + "role": "assistant", + "content": "", + "reasoning_content": "I should call the weather tool.", + "tool_calls": [ + { + "id": "1", + "type": "function", + "function": { + "name": "get_weather", + "arguments": {"city": "Boston"}, + }, + } + ], + }, + {"role": "tool", "content": "72F, sunny", "tool_call_id": "1"}, + ] + rendered = _render_history(messages) + assert "I should call the weather tool." in rendered + # The empty pair is what a dropped/truncated reasoning_content + # would leave behind; its absence here is the direct evidence reasoning survived. + assert "" not in rendered + + +@pytest.mark.integration +@pytest.mark.huggingface +def test_rendered_prompt_drops_reasoning_on_plain_multi_turn() -> None: + """Documents a known, currently-accepted gap: on a plain multi-turn shape + (assistant turn with no tool call, followed by another user turn), the + template's own `truncate_history_thinking` gate strips reasoning even + though D4's forward attaches `reasoning_content` — because the + reconstructed content now carries both and , which is + exactly what that gate matches on. Before #1610, the raw inline form + (opening tag prompt-baked, never present in mot.value) only ever carried + the closing tag, so it was invisible to this same gate and reasoning + passed through by accident. + + This is a known limitation, not an oversight: it is the direct effect of + aligning HF's replay with the #1201 cross-backend consensus (replay only + on tool-call turns), left open for maintainer decision in PR #1616 rather + than resolved silently. If this test starts failing (reasoning present), + a reviewer changed that policy — update this test deliberately, don't + just delete the assertion. + """ + messages = [ + {"role": "user", "content": "What is 2 + 2?"}, + { + "role": "assistant", + "content": "4", + "reasoning_content": "Two plus two equals four.", + }, + {"role": "user", "content": "And 3 + 3?"}, + ] + rendered = _render_history(messages) + assert "Two plus two equals four." not in rendered + + +@pytest.mark.parametrize("with_tool_call", [True, False]) +def test_parse_then_to_chat_round_trips_thinking_for_hf(with_tool_call: bool) -> None: + """Seam test: HF's `Message._parse()` (chat.py) carries `.thinking` onto the + parsed assistant Message in both branches (tool-call and plain), and that + Message then round-trips through `to_chat()` (utils.py) as `reasoning_content`. + + Closes the gap between the e2e tests (stop at `output.thinking`) and the + `to_chat` unit tests (hand-build `Message(..., thinking=...)` directly, + never exercising `_parse`) — this is the only test proving the link between + them for HF specifically. + """ + from typing import cast + + from mellea.backends.utils import to_chat + from mellea.core import ModelToolCall + from mellea.formatters.template_formatter import TemplateFormatter as ChatFormatter + from mellea.stdlib.components import Message + from mellea.stdlib.context import ChatContext + + mot = ModelOutputThunk(value="the answer") + mot.thinking = "reasoning trace" + mot.raw.provider = "huggingface" + mot.raw.response = None + if with_tool_call: + # Only non-None matters for _parse's branch selection; the placeholder is + # never read as a real ModelToolCall. + mot.tool_calls = [cast(ModelToolCall, None)] + + parsed = Message(role="assistant", content="placeholder")._parse(mot) + assert parsed.thinking == "reasoning trace" + + ctx = ChatContext() + ctx = ctx.add(Message("user", "hello")) + ctx = ctx.add(parsed) + action = Message("user", "next question") + formatter = ChatFormatter(model_id="test") + + result = to_chat(action, ctx, formatter, system_prompt=None) + assistant_msg = next(m for m in result if m["role"] == "assistant") + assert assistant_msg["reasoning_content"] == "reasoning trace" diff --git a/test/backends/test_huggingface_thinking_e2e.py b/test/backends/test_huggingface_thinking_e2e.py index 7cd8fe63ed..78ed2cb2fc 100644 --- a/test/backends/test_huggingface_thinking_e2e.py +++ b/test/backends/test_huggingface_thinking_e2e.py @@ -14,6 +14,7 @@ """ import os +import re import pytest @@ -80,7 +81,9 @@ def test_thinking_enabled_populates_mot_thinking(session): # output never contains it — checking for its absence here would be vacuous. # is the tag that actually appears in raw output and must be split out. assert "" not in output.value - assert "4" in output.value + assert re.search(r"\b4\b", output.value), ( + f"Expected the digit 4 as its own token, got: {output.value!r}" + ) @pytest.mark.qualitative @@ -93,4 +96,6 @@ def test_thinking_disabled_leaves_mot_thinking_falsy(session): ) assert not output.thinking, f"Expected no reasoning trace, got: {output.thinking!r}" assert "" not in output.value - assert "4" in output.value + assert re.search(r"\b4\b", output.value), ( + f"Expected the digit 4 as its own token, got: {output.value!r}" + ) diff --git a/test/backends/test_utils.py b/test/backends/test_utils.py index 1fd27b2f4a..fde3def9f5 100644 --- a/test/backends/test_utils.py +++ b/test/backends/test_utils.py @@ -155,7 +155,7 @@ def test_to_chat_attaches_reasoning_content_for_assistant_thinking(): """Regression test: an assistant Message carrying `.thinking` must have it forwarded as `reasoning_content` on the wire dict. Before this fix, `to_chat` read only `m.role`/`m.content` and silently dropped `.thinking` on every - HF replay — see docs/dev/proposals/1604-hf-output-parsing.md, D4. + HF replay — see PR #1616. """ from mellea.backends.utils import to_chat from mellea.formatters.template_formatter import TemplateFormatter as ChatFormatter @@ -178,7 +178,7 @@ def test_to_chat_known_reasoning_content_wins_over_provider_fields(): before `merge_provider_fields` runs (same known-fields-first pattern as `tool_calls`/`tool_call_id`), so an author-declared `provider_fields` collision on the same key is silently dropped (debug-logged, not raised) — - Mellea's own value always wins. See docs/dev/proposals/1604-hf-output-parsing.md, D4. + Mellea's own value always wins. """ from mellea.backends.utils import to_chat from mellea.formatters.template_formatter import TemplateFormatter as ChatFormatter @@ -222,6 +222,26 @@ def test_to_chat_omits_reasoning_content_when_no_thinking(): assert "reasoning_content" not in assistant_msg +def test_to_chat_ignores_thinking_on_non_assistant_message(): + """`.thinking` on a non-assistant Message (never set by real code paths, but + not type-guarded against) must not be forwarded as `reasoning_content` — the + Granite template only consumes that key on the assistant branch, so this is + otherwise inert, but the wire dict should not carry stray, unconsumed keys.""" + from mellea.backends.utils import to_chat + from mellea.formatters.template_formatter import TemplateFormatter as ChatFormatter + from mellea.stdlib.components import Message + from mellea.stdlib.context import ChatContext + + ctx = ChatContext() + ctx = ctx.add(Message("user", "hello", thinking="stray thinking")) + action = Message("user", "next question") + formatter = ChatFormatter(model_id="test") + + result = to_chat(action, ctx, formatter, system_prompt=None) + user_msg = next(m for m in result if m["content"] == "hello") + assert "reasoning_content" not in user_msg + + def test_to_chat_with_system_prompt(): from mellea.backends.utils import to_chat from mellea.formatters.template_formatter import TemplateFormatter as ChatFormatter From fbce642833f156fb449653d14bb16eaab54c45ea Mon Sep 17 00:00:00 2001 From: Nigel Jones Date: Fri, 4 Sep 2026 13:18:13 +0100 Subject: [PATCH 06/10] test(hf): fix marker convention violations from bob review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per test/README.md, backend markers (huggingface, ollama, etc.) apply only to e2e/qualitative tests, not integration. The two rendered-prompt tests added in the previous commit are integration-tier (tokenizer-only, no GPU) and incorrectly carried @pytest.mark.huggingface alongside @pytest.mark.integration — drop the backend marker. Also removes a redundant pytest.mark.skipif(CICD==1) from the e2e file's module-level pytestmark: both tests already carry @pytest.mark.qualitative individually, which conftest's central CICD-skip logic already covers. The skipif duplicated that policy and its only purpose was to justify the now-removed `import os`. A third finding (empty-string mot.thinking forwarded downstream) was checked and refuted: changing the post_processing() guard to a truthy check would break splitting for Granite's thinking-disabled shape, leaving the empty tag pair in the visible answer. Confirmed empirically; not applied. Assisted-by: Claude Code Signed-off-by: Nigel Jones --- test/backends/test_huggingface_thinking.py | 2 -- test/backends/test_huggingface_thinking_e2e.py | 11 +---------- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/test/backends/test_huggingface_thinking.py b/test/backends/test_huggingface_thinking.py index f78fd1c786..38bc4949a9 100644 --- a/test/backends/test_huggingface_thinking.py +++ b/test/backends/test_huggingface_thinking.py @@ -321,7 +321,6 @@ def _render_history(messages: list) -> str: @pytest.mark.integration -@pytest.mark.huggingface def test_rendered_prompt_preserves_reasoning_on_tool_call_turn() -> None: """Regression/contract test against the real template (not a synthetic one): on a tool-call turn, the unconditional `reasoning_content` forward in @@ -356,7 +355,6 @@ def test_rendered_prompt_preserves_reasoning_on_tool_call_turn() -> None: @pytest.mark.integration -@pytest.mark.huggingface def test_rendered_prompt_drops_reasoning_on_plain_multi_turn() -> None: """Documents a known, currently-accepted gap: on a plain multi-turn shape (assistant turn with no tool call, followed by another user turn), the diff --git a/test/backends/test_huggingface_thinking_e2e.py b/test/backends/test_huggingface_thinking_e2e.py index 78ed2cb2fc..ccd70da01a 100644 --- a/test/backends/test_huggingface_thinking_e2e.py +++ b/test/backends/test_huggingface_thinking_e2e.py @@ -13,7 +13,6 @@ synthetic-string test can verify. """ -import os import re import pytest @@ -22,15 +21,7 @@ torch = pytest.importorskip("torch", reason="torch not installed — install mellea[hf]") -pytestmark = [ - pytest.mark.huggingface, - pytest.mark.e2e, - require_gpu(min_vram_gb=8), - pytest.mark.skipif( - int(os.environ.get("CICD", 0)) == 1, - reason="Skipping HuggingFace thinking e2e tests in CI - qualitative test", - ), -] +pytestmark = [pytest.mark.huggingface, pytest.mark.e2e, require_gpu(min_vram_gb=8)] import mellea.backends.model_ids as model_ids from mellea import MelleaSession From 31354cf1519b0bf678055b9516d66c11a46a9af1 Mon Sep 17 00:00:00 2001 From: Nigel Jones Date: Fri, 4 Sep 2026 14:35:52 +0100 Subject: [PATCH 07/10] test(hf): add false-positive test for mentioned in a real answer Covers the exact risk jakelorocco raised on :278: thinking genuinely on, gate correctly fires, and the model's answer itself mentions the literal text "" (e.g. explaining what the tag does). First-occurrence splitting keeps the full answer intact instead of truncating it at the second, unrelated occurrence. Assisted-by: Claude Code Signed-off-by: Nigel Jones --- test/backends/test_huggingface_thinking.py | 73 ++++++++++++++-------- 1 file changed, 48 insertions(+), 25 deletions(-) diff --git a/test/backends/test_huggingface_thinking.py b/test/backends/test_huggingface_thinking.py index 38bc4949a9..d602bc2b5d 100644 --- a/test/backends/test_huggingface_thinking.py +++ b/test/backends/test_huggingface_thinking.py @@ -76,10 +76,12 @@ def test_split_think_tags_leading_whitespace_before_open_tag() -> None: def test_split_think_tags_multiple_close_tags_uses_first() -> None: """Multiple occurrences: only the first is treated as the boundary. - Pins current behavior, not a settled design choice: Granite's own chat - template splits replayed history on the *last* occurrence - (chat_template.jinja, e.g. `c.split('')[-1]`), the opposite rule. - Whether to match the model's own convention is an open question — see PR #1616. + Deliberate: Granite's own template uses the *last* occurrence when + truncating old reasoning out of replayed history, where dropping too much + is the safe direction. Here we're extracting a clean answer from a fresh + completion, where the safe direction is the opposite: if the model's + answer itself mentions the literal text "", splitting on the + first occurrence keeps the answer intact instead of corrupting it. """ thinking, answer = _split_think_tags("abc") assert thinking == "a" @@ -267,6 +269,37 @@ async def test_post_processing_splits_when_thinking_unset() -> None: assert mot.value == "the answer" +async def test_post_processing_preserves_answer_mentioning_think_tag() -> None: + """False-positive risk test: thinking genuinely on, and the model's answer + itself explains what the tag does. The gate correctly fires (a + real reasoning block exists), and first-occurrence splitting keeps the + full answer intact rather than truncating it at the second, unrelated + occurrence — the risk jakelorocco raised in the original PR review. + """ + backend = _make_backend(thinking_template_var="think") + mot = ModelOutputThunk( + value=( + "the user wants an explanation" + "The tag marks the end of a reasoning block." + ) + ) + mot._call.action = CBlock("What does the tag do?") + mot._call.model_options = {} + + await backend.post_processing( + mot, + conversation=[], + _format=None, + tool_calls=False, + tools={}, + seed=None, + input_ids=None, + ) + + assert mot.thinking == "the user wants an explanation" + assert mot.value == "The tag marks the end of a reasoning block." + + async def test_post_processing_cache_key_findable_after_split() -> None: """Regression test: the LRU cache key computed in post_processing must be derived from mot.value *after* the split has run, so a later cache_get() call @@ -322,11 +355,10 @@ def _render_history(messages: list) -> str: @pytest.mark.integration def test_rendered_prompt_preserves_reasoning_on_tool_call_turn() -> None: - """Regression/contract test against the real template (not a synthetic one): - on a tool-call turn, the unconditional `reasoning_content` forward in - `to_chat()` restores reasoning to the rendered prompt, matching pre-#1610 - behavior for this shape. The other shape (plain multi-turn, next test) is - NOT restored by the same fix — see PR #1616. + """On a tool-call turn, the `reasoning_content` forward in `to_chat()` + restores reasoning to the rendered prompt against the real template + (not a synthetic one). The plain multi-turn shape (next test) isn't + restored the same way. """ messages = [ {"role": "user", "content": "What's the weather in Boston?"}, @@ -356,22 +388,13 @@ def test_rendered_prompt_preserves_reasoning_on_tool_call_turn() -> None: @pytest.mark.integration def test_rendered_prompt_drops_reasoning_on_plain_multi_turn() -> None: - """Documents a known, currently-accepted gap: on a plain multi-turn shape - (assistant turn with no tool call, followed by another user turn), the - template's own `truncate_history_thinking` gate strips reasoning even - though D4's forward attaches `reasoning_content` — because the - reconstructed content now carries both and , which is - exactly what that gate matches on. Before #1610, the raw inline form - (opening tag prompt-baked, never present in mot.value) only ever carried - the closing tag, so it was invisible to this same gate and reasoning - passed through by accident. - - This is a known limitation, not an oversight: it is the direct effect of - aligning HF's replay with the #1201 cross-backend consensus (replay only - on tool-call turns), left open for maintainer decision in PR #1616 rather - than resolved silently. If this test starts failing (reasoning present), - a reviewer changed that policy — update this test deliberately, don't - just delete the assertion. + """Deliberate limitation: on a plain multi-turn shape (assistant turn + with no tool call, followed by another user turn), Granite's own + `truncate_history_thinking` gate strips reasoning even though `to_chat()` + attaches `reasoning_content` — the reconstructed content carries both + tags, which is exactly what that gate matches on. Decided: keep HF + consistent with the #1201 cross-backend consensus (replay on tool-call + turns only) rather than extend replay to plain turns. """ messages = [ {"role": "user", "content": "What is 2 + 2?"}, From 2ab14eae82a6be5959c2bfa1249db9a84a43df53 Mon Sep 17 00:00:00 2001 From: Nigel Jones Date: Fri, 4 Sep 2026 14:37:31 +0100 Subject: [PATCH 08/10] docs(hf): trim verbose PR/design-doc references from code comments Comments and docstrings had accumulated references to PR numbers and a since-removed design doc. Tighten to state the behaviour and rationale directly instead of pointing elsewhere. Assisted-by: Claude Code Signed-off-by: Nigel Jones --- mellea/backends/utils.py | 26 ++++++-------------------- test/backends/test_utils.py | 6 ++---- 2 files changed, 8 insertions(+), 24 deletions(-) diff --git a/mellea/backends/utils.py b/mellea/backends/utils.py index 7a39f0c39b..664ae95cac 100644 --- a/mellea/backends/utils.py +++ b/mellea/backends/utils.py @@ -99,26 +99,12 @@ def to_chat( # NOTE: `self.formatter.to_chat_messages` explicitly skips `Message` objects. However, we need # to print `Message`s to correctly serialize any documents with the message. Do the printing here. - # NOTE: reasoning replay here is an interim, unconditional forward — every non-empty - # `Message.thinking` is attached as `reasoning_content` regardless of `should_replay_reasoning` - # (unlike OpenAI/LiteLLM/Watsonx/Ollama, which gate replay on that policy). This exists because - # the HF think-tag capture fix (#1610) alone removed reasoning that Granite's chat template - # previously saw on every turn: before that fix, raw `...` text sat inline in - # `content`, and after it, `content` is tag-free and the template prepends an empty - # `` instead (chat_template.jinja:83-90). - # - # Restores parity ONLY for tool-call turns (a turn after the last user message, or one the - # template's `truncate_history_thinking` gate otherwise doesn't touch) — those match the - # #1201 cross-backend consensus (replay on tool-call turns only) and the reconstructed - # `...` survives unmodified. Does NOT restore parity for plain multi-turn - # (an assistant turn with no tool call, before the last user message): the template's - # truncation branch strips reasoning whenever content carries BOTH tags - # (chat_template.jinja:99,137-145), and the reconstructed form always does — whereas the - # pre-#1610 inline form only ever carried the closing tag (the opening tag is prompt-baked, - # never present in `mot.value`), so it was invisible to that same gate and passed through by - # accident. This is a real, measured functional gap: whether HF should replay reasoning on - # plain turns too (diverging from the #1201 consensus) is open — see PR #1616 and the two - # rendered-prompt tests in test/backends/test_huggingface_thinking.py that pin both shapes. + # NOTE: `Message.thinking` is forwarded as `reasoning_content` (the key Granite/Qwen3 + # templates consume) whenever the message is an assistant turn with captured reasoning. + # This effectively restores replay only on tool-call turns: Granite's own + # `truncate_history_thinking` gate strips reasoning on plain turns regardless, matching + # the #1201 cross-backend consensus (replay on tool-call turns only) rather than + # extending it. See test_rendered_prompt_*_turn in test_huggingface_thinking.py. ctx_as_conversation: list = [] for m in ctx_as_message_list: msg_dict: dict = {"role": m.role, "content": formatter.print(m)} diff --git a/test/backends/test_utils.py b/test/backends/test_utils.py index fde3def9f5..4889d1b687 100644 --- a/test/backends/test_utils.py +++ b/test/backends/test_utils.py @@ -152,10 +152,8 @@ def test_to_chat_basic_message(): def test_to_chat_attaches_reasoning_content_for_assistant_thinking(): - """Regression test: an assistant Message carrying `.thinking` must have it - forwarded as `reasoning_content` on the wire dict. Before this fix, `to_chat` - read only `m.role`/`m.content` and silently dropped `.thinking` on every - HF replay — see PR #1616. + """An assistant Message carrying `.thinking` must have it forwarded as + `reasoning_content` on the wire dict. """ from mellea.backends.utils import to_chat from mellea.formatters.template_formatter import TemplateFormatter as ChatFormatter From f3c36945fea44de900c3c981e86f01a52f7b3672 Mon Sep 17 00:00:00 2001 From: Nigel Jones Date: Thu, 10 Sep 2026 11:06:14 +0100 Subject: [PATCH 09/10] docs(hf): correct overstated token-boundary claim, document gate tradeoff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _split_think_tags()'s docstring claimed token-level detection "can't disambiguate a genuine end-of-reasoning token from literal text either" — overstated. Verified against the real granite-4.2-3b tokenizer: encoding the literal string always yields the same single token id, but that only shows what the tokenizer's own encoder would choose, not what the model can generate — it may emit the tag as separate token pieces that decode to the same string. A check against the raw generated sequence would therefore be strictly more conservative than the string match (though not fully precise, and blocked today by streaming only exposing decoded text via TextIteratorStreamer). Also documents the under-split/over-split tradeoff behind the post_processing() gate, with the specific downstream mechanics (reasoning_content replay, Granite's own history truncation) that make over-splitting the worse failure mode. Assisted-by: Claude Code Signed-off-by: Nigel Jones --- mellea/backends/huggingface.py | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/mellea/backends/huggingface.py b/mellea/backends/huggingface.py index 5a87e01277..cceb664a64 100644 --- a/mellea/backends/huggingface.py +++ b/mellea/backends/huggingface.py @@ -284,9 +284,19 @@ def _split_think_tags(text: str) -> tuple[str | None, str]: A string-level fallback for `transformers.PreTrainedTokenizerBase.parse_response()` (schema-driven, token-decode-then-parse), used because no tokenizer available to this backend today declares a `response_schema`. Even that upstream mechanism is - text-level, not token-level, and can't disambiguate a genuine end-of-reasoning - token from literal `` text for Granite either — its own `` token - is registered non-special. + text-level, not token-level. + + A token-position check against the raw generated sequence (scanning for + Granite's `` token id rather than re-scanning decoded text) would be + strictly more conservative than this string match — it would not misfire on + a mention the model spells out as separate pieces rather than emitting the + single vocab token. It would not fully eliminate false positives either: a + model is free to emit that same single token for a literal in-prose mention, + so a genuine close and a canonical-token literal mention still decode + identically. Not adopted here because HF's streaming path + (`TextIteratorStreamer`) only exposes decoded text, not token ids, so a + token check could not apply uniformly to both the streaming and + non-streaming paths — see #1604 for a scoped follow-up. Splits on alone (not a ... pair) because some chat templates (e.g. granite-4.2 with enable_thinking) bake the opening tag into @@ -1870,6 +1880,18 @@ class used during generation, if any. # this mirrors what the model was actually asked to do on this call. # Skip for streaming: astream() assumes mot.value only grows, and shrinking it here # would corrupt the final delta (see #1604 for proper incremental splitting later). + # + # Known limitation, deliberately not addressed here: this gate can under-split + # for a model that ignores an explicit ModelOption.THINKING=False, or that always + # emits blocks without declaring any of _CHAT_TEMPLATE_THINKING_VARS in its + # template. The resulting raw tags leak into mot.value and get replayed as ordinary + # `content` on the next turn (still read by requirement checks/judges). The + # alternative — splitting unconditionally — trades this for a worse failure: an + # over-split misfiles real answer text into mot.thinking, which then gets attached + # as `reasoning_content` at replay time and is stripped outright by Granite's own + # template on any non-tool-call turn (mellea/backends/utils.py) — a permanent loss + # of answer content, not just a mislabeling. Kept on the more conservative side + # deliberately; see #1604 for detecting always-thinking models without a declared var. thinking_allowlist: frozenset[str] = getattr( self, "_chat_template_allowlist", frozenset() ) From 3eb84c735b884233345ca78c2b9b43f975d7549e Mon Sep 17 00:00:00 2001 From: Nigel Jones Date: Thu, 10 Sep 2026 11:34:43 +0100 Subject: [PATCH 10/10] docs(hf): fix turn-type/turn-recency mix-up in reasoning-replay comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both comments claimed Granite's chat template strips reasoning_content on "any non-tool-call/plain turn" — checked against the actual chat_template.jinja and that's wrong. The template gates on turn recency (loop.index0 >= last_user_idx), identically in both the tool-call and plain-turn branches; there is no turn-type check. Tool-call turns just usually happen to have no intervening user message, so they usually land on the "keep" side of the recency gate. Also notes HF has no should_replay_reasoning() gate unlike the other backends, and that truncate_history_thinking defaults True and is never overridden by mellea today. Assisted-by: Claude Code Signed-off-by: Nigel Jones --- mellea/backends/huggingface.py | 16 +++++++++------- mellea/backends/utils.py | 15 ++++++++++----- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/mellea/backends/huggingface.py b/mellea/backends/huggingface.py index cceb664a64..55b601ba59 100644 --- a/mellea/backends/huggingface.py +++ b/mellea/backends/huggingface.py @@ -1885,13 +1885,15 @@ class used during generation, if any. # for a model that ignores an explicit ModelOption.THINKING=False, or that always # emits blocks without declaring any of _CHAT_TEMPLATE_THINKING_VARS in its # template. The resulting raw tags leak into mot.value and get replayed as ordinary - # `content` on the next turn (still read by requirement checks/judges). The - # alternative — splitting unconditionally — trades this for a worse failure: an - # over-split misfiles real answer text into mot.thinking, which then gets attached - # as `reasoning_content` at replay time and is stripped outright by Granite's own - # template on any non-tool-call turn (mellea/backends/utils.py) — a permanent loss - # of answer content, not just a mislabeling. Kept on the more conservative side - # deliberately; see #1604 for detecting always-thinking models without a declared var. + # `content` on the next turn — visible in the answer, and still read by requirement + # checks/judges, but not silently dropped anywhere. The alternative — splitting + # unconditionally — trades this for a worse failure: an over-split misfiles real + # answer text into mot.thinking, which gets attached as `reasoning_content` at + # replay time and is silently dropped from every subsequent prompt once a newer + # user turn exists, per Granite's own recency-based truncation (see the NOTE in + # mellea/backends/utils.py). Under-split stays diagnosable; over-split is invisible + # once replayed. Kept on the more conservative side deliberately; see #1604 for + # detecting always-thinking models without a declared var. thinking_allowlist: frozenset[str] = getattr( self, "_chat_template_allowlist", frozenset() ) diff --git a/mellea/backends/utils.py b/mellea/backends/utils.py index 664ae95cac..d9fbaa0b96 100644 --- a/mellea/backends/utils.py +++ b/mellea/backends/utils.py @@ -100,11 +100,16 @@ def to_chat( # NOTE: `self.formatter.to_chat_messages` explicitly skips `Message` objects. However, we need # to print `Message`s to correctly serialize any documents with the message. Do the printing here. # NOTE: `Message.thinking` is forwarded as `reasoning_content` (the key Granite/Qwen3 - # templates consume) whenever the message is an assistant turn with captured reasoning. - # This effectively restores replay only on tool-call turns: Granite's own - # `truncate_history_thinking` gate strips reasoning on plain turns regardless, matching - # the #1201 cross-backend consensus (replay on tool-call turns only) rather than - # extending it. See test_rendered_prompt_*_turn in test_huggingface_thinking.py. + # templates consume) for every assistant turn that has it. Granite's own chat template + # (not a turn-type check) then decides whether to keep or strip it: reasoning survives + # only for a turn at or after the most recent user message (`last_user_idx` / + # `truncate_history_thinking`, defaulted True and never overridden by mellea), and is + # stripped for anything from an earlier exchange, tool-call or not. In mellea's typical + # flow a tool-call turn has no intervening user message before its continuation, so it + # tends to survive, and a plain turn from a prior exchange tends not to — but that's a + # consequence of the recency rule, not a tool-call/plain-turn distinction Mellea enforces + # (unlike #1201's cross-backend `should_replay_reasoning`, which HF does not call). See + # test_rendered_prompt_*_turn in test_huggingface_thinking.py. ctx_as_conversation: list = [] for m in ctx_as_message_list: msg_dict: dict = {"role": m.role, "content": formatter.print(m)}