-
Notifications
You must be signed in to change notification settings - Fork 114
Return ContentThinking objects from stream_text()
#909
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
base: main
Are you sure you want to change the base?
Conversation
R/chat.R
Outdated
| "ellmer::ContentThinking" = content@thinking, | ||
| "ellmer::ContentText" = content@text, | ||
| if (S7_inherits(content, Content)) format(content) else content | ||
| ) |
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.
I'm not sure if using switch on S7 objects is considered bad practice, but the control flow was gnarly otherwise😅
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.
Maybe we should have a stream_data() that returns the object and then stream_text() would contain this code to extract out just the text?
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.
Or stream_content()?
| ) | ||
|
|
||
| stream_content <- function(provider, event) { | ||
| result <- stream_text(provider, event) |
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.
This feels inside out to me? Why doesn't stream_text() call stream_content()?
Addresses #837, and a subset of #828. Companion to posit-dev/shinychat#167.
Changes
stream_text()to returnContentobjects instead of strings, allowing downstream consumers to distinguish between content types during streaming. With the companion shinychat PR, results look like this:ellmer-shinychat-thinking.mp4
I'm not sure if this is the opposite direction of the dev dependency that yall would usually introduce. With
mainshinychat and this PR, each thinking delta would otherwise get its own collapsible:So, I opted to make ellmer depend on the shinychat Remotes. If this is a no-go, we could add some version-dependent logic, but felt cleaner to start with this approach.
Here's the reprex I use in the video: