hashable class without deriving from HasUhash by poautran · Pull Request #91 · ewoks-kit/ewokscore

poautran · 2026-02-13T15:17:55Z

In GitLab by @woutdenolf on Mar 10, 2023, 15:31 GMT+1:

The universal hashing is use for output caching. Expand hashing support and allow registering hash functions for third-party types.

a class with __uhash__ becomes universally hashable
remove recursive uhash calls and replace with LIFO stack (depth-first search)
handle circular references when calculating the uhash
make helpers classhashdata and multitype_sorted private
register uhash methods with the entry-point group "ewoks.hashing" for types that do not have a __uhash__ methods and are not natively supported

AI used (Claude) for splitting commits, fixing recursion limit (LIFO solution) and circular references (open_depth) and adding support for more builtin types.

NOTE: this would need bumping the major version again. Perhaps wait together with #490 and #491 for 6.0 release.

Assignees: @woutdenolf

Migrated from GitLab: https://gitlab.esrf.fr/workflow/ewoks/ewokscore/-/merge_requests/180

poautran · 2026-02-13T15:18:00Z

In GitLab by @woutdenolf on Mar 10, 2023, 16:34 GMT+1:

added 1 commit

c693228 - hashable class without deriving from HasUhash

Compare with previous version (link unavailable after force-push)

poautran · 2026-02-13T15:18:03Z

In GitLab by @woutdenolf on Mar 10, 2023, 16:35 GMT+1:

marked this merge request as draft

codecov · 2026-06-05T01:53:51Z

Codecov Report

❌ Patch coverage is 97.66082% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/ewokscore/hashing.py	97.63%	4 Missing ⚠️

📢 Thoughts on this report? Let us know!

loichuder

I can understand that the hashing mechanism can be quite complex depending on the value type and I don't know if I am missing information on how hashing works, but I really hit a wall when reviewing this.

I talked about the tests in an inline comment but also, I really feel like the comments do not help: they are verbose but still ambiguous.

The implementation of _uhash looks sensible but I'd suggest to review the inline comments. Perhaps adding some semantic value by renaming some variables and/or extracting some util functions may be useful as well (e.g numpy arrays, datetime).

loichuder · 2026-09-10T13:08:05Z

+
+    obj = Myclass(1)
+    assert hashing.uhash(obj) == obj.__uhash__()
+    assert hashing.uhash([obj]) != hashing.uhash([obj.__uhash__()])


What is the point of this check?

I'll expand a bit more on this response than on the others as an illustration of the more general respone.

Test that when the top-level value's __uhash__ hands back a UniversalHash, it is returned verbatim:

assert uhash(obj) == obj.__uhash__()

Test in addition that the verbatim returning does not apply to the nesting (list in this case):

assert hashing.uhash([obj]) != hashing.uhash([obj.__uhash__()])

If the verbatim logic is implemented wrong and it would be mangled in with the nesting then this would fail. In the current code you could make it fail like this

diff --git a/src/ewokscore/hashing.py b/src/ewokscore/hashing.py index bcd62d2..2cb91da 100644 --- a/src/ewokscore/hashing.py +++ b/src/ewokscore/hashing.py @@ -157,8 +157,7 @@ def _uhash(value) -> UniversalHash: # Any other return value is the content of the value at_root = False - _hash.update(_classhashdata(type(value))) - _expand(stack, open_depths, value, (nested,)) + stack.append(nested) continue

This shows exactly my point of "covering the holes". A human would test only 1). The AI understands that the verbatim logic itself should not enter other branches of the code, not now and not in the future when the code gets changed.

This also illustrates my dilemma: on the one hard I don't want to delete this hardened tests, on the other hand good luck reading it all during a review (also when creating the PR, I probably spend much more time on the tests than the actual code) or when you add new features and you need to know where you new test needs to fit in and how the tests of the other branches are affected.

loichuder · 2026-09-10T13:20:23Z

+    assert a == a.uhash
+    assert a != c.uhash


Should it not check a == b.uhash here ?

Because it's a duplicate of a == a.uhash, not an independent property.

class HasUhash(ABC): def __eq__(self, other): return uhash(self) == uhash(other)

loichuder · 2026-09-10T13:42:05Z

