Add async support

[Andrew-Chen-Wang]

Fixes #98

Adds asyncio support for API. Does not support CLI (does not make sense).

I haven't tested file upload. aiohttp does not include the files parameter like requests, so I'm using requests' private utility function in the ApiRequestor

Usage (notice the a prefix, used in CPython lib, Django, and other libs):

import openai

openai.api_key = "sk-..."

async def main():
    await openai.Completion.acreate(prompt="This is a test", engine="text-ada-001")

Installation of aiohttp can include speedups that aren't included in setup.py. aiohttp is a required package, but it doesn't have to be. We'd just need to remove the typing.

[MaxTretikov]

Love this addition, hope it gets merged. Wanted to add that I seem to have been getting Unclosed client session errors when using this:

client_session: <aiohttp.client.ClientSession object at 0x104306b00>
Unclosed client session

[Andrew-Chen-Wang]

Thanks for catching that @MaxTretikov ! This should be resolved in the latest commit

[ddeville]

Thank you so much @Andrew-Chen-Wang, this was quite the effort!

I've left a few comments, mostly on the APIRequestor since it's the most interesting part imo.

An obvious issue when looking at this patch is that it introduces a ton of duplication but that's usually what happens when wanting to provide both a sync and async library without requiring callers to use async_to_sync or the like. Another option I've seen others take is to offer different classes entirely for the aio version of the library so that the current sync ones don't need to change but we'd basically end up with even more duplication...

Maybe there's a future world where this library does become entirely async (it's mostly IO bound anyway) and callers who want sync then need to run the task to completion in an event loop on a thread of their own. I'm not sure, but for now I personally feel like this approach is totally fine.

[ddeville]

aiohttp has MultipartWriter for this use case. Not as trivial as just passing a dictionary of files as with requests but I think that'd be the proper way to do this.

[Andrew-Chen-Wang]

I think the preparation of the data will be fairly different between requests and aiohttp causing more duplication of code. I'm planning of essentially copying _encode_files(albeit the actual appending of files is different). Is that alright?

Also, feel free to add my fork as a remote and commit freely. I've enabled maintainers to be able to commit to my fork.

[ddeville]

Makes sense. I took a quick look at MultipartWriter and the implementation of _encode_files and while it should work for us here that's a fair amount of somewhat brittle code that could be easy to mess up. Maybe adding a comment/TODO above this line explaining what is going on is good enough for now.

[ddeville]

At first I thought this was just a global but after learning more about Context and ContextVar I realized that it enabled the thing I was scared this would prevent: using different sessions, by making a copy of the current context, setting the new session and running the code through the new context, thus having the openai module picking the new context without impacting other code that will still get the other one.

This might be worth putting as an example in the readme since it was not clear to me that this was possible without first reading more about ContextVar.

[Andrew-Chen-Wang]

This is already addressed in the README I believe in the async section. Please add suggestions. I've also added a comment in the source code itself.

[ddeville]

That's probably fine I agree. I think it's mostly a matter of knowing how ContextVar works which I expect people dealing to async code to do (and that I didn't initially :))

[ddeville]

Another option could be to move this to extras_require under an async feature and then we would raise an exception in api_requestor.py if we can't import aiohttp. Although it's also fine to always require it tbh.

[Andrew-Chen-Wang]

To be fair, it would be better, but typing would be a pain for mypy users. In redis-py, we install async-timeout, even if you only need the sync Redis

[ddeville]

Yeah fair enough.

[Andrew-Chen-Wang]

Thanks for your time and the review @ddeville !

[ddeville]

Alright, there might be a couple of small pieces to update but I think this is a very good (huge) first step.

Let's do this!

[ddeville]

Would love for @hallacy to take a look before we merge though.

[hallacy]

Love it! Lets merge it

[Andrew-Chen-Wang]

thanks @ddeville and @hallacy !!

in the future to possibly prevent further duplication of code, I highly recommend two resources:

• SansIO approach to implementing a sync/async lib (ex impl)
• A simpler method can be found in redis-py itself. In this method, we have the commands folder (similar to the api_resources folder) and the redis.Redis and redis.asyncio.Redis class (similar to ApiRequestor). The Redis classes implement their own execute_command method and a command from api_resources runs execute_command. The reason for this PR's amount of duplication of code is because of útil.convert_to_openai_object. If that function was instead moved to ApiRequestor.request, you're left with a mixin that is potentially compatible for both sync and async classes. But it can be tricky depending on future implementation and CLI compatibility

[ddeville]

Thanks for the links @Andrew-Chen-Wang this is super interesting. From a quick glance, it looks like it works more or less like this:

class BaseExecutor:
    def execute_cmd(...):
        ...

class SyncExecutor(BaseExecutor):
    def execute_cmd(...):
        return get(...)

class AsyncExecutor(BaseExecutor):
    async def execute_cmd(...):  # not sure how typing works here though since they return different types
        return await get(...)

class Model:
    def __init__(self, client: BaseExecutor):
        self.execute_cmd = client.execute_cmd
   
   def something(self, ...):
       request = ...
       return self.execute_cmd(request)

And then you would use it like that

# from a sync context
def main():
    model = Model(SyncExecutor())
    ret = model.something()

# from an async context
async def main():
    model = Model(AsyncExecutor())
    ret = await model.something()

(so basically BaseExecutor is a mixin and here convert_to_openai_object is a problem because it is called in the calling code rather than in ApiRequestor which means that Model would need to be able to await the result if the executor was async rather than just returning the coroutine?)

Am I reading this right?

The only issue I can think of is that the methods on Model are not marked as async but still return coroutines in the async case which is not super clear and might lead to problems where callers forget to await on their result. Has this been a problem in your experience?

[Andrew-Chen-Wang]

You got it right!

To answer the question on typing, in redis-py, we make a type Union with Awaitable and result type.

However the reason I make these suggestions is because the example usage you've got it quite unwieldy, hence a preference for the current implementation in this PR. A better implementation would be to have something similar to the Redis class, like ApiRequestor but named OpenAI, that could call commands like OpenAI().Completion(...)

But that's essentially a complete rewrite that I'm. not quite for at the moment. Would prefer to have the PR merged as it seems like a complete rewrite would be necessary (looking at the stripe sdk rn which this lib is forked from; for reference: stripe/stripe-python#715 (comment))

[ksromero]

When is the release of this?

[thejaminator]

not sure why the "content" part was removed?