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

feat: Add Zep Cloud Memory component #9657

Merged
merged 10 commits into from
Jul 1, 2024

Conversation

paul-paliychuk
Copy link
Contributor

Summary

Describe what the PR does and how to test. Photos and videos are recommended.
We added a Zep Cloud Memory component that integrates with our Zep Cloud Memory Langchain class.

Example workflow CleanShot 2024-06-06 at 18 15 14@2x

Related tickets and issues

Review / Merge checklist

  • PR title and summary are descriptive. Remember, the title automatically goes into the changelog. Use (no-changelog) otherwise. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.

    A bug is not considered fixed, unless a test is added to prevent it from happening again.
    A feature is not complete without tests.

@CLAassistant
Copy link

CLAassistant commented Jun 6, 2024

CLA assistant check
All committers have signed the CLA.

@n8n-assistant n8n-assistant bot added community Authored by a community member in linear Issue or PR has been created in Linear for internal review labels Jun 6, 2024
@Joffcom
Copy link
Member

Joffcom commented Jun 6, 2024

Hey @paul-paliychuk,

Thanks for the PR, While we are not generally accepting new node PRs from the community this looks like it doesn't need to be a new node.

Are you able to update the existing credential to have an option for "Cloud" that defaults to open source so we don't have a breaking change then update the existing node to use either the url from the credential or the Cloud url.

This will then remove the need for a new node and make this a lot smaller, it looks like the new node is also trying to get the apiUrl from the new credential which doesn't exist as a field and there is no authenticate or test function in the credential... but this wouldn't be an issue if the existing node and credential was modified.

Let me know if you have any questions on this.

@paul-paliychuk
Copy link
Contributor Author

@Joffcom Hey, makes sense thanks, will update the PR. We don't require an api url for the cloud sdk, so I will make it optional.

Noted about nodes contributions, we've had quite a lot of community requests for n8n support recently :)

@paul-paliychuk
Copy link
Contributor Author

@Joffcom removed previously added zep cloud memory node, zep cloud api credential and added cloud support to the original node.

I made apiUrl optional as well as it's built into the sdk, however facing some issues with credential testing. Wondering if there's nice way to bypass those tests if cloud is used (as we have different api routes on cloud)
CleanShot 2024-06-06 at 23 54 35@2x

@Joffcom
Copy link
Member

Joffcom commented Jun 7, 2024

Hey @paul-paliychuk,

If you were to have a toggle or dropdown in the node to select between self hosted or cloud you could use that in the credential test to set the test url to the cloud endpoint and hide the url field.

@paul-paliychuk
Copy link
Contributor Author

@Joffcom Makes sense, just updated the credential. I also realized that now that cloud control inside of memory node is somewhat redundant as we can get that info from the credential object lmk if that's ok

CleanShot.2024-06-07.at.11.30.01.mp4

@Joffcom
Copy link
Member

Joffcom commented Jun 7, 2024

Hey @paul-paliychuk,

That looks pretty good although I would probably have the toggle under the API key so we would have API Key, Cloud, URL.

The only bit I am not sure about is the extra packages but that I will pass this to @OlegIvaniv our main man for the AI nodes 😄

@paul-paliychuk
Copy link
Contributor Author

@Joffcom That makes sense to me, rearranged the order of credential fields. Thanks!

@paul-paliychuk
Copy link
Contributor Author

@Joffcom @OlegIvaniv Hey guys, any update on this PR? Thanks!

@OlegIvaniv
Copy link
Contributor

@paul-paliychuk Appreciate the effort! I've ran into some issue when testing the cloud version though. It seems like the messages are merged together so it would always return just a single HumanMessage. This is problematic because when we pre-load messages we need to populate both AI and Human messages.
CleanShot 2024-06-13 at 13 59 24@2x
The Zep OS version works as expected

@paul-paliychuk
Copy link
Contributor Author

@OlegIvaniv Hi, thanks for reviewing this. The message merging is intentional. The reason why the memory (facts, summary) along with the messages comes as a Human message is that we've faced limitations with some model providers unable to parse more than one system message and being restrictive about the messages order too. In particular, we've had issues with newer anthropic models and gemini. We wanted to make the implementation more flexible so that the user could provide their own system message if they needed to.

