[PR] fix: propagate UNHASHABLE through nested containers in caching fingerprint [hamilton]

["AmirF194 (via GitHub)" <gi...@apache.org>]

AmirF194 opened a new pull request, #1719:
URL: https://github.com/apache/hamilton/pull/1719

   `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
   


-- 
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: notifications-unsubscribe@hamilton.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix: propagate UNHASHABLE through nested containers in caching fingerprint [hamilton]

["AmirF194 (via GitHub)" <gi...@apache.org>]

AmirF194 commented on PR #1719:
URL: https://github.com/apache/hamilton/pull/1719#issuecomment-5788361757

   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 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: notifications-unsubscribe@hamilton.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix: propagate UNHASHABLE through nested containers in caching fingerprint [hamilton]

["skrawcz (via GitHub)" <gi...@apache.org>]

skrawcz commented on PR #1719:
URL: https://github.com/apache/hamilton/pull/1719#issuecomment-5787910113

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


-- 
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: notifications-unsubscribe@hamilton.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix: propagate UNHASHABLE through nested containers in caching fingerprint [hamilton]

["skrawcz (via GitHub)" <gi...@apache.org>]

skrawcz merged PR #1719:
URL: https://github.com/apache/hamilton/pull/1719


-- 
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: notifications-unsubscribe@hamilton.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix: propagate UNHASHABLE through nested containers in caching fingerprint [hamilton]

["skrawcz (via GitHub)" <gi...@apache.org>]

skrawcz commented on PR #1719:
URL: https://github.com/apache/hamilton/pull/1719#issuecomment-5840047166

   @AmirF194 seems like rust issue still?


-- 
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: notifications-unsubscribe@hamilton.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org