kaxil commented on code in PR #71575:
URL: https://github.com/apache/airflow/pull/71575#discussion_r4042148056
##########
providers/common/ai/src/airflow/providers/common/ai/durable/fingerprint.py:
##########
@@ -63,9 +65,13 @@
# Settings that control transport, not response content. Excluded from the
# fingerprint: changing them should not invalidate a cached response, and some
-# (``timeout`` can be an ``httpx.Timeout``) are not JSON-serializable, which
-# would otherwise force the whole fingerprint to ``None`` and silently disable
-# replay verification for every step.
+# (``timeout`` can be an ``httpx.Timeout``) are not JSON-serializable.
+#
+# This frozenset is load-bearing. Model settings ride along with every request,
+# so a single non-JSON member fingerprints every model step as ``None`` --
which
+# now costs durable execution entirely for the run (nothing is cached, nothing
+# is replayed), not merely the verification of a replay. Any non-JSON setting
+# must be listed here or normalized before it reaches the fingerprint.
Review Comment:
Following up on the comment rewrite: two things violate this invariant today
and they share one cause. `_digest` calls `json.dumps` with no `default=`, so
it rejects pydantic-native values that would hash perfectly deterministically.
`tool_choice` is one, since it accepts `ToolOrOutput`, a plain dataclass, and
it can't go in the frozenset because it does affect the response. The other is
on the tool side: `tool_args` reaches `fingerprint_tool_call` after pydantic
has already coerced them, so an ordinary tool with a `datetime` or `Decimal`
parameter fingerprints as `None` on every call, and after this PR that tool is
never cached at all. Both reproduce on 2.23.0 (the floor) and 2.42.0. Passing
the payload through `to_jsonable_python` before `json.dumps` covers both,
leaves plain values byte-identical so in-flight entries don't shift, and still
raises `PydanticSerializationError` on `extra_body=object()`, which the
existing `except (TypeError, ValueError)` catches.
##########
providers/common/ai/src/airflow/providers/common/ai/durable/caching_model.py:
##########
@@ -112,6 +114,11 @@ async def request(
)
response = await self.wrapped.request(messages, model_settings,
model_request_parameters)
+ if fingerprint is None:
+ # Storing this would write an entry the guard above can never
accept,
+ # once per step, each write rewriting the whole cache blob.
Review Comment:
The blob rewrite is specific to the `ObjectStorage` backend.
`TaskStateStoreDurableStorage.save_model_response` sets a single key instead,
and this class holds a `DurableStorageProtocol`, so the first half of the
comment carries the point on its own. Separately, this debug line is the only
place `step` gets attached: the `fingerprint.py` warning that the new docs
paragraph points at as the marker has no `step`, so at production log levels
there is nothing to say which step stopped fingerprinting. Threading `step`
into that warning would be worth more than the debug line.
##########
providers/common/ai/src/airflow/providers/common/ai/durable/caching_model.py:
##########
@@ -112,6 +114,11 @@ async def request(
)
response = await self.wrapped.request(messages, model_settings,
model_request_parameters)
+ if fingerprint is None:
+ # Storing this would write an entry the guard above can never
accept,
+ # once per step, each write rewriting the whole cache blob.
+ log.debug("Durable: not caching model response that cannot be
verified on replay", step=step)
+ return response
Review Comment:
On your question about whether to keep the counter change: I think this half
needs to go the other way. `cached_model` and `cached_tool` are rendered in
`operators/agent.py` as `executed %d new steps`, and agent.rst describes that
line as reporting steps replayed vs executed fresh, so the field means
executed, not persisted. Skipping the increment here makes a run that replays
step 0 and runs an unverifiable step 1 live print `executed 0 new steps` after
paying for a model call, which is the retry cost your new docs paragraph is
trying to make visible. The persisted reading doesn't hold anyway, since
`save_tool_result` returns without writing on a non-serializable result and the
counter still increments there. Either keep the increment and reword the log
line, or add a separate not-persisted counter; the two new tests assert
`cached_model == 0` so they'd move with it.
##########
providers/common/ai/tests/unit/common/ai/durable/test_caching_model.py:
##########
@@ -158,6 +158,40 @@ async def
test_legacy_entry_without_fingerprint_treated_as_miss(
assert result is sample_response
mock_model.request.assert_called_once()
+ @pytest.mark.asyncio
+ async def test_unverifiable_current_request_treated_as_miss(
+ self, mock_model, mock_storage, counter, sample_response
+ ):
+ stale = ModelResponse(parts=[TextPart(content="stale")])
+ mock_storage.load_model_response.return_value = (stale, None)
Review Comment:
One case none of the four new tests reaches: a real stored fingerprint
paired with a current request that cannot be fingerprinted. The read side was
already safe there before this PR, since a real hash never equalled `None`, but
the write side changed. That good entry used to get clobbered with a
`fingerprint=None` one and now it survives for a later attempt to replay, which
is the one new write behaviour with no coverage.
##########
providers/common/ai/docs/operators/agent.rst:
##########
@@ -300,6 +300,17 @@ cache:
never replays responses that belong to a different conversation.
4. After successful completion, the cached steps are deleted.
+If a model request or tool call cannot be fingerprinted -- it carries a value
+that will not serialize to JSON -- that step is not cached, and on retry it
runs
+live rather than replaying an unverified entry. This is rarely confined to a
+single step: the usual causes are a non-serializable value in
``model_settings``,
Review Comment:
Neither of these causes applies to a tool call, though the paragraph opens
by covering both. `fingerprint_tool_call` only sees the tool name, args and
call id, so `model_settings` and message history cannot reach it, and the
warning it emits is `could not fingerprint tool call`, not the string quoted
below. The dominant tool-call cause is an ordinary typed tool parameter, which
is the `_digest` point on fingerprint.py. If that gets normalized this mostly
resolves itself; otherwise the causes here need scoping to the model path.
--
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]
