Skip to content

Commit c080fc0

Browse files
cosmicBboyclaude
andcommitted
fix: root action cache key honors Literal.hash, like sub-actions do
Content-based caching worked between tasks but silently degraded to URI-based caching at the run entrypoint. Re-running a cached root task with byte-identical inputs uploaded to a fresh URI missed the cache; the same task invoked as a sub-action hit. The two paths derive the inputs hash differently. A sub-action's is computed in-process by the controller via `generate_inputs_repr_for_literal`, which substitutes `Literal.hash` for the literal's contents. A root action's comes from `OffloadedInputData.inputs_hash`, which the backend computes over the *marshaled* inputs (cloud `shared_service/cache/cache_key.go:HashInputs`, called from `dataproxy/service/dataproxy_flyte.go:UploadInputs`) — folding in the offloaded blob URI and ignoring `Literal.hash`. Every `flyte.run` uploads to a new URI, so the key changed on every submission. `_submit_remote` now recomputes that field over the same representation the controller uses. The value is exactly what a sub-action would compute, so the two land on the same cache key: the backend folds the field in as `sha256(inputsHash + taskName + interfaceHash + cacheVersion)` (cloud `workflow/service/utils.go:generateCacheKeyFromInputsHash`), which is the same formula as `generate_cache_key_hash`. The shared byte builder is extracted into `_named_literals_repr` so the two representations cannot drift. Cache-ignored inputs are filtered before hashing, matching `filterInputsForHash` on the backend's upload path. Without that, a task combining `Cache(ignored_inputs=...)` with a content-hashed input would key on the very inputs the user asked to exclude. `generate_content_inputs_hash` returns None unless a hashed input survives that filtering, leaving the backend's value in place. Without the gate every root run's key would change and every cache entry already written against the old keys would become unreachable; with it, only callers who opted into content hashing are affected. Overriding the field is safe because the backend treats it as an opaque string: `generateCacheKeyFromInputsHash` concatenates and re-hashes it, never parsing, length-checking, or verifying it against the blob. Two backend deployments even encode it differently — sha256/std-base64 in `cache_key.go`, FNV-64a/url-base64 in `flyte2/dataproxy/service/dataproxy_service.go:hashInputsProto` — which only works because nothing downstream cares. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QCM3MeG4tcFdFCnEiiHeik
1 parent 3239753 commit c080fc0

6 files changed

Lines changed: 499 additions & 9 deletions

File tree

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
"""
2+
Content-based caching for the *root* task of a run.
3+
4+
`hash_flyte_dataframe.py` shows content-based caching between tasks: a driver produces a
5+
DataFrame and calls a cached consumer twice, and the second call hits. This example covers the
6+
other entrypoint — passing a locally-built DataFrame straight into `flyte.run(...)`, so the
7+
cached task *is* the root action of the run.
8+
9+
The two paths compute the cache key differently. A sub-action's key is computed by the
10+
controller in-process, which substitutes `Literal.hash` for the literal's contents. A root
11+
action's key is derived from the offloaded inputs, which reference the upload URI — and every
12+
`flyte.run` uploads the DataFrame to a fresh URI. So without a content hash the second run
13+
misses even though the bytes are identical.
14+
15+
Passing `hash_method=` to `DataFrame.from_local_sync` makes both runs agree: the key follows
16+
the content, not where it happened to land in blob storage.
17+
18+
Run it twice; `check_cache_hit` below asserts the second run returns the first run's value.
19+
"""
20+
21+
import pandas as pd
22+
23+
import flyte
24+
from flyte import Cache
25+
from flyte.io import DataFrame, HashFunction
26+
27+
img = flyte.Image.from_debian_base(name="flyte-root-hash").with_pip_packages("pandas", "pyarrow")
28+
29+
env = flyte.TaskEnvironment(
30+
"flyte_root_action_hash",
31+
image=img,
32+
resources=flyte.Resources(cpu="1", memory="2Gi"),
33+
)
34+
35+
SAMPLE_DATA = {"id": [1, 2, 3, 4, 5], "value": [100, 200, 300, 400, 500]}
36+
37+
38+
def hash_pandas_dataframe(df: pd.DataFrame) -> str:
39+
"""Content-based hash: the same rows always produce the same digest."""
40+
return str(pd.util.hash_pandas_object(df).sum())
41+
42+
43+
@env.task(cache=Cache(behavior="override", version_override="v1"))
44+
async def main(df: DataFrame) -> str:
45+
"""Cached root task.
46+
47+
The random number is the cache probe: it is regenerated on every real execution, so two
48+
runs returning the same string can only mean the second one was served from the cache.
49+
"""
50+
import random
51+
52+
pdf = await df.open(pd.DataFrame).all()
53+
return f"rows={len(pdf)}, total={pdf['value'].sum()}, random={random.randint(1, 1000000)}"
54+
55+
56+
def build_input() -> DataFrame:
57+
"""The DataFrame to submit, tagged with a content-based hash.
58+
59+
Without `hash_method` the cache key would follow the (per-run, always new) upload URI and
60+
the second run would miss.
61+
"""
62+
return DataFrame.from_local_sync(
63+
pd.DataFrame(SAMPLE_DATA),
64+
hash_method=HashFunction.from_fn(hash_pandas_dataframe),
65+
)
66+
67+
68+
if __name__ == "__main__":
69+
flyte.init_from_config()
70+
71+
# Two independent submissions of the same content. Each uploads to its own URI.
72+
run1 = flyte.run(main, df=build_input())
73+
print(f"Run 1: {run1.url}")
74+
run1.wait()
75+
result1 = run1.outputs()[0]
76+
77+
run2 = flyte.run(main, df=build_input())
78+
print(f"Run 2: {run2.url}")
79+
run2.wait()
80+
result2 = run2.outputs()[0]
81+
82+
print(f"\nRun 1: {result1}")
83+
print(f"Run 2: {result2}")
84+
if result1 == result2:
85+
print("\n✓ Cache hit — the new upload URI did not change the cache key.")
86+
else:
87+
print("\n✗ Cache miss — the root action's key still tracks the upload URI.")

