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

feat: exponential backoff with exp decreasing batch size for opensearch and elasticsearch client #3194

Merged
merged 24 commits into from
Sep 13, 2022

Conversation

ArzelaAscoIi
Copy link
Member

@ArzelaAscoIi ArzelaAscoIi commented Sep 9, 2022

Related Issues

  • bugfix

Proposed Changes:

  • add custom _bulk insert for opensearch + elasticsearch client
  • add split method to split list into N chunks of (approx) same size

How did you test it?

  • unit tests

Note to the reviewer

There is also a bulk insert for the clone_embedding_field within the openserach document store. Do we also need to update this ? Currently we just inherit the bulk insert for opensearch from elasticsearch.

I had to merge this branch: #3189 to test it

Checklist

@ArzelaAscoIi ArzelaAscoIi requested a review from a team as a code owner September 9, 2022 15:13
@ArzelaAscoIi ArzelaAscoIi requested review from bogdankostic and removed request for a team September 9, 2022 15:13
@ArzelaAscoIi ArzelaAscoIi changed the base branch from main to custom_mapping-patch September 9, 2022 15:13
@ArzelaAscoIi ArzelaAscoIi requested a review from a team as a code owner September 9, 2022 15:13
…gin/master/feat/opensearchDocumentStoreRetry
@ArzelaAscoIi ArzelaAscoIi changed the base branch from custom_mapping-patch to main September 9, 2022 15:16
Copy link
Contributor

@masci masci left a comment

Choose a reason for hiding this comment

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

Left a couple of comments, waiting for the import path fix to be merged before reviewing tests

ArzelaAscoIi and others added 2 commits September 13, 2022 09:58
Co-authored-by: Massimiliano Pippi <mpippi@gmail.com>
…hub.com:deepset-ai/haystack into origin/master/feat/opensearchDocumentStoreRetry
@ArzelaAscoIi ArzelaAscoIi requested a review from masci September 13, 2022 09:07
Copy link
Contributor

@masci masci left a comment

Choose a reason for hiding this comment

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

Thanks for the update, looks very good! Left a couple of small ones.

@ArzelaAscoIi ArzelaAscoIi requested a review from masci September 13, 2022 12:54
Copy link
Contributor

@masci masci 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 ship it!

@ArzelaAscoIi ArzelaAscoIi merged commit da1cc57 into main Sep 13, 2022
@ArzelaAscoIi ArzelaAscoIi deleted the origin/master/feat/opensearchDocumentStoreRetry branch September 13, 2022 13:30
@tholor
Copy link
Member

tholor commented Sep 13, 2022

Isn't this feature for both opensearch and elasticsearch? If yes, let's change the PR title so that we have it correctly in the next release notes.

@ArzelaAscoIi ArzelaAscoIi changed the title feat: exponential backoff with exp decreasing batch size for opensearch client feat: exponential backoff with exp decreasing batch size for opensearch and elasticsearch client Sep 13, 2022
@ArzelaAscoIi
Copy link
Member Author

Yes :) Adjusted the title

brandenchan pushed a commit that referenced this pull request Sep 21, 2022
…ch client (#3194)

* Validate custom_mapping properly as an object

* Remove related test

* black

* feat: exponential backoff with exp dec batch size

* added docstring and split doc lsit

* fix

* fix mypy

* fix

* catch generic exception

* added test

* mypy ignore

* fixed no attribute

* added test

* added tests

* revert strange merge conflicts

* revert merge conflict again

* Update haystack/document_stores/elasticsearch.py

Co-authored-by: Massimiliano Pippi <mpippi@gmail.com>

* done

* adjust test

* remove not required caplog

* fixed comments

Co-authored-by: ZanSara <sarazanzo94@gmail.com>
Co-authored-by: Massimiliano Pippi <mpippi@gmail.com>
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.

4 participants