-
Notifications
You must be signed in to change notification settings - Fork 2
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
Initial API POC #1
Conversation
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.
good stuff! tests make usage pretty clear, and i like some of the extensions you've added to the Milvus ORM.
think it would be nice to add a readme to articulate a bit what this does that the pymilvus ORM (or other future vector db ORMs for that matter) do not to make usage a bit more obvious. I also think a Jupyter notebook with some examples would make this thing ✨
self._filters.append(f.to_expression()) | ||
return self | ||
|
||
def offset(self, offset: int): |
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.
recommend using the setter decorator for these: https://www.geeksforgeeks.org/getter-and-setter-in-python/
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.
@hweller1 These are intended to be used as chaining functions query().filter(XYZ).offset(2)
- are you envisioning a use case where people are directly using getters and setters as part of this chain?
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.
oh i think the usage shouldn't have to change, was just saying that there is a standard way of defining attribute setting. more syntactic than functional
class IVF_FLAT(IndexBase): | ||
""" | ||
- High-speed query | ||
- Requires a recall rate as high as possible |
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.
would augment this with some quick benchmarks. Very simple search task where IVF_FLAT is a better choice than say HNSW or ANNOY
) | ||
) | ||
|
||
schema = CollectionSchema(fields=fields, description=f"{cls.__name__} vectordb-generated 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.
|
||
def _dict_representation(self): | ||
type_converters = { | ||
np.ndarray: DataType.FLOAT_VECTOR, |
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.
took me a bit of digging but it's probably worth highlighting that this is the one type conversion you have that Milvus does not support out of the box, and is a major improvement on it's ORM! #numpy is all you need
def _result_to_objects(self, search_result: ChunkedQueryResult | list[dict[str, Any]]): | ||
query_results : list[QueryResult] = [] | ||
|
||
if isinstance(search_result, ChunkedQueryResult): |
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.
when does Milvus return this vs an ordinary QueryResult
?
Very rough initial sketch of the ORM syntax. This PR is aiming to:
Insertion instructions:
Search instructions: