openai / openai-python

The official Python library for the OpenAI API
https://pypi.org/project/openai/
Apache License 2.0
23.16k stars 3.27k forks source link

fix(client/async): replace asyncify with asyncio.to_thread (#1827) #1828

Closed jeongsu-an closed 2 weeks ago

jeongsu-an commented 4 weeks ago

Changes being requested

This Pull Request addresses an issue where AnyIO worker threads do not terminate properly when using asyncify(get_platform) in an asynchronous context. This can cause tests to hang, especially when using pytest, due to non-daemon worker threads remaining alive.

Changes Made:

Additional context & links

1827

RobertCraigie commented 4 weeks ago

thanks for the PR, can we instead close the worker threads after they're done? It also sounds like this could be an underlying issue with anyio or just that we're using it incorrectly... do you have any insight there?

jeongsu-an commented 4 weeks ago

Thank you for reviewing my PR.

thanks for the PR, can we instead close the worker threads after they're done?

After investigating, it appears that we cannot close the worker threads after they're done when using AnyIO.

AnyIO does not provide an API to close or terminate these worker threads. According to the AnyIO documentation:

"Note, however, that the thread will still continue running – only its outcome will be ignored."

Source: AnyIO Documentation on Threads

It also sounds like this could be an underlying issue with anyio or just that we're using it incorrectly... do you have any insight there?

The issue occurs because AnyIO's worker threads are non-daemon threads. When AnyIO creates a worker thread, it does not set the daemon attribute, so it inherits the daemon status from the current thread, which is the main thread. Since the main thread is non-daemon (daemon=False), the worker threads are also non-daemon. anyio/src/anyio/_backends/_asyncio.py cpython/Lib/threading.py

image

Non-daemon threads prevent the program from exiting until they have completed. In AnyIO, these worker threads remain alive even after their tasks are done, causing the program to hang, especially in testing environments like pytest.

To resolve this, I replaced asyncify(get_platform) with asyncio.to_thread(get_platform). The asyncio.to_thread function creates daemon threads, which terminate when the main program exits, preventing the hanging issue.

Regarding Deleting asyncify:

I've confirmed that the asyncify function is only used in this part of the code. If it's acceptable, I can remove the related utility function and update the commit accordingly. Please let me know if this is okay.

afbarbaro commented 3 weeks ago

Any update on this? It's caused issues on a github actions we're using and this just makes it hang.

spokeydokeys commented 3 weeks ago

I have been suffering from the same bug as @jeongsu-an when using the openai python library in github workflows. Specifically, the process stalls at the end of the script I'm running and if I print out threads there is a leftover AnyIO thread that is not closing.

I tested the behavior in github workflows using @jeongsu-an's PR branch and it fixes the issues I was having and does not introduce any other issues.

RobertCraigie commented 3 weeks ago

would anyone be able to share a reproduction for this issue? I can't reproduce it with this script running both standard python and pytest

import asyncio
import anyio
import anyio.to_thread

from openai._base_client import get_platform

async def test_main() -> None:
    result = await anyio.to_thread.run_sync(get_platform)
    print(result)

asyncio.run(test_main())
spokeydokeys commented 2 weeks ago

Include nest_asyncio.apply() and your code will trigger the issue.

import asyncio
import anyio
import anyio.to_thread
import nest_asyncio

from openai._base_client import get_platform

async def test_main() -> None:
    result = await anyio.to_thread.run_sync(get_platform)
    print(result)

nest_asyncio.apply()
asyncio.run(test_main())
RobertCraigie commented 2 weeks ago

Okay thanks @spokeydokeys, I can confirm that reproduces the issue.

RobertCraigie commented 2 weeks ago

closing in favour of #1853