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

Add back history and reset subcommand in magics #997

Merged
merged 12 commits into from
Oct 7, 2024

Conversation

akaihola
Copy link
Contributor

@akaihola akaihola commented Sep 16, 2024

  • documentation for %config AiMagics.max_history
  • manual testing for %config AiMagics.max_history
  • manual testing for limiting the maximum number of exchanges to include in the context
  • unit tests for all added features
  • design decision about context for custom chains

#551 removed the history associated uniquely with the openai-chat provider in magic commands. It also removed the "reset" command to delete said history. Docs were updated to remove mention of the history and the reset command.

This PR adds back the history in magic commands. It also adds the %ai reset subcommand to delete said history. A mention of the history and the reset command are added to docs.

The history transcript maintains the distinction between human and AI messages by wrapping the prompts and responses in HumanMessage and AIMessage objects.

The maximum number of Human/AI message exchanges to include before the new prompt can be configured using %config AiMagics.max_history = <n>, defaulting to 2. The whole history is still kept in memory, so raising this option value will start including older messages in subsequent interactions.

For non-chat providers, human messages are wrapped in pseudo XML <HUMAN>...</HUMAN> tags prepended with AI: or Human: unless there is nothing but the first prompt. This is yet untested.

@dlqqq dlqqq added the enhancement New feature or request label Sep 16, 2024
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.

Thank you for opening this PR and for re-implementing message history in the magics! I've left suggestions about 1) bounding the chat history to 2 exchanges at most, and 2) avoiding the pseudo-XML syntax being used for non-chat providers. This is a good stopgap solution for users who want to use history in AI magics as soon as possible.

There are better ways to pass message history in LangChain however. In the future, we will definitely want to rework this logic to use the new LCEL syntax and use the RunnableWithMessageHistory class from langchain_core.runnables.history; see #392.

@dlqqq
Copy link
Member

dlqqq commented Sep 25, 2024

Thank you for your contribution! It's been a week since we've last heard from you, so I'm closing this PR due to inactivity. Please feel free to re-open this PR after addressing the review feedback.

@dlqqq dlqqq closed this Sep 25, 2024
@akaihola
Copy link
Contributor Author

Thank you for your contribution! It's been a week since we've last heard from you, so I'm closing this PR due to inactivity. Please feel free to re-open this PR after addressing the review feedback.

I don't seem to have permission to re-open. Could you do that?

@krassowski
Copy link
Member

Yeah, that's the problem with closing PRs on GitHub.

@akaihola
Copy link
Contributor Author

akaihola commented Oct 1, 2024

Update: I solved the below question myself, see next message.


May I ask for advice @dlqqq and @krassowski?

What might be the reason %config doesn't work for max_history?

image

@akaihola
Copy link
Contributor Author

akaihola commented Oct 1, 2024

I asked:

What might be the reason %config doesn't work for max_history?

Oh, I seem to have forgotten to add

    config=True,

to the traitlet!

@akaihola
Copy link
Contributor Author

akaihola commented Oct 1, 2024

Based on my manual testing, max_history works as expected when using openrouter:openai/gpt-4o as the model. If I prompt the model for my very first question, verbatim, it gives me the question max_history exchanges back.

Interestingly, if I use openrouter:anthropic/claude-3.5-sonnet:beta instead, the model remembers everything! So is there a history layer on OpenRouter side?!?

@akaihola
Copy link
Contributor Author

akaihola commented Oct 1, 2024

I fixed the behavior when max_history == 0, and added unit tests.

@akaihola
Copy link
Contributor Author

akaihola commented Oct 1, 2024

@krassowski & @dlqqq would it make sense to include message history in the context also when invoking custom chains? I haven't touched that part, so it still only sends the prompt entered in the cell:

        if args.model_id in self.custom_model_registry and isinstance(
            self.custom_model_registry[args.model_id], LLMChain
        ):
            # Get the output, either as raw text or as the contents of the 'text' key of a dict
            invoke_output = self.custom_model_registry[args.model_id].invoke(prompt)

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.

@akaihola Awesome work! Thank you for including unit test coverage as well. I've left some additional minor feedback below. Once that's addressed, this is ready to be merged. 👍

packages/jupyter-ai-magics/jupyter_ai_magics/magics.py Outdated Show resolved Hide resolved
packages/jupyter-ai-magics/jupyter_ai_magics/magics.py Outdated Show resolved Hide resolved
@akaihola
Copy link
Contributor Author

akaihola commented Oct 5, 2024

@dlqqq I believe I've now addressed your fine review feedback!

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.

@akaihola Awesome work, thank you! 🎉

@dlqqq dlqqq merged commit b15ebb5 into jupyterlab:main Oct 7, 2024
9 checks passed
@akaihola akaihola deleted the transcript branch October 8, 2024 14:51
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.

3 participants