Skip to content

fix(hf): handle ModelOption.THINKING chat templates - #1588

Open
planetf1 wants to merge 5 commits into
generative-computing:mainfrom
planetf1:issue-1212
Open

fix(hf): handle ModelOption.THINKING chat templates#1588
planetf1 wants to merge 5 commits into
generative-computing:mainfrom
planetf1:issue-1212

Conversation

@planetf1

@planetf1 planetf1 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Pull Request

Issue

Fixes #1212

Description

LocalHFBackend previously removed ModelOption.THINKING before applying the
tokenizer chat template. This change detects the template's supported thinking
variable (think, thinking, or enable_thinking) and forwards boolean
values under that name. It also canonicalises native aliases during option
resolution, so per-call options correctly override backend defaults.

This PR deliberately does not add Granite 4.2 model identifiers or change
defaults; #1587 owns that work and its GPU span coverage. The tokenizer-only
integration test uses the public Granite 4.2 3B ID directly to prove the
generic mapping works with the template that first exposes a thinking control.

During validation, the parallel tool-call regression tests were found to create
a live Ollama backend despite exercising only formatter behaviour. This PR
switches them to the existing mocked backend fixture so the local suite does
not require Ollama for those tests.

Testing

  • Tests added to the respective file if code was changed
  • New code has 100% coverage if code was added
  • Ensure existing tests and github automation passes (a maintainer will kick off the github automation when the rest of the PR is populated)
    • uv run pytest test/backends/test_huggingface_filter_options.py -q: 33 passed, 3 skipped; Granite 4.2 tokenizer regression executed
    • uv run pytest test/backends/test_acall_tools_parallel_execution.py -q: 4 passed without Ollama
    • uv run ruff format --check . && uv run ruff check .: passed
    • uv run mypy .: passed
    • uv run pytest test/ -m "not qualitative": 3,988 passed, 179 skipped, 128 deselected, 1 xpassed

Attribution

  • AI coding assistants used

Adding a new component, requirement, sampling strategy, or tool?

  • Component
  • Requirement
  • Sampling Strategy
  • Tool

NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.

Assisted-by: Codex
Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
Assisted-by: Codex
Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
@github-actions github-actions Bot added the bug Something isn't working label Aug 26, 2026
Assisted-by: Codex
Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
Assisted-by: Codex
Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
@planetf1
planetf1 marked this pull request as ready for review August 26, 2026 09:48
@planetf1
planetf1 requested a review from a team as a code owner August 26, 2026 09:48
Assisted-by: Codex
Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>

@jakelorocco jakelorocco left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree with the approach; I think there's a few small concerns

Comment on lines +2087 to +2092
if (
thinking_template_var is not None
and type(model_options.get(ModelOption.THINKING)) is bool
and thinking_template_var not in backend_opts
):
backend_opts[thinking_template_var] = model_options[ModelOption.THINKING]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only forwards the value if it's a bool. That seems to differ slightly from the docstring updates, and I'm not sure it's the correct behavior.

)
if (
thinking_template_var is not None
and type(model_options.get(ModelOption.THINKING)) is bool

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please include a comment here that this is using model_options and specifically not using the backend_opts that have the THINKING flags removed?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This being after the backend specificity flag also causes ordering differences between the other backends and hf. For instance:

dict = {ModelOption.THINKING: True, "thinking": False}

# Ollama
backend_dict = {"thinking": True}  # Since _make_backend_specific_and_remove prefers the ModelOption sentinel

# HF
backend_dict = {"thinking": False} # Since _make_backend_specific_and_remove gets rid of the THINKING sentinel; causing the regular "thinking" option to take precedence when looking if it exists in the `backend_opts` dict

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

enhancement: huggingface should handle ModelOption.THINKING

2 participants