kaxil commented on code in PR #71575:
URL: https://github.com/apache/airflow/pull/71575#discussion_r4109176855
##########
providers/common/ai/src/airflow/providers/common/ai/durable/fingerprint.py:
##########
@@ -98,11 +127,56 @@ def _strip_volatile(messages_dump: list[dict[str, Any]])
-> list[dict[str, Any]]
return stripped
+def _canonical(value: Any) -> Any:
+ """
+ Render ``value`` as JSON-safe data whose encoding is identical on every
attempt.
+
+ ``to_jsonable_python`` on its own is not a safe fingerprint input, for two
+ reasons that both matter here because tool arguments arrive as live Python
+ objects that pydantic has already validated.
+
+ It renders a ``set`` in iteration order, and for string members that order
+ follows the interpreter's hash seed. Every task attempt is a fresh
process, so
+ a ``set[str]`` argument would hash differently each time and never replay
--
+ worse than declining to cache, because the step re-runs live on every
retry.
+ Members are therefore ordered by their own JSON encoding.
+
+ It also *consumes* an iterator, and pydantic validates an ``Iterable[T]``
+ parameter lazily into a ``ValidatorIterator``. Normalizing the arguments
would
+ drain the tool's own input before the tool ran, so the tool would see an
empty
+ sequence and that wrong result would be cached under the fingerprint of the
+ full one. Such a value is refused instead, which degrades the step to the
+ not-cached path rather than corrupting it.
+ """
+ if value is None or isinstance(value, (str, bool, int, float)):
+ return value
+ if isinstance(value, Iterator):
+ # Hashing this means draining it, leaving nothing for the tool to read.
+ raise TypeError(f"cannot fingerprint {type(value).__name__} without
consuming it")
+ if isinstance(value, Mapping):
+ return {key: _canonical(item) for key, item in value.items()}
Review Comment:
Keys are passed through as-is here, and `json.dumps(sort_keys=True)` raises
`TypeError` for anything but homogeneous `str` or `int` keys, where the
json-mode dump used to stringify them. So a tool returning `dict[date, float]`
(a series keyed by day), `dict[MyEnum, int]`, `dict[UUID, str]`, or a mixed
`{1: "a", "b": 2}` lands in `ToolReturnPart.content` and every later model step
fingerprints `None`; all of those digested on the merge-base (`495b81bd...` for
the `date` case, `None` here). Rendering the key through pydantic, for example
`next(iter(to_jsonable_python({key: None}, bytes_mode="base64")))` for
non-`str` keys, gives digests identical to base for every key type I tried.
##########
providers/common/ai/src/airflow/providers/common/ai/durable/fingerprint.py:
##########
@@ -85,9 +107,16 @@ def _strip_volatile(messages_dump: list[dict[str, Any]]) ->
list[dict[str, Any]]
timestamps, part-level timestamps); user data such as tool arguments is
never recursed into, so an argument legitimately named ``run_id`` still
affects the fingerprint.
+
+ Raises ``TypeError`` if a message did not dump to a mapping. A python-mode
dump
+ passes an object it does not recognize straight through, and a fingerprint
that
Review Comment:
The reason given here is not what happens. Without the guard, an object
passed straight through by the python-mode dump makes the next line raise
`AttributeError`, which the `except (TypeError, ValueError, RecursionError)`
below does not catch, so the task would fail rather than the fingerprint
drifting between attempts. The only producer of `messages` is pydantic-ai's
`Agent`, so `test_a_message_that_does_not_dump_to_a_mapping_returns_none`
covers an input that cannot occur, and it passes for a different reason anyway:
the `None` comes from `_canonical(object())` raising
`PydanticSerializationError`, so the guard is never what returns it (swapping
the raise for a passthrough leaves the suite green). Either drop the guard and
that test, or keep them and state the real reason (and name `_strip_volatile`
in the `except` comment).
##########
providers/common/ai/docs/durable_execution.rst:
##########
@@ -102,6 +102,30 @@ cache:
never replays responses that belong to a different conversation.
4. After successful completion, the cached steps are deleted.
+Fingerprints are computed from each value's canonical form, so ordinary types
that
Review Comment:
Two things this paragraph promises are not true at head until the bytes and
dict-key fixes land: "ordinary types that are not JSON fingerprint normally"
fails for `bytes`, `BinaryContent` and non-string dict keys, and the older
paragraph further down ("a `BinaryContent` from MCP tools means that step is
skipped") now understates it, since the return stays in
`ToolReturnPart.content` and every later model step misses too. Separately, the
run-wide causes below list `model_settings` and message history, but
`model_request_parameters` is hashed as well, so a tool definition with a
non-serializable `json_schema_extra` has the same reach; "in `model_settings`,
in the tool definitions the request carries, or in the message history" would
cover it.
##########
providers/common/ai/tests/unit/common/ai/durable/test_fingerprint.py:
##########
@@ -210,3 +224,199 @@ def test_arg_order_does_not_matter(self):
assert fingerprint_tool_call("t", {"a": 1, "b": 2}, "id1") ==
fingerprint_tool_call(
"t", {"b": 2, "a": 1}, "id1"
)
+
+
+class TestPydanticNativeValues:
+ """Values that are not JSON types but hash the same on every attempt must
still fingerprint.
+
+ Tool arguments reach ``fingerprint_tool_call`` already coerced by
pydantic, and
+ ``tool_choice`` accepts a dataclass while genuinely affecting the
response, so it
+ cannot be stripped as transport-only. Since a step that cannot be
fingerprinted is
+ no longer cached at all, refusing these values would stop an ordinary
typed tool
+ from ever being cached.
+ """
+
+ def test_datetime_tool_argument_fingerprints(self):
+ when = datetime.datetime(2026, 1, 1, tzinfo=datetime.timezone.utc)
+
+ assert fingerprint_tool_call("t", {"when": when}, "id1") is not None
+
+ def test_decimal_tool_argument_fingerprints(self):
+ assert fingerprint_tool_call("t", {"amount": Decimal("10.5")}, "id1")
is not None
+
+ def test_datetime_tool_argument_is_stable_and_distinguishing(self):
+ early = datetime.datetime(2026, 1, 1, tzinfo=datetime.timezone.utc)
+ late = datetime.datetime(2026, 6, 1, tzinfo=datetime.timezone.utc)
+
+ assert fingerprint_tool_call("t", {"when": early}, "id1") ==
fingerprint_tool_call(
+ "t", {"when": early}, "id1"
+ )
+ assert fingerprint_tool_call("t", {"when": early}, "id1") !=
fingerprint_tool_call(
+ "t", {"when": late}, "id1"
+ )
+
+ def test_tool_choice_dataclass_fingerprints(self):
+ fp = fingerprint_model_request(
+ "m",
+ make_messages(),
+ {"tool_choice": ToolOrOutput(function_tools=["my_tool"])},
+ ModelRequestParameters(),
+ )
+
+ assert fp is not None
+
+ def test_tool_choice_dataclass_still_affects_the_fingerprint(self):
+ one = fingerprint_model_request(
+ "m",
+ make_messages(),
+ {"tool_choice": ToolOrOutput(function_tools=["a"])},
+ ModelRequestParameters(),
+ )
+ other = fingerprint_model_request(
+ "m",
+ make_messages(),
+ {"tool_choice": ToolOrOutput(function_tools=["b"])},
+ ModelRequestParameters(),
+ )
+
+ assert one is not None
+ assert one != other
+
+ def test_value_pydantic_cannot_serialize_still_returns_none(self):
+ """Normalization must not turn a genuinely unserializable value into a
hash."""
+ assert fingerprint_tool_call("t", {"v": object()}, "id1") is None
+
+ def test_plain_payload_digest_is_unchanged_by_normalization(self):
Review Comment:
This pins a hand-built dict, not the message dump path, so the
`mode="python"` switch is invisible to the suite: with both `dump_python` calls
reverted to `mode="json"` all 68 tests still pass, and nothing here mentions
`BinaryContent` or `bytes`. A golden digest recorded from main for a
`ModelRequest` carrying a non-UTF-8 `BinaryContent`, and one for a `bytes`
`ToolReturnPart`, would fail without the `bytes_mode` fix; and a `set` inside
`ToolReturnPart.content` asserted stable across hash seeds would couple the
mode switch to the case it was made for.
##########
providers/common/ai/src/airflow/providers/common/ai/durable/fingerprint.py:
##########
@@ -85,9 +107,16 @@ def _strip_volatile(messages_dump: list[dict[str, Any]]) ->
list[dict[str, Any]]
timestamps, part-level timestamps); user data such as tool arguments is
never recursed into, so an argument legitimately named ``run_id`` still
affects the fingerprint.
+
+ Raises ``TypeError`` if a message did not dump to a mapping. A python-mode
dump
+ passes an object it does not recognize straight through, and a fingerprint
that
+ cannot strip the volatile fields would change on every attempt, so the
caller
+ degrades to an unverifiable ``None`` instead.
"""
stripped = []
for message in messages_dump:
+ if not isinstance(message, Mapping):
+ raise TypeError(f"expected a dumped message mapping, got
{type(message).__name__}")
cleaned = {k: v for k, v in message.items() if k not in
_VOLATILE_MESSAGE_KEYS}
if isinstance(cleaned.get("parts"), list):
Review Comment:
One more thing the mode switch exposes: the json-mode dump rendered `parts`
as a list whatever the input, but python mode preserves a tuple, and this
`list` check then skips the timestamp strip.
`AgentOperator(message_history=[ModelRequest(parts=(UserPromptPart(...),))])`
reaches here with a tuple because `_resolve_message_history` runs
`validate_python` on object input, which keeps instances as-is (I checked:
`parts` is still a `tuple` after validation). Each attempt then gets a fresh
part timestamp into the digest, so every model step re-runs live with a "does
not match the current request" warning that reads as if the agent changed,
where the merge-base was stable across attempts. `isinstance(..., (list,
tuple))` fixes it; a test with tuple parts and two different part timestamps
asserting equal fingerprints would cover it. The XCom round-trip yields lists,
so that path is unaffected.
##########
providers/common/ai/src/airflow/providers/common/ai/durable/fingerprint.py:
##########
@@ -119,13 +195,21 @@ def fingerprint_model_request(
output mode and schema, native tools, ...) so any change to what is sent
to the model invalidates the cached response.
- Returns ``None`` when the request cannot be serialized; ``None`` compares
- equal to ``None``, so requests that cannot be fingerprinted degrade to
- unverified positional replay rather than disabling caching.
+ Returns ``None`` when the request cannot be serialized even through
pydantic,
+ which prevents the step from being replayed or cached. Because model
settings
+ and message history are carried into every later request, such a value in
+ either usually degrades every subsequent model step of the run the same
way.
+ ``step`` is attached to that warning so the log names where it began.
"""
try:
- dumped = ModelMessagesTypeAdapter.dump_python(messages, mode="json")
- params =
_MODEL_REQUEST_PARAMETERS_ADAPTER.dump_python(model_request_parameters,
mode="json")
+ # ``mode="python"``, not ``mode="json"``: a json-mode dump renders a
set as
+ # a list in iteration order, and a tool that returned a set puts one
in the
+ # message history, where it would reach the hash already unstably
ordered.
+ # Python mode leaves it a set for ``_canonical`` to order. For values
that
+ # are not sets the two modes produce the same digest, so stored
+ # fingerprints are unaffected.
+ dumped = ModelMessagesTypeAdapter.dump_python(messages)
Review Comment:
`ModelMessagesTypeAdapter` sets `ser_json_bytes="base64"`, but that config
only applies to json-mode dumps. With `mode="python"` the `bytes` inside
`BinaryContent.data` reach `_canonical` raw, and `to_jsonable_python` decodes
bytes as utf-8 by default. A PNG raises `UnicodeDecodeError` there, so the
request fingerprints as `None`.
The attachment stays in the message history, so once a prompt contains an
image the model steps after it are never cached for that run. The merge-base
handled this case: a PNG in a `UserPromptPart` digested to `956c3851...` there
and gives `None` here. I ran it end to end with `CachingModel` and
`CachingToolset`: attempt 1 wrote no model entry, and attempt 2 reported
`replayed_model=0 replayed_tool=0`. The same happens for a `bytes` or
`BinaryContent` tool return, a `FilePart` in a response, and bytes in
`provider_details`. Bytes that happen to decode as utf-8 do not raise; they
produce a different digest than main, so those stored entries mismatch without
any warning.
`to_jsonable_python(value, bytes_mode="base64")` on line 173 produces the
same `iVBORw==` string the json-mode dump did, and in my probes it restored the
base digest for every carrier above. A test that pins `_digest` of a history
containing a `BinaryContent` to its pre-PR json-mode digest would guard it, in
the same shape as `test_plain_payload_digest_is_unchanged_by_normalization`.
The larger issue is that `_canonical` now has to reproduce pydantic's
json-mode rules by hand. Bytes are one case; dict keys (next comment) and
`nan`/`inf` (rendered as `NaN`/`Infinity` here, `null` in json mode) are two
more. If `_canonical` only did what pydantic cannot do stably, which is
ordering sets, refusing iterators and expanding dataclasses, and passed every
leaf and every key through `to_jsonable_python(bytes_mode="base64")`, pydantic
would stay the only renderer. A golden-digest fixture covering bytes, a
`datetime`, a `Decimal`, a set and a dict with non-string keys would then catch
any future drift.
##########
providers/common/ai/src/airflow/providers/common/ai/durable/fingerprint.py:
##########
@@ -119,13 +195,21 @@ def fingerprint_model_request(
output mode and schema, native tools, ...) so any change to what is sent
to the model invalidates the cached response.
- Returns ``None`` when the request cannot be serialized; ``None`` compares
- equal to ``None``, so requests that cannot be fingerprinted degrade to
- unverified positional replay rather than disabling caching.
+ Returns ``None`` when the request cannot be serialized even through
pydantic,
+ which prevents the step from being replayed or cached. Because model
settings
+ and message history are carried into every later request, such a value in
+ either usually degrades every subsequent model step of the run the same
way.
+ ``step`` is attached to that warning so the log names where it began.
"""
try:
- dumped = ModelMessagesTypeAdapter.dump_python(messages, mode="json")
- params =
_MODEL_REQUEST_PARAMETERS_ADAPTER.dump_python(model_request_parameters,
mode="json")
+ # ``mode="python"``, not ``mode="json"``: a json-mode dump renders a
set as
+ # a list in iteration order, and a tool that returned a set puts one
in the
+ # message history, where it would reach the hash already unstably
ordered.
+ # Python mode leaves it a set for ``_canonical`` to order. For values
that
+ # are not sets the two modes produce the same digest, so stored
Review Comment:
"Stored fingerprints are unaffected" is not true for any agent with
instructions, independent of the bytes case. `InstructionPart.id` is an
`InstructionId` dataclass with a `PlainSerializer(when_used="json")` that
renders `"agent"` / `"toolset:x:name"` only in json mode; python mode dumps it
as a nested dict, so a real `Agent(instructions="Be terse.")` digests
`d7694c37...` on main and `a4f1c4aa...` here, while an uninstructed agent
matches. The same applies to `nan`/`inf` (`null` vs `NaN`) and to any user
`BaseModel` field with a `when_used="json"` serializer. The effect is bounded:
the first retry that straddles the upgrade misses every cached model step and
re-runs the agent once, with the "model, prompt, message history, settings, or
tools changed" reason, and is stable afterwards. Either restore json-mode
rendering for these (handing leaves to `to_jsonable_python` in json semantics
gets most of the way) or replace the "unaffected" claims here, in `_digest`,
and in the docs wi
th a statement of the one-time re-run.
--
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]
