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

OpenAIChatCompletionClient should be a ContextManager #5108

Open
Leon0402 opened this issue Jan 18, 2025 · 1 comment
Open

OpenAIChatCompletionClient should be a ContextManager #5108

Leon0402 opened this issue Jan 18, 2025 · 1 comment

Comments

@Leon0402
Copy link
Contributor

Right now OpenAIChatCompletionClient is no Context Manager and instead relies on a deletion method for cleanup. In particular it does:

So we rely on __del__ for cleanup of resources, which is not too great. There are many posts about the danger of it, although I am not quite sure what exactly applies to a modern python version as there has been some PEPs improving the situation. Nevertheless, there seems to be a common agreement that context managers are always preferred for cleaning up resources. As AsyncAPIClient already is a context manager, we would just need to make OpenAIChatCompletionClient one as well (an async one to be more concrete).

@ekzhu
Copy link
Collaborator

ekzhu commented Jan 20, 2025

Good point. We may end up having states inside the client that requires clean up. Consider perhaps a close() method for the client class as the first step?

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

No branches or pull requests

2 participants