Could you please elaborate on how this breaks the preloading of messages?

@OlegIvaniv
Copy link
Contributor

@OlegIvaniv Hi, thanks for reviewing this. The message merging is intentional. The reason why the memory (facts, summary) along with the messages comes as a Human message is that we've faced limitations with some model providers unable to parse more than one system message and being restrictive about the messages order too. In particular, we've had issues with newer anthropic models and gemini. We wanted to make the implementation more flexible so that the user could provide their own system message if they needed to.

Could you please elaborate on how this breaks the preloading of messages?

I see, it would be nice for this behavior to be configurable. Currently it breaks pre-loading for both debug and public chat and also "Get Many" and "Delete Messages" operations in Chat Memory Manager node. These expect a Langchain's standard single message format. Returning them as a single message would result in a message like this when we load messages from history:

CleanShot 2024-06-17 at 10 15 11@2x

Here's a simple workflow to demonstrate that:
My_workflow_95.json

@paul-paliychuk
Copy link
Contributor Author

@OlegIvaniv Got it makes sense. I will add a configurable option to our ZepCloudChatMessageHistory class to return all messages. The Zep Memory Context (facts and summary) will be put inside of a SystemMessage in this case, and the message history will be returned as Human and AI messages. Will keep you posted

@paul-paliychuk
Copy link
Contributor Author

paul-paliychuk commented Jun 24, 2024

@OlegIvaniv Just updated langchain community version to include a recently added separateMessages option to zep memory and chat history. Lmk if that works for you.

I also had to bump langchain-core version to 0.2.9, otherwise the build was failing.

@OlegIvaniv
Copy link
Contributor

OlegIvaniv commented Jun 28, 2024

@OlegIvaniv Just updated langchain community version to include a recently added separateMessages option to zep memory and chat history. Lmk if that works for you.

I also had to bump langchain-core version to 0.2.9, otherwise the build was failing.

There's still something funky going on when pre-loading the messages:

  1. The messages are duplicated when using Zep Cloud version
  2. It seems like the summary is inserted as system message, which breaks Anthropic Chat model(and possibly others) because it allows only for the first message to be the system message.

Video for better demonstration:

CleanShot.2024-06-28.at.12.04.23.mp4

@paul-paliychuk
Copy link
Contributor Author

@OlegIvaniv Hi, thanks for reviewing the PR one more time.

It turned out that both issues (preloading of messages and duplication) have been fixed by removing the assignment of ZepCloudChatMessageHistory to ZepCloudMemory class instance. Also confirmed that it works with anthropic model.

While testing this I also found another issue with anthropic model, when a new session is started, it would output an internal error because an \n content message would be passed which anthropic errors out on. I have set up a simple ZepCloudMemory extension to fix this, we will include this in one of our next langchain repo updates.

Also confirmed that it works well for me on the flow with preloaded messages that you attached previously (with wikipedia agent tool) (both anthropic and openai)

CleanShot.2024-06-28.at.16.25.04.mp4
CleanShot.2024-06-28.at.16.26.28.mp4

@paul-paliychuk
Copy link
Contributor Author

Also - answering your question previously, we are aware of the anthropic limitation (we've encountered the same on with gemini too), that's why we opted to merge messages. Was able to confirm that it doesn't break the pre loading of messages.

Copy link
Contributor

@OlegIvaniv OlegIvaniv left a comment

Choose a reason for hiding this comment

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

@paul-paliychuk Thank you so much for fixing these two issues! Seems to be working as expected now :-)

@OlegIvaniv OlegIvaniv merged commit 41c47a2 into n8n-io:master Jul 1, 2024
8 of 9 checks passed
@github-actions github-actions bot mentioned this pull request Jul 3, 2024
@janober
Copy link
Member

janober commented Jul 3, 2024

Got released with n8n@1.49.0

adrian-martinez-onestic pushed a commit to onesdata/n8n-fork that referenced this pull request Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Authored by a community member in linear Issue or PR has been created in Linear for internal review Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants