kaxil commented on code in PR #74451:
URL: https://github.com/apache/airflow/pull/74451#discussion_r4237632760
##########
providers/common/ai/src/airflow/providers/common/ai/hooks/pydantic_ai.py:
##########
@@ -406,13 +406,12 @@ def _resolve_own_model(
def _provider_factory(pname: str) -> Any:
try:
return infer_provider_class(pname)(**_kwargs)
- except TypeError:
- self.log.warning(
- "Provider '%s' rejected kwargs %s; falling back to
env-var auth",
- pname,
- list(_kwargs),
- )
- return infer_provider(pname)
+ except TypeError as e:
+ raise ValueError(
+ f"Provider '{pname}' rejected the credentials from
connection "
+ f"'{self.llm_conn_id}' (ignored: {',
'.join(_kwargs)}). Not falling back to "
Review Comment:
This always lists every kwarg as ignored and drops the `TypeError` text, and
`test_connection` returns only `str(e)`, so the connection test never says
which field broke. A `litellm:` connection with Host set reports `(ignored:
api_key, base_url)` when only `base_url` is wrong (`LiteLLMProvider` takes
`api_base`), and `PydanticAIVertexHook` with a `google:` model reports
`project, location` as rejected credentials because `GoogleProvider` doesn't
take them. Could the message include `{e}` and say "fields passed" rather than
"ignored", now that the whole connection is refused?
##########
providers/common/ai/docs/provider_fallback.rst:
##########
@@ -195,15 +195,11 @@ Two checks, neither of which requires waiting for a real
outage:
*Test the connection.* ``test_connection`` on the primary resolves every
connection in the
chain, so a fallback with a missing ``model`` or an unknown connection ID is
reported by name
there rather than discovered mid-incident. Credential fields a provider class
rejects with a
-``TypeError`` are caught by the hook, which retries with the env-var-based
provider
-constructor and logs a warning either way; if the required env var is also
missing, that
-retry raises ``pydantic_ai.exceptions.UserError``, which ``test_connection``
does surface
-since it wraps the whole resolution in a broad exception handler. What it
cannot show is the
-opposite case: the env var *is* set on the worker, the retry quietly succeeds,
and
-``test_connection`` reports success even though the credentials you configured
on the
-connection were silently ignored -- check the logs for that warning rather
than relying on
-``test_connection`` alone. It also does not call the provider, so a
well-formed but revoked
-key still passes -- that is what the drill below is for.
+``TypeError`` fail the resolution with a ``ValueError`` naming the connection,
the provider and
+the ignored fields -- the hook never falls back to environment-variable auth,
which would
Review Comment:
The `LiteLLMProvider` note in `self_hosted_models.rst` (around line 318)
still says the hook catches this `TypeError`, logs a warning and falls back to
environment-variable auth, and the
`test_ignores_connection_password_and_omits_api_key` docstring in
`test_cortex_model.py` (lines 66-69) says the same. Both should now describe
the `ValueError`. If the message wording above changes, "the ignored fields"
here would follow it.
##########
providers/common/ai/src/airflow/providers/common/ai/hooks/mcp.py:
##########
@@ -213,10 +214,10 @@ def get_conn(self) -> Any:
try:
from fastmcp.client.transports import SSETransport,
StdioTransport, StreamableHttpTransport
from pydantic_ai.mcp import MCPToolset
- except ImportError:
- raise ImportError(
+ except ImportError as e:
+ raise AirflowOptionalProviderFeatureException(
Review Comment:
`docs/toolsets/index.rst` (lines 115-116) still says a missing `mcp` extra
raises `ImportError` when `MCPToolset` first connects, which goes through this
`get_conn()`. It now matches the sentence just before it about `SQLToolset` and
`DataFusionToolset`, so the two could merge.
--
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]
