kaxil commented on PR #70441:
URL: https://github.com/apache/airflow/pull/70441#issuecomment-5097467354
Thanks for this, and for the offer on Bedrock/Vertex. Before we extend the
per-vendor-subclass pattern across a second framework I want to pin down the
use case, because a couple of the answers change the shape of the fix.
Everything below was checked against `langchain-openai` 1.4.1.
### 1. What can't a user do today?
The existing `LangChainHook` already reaches Azure OpenAI as long as `host`
is left empty and the worker has `AZURE_OPENAI_ENDPOINT` + `OPENAI_API_VERSION`
set:
```python
init_chat_model("azure_openai:gpt-4o", api_key="k")
# -> resolves fine, endpoint and api_version come from env
```
So what this PR newly enables is carrying the endpoint and API version *in
the connection* rather than in the worker env. That is worth wanting (secrets
backend, several Azure resources in one deployment, per-connection routing),
but it is narrower than "Azure doesn't work". Which of those are you actually
hitting? A concrete deployment shape would help.
### 2. Why a new connection type instead of kwargs passthrough?
`init_chat_model` forwards arbitrary kwargs to the vendor class. This works
today:
```python
init_chat_model("azure_openai:gpt-4o", api_key="k",
azure_endpoint="https://x.openai.azure.com";,
api_version="2024-07-01-preview",
azure_deployment="gpt4o-prod-eu")
```
So having `_connection_kwargs` merge an allow-listed set of `extra` keys
would cover Azure, Bedrock, Vertex, Cohere and HuggingFace in roughly five
lines, without a new class or a new conn type to maintain a UI form for. What
does the subclass buy over that?
It does need to be an allow-list rather than a blind passthrough, because
`init_chat_model` sweeps unrecognised kwargs into `model_kwargs` behind a
`UserWarning` instead of rejecting them, so a typo would surface as a
request-time API error rather than a construction error.
I am raising this here rather than against the pydantic-ai hooks because of
the trajectory: the provider registers 7 connection types today, your
Bedrock/Vertex follow-ups take LangChain to 10, and `LlamaIndexHook` has the
identical `api_key`/`api_base` shape and the identical gap, so the same
argument adds a third row. Each conn type is permanent public surface, so it is
better to settle the axis now.
### 3. `azure_deployment` isn't reachable
Azure deployment names are user-chosen and routinely differ from the model
name. The SDK routes on it:
```
azure_deployment=None ->
.../openai/deployments/gpt-4o/chat/completions
azure_deployment='gpt4o-prod-eu' ->
.../openai/deployments/gpt4o-prod-eu/chat/completions
```
The hook offers no way to set it, so anyone whose deployment is not named
exactly after the model cannot use this. Deliberate?
### 4. `api_version` probably shouldn't be optional
`AzureChatOpenAI` has no default for it, so omitting it surfaces a raw
pydantic `ValidationError` from inside the constructor rather than an
Airflow-side message naming the connection field. `AzureOpenAIEmbeddings`
defaults to `2023-05-15`, so the same omission silently pins a three-year-old
API version instead of failing. Given the hook exists largely to carry this
field, should it be required with a clear error, the way `_resolve_model_id`
already does for the model id?
### 5. Static API key only, what about Entra ID?
`AzureChatOpenAI` accepts `azure_ad_token` and `azure_ad_token_provider`,
and managed identity is the default auth path for Azure OpenAI in most
enterprise setups (AKS workload identity, no key in the connection at all). A
hook whose whole value is Azure-specific credential handling, but which only
reads a static key from `conn.password`, misses the deployments that most need
it. Scope cut for a follow-up, or oversight? `PydanticAIAzureHook` has the same
limitation, so this is not on you, but it is the piece I would most want
covered before widening the pattern.
### 6. Naming and packaging
LangChain has two Azure providers in `_BUILTIN_PROVIDERS`: `azure_openai`
(langchain-openai) and `azure_ai` (langchain-azure-ai, AI Foundry). Different
classes, different kwargs. `langchain-azure` does not say which, and it is the
name you would want if Foundry is ever added. `langchain-azure-openai`?
Separately, the `langchain` extra is just `langchain>=1.0.0`. Nothing pulls
in `langchain-openai`, so installing `[langchain]` and creating this connection
fails at `init_chat_model` with an ImportError.
### 7. Prefix validation and test_connection
Nothing checks the model prefix against the hook. Putting `openai:gpt-4o` on
a `langchain-azure` connection does not raise: `init_chat_model` transfers
`azure_endpoint` and `api_version` into `model_kwargs` with a `UserWarning` and
hands back a plain `ChatOpenAI`, so it fails later at request time with a
confusing error. Worth defaulting or validating the prefix?
And `LangChainHook` has no `test_connection` (`PydanticAIHook` does), so
this registers a conn type whose Test button will not tell the user anything.
### 8. If we do go the per-vendor-subclass route, this probably belongs in
its own provider
`common.ai`'s stated scope is pydantic-ai (`provider.yaml`: "AI/LLM hooks
and operators for Airflow pipelines using pydantic-ai"). The LangChain and
LlamaIndex hooks already sit slightly outside that, and a full vendor matrix
pushes it well past.
Concretely, doing LangChain properly means `langchain-openai`,
`langchain-aws`, `langchain-google-vertexai`, `langchain-cohere`,
`langchain-huggingface` and possibly `langchain-azure-ai` as extras, plus a
conn type each. That is a dependency and CVE surface bolted onto a provider
whose actual product is the pydantic-ai operators, toolsets and capabilities,
and LangChain 1.x moves fast enough that its churn would start gating
`common.ai` releases. Airflow already ships per-vendor providers (`anthropic`,
`cohere`, `openai`, `pinecone`, `qdrant`, `weaviate`), so
`apache-airflow-providers-langchain` fits the existing shape better than a
`langchain-azure` conn type inside a pydantic-ai provider does.
The moment to decide is now. `common.ai` is 0.x and `lifecycle: incubation`,
the `langchain` conn type has only been out since 0.4.0, and nothing is
committed to compatibility yet. Moving one hook is cheap. Moving six conn types
and their UI forms after they have shipped is not.
One thing that should not move: the toolset bridge from #67791 exposes
`common.ai` toolsets as LangChain tools, so it stays here regardless. The split
would be hooks and connections out, bridge stays. The same question then
applies to `LlamaIndexHook`.
---
The code itself is clean and well tested, so please read 2 and 8 as
questions about the pydantic-ai pattern we shipped rather than about your PR.
Neither should turn into you having to build a new provider before this can
merge; if that is where we land, it gets scoped separately.
--
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]