+
+
+def test_hashing_uncomparable_keys():
+    """Mapping keys and set items need no total order between them."""


What does "need no total order" mean?

These keys have no total order:

sorted([(1, 2), (1, "b")]) TypeError: '<' not supported between instances of 'str' and 'int'

I replace "total order" by "comparable". It is for sorting purposes, which "total order" would include, but "comparable" is more intuitive.

loichuder · 2026-09-10T13:46:09Z

+    """Mapping keys and set items need no total order between them."""
+    adict = {(1, 2): "a", (1, "b"): "c"}
+
+    assert hashing.uhash(adict) == hashing.uhash(dict(reversed(adict.items())))


Well, last comment for this file. I think that the tests are too complex.

For example, I had to run this in the Python console to see the output to understand what the test was trying to achieve.

I'd suggest to review the tests and simplify them by:

more obvious generation of the tested data that leaves room for second guessing.

document the aim of the tests more clearly through the test name

breaking down longer tests. That should help with the point above.

loichuder · 2026-09-10T14:02:10Z

+# Hashing `[1, [2]]` pops values from the stack until it is empty. Expanding a
+# value replaces it by the marker `)` that closes it, followed by its items in
+# reverse, so that the items are popped in order and the marker last:
+#
+#   stack (top last)  popped value  hashed
+#   ----------------  ------------  --------------------
+#   [1, [2]]          [1, [2]]      "builtins.list"
+#   ), [2], 1         1             "builtins.int", "0x1"
+#   ), [2]            [2]           "builtins.list"
+#   ), ), 2           2             "builtins.int", "0x2"
+#   ), )              )             ")"
+#   )                 )             ")"


I thought I understood _expand until I read that.

What is popped values? What is (top last) ?

Is the table showing different examples or subsequent steps?

