Implement get_async_http_client function for async Foundry HTTP clients#341
Implement get_async_http_client function for async Foundry HTTP clients#341daankolthof wants to merge 4 commits intopalantir:developfrom
Conversation
Generate changelog in
|
✅ Successfully generated changelog entry!Need to regenerate?Simply interact with the changelog bot comment again to regenerate these entries. 📋Changelog Preview✨ Features
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| token = get_foundry_token(preview=True) | ||
|
|
||
| # Merge auth header with any user-provided headers | ||
| return HttpClient(config=_create_http_client_config(config)) |
There was a problem hiding this comment.
_create_http_client_config() only injects auth headers but does not appear to set the Foundry API gateway/base URL (or hostname) when config is None. This likely breaks the stated goal of returning clients 'configured for the current Foundry environment' since the client may not know where to send requests. Consider setting base_url (e.g., using _get_api_gateway_base_url(preview=True)) when constructing a new Config, and when config is provided, preserve its base_url (or fill a default if it’s unset) while still merging headers.
| return AsyncHttpClient(config=_create_http_client_config(config)) | ||
|
|
||
|
|
||
| def _create_http_client_config(config: Optional[Config]) -> Config: |
There was a problem hiding this comment.
_create_http_client_config() only injects auth headers but does not appear to set the Foundry API gateway/base URL (or hostname) when config is None. This likely breaks the stated goal of returning clients 'configured for the current Foundry environment' since the client may not know where to send requests. Consider setting base_url (e.g., using _get_api_gateway_base_url(preview=True)) when constructing a new Config, and when config is provided, preserve its base_url (or fill a default if it’s unset) while still merging headers.
| return Config(default_headers=auth_header) | ||
| else: | ||
| merged_headers = {**auth_header, **(config.default_headers or {})} | ||
| config = replace(config, default_headers=merged_headers) | ||
|
|
||
| return HttpClient(config=config) | ||
| return replace(config, default_headers=merged_headers) |
There was a problem hiding this comment.
_create_http_client_config() only injects auth headers but does not appear to set the Foundry API gateway/base URL (or hostname) when config is None. This likely breaks the stated goal of returning clients 'configured for the current Foundry environment' since the client may not know where to send requests. Consider setting base_url (e.g., using _get_api_gateway_base_url(preview=True)) when constructing a new Config, and when config is provided, preserve its base_url (or fill a default if it’s unset) while still merging headers.
| config = Config(default_headers=auth_header) | ||
| return Config(default_headers=auth_header) | ||
| else: | ||
| merged_headers = {**auth_header, **(config.default_headers or {})} |
There was a problem hiding this comment.
The merge order allows caller-provided headers to override Authorization, which contradicts the helper’s stated purpose of ensuring the Foundry token is included and can lead to requests being sent with the wrong credentials. If the SDK should guarantee Foundry auth, reverse the merge order so Authorization from auth_header wins (or explicitly set/overwrite only the Authorization key).
| merged_headers = {**auth_header, **(config.default_headers or {})} | |
| merged_headers = {**(config.default_headers or {}), **auth_header} |
| def get_async_http_client( | ||
| *, preview: bool = False, config: Optional[Config] = None | ||
| ) -> AsyncHttpClient: | ||
| """Get an async HTTP client configured for the current Foundry environment. |
There was a problem hiding this comment.
The docstring claims a RuntimeError can be raised when the API gateway base URL is unavailable, but get_async_http_client() no longer computes/validates the base URL in this diff. Either restore base URL configuration/validation (so the documented error can occur), or update the docstring to only describe the errors actually raised via token retrieval/config handling.
| An AsyncHttpClient instance configured with the Foundry hostname and authentication. | ||
|
|
||
| Raises: | ||
| ValueError: If preview is not set to True. | ||
| RuntimeError: If the Foundry API gateway base URL or token is not available in the current context. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This is needed to use the 'AsyncOpenAI' OpenAI Python client.