haystack-core-integrations icon indicating copy to clipboard operation
haystack-core-integrations copied to clipboard

Improve `OpenSearchDocumentStore.__init__` arguments

Open EdAbati opened this issue 1 year ago • 3 comments

Closes #671

As explained in the issue, I am proposing to change a bit the arguments that OpenSearchDocumentStore accepts.

The goal is:

  1. First, to allow the user to set the most appropriate max_chunk_bytes. (The current default makes this component fail for certain deployments)
  2. To be more transparent on which arguments are required and which **kwargs are passed to the OpenSearch client. Currently the docstring does not specify which **kwargs are passed to the client, and which are passed to self._client.indices.create.

Happy to hear your thoughts on how I can improve this PR :)

EdAbati avatar May 18 '24 07:05 EdAbati

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar May 18 '24 07:05 CLAassistant

Thanks for this contribution @EdAbati we'll be getting back to you shortly. @tstadel how does this proposal sound to you?

vblagoje avatar May 21 '24 08:05 vblagoje

@vblagoje contribution looks like a nice improvement. @EdAbati thanks for raising this. I left two comments that should be tackled before merging.

tstadel avatar May 21 '24 09:05 tstadel

@EdAbati have you had a chance to look at the potential issues pointed by Thomas?

vblagoje avatar May 24 '24 05:05 vblagoje

Hi both thank you for the review. :) I am a bit busy this week, I'll go back to this PR next week

EdAbati avatar May 24 '24 06:05 EdAbati

Hi, I made some changes to fix what was suggested :) I can see a mypy error but only in the python 3.9 CI. do you know what is causing it and/or how I could solve it?

EdAbati avatar May 26 '24 19:05 EdAbati

Tests are green 👍

masci avatar May 27 '24 06:05 masci

🙏 @masci and thanks a lot for this contribution @EdAbati , looks ok to you now @tstadel ?

vblagoje avatar May 27 '24 06:05 vblagoje