feat: Native Anthropic adapter with shared HTTP client infrastructure#426
feat: Native Anthropic adapter with shared HTTP client infrastructure#426
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…provements - Wrap all LiteLLM router calls in try/except to normalize raw exceptions into canonical ProviderError at the bridge boundary (blocking review item) - Extract reusable response-parsing helpers into clients/parsing.py for shared use across future native adapters - Add async image parsing path using httpx.AsyncClient to avoid blocking the event loop in agenerate_image - Add retry_after field to ProviderError for future retry engine support - Fix _to_int_or_none to parse numeric strings from providers - Create test conftest.py with shared mock_router/bridge_client fixtures - Parametrize duplicate image generation and error mapping tests - Add tests for exception wrapping across all bridge methods
…larity - Parse RFC 7231 HTTP-date strings in Retry-After header (used by Azure and Anthropic during rate-limiting) in addition to numeric delay-seconds - Clarify collect_non_none_optional_fields docstring explaining why f.default is None is the correct check for optional field forwarding - Add tests for HTTP-date and garbage Retry-After values
- Fix misleading comment about prompt field defaults in _IMAGE_EXCLUDE - Handle list-format detail arrays in _extract_structured_message for FastAPI/Pydantic validation errors - Document scope boundary for vision content in collect_raw_image_candidates
…el-facade-guts-pr2
…el-facade-guts-pr2
...s/data-designer-engine/src/data_designer/engine/models/clients/adapters/http_model_client.py
Outdated
Show resolved
Hide resolved
...s/data-designer-engine/src/data_designer/engine/models/clients/adapters/http_model_client.py
Show resolved
Hide resolved
...s/data-designer-engine/src/data_designer/engine/models/clients/adapters/http_model_client.py
Outdated
Show resolved
Hide resolved
...ta-designer-engine/src/data_designer/engine/models/clients/adapters/anthropic_translation.py
Outdated
Show resolved
Hide resolved
|
(AR) Warning: AnthropicClient not exported from adapters __init__.py
What: __all__ exports OpenAICompatibleClient but omits AnthropicClient, creating an inconsistent public surface. Why: Both are peer adapters in the factory routing table. The asymmetry suggests AnthropicClient is secondary, which does not match the design. Suggestion: |
|
(AR) Warning: Missing litellm_bridge env override test for anthropic provider
What: test_bridge_env_override_forces_bridge_for_openai_provider exists but there is no equivalent test for the anthropic provider type. Why: The env-var bypass is part of the routing contract. A refactor moving the check below the anthropic branch would go undetected. Suggestion: Add a parallel test verifying DATA_DESIGNER_MODEL_BACKEND=litellm_bridge overrides the anthropic provider to bridge. |
|
(AR) Warning: Missing ConnectionError and non-JSON response tests for AnthropicClient
What: test_openai_compatible.py has transport_connection_error and non_json_response tests; test_anthropic.py does not. Why: Symmetric test coverage ensures adapter-specific code (headers, payload building) does not short-circuit shared error paths. Suggestion: Add test_transport_connection_error_raises_provider_error and test_non_json_response_raises_provider_error to test_anthropic.py. |
|
(AR) Suggestion: Mock helpers duplicated across three test files
What: _mock_httpx_response, _make_sync_client, and _make_async_client are defined independently in multiple test files with different resp.text defaults. Why: AGENTS.md says shared fixtures belong in conftest.py. The text default difference (json.dumps vs empty string) may mask subtle bugs. Suggestion: |
|
(AR) Suggestion: Factory tests don't assert resolved API key is forwarded
What: Factory routing tests only check isinstance(client, ...) but don't verify the resolved API key reaches the client. Why: Testing only the return type exercises ~30% of the function's routing logic while leaving key resolution untested. Suggestion: Add assert client._api_key == 'resolved-key' in the routing tests. |
|
(AR) This PR adds a native Anthropic Messages API adapter with clean separation between translation logic and HTTP transport, extracts shared infrastructure into an HttpModelClient base class, and updates the factory routing. The review covered all 12 changed files (+2455/-236 lines), ran linting (ruff, all passed) and tests (135 passed in 3.50s). The architecture is well-executed — the stateless translation module is independently testable, the DRY refactoring of HttpModelClient removes real duplication without over-abstracting, and test coverage is thorough across translation, lifecycle, and error paths. The one critical finding is that Verdict: Ship it (with nits) — 1 critical, 5 warnings, 3 suggestions. |
- Handle /v1 in Anthropic endpoint gracefully to avoid path duplication - Add QUOTA_EXCEEDED provider error kind for credit/billing failures - Extend UNSUPPORTED_PARAMS detection for mutually exclusive params - Surface raw provider message in formatted errors for 400 status codes - Consolidate provider message helpers into single _attach_provider_message Made-with: Cursor
- Fix close() double-close of shared transport by closing self._transport directly instead of accessing private aclient._transport (critical) - Add TODO for threading.Lock → asyncio.Lock split (plan-346) - Remove unused _model_id from HttpModelClient and all callers - Export AnthropicClient from adapters __init__.py - Filter empty text blocks in translate_tool_result_content join - Move mock helpers to conftest.py with consistent json.dumps text default - Add __init__.py files to enable absolute imports from test conftest - Add bridge env override test for anthropic provider - Add ConnectionError and non-JSON response tests for AnthropicClient - Assert secret_resolver.resolve called with correct key ref in factory tests Made-with: Cursor
|
Addressed all review findings in b8add5d: Re: AnthropicClient not exported from adapters __init__.py (#426 (comment)) Re: Missing litellm_bridge env override test for anthropic provider (#426 (comment)) Re: Missing ConnectionError and non-JSON response tests for AnthropicClient (#426 (comment)) Re: Mock helpers duplicated across three test files (#426 (comment)) Re: Factory tests don't assert resolved API key is forwarded (#426 (comment)) |
| ) | ||
| return self._aclient | ||
|
|
||
| def close(self) -> None: |
There was a problem hiding this comment.
I think close() still leaves the real AsyncClient open after an async request path. I hit the live Anthropic path with acompletion(), then called close(), and client._aclient.is_closed stayed False while aclose() flips it to True. Maybe worth tightening close() so sync teardown fully releases async-owned resources too.
packages/data-designer-engine/src/data_designer/engine/models/errors.py
Outdated
Show resolved
Hide resolved
|
I ran some extra QA on this branch with live Anthropic smoke tests through both the native path and the LiteLLM bridge for text generation, tool calling, multimodal input, and an unsupported embedding call. Text and tool flows looked good on both paths, and native multimodal worked in the live check. Just out of curiosity, I also ran a small live latency check on Claude Sonnet; in that sample I did not see a consistent latency difference in favor of the native path. Not blocking, just sharing the extra validation data point. The unsupported-capability case still hits the generic error path noted inline above. |
|
quick docs question: now that |
| # releases the connection pool without re-closing the transport. | ||
| client.close() | ||
|
|
||
| async def aclose(self) -> None: |
There was a problem hiding this comment.
same transport leak as the close() one from earlier, but on the async side. close() got explicit transport cleanup in b8add5d but aclose() doesn't have it. since the transport is injected, httpx won't close it for you - needs the same capture-and-close pattern.
andreatgretel
left a comment
andreatgretel
left a comment
There was a problem hiding this comment.
lgtm. clean code, good test coverage, e2e works against live anthropic.
two nits inline - the close/aclose transport leak and the UNSUPPORTED_CAPABILITY error mapping. not blocking.
- Add ModelUnsupportedCapabilityError so unsupported operations (e.g. Anthropic embeddings/image-generation) surface a specific error instead of falling through to generic ModelAPIError - Forward the provider's operation-specific message in the cause - Add parametrized test case for the new error path
3 participants
Summary
Fourth PR in the model facade overhaul series (plan, architecture notes). Adds the native Anthropic Messages API adapter and routes
provider_type="anthropic"through the native client path. Extracts shared nativehttpxtransport, lifecycle, timeout, and error-wrapping logic intoHttpModelClientandhttp_helpers.pysoAnthropicClientandOpenAICompatibleClientshare the same HTTP machinery while keeping provider-specific translation separate.Previous PRs:
ModelFacadefrom LiteLLM viaModelClientadapterChanges
Added
anthropic.py— native Anthropic Messages API adapter with/v1endpoint handlinganthropic_translation.py— request/response translation (system message lifting, tool turns, image blocks, empty text block filtering)http_helpers.py— shared HTTP transport utilitieshttp_model_client.py— abstract base for native HTTP adapters with lazy client init and close semanticsmodel-facade-overhaul-pr-4-architecture-notes.md— architecture notes for this PRProviderErrorKind.QUOTA_EXCEEDED— new error kind for credit/billing failures (e.g. Anthropic "credit balance is too low")ModelQuotaExceededError— user-facing error for quota/billing issues_attach_provider_messagehelper — surfaces raw provider error messages in formatted output for HTTP 400 errorsChanged
openai_compatible.py— refactored to inherit fromHttpModelClientfactory.py— routesprovider_type="anthropic"to native adapter, preserves bridge fallbackerrors.py(client) — extended_looks_like_unsupported_params_errorfor mutually exclusive params, added_looks_like_quota_exceeded_errorerrors.py(model) —FormattedLLMErrorMessagenow includes optionalprovider_message(shown before Cause for 400s);_raise_from_provider_errorhandlesQUOTA_EXCEEDEDand attaches provider messages selectivelyFixed
{"type": "text", "text": ""}) in assistant messages with tool calls are now filtered out — Anthropic API rejects them (f5b0b39e)/v1endpoint handling:_get_messages_route()avoids/v1/v1/messagesduplication when endpoint already includes/v1ModelQuotaExceededErrorinstead of genericModelBadRequestError; "temperatureandtop_pcannot both be specified" maps toModelUnsupportedParamsErrorAttention Areas
anthropic_translation.py— request/response translation, especially system message lifting, tool turns, image block conversion, and empty text block filteringhttp_model_client.py— shared lazy client initialization and close semantics across native adaptersfactory.py—anthropicrouting and bridge fallback behaviorerrors.py(model) —_attach_provider_messagehelper and selective provider message surfacing logicTest plan
uv run ruff checkon all changed source filesuv run pyteston all new and updated test filesDescription updated with AI