I first wanted to add this example to the PR but then decided to put it in the code (since it is rather complex). At least the LIFO concept with container expansion is covered by that example. Not the more intricate corner cases like handling circular references (e.g. for this _EndOfNesting holds references for the id's in open_depths are valid).

This is not a single _expand call, this shows how [1, [2]] is hashed.

I should either clarify that or move it somewhere else. The example is mainly there to illustrate the LIFO with _expand logic.

popped values: popped from the stack in each iteration of the _uhash() function. I see now that putting it with _expand() really set you on the wrong track.

(top last) should be removed, it is wrong. Left-over from a previous approach that was abandoned.

Done and comment moved.

loichuder · 2026-09-10T14:02:34Z

+#   ), )              )             ")"
+#   )                 )             ")"
+def _expand(
+    stack: List[Any], open_depths: Dict[int, int], value: Any, nested: Iterable


Suggested change

stack: List[Any], open_depths: Dict[int, int], value: Any, nested: Iterable

stack: List[Any], open_depths: Dict[int, int], value: Any, value_items: Iterable

I went for parent and parent_items. I also reviewed all usage of "nested" and replaced it.

loichuder · 2026-09-10T14:04:28Z

+        elif isinstance(value, UniversalHash):
+            _hash.update(repr(value).encode())
+        elif isinstance(value, enum.Enum):
+            # The name identifies the member, except for a combination of flags


What does "except for a combination of flags" mean? What to do in this case?

Flag or IntFlag which can be bitwise combined.

This is the reason we need name and value

import enum class Perm(enum.Flag): R = 1 W = 2 X = 4 combos = [Perm.R | Perm.W, Perm.R | Perm.X, Perm.W | Perm.X] print([c.name for c in combos]) # python 3.12: ['R|W', 'R|X', 'W|X'] # python <3.12: [None, None, None]

Otherwise we could take only the name (as one would expect for an enum).

This is also something I learned (name only would be enough, hence only the flags are mentioned)

class Color(enum.Enum): RED = 1 ALIAS = 1 # not a new member assert Color.ALIAS is Color.RED assert Color.ALIAS.name == "RED"

woutdenolf · 2026-09-11T20:07:17Z

Yes the tests are a problem.

Doing this by hand makes it more readable but leaves lots of untested holes. Letting AI cover those holes leads to unreadable tests.

A pattern that appears not just in this PR. Up to now I was thinking to better fill the holes but now I'm on the fence. Or I should find a better way in guiding the AI in the "filling the holes". The problem is that for complex code there are so many possible permutations of branches that if you fill all holes it might always be a struggle to read through the tests.

…t of an entire hashing

Co-authored-by: Loïc Huder <42204205+loichuder@users.noreply.github.com>

poautran added the gitlab merge request label Feb 13, 2026

poautran assigned woutdenolf Feb 13, 2026

poautran force-pushed the main branch 2 times, most recently from 8dcd50a to b58fb90 Compare February 13, 2026 15:30

woutdenolf mentioned this pull request Jun 5, 2026

Feature: universal hashing of any object #465

Open

woutdenolf force-pushed the uhash_for_custom_class branch from c693228 to e15bcf0 Compare June 5, 2026 01:52

woutdenolf marked this pull request as draft June 5, 2026 01:52

woutdenolf changed the title ~~Draft: hashable class without deriving from HasUhash~~ hashable class without deriving from HasUhash Jun 5, 2026

woutdenolf force-pushed the uhash_for_custom_class branch 10 times, most recently from 4c2f270 to 6bfdb46 Compare September 8, 2026 17:46

woutdenolf added 8 commits September 8, 2026 20:02

Split the universal hashing tests over modules

be87339

Universally hash any type that implements __uhash__

81fb500

Fix the universal hash of values containing bytes or str

195b9f8

Universally hash mappings and sets with uncomparable keys

d6f11b5

Do not universally hash iterators

0c20984

Fix the universal hash of numpy arrays

ecbab8c

Universally hash more builtin types

048315f

Register universal hash methods for types from other libraries

bf69894

woutdenolf force-pushed the uhash_for_custom_class branch from 6bfdb46 to bf69894 Compare September 8, 2026 18:08

woutdenolf marked this pull request as ready for review September 8, 2026 18:10

woutdenolf requested a review from a team September 8, 2026 18:11

woutdenolf added this to the v6.0 milestone Sep 8, 2026

loichuder reviewed Sep 10, 2026

View reviewed changes

woutdenolf added 7 commits September 11, 2026 23:36

make fixture for mocking uhash entry points

4e8ab82

improve comments related to _uhash() of nonces

5bbaed7

improve comment

04378f4

other variant for insertion order different from sorted order

24dd8dc

better wording

df6d4f8

move example it _uhash(), it demonstrates _expand() but in the contex…

54df839

…t of an entire hashing

rename 'nested' appropriately in each situation

1182ffa

woutdenolf force-pushed the uhash_for_custom_class branch from 714f95d to 1182ffa Compare September 11, 2026 22:46

woutdenolf and others added 2 commits September 12, 2026 00:57

canonical class name

9736f1a

Apply batched suggestions from code review

ab74d75

Co-authored-by: Loïc Huder <42204205+loichuder@users.noreply.github.com>

woutdenolf removed the gitlab merge request label Oct 6, 2026

woutdenolf mentioned this pull request Oct 6, 2026

Rename universal hashing to deterministic hashing #49

Open



		def test_hashing_uncomparable_keys():
		"""Mapping keys and set items need no total order between them."""

	stack: List[Any], open_depths: Dict[int, int], value: Any, nested: Iterable
	stack: List[Any], open_depths: Dict[int, int], value: Any, value_items: Iterable

Conversation

poautran commented Feb 13, 2026 • edited by woutdenolf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poautran commented Feb 13, 2026

Uh oh!

poautran commented Feb 13, 2026

Uh oh!

codecov Bot commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

loichuder left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woutdenolf Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woutdenolf Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woutdenolf Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woutdenolf Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woutdenolf Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woutdenolf commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

poautran commented Feb 13, 2026 •

edited by woutdenolf

Loading

codecov Bot commented Jun 5, 2026 •

edited

Loading

loichuder left a comment •

edited

Loading

woutdenolf Sep 11, 2026 •

edited

Loading

woutdenolf Sep 11, 2026 •

edited

Loading

woutdenolf Sep 11, 2026 •

edited

Loading

woutdenolf Sep 11, 2026 •

edited

Loading

woutdenolf Sep 11, 2026 •

edited

Loading

woutdenolf commented Sep 11, 2026 •

edited

Loading