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

Update tool callbacks to send the actual response from the tool #18760

Closed
1 task done
eyurtsev opened this issue Mar 7, 2024 · 12 comments
Closed
1 task done

Update tool callbacks to send the actual response from the tool #18760

eyurtsev opened this issue Mar 7, 2024 · 12 comments
Assignees
Labels
01 bug Confirmed bug 03 enhancement Enhancement of existing functionality 🤖:improvement Medium size change to existing code to handle new use-cases streaming

Comments

@eyurtsev
Copy link
Collaborator

eyurtsev commented Mar 7, 2024

Privileged issue

  • I am a LangChain maintainer, or was asked directly by a LangChain maintainer to create an issue here.

Issue Content

Reproducible issue. on_tool_end logs the string representation of the tool. All tool callbacks need to be updated to accommodate

from langchain_core.tools import tool
from langchain_core.documents import Document

def foo(x):
    return {
        'x': 5
    }

@tool
def get_docs(x: int):
    """Hello"""
    return [Document(page_content='hello')]

chain = foo | get_docs

async for event in chain.astream_events({}, version='v1'):
    if event['event'] == 'on_tool_end':
        print(event)
@eyurtsev eyurtsev self-assigned this Mar 7, 2024
@eyurtsev
Copy link
Collaborator Author

eyurtsev commented Mar 7, 2024

#18694

@eyurtsev eyurtsev added 01 bug Confirmed bug 03 enhancement Enhancement of existing functionality labels Mar 7, 2024
@dosubot dosubot bot added the 🤖:improvement Medium size change to existing code to handle new use-cases label Mar 7, 2024
@eyurtsev
Copy link
Collaborator Author

eyurtsev commented Mar 7, 2024

This is a bug in the astream_events API and an enhancement for existing tool callback handlers

@keenborder786
Copy link
Contributor

@eyurtsev I am taking this and working on it.

@eyurtsev
Copy link
Collaborator Author

eyurtsev commented Mar 8, 2024

@keenborder786 thanks for taking a look at this! The fix is actually more involved since it needs to be backwards compatible with existing callback handlers. I'm going to scope it out today

@keenborder786
Copy link
Contributor

@eyurtsev oh okay. Can you please explain a little about the backwards compatibility in this case so I can help you as well. All the existing handlers that are using on_tool_end need to work properly??? Correct.

@eyurtsev
Copy link
Collaborator Author

eyurtsev commented Mar 8, 2024

Any code that relies on the output from the handlers should not break

@keenborder786
Copy link
Contributor

keenborder786 commented Mar 8, 2024 via email

@eyurtsev
Copy link
Collaborator Author

eyurtsev commented Mar 8, 2024

I'm looking through to figure out what can be affected. For example, one thing is BaseTracer which is used to power both astream_log and astream_events. If we change the output of the on_tool_end, we will fix the output from astream_events, but we also might cause an unexpected breaking change (e.g., if users are sending the output of these APIs from the backend to the front end suddenly the code might stop serializing properly).

it's a bit tricky in some of these cases to figure out whether to make a breaking change (and keep the code simple) or make a more involved change so that we don't break user code

@eyurtsev
Copy link
Collaborator Author

eyurtsev commented Mar 8, 2024

OK @keenborder786 I think we can proceed with the PR that you made! I'm going to outline a few places to propagate information to so that we break the minimum amount of code

eyurtsev added a commit that referenced this issue Mar 11, 2024
This PR updates the on_tool_end handlers to return the raw output from the tool instead of casting it to a string. 

This is technically a breaking change, though it's impact is expected to be somewhat minimal. It will fix behavior in `astream_events` as well.

Fixes the following issue #18760 raised by @eyurtsev

---------

Co-authored-by: Eugene Yurtsev <eyurtsev@gmail.com>
@eyurtsev
Copy link
Collaborator Author

PR merged will be available in next release

@alex-dreo-persefoni
Copy link

@eyurtsev I don't see this issue (#18760) referenced in the latest release: https://github.com/langchain-ai/langchain/releases/tag/v0.1.12

When I test the original code that output a str(Document), I still see the same problematic output:
Screenshot 2024-03-13 at 11 38 16

Can you confirm that this issue was not fixed by the latest release?

@alex-dreo-persefoni
Copy link

alex-dreo-persefoni commented Mar 13, 2024

It looks like this PR might have to be merged before this issue can actually be corrected in a release. Is that correct?

bechbd pushed a commit to bechbd/langchain that referenced this issue Mar 29, 2024
This PR updates the on_tool_end handlers to return the raw output from the tool instead of casting it to a string. 

This is technically a breaking change, though it's impact is expected to be somewhat minimal. It will fix behavior in `astream_events` as well.

Fixes the following issue langchain-ai#18760 raised by @eyurtsev

---------

Co-authored-by: Eugene Yurtsev <eyurtsev@gmail.com>
gkorland pushed a commit to FalkorDB/langchain that referenced this issue Mar 30, 2024
This PR updates the on_tool_end handlers to return the raw output from the tool instead of casting it to a string. 

This is technically a breaking change, though it's impact is expected to be somewhat minimal. It will fix behavior in `astream_events` as well.

Fixes the following issue langchain-ai#18760 raised by @eyurtsev

---------

Co-authored-by: Eugene Yurtsev <eyurtsev@gmail.com>
hinthornw pushed a commit that referenced this issue Apr 26, 2024
This PR updates the on_tool_end handlers to return the raw output from the tool instead of casting it to a string. 

This is technically a breaking change, though it's impact is expected to be somewhat minimal. It will fix behavior in `astream_events` as well.

Fixes the following issue #18760 raised by @eyurtsev

---------

Co-authored-by: Eugene Yurtsev <eyurtsev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
01 bug Confirmed bug 03 enhancement Enhancement of existing functionality 🤖:improvement Medium size change to existing code to handle new use-cases streaming
Projects
None yet
Development

No branches or pull requests

3 participants