draft: compass async sdk #90
Conversation
cohere/compass/clients/compass.py
Outdated
|
|
||
| @retry( | ||
| stop=stop_after_attempt(compass_request["max_retries"]), | ||
| wait=wait_fixed(compass_request["sleep_retry_seconds"]), |
There was a problem hiding this comment.
I hope for async it is using asyncio.sleep but should probably double check
cohere/compass/clients/compass.py
Outdated
|
|
||
| def __init__( | ||
| self, | ||
| *, | ||
| index_url: str, | ||
| bearer_token: Optional[str] = None, | ||
| http_session: Optional[requests.Session] = None, | ||
| default_max_retries: int = DEFAULT_MAX_RETRIES, | ||
| default_sleep_retry_seconds: int = DEFAULT_SLEEP_RETRY_SECONDS, | ||
| ): |
There was a problem hiding this comment.
Curious why you removed the docstring instead of changing it to fit the new BaseCompassClient.
cohere/compass/clients/compass.py
Outdated
| ) | ||
| self.session = http_session or aiohttp.ClientSession() | ||
|
|
||
| async def search_documents( |
There was a problem hiding this comment.
I see you only implemented the search APIs. Do you plan to implement the rest of the async methods?
There was a problem hiding this comment.
Yes, this PR is more of a draft than anything else. Just wanted to make sure you guys are okay with this change before I commit to implementing the rest.
There was a problem hiding this comment.
Great, thanks.
I am wondering whether there is a way to automate this, somehow, so we don't have to keep adding/modifying APIs in two places. The only difference really between the two clients is how the _send_request method is implemented, so I can suggest the following, though it is an abstract idea that I have not tested:
- Define all the API methods in the base class and let them simply just call the abstract method
_send_requestdefined in the base class. - The sync and async clients will define the
_send_requestmethod differently. - To support type annotation, the base client class can be made generic with a generic argument for the return type of
_send_request, e.g.
class BaseClient(T):
def _send_request(...) -> T
pass- The Sync and Async client specifies the expected return type, e.g.:
class CompassClient(BaseClient[RetryResponse]):
pass
class CompassAsyncClient(BaseClient[Awaitable[RetryResponse]]):
passThought?
There was a problem hiding this comment.
nice idea, let me tinker with this
There was a problem hiding this comment.
Okay I tested this idea. It works well if we do not parse the response but can just pass it along. Then we can hoist the method to the base class. So it doesn't work for all methods but should cut down on duplication
|
Looks good overall, but you would need to run the pre-commits to fix the linting issues. We also need some tests for the async client. |
Add an async client to the compass sdk. Was playing around with the code, there is probably more logic that can be extracted. If you do not feel this is the right direction please provide guidance how you would like this to look like.