its-amann commented on code in PR #76977:
URL: https://github.com/apache/airflow/pull/76977#discussion_r4559914853
##########
task-sdk/src/airflow/sdk/definitions/callback.py:
##########
@@ -132,14 +133,11 @@ def __eq__(self, other):
return self.serialize() == other.serialize()
def __hash__(self):
Review Comment:
`json.dumps` breaks the hash/eq contract here: `{"timeout": 30}` and
`{"timeout": 30.0}` are equal in `__eq__` but dump to different strings, so
they hash differently and both stay in a set (same with `1` vs `True`).
freezing the values recursively keeps them in sync:
```suggestion
def __hash__(self):
# ``kwargs`` may hold nested dicts/lists, so freeze them
recursively. Hashing the frozen
# values (not a JSON dump) keeps values that ``__eq__`` treats as
equal, like 30 and 30.0,
# on the same hash.
def freeze(value):
if isinstance(value, dict):
return frozenset((k, freeze(v)) for k, v in value.items())
if isinstance(value, (list, tuple)):
return tuple(freeze(v) for v in value)
if isinstance(value, (set, frozenset)):
return frozenset(freeze(v) for v in value)
return value
return hash(freeze(self.serialize()))
```
the `import json` at the top goes unused after this. fixed this plus a test
in this commit, you can just cherry pick it instead of redoing it:
https://github.com/its-amann/airflow/commit/3de876e41a0ec2622222be6711c368ee81b17dc8
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
