ColtenOuO commented on PR #70441: URL: https://github.com/apache/airflow/pull/70441#issuecomment-5107664819
Thanks a lot for the review, and for putting this much thought into the feedback! To be honest, what got me started on this PR was this part of the docstring: > Providers with bespoke auth (AWS Bedrock, Google Vertex AI / GenAI, Azure OpenAI, Cohere, HuggingFace) reject these kwargs; per-vendor subclasses can be added later mirroring the pydantic-ai pattern. So I followed the pydantic-ai pattern and put together an Azure version, hoping to help extend it. --- ## What can't a user do today? Honestly — nothing is broken. Users really can use Azure OpenAI today without an Azure hook. As I mentioned above, because the docstring brings this up, I took it to mean that adding per-vendor subclasses was a planned direction for the future. (Or maybe I misread what that comment meant — please let me know if that's the case ><) But what I can confirm is that there's no problem with the current usage at all. --- ## Why a new connection type instead of kwargs passthrough? You're right that a plain allow-list of per-vendor keys would be considerably easier to maintain. The advantage I see in the subclasses is on the UI side: when a user picks a vendor, we can show the fields that vendor actually needs, which makes the experience friendlier and gives them an easy way to see what LangChain vendors are supported right now. <img width="1895" height="918" alt="image" src="https://github.com/user-attachments/assets/1b85e8f5-24a0-4b1a-be24-65b790225b74"; /> As in the screenshot above — if we pick Azure as the vendor, both the Standard Fields and the Extra Fields immediately adapt to it and hint at the keys the user should fill in. Maybe there's a way to ask the user to choose the vendor first when they select LangChain, and then list the corresponding Standard Fields and Extra Fields? If that were possible, I think the user experience would be even friendlier >< That said, it would probably mean touching things inside `airflow-core`, so it may not be a good approach either. I may well be overthinking this (sorry, I do that a lot xD) — if we set the UI experience aside, I agree that a plain allow-list is the better way to maintain this. --- ## If we do keep per-vendor subclasses, this may belong in its own provider I agree on all three points. --- I've also gone through all the code-level suggestions and I'm happy to handle them. Before that, though, I'd like to wait until the direction on subclasses is settled before moving on to the next step. Thanks again for raising these questions and suggestions! -- 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]
