kaxil commented on PR #71437:
URL: https://github.com/apache/airflow/pull/71437#issuecomment-5722575687
Approving this. Everything from the last round landed: the provider
`TypeError` now interpolates the cause, `ignored_extra_fields` covers the
Google case, the Bedrock replacement list is complete, an unprefixed
`embed_model` returns early, the precedence assertion is in, and both vendor
page openers are rewritten.
Five follow-ups below, none of them gating.
The first one I should have caught last round, so it is not a condition on
this approve. `_validate_embedding_connection_provider` runs before the
provider factory is resolved (`hooks/pydantic_ai.py:316`), so it fires on
connections where nothing could reach the wrong provider. A
`pydanticai_bedrock` connection hides `password` and `host`, so an `openai:`
embed model falls to the base mapper, which reads exactly those two, returns
`{}`, and leaves the factory `None`. That embedder would resolve through
`OPENAI_API_KEY`, which is resolution step 2 in `get_conn`'s own docstring, yet
the call still raises `configures different LLM and embedding providers
('bedrock' and 'openai')` and points at an `embed_conn_id` connection that
would carry no credentials. Moving the call below the factory resolution and
gating on a non-`None` factory matches the rationale already written at
`docs/hooks/pydantic_ai.rst:66`. Gating on "has a registered mapper" instead
would leave a hole, since the azur
e mapper is the one that does read `password` and `host`.
`hooks/pydantic_ai.py:250`: the prefixless fix covers `embed_model`, but an
unprefixed `model` reaches the same branch from the other side, so `model:
"gpt-4o"` with a prefixed embed model reports `(None and 'openai')` and sends
you to `embed_conn_id` when the mistake is in the Model field. `test` is the
one unprefixed entry in `KnownModelName`, so a working `TestModel` connection
with an embedding model configured hits it too.
`hooks/pydantic_ai.py:733`: `google-cloud` names four of the five keys
`_get_google_cloud_provider_kwargs` reads and leaves out
`service_account_info`, which is credential-resolution step 1 in
`PydanticAIVertexHook`'s own docstring. Same gap you closed for Bedrock one
entry up.
`docs/hooks/pydantic_ai.rst:86`: `defer_model_check` came out of the example
but is still listed as an accepted kwarg. `get_embedder` always passes an
already-inferred model and `infer_embedding_model(m) is m`, so either value
produces the same object.
`docs/connections/pydantic_ai.rst:193`: the resolution-order block scopes
the `embed_conn_id` fallback to credentials and endpoints, but `get_embedder`
reads the `embed_model` string off that connection too, so setting
`embed_conn_id` and leaving `embed_model` on the LLM connection gives a "no
embedding model specified" error about a field you did set.
--
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]
