-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
bugfix when initializing with async aoss vector store #17340
base: main
Are you sure you want to change the base?
Conversation
except TypeError: | ||
# Probably using async so switch to async client | ||
try: | ||
asyncio.run(self._os_async_client.indices.get(index=self._index)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a little error prone -- a little safer to do
from llama_index.core.async_utils import asyncio_run
asyncio_run(...)
This will make sure any existing async loop is used
@@ -123,14 +123,18 @@ def __init__( | |||
self._os_async_client = os_async_client or self._get_async_opensearch_client( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious, there's no way to create an sync client from an async client, or vice-versa? This would solve a lot of headaches if possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you could if you dig into the kwargs and change it accordingly for each sync/async instantiation
from opensearchpy import AsyncHttpConnection, AWSV4SignerAsyncAuth, RequestsHttpConnection, AWSV4SignerAuth
# Sync parameters
kwargs['auth'] = AWSV4SignerAuth(credentials, region, 'aoss')
kwargs['connection_class'] = RequestsHttpConnection
self._os_client = os_client or self._get_opensearch_client(
self._endpoint, **kwargs
)
# Async parameters
kwargs['auth'] = AWSV4SignerAsyncAuth(credentials, region, 'aoss')
kwargs['connection_class'] = AsyncHttpConnection
self._os_client = os_client or self._get_opensearch_client(
self._endpoint, **kwargs
)
But you'd need the AWS credentials/region/service ('aoss') as additional inputs, seems too much
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yea, thats pretty messy. Too bad
Looks like you'll need to run |
…Aydin-ab/llama_index into bugfix-async-aoss-vector-stores
Sorry I don't know how that works :( |
Description
@logan-markewich
Following up this issue [Bug]: Can't initialize OpensearchVectorClient through AWS endpoint in async mode #16746
16746
At the moment, we can't initialize a OpensSearchVectorClient using AOSS (amazon opensearch serverless) and/or an async connection. There are 2 problems here:
GET /
call to the endpoint. This works on standard Opensearch but OpenSearch Serverless doesn't support that command.__init__()
. This is because the code doesn't use the async client but only the "sync" one, this raises a TypeError due to a coroutine appearing somewhere.Fixes # (issue)
self.aoss
). Ideally, we'd like to find a workaround to check the version so we can still use efficient-kNN filtering while using AOSS.except
clause that catches a TypeError and retry with the async client this time.New Package?
Had to import asyncio to run the async client but it's a python built-in anyway
Version Bump?
Type of Change
Please delete options that are not relevant.
How Has This Been Tested?
Not sure how to test that, we'd need to simulate an endpoint ?
Suggested Checklist:
make format; make lint
to appease the lint gods