examples/integration_tests.py

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -393,6 +393,35 @@ async def test_advanced_local_tasks(flyte_client):
393393
await _run_and_wait(parallel_main_no_io, "test_advanced_local_tasks", q="hello")
394394

395395

396+
@pytest.mark.integration
397+
@pytest.mark.asyncio
398+
async def test_advanced_hash_root_action(flyte_client):
399+
"""Content-based caching must work when the cached task is the run's root action.
400+
401+
A sub-action's cache key is computed by the controller, which substitutes `Literal.hash`
402+
for the literal's contents; a root action's key is derived from the offloaded inputs, which
403+
reference the upload URI. Each `flyte.run` uploads to a fresh URI, so this only passes if
404+
the content hash — not the URI — drives the key.
405+
"""
406+
from examples.advanced.hash_root_action import build_input, main
407+
408+
results = []
409+
for i in (1, 2):
410+
# Rebuilt each time, so run 2 uploads identical bytes to a brand-new URI.
411+
run = await flyte.with_runcontext(log_level=logging.DEBUG).run.aio(main, df=build_input())
412+
print(f"\n[test_advanced_hash_root_action] Run {i}: {run.url}")
413+
run.wait()
414+
detail = await run.action.details()
415+
if detail.error_info:
416+
raise RuntimeError(f"Run {i} failed with error: {detail.error_info.message}")
417+
results.append(run.outputs()[0])
418+
419+
# The task embeds a fresh random number on every real execution, so identical outputs
420+
# mean run 2 was served from run 1's cache entry.
421+
assert results[0] == results[1], f"root action cache miss across a new upload URI: {results[0]!r} != {results[1]!r}"
422+
print(" Cache hit across a new upload URI\n")
423+
424+
396425
@pytest.mark.integration
397426
@pytest.mark.asyncio
398427
async def test_advanced_multi_loops(flyte_client):

src/flyte/_internal/runtime/convert.py

Lines changed: 59 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -668,6 +668,20 @@ def generate_inputs_repr_for_literal(literal: literals_pb2.Literal) -> bytes:
668668
return literal.SerializeToString(deterministic=True)
669669

670670

