Repository navigation
fix(serve): preserve tool-result image ownership - #5044
silencelamb wants to merge 2 commits into
Conversation
| @@ -251,37 +251,28 @@ def _merge_system_messages(messages: list[_MessageT]) -> list[_MessageT]: | |||
|
|
|||
|
|
|||
| def _convert_user_tool_result(block: ContentBlockParam | dict[str, Any]) -> list[dict[str, Any]]: | |||
There was a problem hiding this comment.
Keeping images associated with their tool result makes sense, but this changes the internal message format for all models. InternVLVisionModel.proc_messages() currently generates <IMG_CONTEXT> placeholders only for role="user", while
collect_multimodal_items() also collects images from tool messages.
Consequently, an Anthropic tool-result image that previously passed through the user-image path now bypasses placeholder conversion. With the real InternVL3-8B and InternVL3.5-8B templates, the same input changes from one image with one <IMG_CONTEXT>
placeholder to one image with zero placeholders, violating the downstream image-count assertion.
Please update the InternVL message normalization alongside this change:
- Handle multimodal content in both user and tool messages.
- Preserve the original message fields when replacing content, e.g. dict(message, content=normalized_content), instead of reconstructing a message with the hardcoded role="user". Apply this to both branches of proc_messages().
- Add a regression covering Anthropic tool-result images through an actual InternVL template, checking that image placeholders match the collected images.
This would preserve the ownership improvement while keeping the existing InternVL path working.
Keep images attached to tool call IDs across Chat, Messages and Responses. Align GLM-5.3-Flash's known template order, normalize InternVL tool images without losing metadata, and handle null assistant content. Cover real templates and custom Jinja rendering without introducing generic media-reference instrumentation.
f1eb40e to
603c608
Compare
Motivation
Tool-result images must remain associated with the call that produced them. Splitting an Anthropic result into tool text and a separate user image loses that association. For GLM-5.3-Flash, reversed result arrival can also mismatch rendered image placeholders and collected images because its default template renders results in call order.
Responses additionally rejects images in
function_call_outputand separates consecutive parallel calls into different assistant messages.Modification
Glm5NextForConditionalGenerationarchitecture. Other architectures, text-only groups, ambiguous IDs, and user/round boundaries keep their existing behavior. Text-only GLM-5.3 does not enter this path.<IMAGE_TOKEN>handling per message. A legacy placeholder in a user message must not suppress images in a separate tool result.Scope and follow-up
This revision retains the narrow GLM ordering rule. It does not add generic Jinja instrumentation or replace the ordinary Hugging Face rendering path.
A general implementation that derives media order from the final rendered template output is being investigated separately. The initial loop-tracking prototype rejected valid Qwen3-Omni templates that accumulate media tokens in variables, as well as custom templates using indexing, macro/string transformations, or loop control. That prototype is excluded from this PR. Its code and reproducible compatibility blockers are archived on the fork branch
experiment/rendered-media-order; the continuation notes describe the implementation, known regressions, and next steps. This remains an experimental snapshot, not a production-ready generic implementation. A follow-up must preserve existing rendering behavior and handle assignment, concatenation, macros, and final output order; blindly falling back to input order would reintroduce wrong-image associations for templates that reorder results.This PR does not establish tool-image support for every vision model. MiniCPM and other legacy/custom preprocessing paths still have independent limitations. Earlier Qwen3-VL-2B GPU runs also had tool-task answer failures in the baseline versions; the template/processor checks here do not claim those tasks are solved.
Validation
641 automated tests passed at
799a1969: the complete serving suite plus the new InternVL and HF-template compatibility tests. HTTP/SSE tests simulate generation; shared preprocessing tests decode real images. This is the selected regression suite, not the whole repository test suite.Complete raw request histories cover two representative scenarios across all three protocols: consecutive tool rounds with distinct IDs/images, and mixed plain-text/image results arriving out of order. Conversion is performed once on the whole history; assertions check roles, call grouping, result IDs/content, and final image collection. These six cases replace the earlier three cases that assembled/modified history after protocol conversion.
The existing GLM permutation matrix and Qwen/InternVL template tests now check unique text markers before/after each rendered image placeholder against the expected image pixel sequence. Placeholder counts alone are no longer the order oracle.
The shared GLM matrix covers all three protocols, all six permutations of three tool results, one/two images per result, and both PyTorch/TurboMind preprocessing branches, using the pinned official GLM template. These are frontend tests, not GPU tests of both backends.
Official InternVL3/3.5 templates cover all three protocols, retained tool metadata, image-only tool results, and mixed legacy/ordinary placeholders. Qwen3-VL, Qwen2.5-VL, and Qwen3-Omni retain their own template behavior. Custom indexing, macro/strip, and break/continue templates remain usable.
24/24 actual CPU tokenizer/vision-processor cases passed across GLM5.3-Flash, Qwen3-VL, Qwen3.5, and Intern-S1-Pro. The Qwen3-Omni processor could not initialize from the available invalid local metadata; its pinned official template was tested independently.
An exploratory three-version comparison covered 19 model configurations × 11 scenarios × three protocols. This revision introduced no pass-to-fail cases relative to the merged prerequisite or the previous published revision. This is a frontend diagnostic matrix, not an all-model acceptance suite: some legacy template combinations remain unsupported, and four configurations use synthetic template contracts.
18/18 standalone custom-template compatibility checks passed.
Applicable pre-commit checks and
git diff --checkpassed.Real GLM-5.3-Flash GPU validation at the production-code revision (
603c6082): 104/106 serial checks passed, including 42/42 ordinary user-image checks and 48/48 tool-image checks across Chat, Messages, and Responses, with non-streaming/SSE, reversed results, three-tool permutations, multiple images per tool, and multi-turn history. Final model answers and completion were checked.The remaining 2/16 negative-test failures are the existing unknown-only Chat content cases (non-streaming/SSE returning 200). This revision does not include the separate validation fix.
The service used the real GLM-5.3-Flash snapshot, actual vision preprocessing, and eight H200 GPUs with PyTorch TP=8, MP executor, eager mode, max batch size=2, and client concurrency=1. Its source hashes matched
603c6082throughout the run. The later799a1969commit changes tests only; the GPU suite was not rerun for that test-only update. The service has been stopped and its GPU processes released.The unknown-only Chat content validation issue is handled independently by #5047. No concurrency stability fix is included here. An earlier intermittent MP-executor stall did not recur in subsequent baseline/candidate A/B retests; its cause remains unresolved, and the serial functional results do not prove concurrent-serving stability.
Dependency and review scope
Rebased onto upstream
mainafter #5043 merged. The current diff contains the tool-image changes and their compatibility fixes; ordinary Responses image support is already in the base. The separate three-protocol ordinary-image HTTP test expansion remains in #5046.Checklist