-
Notifications
You must be signed in to change notification settings - Fork 434
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 Weaviate memory adapter #95
base: main
Are you sure you want to change the base?
Conversation
Hi @zainhas! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few important comments inline. But overall:
- let's figure out a way to test this end to end
- also, could you please install pre-commit and lint / format the code
llama_stack/apis/memory/client.py
Outdated
@@ -42,7 +42,11 @@ async def get_memory_bank(self, bank_id: str) -> Optional[MemoryBank]: | |||
params={ | |||
"bank_id": bank_id, | |||
}, | |||
headers={"Content-Type": "application/json"}, | |||
headers={ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we revert these changes from here, since this is a pretty generic client -- we will figure out how to get a bunch of these examples situated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
async def get_adapter_impl(config: RemoteProviderConfig, _deps): | ||
from .weaviate import WeaviateMemoryAdapter | ||
|
||
impl = WeaviateMemoryAdapter(config.url, config.username, config.password) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the config does not define url / username / password but instead only defines a collection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the cluster url and api_key for weaviate will come in via the WeaviateRequestProviderData class, in this case should I just get rid of the WeaviateConfig class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
async def initialize(self) -> None: | ||
try: | ||
|
||
request_provider_data = get_request_provider_data() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
provider data isn't present at this point -- it is only provided at the time of the request. you should initialize the client on every client call and if we need a cache of clients then, we'd need to build that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified the method to return an initialized client. following this I initialize the client on every client call using the method.
@ashwinb addressed comments. Please let me know how we want to test this end to end. I can provide a weaviate cloud url and api_key to you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also curious about tests, possibly using vcr might work so that folks don't need a real endpoint?
|
||
data_objects = [] | ||
for i, chunk in enumerate(chunks): | ||
print(f"Adding chunk #{i} tokens={chunk.token_count}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious: are we ok with print logging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to remove this
if request_provider_data is not None: | ||
assert isinstance(request_provider_data, WeaviateRequestProviderData) | ||
|
||
print(f"WEAVIATE API KEY: {request_provider_data.weaviate_api_key}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really want to print the api key into the console?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
Why this PR
We want to setup Weaviate as a remote vector db provider for llama-stack.
What is in the PR