Skip to content

fix: propagate UNHASHABLE through nested containers in caching fingerprint - #1719

Open
AmirF194 wants to merge 2 commits into
apache:mainfrom
AmirF194:fix/1715-unhashable-sentinel-stable-hash
Open

AmirF194 wants to merge 2 commits into
apache:mainfrom
AmirF194:fix/1715-unhashable-sentinel-stable-hash

Conversation

@AmirF194

Copy link
Copy Markdown

hash_value's depth-limited default case returns the literal string <unhashable> once it can't recurse further, but hash_sequence, hash_mapping, and hash_set were hashing that string like any other value instead of treating it specially. Two objects that differ only below MAX_DEPTH, or that contain a type with no registered fingerprint (torch.Tensor, per the follow-up comment), end up with the same normal-looking hash. The cache adapter's data_version == fingerprinting.UNHASHABLE guard never fires on that hash, so it gets treated as a real, stable fingerprint and the cache serves a stale result instead of re-executing.

Fixed by propagating the sentinel up: if any element, key, or value inside a sequence/mapping/set comes back UNHASHABLE, the container now returns UNHASHABLE too, all the way to the top. That's the only change; the three call sites in adapter.py that already check for the sentinel don't need to move.

Ran your script from the issue against HEAD: weight=1.0 and weight=2.0 both hashed to the same value and both returned the 10.0 prediction before the fix. After it, both return <unhashable>, so caching correctly refuses to reuse the cached result. I didn't independently run the torch.Tensor example from the follow-up comment (no torch in this environment), but it hits the exact same propagation path: any value hash_value can't fingerprint now bubbles up instead of getting silently absorbed into a parent hash.

Added one test with a plain nested-object chain that reproduces the collision (no torch needed), plus a direct test per container type, since sequence/mapping/set each had their own copy of the bug.

Fixes #1715

…print

hash_sequence, hash_mapping, and hash_set hashed the literal "<unhashable>"
sentinel like any other value instead of propagating it, so two objects
differing only below MAX_DEPTH (or containing an unfingerprintable type)
got the same normal-looking hash and the caching adapter's UNHASHABLE
guard never fired.
@skrawcz

skrawcz commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

@AmirF194 mind fixing the pre-commit checks -- you should be able to run uv to run them and fix things.

Running 'uv run prek run --all-files --hook-stage pre-merge-commit'
(per the repo's Static Checks CI step) flags two files under
ui/backend/server/tests/ that were already out of sync with the
pinned ruff formatter on main, unrelated to the fingerprinting
change in this PR. Applying 'ruff format' fixes it; no behavior
change, whitespace/line-wrap only.
@AmirF194

Copy link
Copy Markdown
Author

Thanks for the pointer. Ran uv run prek run --all-files --hook-stage pre-merge-commit and the Static Checks failure was ruff format flagging two files under ui/backend/server/tests/ that were already out of sync with the pinned formatter on main, unrelated to the fingerprinting change here. Pushed the formatted versions (whitespace/line wrap only, no behavior change).

The Unit Tests failures across 3.10 through 3.14 are a separate, pre-existing issue: they are pandas/polars API drift (dtype naming, removed kwargs like read_csv(rechunk=...)) that also fails identically on an unrelated open PR (#1720) and on current main, so I left those alone rather than touch unrelated plugin code. tests/caching/test_fingerprinting.py itself passes 48/48 locally against the new commit.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Caching serves stale results when a node's output differs only below MAX_DEPTH

2 participants