Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,11 @@ def __init__(
def concurrency_mode(self) -> ClientConcurrencyMode:
return self._mode

@property
def limits(self) -> httpx.Limits:
"""Connection pool limits derived from ``max_parallel_requests`` at construction time."""
return self._limits

@abstractmethod
def _build_headers(self, extra_headers: dict[str, str]) -> dict[str, str]:
"""Build provider-specific request headers."""
Expand All @@ -97,10 +102,12 @@ def _get_sync_client(self) -> httpx.Client:
raise RuntimeError("Model client is closed.")
if self._client is None:
if self._transport is None:
self._transport = create_retry_transport(self._retry_config, strip_rate_limit_codes=False)
inner = lazy.httpx.HTTPTransport(limits=self._limits)
self._transport = create_retry_transport(
self._retry_config, strip_rate_limit_codes=False, transport=inner
)
self._client = lazy.httpx.Client(
transport=self._transport,
limits=self._limits,
timeout=lazy.httpx.Timeout(self._timeout_s),
)
return self._client
Expand All @@ -113,10 +120,12 @@ def _get_async_client(self) -> httpx.AsyncClient:
raise RuntimeError("Model client is closed.")
if self._aclient is None:
if self._transport is None:
self._transport = create_retry_transport(self._retry_config, strip_rate_limit_codes=True)
inner = lazy.httpx.AsyncHTTPTransport(limits=self._limits)
self._transport = create_retry_transport(
self._retry_config, strip_rate_limit_codes=True, transport=inner
)
self._aclient = lazy.httpx.AsyncClient(
transport=self._transport,
limits=self._limits,
timeout=lazy.httpx.Timeout(self._timeout_s),
)
return self._aclient
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,13 @@

import logging
from dataclasses import dataclass, field
from typing import TYPE_CHECKING

from httpx_retries import Retry, RetryTransport

if TYPE_CHECKING:
import httpx

logger = logging.getLogger(__name__)

# 429 must not be retried at the transport layer so that rate-limit signals
Expand Down Expand Up @@ -37,6 +41,7 @@ def create_retry_transport(
config: RetryConfig | None = None,
*,
strip_rate_limit_codes: bool = True,
transport: httpx.BaseTransport | httpx.AsyncBaseTransport | None = None,
) -> RetryTransport:
"""Build an httpx ``RetryTransport`` from a :class:`RetryConfig`.

Expand All @@ -51,6 +56,12 @@ def create_retry_transport(
AIMD feedback loop. When ``False`` (used by the sync engine, which has
no salvage queue), 429 is kept in the retry list so the transport layer
retries it transparently.
transport: Optional pre-configured transport to pass directly to
``RetryTransport``. Pass ``httpx.HTTPTransport`` for sync clients or
``httpx.AsyncHTTPTransport`` for async clients — typically with a custom
``limits=`` — so that the connection pool is sized correctly. When
``None`` (default), ``RetryTransport`` creates its own default pools for
both sync and async requests.
"""
cfg = config or RetryConfig()
status_codes = cfg.retryable_status_codes
Expand All @@ -72,4 +83,4 @@ def create_retry_transport(
respect_retry_after_header=True,
allowed_methods=Retry.RETRYABLE_METHODS | frozenset(["POST"]),
)
return RetryTransport(retry=retry)
return RetryTransport(transport=transport, retry=retry)
Original file line number Diff line number Diff line change
Expand Up @@ -289,3 +289,23 @@ async def test_acompletion_lazy_initializes_async_client(

mock_ctor.assert_called_once()
assert result.message.content == "lazy result"


# ---------------------------------------------------------------------------
# Connection pool size regression tests (issue #459)
# ---------------------------------------------------------------------------


def test_client_limits_respect_max_parallel_requests() -> None:
"""Connection pool limits must reflect max_parallel_requests (regression for issue #459).

pool_max = max(32, 2 * max_parallel_requests) = max(32, 600) = 600
"""
client = OpenAICompatibleClient(
provider_name=_OPENAI_PROVIDER,
endpoint=_OPENAI_ENDPOINT,
api_key="sk-test",
max_parallel_requests=300,
concurrency_mode=ClientConcurrencyMode.SYNC,
)
assert client.limits.max_connections == 600
Comment on lines +299 to +311
Copy link
Contributor

Choose a reason for hiding this comment

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

P1 Regression test doesn't verify the actual fix

client.limits.max_connections reads self._limits — a value that was computed correctly even before this PR. The bug was that _limits was never forwarded to the underlying httpcore connection pool. This test passes on the old (broken) code just as easily as on the new code, so it provides no regression protection.

To actually verify the fix, the test needs to trigger lazy transport initialization and then inspect the pool's _max_connections. The PR description lists exactly these missing tests (all four checkboxes are unchecked):

  • test_sync_client_pool_size_respects_max_parallel_requests
  • test_async_client_pool_size_respects_max_parallel_requests
  • test_create_retry_transport_forwards_sync_transport
  • test_create_retry_transport_forwards_async_transport

The existing test is fine as a unit-level sanity check for the limits property, but it should not be the sole regression guard for this bug.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/data-designer-engine/tests/engine/models/clients/test_native_http_clients.py
Line: 299-311

Comment:
**Regression test doesn't verify the actual fix**

`client.limits.max_connections` reads `self._limits` — a value that was computed correctly even *before* this PR. The bug was that `_limits` was never forwarded to the underlying `httpcore` connection pool. This test passes on the old (broken) code just as easily as on the new code, so it provides no regression protection.

To actually verify the fix, the test needs to trigger lazy transport initialization and then inspect the pool's `_max_connections`. The PR description lists exactly these missing tests (all four checkboxes are unchecked):
- `test_sync_client_pool_size_respects_max_parallel_requests`
- `test_async_client_pool_size_respects_max_parallel_requests`
- `test_create_retry_transport_forwards_sync_transport`
- `test_create_retry_transport_forwards_async_transport`

The existing test is fine as a unit-level sanity check for the `limits` property, but it should not be the sole regression guard for this bug.

How can I resolve this? If you propose a fix, please make it concise.

Loading