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

Update README for fastapi integration #912

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

brian-goo
Copy link

@brian-goo brian-goo commented Dec 1, 2023

  • I understand that this repository is auto-generated and my pull request may not be merged

Changes being requested

Additional context & links

I guess how to integrate with frameworks like FastAPI should be documented to avoid confusion.
#820

@brian-goo brian-goo requested a review from a team as a code owner December 1, 2023 03:13
@antont
Copy link

antont commented Dec 1, 2023

Maybe that's overly complex, though?

It also works to just to instantiate a client in some module, and use that in places where you need it, without the singleton class nor depends or anything such. If you don't need a connection pool or something.

Copy link
Author

@brian-goo brian-goo left a comment

Choose a reason for hiding this comment

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

Well, I kind of intentionally put this right below Module-level client so that this can be implemented as a dependency in FastAPI with a singleton approach.
We need a full-featured async client for our production server and I believe somebody else needs it as well.

If a complexity is a problem, I guess we could add a simplified example to this.

@antont
Copy link

antont commented Dec 1, 2023

Well, I kind of intentionally put this right below Module-level client

Ah sorry, I missed that it was there, was just reading the diff.

I think this way it makes a lot of sense, to have the trivially simple option, and then this fully featured way.

@antont
Copy link

antont commented Dec 1, 2023

We need a full-featured async client for our production server and I believe somebody else needs it as well.

Actually, if you don't mind me asking, what's the need exactly? Your code seems to do a client per session, no? Why?

I'm also using FastAPI and so far with just the client-in-module approach, and am not (yet) aware of problems it would introduce. A single client gets reused within a process, and is closed when the process shuts down. AFAIK reusing the same client for multiple sessions is fine.

I'm not saying that there would not be problems, would just like to learn what exactly, and I guess that would be useful for the documentation too.

@RobertCraigie
Copy link
Collaborator

Like @antont mentioned you should be able to share the same client between requests, is there a reason you set one up per-session?

This then simplifies the code a lot, so it should look something like this:

from contextlib import asynccontextmanager
from fastapi import FastAPI
from openai import AsyncOpenAI

client = AsyncOpenAI()

@asynccontextmanager
async def lifespan(app: FastAPI):
    yield
    await client.close()

app = FastAPI(lifespan=lifespan)

Copy link
Author

@brian-goo brian-goo left a comment

Choose a reason for hiding this comment

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

@RobertCraigie
Thank you for your comments.

But the class AioClient in the example is a wrapper to handle the state of a single client.
The wrapper class sets a new client, a prop of itself, when the server starts and returns the initialized
client when __call__ gets called, ie its instance (aio_client) gets called as a FastAPI's dependency.
The wrapper class will set a new client after initialization only if the client prop is None for some reason.
You can check the object identity of aio_client in async def foo with python's id() etc.

The reason why I included the wrapper class is that a wrapper makes it easier to handle states of multiple initialized clients
for multiple api keys etc AND fits into the dependency injection system of FastAPI.

If you want to simplify the code it is ok with me, but I guess it'd be better to add another example for dependency injection to the example above.
We use it since the same client is required for a bunch of routes and I believe it would be helpful
for most of FastAPI users.

If you are ok with this I will fix the proposed example in this PR so that two examples would be there for FastAPi.

from openai import AsyncOpenAI
from fastapi import Depends, FastAPI

client = AsyncOpenAI()

def get_client() -> AsyncOpenAI:
    return client

app = FastAPI()

@app.get("/foo")
async def foo(client: AsyncOpenAI = Depends(get_client)) -> str:
    res = await client.chat.completions.create(...)
     ...
    return "ok"

https://fastapi.tiangolo.com/tutorial/dependencies/
https://fastapi.tiangolo.com/advanced/events/

@antont
Copy link

antont commented Dec 2, 2023

@brian-goo The reason why I included the wrapper class is that a wrapper makes it easier to handle states of multiple initialized clients

for multiple api keys etc AND fits into the dependency injection system of FastAPI.

Right, figures.

I'd think that a more complex example is useful, for example one that indeed gives different clients for different request handlers.

The minimal case seems weird as it does not really have any benefit over just using a single client without the Dependency.

The client supports using different API keys for different requests, though, with a single client, so I'm not sure what would be a good simple example case for a wrapper.

Just my 2 cents, lib authors may see this differently.

https://github.com/tiangolo/fastapi/discussions/8301
https://fastapi.tiangolo.com/advanced/events/
"""
client: AsyncOpenAI | None = None

Choose a reason for hiding this comment

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

It's important to note that the use of | as a Union operator is supported starting from Python 3.10, as detailed in PEP 604. To ensure broader compatibility, especially with versions as early as Python 3.7, I recommend updating the code to use an alternative approach that aligns with the capabilities of these earlier Python versions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants