Skip to content

core[patch]: Fix RemoteRunnable streaming when used in sequences, add tracing #4882

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

Merged

Conversation

rahilvora
Copy link
Contributor

@rahilvora rahilvora commented Mar 26, 2024

This PR is trying to solve issue where RemoteRunnable object does not support streaming with RunnableSequence

Code Flow
RunnableSequence and RemoteRunnable both the classes extends Runnable class

Runnable class has implementation of stream. Now, const chain = RunnableSequence.from([prompt, model]) returns RunnableSequence object. chain.stream would call stream function on Runnable class (parent class) as RunnableSequence class (child class) does not have stream function on it.

Now, stream function in Runnable class calls this._streamIterator . In this context, this is RunnableSequence , Hence it will call _streamIterator of RunnableSequence.

_streamIterator calls object.transform function, which calls transform function of Runnable class. transform function internally calls this._streamIterator again. Now, this is object context which is RemoteRunnable

But, in case of RemoteRunnable object (which is passed as model in above example), does not have implementation of _streamIterator function hence it calls _streamIterator of parent class i.e. Runnable class which internally is yielding invoke (this.invoke(input, options);. Hence, this calls RemoteRunnable invoke function.

Fix

We need to override _streamIterator for RemoteRunnable class, so that it yield stream instead of invoke.

Fixes #4811

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

vercel bot commented Mar 26, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
langchainjs-api-refs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 30, 2024 9:47am
langchainjs-docs ✅ Ready (Inspect) Visit Preview Mar 30, 2024 9:47am

@jacoblee93
Copy link
Collaborator

Thanks for this!

Would be good to add a test confirming the fix?


test("StreamIterator yield stream model output", async () => {
const remote = new RemoteRunnable({ url: `${BASE_URL}/b` });
const streamIterator = await remote._streamIterator({ text: "What are the 5 best apples?" });
Copy link
Collaborator

@jacoblee93 jacoblee93 Mar 26, 2024

Choose a reason for hiding this comment

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

This should test streaming usage in a chain though? That's what the issue was about:

#4811

e.g.

const chain = prompt.pipe(remote).stream();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I would just test _streamIterator function which I added. But, I think what you said make sense. Let me try with chain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!!

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Mar 27, 2024
@jacoblee93 jacoblee93 changed the title Adds streamIterator override method in RemoteRunnable class core[patch]: Fix RemoteRunnable streaming when used in sequences, add tracing Mar 30, 2024
@jacoblee93 jacoblee93 added lgtm PRs that are ready to be merged as-is and removed question Further information is requested labels Mar 30, 2024
@jacoblee93
Copy link
Collaborator

Thanks!

@rahilvora rahilvora deleted the rahilvora/override_stream_iterator branch April 12, 2024 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto:improvement Medium size change to existing code to handle new use-cases lgtm PRs that are ready to be merged as-is size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Streaming does not work if RemoteRunnable is part of RunnableSequence
2 participants