From 5c138f4a7947e5b4aae8779fae78ca51269b355a Mon Sep 17 00:00:00 2001 From: Stainless Bot <107565488+stainless-bot@users.noreply.github.com> Date: Tue, 12 Dec 2023 14:53:07 +0000 Subject: [PATCH] refactor(client): simplify cleanup (#966) This removes Client.__del__, but users are not expected to call this directly. --- pyproject.toml | 2 +- src/openai/__init__.py | 7 ------- src/openai/_base_client.py | 26 ++++++++++++++++++++------ src/openai/_client.py | 24 ------------------------ tests/test_client.py | 23 ++--------------------- 5 files changed, 23 insertions(+), 59 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index fab8bf4250..57fe2afc6d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -84,7 +84,7 @@ typecheck = { chain = [ ]} "typecheck:pyright" = "pyright" "typecheck:verify-types" = "pyright --verifytypes openai --ignoreexternal" -"typecheck:mypy" = "mypy --enable-incomplete-feature=Unpack ." +"typecheck:mypy" = "mypy ." [build-system] requires = ["hatchling"] diff --git a/src/openai/__init__.py b/src/openai/__init__.py index d90f777cdc..0d66b3c682 100644 --- a/src/openai/__init__.py +++ b/src/openai/__init__.py @@ -221,13 +221,6 @@ def _client(self, value: _httpx.Client) -> None: # type: ignore http_client = value - @override - def __del__(self) -> None: - try: - super().__del__() - except Exception: - pass - class _AzureModuleClient(_ModuleClient, AzureOpenAI): # type: ignore ... diff --git a/src/openai/_base_client.py b/src/openai/_base_client.py index bbbb8a54ab..04a20bfd91 100644 --- a/src/openai/_base_client.py +++ b/src/openai/_base_client.py @@ -5,6 +5,7 @@ import time import uuid import email +import asyncio import inspect import logging import platform @@ -672,9 +673,16 @@ def _idempotency_key(self) -> str: return f"stainless-python-retry-{uuid.uuid4()}" +class SyncHttpxClientWrapper(httpx.Client): + def __del__(self) -> None: + try: + self.close() + except Exception: + pass + + class SyncAPIClient(BaseClient[httpx.Client, Stream[Any]]): _client: httpx.Client - _has_custom_http_client: bool _default_stream_cls: type[Stream[Any]] | None = None def __init__( @@ -747,7 +755,7 @@ def __init__( custom_headers=custom_headers, _strict_response_validation=_strict_response_validation, ) - self._client = http_client or httpx.Client( + self._client = http_client or SyncHttpxClientWrapper( base_url=base_url, # cast to a valid type because mypy doesn't understand our type narrowing timeout=cast(Timeout, timeout), @@ -755,7 +763,6 @@ def __init__( transport=transport, limits=limits, ) - self._has_custom_http_client = bool(http_client) def is_closed(self) -> bool: return self._client.is_closed @@ -1135,9 +1142,17 @@ def get_api_list( return self._request_api_list(model, page, opts) +class AsyncHttpxClientWrapper(httpx.AsyncClient): + def __del__(self) -> None: + try: + # TODO(someday): support non asyncio runtimes here + asyncio.get_running_loop().create_task(self.aclose()) + except Exception: + pass + + class AsyncAPIClient(BaseClient[httpx.AsyncClient, AsyncStream[Any]]): _client: httpx.AsyncClient - _has_custom_http_client: bool _default_stream_cls: type[AsyncStream[Any]] | None = None def __init__( @@ -1210,7 +1225,7 @@ def __init__( custom_headers=custom_headers, _strict_response_validation=_strict_response_validation, ) - self._client = http_client or httpx.AsyncClient( + self._client = http_client or AsyncHttpxClientWrapper( base_url=base_url, # cast to a valid type because mypy doesn't understand our type narrowing timeout=cast(Timeout, timeout), @@ -1218,7 +1233,6 @@ def __init__( transport=transport, limits=limits, ) - self._has_custom_http_client = bool(http_client) def is_closed(self) -> bool: return self._client.is_closed diff --git a/src/openai/_client.py b/src/openai/_client.py index 8cf0fa6797..dacadf5aff 100644 --- a/src/openai/_client.py +++ b/src/openai/_client.py @@ -3,7 +3,6 @@ from __future__ import annotations import os -import asyncio from typing import Any, Union, Mapping from typing_extensions import Self, override @@ -205,16 +204,6 @@ def copy( # client.with_options(timeout=10).foo.create(...) with_options = copy - def __del__(self) -> None: - if not hasattr(self, "_has_custom_http_client") or not hasattr(self, "close"): - # this can happen if the '__init__' method raised an error - return - - if self._has_custom_http_client: - return - - self.close() - @override def _make_status_error( self, @@ -415,19 +404,6 @@ def copy( # client.with_options(timeout=10).foo.create(...) with_options = copy - def __del__(self) -> None: - if not hasattr(self, "_has_custom_http_client") or not hasattr(self, "close"): - # this can happen if the '__init__' method raised an error - return - - if self._has_custom_http_client: - return - - try: - asyncio.get_running_loop().create_task(self.close()) - except Exception: - pass - @override def _make_status_error( self, diff --git a/tests/test_client.py b/tests/test_client.py index cd374a49db..92998769d8 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -591,14 +591,6 @@ def test_absolute_request_url(self, client: OpenAI) -> None: ) assert request.url == "https://myapi.com/foo" - def test_client_del(self) -> None: - client = OpenAI(base_url=base_url, api_key=api_key, _strict_response_validation=True) - assert not client.is_closed() - - client.__del__() - - assert client.is_closed() - def test_copied_client_does_not_close_http(self) -> None: client = OpenAI(base_url=base_url, api_key=api_key, _strict_response_validation=True) assert not client.is_closed() @@ -606,9 +598,8 @@ def test_copied_client_does_not_close_http(self) -> None: copied = client.copy() assert copied is not client - copied.__del__() + del copied - assert not copied.is_closed() assert not client.is_closed() def test_client_context_manager(self) -> None: @@ -1325,15 +1316,6 @@ def test_absolute_request_url(self, client: AsyncOpenAI) -> None: ) assert request.url == "https://myapi.com/foo" - async def test_client_del(self) -> None: - client = AsyncOpenAI(base_url=base_url, api_key=api_key, _strict_response_validation=True) - assert not client.is_closed() - - client.__del__() - - await asyncio.sleep(0.2) - assert client.is_closed() - async def test_copied_client_does_not_close_http(self) -> None: client = AsyncOpenAI(base_url=base_url, api_key=api_key, _strict_response_validation=True) assert not client.is_closed() @@ -1341,10 +1323,9 @@ async def test_copied_client_does_not_close_http(self) -> None: copied = client.copy() assert copied is not client - copied.__del__() + del copied await asyncio.sleep(0.2) - assert not copied.is_closed() assert not client.is_closed() async def test_client_context_manager(self) -> None: