-
Notifications
You must be signed in to change notification settings - Fork 115
feat(Chat): Add .message_stream_context()
#1906
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
Conversation
…inject_message_chunk(). Append instead of replace messages unless transforms are used
.inject_message_chunk(), .start_message_stream(), and .end_message_stream().append_message_chunk(), .start_message_stream(), and .end_message_stream()
5a3d451 to
08c104a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
4b9cbc6 to
24f9f3a
Compare
24f9f3a to
28f148f
Compare
.append_message_chunk(), .start_message_stream(), and .end_message_stream().append_message_chunk() and .message_stream()
1bd16ec to
3786b79
Compare
|
I don't love What if we solve both problems, by replacing: with chat.message_stream():
chat.append_message_chunk("Foo")
chat.append_message_chunk("Bar")
chat.append_message_chunk("Baz", operation="replace")with: with chat.message_checkpoint() as cp:
chat.append_message_chunk("Foo")
chat.append_message_chunk("Bar")
cp.restore()
chat.append_message_chunk("Baz")We wouldn't even need the cp = chat.message_checkpoint()
chat.append_message_chunk("Foo")
chat.append_message_chunk("Bar")
cp.restore()
chat.append_message_chunk("Baz")In theory maybe it's slightly less efficient because the restoring and the sending of "Baz" happens as two separate messages instead of one, but we're talking about a scenario where each token is generally being streamed as a separate message already, so who cares. The other downside (if it's even a downside, arguably it's a good thing) is that in order to restore to any checkpoint, you need to actually have the instance of that checkpoint. As opposed to the current approach, where you can only restore to the innermost checkpoint, but you can do it from anywhere without anything but the |
|
One thing to consider is that the current implementation of @chat.on_user_submit
async def _(user_input):
stream = await chat_model.stream_async(user_input)
with chat.message_stream():
for chunk in stream:
chat.append_message_chunk(chunk.upper())So, given that we like that behavior, and it's worth keeping, I'm not in love with That said, I like the |
.append_message_chunk() and .message_stream().append_message_context()
7276bbd to
5e822fc
Compare
78f527c to
3c0ca35
Compare
|
@jcheng5 I'm now leaning towards Also tagging in @gadenbuie to see what he thinks. |
|
I really liked I do not like I think |
.append_message_context().message_stream_context()
a7988f1 to
7ac2ba1
Compare
shiny/ui/_chat.py
Outdated
| await progress.append(f" {x}%") | ||
| await asyncio.sleep(1) | ||
| await progress.restore() | ||
| await msg.restore() |
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.
If you're not going to use the term "checkpoint" then I think this should be msg.clear(). What do you think?
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.
Yea, that is better, thanks 👍
e1c7431 to
773e4f1
Compare
773e4f1 to
f46aaf8
Compare
|
After playing with this with some real world examples, I feel pretty strongly that we shouldn't have a Will be push an update soon |
This PR adds a
.message_stream_context()method toChat, a new more composable and configurable way to append streaming messages to the chat. This context manager can be used in isolation, and also nested within itself, which is primarily useful making checkpoints in the message stream to.clear()back to:Basic example app.py
Kapture.2025-03-17.at.11.18.33.mp4
A big motivator for adding
.message_stream_context()is to have a mechanism to display content during tool calls. In this context, you likely have a long-running.append_message_stream()handling the response generation, but need to also display other UI in the message during a tool call. Here's an mock example of what that might look like:Tool call app.py
Kapture.2025-03-17.at.11.28.19.mp4
This PR also comes with a performance bonus that streaming messages no longer send the entire accumulated content string on every update (that will still be necessary, however, when
@transform_assistant_responseis used). I figured it was addressing in this PR, since it took some surgery to get._append_message_chunk()correctly reflected in the server-side message state to reflected.