-
Notifications
You must be signed in to change notification settings - Fork 677
feat: accept chunks as arguments to chat stream helper #2470
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
feat: accept chunks as arguments to chat stream helper #2470
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat-ai-apps-thinking-steps #2470 +/- ##
==============================================================
Coverage ? 93.07%
==============================================================
Files ? 40
Lines ? 11213
Branches ? 708
==============================================================
Hits ? 10437
Misses ? 764
Partials ? 12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
mwbrooks
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.
e797040 to
5e86b29
Compare
packages/web-api/src/chat-stream.ts
Outdated
| private async flushBuffer( | ||
| args: Omit<ChatStartStreamArguments | ChatAppendStreamArguments, 'channel' | 'ts'>, | ||
| ): Promise<ChatStartStreamResponse | ChatAppendStreamResponse> { | ||
| const finalArgs = this.buffer.length > 0 && !args.chunks ? { ...args, markdown_text: this.buffer } : args; |
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.
👀 note: We might want to add existing buffer to a new chunk with just markdown text, if the buffer has anything, then add the chunks argument. This would help make sure we're not appending while text is in the buffer!
mwbrooks
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.
packages/web-api/src/chat-stream.ts
Outdated
| if (this.buffer.length >= this.options.buffer_size) { | ||
| if (args?.markdown_text) { | ||
| this.buffer += args.markdown_text; | ||
| args.markdown_text = undefined; |
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.
note: I pushed commit fc5ed1a to unset the args.markdown_text after adding it to the buffer.
If this is not done, then the markdown_text will be added to the API request, which is also included in the chunks: [{ type: "markdown_text", "text": "...." }].
@zimeg Does this look correct to you? I noticed this wasn't explicitly done in the Python SDK.
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.
@mwbrooks A change similar to other comments might be useful to destructure markdown_text and chunks from the rest of args before a chance to flush the buffer?
const { markdown_text, chunks, ...opts } = args;IIRC the arguments alongside **kwargs in the Python implementation do this without added lines!
packages/web-api/src/chat-stream.ts
Outdated
| channel: this.streamArgs.channel, | ||
| ts: this.streamTs, | ||
| chunks: flushings, | ||
| ...args, |
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.
@mwbrooks Thanks so much for catching this! We might want to consider a "destructured" approach to separate markdown_text and also chunks from the remaining options:
const { markdown_text, chunks, ...opts } = args;This might help us avoid overriding new chunks with ...args as well, or chunks can be a following parameter?
| .post('/api/chat.stopStream', { | ||
| channel: 'C0123456789', | ||
| ts: '123.123', | ||
| chunks: JSON.stringify([{ type: 'markdown_text', text: 'nice!' }]), |
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.
note: I made commit fc5ed1a which updates this test to support chunks instead of markdown_text because the streaming helper will now convert all markdown text into a chunk.
zimeg
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.
📝 A quick note on separating markdown_text and chunks from extra options since we might be adding from the buffer to chunks before making an API call!
👁️🗨️ Appreciate these catches different from python nuance @mwbrooks!
packages/web-api/src/chat-stream.ts
Outdated
| channel: this.streamArgs.channel, | ||
| ts: this.streamTs, | ||
| chunks: flushings, | ||
| ...args, |
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.
@mwbrooks Thanks so much for catching this! We might want to consider a "destructured" approach to separate markdown_text and also chunks from the remaining options:
const { markdown_text, chunks, ...opts } = args;This might help us avoid overriding new chunks with ...args as well, or chunks can be a following parameter?
packages/web-api/src/chat-stream.ts
Outdated
| if (this.buffer.length >= this.options.buffer_size) { | ||
| if (args?.markdown_text) { | ||
| this.buffer += args.markdown_text; | ||
| args.markdown_text = undefined; |
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.
@mwbrooks A change similar to other comments might be useful to destructure markdown_text and chunks from the rest of args before a chance to flush the buffer?
const { markdown_text, chunks, ...opts } = args;IIRC the arguments alongside **kwargs in the Python implementation do this without added lines!
zimeg
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.
@srtaalej LGTM! Thanks for bringing chunks to the streamer! I left a quibble on internals but testing works well for me 🤓
packages/web-api/src/chat-stream.ts
Outdated
| this.state = 'in_progress'; | ||
| } | ||
|
|
||
| const flushings: AnyChunk[] = []; |
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.
🙊 quibble: This variable was changed alike to "chunks to flush" in updates to the Python SDK that we might mirror here? Also unrelated but empty lines might be nice to remove before merging-
mwbrooks
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.
🧪 Manually testing with a modified sample app works well for chunks as markdown_text, task_update, and plan_update. 🎉 👏🏻
8d237be
into
feat-ai-apps-thinking-steps
Co-authored-by: Michael Brooks <[email protected]> Co-authored-by: Eden Zimbelman <[email protected]>
Summary
This PR enables chunks processing in the chat stream helper
Requirements (place an
xin each[ ])