feat: Convert message[content] from list to string. #3067
Conversation
Signed-off-by: Krishnan Prashanth <[email protected]>
Signed-off-by: Krishnan Prashanth <[email protected]>
WalkthroughIntroduces a private helper to collapse array-based message content into newline-joined text. Updates NvCreateChatCompletionRequest::messages() to conditionally apply this collapse when any message content is an array. Adds unit tests covering multipart text collapsing, mixed message handling, empty arrays preservation, and unchanged single-string content. No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant R as NvCreateChatCompletionRequest
participant S as Serializer
participant F as may_be_fix_msg_content
participant H as HTTP Client
C->>R: build request (messages)
R->>S: serialize inner messages -> Value
alt any content is array
R->>F: collapse text parts
F-->>R: Value with collapsed content
else
R-->>R: use serialized messages as-is
end
R->>H: send request payload
H-->>C: response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/llm/src/preprocessor/prompt/template/oai.rs (2)
142-160: Tighten failure mode and keep behavior localized.
- Prefer expect(...) over unwrap() for clearer diagnostics.
- Optional: let the helper decide whether to change anything and remove the pre‑scan to avoid double traversal (minor).
- let messages_json = serde_json::to_value(&self.inner.messages).unwrap(); + let messages_json = serde_json::to_value(&self.inner.messages) + .expect("serializing chat messages to JSON should not fail");If you want to simplify:
- let needs_collapse = if let Some(arr) = messages_json.as_array() { - arr.iter().any(|msg| { - msg.get("content") - .and_then(|c| c.as_array()) - .is_some() - }) - } else { - false - }; - - if needs_collapse { - may_be_fix_msg_content(messages_json) - } else { - Value::from_serialize(&messages_json) - } + may_be_fix_msg_content(messages_json)
410-487: Add a test for mixed content (text + image) to prevent regressions.Ensure we don’t collapse when non‑text parts are present.
@@ fn test_may_be_fix_msg_content_mixed_messages() { @@ } + + #[test] + fn test_may_be_fix_msg_content_text_and_image_preserved() { + let json_str = r#"{ + "model": "gpt-4o", + "messages": [ + { + "role": "user", + "content": [ + {"type": "text", "text": "Caption:"}, + {"type": "image_url", "image_url": {"url": "https://example.com/cat.png"}} + ] + } + ] + }"#; + + let request: NvCreateChatCompletionRequest = serde_json::from_str(json_str).unwrap(); + let messages = serde_json::to_value(request.messages()).unwrap(); + + assert!(messages[0]["content"].is_array(), "Mixed parts must not be collapsed"); + let parts = messages[0]["content"].as_array().unwrap(); + assert_eq!(parts.len(), 2); + assert_eq!(parts[0]["type"], "text"); + assert_eq!(parts[1]["type"], "image_url"); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/llm/src/preprocessor/prompt/template/oai.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/llm/src/preprocessor/prompt/template/oai.rs (1)
lib/llm/src/preprocessor/prompt.rs (2)
messages(62-62)model(61-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and Test - dynamo
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: pre-merge-rust (.)
🔇 Additional comments (2)
lib/llm/src/preprocessor/prompt/template/oai.rs (2)
509-528: LGTM: single-string content remains unchanged.
491-507: ```shell
#!/bin/bash
set -euo pipefailecho "=== lib/llm/src/preprocessor/prompt/template/oai.rs (lines 480-520) ==="
if [ -f lib/llm/src/preprocessor/prompt/template/oai.rs ]; then
sed -n '480,520p' lib/llm/src/preprocessor/prompt/template/oai.rs || true
else
echo "file not found: lib/llm/src/preprocessor/prompt/template/oai.rs"
fiecho
echo "=== Search for template-related crates/usages (minijinja/jinja) ==="
rg -n --hidden --follow -S 'minijinja|jinja' || trueecho
echo "=== Find common template files by glob ==="
fd -t f -g '.j2' -g '.jinja' -g '.jinja2' -g '.tmpl' -g '.template' -g '.jinja.html' -g '*.jinja2.html' . || trueecho
echo "=== Search for 'content' inside Jinja-like braces across repo ==="
rg -n --hidden --follow -S '{{[^}]\bcontent\b[^}]}}' || trueecho
echo "=== Search for '+' concatenation with 'content' anywhere ==="
rg -n --hidden --follow -S '+\scontent|content\s+' || trueecho
echo "=== Broader search for 'content' occurrences (showing context) ==="
rg -n --hidden --follow -C2 '\bcontent\b' || trueecho
echo "=== Done ==="</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Signed-off-by: Krishnan Prashanth <[email protected]>
|
When have you seen this happen, these multiple text content blocks? Solution sounds good to me, but I'm curious what kind of clients do this. |
@ayushag-nv might be able to provide more insight into this. Additionally, this GitHub Issue (#2874) shows an inference request that passes in message[content] as a list of one element. Additionally, collapsing cc: @grahamking |
Signed-off-by: Krishnan Prashanth <[email protected]>
|
@KrishnanPrash Can you validate this as well. |
Signed-off-by: Krishnan Prashanth <[email protected]>
Signed-off-by: Krishnan Prashanth <[email protected]>
… multi-content, added docstrings Signed-off-by: Krishnan Prashanth <[email protected]>
|
/ok to test 9b69274 |
Signed-off-by: Krishnan Prashanth <[email protected]>
Overview:
This PR addresses an issue where Dynamo's OpenAI-compatible API fails when receiving messages with multi-part content arrays. This failure occurs because the underlying chat (jinja) template expects
messages[content]to be a string, but a list is provided.Example Request:
Output:
Details:
For text models, if a list is provided for
messages[content], we concatenate all the text provided into a single string to match the expectations of the chat template.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Bug Fixes
Tests