Skip to content

Commit 088e037

Browse files
przemekborutaclaude
andcommitted
refactor: address review feedback — public limits property, clean tests
- Drop three tests from test_retry.py that reached into private attributes of third-party RetryTransport (_sync_transport, _async_transport); the end-to-end contract is covered by the pool-size regression test - Expose a public `limits` property on HttpModelClient so tests and diagnostic code can assert the pool configuration without walking private attribute chains across three libraries - Replace two private-chain pool assertions with a single `client.limits.max_connections == 600` check against the new property - Trim "inner" from the transport docstring entry (nabinchha suggestion) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Przemysław <przemekboruta@interia.pl>
1 parent e558dd7 commit 088e037

4 files changed

Lines changed: 12 additions & 58 deletions

File tree

packages/data-designer-engine/src/data_designer/engine/models/clients/adapters/http_model_client.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,11 @@ def __init__(
8383
def concurrency_mode(self) -> ClientConcurrencyMode:
8484
return self._mode
8585

86+
@property
87+
def limits(self) -> httpx.Limits:
88+
"""Connection pool limits derived from ``max_parallel_requests`` at construction time."""
89+
return self._limits
90+
8691
@abstractmethod
8792
def _build_headers(self, extra_headers: dict[str, str]) -> dict[str, str]:
8893
"""Build provider-specific request headers."""

packages/data-designer-engine/src/data_designer/engine/models/clients/retry.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ def create_retry_transport(
5656
AIMD feedback loop. When ``False`` (used by the sync engine, which has
5757
no salvage queue), 429 is kept in the retry list so the transport layer
5858
retries it transparently.
59-
transport: Optional pre-configured inner transport to pass directly to
59+
transport: Optional pre-configured transport to pass directly to
6060
``RetryTransport``. Pass ``httpx.HTTPTransport`` for sync clients or
6161
``httpx.AsyncHTTPTransport`` for async clients — typically with a custom
6262
``limits=`` — so that the connection pool is sized correctly. When

packages/data-designer-engine/tests/engine/models/clients/test_native_http_clients.py

Lines changed: 6 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -296,42 +296,16 @@ async def test_acompletion_lazy_initializes_async_client(
296296
# ---------------------------------------------------------------------------
297297

298298

299-
def test_sync_client_pool_size_respects_max_parallel_requests() -> None:
300-
"""Connection pool max_connections must be 2*max_parallel_requests, not the httpx default of 100."""
301-
client = OpenAICompatibleClient(
302-
provider_name=_OPENAI_PROVIDER,
303-
endpoint=_OPENAI_ENDPOINT,
304-
api_key="sk-test",
305-
max_parallel_requests=300,
306-
concurrency_mode=ClientConcurrencyMode.SYNC,
307-
)
308-
with patch(_SYNC_CLIENT_PATCH):
309-
client._get_sync_client()
310-
311-
# Attribute chain explained:
312-
# RetryTransport._sync_transport → the httpx.HTTPTransport we injected
313-
# HTTPTransport._pool → the underlying httpcore.ConnectionPool
314-
# ConnectionPool._max_connections → the hard cap configured via Limits
315-
# pool_max = max(32, 2 * 300) = 600
316-
assert client._transport._sync_transport._pool._max_connections == 600
299+
def test_client_limits_respect_max_parallel_requests() -> None:
300+
"""Connection pool limits must reflect max_parallel_requests (regression for issue #459).
317301
318-
319-
@pytest.mark.asyncio
320-
async def test_async_client_pool_size_respects_max_parallel_requests() -> None:
321-
"""Async connection pool max_connections must be 2*max_parallel_requests, not the httpx default of 100."""
302+
pool_max = max(32, 2 * max_parallel_requests) = max(32, 600) = 600
303+
"""
322304
client = OpenAICompatibleClient(
323305
provider_name=_OPENAI_PROVIDER,
324306
endpoint=_OPENAI_ENDPOINT,
325307
api_key="sk-test",
326308
max_parallel_requests=300,
327-
concurrency_mode=ClientConcurrencyMode.ASYNC,
309+
concurrency_mode=ClientConcurrencyMode.SYNC,
328310
)
329-
with patch(_ASYNC_CLIENT_PATCH):
330-
client._get_async_client()
331-
332-
# Attribute chain explained:
333-
# RetryTransport._async_transport → the httpx.AsyncHTTPTransport we injected
334-
# AsyncHTTPTransport._pool → the underlying httpcore.AsyncConnectionPool
335-
# AsyncConnectionPool._max_connections → the hard cap configured via Limits
336-
# pool_max = max(32, 2 * 300) = 600
337-
assert client._transport._async_transport._pool._max_connections == 600
311+
assert client.limits.max_connections == 600

packages/data-designer-engine/tests/engine/models/clients/test_retry.py

Lines changed: 0 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -82,28 +82,3 @@ def test_create_retry_transport_default_codes_exclude_429() -> None:
8282
"""Default retryable_status_codes do not include 429 regardless of strip flag."""
8383
transport = create_retry_transport(strip_rate_limit_codes=False)
8484
assert 429 not in transport.retry.status_forcelist
85-
86-
87-
def test_create_retry_transport_forwards_sync_transport() -> None:
88-
"""Provided sync transport is used directly rather than replaced with a default pool."""
89-
import httpx
90-
91-
inner = httpx.HTTPTransport(limits=httpx.Limits(max_connections=600))
92-
transport = create_retry_transport(transport=inner)
93-
assert transport._sync_transport is inner
94-
95-
96-
def test_create_retry_transport_forwards_async_transport() -> None:
97-
"""Provided async transport is used directly rather than replaced with a default pool."""
98-
import httpx
99-
100-
inner = httpx.AsyncHTTPTransport(limits=httpx.Limits(max_connections=600))
101-
transport = create_retry_transport(transport=inner)
102-
assert transport._async_transport is inner
103-
104-
105-
def test_create_retry_transport_no_transport_creates_defaults() -> None:
106-
"""Without a transport argument both sync and async default pools are created."""
107-
transport = create_retry_transport()
108-
assert transport._sync_transport is not None
109-
assert transport._async_transport is not None

0 commit comments

Comments
 (0)