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

refactor!: extract elasticsearch #4668

Merged
merged 8 commits into from
Apr 26, 2023
Merged

refactor!: extract elasticsearch #4668

merged 8 commits into from
Apr 26, 2023

Conversation

ZanSara
Copy link
Contributor

@ZanSara ZanSara commented Apr 14, 2023

Related Issues

Proposed Changes:

  • Extract elasticsearch into an elasticsearch extra and add it to the docstores extra
  • Fence off all imports to make them optional

How did you test it?

  • CI

Notes for the reviewer

  • This is likely to affect many tutorials, examples, snippets, etc etc. Let's try to find everything that is affected by this change.
  • Docker images seems to be unaffected ✔️
  • TODO update tutorials
  • TODO update documentation

Checklist

  • I have read the contributors guidelines and the code of conduct
  • I have updated the related issue with new insights and changes
  • I added tests that demonstrate the correct behavior of the change
  • I've used one of the conventional commit types for my PR title: fix:, feat:, build:, chore:, ci:, docs:, style:, refactor:, perf:, test:.
  • I documented my code
  • I ran pre-commit hooks and fixed any issue

@ZanSara ZanSara marked this pull request as ready for review April 18, 2023 13:25
@ZanSara ZanSara requested a review from a team as a code owner April 18, 2023 13:25
@ZanSara ZanSara requested review from silvanocerza and removed request for a team April 18, 2023 13:25
@silvanocerza
Copy link
Contributor

Looks alright to me, I'd wait to merge #4602 before this one though.

Also let's plan all the tutorials and docs edits before merging it maybe.

@ZanSara
Copy link
Contributor Author

ZanSara commented Apr 20, 2023

Alright, I'll talk to @TuanaCelik and @bilgeyucel to see how to proceed with the tutorials forst. They have an issue open, but I think we should have a PR ready too. deepset-ai/haystack-tutorials#166

Regarding the docs, I'll check with the docs team and update this thread as well.

@ZanSara ZanSara added this to the 1.16 milestone Apr 20, 2023
@tholor
Copy link
Member

tholor commented Apr 20, 2023

Nice, makes sense to carve it out and make MemoryDocumentStore the go-to-doc-store for beginners/tutorials ...!

Quick question: What error message do users see if they try to import ElasticsearchDocumentstore but miss the dependency? Do we give them advise on how to fix it?

@dfokina dfokina linked an issue Apr 20, 2023 that may be closed by this pull request
@ZanSara
Copy link
Contributor Author

ZanSara commented Apr 20, 2023

@tholor they will see the same error they get from all other document stores, so:

Failed to import 'ElasticsearchDocumentStore' which is an optional component in Haystack.
Run 'pip install 'farm-haystack[elasticsearch]'' to install the required dependencies and make this 
component available. (Original error: <the import error>)

See https://github.com/deepset-ai/haystack/blob/main/haystack/utils/import_utils.py#L56-L63 and https://github.com/deepset-ai/haystack/blob/main/haystack/document_stores/elasticsearch.py#L13

@bogdankostic
Copy link
Contributor

I added the elasticsearch extra to this PR of the tutorials repo and made a suggestion on this documentation page, so I think we're good to go.

@ZanSara ZanSara merged commit 1b57b96 into main Apr 26, 2023
@ZanSara ZanSara deleted the extract-es branch April 26, 2023 08:14
bogdankostic pushed a commit that referenced this pull request Apr 26, 2023
* extract elasticsearch

* update pyproject.toml

* make more import optional

* move MockBaseRetriever in conftest

* install es in the es integration tests
koganei pushed a commit to koganei/haystack that referenced this pull request Apr 29, 2023
* extract elasticsearch

* update pyproject.toml

* make more import optional

* move MockBaseRetriever in conftest

* install es in the es integration tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extract elasticsearch
4 participants