-
-
Notifications
You must be signed in to change notification settings - Fork 341
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
Make chat memory size traitlet configurable + /clear to reset memory #943
Conversation
3afe37c
to
d925acf
Compare
@michaelchia Thanks for submitting this PR. Note that PR #842 was closed and not merged given that several modifications had been made to the base chat handler in the interim and this PR was going to be reworked from scratch as a new PR. You may assume PR 842 was not implemented, so this PR 943 supersedes 842. As you note, Issues #616 and #817 are related and it is good to handle them in one PR here. Update the documentation in the section |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
4870dbc
to
64d5e6f
Compare
Note: Need to check if PR #938 which also moves the |
I don't see any conflict as the chat_memory clearing remained within ClearChatHandler in that PR. It's just the help message building and sending gets moved, which this PR doesn't touch. However, I wouldn't mind waiting for that to be approved first and merging its changes. |
@michaelchia - note that PR #938 is now merged. @dlqqq is away next week also, we may wait to make sure he does not have any further suggestions on this PR -- thanks for your detailed work and patience. |
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.
@michaelchia Great work on this! This has been a long-outstanding issue that I'm really excited to see addressed.
The code looks excellent, and I was not able to find any issues with the implementation after local testing. Left one minor comment below.
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.
@michaelchia Awesome work! 🎉
…upyterlab#943) * make chat memory a shared object * store all messages internally * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix test * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix chat history class * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * prevent add history after clear * docs * clear pending and do not show replies to cleared msg * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * make all_messages private * rename to llm_chat_memory * fix docs * update docs * rename human_msg param in history to last_human_msg --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Description
Makes chat memory size configurable via
default_max_chat_history
traitlet. Also makes /clear reset memory.The reason why these unrelated features are in same PR stems from the same core change of making the default chat handler's
BoundedChatHistory
object globally accessible.Fixes #616 and #817 (partially. fully with enhancement to /ask mentioned below)
Implementation summary
BoundedChatHistory
initiation to AiExtension.initialize_settingsBaseChatHandler
BoundedChatHistory
to not pop the internal memory object to fit sizek
but instead only return the lastk*2
messages.Extensions
llm_chat_history
object.ConversationBufferWindowMemory
toRunnableWithMessageHistory
.ConversationBufferWindowMemory
is deprecated and will need to be changed eventually anywayllm_chat_history
object.extra_instructions
may suffice.llm_chat_history.k
Possible amendments
Let me know if you would like me to rename:
Although #616 argues for making clearing of chat memory a separate command, #842 seems to imply that has changed. Also, I found that most users assumed that /clear resets the memory perhaps because the /clear command in github copilot behaves that way. Let me know if you would like me to remove this from the PR.
I realise this is introducing multiple features and that this is all in a single PR. I mostly wanted to add the globally accessible memory but the rest were just natural and tiny additions. Let me know if you would like me to split it into 2 PRs instead (1. global memory + config, 2. /clear to reset memory)
Let me know what you guys think and if there is anything you would like me to add.