YAshhh29 commented on code in PR #71575: URL: https://github.com/apache/airflow/pull/71575#discussion_r4105750067
########## 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: Confirmed, including the parts you scoped out: `set[int]` and single-member sets are stable, `set[str]` isn't. Six hash seeds gave me five distinct digests for one `set[str]` payload. `_canonical` now orders set and frozenset members by their own JSON encoding — one digest across every seed I tried, and plain payloads unchanged. It recurses, so a set nested in a list and a `set[str]` field on a dataclass or `BaseModel` are covered. `BaseModel` goes through `model_dump(mode="python")`, so its nested sets arrive as sets and get ordered rather than being flattened on the way in. The tool-return case needed one more change: the message dump was `mode="json"`, which turned a returned set into a list before `_canonical` ever saw it (four digests across seeds). Messages and request parameters now dump in `mode="python"`. I checked that for anything that isn't a set, the two modes give the same digest — including on a 451-message history — so stored fingerprints don't move. On tests, you were right that in-process comparisons can't catch this, so there are both kinds: deterministic ones that assert the canonical form directly, and a subprocess test across four `PYTHONHASHSEED` values. The subprocess one earned its keep — with the ordering switched off, several in-process set assertions still passed by luck, while it failed with three distinct digests. -- 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]
