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.

Sign up for GitHub

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

Jump to bottom

vertexai: refactor: simplify content processing in anthropic formatter #601

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

vertexai: refactor: simplify content processing in anthropic formatter #601

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aec906f
refactor: simplify content processing in anthropic formatter
Nov 14, 2024
b9ac975
chore: adding test and extract_tool_call for updated anthropic format…
Nov 16, 2024
c90f5d8
chore: fix test_anthropic_tool_calling integration test
Nov 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions libs/vertexai/langchain_google_vertexai/model_garden.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,14 +205,18 @@ def _format_params(

def _format_output(self, data: Any, **kwargs: Any) -> ChatResult:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we add a test for _format_output similar to the test that we have in the main lanchain library: https://github.com/langchain-ai/langchain/blob/f1222739f88bfdf37513af146da6b9dbf2a091c4/libs/partners/anthropic/tests/unit_tests/test_chat_models.py#L87-L109

We should base the input for the test here on what we noticed was causing the errors. So instead of passing only a TextBlock, we should only pass a ToolUseBlock like we do in our test. I suspect this will fail in the previous implementation.

What do you think?

data_dict = data.model_dump()
content = [c for c in data_dict["content"] if c["type"] != "tool_use"]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main difference, here, we would only have elements in content for which the type was not tool_use. This would break when there is only one element, and that element had the this type set. Causing content to be empty on L215

Copy link
Author

@jfypk jfypk Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome -- thanks for calling this out @Reprazent! updated the description as well.

content = content[0]["text"] if len(content) == 1 else content
content = data_dict["content"]
llm_output = {
k: v for k, v in data_dict.items() if k not in ("content", "role", "type")
}
tool_calls = _extract_tool_calls(data_dict["content"])
if tool_calls:
msg = AIMessage(content=content, tool_calls=tool_calls)
if len(content) == 1 and content[0]["type"] == "text":
msg = AIMessage(content=content[0]["text"])
elif any(block["type"] == "tool_use" for block in content):
tool_calls = _extract_tool_calls(content)
msg = AIMessage(
content=content,
tool_calls=tool_calls,
)
else:
msg = AIMessage(content=content)
# Collect token usage
Expand Down
Loading