dask / dask/distributed

Scheduler `TaskState` objects should be unique, not hashed by key

Open
#7,510 5 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
1.7k
Forks
778
Avg merge
2h 50m
Merged PRs (30d)
3

Description

Currently, two `TaskState` objects with the same key will hash and compare as equal. However, there should only ever be one `TaskState` object per logical task. Therefore, if there are two `TaskState` objects with the same key, they must refer to logically different tasks—such as the same key being re-submitted—and should _not_ be equal, nor hash the same. They refer to different things.

Using the key as the hash causes errors like https://github.com/dask/distributed/issues/7504.

This is a similar theme to:
- https://github.com/dask/distributed/issues/7356
- https://github.com/dask/distributed/issues/6392

@crusaderky already fixed the equivalent problem on the worker side in https://github.com/dask/distributed/pull/6593.

Like there, I think **we should simply remove `__hash__` and `__eq__` from `TaskState` on the scheduler**. Then we'll automatically get what we want, where only `TaskState`s with the same `id` are equal:
> User-defined classes have [`__eq__()`](https://docs.python.org/3/reference/datamodel.html#object.__eq__) and [`__hash__()`](https://docs.python.org/3/reference/datamodel.html#object.__hash__) methods by default; with them, all objects compare unequal (except with themselves) and `x.__hash__()` returns an appropriate value such that `x == y` implies both that `x is y` and `hash(x) == hash(y)`.
https://docs.python.org/3/reference/datamodel.html#object.__hash__

See prior discussion in https://github.com/dask/distributed/pull/6593#discussion_r902943408, https://github.com/dask/distributed/pull/6585#discussion_r901672210.

Note that I could see an argument for instead including the recently-added `run_id` https://github.com/dask/distributed/pull/7463 in the hash, in order to disambiguate between reruns of the same key. That would probably also fix things, for now, but I don't see the advantage of it. Since there should never be multiple `TaskState`s per task in the first place, what's the need for a custom `__hash__` or `__eq__` method? The default identity-based method is the simplest and most correct.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.