-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Python: Introducing Vector Search to Weaviate and adding the ability to have unnamed vectors #9684
base: main
Are you sure you want to change the base?
Conversation
1113161
to
f72e542
Compare
except Exception as ex: | ||
raise MemoryConnectorException(f"Failed to upsert records: {ex}") | ||
try: | ||
collection: CollectionAsync = self.async_client.collections.get(self.collection_name) |
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.
Without the context manage, don't we need to call .connect()
manually: https://weaviate.io/developers/weaviate/client-libraries/python/async#context-manager?
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.
yes, we do in our aenter
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.
Same question, what if people don't use the context manager? That shouldn't stop people from being able to use the connector.
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.
this is the same for other collections, and it will throw, but I think we need to steer people to using the context manager, since it is much cleaner anyway, I will update docs accordingly
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 thing about context manager is that users will have to use the connector within a with
block. Similar to the clients we are using, we need to do special handlings to use them properly. I am not sure if this is the direction we are going, but it seems counter intuitive to take away the ability to use our connectors outside a context manager, i.e. what if we need to pass the connector around in the kernel.
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.
I agree people should use context manage as much as possible but we should still allow them to use the connectors without a context manager.
python/semantic_kernel/connectors/memory/weaviate/weaviate_collection.py
Show resolved
Hide resolved
python/semantic_kernel/connectors/memory/weaviate/weaviate_collection.py
Outdated
Show resolved
Hide resolved
28d4d25
to
7161437
Compare
7161437
to
a1a4032
Compare
Motivation and Context
This PR adds vector search to Weaviate.
All three types of search are supported, however vectorizable_text_search depends on a setup outside SK.
Also adds a parameter to the Weaviate Collection called 'named_vectors', default is True.
When set to False it uses unnamed vectors instead of named.
Because of this there is a slight difference in how vectors are represented to Weaviate which might be breaking.
Description
The breaking change is that, vector were set to have the name of the content that it vectorized, for instance:
With a datamodel like this (shortened some of the class names):
This would be set as
DataObject(vector: {"content": [vector content]})
, while it will now set this toDataObject(vector: {"vector": [vector content]})
Where DataObject is the Weaviate object used to insert items.Closes #6839
Contribution Checklist