Repository navigation
Add new_chat for conversation history - #399
Conversation
gadenbuie
changed the title
| if history_controller is not None: | ||
| raise ValueError( | ||
| "Can't clear a chat with conversation history enabled; use " | ||
| "`await chat.client.new_chat()` to start a new conversation." | ||
| ) |
There was a problem hiding this comment.
I still think the concept of changing messages (& turns) for the currently active conversation is going to be useful (e.g., compacting a conversation).
This is also why (privately, via Slack) I was advocating for deprecating .clear() in favor of a new .set_messages() method, which would basically be the same as the current .clear(), but assumes you are going to provide messages
There was a problem hiding this comment.
Are you okay moving forward without working this out yet? I'm on board with the general idea but I think we'll need to think about it a little more carefully and make sure we're getting the API right in tandem with ellmer/chatlas APIs.
One small detail, I have generally aligned on messages being UI chat state and turns being client chat state. And here on first read compaction seems to be more about changing client state independently from UI state.
Anyway, we can work this out but I do want to find the minimal immediate fix for anyone using the history with chat_server().
There was a problem hiding this comment.
For posterity I mentioned on Slack that set_messages() makes sense to me if we think we can get it together quickly
I was advocating for deprecating
.clear()in favor of a new.set_messages()method
I guess my last comment is just that if we're thinking of deprecating .clear(), I'd like to consider leaving it as an alias for .new_chat() and deprecating internal arguments. That would give most general-case users a chance to keep using .clear() without breaking their apps and we could the more advanced usecase users migrate in the right direction.
There was a problem hiding this comment.
Are you okay moving forward without working this out yet? I'm on board with the general idea but I think we'll need to think about it a little more carefully and make sure we're getting the API right in tandem with ellmer/chatlas APIs.
Yep, and the more I looked into this, the more I'm happy with the PR as it currently stands (i.e.., hard error when history is enabled, but no deprecation, at least for now).
I think .set_messages() is probably worth doing, but I also think it should probably come after #379, especially if we want it to automatically save to history before mutation of the current conversation.
Chat.history is always set in __init__, so the double getattr fallback in ChatClient.clear()/new_chat() was unreachable. Addresses review feedback on #399.
shinychat's clear() now aborts when conversation history is enabled (the only configuration commons_server() creates), which broke this test. Drop the clear() assertion since new_chat() covers the same commons behavior, and replace controller$new_chat() with the public chat$new_chat() added in posit-dev/shinychat#399. Closes #324
2 participants
Fixes #397
Summary
new_chat()to the managed R and Python chat handles. It saves the active conversation, clears the rendered messages and client turns, resets the active conversation ID, and refreshes the history drawer.clear()as the lower-level UI/client-turn reset. It errors when conversation history is enabled and directs callers tonew_chat().new_chat()while a response streams in R and Python, so a late response cannot enter the new conversation.Verification
make r-checkmake py-check