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

Implemented appending arbitrary messages #5293

Merged
merged 6 commits into from
May 29, 2023

Conversation

eavanvalkenburg
Copy link
Contributor

Implemented appending arbitrary messages to the base chat message history, the in-memory and cosmos ones.

As discussed this is the alternative way instead of #4480, with a add_message method added that takes a BaseMessage as input, so that the user can control what is in the base message like kwargs.

Fixes # (issue)

Before submitting

Who can review?

Community members can review the PR once tests pass. Tag maintainers/contributors who might be interested:

@hwchase17

@eyurtsev eyurtsev added the lgtm PR looks good. Use to confirm that a PR is ready for merging. label May 26, 2023
@eyurtsev
Copy link
Collaborator

@hwchase17 -- Proposal seems to add an abstract method to add a custom message which seems reasonable to me -- it looks like a more appropriate method for the base schema than add_ai_message or add_human_message. Waiting for your 👁️

@@ -261,6 +266,10 @@ def add_user_message(self, message: str) -> None:
def add_ai_message(self, message: str) -> None:
"""Add an AI message to the store"""

@abstractmethod
def add_message(self, message: BaseMessage) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems reasonable to me. Looks more appropriate than add_ai_message or add_human_message for the lowest building block of chat history.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

went a step further now, got rid of all implementations of add_xx_message, instead using add_message in each subclass, and then noticed most had another method (usually append) with the same footprint, so got rid of those as well, much cleaner IMHO!

Copy link
Contributor

Choose a reason for hiding this comment

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

i dont think this is super backwards compatible, can we raise NotImplementedError rather than have as abstract? that way if people subclassed it it doesnt break

Copy link
Contributor

@hwchase17 hwchase17 left a comment

Choose a reason for hiding this comment

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

made my suggested change myself - thanks!

@hwchase17 hwchase17 merged commit ccb6238 into langchain-ai:master May 29, 2023
@eavanvalkenburg
Copy link
Contributor Author

Thanks @hwchase17

@eavanvalkenburg eavanvalkenburg deleted the append_message branch May 29, 2023 14:34
@eavanvalkenburg
Copy link
Contributor Author

@amitmukh this has been merged and release and will allow you to override the regular append_message behaviour

vowelparrot pushed a commit that referenced this pull request May 31, 2023
# Implemented appending arbitrary messages to the base chat message
history, the in-memory and cosmos ones.

<!--
Thank you for contributing to LangChain! Your PR will appear in our next
release under the title you set. Please make sure it highlights your
valuable contribution.

Replace this with a description of the change, the issue it fixes (if
applicable), and relevant context. List any dependencies required for
this change.

After you're done, someone will review your PR. They may suggest
improvements. If no one reviews your PR within a few days, feel free to
@-mention the same people again, as notifications can get lost.
-->

As discussed this is the alternative way instead of #4480, with a
add_message method added that takes a BaseMessage as input, so that the
user can control what is in the base message like kwargs.

<!-- Remove if not applicable -->

Fixes # (issue)

## Before submitting

<!-- If you're adding a new integration, include an integration test and
an example notebook showing its use! -->

## Who can review?

Community members can review the PR once tests pass. Tag
maintainers/contributors who might be interested:

@hwchase17

---------

Co-authored-by: Harrison Chase <hw.chase.17@gmail.com>
@danielchalef danielchalef mentioned this pull request Jun 5, 2023
Undertone0809 pushed a commit to Undertone0809/langchain that referenced this pull request Jun 19, 2023
# Implemented appending arbitrary messages to the base chat message
history, the in-memory and cosmos ones.

<!--
Thank you for contributing to LangChain! Your PR will appear in our next
release under the title you set. Please make sure it highlights your
valuable contribution.

Replace this with a description of the change, the issue it fixes (if
applicable), and relevant context. List any dependencies required for
this change.

After you're done, someone will review your PR. They may suggest
improvements. If no one reviews your PR within a few days, feel free to
@-mention the same people again, as notifications can get lost.
-->

As discussed this is the alternative way instead of langchain-ai#4480, with a
add_message method added that takes a BaseMessage as input, so that the
user can control what is in the base message like kwargs.

<!-- Remove if not applicable -->

Fixes # (issue)

## Before submitting

<!-- If you're adding a new integration, include an integration test and
an example notebook showing its use! -->

## Who can review?

Community members can review the PR once tests pass. Tag
maintainers/contributors who might be interested:

@hwchase17

---------

Co-authored-by: Harrison Chase <hw.chase.17@gmail.com>
This was referenced Jun 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm PR looks good. Use to confirm that a PR is ready for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants