kaxil commented on code in PR #73024:
URL: https://github.com/apache/airflow/pull/73024#discussion_r4004795272
##########
providers/common/ai/docs/connections/pydantic_ai_azure.rst:
##########
@@ -85,15 +86,16 @@ Examples
{
"conn_type": "pydanticai_azure",
"password": "<azure-api-key>",
- "host": "https://<resource>.openai.azure.com",
+ "host": "https://<resource>.openai.azure.com/openai/v1",
"extra": "{\"model\": \"azure:gpt-4o\", \"api_version\":
\"2024-07-01-preview\"}"
Review Comment:
This example still carries `api_version` alongside the new `/openai/v1`
host, and `AzureProvider` rejects exactly that pair: "`api_version` must not be
set when `azure_endpoint` targets the Azure OpenAI v1 API or an Azure AI
Foundry serverless model endpoint, which do not accept it." I ran the
constructor on 2.0.0, 2.20.0 and 2.40.0 and it raises `UserError` on all three.
The `provider.yaml` placeholder and both hook placeholders dropped
`api_version` in this same commit, so this example looks like the one spot that
got missed.
##########
providers/common/ai/docs/connections/pydantic_ai.rst:
##########
@@ -60,7 +60,7 @@ Host (optional)
- Ollama: ``http://localhost:11434/v1``
- vLLM: ``http://localhost:8000/v1``
- - Azure OpenAI:
``https://<resource>.openai.azure.com/openai/deployments/<deployment>``
+ - Azure OpenAI: ``https://<resource>.openai.azure.com/openai/v1``
Review Comment:
The `/openai/v1` form fixes the URL half of this, thanks. The credential
half is still open: on this generic `pydanticai` connection `host` maps to
`base_url`, and `AzureProvider` has no `base_url` parameter on 2.0.0, 2.20.0 or
2.40.0, so an `azure:gpt-4o` model hits the `except TypeError` in `get_conn()`
and falls back to env-var auth with the API key dropped. Since
`pydantic_ai_azure.rst` says the `azure:` prefix is required, could this bullet
note that the generic connection needs an `openai:` model and point at
`pydanticai_azure` otherwise?
##########
providers/common/ai/tests/unit/common/ai/hooks/test_pydantic_ai.py:
##########
@@ -528,6 +528,10 @@ def test_hook_name(self):
def test_ui_field_behaviour_relabels_host(self):
behaviour = PydanticAIAzureHook.get_ui_field_behaviour()
assert behaviour["relabeling"].get("host") == "Azure Endpoint"
+ assert behaviour["placeholders"] == {
Review Comment:
This commit changes the Azure values in three places (`provider.yaml`
`ui-field-behaviour.placeholders`, `provider.yaml`
`conn-fields.api_version.description`, and this hook copy) but the new
assertion covers only the hook copy; `TestPydanticAIVertexHook` pins all three
for vertex for that reason, and `get_provider_info` is already imported at the
top of this file. Separately, nothing in this class constructs a real
`AzureProvider`: `test_get_conn_uses_azure_endpoint` patches
`infer_provider_class` and asserts what the hook passes rather than what the
provider accepts, so the pairing this PR now recommends (a `/openai/v1` host
with no `api_version`) has no coverage. One test with only `get_connection`
patched would exercise it, and needs no credentials or network.
##########
providers/common/ai/provider.yaml:
##########
@@ -206,7 +206,7 @@ connection-types:
- 'null'
api_version:
label: API Version
- description: "Azure OpenAI API version (e.g. 2024-07-01-preview).
Falls back to OPENAI_API_VERSION."
+ description: "Only for legacy Azure OpenAI endpoints. Omit for the v1
API."
Review Comment:
Minor, but dropping the example took the format with it, and this field is
required for the legacy endpoints it now describes: with a legacy endpoint and
neither `api_version` nor `OPENAI_API_VERSION` set, `AzureProvider` raises
"Must provide one of the `api_version` argument or the `OPENAI_API_VERSION`
environment variable". The v1-versus-legacy framing here and in the three
matching descriptions is also narrower than the real rule in
`_openai_compatible_v1_base_url`, which treats any path ending in `/v1` and any
`*.models.ai.azure.com` Foundry host as v1: a Foundry endpoint carries no
`/v1`, so it reads as legacy under this wording and raises the same `UserError`
if someone keeps `api_version` set. Naming the rule rather than the label would
cover both.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
