Skip to content

fix: allow same vector_id in different indexes for SQL-based Document stores#3383

Merged
ZanSara merged 2 commits intodeepset-ai:mainfrom
anakin87:fix_multiple_indexes_for_FAISSDocumentStore
Oct 14, 2022
Merged

fix: allow same vector_id in different indexes for SQL-based Document stores#3383
ZanSara merged 2 commits intodeepset-ai:mainfrom
anakin87:fix_multiple_indexes_for_FAISSDocumentStore

Conversation

@anakin87
Copy link
Member

@anakin87 anakin87 commented Oct 13, 2022

Related Issues

Proposed Changes:

In SQLDocumentStore:

  • vector_id is no longer unique
  • added a UniqueConstraint for index + vector_id

In FAISSDocumentStore:

  • modified update_embeddings method to take this change into account.
    Now for every index, vector_id starts at 0

How did you test it?

  • As suggested by @ZanSara, I locally ran the tests using PostgreSQL with pytest . --document_store_type="sql,faiss"
  • Added new tests for SQLDocumentStore and FAISSDocumentStore

Notes for the reviewer

Checklist

@anakin87 anakin87 requested a review from a team as a code owner October 13, 2022 21:21
@anakin87 anakin87 requested review from vblagoje and removed request for a team October 13, 2022 21:21
@anakin87 anakin87 marked this pull request as draft October 13, 2022 21:46
@anakin87 anakin87 marked this pull request as ready for review October 13, 2022 21:46
@ZanSara ZanSara requested review from ZanSara and removed request for vblagoje October 14, 2022 07:53
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.

Spot on! No remarks, this is good to go right away. Thank you! 😊

@ZanSara ZanSara changed the title refactor: allow same vector_id in different indexes for SQL-based Document stores fix: allow same vector_id in different indexes for SQL-based Document stores Oct 14, 2022
@ZanSara ZanSara merged commit 7290196 into deepset-ai:main Oct 14, 2022
@anakin87 anakin87 deleted the fix_multiple_indexes_for_FAISSDocumentStore branch October 14, 2022 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Using multiple indices in FAISS seems broken

2 participants