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.

Sign up for GitHub

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

Jump to bottom

Integrate VectorStore from Elasticsearch client #13291

Merged
merged 11 commits into from May 15, 2024
Merged

Integrate VectorStore from Elasticsearch client #13291

merged 11 commits into from May 15, 2024

Conversation

maxjakob
Copy link
Contributor

@maxjakob maxjakob commented May 6, 2024 •
edited

Description

We recently added a VectorStore abstraction to the Elasticsearch client library (elastic/elasticsearch-py#2528, see module) in order to centralize the development and ensure all GenAI library integrations work the same.

This PR integrates the new module into LlamaIndex. The LlamaIndex class keeps its existing interface, it is just extended with the option to specify more retrieval strategies.

Type of Change

  • New feature
    • non-breaking change in default usage of the vector store
    • breaking change when using query modes that do not match the retrieval mode specified at init time
    • --> I would suggest making a breaking release with version 0.2.0
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Adapted existing unit and integration tests. Note that the abstract library already tests its own functionality with respect to the different retrieval strategies.
  • I stared at the code and made sure it makes sense

Suggested Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added Google Colab support for the newly added notebooks.
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I ran make format; make lint to appease the lint gods

@maxjakob maxjakob force-pushed the es-use-orchestration-lib branch 2 times, most recently from 260132f to 6ca21d0 Compare May 13, 2024 13:13
@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@maxjakob maxjakob force-pushed the es-use-orchestration-lib branch 4 times, most recently from 85d1b09 to 99b61bd Compare May 13, 2024 13:41
@maxjakob maxjakob marked this pull request as ready for review May 13, 2024 13:51
@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label May 13, 2024
logan-markewich
logan-markewich reviewed May 13, 2024
View reviewed changes
Copy link
Collaborator

@logan-markewich logan-markewich left a comment

Choose a reason for hiding this comment

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

This looks good to me. I tried running ElasticsearchIndexDemo though and got this error:

NotFoundError: NotFoundError(404, 'resource_not_found_exception', 'Could not find trained model [.elser_model_2]')

Did I miss a step?

@maxjakob
Copy link
Contributor Author

@logan-markewich The ELSER model needs to be deployed first. I added a comment to this section.

logan-markewich
logan-markewich reviewed May 14, 2024
View reviewed changes
...ores/llama-index-vector-stores-elasticsearch/llama_index/vector_stores/elasticsearch/base.py
query_embedding = cast(List[float], query.query_embedding)

es_query = {}
_mode_must_match_retrieval_strategy(query.mode, self.retrieval_strategy)

if query.filters is not None and len(query.filters.legacy_filters()) > 0:
Copy link
Collaborator

Choose a reason for hiding this comment

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

fyi this is why less filters are supported, elastic is only using the legacy exact match filters. If updated, it could use many filters (less than, greater than, contains, etc.) and operators (AND, OR)

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label May 14, 2024
@logan-markewich logan-markewich enabled auto-merge (squash) May 14, 2024 17:32
@logan-markewich
Copy link
Collaborator

Seems like aiohttp needs to be an explicit dependency -- adding that now

@maxjakob
Copy link
Contributor Author

maxjakob commented May 15, 2024 •
edited

How odd, CI ran successfully before, then you changed some text in a notebook and from then on it's failing. Let me trigger CI again (without explicit aiohttp installation).

auto-merge was automatically disabled May 15, 2024 10:06

Head branch was pushed to by a user without write access

@maxjakob
Copy link
Contributor Author

Rebased on main.

@logan-markewich logan-markewich merged commit 07a0c66 into run-llama:main May 15, 2024
8 checks passed
@maxjakob
Copy link
Contributor Author

Thank you so much for pushing this over the finish line @logan-markewich!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@logan-markewich logan-markewich logan-markewich approved these changes

Assignees
No one assigned
Labels
lgtm This PR has been approved by a maintainer size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@maxjakob @logan-markewich