-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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: Milvus memory connector #1941
Conversation
Signed-off-by: Filip Haltmayer <filip.haltmayer@zilliz.com>
@microsoft-github-policy-service agree company="Zilliz" |
@filip-halt Following up on your offline note. Looks like it's already assigned for review. @alexchaomander for visibility. |
python/semantic_kernel/connectors/memory/milvus/milvus_memory_store.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Filip Haltmayer <filip.haltmayer@zilliz.com>
Hi @filip-halt the code looks good, but I'm hitting errors with the integration tests where the default server is refusing to start due to communication timeouts. I'm seeing the issue standalone as well. I'm not sure if it's just a me problem, or if there's a bug with the default server - this wasn't an issue for me last week. Are you able to run these locally today? |
What kind of errors are you hitting? (PR seem to be working for me) |
python/semantic_kernel/connectors/memory/milvus/milvus_memory_store.py
Outdated
Show resolved
Hide resolved
I would also like to know what kind of error is being seen? I just tested locally and everything seems to be running fine. Did the error happen on a first runthrough or after a few? |
It might be corruption of the persisted data. If the system crashes before the cleanup of the fixture there might be problems in future runs. Let me take a look. |
Signed-off-by: Filip Haltmayer <filip.haltmayer@zilliz.com>
@awharrison-28 I put the call to preclean the service before running. This might help with your errors. |
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.
Looks like I had an orphan process hogging the default server. Restarting my machine did the trick :)
This change brings in Milvus to the semantic kernel connectors. --------- Signed-off-by: Filip Haltmayer <filip.haltmayer@zilliz.com> Co-authored-by: Abby Harrison <abby.harrison@microsoft.com> Co-authored-by: Abby Harrison <54643756+awharrison-28@users.noreply.github.com>
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.
@awharrison-28 hey, I'm working on a .NET version of the Milvus connector - see #2694. Below are some questions that came up while implementing this, can you give these a quick look? It would be great if we can more or less align across to connectors to ensure a uniform experience etc.
|
||
# Clean up results, filter and get ids for fetch | ||
for x in results: | ||
if min_relevance_score is not None and x["distance"] < min_relevance_score: |
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.
@awharrison-28 unless I'm mistaken, the interaction between limit and the minimal relevance score looks like it yields buggy behavior: since limit is sent to Milvus but not the relevance score, Milvus only returns the top X, which may then get filtered client-side to return less than X to the user. In other words, there may be additional matching results which meet the relevance score, but which aren't returned by Milvus because of the limit.
I indeed cannot see a way to send the minimum relevance score to Milvus for processing server-side, so this behavior may be an acceptable compromise - just wanted to check that you're aware of it etc.
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.
Thank you for raising the issue here, I was not aware.
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.
Update: the recently-released Milvus 2.3 (release notes) has a new feature - range search - which allows doing this. This would be considerably better than the current client-side implementation.
)[0] | ||
return results, distance_metric | ||
except Exception as e: | ||
self._logger.debug(f"Search failed with IP, testing L2: {e}") |
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.
@awharrison-28 rather than catching and retrying the other similarity type, does it make sense to simply allow the user to pass the similarity type when invoking get_nearest_matches_async? After all, the only way to get an index with a similarity type other than the default IP is to explicitly pass L2 to create_collection_async.
Another approach I've seen in other SK memory connectors is to accept such settings in the MemoryStore constructor, and have it affect both create_collection and get_nearest_matches_async. This has the advantage of allowing users to work purely with the MemoryStoreBase abstraction without non-standard arguments to the various functions (only when constructing the memory store, which is non-standard anyway).
additional_metadata=milvus_dict.get("additional_metadata", None), | ||
embedding=embedding, | ||
key=milvus_dict.get("key", None), | ||
timestamp=milvus_dict.get("timestamp", None), |
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.
What representation actually gets saved to Milvus, given that it doesn't seem to support any sort of date/time type?
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.
my understanding is that timestamp resolves to a string
This is a working version of an SK memory store connector for Milvus, for .NET. It is built on top of the recently-released [Milvus.Client](https://www.nuget.org/packages/Milvus.Client) (experimental, in preview), which uses gRPC to communicate with Milvus. There are some minor outstanding issues which are also relevant for the Python connector (merged via #1941), I'll sync with @awharrison-28 to align on these. But this should be more or less ready to merge, we can iterate on the remaining stuff. Closes #2316 /cc @stephentoub @awharrison-28 @luisquintanilla @MarianaGG @tawalke @ajcvickers --------- Co-authored-by: Shawn Callegari <36091529+shawncal@users.noreply.github.com> Co-authored-by: Lee Miller <lemiller@microsoft.com>
This is a working version of an SK memory store connector for Milvus, for .NET. It is built on top of the recently-released [Milvus.Client](https://www.nuget.org/packages/Milvus.Client) (experimental, in preview), which uses gRPC to communicate with Milvus. There are some minor outstanding issues which are also relevant for the Python connector (merged via microsoft#1941), I'll sync with @awharrison-28 to align on these. But this should be more or less ready to merge, we can iterate on the remaining stuff. Closes microsoft#2316 /cc @stephentoub @awharrison-28 @luisquintanilla @MarianaGG @tawalke @ajcvickers --------- Co-authored-by: Shawn Callegari <36091529+shawncal@users.noreply.github.com> Co-authored-by: Lee Miller <lemiller@microsoft.com>
This change brings in Milvus to the semantic kernel connectors.