fix(hf): handle ModelOption.THINKING chat templates - #1588
Conversation
Assisted-by: Codex Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
Assisted-by: Codex Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
Assisted-by: Codex Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
Assisted-by: Codex Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
Assisted-by: Codex Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
jakelorocco
left a comment
There was a problem hiding this comment.
agree with the approach; I think there's a few small concerns
AngeloDanducci
left a comment
There was a problem hiding this comment.
Looks like _make_backend hardcodes to_mellea_model_opts_map = {}, so test_filter_for_chat_template_uses_per_call_thinking_alias_over_default and _preserves_native_thinking_value pass only because the real map is absent. They don't exercise real behavior, which is why the ordering bug isn't caught.
See suggested code block - though I think the tests will need to be updated if that change is accepted.
to_mellea_model_opts_map had no entries for think/thinking/enable_thinking, so a raw alias key never folded into ModelOption.THINKING the way it does for Ollama. This let a pre-existing alias key in backend_opts silently block the resolved THINKING value in _filter_for_chat_template, giving HF different precedence than the other backends for the same input. Add the alias entries and let the resolved sentinel value win outright over any alias already present, matching Ollama's behavior. Also documents that non-boolean THINKING values are ignored on HF, and updates/adds tests covering the alias fold and the precedence fix. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
_try_load_granite_tokenizer only checked for a cached config.json before calling AutoTokenizer.from_pretrained(local_files_only=True). A cached config.json does not guarantee the tokenizer's own files are cached too (e.g. after a partial/interrupted download), so from_pretrained could raise OSError instead of the test skipping as intended. Catch it and treat it the same as an absent cache. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
|
Heads up: this PR and #1587 both independently touch `test/backends/test_acall_tools_parallel_execution.py`'s `backend` fixture (both switch it from a live `OllamaModelBackend()` to `mock_ollama_backend()`) — but the diffs aren't identical: this PR also drops `pytestmark = [pytest.mark.integration]` and rewords the fixture's docstring, #1587 keeps both as-is. Whichever merges second will hit a small, easily-resolved conflict on that one fixture — worth resolving deliberately (keep #1588's `pytestmark` removal + docstring wording, or not) rather than letting the merge tool guess. No other overlap: #1587 doesn't touch `LocalHFBackend`/HF THINKING handling (that's this PR's/#1212's scope), and this PR doesn't touch Granite 4.2 defaults or CI/`quality.yml`. |
Pull Request
Issue
Fixes #1212
Description
LocalHFBackendpreviously removedModelOption.THINKINGbefore applying thetokenizer chat template. This change detects the template's supported thinking
variable (
think,thinking, orenable_thinking) and forwards booleanvalues under that name. It also canonicalises native aliases during option
resolution, so per-call options correctly override backend defaults.
Following review,
to_mellea_model_opts_mapnow mapsthink/thinking/enable_thinkingtoModelOption.THINKING(it previously had no entries forthese, so a raw alias never folded into the sentinel the way it does for
Ollama).
_filter_for_chat_templatenow lets the resolved sentinel value winoutright over any leftover alias key instead of deferring to whichever
happened to already be present, matching Ollama's precedence for the same
input. The
ModelOption.THINKINGdocstring now notes that non-boolean valuesare silently ignored on HF.
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.
Its cache-presence check now also tolerates a partial local cache (cached
config.jsonwithout the tokenizer's own files) by treating anOSErrorfromAutoTokenizer.from_pretrained(local_files_only=True)as "not cached" ratherthan letting the test error.
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
uv run pytest test/backends/test_huggingface_filter_options.py -q: 34 passed, 3 skipped; Granite 4.2 tokenizer regression executeduv run pytest test/backends/test_acall_tools_parallel_execution.py -q: 4 passed without Ollamauv run ruff format --check . && uv run ruff check .: passeduv run mypy .: passeduv run pytest test/ -m "not qualitative": 4,146 passed, 19 skipped, 128 deselected, 4 xpassedAttribution
Adding a new component, requirement, sampling strategy, or 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.