added azureopenai support & extandable architecture for pydanticai#62816
Conversation
kaxil
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Adding Azure OpenAI support is a good idea.
My main concern: pydantic-ai already ships with native AzureProvider support — see the docs. You can do:
from pydantic_ai.providers.azure import AzureProvider
model = OpenAIChatModel(
'gpt-5.2',
provider=AzureProvider(
azure_endpoint='https://myresource.openai.azure.com',
api_version='2024-07-01-preview',
api_key='...',
),
)Or even just use "azure:gpt-5.2" as the model string (with env vars set). So we don't need to manually construct AsyncAzureOpenAI clients from the openai SDK — pydantic-ai handles Azure natively.
The current hook's get_conn() is ~25 lines and delegates to pydantic-ai's infer_model() + provider_factory. Azure support could be a simple conditional branch using AzureProvider, without the 4-file builder pattern. The whole point of using pydantic-ai is that it abstracts provider differences for us — we should lean on that rather than wrapping it in another layer.
Specific comments inline.
|
Thanks for the comments. |
Guide AI coding tools and contributors toward the right design decisions: delegate to pydantic-ai instead of re-implementing provider-specific logic, keep the hook thin, avoid premature abstractions like builder patterns or registries. Motivated by PR apache#62816 which added ~280 lines of Azure OpenAI builder code that duplicated what pydantic-ai's AzureProvider already handles natively.
|
Hi @kaxil I have taken your "thin hook" suggestion into consideration. However, I believe that relying solely on environment variables might not be sufficient for Airflow users; being able to fetch configurations directly from connection extras is highly valuable. I've reviewed all the current pydanticai providers and noticed that only AzureProvider and LiteLLMProvider require their own specific parameters instead of the standard base_url. To handle this distinction cleanly without over-engineering or building registries, I am planning to introduce a simple _build_provider_kwargs(conn) helper method. This method will parse the connection and return the appropriate kwargs dictionary to feed directly into the resolved provider_cls. What are your thoughts on proceeding with this approach? Does this align better with the design goals? |
|
Thanks for the follow-up! I think there's a misunderstanding — I'm not suggesting env-vars-only. The hook already reads credentials from the Airflow Connection ( What I'm suggesting is: instead of introspecting provider constructors with provider_cls = infer_provider_class(provider_name)
if provider_name == "azure":
return provider_cls(azure_endpoint=base_url, api_key=api_key)
if provider_name == "litellm":
return provider_cls(api_base=base_url, api_key=api_key)
# Default path — works for OpenAI, Anthropic, Groq, Mistral, etc.
kwargs = {}
if api_key:
kwargs["api_key"] = api_key
if base_url:
kwargs["base_url"] = base_url
return provider_cls(**kwargs)This is explicit, easy to read, and doesn't break if pydantic-ai renames a constructor parameter. The For connection extras: I'm fine with passing extra fields through to the provider constructor for the specific branches that need them (e.g. |
What if we simply read key-value pairs from conn_extras and pass them directly to the provider? We could define a schema in the connection extras like {provider_extra_args: {...}}, then document that this field and point users to the pydantic-ai docs to configure it correctly themselves ? |
|
Thanks for the guidance! I completely understand the "thin hook" approach and using a simpler method for Azure and LiteLLM. I just have one concern and wanted to get your thoughts on it. When I look at other providers, they seem to require entirely different parameters from the connection. For example: BedrockProvider uses aws_access_key_id, region_name, etc. I'm a bit worried that if we only map api_key and base_url right now, we might eventually have to write endless if/elif blocks in the hook for every single provider's unique parameters in the future. But of course, if you wish, we can start as you suggested and then revise it as needed. |
Yes we can follow KISS principe so then user could manage what need to pass extra properties for thier provider |
|
Good question — and I think we should go further than just extras. The right approach for Airflow is separate connection types for the cloud providers that need distinct credentials. Airflow's connection form doesn't support conditional fields (no way to show/hide fields based on a dropdown). But it does have an established pattern for this: the Google provider ships 5 connection types ( We should follow the same pattern:
Each connection type is a hook subclass that overrides The generic Implementation-wise, each subclass is small: class PydanticAIAzureHook(PydanticAIHook):
conn_type = "pydanticai_azure"
hook_name = "Pydantic AI (Azure OpenAI)"
@staticmethod
def get_ui_field_behaviour() -> dict[str, Any]:
return {
"hidden_fields": ["schema", "port", "login"],
"relabeling": {"password": "API Key", "host": "Azure Endpoint"},
"placeholders": {"host": "https://myapp.openai.azure.com/"},
}The provider-specific credential mapping lives in an overridable method on the base class. No if/elif chains, no So to answer your concern directly: we won't end up with endless if/elif. Each cloud provider gets its own connection type + hook subclass as separate PRs. No factory or registry needed. |
|
Hi @kaxil, @gopidesupavan I've refactored the PR to fully implement your suggested approach using dedicated hook subclasses and conn_types. I also removed the builder and dynamic parsing logic. Could you take a look when you have a chance? Please let me know if there is anything missing or if you'd like me to make any further tweaks/additions. |
kaxil
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround — removing the builder pattern and using subclass hooks instead is a much better direction.
A few things still need attention before this can merge. The most important:
-
provider.yamlnot updated —get_provider_info.pyis auto-generated fromprovider.yaml. Your manual edits toget_provider_info.pywill be overwritten on the nextprek run update-providers-build-files. The new connection types (pydanticai_azure,pydanticai_bedrock,pydanticai_vertex) need to be added toprovider.yamlunderconnection-types:, then regenerated. -
Vertex kwargs don't match pydantic-ai's
GoogleProvider— see inline comment. -
Subclass
default_conn_namedoesn't propagate —PydanticAIHook.__init__bindsllm_conn_id's default at class-definition time to"pydanticai_default". Subclasses inherit this bound default, soPydanticAIAzureHook()(no args) looks up"pydanticai_default", not"pydantic_ai_azure_default". Either override__init__in each subclass or use a sentinel that resolves at runtime.
Inline comments below.
kaxil
left a comment
There was a problem hiding this comment.
(Reposting inline comments with correct line positions — previous batch was off. PR-level feedback from the earlier review still applies.)
cetingokhan
left a comment
There was a problem hiding this comment.
Fixed all comments ;)
kaxil
left a comment
There was a problem hiding this comment.
Big improvement over the builder/protocol version. The separate connection types + subclass approach is clean and follows existing Airflow patterns.
A few things still need fixing before this is ready.
kaxil
left a comment
There was a problem hiding this comment.
Architecture looks good — subclass hooks with _get_provider_kwargs() as the extension point is the right pattern. A few issues inline.
kaxil
left a comment
There was a problem hiding this comment.
Left 3 inline comments on this round of review. The subclass architecture and _get_provider_kwargs() extension point are clean. The model_id regression from last round is fixed. Main feedback is about empty-string handling consistency and silent fallback logging.
|
Hi @kaxil and @gopidesupavan , I would like to sincerely thank you for the time and patience you've invested in reviewing my PRs. I feel a bit embarrassed knowing that I’ve taken up so much of your time with my numerous mistakes, especially when I know you could have implemented these features much faster yourselves. I truly appreciate the guidance you've provided despite the extra work I've caused. These reviews are proving to be incredibly valuable for me in understanding the "Airflow Gurus" perspective on architecture. I believe that as I adopt this mindset, my future PRs will require much less back-and-forth, though I know this takes time and experience. Regarding my other PR (LLMDataQualityOperator #62963 ), I am fully committed to it. However, if you have a strict deadline or if you feel the review process will be too taxing for your current schedule, I would completely understand if you prefer to put it on hold. That said, if you are willing to bear with me, I am more than eager to make all the necessary improvements and see it through to completion! |
|
Hey @cetingokhan — don't be embarrassed, this is exactly how the process is supposed to work! The PR has genuinely improved a lot through each round — going from the builder pattern to the subclass hooks was a big architectural shift and you landed it well. That's the whole point of code review. For #62963 — no rush at all, take your time with it. Happy to review when you're ready 👍 |
kaxil
left a comment
There was a problem hiding this comment.
Looks good! All feedback from previous rounds addressed. One tiny nit inline.
|
@cetingokhan looks like there is a conflict! Could you resolve that and we are good to go |
no worries @cetingokhan :) we are all here learning from each other .. |
This pull request refactors the model resolution logic in the
PydanticAIHookto use a modular builder pattern, improving extensibility and clarity for handling different AI providers, especially Azure OpenAI. It introduces dedicated builder classes for Azure OpenAI, custom endpoints, and default resolution, and updates documentation and tests to match the new structure.Builder pattern for model resolution:
AzureOpenAIBuilder,CustomEndpointBuilder,DefaultBuilder, and aProviderBuilderprotocol in thebuilderspackage to modularize how models are constructed from Airflow connection details. [1] [2] [3] [4]Refactoring in
PydanticAIHook:infer_modeland provider factory logic with a prioritized builder selection process (AzureOpenAIBuilder→CustomEndpointBuilder→DefaultBuilder), improving support for Azure OpenAI and custom endpoints.Documentation and UI improvements:
PydanticAIHookto clarify connection fields for Azure OpenAI and provide examples for required extras likeapi_versionandazure_deployment. [1] [2]Testing updates:
infer_modelandinfer_provider_classfrom the new builder modules instead of the hook, ensuring tests match the refactored code structure. [1] [2] [3] [4] [5]Connection validation enhancements:
api_versionandhost) and clarified error handling for missing model configuration.Was generative AI tooling used to co-author this PR?
Cloude Sonnet 4.6 & Gemini 3.1 Pro
Filled some of methods scope and tests created via copilot
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.