kaxil commented on code in PR #67788:
URL: https://github.com/apache/airflow/pull/67788#discussion_r3329421393
##########
providers/common/ai/src/airflow/providers/common/ai/hooks/pydantic_ai.py:
##########
@@ -179,16 +180,54 @@ def create_agent(
@overload
def create_agent(self, *, instructions: str, **agent_kwargs) ->
Agent[None, str]: ...
+ @overload
+ def create_agent(
+ self,
+ output_type: type[OutputT],
+ *,
+ spec_file: str | Path,
+ instructions: str | None = ...,
+ **agent_kwargs,
+ ) -> Agent[None, OutputT]: ...
+
+ @overload
def create_agent(
- self, output_type: type[Any] = str, *, instructions: str,
**agent_kwargs
+ self,
+ *,
+ spec_file: str | Path,
+ instructions: str | None = ...,
+ **agent_kwargs,
+ ) -> Agent[None, str]: ...
+
+ def create_agent(
+ self,
+ output_type: type[Any] = str,
+ *,
+ instructions: str | None = None,
+ spec_file: str | Path | None = None,
+ **agent_kwargs,
) -> Agent[None, Any]:
"""
Create a pydantic-ai Agent configured with this hook's model.
:param output_type: The expected output type from the agent (default:
``str``).
:param instructions: System-level instructions for the agent.
+ Required when *spec_file* is not given. When *spec_file* is
given, this
+ value overrides the instructions in the file; omit to use the file
value.
+ :param spec_file: Path to a YAML or JSON ``AgentSpec`` file. When
supplied,
+ delegates to ``Agent.from_file(spec_file, model=..., ...)``.
:param agent_kwargs: Additional keyword arguments passed to the Agent
constructor.
"""
+ if spec_file is not None:
+ return Agent.from_file(
+ spec_file,
+ model=self.get_conn(),
+ output_type=output_type,
+ instructions=instructions,
+ **agent_kwargs,
+ )
Review Comment:
Confirmed against pydantic-ai source: `Agent.from_file` → `from_spec`, where
`effective_model = model or validated_spec.model` (`agent/__init__.py:731`).
Since `create_agent` always passes `model=self.get_conn()`, and `get_conn()`
returns `model_id or extra["model"]` or raises `ValueError("No model
specified...")` (`hooks/pydantic_ai.py:139`), the spec file's `model:` is never
used: with a model configured, that wins (the "model_id takes precedence"
half); with none, `get_conn()` raises before `from_spec` runs, so a spec-only
model can't work either.
So "The model declared in the spec file is used unless you pass model_id"
and the example's "Model ... come from the YAML file" don't hold -- `model:
openai:gpt-4o-mini` in the example spec is dead config. If the file should be
able to supply the model, only pass `model=` when one is configured and omit it
otherwise (credentials then fall back to env auth, since the provider_factory
is built inside `get_conn()`). Otherwise drop the "model comes from the spec"
framing.
##########
providers/common/ai/src/airflow/providers/common/ai/hooks/pydantic_ai.py:
##########
@@ -179,16 +180,54 @@ def create_agent(
@overload
def create_agent(self, *, instructions: str, **agent_kwargs) ->
Agent[None, str]: ...
+ @overload
+ def create_agent(
+ self,
+ output_type: type[OutputT],
+ *,
+ spec_file: str | Path,
+ instructions: str | None = ...,
+ **agent_kwargs,
+ ) -> Agent[None, OutputT]: ...
+
+ @overload
def create_agent(
- self, output_type: type[Any] = str, *, instructions: str,
**agent_kwargs
+ self,
+ *,
+ spec_file: str | Path,
+ instructions: str | None = ...,
+ **agent_kwargs,
+ ) -> Agent[None, str]: ...
+
+ def create_agent(
+ self,
+ output_type: type[Any] = str,
+ *,
+ instructions: str | None = None,
+ spec_file: str | Path | None = None,
+ **agent_kwargs,
) -> Agent[None, Any]:
"""
Create a pydantic-ai Agent configured with this hook's model.
:param output_type: The expected output type from the agent (default:
``str``).
:param instructions: System-level instructions for the agent.
+ Required when *spec_file* is not given. When *spec_file* is
given, this
+ value overrides the instructions in the file; omit to use the file
value.
+ :param spec_file: Path to a YAML or JSON ``AgentSpec`` file. When
supplied,
+ delegates to ``Agent.from_file(spec_file, model=..., ...)``.
:param agent_kwargs: Additional keyword arguments passed to the Agent
constructor.
"""
+ if spec_file is not None:
+ return Agent.from_file(
+ spec_file,
+ model=self.get_conn(),
+ output_type=output_type,
+ instructions=instructions,
+ **agent_kwargs,
+ )
+ if instructions is None:
+ raise ValueError("instructions is required when spec_file is not
provided.")
Review Comment:
`instructions` doesn't override the spec file -- pydantic-ai merges them.
`from_spec` does `merged = normalize_instructions(spec.instructions);
merged.extend(normalize_instructions(instructions))`
(`agent/__init__.py:712-713`), so the file's instructions and the passed
`instructions` are both sent to the model concatenated. In
`summarize_with_instruction_override` the model receives both "respond with a
single paragraph" (file) and "Summarize in exactly one sentence" (call) --
contradictory, not an override.
Three spots claim override: the RST ("Passing instructions ... **overrides**
the file value"), this `:param instructions:` docstring, and the example
name/docstring. `test_create_agent_with_spec_file_instructions_overrides_file`
only asserts the kwarg is forwarded to a mocked `from_file`, so it doesn't
catch the merge. Either reword to "appended to / merged with", or if override
is intended, load the spec and clear its `instructions` before constructing.
--
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]
