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

fix(vector_stores): elasticsearch - add missing await #11089

Closed
wants to merge 2 commits into from

Conversation

aGallea
Copy link

@aGallea aGallea commented Feb 21, 2024

Description

vector_stores @ elasticsearch - Added missing await for indices.exists function call.
solves: /llama_index/vector_stores/elasticsearch/base.py:344: RuntimeWarning: coroutine 'IndicesClient.exists' was never awaited - which causes not creating the ES index.

Fixes # (issue)

Type of Change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

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

  • Added new unit/integration tests
  • Added new notebook (that tests end-to-end)
  • 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

@aGallea aGallea marked this pull request as ready for review February 21, 2024 13:25
@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Feb 21, 2024
@@ -239,7 +239,7 @@ async def _create_index_if_not_exists(
index_name: Name of the AsyncElasticsearch index to create.
dims_length: Length of the embedding vectors.
"""
if self.client.indices.exists(index=index_name):
if await self.client.indices.exists(index=index_name):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually removed this because in another version of the elastic client, this was synchronous

Can we figure out which version has this synchronous and pin the version in pyproject.toml

Copy link
Author

Choose a reason for hiding this comment

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

@logan-markewich thanks for the review
I see that the ES client being used is actually the async one here
which is also mentioned when providing it as a paramater here

What say you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@aGallea I'm taking about this issue
#10548

It seems like there is some client version where this operation is not actually async? And we should avoid that version using the dependency in the pyproject.toml

Choose a reason for hiding this comment

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

Hi. I am having the same issue with the method not being awaited. What is the best way to get this change on my machine? I am a C# developer and I am finding my way with python :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@afederici75 this fix is not merged yet -- was hoping we could narrow down proper requirements on the client version (since in some client version, this isn't async, as seen in the issue linked above)

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking maybe to wrap the function with asyncio.coroutine, so both clients will work when calling the functions with await.
What do you say?

Choose a reason for hiding this comment

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

@logan-markewich thank you. I temporarily changed the code in the imported package and it seems to work for now.

@nerdai
Copy link
Contributor

nerdai commented Feb 27, 2024

Duplicate of #11438

(which came after, but nevertheless is closed now). Thanks @aGallea for raising, sorry we didn't get this in sooner!

@nerdai nerdai closed this Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants