Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Add benchmarks for the geosearch #357

Merged
merged 2 commits into from
Sep 21, 2021
Merged

Add benchmarks for the geosearch #357

merged 2 commits into from
Sep 21, 2021

Conversation

irevoire
Copy link
Member

closes #336

Should I merge this PR in #322 and then we merge everything in main or should we wait for #322 to be merged and then merge this one in main later?

@irevoire
Copy link
Member Author

Oops I forgot to update the README and the bench.yaml. I'm putting this in draft PR for now sorry

@irevoire irevoire marked this pull request as draft September 13, 2021 16:23
@irevoire irevoire force-pushed the bench/geosearch branch 3 times, most recently from 852a076 to 97a9f6c Compare September 15, 2021 09:23
benchmarks/README.md Outdated Show resolved Hide resolved
benchmarks/README.md Outdated Show resolved Hide resolved
benchmarks/README.md Outdated Show resolved Hide resolved
irevoire and others added 2 commits September 20, 2021 10:44
Co-authored-by: Clémentine Urquizar <clementine@meilisearch.com>
@irevoire irevoire marked this pull request as ready for review September 20, 2021 08:46
@irevoire irevoire requested a review from curquiza September 20, 2021 08:46
Copy link
Member

@curquiza curquiza 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 Tamo!!

@irevoire
Copy link
Member Author

@Kerollmops, should I merge this in the #322, or should we first merge the geosearch and then the benchmarks on main? What do you prefer?

@Kerollmops
Copy link
Member

I prefer that we merge #322 and this one after :)

Base automatically changed from geosearch to main September 20, 2021 19:33
@irevoire
Copy link
Member Author

Ok so I guess we can merge now 😁

Copy link
Member

@Kerollmops Kerollmops 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!
Have you tested it with the CI, already?

@irevoire
Copy link
Member Author

irevoire commented Sep 21, 2021

Yes!
But now I'm wondering, maybe we could add a bench on a really small dataset? ~10.000 documents

@Kerollmops
Copy link
Member

How much time does it take to run this one? If it is much longer than the other ones, maybe we should indeed!

@irevoire
Copy link
Member Author

No, currently, all the bench related to the geosearch are super fast. It takes 20 minutes for the search and an insignificant amount of time in the indexing benchmark (1h50m5s before the geosearch vs 1h50m6s with this branch).

So I think it would be good to keep this dataset of 1M documents because it's what we want to improve.
But at the same time, add a really small dataset to ensure we don't lose any performance on small datasets.

Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

You can open a new PR to add a small dataset if you want, I prefer to merge this one for now :)
bors merge

@bors
Copy link
Contributor

bors bot commented Sep 21, 2021

@bors bors bot merged commit 700318d into main Sep 21, 2021
@bors bors bot deleted the bench/geosearch branch September 21, 2021 16:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add GeoSearch test to benchmarks (for indexation and search)
3 participants