Fix KeyError in apply_chat_template when message has no content (#45290)#45309
Closed
agentspan wants to merge 1 commit intohuggingface:mainfrom
Closed
Conversation
35eb7da to
4244cad
Compare
Contributor
|
[For maintainers] Suggested jobs to run (before merge) run-slow: smolvlm |
…ntent (huggingface#45290) Closes huggingface#45290 ## Root cause `ProcessorMixin.apply_chat_template` and several related code paths assumed every message in a conversation has a `content` key, then iterated over it directly. Assistant messages with `tool_calls` and no textual content (a valid shape per the OpenAI chat-completion spec) would crash with `KeyError: 'content'`. ## Fix strategy Default missing `content` to an empty list (`message.get("content", [])`) so the loop body still iterates correctly with zero items, instead of either skipping the message (which breaks downstream pairing of messages to model output) or raising. Where downstream code expected a string, default to "" instead. This preserves all existing semantics for messages that DO have content while making the missing-content case a no-op rather than a crash. ## Locations patched (5 sites) 1. `src/transformers/processing_utils.py` (~L1804) — main `apply_chat_template` loop that extracts images/videos/audio from message content. Also tightened a follow-up `message["content"].append(...)` to guard the list type. 2. `src/transformers/models/smolvlm/processing_smolvlm.py` (~L336) — video-detection list comprehension across all messages. 3. `src/transformers/cli/serving/utils.py` (~L927) — VLM/LLM modality message parser used by the OpenAI-compatible serving CLI. 4. `src/transformers/utils/chat_template_utils.py` (~L539) — Jinja template renderer; normalises every message to have a `content` key before passing to the template engine, and defends `chat[-1]["content"]` in the `continue_final_message` path. 5. `tests/test_tokenization_mistral_common.py` already targets the tokenization_mistral_common path; the source there already uses `.get("content")` so no source patch was needed there. ## Why safe - Empty list / empty string is the correct semantic default — assistant messages with only `tool_calls` simply have no visual/audio/text payload to extract. - Downstream template rendering and tokenization already handle empty content lists/strings. - No existing tests reference messages without content, so this change is strictly additive: every prior input still produces the same output, and the previously crashing input now produces the natural empty result. ## Test coverage added Three new regression tests across three different test files, one per public surface that was patched, addressing iter-2 review feedback that prior coverage was too narrow: 1. `tests/test_processing_common.py::test_apply_chat_template_with_tool_calls_no_content` — exercises `ProcessorMixin.apply_chat_template` with the exact failing conversation shape from the issue. 2. `tests/cli/test_serve.py::test_get_processor_inputs_with_tool_calls_no_content_vlm` and `test_get_processor_inputs_with_tool_calls_no_content_llm` — exercise both modalities of the CLI serving message parser. 3. `tests/test_tokenization_mistral_common.py::test_apply_chat_template_with_tool_calls_no_content` — exercises the Mistral tokenizer's chat-template path. All three tests construct a conversation with an assistant message that has `tool_calls` and no `content`, then call the surface under test and assert that no `KeyError` is raised. ## Validation - Original failing repro from the issue now exits 0 (verified locally). - Diff scope: 5 source files, 3 test files, 149 insertions, 17 deletions. - No regressions vs baseline test snapshot. - Iterated 3 times against an automated review loop; this commit addresses every concern from iter-1 (wrong strategy) and iter-2 (narrow test coverage). --- *Prepared by an AgentSpan agent. Manual review expected before pushing upstream.*
4244cad to
90130ae
Compare
Contributor
|
View the CircleCI Test Summary for this PR: https://huggingface.co/spaces/transformers-community/circle-ci-viz?pr=45309&sha=90130a |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #45290.
ProcessorMixin.apply_chat_templateand several related code paths assumedevery message in a conversation has a
contentkey. Assistant messages withtool_callsand no textual content (a valid shape per the OpenAIchat-completion spec — the assistant is requesting tool execution and has
no textual reply yet) crashed with
KeyError: 'content'.This PR introduces a single
get_message_contenthelper intransformers.utils.chat_template_utilsand migrates every call site thatpreviously assumed
message["content"]exists to use it. The helper defaultsmissing
contentto either an empty list (for code that iterates overmultimodal content blocks) or an empty string (for text-only paths), so the
loop body still runs over zero items instead of either skipping the message
entirely or raising.
Why a centralized helper
Without a helper, every processor / tokenizer / serving handler that needs
to access
message["content"]would have to remember the conventionindependently, and any future contributor adding a new entry point could
re-introduce the same bug. The helper:
defaultparameter that is keyword-only so call sites areself-documenting (
get_message_content(msg, default=[])vsget_message_content(msg, default=""))message.get("content", ...)Sites patched
All five via the new helper:
src/transformers/processing_utils.py— mainapply_chat_templateloopthat extracts images / videos / audio from message content
src/transformers/models/smolvlm/processing_smolvlm.py— video-detectionlist comprehension across all messages
src/transformers/cli/serving/utils.py— VLM and LLM modality messageparser used by the OpenAI-compatible serving CLI
src/transformers/utils/chat_template_utils.py— Jinja template renderer;normalises every message before passing to the template engine
src/transformers/tokenization_mistral_common.py—_maybe_adapt_messagehelper that normalizes content format
is_valid_messageconsistency fixis_valid_message(used bytransformers.pipelines.baseto detectchat-format inputs) previously rejected any message that did not have a
contentkey. Under the new convention this would reject validtool-call-only assistant messages, so it now accepts messages with either a
contentkey OR atool_callskey (still requiringrole). TheChatclass docstring and its constructor's error message are updated to match.
Tests
New unit tests in
tests/utils/test_chat_template_utils.pyGetMessageContentTest— 5 tests covering returns-content, default-list,explicit-string default, explicit-list default, and keyword-only enforcement
(positional default raises
TypeError)IsValidMessageTest— 5 tests covering accepts role+content, acceptsrole+tool_calls without content, rejects missing role, rejects messages
missing both content and tool_calls, rejects non-dict inputs
New regression tests across the patched surfaces
tests/test_processing_common.py::ProcessorTesterMixin::test_apply_chat_template_with_tool_calls_no_contenttests/cli/test_serve.py::TestProcessorInputsFromMessages::test_vlm_tool_calls_without_contenttests/cli/test_serve.py::TestProcessorInputsFromMessages::test_llm_tool_calls_without_contenttests/test_tokenization_mistral_common.py::TestMistralCommonBackend::test_apply_chat_template_with_tool_calls_no_contentAll construct a conversation containing
{"role": "assistant", "tool_calls": [...]}with nocontentkey andassert no
KeyErroris raised.Verification
The exact failing repro from the issue body now exits 0:
Locally:
ruff format --checkandruff checkare both clean on every changed filegrepfor rawmessage.get("content"/msg.get("content"across thepatched surface returns zero hits except inside the helper definition
Stats
5 source files patched, 1 source file gains the helper, 1 test file gains
helper unit tests, 3 test files gain regression tests. +282 / -23 lines.
Closes #45290