YAshhh29 commented on code in PR #71575: URL: https://github.com/apache/airflow/pull/71575#discussion_r3972084886
########## providers/common/ai/src/airflow/providers/common/ai/durable/fingerprint.py: ########## @@ -34,9 +34,8 @@ 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. +JSON fingerprint as ``None`` and re-run live instead of replaying without Review Comment: Thanks @kaxil — all three addressed in `98004ea265`. **1. Skipping the save when `fingerprint is None`** — done, in both `caching_model.py` and `caching_toolset.py`. The entry is unreachable exactly as you describe: the guard needs a non-`None` current fingerprint, and a real digest never equals a stored `None`, so the write only bought a dead entry per step plus a full rewrite of the cache blob each time. The step now returns before the save. One consequence worth noting: the warning below the comparison is now reachable only for entries that predate fingerprinting, since a `None` fingerprint is no longer something we can write. I also stopped incrementing `cached_model` / `cached_tool` on that path — otherwise the INFO summary reports steps as cached when nothing was persisted. Happy to drop that part if you'd rather keep this commit strictly to the dead-write fix. **2. "That step" undersells it** — agreed, and it matches what I see putting those cases through `fingerprint_model_request`. The docs now state that the realistic causes — a non-JSON value in `model_settings`, which accompanies every request, or in the message history, which every later request carries forward — degrade every subsequent model step, leaving durable execution with nothing to replay and a retry that re-runs the agent at full cost. It also points at the `could not fingerprint model request` warning as the marker for where it began. **3. The `_TRANSPORT_ONLY_SETTINGS` comment** — updated to the current consequence: the frozenset is load-bearing, because one non-JSON member now costs caching and replay for the whole run rather than just the verification. I carried the same correction into the module docstring, `fingerprint_model_request`'s docstring, both `log.warning` strings (they still read "this step will execute live on retry"), and the docstring on `test_httpx_timeout_does_not_disable_fingerprint`, which had the same stale framing. **Tests** — the two existing unverifiable-step tests now assert that nothing is written, plus one new test per wrapper for the fresh-cache case. All four fail without this change and pass with it. One adjacent thing I noticed but deliberately left out of scope: `DurableStorage.save_tool_result` skips the write for a non-serializable result, but the caller still counts it as cached, so the same counter skew exists on that path. Want that folded in here, or kept separate? -- 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]
