Harden vs. TaskState collisions #6593
Conversation
crusaderky
added
the
stability
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 15 files + 15 15 suites +15 10h 20m 22s ⏱️ + 10h 20m 22s For more details on these failures, see this check. Results for commit 6294428. ± Comparison against base commit 3551d15. ♻️ This comment has been updated with latest results. |
|
This PR accidentally relates with #6585, which is dealing with a very similar design issue on the scheduler side:
In both cases, there should never be a case of duplicate objects in the state sets. |
There was a problem hiding this comment.
We should not merge this until the conversation in #6585 (comment) is settled.
distributed/worker_state_machine.py
Outdated
| def __hash__(self) -> int: | ||
| return hash(self.key) | ||
| # See note in __eq__ | ||
| return id(self) |
There was a problem hiding this comment.
Is this change required to get the validation passing?
There was a problem hiding this comment.
Strictly speaking, both __eq__ and __hash__ are unnecessary to get the behavior we want:
User-defined classes have
__eq__()and__hash__()methods by default; with them, all objects compare unequal (except with themselves) andx.__hash__()returns an appropriate value such thatx == yimplies both thatx is yandhash(x) == hash(y).
https://docs.python.org/3/reference/datamodel.html#object.__hash__
I'd kinda prefer to not implement them at all and just use the defaults. We can keep a comment explaining this, but having the explicit methods feels redundant.
There was a problem hiding this comment.
You must implement __hash__ otherwise @dataclass will make it unhashable.
I removed __eq__ and overhauled the PR.
crusaderky
force-pushed
the
WSMR/task_hash
branch
6 times, most recently
from
49ffe66 to
6f9acb8
Compare
crusaderky
force-pushed
the
WSMR/task_hash
branch
from
6f9acb8 to
bcbf716
Compare
#6525 removed the assertion that multiple TaskState objects can't exist at the same time, because it was incompatible with the new instances weakset.
This PR reintroduces the check, but limited to the WorkerState and more methodical. It also makes sure that validate_state will fail if the previous incarnation of a task remained in the WorkerState for any reason, whereas before it would only happen in case of key hash collision.