kaxil commented on code in PR #72151:
URL: https://github.com/apache/airflow/pull/72151#discussion_r3968176161
##########
providers/openai/src/airflow/providers/openai/operators/openai.py:
##########
@@ -83,25 +83,34 @@ class OpenAIResponseOperator(BaseOperator):
"""
Operator that generates a model response using the OpenAI Responses API.
- The operator is synchronous and returns the response's aggregated output
text. For
- ``previous_response_id`` chaining, ``background=True`` responses, or
access to the full
- structured response, use
:class:`~airflow.providers.openai.hooks.openai.OpenAIHook` directly.
+ The operator is synchronous and returns the response's aggregated output
text; the
+ response id is also pushed to XCom (see below), so a downstream task can
pick it up
+ for ``previous_response_id`` chaining without going through the hook. For
+ ``background=True`` responses, or access to the full structured response,
use
+ :class:`~airflow.providers.openai.hooks.openai.OpenAIHook` directly.
:param conn_id: The OpenAI connection ID to use.
:param input_text: The input prompt for the model. This can be a string or
a structured list of
input items.
:param model: The OpenAI model to use.
:param response_kwargs: Additional keyword arguments to pass to the OpenAI
``create_response``
method (for example ``instructions``, ``tools``, ``conversation`` or
``previous_response_id``).
+ Templated, so values (e.g. ``previous_response_id``) may reference
upstream XCom.
.. seealso::
For more information on how to use this operator, take a look at the
guide:
:ref:`howto/operator:OpenAIResponseOperator`
For possible options, see:
https://platform.openai.com/docs/api-reference/responses/create
+
+ ``execute`` also pushes two XCom keys: ``response_id`` (the response's ID,
usable as
+ a downstream call's ``previous_response_id``) and ``usage`` (the flattened
response
Review Comment:
`ResponseUsage.model_dump()` is nested rather than flattened: on the
`openai>=2.37.0` floor it returns `{'input_tokens': 5, 'input_tokens_details':
{'cached_tokens': 1}, 'output_tokens': 7, 'output_tokens_details':
{'reasoning_tokens': 2}, 'total_tokens': 12}`. Someone who trusts "flattened"
reaches for `usage["cached_tokens"]` and gets a `KeyError`. The nesting also
matters for the pricing sentence: cached input tokens bill at a reduced rate,
so `input_tokens_details.cached_tokens` is exactly the field you need to price
a run correctly. Showing the literal dict here and in the rst would settle both.
##########
providers/openai/src/airflow/providers/openai/operators/openai.py:
##########
@@ -83,25 +83,34 @@ class OpenAIResponseOperator(BaseOperator):
"""
Operator that generates a model response using the OpenAI Responses API.
- The operator is synchronous and returns the response's aggregated output
text. For
- ``previous_response_id`` chaining, ``background=True`` responses, or
access to the full
- structured response, use
:class:`~airflow.providers.openai.hooks.openai.OpenAIHook` directly.
+ The operator is synchronous and returns the response's aggregated output
text; the
+ response id is also pushed to XCom (see below), so a downstream task can
pick it up
+ for ``previous_response_id`` chaining without going through the hook. For
+ ``background=True`` responses, or access to the full structured response,
use
+ :class:`~airflow.providers.openai.hooks.openai.OpenAIHook` directly.
:param conn_id: The OpenAI connection ID to use.
:param input_text: The input prompt for the model. This can be a string or
a structured list of
input items.
:param model: The OpenAI model to use.
:param response_kwargs: Additional keyword arguments to pass to the OpenAI
``create_response``
method (for example ``instructions``, ``tools``, ``conversation`` or
``previous_response_id``).
+ Templated, so values (e.g. ``previous_response_id``) may reference
upstream XCom.
.. seealso::
For more information on how to use this operator, take a look at the
guide:
:ref:`howto/operator:OpenAIResponseOperator`
For possible options, see:
https://platform.openai.com/docs/api-reference/responses/create
+
+ ``execute`` also pushes two XCom keys: ``response_id`` (the response's ID,
usable as
+ a downstream call's ``previous_response_id``) and ``usage`` (the flattened
response
+ usage, or ``None`` when the API omits it). ``usage`` reports token counts
only --
+ the OpenAI response carries no cost field, so pricing a run means
multiplying these
+ counts by your own per-token rate.
"""
- template_fields: Sequence[str] = ("input_text",)
+ template_fields: Sequence[str] = ("input_text", "response_kwargs")
Review Comment:
`OpenAIResponseOperator` shipped in 1.8.0, so widening this tuple changes
behaviour for Dags that already pass `response_kwargs`. The templater recurses
into every string in the dict and `template_undefined` defaults to
`StrictUndefined` on both the 2.11 floor and 3.x, so `instructions="Fill in
Hello {{ customer_name }}"` now hard-fails, and `instructions="Refer to the run
as {{ ds }}"` silently sends a different prompt to a paid API on a run that
still goes green. Not asking to drop the templating, it is what makes the
chaining work, but could it get a `.. note::` at the top of
`providers/openai/docs/changelog.rst` and a mention of `{% raw %}` as the
escape in the howto? #69506 added that kind of note in this provider and the
release manager folded it into 1.8.2.
##########
providers/openai/src/airflow/providers/openai/operators/openai.py:
##########
@@ -83,25 +83,34 @@ class OpenAIResponseOperator(BaseOperator):
"""
Operator that generates a model response using the OpenAI Responses API.
- The operator is synchronous and returns the response's aggregated output
text. For
- ``previous_response_id`` chaining, ``background=True`` responses, or
access to the full
- structured response, use
:class:`~airflow.providers.openai.hooks.openai.OpenAIHook` directly.
+ The operator is synchronous and returns the response's aggregated output
text; the
+ response id is also pushed to XCom (see below), so a downstream task can
pick it up
+ for ``previous_response_id`` chaining without going through the hook. For
+ ``background=True`` responses, or access to the full structured response,
use
+ :class:`~airflow.providers.openai.hooks.openai.OpenAIHook` directly.
:param conn_id: The OpenAI connection ID to use.
:param input_text: The input prompt for the model. This can be a string or
a structured list of
input items.
:param model: The OpenAI model to use.
:param response_kwargs: Additional keyword arguments to pass to the OpenAI
``create_response``
method (for example ``instructions``, ``tools``, ``conversation`` or
``previous_response_id``).
+ Templated, so values (e.g. ``previous_response_id``) may reference
upstream XCom.
Review Comment:
One knock-on worth a line in the PR body: rendered template fields are
persisted to `rendered_task_instance_fields` on every run, so the full
`instructions` text and any tool schemas now land in the metadata DB per run,
and that write is not gated by `do_xcom_push`. A dedicated
`previous_response_id` parameter in `template_fields` would give the same
chaining without templating the whole prompt payload. Was the broader field a
deliberate call so `tools` and `instructions` can be templated too?
##########
providers/openai/tests/unit/openai/operators/test_openai.py:
##########
@@ -89,20 +90,84 @@ def test_openai_response_operator_execute():
response_kwargs={"instructions": "Be concise.",
"previous_response_id": "resp_prev"},
)
mock_hook_instance = Mock(spec=OpenAIHook)
- mock_hook_instance.create_response.return_value = Mock(
- spec=Response, output_text="haiku text", id="resp_123",
status="completed"
+ mock_usage = Mock(spec=ResponseUsage)
+ mock_usage.model_dump.return_value = {"input_tokens": 5, "output_tokens":
7}
Review Comment:
`ResponseUsage` is a plain pydantic value object, so building a real one
here instead of stubbing `model_dump` would pin the actual nested shape and
stop the docstring and the test agreeing on a shape the SDK never returns.
While you are in this file, the templating test below only exercises
`response_kwargs`: reverting `template_fields` to drop `input_text` still
passes every test in the file.
--
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]
