Skip to content
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

Cover Edge Case: Empty array instead of None for Assistant tool calls #61

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Eliotdoesprogramming
Copy link

@Eliotdoesprogramming Eliotdoesprogramming commented Oct 24, 2024

Hey didn't see any contributing guidelines so let me know if I can clean up this PR more

Adding an empty array check for assistant message normalization to account for a bug I had encountered working with langchain AIMessage default behavior (but also could be a valid edge case working purely with mistral).

When passing an assistant message to normalizer, if the assistant message has [] as tool calls instead of None, the content will not be added to the message. Having a message with content = None causes errors on chat serving endpoints downstream e.g. vLLM

Current Behavior:

# relevant snippet
            if message.tool_calls is not None:
                for tool_call in message.tool_calls:
                    normalized_tool_call = self._normalize_tool_call(tool_call)
                    tool_calls.append(normalized_tool_call)
            elif message.content:
                aggregated_content.append(self._aggregate_content_chunks(message.content))
# test code
      message = AssistantMessage(role="assistant", content="Hello", tool_calls=[])
      normalized_message = normalizer._aggregate_assistant_messages([message])
      assert normalized_message.content == "Hello" # message content will be None, and assertion fails

If [] is passed to this function, the if statement will evaluate as True, making it so that the elif statement is skipped and any message content is ignored.

Proposed behavior

# relevant snippet
            if message.tool_calls: # empty array evaluates to false
                for tool_call in message.tool_calls:
                    normalized_tool_call = self._normalize_tool_call(tool_call)
                    tool_calls.append(normalized_tool_call)
            elif message.content:
                aggregated_content.append(self._aggregate_content_chunks(message.content))

If [] is passed to this function, the if statement will evaluate as False, making it so that the elif statement will add its message content.

# test code
      message = AssistantMessage(role="assistant", content="Hello", tool_calls=[])
      normalized_message = normalizer._aggregate_assistant_messages([message])
      assert normalized_message.content == "Hello" # message content Hello and assertion fails

Hopefully this helps keep our AI ecosystem bug free 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant