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

Milvus integration #771

Merged
merged 16 commits into from
Jan 29, 2021
Merged

Milvus integration #771

merged 16 commits into from
Jan 29, 2021

Conversation

lalitpagaria
Copy link
Contributor

@lalitpagaria lalitpagaria commented Jan 25, 2021

This PR to integrate Milvus as document store with Haystack.

Task list -

  • Fix code (Currently it have lot of copied code from fiass)
  • Parameterise index search and creation parameters using kwargs dict
  • Add test cases
  • Fix Doc string

Not handling these task in this PR, because it require discussion as milvus server need DB folder mounting for persistent storage -

  • Add tutorial
  • Integrate milvus in docker compose instead of ES

@lalitpagaria
Copy link
Contributor Author

lalitpagaria commented Jan 25, 2021

@tholor and @tanaysoni

Please find WIP PR for Milvus integration. Currently I just merged my old PR, and latest master changes. There are many things require changes (inside the methods), I will do it in coming days.

From design perspective I using same design as FaissDocumentStore, but my old PR had different design. With my old PR we would use any of storage ie RDBMS, ES or Memory to store documents and it's metadata. But I thought now we want to get rid of ES so not used that design. Please let me know if you want to make this document store more generic. Also review task list and add/remove/update accordingly.

@lalitpagaria lalitpagaria changed the title [WIP] Milvus integration Milvus integration Jan 26, 2021
@lalitpagaria
Copy link
Contributor Author

@tholor and @tanaysoni PR is ready for review.
I am handling following tasks with this PR, because it require discussion as milvus server need DB folder mounting for persistent storage -

  • Add tutorial
  • Integrate milvus in docker compose instead of ES

Copy link
Member

@tholor tholor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @lalitpagaria !

I think the overall design is great. I left a few minor comments around naming and exceptions.
I will also adjust a few docstrings tomorrow.

I am handling following tasks with this PR, because it require discussion as milvus server need DB folder mounting for persistent storage -

I am not sure what you meant here. Are you talking about the integration into the docker-compose requiring mounted volumes for persisting the data?

Open TODOs (we can handle them in a separate PR if preferred):

  • Tutorial
  • Docker integrations
  • Support filters
  • Documentation why/when to use milvus FYI @brandenchan
  • Benchmarks FYI @brandenchan

haystack/document_store/milvus.py Outdated Show resolved Hide resolved
haystack/document_store/milvus.py Outdated Show resolved Hide resolved
haystack/document_store/milvus.py Outdated Show resolved Hide resolved
haystack/document_store/milvus.py Outdated Show resolved Hide resolved
haystack/document_store/milvus.py Outdated Show resolved Hide resolved
@tholor tholor requested a review from tanaysoni January 26, 2021 18:35
@lalitpagaria
Copy link
Contributor Author

Sorry I mean "I am not handling".
Yes need to adjust docker compose and adding these changes will increase PR size. Better to create another PR to address the tasks your already added in your above comment.

Copy link
Contributor

@tanaysoni tanaysoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this @lalitpagaria!

haystack/document_store/milvus.py Outdated Show resolved Hide resolved
haystack/document_store/milvus.py Outdated Show resolved Hide resolved
haystack/document_store/milvus.py Outdated Show resolved Hide resolved
@lalitpagaria
Copy link
Contributor Author

@tholor and @tanaysoni can you please check see if anything is pending.

@xpatronum
Copy link
Contributor

@lalitpagaria Have you tried benchmarking MILVUS vs FAISS on the number of passages retrieved per sec.?

@lalitpagaria
Copy link
Contributor Author

No it will be done after merging this PR.
Please refer this ticket to track progress #778

@lalitpagaria
Copy link
Contributor Author

And for latest Faiss benchmark please refer this #773

@tholor tholor merged commit 9f7f952 into deepset-ai:master Jan 29, 2021
@lalitpagaria lalitpagaria deleted the milvus branch January 29, 2021 12:31
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.

None yet

4 participants