Repository navigation
fix(utilities): make hash_objects type-preserving so tuple/list inputs do not collide - #23819
Shahzeb-DataEngineer wants to merge 6 commits into
Conversation
…s do not collide hash_objects tried JSON first and only fell back to cloudpickle on failure. JSON erases the tuple/list distinction and coerces int dict keys to strings, so different task inputs hashed identically and the second call was served the first call's cached result (stale results via task_input_hash and the Inputs cache policy). Encode values into a type-tagged JSON structure before serializing: tuples hash differently from lists, int dict keys differently from str keys, and set/frozenset distinctly from lists, while equal inputs keep equal hashes and dict/set ordering still does not affect the hash. Values the encoding cannot represent are passed through to the existing JSON object encoder (or the cloudpickle fallback) as before. Note: this changes the hash values themselves, invalidating cache keys computed by earlier versions. Closes PrefectHQ#23817 Signed-off-by: Shahzeb Ali <[email protected]>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8b7cd34c8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| serializer = JSONSerializer(dumps_kwargs={"sort_keys": True}) | ||
| return stable_hash(serializer.dumps((args, kwargs)), hash_algo=hash_algo) | ||
| return stable_hash( | ||
| serializer.dumps(_encode_for_hashing((args, kwargs))), |
There was a problem hiding this comment.
When Prefect is upgraded while an unversioned custom Block schema is unchanged, this unconditional re-encoding changes its checksum because Block._calculate_schema_checksum() calls hash_objects on the JSON-schema dictionary (src/prefect/blocks/core.py:573). The next save or registration therefore misses the persisted checksum and creates another schema (core.py:1578-1589), changing the block document's schema identity despite no schema change; preserve the legacy encoding for unambiguous inputs or decouple cache-input hashing from persistent schema checksums.
AGENTS.md reference: src/prefect/AGENTS.md:L5-L7
Useful? React with 👍 / 👎.
hash_objects is now type-preserving, so the checksums computed from block schemas changed. Update the hardcoded expected values to the new checksums (verified identical across all CI jobs). Signed-off-by: Shahzeb Ali <[email protected]>
The type-preserving encoding re-hashed every input, which changed persisted block schema checksums: on upgrade, an unchanged unversioned custom Block schema would get a new checksum and the next save would create a duplicate schema instead of matching the persisted one (Codex review on PrefectHQ#23819). Only apply the type-tagged encoding when the input actually contains an ambiguity plain JSON would erase (tuples, sets, non-string dict keys). Everything else hashes exactly as before, so existing block schema checksums are unchanged and the checksum expectations in tests/blocks/test_core.py and tests/server/models/test_block_schemas.py revert to their original values.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4aca565c34
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return False | ||
| _seen.add(id(obj)) | ||
| return any(_contains_ambiguous_type(item, _seen) for item in obj) | ||
| return False |
There was a problem hiding this comment.
When a cached task receives a Pydantic model whose Any or union field holds a tuple in one call and a list in another, this fallback reports no ambiguity and selects the legacy JSON path. prefect_json_object_encoder serializes the model through model_dump(mode="json"), which normalizes both field values to the same JSON array, so task_input_hash still gives both calls the same cache key and the later task can return a stale result; the ambiguity check and tagged encoder need to examine serializer-supported wrappers rather than treating every non-builtin container as unambiguous.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, thanks. Fixed in 179f4b7: the ambiguity scan and the tagged encoder now look inside Pydantic models (via model_dump(mode="python")) and dataclasses (via asdict), so a tuple nested in a model field hashes differently from a list. Added regression tests in tests/utilities/test_hashing.py. Verified locally that unambiguous inputs (including block schema dicts) still hash exactly as before.
…sh inputs Codex review on PrefectHQ#23819 noted the ambiguity scan missed tuples hidden inside Pydantic models: the JSON serializer dumps models with model_dump(mode="json"), which normalizes tuple and list fields to the same array, so the collision survived inside the wrapper. Scan and encode models (and dataclasses, which the serializer flattens the same way) through their python-mode dump, which preserves container types. Unambiguous inputs still take the legacy path, so persistent hashes such as block schema checksums are unchanged. Signed-off-by: Shahzeb Ali <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 179f4b7307
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| dumped = obj.model_dump(mode="python") | ||
| except Exception: | ||
| return False | ||
| return _contains_ambiguous_type(dumped, _seen) |
There was a problem hiding this comment.
On CPython, when one argument is a list or dictionary with multiple Pydantic models and only a later sibling contains the tuple/list distinction, the first sibling's dumped dictionary is freed after this recursive call while its ID remains in _seen; the next model_dump() reuses that ID and is treated as already visited. The scan then selects legacy JSON for both variants, where tuple and list fields serialize identically, so task_input_hash can still serve the stale cached result (the dataclasses.asdict branch below has the same lifetime bug). Keep the projected objects alive or use an active recursion stack.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, thanks. Fixed in 6c3e3ac: the scan now keeps every model/dataclass dump alive in a _keepalive list for the whole traversal, so a later sibling model's dump can no longer reuse a freed dump's id and get skipped as already visited. Added a regression test with the tuple in a later sibling model.
| if isinstance(obj, (set, frozenset)): | ||
| items = sorted( | ||
| (_encode_for_hashing(item) for item in obj), | ||
| key=lambda item: json.dumps(item, sort_keys=True, default=str), |
There was a problem hiding this comment.
When a set contains two encoder-supported objects whose str() forms are equal but whose final JSON differs—for example, same-valued members named Color.RED from two different plain Enum classes, which also have equal hashes—these sort keys tie. On CPython, building the equal set in opposite insertion orders then preserves the opposite item order, and prefect_json_object_encoder includes each enum's qualified class, so hash_objects returns different hashes for equal sets and cache keys become insertion-order dependent. Use the same class-aware serialized form as a tiebreaker instead of default=str.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, thanks. Fixed in 6c3e3ac: set items and dict entries now sort by (json form, qualified class name), so same-named enum members from different classes no longer tie on str() and equal sets hash the same regardless of insertion order. Added a regression test.
Merging this PR will not alter performance1 warning
Comparing 
Footnotes
|
…t ties Codex review on PrefectHQ#23819 found two follow-ups in the type-preserving hashing change. First, the ambiguity scan records the ids of temporary model dumps in `_seen`, but once a dump was freed a later sibling model's dump could reuse its id and be skipped as already visited, so a tuple hidden in a later sibling model was missed and both inputs hashed identically. Keep every dump alive until the top-level scan finishes. Second, the set sort key tied for values whose `str()` form is the same while their serialized form differs, for example members named `Color.RED` from two different `Enum` classes, which also hash equally; `sorted` then kept insertion order and equal sets hashed differently. Break ties with the qualified class name. Adds regression tests for both cases. Signed-off-by: Shahzeb Ali <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c3e3acafb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return { | ||
| "__prefect_type__": "pydantic", | ||
| "__class__": to_qualified_name(obj.__class__), | ||
| "data": _encode_for_hashing(obj.model_dump(mode="python")), |
There was a problem hiding this comment.
Whenever a model contains an ambiguous value and another field relies on @field_serializer(..., when_used="json"), this Python-mode projection skips that field serializer. For example, a tuple field plus a _thread.lock field serialized to a string in JSON mode used to hash successfully through prefect_json_object_encoder, but the new payload contains the raw lock; JSON then raises and cloudpickle cannot pickle it, so hash_objects(..., raise_on_failure=True) now aborts task caching. Build the tagged projection without bypassing the model's JSON-only serializers.
AGENTS.md reference: src/prefect/AGENTS.md:L5-L7
Useful? React with 👍 / 👎.
| return False | ||
| _seen.add(id(obj)) | ||
| for key, value in obj.items(): | ||
| if not isinstance(key, str): |
There was a problem hiding this comment.
When a dictionary key is a StrEnum or another str subclass, this check classifies it as an ordinary string and selects the legacy payload, so hash_objects({Key.X: 1}) still collides with hash_objects({"x": 1}); a cached task that inspects the retained key type can therefore receive the stale result. Even if the scan selected tagging, _encode_for_hashing uses the same isinstance(obj, str) logic and erases the subclass, so the scan and encoder need exact built-in checks plus class-aware handling for subclasses.
Useful? React with 👍 / 👎.
ToniAdreal
left a comment
ToniAdreal
left a comment
There was a problem hiding this comment.
I verified this against the PR head (6c3e3ac) locally: installed the PR
build and ran the reporter's three collision cases from #23817 through the
real hash_objects — tuple vs list, int vs str dict keys, and the nested
case all hash differently now, while equal inputs still hash equally and
dict/set ordering is still ignored. The pickle fallback is untouched
(a non-JSON object hashes identically to a direct cloudpickle.dumps of
the same input).
I also checked the two things that changed since the earlier heads. First,
legacy preservation holds: plain str/dict/list/bytes hashes are identical
to main, and an unambiguous pydantic model's hash is byte-identical between
a main build and this PR build (I ran the same probe in both). The list side
of each old collision keeps the main hash and only the ambiguous side moves,
so no persisted hash churn. Second, the model/dataclass case from the Codex
review works: a pydantic model holding a tuple hashes differently from the
same model holding a list, including nested inside a dict, and the same for
dataclasses.
The two follow-ups from the second Codex review are also fixed in this
head, and I checked both locally. A tuple hidden inside a later sibling
model is now distinguished (I ran the new regression case through the real
hash_objects in both list and dict forms — on the previous head the same
probe hashed them identically, so this is a real fix, not a vacuous test).
And sets of same-named enum members from different modules no longer depend
on insertion order, while members from different enum classes still hash
differently from each other.
One CI note, for the record rather than as a blocker: the six Server Tests
jobs fail on this head, but I read the py3.14/sqlite job log and it is 3,473
passed with exactly one failure — a TimeZoneNotFoundError: time zone ID 'US/Pacific' not found in the daylight-saving schedule test, which looks
like a runner tzdata gap rather than anything this PR touches. The same six
jobs failed identically on every head of this PR.
One question: the tagged encoding now only triggers when the ambiguity scan
finds something, so a block schema or other persisted hash containing an
ambiguous input (say a tuple inside a schema default) would still move on
upgrade. The three pinned checksums from the earlier head are gone from the
diff, which matches the legacy-preservation story — but did you check whether
any persisted hash in the repo covers an ambiguous input? I grepped the test
tree and only found the stable_hash pins in test_hashing.py, which this
does not touch — a second pair of eyes on that would be good before merge.
The end-to-end task test I did not run locally (needs the full engine
harness); CI is green on it so far.
Assisted-by: Muse Spark:1.3
MateehUllah
left a comment
MateehUllah
left a comment
There was a problem hiding this comment.
The tagged serializer calls dataclasses.asdict(obj) before hashing dataclass fields. asdict deep-copies non-dataclass leaf objects, which can fail on resources such as locks and may alter identity-sensitive values, even if the legacy JSON serializer or cloudpickle would otherwise handle the original object. Could you traverse dataclasses.fields() directly (without deep-copying arbitrary field values), retaining the desired tuple/list distinction, and test a dataclass holding a non-deepcopyable value?
ToniAdreal
left a comment
ToniAdreal
left a comment
There was a problem hiding this comment.
I checked @MateehUllah's asdict point locally against this head (6c3e3ac) — the mechanism is real, but the blast radius is narrower than it looks:
_encode_for_hashingdoes calldataclasses.asdict(obj)unguarded (hashing.py:105): on a dataclass holding athreading.Lockit raisesTypeError: cannot pickle '_thread.lock' object. The scan'sasdict(line 170) is already wrapped in try/except, so a lock-holding dataclass on its own just takes the legacy path.- It only bites when an ambiguous sibling triggers the tagged path, e.g.
{"t": (1, 2), "d": <dataclass holding a lock>}. Then the encoder raises inside the JSON attempt andhash_objectsfalls back to cloudpickle, which also cannot pickle a lock, so the call returnsNone(or raisesHashErrorwithraise_on_failure=True, with both errors naming the lock). - Control on main: the same three inputs also return
None— the legacy JSON path fails on the lock the same way — so this is not a regression vs main, and the tuple/list fix still holds for every deepcopyable input I checked.
So I read this as hardening rather than a blocker: traversing fields() directly would keep one non-deepcopyable leaf from killing the tagged encoding for its siblings. A regression test with a dataclass holding a lock next to a tuple would pin it.
Assisted-by: Muse Spark:1.3
…ash scan and encoder dataclasses.asdict deep-copies field values, so a dataclass holding a non-deepcopyable value (a lock or a file) raised inside the tagged encoder when a sibling tuple triggered the tagged path, and the scan missed the ambiguity inside such dataclasses. Read fields directly with dataclasses.fields so values are never deep-copied; a non-deepcopyable leaf no longer kills the tagged encoding for its siblings. A lock still cannot be hashed (neither the JSON encoder nor cloudpickle can represent it), matching main. Signed-off-by: Shahzeb Ali <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9f9c9c6de
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return False | ||
| _seen.add(id(obj)) | ||
| return any(_contains_ambiguous_type(item, _seen, _keepalive) for item in obj) | ||
| return False |
There was a problem hiding this comment.
When a cached task receives a Pydantic model with a deque[Any] field whose element is a tuple in one call and a list in another, model_dump(mode="python") leaves the deque intact, but this scan has no deque/serializer-supported-container branch and returns False; the legacy model_dump(mode="json") then normalizes both to arrays, so task_input_hash can still serve the tuple call's cached result for the list call. Fresh evidence after the claimed model fix is Pydantic's typed-deque split: Python mode returns deque([(1, 2)]) while JSON mode returns [[1, 2]], and the current code stops at this return; recurse through and tag these containers too.
Useful? React with 👍 / 👎.
| json.dumps(item, sort_keys=True, default=str), | ||
| to_qualified_name(type(item)), |
There was a problem hiding this comment.
When a set or mapping contains two encoder-supported values of the same class whose str() forms match but serialized data differs (for example, two members of one Enum that overrides __str__ to a constant and __hash__ to collide), both fields of this key are identical. sorted therefore preserves opposite insertion orders, and final JSON encodes the enum values differently, so equal sets or mappings produce different cache keys. Fresh evidence beyond the earlier fixed two-class example is that the single enum class makes the new qualified-class tiebreaker tie as well; use the same final serializer output as a tiebreaker.
Useful? React with 👍 / 👎.
|
Good catch, checked it. There are no pinned hash_objects values in the test tree: the pins in test_hashing.py are stable_hash pins, which this PR does not touch. The only persisted hash through hash_objects is the block schema checksum, and it hashes model_json_schema(), which is JSON-native by construction: pydantic serializes tuple defaults to JSON arrays, so no block schema dict contains a tuple, int key or set, and the ambiguity scan never triggers there. The two pinned checksums in tests/blocks/test_core.py are plain str/int schemas and CI passes them on this head. The other callers produce transient keys (task and flow cache keys, the server memo hash, integration caches), where a move would only invalidate a cache. The server memo hash goes through the pickle fallback for the block registry, which this PR does not touch. |
|
Thanks both. Addressed in the latest push: dataclasses are now read field by field with dataclasses.fields() in both _encode_for_hashing and _contains_ambiguous_type, instead of dataclasses.asdict(), so field values are never deep-copied. Added regression tests with a dataclass holding a non-deepcopyable value (an open file): the tuple/list distinction inside the dataclass is preserved, and a file-holding dataclass next to a tuple now hashes instead of returning None. One boundary: a dataclass holding a threading.Lock still returns None, since neither the JSON encoder nor cloudpickle can represent a lock; I verified the same inputs returned None before this change too, so that is not a regression. All 27 tests in tests/utilities/test_hashing.py pass locally. |
ToniAdreal
left a comment
ToniAdreal
left a comment
There was a problem hiding this comment.
Checked the new head (f9f9c9c) and your two comments against it — this implements the suggestion exactly. Both call sites now read dataclass fields directly (_encode_for_hashing and the scan in _contains_ambiguous_type), with the docstrings updated to say why, and the three new regression tests pin the cases from the discussion. I ran the file locally on this head: all 27 tests in tests/utilities/test_hashing.py pass, and a base-vs-head control run confirms the behavior change — a dataclass holding a file next to a tuple now distinguishes tuple from list (on the base both hashed identically), while the lock-holding dataclass still returns None, same as on main, so no regression there. I also spot-checked the pinned-hash point: the literal pins in test_hashing.py are all stable_hash, and the block schema checksum hashes model_json_schema(), so that side is clear.
One thing worth a look before merge: Pre-commit checks is failing on this PR, and I traced it to the repo's no-double-backticks hook — it flags five lines in hashing.py (:22, :24, :25 from the earlier commits, plus :59 and :150, the two new dataclasses.asdict docstring literals in this commit). Single backticks there should clear it; ruff and codespell are clean on both changed files.
Nothing further from me on the hashing itself; the rest is the maintainer's call on the PR.
Assisted-by: Muse Spark:1.3
desertaxle
left a comment
desertaxle
left a comment
There was a problem hiding this comment.
Can you address the model serializer regression, the deque case, and the remaining sort ties before we merge? The failing pre-commit check also needs to be fixed.
closes #23817
This PR makes the JSON path of
hash_objectstype-preserving, so task inputs that differ only by container or dict-key type no longer hash identically and serve stale cached results.Problem
hash_objectstries JSON first and only falls back to cloudpickle when JSON raises. JSON erases two type distinctions: tuples serialize exactly like lists, and int dict keys are coerced to strings. Two different task inputs therefore hashed identically, and the second call was served the first call’s cached result. Reproduced onmain:hash_objects({"x": (1, 2, 3)}) == hash_objects({"x": [1, 2, 3]}), and a task returningtype(x).__name__withcache_key_fn=task_input_hashreturnedtuple(Cached) for a later[1, 2, 3]call. Affected paths are the opt-in caching paths that hash inputs withhash_objects(task_input_hash, theInputscache policy); a plain@taskwith no cache args is unaffected.Approach
Values are encoded into a type-tagged JSON structure before serializing: tuples hash differently from lists, int dict keys differently from str keys, and set/frozenset distinctly from lists, while equal inputs keep equal hashes and dict/set ordering still does not affect the hash. Values the encoding cannot represent (bytes, datetimes, arbitrary objects) pass through to the existing JSON object encoder (or the cloudpickle fallback) exactly as before. Alternative considered: hashing with cloudpickle first. That would change the hash of every input and give up the JSON path’s cross-process stability, so the minimal type-tagging change was preferred.
Tradeoff: this changes the hash values themselves, invalidating cache keys computed by earlier versions. Cache keys are not a stable API, but it is worth calling out.
The related
JSONSerializertype coercion (int keys to str keys, tuples to lists for opt-inresult_serializer="json"users) is mentioned in the issue as the same root cause but is not changed here, since its docstring documents stdlib-JSON-compatible inputs.Test plan
tests/utilities/test_hashing.py: tuple vs list (incl. nested), int vs str dict keys, dict-key-order insensitivity, set-order insensitivity, equal-inputs equality. The two collision tests fail on unmodifiedmain(identical hashes) and pass with this change.tests/test_tasks.py:task_input_hashtask called with a tuple then a list returnstuplethenlist, both Completed.pytest tests/utilities/test_hashing.py tests/test_cache_policies.py: 78 passed. The 3 neighboringtask_input_hashtests intests/test_tasks.pypass.ruff checkon the touched files: identical findings to unmodifiedmain(76 pre-existing, 0 new).ruff format --checkclean.