openai-python: Memory leak in the chat completion `create` method

Wouldn’t “moving the client outside of the request handler” be the same thing as a module level client?

From the README, I figured the network resources are getting cleaned up when the OpenAI client gets garbage collected. Sounds like this isn’t necessarily true?

This might be outside of the scope of this library but I’m also trying to understand how this fits in your typical backend app.

It seems like using a module scoped client is heavily discouraged but so is instantiating a new client with every request (ie a FastAPI dependency). What is the other option here?

You shouldn’t be instantiating a new client for every request, if you move the client outside of the request handler you shouldn’t see any memory leaks.

I did manage to replicate the memory leak you’re seeing with your example and moving the client instantiation outside of the function resulted in stable memory usage.

@RobertCraigie

How can we implement an async client with fastapi correctly?

Is it safe to implement like this (singleton approach) ? https://github.com/tiangolo/fastapi/discussions/8301#discussioncomment-5150759

Another problem is that we have multiple api keys like below. How can we handle this nicely if we have to instantiate a client once as mentioned in this issue https://github.com/openai/openai-python/issues/874?

    client = AsyncAzureOpenAI(
        api_key=api_keys_list[random_index],
        ...

@brian-goo you can create a single client and use FastAPI app events to close it when the server is stopped and then use client.with_options(api_key='foo') to use different API keys for different requests.

Anyway, it really does not matter as its not in the scope of this issue. Focus is that there seems to be memory leaks, regardless of how the library is used.

@RobertCraigie that’s super helpful, both of those patterns. Didn’t realize you could pass in the http_client for the client.

I pushed client.close() a couple of hours ago BTW, and that solved my memory leak issues. Thanks for your help on this!

It’s a bit scary to potentially serve 1 user’s request with another user’s api key, even if that can only happen due to a bug in my code

This is a fair point, I do think there are ways you could structure your code to make this practically impossible to happen though. Even for anyone making changes to your codebase for the first time.

For example, instead of using .with_options() yourself, you could define a helper function that you use whenever you need to make an OpenAI request, e.g.

# utils/openai_client.py
from openai import AsyncOpenAI

_client = AsyncOpenAI(
  # provide a dummy API key so that requests made directly will always fail
  api_key='<this client should never be used directly!>',
)

def get_openai(user: User) -> AsyncOpenAI:
  return _client.with_options(api_key=user.openai_api_key)

# backend.py

async def handler():
  user = get_user()

  completion = await get_openai(user).chat.completions.create(...)
  # ...

It’s also worth noting that you could use your existing pattern and avoid memory leaks entirely by sharing the http client instance if you really don’t want to use with_options, the only risk here is that you could instantiate a client and forget to pass in your http client which then gives you memory leaks again.

import httpx
from openai import AsyncOpenAI

http_client = httpx.AsyncClient()

async def handler(api_key):
  openai_client = AsyncOpenAI(api_key=api_key, http_client=http_client)

I would argue though that the first snippet in this comment is less likely to result in accidentally using the incorrect API key because you could accidentally just write AsyncOpenAI() and then it will use your API key from your environment.

However with a helper function similar to get_openai() you’re encoding the setup of your system directly into it so instead of having to know to pass api_key=user_api_key you’re forced to by your own function definition.

The only way you’ll accidentally use the wrong API key is by forgetting to use this helper function, which it may be possible to define lint rules for (I don’t know of an easy way to do this off the top of my head).

Hi!

Started noticing a weird behaviour in our api server and after digging a bit deeper I think I found the underlaying issue with memory leaks. The way classes uses references to self won’t allow for resources to be freed as classes have objects that in turn have references to the class it self.

After testing a bit with forced close functions that frees resources, it runs deinit and frees memory as it should.