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.

Sign up for GitHub

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

Jump to bottom

BREAKING CHANGE: remove Milvus1DocumentStore along with support for Milvus < 2.x #3552

Merged
merged 3 commits into from
Nov 15, 2022
Merged

BREAKING CHANGE: remove Milvus1DocumentStore along with support for Milvus < 2.x #3552

merged 3 commits into from
Nov 15, 2022

Conversation

masci
Copy link
Contributor

@masci masci commented Nov 10, 2022

Related Issues

  • fixes n/a (going on, we should have an issue to schedule removal of deprecated components)

Proposed Changes:

  • Remove support for Milvus 1.x
  • Deprecate Milvus2DocumentStore in favor of MilvusDocumentStore [FIXME: link removal issue]

How did you test it?

Notes for the reviewer

Checklist

@masci masci requested review from a team as code owners November 10, 2022 15:46
@masci masci requested review from mayankjobanputra and removed request for a team November 10, 2022 15:46
@masci
Copy link
Contributor Author

masci commented Nov 11, 2022

Paging @ZanSara for reviewing the deprecation policy and @bogdankostic who did the heavy lifting.

ZanSara
ZanSara reviewed Nov 14, 2022
View reviewed changes
README.md Show resolved Hide resolved
haystack/utils/doc_store.py Show resolved Hide resolved
test/document_stores/test_milvus.py Outdated Show resolved Hide resolved
ZanSara
ZanSara suggested changes Nov 14, 2022
View reviewed changes
Copy link
Contributor

@ZanSara ZanSara left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Let's fix the deprecation warning situation before merging

@masci masci requested a review from ZanSara November 14, 2022 14:33
mayankjobanputra
mayankjobanputra approved these changes Nov 14, 2022
View reviewed changes
Copy link
Contributor

@mayankjobanputra mayankjobanputra left a comment

Choose a reason for hiding this comment

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

@masci I looked at the PR just out of curiosity... so Milvus2 doesn't require a separate docker and gets installed with the library itself?

@masci
Copy link
Contributor Author

masci commented Nov 14, 2022

@mayankjobanputra uhm are you referring to the functions in utils/doc_store.py? Those are merely utility functions that you can use to orchestrate a bunch of docker containers through Haystack.

ZanSara
ZanSara approved these changes Nov 15, 2022
View reviewed changes
Copy link
Contributor

@ZanSara ZanSara left a comment

Choose a reason for hiding this comment

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

🚀 Let's merge this!

@masci masci merged commit 6a48ace into main Nov 15, 2022
@masci masci deleted the massi/remove-milvus1 branch November 15, 2022 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZanSara ZanSara ZanSara approved these changes

@mayankjobanputra mayankjobanputra mayankjobanputra approved these changes

@agnieszka-m agnieszka-m agnieszka-m approved these changes

@bogdankostic bogdankostic Awaiting requested review from bogdankostic

Assignees
No one assigned
Labels
breaking change topic:document_store
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@masci @ZanSara @mayankjobanputra @agnieszka-m