kaxil commented on code in PR #74811:
URL: https://github.com/apache/airflow/pull/74811#discussion_r4289883041
##########
providers/common/ai/src/airflow/providers/common/ai/operators/agent.py:
##########
@@ -1361,6 +1371,8 @@ def _resolve_message_history(self) -> list[ModelMessage]
| None:
def _emit_message_history(self, context: Context, result: Any) -> None:
"""Push the full post-run transcript to XCom for the next turn to
resume."""
+ if not self.do_xcom_push:
Review Comment:
This gate also stops the push for `message_history` users, not only the new
flag. In 1.0.0rc1 `_emit_message_history` pushed unconditionally, and
`stability.rst` lists `message_history` as stable, where narrowing what a
feature does needs a major release. A session Dag that sets
`do_xcom_push=False` to keep a big answer out of XCom now pulls nothing on the
next turn: with `default='[]'` every run silently starts fresh, and the store
pattern in `example_agent.py` (no default) saves `"None"`, which fails to parse
on the turn after.
Could the gate apply only to the new flag, e.g. `if self.message_history is
None and not self.do_xcom_push: return`? A test that `message_history=[]` still
pushes with `do_xcom_push=False` would pin it; nothing covers that case today.
##########
providers/common/ai/src/airflow/providers/common/ai/operators/agent.py:
##########
@@ -502,6 +507,7 @@ def __init__(
durable: bool = False,
cache_prompt: bool = True,
message_history: list[ModelMessage] | str | bytes | None = None,
+ emit_message_history: bool = False,
Review Comment:
`emit_message_history=False` doesn't stop the push when `message_history` is
set, so `False` here doesn't really mean "don't emit". Would `bool | None =
None`, resolving to `message_history is not None`, read better? `True` opts a
single-turn run in, `False` lets a seeded run opt out, and the transcript gets
its own switch instead of riding on `do_xcom_push`.
##########
providers/common/ai/docs/message_history.rst:
##########
@@ -49,7 +55,8 @@ fresh session instead of trying to parse the string
``"None"``.
.. note::
- ``message_history`` cannot be combined with ``enable_hitl_review`` -- the
+ ``message_history`` and ``emit_message_history`` cannot be combined with
Review Comment:
`stability.rst` and `troubleshooting.rst` need a line each for this. The
stable row names only `message_history`, so is `emit_message_history` stable or
experimental? And the troubleshooting entry for `message_history and
enable_hitl_review=True cannot be used together` doesn't cover the new
`emit_message_history` error.
--
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]