671+
def _named_literals_repr(inputs: list[common_pb2.NamedLiteral]) -> bytes:
672+
"""Deterministic byte representation of named inputs, honoring any `Literal.hash`.
673+
674+
Names are folded in so argument order and naming matter, with `:`/`;` framing so distinct
675+
inputs cannot concatenate into the same bytes.
676+
"""
677+
combined_bytes = b""
678+
for named_literal in inputs:
679+
name_bytes = named_literal.name.encode("utf-8")
680+
literal_bytes = generate_inputs_repr_for_literal(named_literal.value)
681+
combined_bytes += name_bytes + b":" + literal_bytes + b";"
682+
return combined_bytes
683+
684+
671685
def generate_inputs_hash_for_named_literals(
672686
inputs: list[common_pb2.NamedLiteral],
673687
) -> str:
@@ -685,16 +699,52 @@ def generate_inputs_hash_for_named_literals(
685699
if not inputs:
686700
return ""
687701

688-
# Build the byte representation by concatenating each literal's representation
689-
combined_bytes = b""
690-
for named_literal in inputs:
691-
# Add the name to ensure order matters
692-
name_bytes = named_literal.name.encode("utf-8")
693-
literal_bytes = generate_inputs_repr_for_literal(named_literal.value)
694-
# Combine name and literal bytes with a separator to avoid collisions
695-
combined_bytes += name_bytes + b":" + literal_bytes + b";"
702+
return hash_data(_named_literals_repr(inputs))
703+
696704

697-
return hash_data(combined_bytes)
705+
def literal_carries_hash(literal: literals_pb2.Literal) -> bool:
706+
"""Whether `literal` (or anything nested in it) carries a user-supplied content hash.
707+
708+
Mirrors the cases `generate_inputs_repr_for_literal` substitutes a hash for, so the two
709+
stay in step: a literal that would change the repr is exactly one this reports True for.
710+
"""
711+
if literal.hash:
712+
return True
713+
if literal.HasField("collection"):
714+
return any(literal_carries_hash(nested) for nested in literal.collection.literals)
715+
if literal.HasField("map"):
716+
return any(literal_carries_hash(nested) for nested in literal.map.literals.values())
717+
return False
718+
719+
720+
def generate_content_inputs_hash(
721+
inputs: common_pb2.Inputs,
722+
ignored_input_vars: List[str],
723+
) -> Optional[str]:
724+
"""Content-addressed replacement for `OffloadedInputData.inputs_hash`, or None to defer.
725+
726+
The backend fills that field by hashing the *marshaled* inputs, which folds in the offloaded
727+
blob URI and ignores `Literal.hash` (cloud `shared_service/cache/cache_key.go:HashInputs`) —
728+
so a root action re-run with identical content at a fresh upload URI misses the cache, while
729+
the same task invoked as a sub-action hits.
730+
731+
Returns exactly what a sub-action would compute, so both land on the same cache key: the
732+
backend derives it as `sha256(inputsHash + taskName + interfaceHash + cacheVersion)` (cloud
733+
`workflow/service/utils.go:generateCacheKeyFromInputsHash`), the same formula as
734+
`generate_cache_key_hash` below. Overriding a server-computed field is safe because it is
735+
only ever concatenated and re-hashed — never parsed, nor checked against the blob.
736+
737+
`ignored_input_vars` is filtered out first, matching the backend's `filterInputsForHash`;
738+
without it, inputs excluded via `Cache(ignored_inputs=...)` would leak into the key. None
739+
when no hashed input survives that filtering — the common case, where deferring to the
740+
backend keeps existing cache keys, and the entries written against them, valid.
741+
"""
742+
if not inputs or not inputs.literals:
743+
return None
744+
literals = [named for named in inputs.literals if named.name not in ignored_input_vars]
745+
if not any(literal_carries_hash(named.value) for named in literals):
746+
return None
747+
return generate_inputs_hash_for_named_literals(literals)
698748

699749

700750
def generate_inputs_hash_from_proto(inputs: common_pb2.Inputs) -> str:

src/flyte/_run.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -688,6 +688,7 @@ async def _submit_remote(
688688
from flyteidl2.workflow import run_service_pb2
689689

690690
import flyte.errors
691+
from flyte._internal.runtime.convert import generate_content_inputs_hash
691692
from flyte.remote import Run
692693

693694
try:
@@ -711,6 +712,25 @@ async def _submit_remote(
711712
upload_resp = await get_client().dataproxy_service.upload_inputs(upload_req)
712713
offloaded_input_data = upload_resp.offloaded_input_data
713714

715+
# The hash the server derives from the marshaled inputs folds in the offloaded
716+
# blob URI and ignores `Literal.hash`, so content-based caching silently degrades
717+
# to URI-based caching at the run entrypoint: identical content uploaded to a
718+
# fresh URI misses. Sub-actions don't have this problem — the controller hashes
719+
# the same inputs through `generate_inputs_repr_for_literal`, which substitutes
720+
# the content hash. Recompute over that representation so the root action agrees.
721+
# Returns None (leaving the server's value alone) unless a hashed input survives
722+
# cache-ignore filtering, so cache keys for everyone else are untouched.
723+
#
724+
# `task_spec` is populated on every path into here, including the by-reference
725+
# one where `task_id` is also set (`task_spec = task.pb2.spec` on a fetched
726+
# task), so the ignore list is always the registered task's own.
727+
md = task_spec.task_template.metadata if task_spec is not None else None
728+
content_hash = generate_content_inputs_hash(
729+
proto_inputs, list(md.cache_ignore_input_vars) if md else []
730+
)
731+
if content_hash is not None:
732+
offloaded_input_data.inputs_hash = content_hash
733+
714734
create_req = run_service_pb2.CreateRunRequest(
715735
run_id=run_id,
716736
project_id=project_id,

tests/flyte/internal/runtime/test_convert.py

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
LiteralMap,
1515
Primitive,
1616
Scalar,
17+
StructuredDataset,
18+
StructuredDatasetMetadata,
1719
)
1820
from flyteidl2.core.types_pb2 import (
1921
BlobType,
@@ -1593,3 +1595,139 @@ async def test_convert_inputs_no_kickoff_key_is_noop():
15931595
out = await convert.convert_inputs_to_native(Inputs(proto_inputs=proto), interface)
15941596

15951597
assert out == {"x": 7}
1598+
1599+
1600+
# ---------------------------------------------------------------------------
1601+
# Content-addressed inputs hash for root actions
1602+
#
1603+
# The backend derives `OffloadedInputData.inputs_hash` from the marshaled inputs, which folds
1604+
# in the offloaded blob URI and ignores `Literal.hash`. Sub-actions don't go through that path
1605+
# — the controller hashes via `generate_inputs_repr_for_literal`, which substitutes the content
1606+
# hash — so content-based caching worked for sub-actions but degraded to URI-based caching at
1607+
# the run entrypoint. `generate_content_inputs_hash` closes that gap.
1608+
# ---------------------------------------------------------------------------
1609+
1610+
_CONTENT_HASH = "sha256-of-geoparquet-bytes"
1611+
1612+
1613+
def _sd_literal(uri: str, hash_val: str | None = None) -> Literal:
1614+
return Literal(
1615+
scalar=Scalar(
1616+
structured_dataset=StructuredDataset(
1617+
uri=uri,
1618+
metadata=StructuredDatasetMetadata(structured_dataset_type=StructuredDatasetType(format="parquet")),
1619+
)
1620+
),
1621+
hash=hash_val,
1622+
)
1623+
1624+
1625+
def _int_literal(v: int) -> Literal:
1626+
return Literal(scalar=Scalar(primitive=Primitive(integer=v)))
1627+
1628+
1629+
def _named_inputs(**kwargs: Literal) -> _task_common_pb2.Inputs:
1630+
return _task_common_pb2.Inputs(
1631+
literals=[_task_common_pb2.NamedLiteral(name=name, value=lit) for name, lit in kwargs.items()]
1632+
)
1633+
1634+
1635+
def test_content_inputs_hash_ignores_upload_uri():
1636+
"""Identical content re-uploaded to a fresh URI must produce the same key."""
1637+
run1 = _named_inputs(aoi=_sd_literal("s3://bkt/run-1/abc/0", _CONTENT_HASH))
1638+
run2 = _named_inputs(aoi=_sd_literal("s3://bkt/run-2/xyz/0", _CONTENT_HASH))
1639+
1640+
assert convert.generate_content_inputs_hash(run1, []) == convert.generate_content_inputs_hash(run2, [])
1641+
1642+
1643+
def test_content_inputs_hash_tracks_content():
1644+
same_uri_other_content = _named_inputs(aoi=_sd_literal("s3://bkt/run-1/abc/0", "a-different-digest"))
1645+
baseline = _named_inputs(aoi=_sd_literal("s3://bkt/run-1/abc/0", _CONTENT_HASH))
1646+
1647+
assert convert.generate_content_inputs_hash(baseline, []) != convert.generate_content_inputs_hash(
1648+
same_uri_other_content, []
1649+
)
1650+
1651+
1652+
def test_content_inputs_hash_is_name_sensitive():
1653+
"""Same literal bound to a different parameter is a different call."""
1654+
as_aoi = _named_inputs(aoi=_sd_literal("s3://bkt/1", _CONTENT_HASH))
1655+
as_other = _named_inputs(other=_sd_literal("s3://bkt/1", _CONTENT_HASH))
1656+
1657+
assert convert.generate_content_inputs_hash(as_aoi, []) != convert.generate_content_inputs_hash(as_other, [])
1658+
1659+
1660+
@pytest.mark.parametrize(
1661+
"name,inputs",
1662+
[
1663+
("empty", _task_common_pb2.Inputs()),
1664+
("plain scalar", _named_inputs(x=Literal(scalar=Scalar(primitive=Primitive(integer=5))))),
1665+
("dataframe without a hash", _named_inputs(aoi=_sd_literal("s3://bkt/run-1/abc/0"))),
1666+
],
1667+
)
1668+
def test_content_inputs_hash_defers_when_no_input_is_hashed(name, inputs):
1669+
"""None means "leave the backend's value alone", which keeps already-written cache entries
1670+
reachable for the overwhelmingly common case of no content hashes at all."""
1671+
assert convert.generate_content_inputs_hash(inputs, []) is None
1672+
1673+
1674+
def test_content_inputs_hash_equals_the_sub_action_hash():
1675+
"""The value must be exactly what the controller computes for a sub-action.
1676+
1677+
The backend folds this field into the cache key as
1678+
`sha256(inputsHash + taskName + interfaceHash + cacheVersion)`
1679+
(cloud `workflow/service/utils.go:generateCacheKeyFromInputsHash`), which is the same
1680+
formula as `generate_cache_key_hash`. Equal inputs hashes therefore mean equal cache keys,
1681+
so a root action and a sub-action of the same task share cache entries.
1682+
"""
1683+
inputs = _named_inputs(aoi=_sd_literal("s3://bkt/1", _CONTENT_HASH))
1684+
1685+
assert convert.generate_content_inputs_hash(inputs, []) == convert.generate_inputs_hash_from_proto(inputs)
1686+
1687+
1688+
def test_content_inputs_hash_excludes_cache_ignored_inputs():
1689+
"""Matches `filterInputsForHash` on the backend's upload path.
1690+
1691+
Without this, a task combining `Cache(ignored_inputs=...)` with a content-hashed input
1692+
would key on the very inputs the user asked to exclude.
1693+
"""
1694+
run1 = _named_inputs(aoi=_sd_literal("s3://bkt/1", _CONTENT_HASH), seed=_int_literal(1))
1695+
run2 = _named_inputs(aoi=_sd_literal("s3://bkt/1", _CONTENT_HASH), seed=_int_literal(2))
1696+
1697+
assert convert.generate_content_inputs_hash(run1, ["seed"]) == convert.generate_content_inputs_hash(run2, ["seed"])
1698+
# ...and without the ignore list, the differing input does move the key.
1699+
assert convert.generate_content_inputs_hash(run1, []) != convert.generate_content_inputs_hash(run2, [])
1700+
1701+
1702+
def test_content_inputs_hash_defers_when_only_ignored_inputs_are_hashed():
1703+
"""Nothing left to fix once the hashed input is filtered out — leave the backend's value."""
1704+
inputs = _named_inputs(aoi=_sd_literal("s3://bkt/1", _CONTENT_HASH), seed=_int_literal(1))
1705+
1706+
assert convert.generate_content_inputs_hash(inputs, ["aoi"]) is None
1707+
1708+
1709+
@pytest.mark.parametrize(
1710+
"name,wrap",
1711+
[
1712+
("collection", lambda lit: Literal(collection=LiteralCollection(literals=[lit]))),
1713+
("map", lambda lit: Literal(map=LiteralMap(literals={"k": lit}))),
1714+
],
1715+
)
1716+
def test_content_inputs_hash_sees_nested_hashes(name, wrap):
1717+
"""`Literal.hash` nested in a collection/map counts, matching what the repr substitutes."""
1718+
run1 = _named_inputs(aoi=wrap(_sd_literal("s3://bkt/run-1/abc/0", _CONTENT_HASH)))
1719+
run2 = _named_inputs(aoi=wrap(_sd_literal("s3://bkt/run-2/xyz/0", _CONTENT_HASH)))
1720+
1721+
assert convert.generate_content_inputs_hash(run1, []) is not None
1722+
assert convert.generate_content_inputs_hash(run1, []) == convert.generate_content_inputs_hash(run2, [])
1723+
1724+
1725+
def test_root_and_sub_action_agree_on_uri_independence():
1726+
"""The property the fix is really about: both paths now ignore a changed upload URI."""
1727+
run1 = _named_inputs(aoi=_sd_literal("s3://bkt/run-1/abc/0", _CONTENT_HASH))
1728+
run2 = _named_inputs(aoi=_sd_literal("s3://bkt/run-2/xyz/0", _CONTENT_HASH))
1729+
1730+
# sub-action path (controller-side), unchanged by this fix
1731+
assert convert.generate_inputs_hash_from_proto(run1) == convert.generate_inputs_hash_from_proto(run2)
1732+
# root-action path (client-side), previously URI-sensitive via the server's hash
1733+
assert convert.generate_content_inputs_hash(run1, []) == convert.generate_content_inputs_hash(run2, [])

0 commit comments

Comments
 (0)