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 Qdrant as a long memory backend. #1515

Closed
wants to merge 4 commits into from

Conversation

wangxuqi
Copy link
Contributor

@wangxuqi wangxuqi commented Apr 15, 2023

Hi there!
For this pull request, I have read your announcement carefully.

Background

As I notice there are many vector databases provided in chatgpt-retrieve-plugin.
I think all the vector databases can be used by you as a long-term memory. So this is the first commit to add the most easier used one 'Qdrant'. And I am willing to implement them all!

Changes

This commit only "Add Qdrant as a long memory backend."
what I do is:

  • Implement QdrantMemory based on MemoryProviderSingleton
  • add 'qdrant' option in "memory/init.py"
  • add 'qdrant-client' in requirements.txt
  • add unit test and integration test in 'qdrant_memory_test.py' and 'qdrant_memory_tests.py'

Documentation

The implement is very clear in-code comments. I just implement all your API and test all them to work.

  • def add(self, data)
  • def get(self, data)
  • def clear(self)
  • def get_relevant(self, data, num_relevant=5)
  • def get_stats(self)

Test Plan

  • Unit test in 'qdrant_memory_test.py'
  • IntegrationTest in 'qdrant_memory_tests.py'
  • Test step: Lauch a Qdrant docker instance docker run -p "6333:6333" -p "6334:6334" qdrant/qdrant:v1.0.3 and then just run all the related cases.
  • I've also tested with the following instructions:
Enter up to 5 goals for your AI:  For example: Increase net worth, Grow Twitter Account, Develop and manage multiple businesses autonomously'
Enter nothing to load defaults, enter nothing when finished.
Goal 1: Visit a website which provides today's news.
Goal 2: Summarize the articles and write the results to text files
Goal 3: Terminate
Goal 4: 
Using memory of type: QdrantMemory
SUMMARIZE TODAY'S LEADING NEWS THOUGHTS:  I need to determine which news website to visit and then summarize 
...

PR Quality Checklist

  • My pull request is atomic and focuses on a single change.
  • I have thoroughly tested my changes with multiple different prompts.
  • I have considered potential risks and mitigations for my changes.
  • I have documented my changes clearly and comprehensively.
  • I have not snuck in any "extra" small tweaks changes

nponeccop
nponeccop previously approved these changes Apr 15, 2023
@nponeccop
Copy link
Contributor

@wangxuqi There are conflicts now

@wangxuqi
Copy link
Contributor Author

wangxuqi commented Apr 15, 2023

@wangxuqi There are conflicts now

The conflict is caused by another long memory 'Milvus' merged. I have resolved the conflict.

@wangxuqi
Copy link
Contributor Author

@nponeccop As I solve the conflict , I find the merged 'Milvus memory' code is very poor. With no tests and even not worked. Also the package import is wrong and the milvs.py is not under the correct directory.
Please review the pull request again #801 or I will fixed it later.

nponeccop
nponeccop previously approved these changes Apr 15, 2023
@richbeales richbeales added the needs discussion To be discussed among maintainers label Apr 15, 2023
@nponeccop nponeccop mentioned this pull request Apr 15, 2023
1 task
@richbeales richbeales added the potential plugin This may fit better into our plugin system. label Apr 15, 2023
@wangxuqi
Copy link
Contributor Author

Fix conflict with another merged PR 1658.
As I see you have add a potential plugin label. I think it maybe a better way to fit all chatgpt-retrieve-plugin. vector databases. Try to do this later.

README.md Outdated
@@ -342,19 +343,6 @@ export PINECONE_ENV="<YOUR_PINECONE_REGION>" # e.g: "us-east4-gcp"
export MEMORY_BACKEND="pinecone"
```

### Milvus Setup
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't remove milvus

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Milvus Setup is duplicated. You can see line 367 to 377

nponeccop
nponeccop previously approved these changes Apr 16, 2023
@wangxuqi
Copy link
Contributor Author

Ops! Conflict with README.md again. resolve it. Hope this time no more conflicts.

@wangxuqi wangxuqi force-pushed the qdrant_memory branch 2 times, most recently from 2f1c605 to 4d84b25 Compare April 17, 2023 02:34
@wangxuqi
Copy link
Contributor Author

CI broken fixed by #2061

@nponeccop nponeccop mentioned this pull request Apr 17, 2023
1 task
@wangxuqi wangxuqi requested a review from nponeccop April 18, 2023 11:12
@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Apr 18, 2023
@github-actions
Copy link
Contributor

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Apr 19, 2023
@github-actions
Copy link
Contributor

Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly.

@github-actions
Copy link
Contributor

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Apr 22, 2023
@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Apr 22, 2023
@github-actions
Copy link
Contributor

Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly.

@codecov
Copy link

codecov bot commented Apr 22, 2023

Codecov Report

Patch coverage: 14.06% and project coverage change: -0.46 ⚠️

Comparison is base (8e8a5a1) 36.22% compared to head (0457470) 35.77%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1515      +/-   ##
==========================================
- Coverage   36.22%   35.77%   -0.46%     
==========================================
  Files          60       61       +1     
  Lines        2849     2913      +64     
  Branches      471      477       +6     
==========================================
+ Hits         1032     1042      +10     
- Misses       1755     1808      +53     
- Partials       62       63       +1     
Impacted Files Coverage Δ
autogpt/memory/qdrant.py 4.00% <4.00%> (ø)
autogpt/memory/__init__.py 43.07% <41.66%> (-0.32%) ⬇️
autogpt/config/config.py 75.00% <100.00%> (+0.35%) ⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Apr 26, 2023
@github-actions
Copy link
Contributor

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

@p-i-
Copy link
Contributor

p-i- commented May 5, 2023

This is a mass message from the AutoGPT core team.
Our apologies for the ongoing delay in processing PRs.
This is because we are re-architecting the AutoGPT core!

For more details (and for infor on joining our Discord), please refer to:
https://github.com/Significant-Gravitas/Auto-GPT/wiki/Architecting

@ntindle
Copy link
Member

ntindle commented May 25, 2023

Hey, I've marked this as don't merge until the Memory Fixes are in. Sorry to keep it on hold longer, just no real way to test/validate functionality until the fixes are in

@wangxuqi
Copy link
Contributor Author

Hey, I've marked this as don't merge until the Memory Fixes are in. Sorry to keep it on hold longer, just no real way to test/validate functionality until the fixes are in

As I see the discussion #4280. So close the pr for now. Thanks for your attention and reply. Wish you all good.

@wangxuqi wangxuqi closed this May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflicts Automatically applied to PRs with merge conflicts needs discussion To be discussed among maintainers potential plugin This may fit better into our plugin system. size/xl
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants