-
Notifications
You must be signed in to change notification settings - Fork 38
Langchain MCP Adapters #228
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
base: main
Are you sure you want to change the base?
Conversation
| workspace_client=WorkspaceClient(), | ||
| timeout=30.0, | ||
| sse_read_timeout=60.0, | ||
| handle_tool_error=True, # Return errors as strings instead of raising |
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.
QQ is this handle_tool_error param standard/necessary to start with?
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.
We are starting with this for bricks. Bricks is going to use it to handle tool errors and then show the tool call confirmations correctly.
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 think we should still include this since it's a base langchain tool concept, but won't the bricks folks encounter it at get_tools time?
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.
Just to help usability, is there a langchain doc page we can link to to explain? I saw the docstring above that keyword parameters like hanlde_tool_error get passed through to langchain's connection object, but IDK what that is :D. Maybe in DatabricksMCPServer's docstring we can link the relevant LangChain docs for LangChain's connection object?
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.
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.
Oh didnt know there were two more types, will add that in. I was trying to have the same parameter here as langchain
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 think the Langgraph toolNode is different. In Langchain we return BaseTool defined here: https://github.com/langchain-ai/langchain/blob/0a6d01e61df86b70d5b9afdb1ba68e7b2943e787/libs/core/langchain_core/tools/base.py#L483
But I cant find documentation for it
bbqiu
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.
overall LGTM!
did we figure out if we are able to propagate the OAuth error in a meaningful way? i remember you mentioned needing to modify something in the langchain code to surface it? (nb cell link)
integrations/langchain/src/databricks_langchain/multi_server_mcp_client.py
Show resolved
Hide resolved
smurching
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.
Mostly looks good, just some small comments!
Yep merged the PR here, will be in the next release: https://github.com/databricks-eng/universe/pull/1446936 |
Introduce Langchain MCP Adapters to make the MCP client easier to use.
Example Notebook: https://e2-spog.staging.cloud.databricks.com/editor/notebooks/367820108530247?o=6051921418418893#command/8418062200632198