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

Integrate sentence transformers into benchmarks #843

Merged
merged 8 commits into from
Apr 9, 2021

Conversation

Timoeller
Copy link
Contributor

@Timoeller Timoeller commented Feb 17, 2021

Would be nice to compare the new sentencetransformers models, especially the
SentenceTransformer('nq-distilbert-base-v1') and how it compares against DPR finetuned on NQ.

Made sure to use cosine similarity (so had to use ES doc store).
Documents for Sentence Transformers should be a list of [title, text] lists.

Preliminary results do not look better than DPR:
for 100k docs we have mAP of 82.7 SBert vs 86.5 DPR
Details:

100k reference docs
'retriever': 'sentence_transformers', 'doc_store': 'elasticsearch', 'n_docs': 100000, 
'n_queries': 5637, 'retrieve_time': 1288.7816320069996, 
'queries_per_second': 4.37389846348259, 'seconds_per_query': 0.22862899272786938, 
'recall': 93.79102359411034, 'map@10': 82.74644426986096, 
'top_k': 10, 'date_time': datetime.datetime(2021, 2, 17, 19, 29, 25, 961023), 'error': None}


500k reference docs
{'retriever': 'sentence_transformers', 'doc_store': 'elasticsearch', 'n_docs': 500000, 
'n_queries': 5637, 'retrieve_time': 5642.244277841075, 
'queries_per_second': 0.9990705333582115, 'seconds_per_query': 1.0009303313537476, 
'recall': 89.62213943587014, 'map@10': 76.49362488771762, 
'top_k': 10, 'date_time': datetime.datetime(2021, 2, 17, 23, 44, 11, 757607), 'error': None}

@Timoeller
Copy link
Contributor Author

I checked the data and it seems ok.

No answers as well as long answers are removed from NQ dev set.
We put in 100 word passages containing an answer string as positive passages.

Lets revert the config and fix the mypy bug, then we are ready to merge from my side. What do you think @brandenchan ?

Copy link
Contributor

@brandenchan brandenchan left a comment

Choose a reason for hiding this comment

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

This all looks good to me. Just one very tiny comment. Its ready for merge as far as I'm concerned once the mypy bug is fixed and the config is reverted.

test/benchmarks/retriever.py Outdated Show resolved Hide resolved
@Timoeller Timoeller changed the title WIP: Integrate sentence transformers into benchmarks Integrate sentence transformers into benchmarks Apr 8, 2021
@Timoeller
Copy link
Contributor Author

Hey @brandenchan I fixed mypy and reverted the config.

Could you double check, also with the conflicting docs files, so that we can merge?

@brandenchan
Copy link
Contributor

Hey @brandenchan I fixed mypy and reverted the config.

Could you double check, also with the conflicting docs files, so that we can merge?

Ok nice! The conflicting docs seem to be because new arguments were added to functions in master, we updated doc strings and the api documentation was regenerated. I would in each conflict case take the change from master

Copy link
Contributor

@brandenchan brandenchan left a comment

Choose a reason for hiding this comment

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

Once documentation conflicts are resolved, this PR is ready for merge

@Timoeller Timoeller merged commit 837dea4 into master Apr 9, 2021
@Timoeller Timoeller deleted the add_sentencetrans_benchmark branch April 9, 2021 15:24
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

3 participants