YAshhh29 commented on code in PR #71575:
URL: https://github.com/apache/airflow/pull/71575#discussion_r4150122713
##########
providers/common/ai/src/airflow/providers/common/ai/durable/fingerprint.py:
##########
@@ -98,19 +123,107 @@ def _strip_volatile(messages_dump: list[dict[str, Any]])
-> list[dict[str, Any]]
return stripped
+def _is_dataclass_instance(value: Any) -> bool:
+ return dataclasses.is_dataclass(value) and not isinstance(value, type)
+
+
+def _refuse_iterators(value: Any) -> None:
+ """
+ Raise ``TypeError`` if ``value`` holds an iterator anywhere inside it.
+
+ Rendering an iterator consumes it, and pydantic validates an
``Iterable[T]``
+ tool parameter lazily into a ``ValidatorIterator``. Tool arguments are
+ fingerprinted before the tool runs, so rendering them would drain the
tool's
+ own input: the tool would see an empty sequence, and that wrong result
would be
+ cached under the fingerprint of the full one. Refusing degrades the step
to the
+ not-cached path instead.
+ """
+ if value is None or isinstance(value, (str, bytes, bytearray, bool, int,
float)):
+ return
+ if isinstance(value, Iterator):
+ raise TypeError(f"cannot fingerprint {type(value).__name__} without
consuming it")
+ if isinstance(value, Mapping):
+ children: Iterable[Any] = value.values()
+ elif isinstance(value, (list, tuple, set, frozenset)):
+ children = value
+ elif isinstance(value, BaseModel):
+ children = [getattr(value, name, None) for name in
type(value).model_fields]
+ elif _is_dataclass_instance(value):
+ children = [getattr(value, field.name, None) for field in
dataclasses.fields(value)]
+ else:
+ return
+ for child in children:
+ _refuse_iterators(child)
+
+
+def _order_sets(value: Any, rendered: Any) -> Any:
+ """
+ Return ``rendered`` with every list that pydantic rendered from a set
sorted.
+
+ ``rendered`` is pydantic's JSON rendering of ``value`` and is otherwise
kept as
+ is, so pydantic stays the only renderer: bytes, dates, dict keys, ``NaN``
and
+ custom serializers come out exactly as a json-mode dump renders them. The
one
+ thing pydantic cannot do stably is order a set. It lists the members in
+ iteration order, which for strings follows the interpreter's hash seed, and
+ every task attempt is a fresh process, so a ``set[str]`` would hash
differently
+ on each attempt and never replay. ``value`` is walked alongside
``rendered``
+ only to find those lists; a branch whose rendering does not line up with
the
+ object, such as one with a custom serializer, is left exactly as rendered.
+ """
+ if isinstance(value, (set, frozenset)):
+ if not isinstance(rendered, list) or len(rendered) != len(value):
+ return rendered
+ members = [_order_sets(member, item) for member, item in zip(value,
rendered)]
+ return sorted(members, key=lambda member: json.dumps(member,
sort_keys=True))
+ if isinstance(value, Mapping):
+ if not isinstance(rendered, dict):
+ return rendered
+ if len(rendered) != len(value):
+ # Distinct keys that render alike, such as 1 and "1": the digest
could no
+ # longer tell those payloads apart, so refuse rather than hash
either one.
+ raise TypeError("dict keys collide once rendered as JSON")
Review Comment:
You're right on both. It now only refuses when a key isn't a string and the
rendering ends up with fewer keys, since that's the only way keys can actually
collide. A serializer that drops or reorders keys does the same thing in both
dumps, so nothing gets paired with a neighbour's value anymore. The
dropped-`authorization` case hashes like main again, your two `order` lists
give different digests, and `{1: "a", "1": "b"}` is still refused. Tests cover
both, including a serializer nested in the annotation.
##########
providers/common/ai/src/airflow/providers/common/ai/durable/fingerprint.py:
##########
@@ -33,24 +33,43 @@
Fields that pydantic-ai regenerates on every attempt (message-level
``timestamp``/``run_id``/``conversation_id`` and part-level ``timestamp``)
-are excluded from the fingerprint. Requests that cannot be serialized to
-JSON fingerprint as ``None``, which degrades that step to unverified
-positional replay (the pre-fingerprint behavior) rather than disabling
-caching.
+are excluded from the fingerprint. Every payload is rendered by pydantic in
+JSON mode before hashing, so values that are not JSON types but render the same
+way on every attempt -- a ``datetime`` or ``Decimal`` tool argument, a
dataclass
+in ``tool_choice``, bytes in a ``BinaryContent`` -- still produce a usable
+fingerprint, and the message history hashes exactly as it always has. The one
+correction applied on top is that set members are ordered by their JSON
encoding,
+because a set of strings iterates in an order that follows the interpreter's
hash
+seed and every task attempt is a fresh process (see ``_order_sets``).
+
+A request that cannot be rendered fingerprints as ``None``: that step is
+neither replayed nor cached, and re-runs live instead of replaying without
+verification. A lazily validated ``Iterable`` argument lands there
deliberately,
+since hashing it would consume the input the tool has not read yet, as does a
+value pydantic cannot serialize at all. On the model path this is seldom
+confined to one step, because model settings, the tool definitions and the
+message history are carried into every later request, so durable execution
stops
+contributing anything for the rest of the run. A tool call is fingerprinted
from its name, arguments and
+call id alone, so it can only lose its own step.
Review Comment:
Fixed. It now says a tool call can't stop another step from being
fingerprinted, but a re-run that returns something different still invalidates
the model steps after it.
--
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]
