Skip to content
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

Merged
merged 20 commits into from
Aug 27, 2024

Conversation

michaelchia
Copy link
Collaborator

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

  • create a new global memory object 'llm_chat_history' which mirrors the 'chat_history' object
    • Move the BoundedChatHistory initiation to AiExtension.initialize_settings
    • Include it in 'self.settings' and as an attribute of BaseChatHandler
  • include traitlet 'default_max_chat_history' in AiExtension
  • add a line to ClearChatHandler to clear the chat memory.
  • changed BoundedChatHistory to not pop the internal memory object to fit size k but instead only return the last k*2 messages.
    • not directly related to features in this PR but will enable future features. Those involving dynamic configuration of history size and partial clearing (see Extensions)

Extensions

  • Reimplement AskChatHandler to use this llm_chat_history object.
    • Involves changing chain from ConversationBufferWindowMemory to RunnableWithMessageHistory.
    • ConversationBufferWindowMemory is deprecated and will need to be changed eventually anyway
    • I've left this for the team to implement as would involve some new prompt templates
  • FixChatHandler to add to llm_chat_history object.
    • May not be useful for /fix to use the chat history but may be useful to add to it.
    • This will allow user to ask follow up questions.
    • Some decisions need to be made regarding what to add as the human message.
    • Adding all the context may bloat the chat memory so a simple replacement to "Fix the errors in my notebook cell" + extra_instructions may suffice.
  • UI-based configuration of chat memory size by modifying llm_chat_history.k
  • Partial clearing on memory (reset to earlier chat state)
    • X button on user messages to remove all messages after
    • Allows users to reset to a previous chat history state
    • FYI, this is actually the feature that I want to implement and this PR is just a pre-requisite for that.

Possible amendments

Let me know if you would like me to rename:

  • memory object 'llm_chat_history'
  • traitlet 'default_max_chat_history' or the help description.
    • it is "default" to align with additional UI-based configuration in the future.

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.

@krassowski krassowski added the enhancement New feature or request label Aug 9, 2024
@michaelchia michaelchia force-pushed the global-memory-obj branch 2 times, most recently from 3afe37c to d925acf Compare August 12, 2024 13:45
@srdas
Copy link
Collaborator

srdas commented Aug 12, 2024

@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 Additional chat commands in docs/source/users/index.md to explain how the change impacts both chat and memory histories, and how users should set history length k.
image

docs/source/users/index.md Outdated Show resolved Hide resolved
@srdas
Copy link
Collaborator

srdas commented Aug 15, 2024

Note: Need to check if PR #938 which also moves the /clear handling to base.py has been taken into account in this PR.

@michaelchia
Copy link
Collaborator Author

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.

@jtpio jtpio mentioned this pull request Aug 17, 2024
11 tasks
@srdas
Copy link
Collaborator

srdas commented Aug 17, 2024

@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.

Copy link
Member

@dlqqq dlqqq left a 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.

packages/jupyter-ai/jupyter_ai/history.py Outdated Show resolved Hide resolved
Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelchia Awesome work! 🎉

@dlqqq dlqqq merged commit 41ccd10 into jupyterlab:main Aug 27, 2024
8 checks passed
@michaelchia michaelchia deleted the global-memory-obj branch September 4, 2024 05:49
Marchlak pushed a commit to Marchlak/jupyter-ai that referenced this pull request Oct 28, 2024
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New command to clear chat message history
4 participants