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

chore: add DenseRetriever abstraction #3252

Merged
merged 53 commits into from
Sep 21, 2022
Merged

chore: add DenseRetriever abstraction #3252

merged 53 commits into from
Sep 21, 2022

Conversation

tstadel
Copy link
Member

@tstadel tstadel commented Sep 20, 2022 •
edited by ZanSara
Loading

Related Issues

Proposed Changes:

  • introduce DenseRetriever abstraction to facilitate usage of retrievers in document store's update_embeddings
  • streamline validation of embedding shape across all document stores (BaseDocumentStore_validate_embeddings_shape)

Breaking Changes:

DensePassageRetriever and EmbeddingRetriever: embed_queries's arg changed from texts to queries

How did you test it?

  • no new tests necessary

Notes for the reviewer

Checklist

tstadel and others added 30 commits September 14, 2022 17:53
@tstadel
support cosine similiarity with faiss
9b74ab9
@tstadel
update docs
b81cc00
@tstadel
update api docs
6138fdf
@tstadel
fix tests
0b65cd0
@tstadel
Revert "update api docs"
b107cfe 
This reverts commit 6138fdf.
@tstadel
fix api docs
7178027
@tstadel
collapse test
c9c050f
@tstadel
rename similairity to space_type mappings
4786d8b
@tstadel
only normalize for faiss
c441a1a
@tstadel
Merge branch 'main' into os_faiss_cosine
fbd5983
@tstadel
fix merge
77869f9
@tstadel
fix docs normalization
25592f5
@tstadel
get rid of List[np.array]
277d3ea
@tstadel
update docs
2f08839
@tstadel
fix tests and tutorials
9cb849b
@tstadel
fix mypy
cf1f575
@tstadel
fix mypy
6714e1a
@tstadel
fix mypy again
088a00d
@tstadel
again mypy
fae55bf
@tstadel
blacken
52ebe5c
@tstadel
update tutorial 4 docs
1b284ad
@tstadel
fix embeddingretriever
6df02ab
@tstadel
fix faiss
408d81b
@tstadel
move dense specific logic to DenseRetriever
b16ab45
@tstadel
fix mypy
a17fc4c
@tstadel
cosine tests for all documents stores
51a8154
@tstadel
fix pinecone
6cee6f2
@tstadel
add docstring
a56d424
@tstadel
docstring corrections
6ad31f4
@tstadel
update docs
603c2ab
@tstadel tstadel requested review from a team as code owners September 20, 2022 17:50
@tstadel tstadel requested review from vblagoje and removed request for a team September 20, 2022 17:50
@ZanSara ZanSara requested review from ZanSara and removed request for vblagoje September 21, 2022 07:45
ZanSara
ZanSara suggested changes Sep 21, 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.

Super cool! I'm really happy for this change 🤩 There is one "big" question we might want to address, the other are smaller notes. Thank you for taking care of this!

haystack/document_stores/elasticsearch.py Outdated Show resolved Hide resolved
haystack/document_stores/elasticsearch.py Show resolved Hide resolved
haystack/document_stores/milvus2.py Show resolved Hide resolved
haystack/nodes/retriever/_embedding_encoder.py Show resolved Hide resolved
haystack/nodes/retriever/base.py Show resolved Hide resolved
haystack/nodes/retriever/dense.py Outdated Show resolved Hide resolved
@ZanSara ZanSara mentioned this pull request Sep 21, 2022
Closed
6 tasks
@ZanSara ZanSara mentioned this pull request Sep 21, 2022
Closed
ZanSara
ZanSara approved these changes Sep 21, 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.

Looks good! Thank you 😊

@ZanSara ZanSara merged commit b10e2c3 into main Sep 21, 2022
@ZanSara ZanSara deleted the dense_retriever branch September 21, 2022 17:08
@tstadel tstadel mentioned this pull request Sep 21, 2022
Merged
6 tasks
@TuanaCelik TuanaCelik mentioned this pull request Sep 22, 2022
Merged
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

Assignees
No one assigned
Labels
breaking change topic:document_store topic:retriever type:refactor Not necessarily visible to the users
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BaseRetriever deepcopies all documents at indexing
2 participants
@tstadel @ZanSara