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

Updating Milvus2 implementation for new dependency management #2121

Closed
wants to merge 6 commits into from

Conversation

MichelBartels
Copy link
Contributor

Proposed changes:
The new dependency management was not working properly with Milvus2 (reference to milvus2 install option, but not present in setup.cfg and __init__.py in document_stores did not load Milvus2DocumentStore correctly). This PR fixes this.

Status (please check what you already did):

  • First draft (up for discussions & feedback)
  • Final code
  • Added tests
  • Updated documentation

@ZanSara
Copy link
Contributor

ZanSara commented Feb 3, 2022

I'm not sure we want to have 3 separate extras for Milvus though. Is there a special reason to pin the release candidate 6 in a separate one? I definitely made a mistake by leaving references to milvus2 around, but the aim was to have only milvus1 (<2.0.0) and milvus (>=2.0.0), as a sign that soon Milvus 2 will soon be our default. However, aggregate extras like docstores will still use milvus1 until the code enabled by milvus becomes stable.

Let me know if I missed something though! Maybe we really want to keep the rc6 into our dependencies for some reason.

@julian-risch julian-risch requested a review from ZanSara February 4, 2022 09:22
@MichelBartels
Copy link
Contributor Author

You are right. I was a confused by the reference to "milvus2".
Then, however, we should also change "milvus" in this line to "milvus1".

The current implementation of Milvus2DocumentStore does require rc6. However, I have already been working on updating this document store as part of #2120.
I think I'm going to close this PR for now and separate the doc store update of #2120 into a separate PR which will also fix the references to the Milvus extras.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants