kaxil commented on code in PR #71575:
URL: https://github.com/apache/airflow/pull/71575#discussion_r4087586622
##########
providers/common/ai/src/airflow/providers/common/ai/durable/fingerprint.py:
##########
@@ -137,26 +164,40 @@ def fingerprint_model_request(
except (TypeError, ValueError):
# TypeError from json.dumps, ValueError covers
PydanticSerializationError
log.warning(
- "Durable: could not fingerprint model request; cached responses
for this "
- "step replay without verification"
+ "Durable: could not fingerprint model request; this step will not
be cached and will "
+ "execute live on retry. If the cause is in model settings or
message history, every "
+ "later model step of this run is affected too",
+ step=step,
Review Comment:
Now that `step` is here, the one thing still missing from this warning is
what could not be serialized. `PydanticSerializationError` carries the
offending type in its message ("Unable to serialize unknown type: <class
'httpx.Timeout'>"), so `except (TypeError, ValueError) as exc` plus
`error=str(exc)` here and on the tool warning below would let someone find the
setting or argument without a debugger.
##########
providers/common/ai/docs/operators/agent.rst:
##########
@@ -300,6 +300,23 @@ cache:
never replays responses that belong to a different conversation.
4. After successful completion, the cached steps are deleted.
+Fingerprints are computed from values normalized through pydantic, so ordinary
Review Comment:
Two things on this hunk. First, it now conflicts with main: #73523 moved
this whole section to `providers/common/ai/docs/durable_execution.rst` (the
anchor line "After successful completion, the cached steps are deleted." is at
line 103 there, and `agent.rst` now just says "Moved to
:doc:`../durable_execution`"), so these three paragraphs need to land there on
rebase. Second, "still fingerprint normally" is broader than what the code
delivers: a `set` argument fingerprints, but not stably across attempts (see
the `_digest` comment). If the canonicalization lands this sentence becomes
true as written; otherwise it needs the module docstring's "serialize to the
same bytes on every attempt" hedge with `set` named as the exception.
##########
providers/common/ai/src/airflow/providers/common/ai/durable/fingerprint.py:
##########
@@ -99,10 +114,18 @@ def _strip_volatile(messages_dump: list[dict[str, Any]])
-> list[dict[str, Any]]
def _digest(payload: Any) -> str:
Review Comment:
One thing the normalization changes on the tool path that the old
`json.dumps` did not: pydantic validates an `Iterable[int]` (or any
`Iterable[T]`) tool parameter lazily and hands the tool a `ValidatorIterator`,
and `to_jsonable_python` drains it to build the list. `call_tool` fingerprints
`tool_args` on line 73 and passes the same dict to the wrapped tool on line 93,
so the tool sees an exhausted iterator. I ran it in breeze with the installed
pydantic-ai and `def total(values: Iterable[int]) -> int: return sum(values)`:
the validated arg is a `ValidatorIterator`, after `to_jsonable_python` it
yields nothing and the tool returns 0, and that 0 gets cached under the
fingerprint of `[1, 2, 3]`. On the merge-base `json.dumps` raised `TypeError`
without advancing it, so the tool still got 6. Worth either rejecting
iterator-valued args before normalizing (return `None`, which is the not-cached
path), or normalizing a deep copy so the original objects reach the tool
untouched, plus a r
egression test with a real `FunctionToolset` and an `Iterable[int]` parameter.
##########
providers/common/ai/docs/operators/agent.rst:
##########
@@ -300,6 +300,23 @@ cache:
never replays responses that belong to a different conversation.
4. After successful completion, the cached steps are deleted.
+Fingerprints are computed from values normalized through pydantic, so ordinary
+types that are not JSON -- a ``datetime`` or ``Decimal`` tool argument, a
+dataclass in ``tool_choice`` -- still fingerprint normally. If a value cannot
be
+serialized even then, that step is not cached, and on retry it runs live rather
+than replaying an unverified entry.
+
+On the model path this is rarely confined to a single step: the causes are
such a
+value in ``model_settings``, which is attached to every request, or in the
message
+history, which every later request carries forward. Either one degrades all
+subsequent model steps the same way, leaving durable execution with nothing to
+replay, so the retry re-runs the agent at full cost. The
+``could not fingerprint model request`` warning names the step where this
began.
+
+A tool call is fingerprinted from its name, arguments and call id alone, so
+neither of those causes reaches it. One that cannot be fingerprinted is
reported
+as ``could not fingerprint tool call`` and costs only that call.
Review Comment:
"costs only that call" holds only when the live re-run returns the same
content as the original attempt. The return lands in the next request's
`ToolReturnPart.content`, which is part of the message history the following
model step fingerprints, so a tool that reads mutable state or returns a
timestamp makes every later model step miss too. Step 3 above already describes
that cascade for divergence; a clause like "and, if its result differs, the
model steps after it" keeps this sentence consistent with it.
##########
providers/common/ai/src/airflow/providers/common/ai/durable/fingerprint.py:
##########
@@ -99,10 +114,18 @@ def _strip_volatile(messages_dump: list[dict[str, Any]])
-> list[dict[str, Any]]
def _digest(payload: Any) -> str:
- # No ``default=`` fallback: a non-JSON-serializable value must raise so the
- # callers degrade to an unverifiable (None) fingerprint instead of hashing
- # process-local reprs like ``<object at 0x...>`` that never match on retry.
- canonical = json.dumps(payload, sort_keys=True)
+ # Normalize through pydantic first. Values that are not JSON types but do
have
+ # a deterministic pydantic serialization must not cost us a fingerprint: a
+ # ``datetime`` or ``Decimal`` tool argument (pydantic has already coerced
tool
+ # arguments by the time they arrive) and a dataclass in ``tool_choice`` are
+ # ordinary, and hash identically on every attempt. Plain JSON values pass
+ # through unchanged, so fingerprints written by earlier versions still
match.
+ #
+ # Still no ``default=`` fallback: a value pydantic cannot serialize either
+ # raises ``PydanticSerializationError`` (a ``ValueError``), so callers
degrade
+ # to an unverifiable ``None`` fingerprint rather than hashing process-local
+ # reprs like ``<object at 0x...>`` that never match on retry.
+ canonical = json.dumps(to_jsonable_python(payload), sort_keys=True)
Review Comment:
`to_jsonable_python` turns a `set` or `frozenset` into a list in iteration
order, and for `str` members that order depends on the process hash seed;
`sort_keys=True` only sorts dict keys. Since every task attempt is a fresh
process, a tool with a `set[str]` parameter now stores a digest on attempt 1
that attempt 2 never matches. Measured in breeze: six seeds gave six different
digests for one `set[str]` payload, and a real pydantic-ai run stored
`ba98e1f0...` then under another seed logged `reason='the conversation diverged
from the previous attempt' step=1` and re-ran the tool live, while the
same-seed control replayed. Before this PR that argument fingerprinted as
`None`, so this is the one input where the new code costs a live re-run every
retry rather than just declining to cache. A dataclass or `BaseModel` argument
with a `set[str]` field, a nested set inside a list, and a tool that *returns*
a set (via `ToolReturnPart.content` into the next model fingerprint) all behave
the
same way; `set[int]` and single-member sets happen to be stable. A recursive
canonicalizer that sorts set members by their JSON encoding before hashing
(dump in `mode="python"` first so dataclasses unwrap but sets stay sets) gave
one digest across all seeds and left plain payloads byte-identical when I tried
it. No current test can catch this because every stability test compares two
digests inside one process; a subprocess pair with different `PYTHONHASHSEED`,
or asserting `_digest({"a": {"x", "y"}}) == _digest({"a": ["x", "y"]})`, would.
--
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]
