-
Notifications
You must be signed in to change notification settings - Fork 330
feat: structured output support, using LC standard content blocks #379
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
Conversation
060a931 to
b50e975
Compare
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.
linting fixes
langchain_mcp_adapters/tools.py
Outdated
| if isinstance(content, ResourceLink): | ||
| block: ToolMessageContentBlock = {"type": "file", "url": str(content.uri)} | ||
| if content.mimeType: | ||
| block["mime_type"] = content.mimeType | ||
| return block |
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.
do we want to use a file for this?
| # Otherwise, convert from CallToolResult | ||
| text_contents: list[TextContent] = [] | ||
| non_text_contents = [] | ||
| for content in call_tool_result.content: | ||
| if isinstance(content, TextContent): | ||
| text_contents.append(content) | ||
| else: | ||
| non_text_contents.append(content) | ||
|
|
||
| tool_content: str | list[str] = [content.text for content in text_contents] | ||
| if not text_contents: | ||
| tool_content = "" | ||
| elif len(text_contents) == 1: | ||
| tool_content = tool_content[0] |
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.
This is the big question, can we remove this funky str concat logic.
I think it'd be more consistent if we always populated tool messages w/ standard content blocks, and we're not on 1.0 yet, so we do have some room to make breaking changes.
On the other hand, the value here isn't that high, so I'm ok w/ leaving as is if desired to avoid pain for users.
cc @eyurtsev
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.
Do we expect this would break stuff? Would .text / .content continue working on tool messages as before? (with change being only on content_blocks?)
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.
This would break .content. So it's probably not worth the change.
BUT we are on 0.0.X here, so we have the ability to make this change if we want.
eyurtsev
left a comment
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.
This looks good, let's verify that content blocks work for a few different model providers (openai, anthropic) + chat history and that the trace looks OK on langsmith?
This PR introduces a few significant changes
MCPToolArtifacttype that containsstructured_outputfrom the MCP tool, if availableToolMessagecontentwith LangChain's standard content blocks. We do a best effort of the MCP content blocks onto these LC content blocks.Closes #283 (adding structured content support)
Closes #41 (supporting images + file content)
Fixes #295 (tool message contents should be serializable now)