-
Notifications
You must be signed in to change notification settings - Fork 65
Remote empty text content for tool calls & support kimi-k2-thinking
#1093
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
563781b
b34379e
a1d7f6d
0d87426
5f413c9
c3ec892
c187634
c4a7a75
d218af7
0bc52b2
2fb7339
6d11aa2
e586309
a95266c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -269,6 +269,14 @@ def to_chat_dict(self) -> dict[str, Any]: | |
| # Assistant function_call(s) | ||
| if self.role == "assistant" and self.tool_calls: | ||
| message_dict["tool_calls"] = [tc.to_chat_dict() for tc in self.tool_calls] | ||
| if "content" in message_dict: | ||
| normalized_content = self._normalize_tool_call_content( | ||
| message_dict["content"] | ||
| ) | ||
| if normalized_content is None: | ||
| message_dict.pop("content", None) | ||
| else: | ||
| message_dict["content"] = normalized_content | ||
|
|
||
| # Tool result (observation) threading | ||
| if self.role == "tool" and self.tool_call_id is not None: | ||
|
|
@@ -331,6 +339,35 @@ def _list_serializer(self) -> dict[str, Any]: | |
| # tool call keys are added in to_chat_dict to centralize behavior | ||
| return message_dict | ||
|
|
||
| def _normalize_tool_call_content(self, content: Any) -> Any | None: | ||
|
||
| """Remove empty text payloads from assistant tool call messages.""" | ||
| if isinstance(content, str): | ||
| if content.strip() == "": | ||
| return None | ||
| return content | ||
|
|
||
| if isinstance(content, list): | ||
| normalized: list[Any] = [] | ||
| for item in content: | ||
| if not isinstance(item, dict): | ||
| normalized.append(item) | ||
| continue | ||
|
|
||
| if item.get("type") == "text": | ||
| text_value = item.get("text", "") | ||
| if isinstance(text_value, str): | ||
| if text_value.strip() == "": | ||
| continue | ||
| else: | ||
| if str(text_value).strip() == "": | ||
| continue | ||
xingyaoww marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| normalized.append(item) | ||
|
|
||
| return normalized if normalized else None | ||
|
|
||
| return content | ||
|
|
||
| def to_responses_value(self, *, vision_enabled: bool) -> str | list[dict[str, Any]]: | ||
| """Return serialized form. | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OpenHands Read this PR diff carefully. First, I don't really like the name "normalize", it means everything so nothing, name this...
remove_empty_content_stringsorremove_content_if_empty, so it says what it does.Second, look at the whole file message.py and what we do, maybe it's better to refactor a little bit so we do the removal inside the helper method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on it! enyst can track my progress at all-hands.dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of changes applied to address your comment on openhands-sdk/openhands/sdk/llm/message.py:
What I changed
Why
Checklist
Notes
View full conversation