Repository navigation
[Misc] Break chat_utils into more maintainable modules. - #60972
Conversation
Signed-off-by: wang.yuqi <yuqi.wang@daocloud.io>
Signed-off-by: wang.yuqi <yuqi.wang@daocloud.io>
3f81def to
be90fe1
Compare
Signed-off-by: wang.yuqi <yuqi.wang@daocloud.io>
703df09 to
e4c38db
Compare
|
/ci run |
github-actions
Bot
added
the
ready
|
✅ Triggered Buildkite CI #94432 for commit |
|
This pull request has merge conflicts that must be resolved before it can be |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
LGTM. The issues raised in earlier review rounds on this PR have since been fixed: ChatTemplateResolutionError is now a VLLMClientError subclass (correct 4xx mapping instead of the earlier 5xx regression), and chat_utils/__init__.py builds __all__ as a new list ([*all_types, ...]) rather than mutating types.__all__ in place. The remaining finding on this run is a stale docstring reference, not a functional issue.
Extended reasoning...
Verified in the current HEAD that vllm/exceptions.py:65 defines ChatTemplateResolutionError as a VLLMClientError (fixing the earlier 500-vs-400 regression I flagged), and vllm/renderers/chat_utils/init.py:26-41 now constructs all via [*all_types, ...] instead of mutating types.py's own all list in place (fixing the other earlier finding). The only remaining open item — missing re-export of AudioURL/VideoURL/MultiModalContentParser from the package — was discussed directly with DarkLight1337 (a maintainer, independent non-author) on the exact init.py:24 thread, which was resolved by that non-author and followed by their APPROVED review, so it reads as a reviewed-and-accepted tradeoff rather than an open objection. The change is a large but mechanical file-split refactor with renames and one exception-hierarchy relocation; no new security-sensitive surface (auth/crypto/permissions) is touched. This run's only new finding is a minor stale-docstring nit in vllm/entrypoints/cohere/serving.py, not something that should block merge.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
vllm/entrypoints/cohere/serving.py— nit: stale comment still names the old private helper and file. Line 514 says vLLM's shared_parse_chat_message_content_partsinvllm/renderers/chat_utils.pyrejects document blocks, but that function was renamed to parse_chat_message_content_parts and chat_utils.py was split into the vllm/renderers/chat_utils/ package. Fix: update this comment to the new public name and package path, matching the one already fixed three lines above at serving.py:505.Why this was flagged
vllm/entrypoints/cohere/serving.py:514-515 references
_parse_chat_message_content_partsinvllm/renderers/chat_utils.py, neither of which exists post-refactor (renamed to parse_chat_message_content_parts, moved into vllm/renderers/chat_utils/content_parser.py). A maintainer reading this docstring while debugging tool-message content handling is pointed at a nonexistent symbol/path, unlike serving.py:505 a few lines above which was correctly updated in the same diff. No functional effect, documentation only.Verification: nit: serving.py:515-516 still reads "vLLM's shared
_parse_chat_message_content_partsinvllm/renderers/chat_utils.pyrejects" — both the private name and the monolithic file path are stale post-refactor. The function was renamed to the publicparse_chat_message_content_partsand chat_utils.py was split into the vllm/renderers/chat_utils/ package.
|
/ci run |
|
✅ Triggered Buildkite CI #94477 for commit |
|
/ci run |
|
✅ Triggered Buildkite CI #94480 for commit |
Resolve conflict with the chat_utils split (vllm-project#60972): move normalize_reasoning_content into vllm/renderers/chat_utils/chat_message.py next to has_non_text_content and export it from the package. Signed-off-by: ExtReMLapin <3909752+ExtReMLapin@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fhutqf6Gos9LrhuBD6UCJv
…#60972) Signed-off-by: wang.yuqi <yuqi.wang@daocloud.io> Signed-off-by: Andreas Karatzas <AndreasKaratzas@users.noreply.github.com>
…#60972) Signed-off-by: wang.yuqi <yuqi.wang@daocloud.io>
…is a package, ChatTemplateResolutionError in vllm/exceptions.py Classification: version-drift (moved paths and symbols) Source: vllm-project/vllm#60972 (commit 669f85e686, first in v0.33.0) Source: vllm-project/vllm#62390 (commit 5caabdbb65, first in v0.33.0) Before: SKILL.md cited vllm/renderers/chat_utils.py and _get_full_multimodal_text_prompt(); debugging.md labelled five validation errors ValueError. After: vllm/renderers/chat_utils/{chat_template,chat_message,content_parser}.py, get_full_multimodal_text_prompt(), vllm/exceptions.py; the five errors raise VLLMValidationError (read at the v0.33.0 tag; the path-like one already did at v0.32.0). sources.md carries the v0.33.0 line anchors. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Overview
[Misc] Break chat_utils into more maintainable modules.
#60522 (review)
Claims
Validation
Details
Pull Request Checklist
I used vLLM's
/pr-checklistskill. (Mandatory for agents, optional for humans).AI assistance was used during the creation of this PR.
Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.
Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.
Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.
Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.