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

fix: add a timeout to langchain callback handler #17296

Merged
merged 1 commit into from
Dec 17, 2024
Merged

Conversation

masci
Copy link
Member

@masci masci commented Dec 17, 2024

Description

The StreamingGeneratorCallbackHandler methods only handles events related to the llm lifecycle. If the thread terminates abnormally, then the while loop in get_response_gen will run indefinitely.

Fixes # n/a

Notes:

  • since the timeout has a default value, the change is not backward compatible for callers implicitly relying on a longer execution time - they will get a timeout error if this time is longer than the default 120 seconds. We might increase the default timeout to mitigate this problem, but we can't fix it for good.
  • given the dependency on langchain, this change doesn't have unit tests.

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Dec 17, 2024
Copy link
Collaborator

@logan-markewich logan-markewich left a comment

Choose a reason for hiding this comment

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

The fix looks good to me. Although I wonder if this should be moved to LangChainLLM?

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Dec 17, 2024
@masci masci merged commit d1ecfb7 into main Dec 17, 2024
11 checks passed
@masci masci deleted the massi/gen-timeout branch December 17, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants