Track worker_state_machine.TaskState instances
#6525
Conversation
| def test_task_state_tracking(): | ||
| x = TaskState("x") | ||
| assert len(TaskState._instances) == 1 | ||
| assert first(TaskState._instances) == x | ||
|
|
||
| del x | ||
| assert len(TaskState._instances) == 0 |
There was a problem hiding this comment.
You should add the clean ctxmanager or just the clean_instances ctxmanager. I don't think this is applied automatically and this test would otherwise be fragile and dependent on test order
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 15 files ± 0 15 suites ±0 6h 26m 53s ⏱️ + 12m 59s For more details on these failures, see this check. Results for commit 748dc48. ± Comparison against base commit a1840bb. ♻️ This comment has been updated with latest results. |
|
The new flake #6530 might be related, not sure. |
| def __post_init__(self): | ||
| TaskState._instances.add(self) |
There was a problem hiding this comment.
This doesn't work on unpickle.
| def __post_init__(self): | |
| TaskState._instances.add(self) | |
| def __new__(cls, *args, **kwargs): | |
| TaskState._instances.add(self) | |
| return object.__new__(cls) |
+ unit test for pickle/unpickle round trip
There was a problem hiding this comment.
We're not pickling TaskState objects anywhere, are we?
There was a problem hiding this comment.
We aren't today, but that's not an excuse to have it future-proofed. Namely it would make a lot of sense to pickle dump our new WorkerState class and everything it contains.
There was a problem hiding this comment.
Well, the "excuse" is scope creep.
Using new as in
def __new__(cls, *args, **kwargs):
inst = object.__new__(cls)
TaskState._instances.add(inst)
return instdoes not work because we're defining the hash function as the hash of the key, i.e. we can only add fully initialized TaskState objects to the weakref.
Apart from this, we actually can (un-)pickle this class but will simply not add the instance to this weakset. For the only purpose of dumping this (like in our cluster dump) this is absolutely sufficient.
At this point and for this functionality I'm not willing to reconsider the hash function
fjetter
force-pushed
the
track-worker-task-state-instances
branch
from
ea0281d to
7be8d35
Compare
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 15 files ± 0 15 suites ±0 6h 35m 33s ⏱️ - 31m 32s Results for commit 1edbb34. ± Comparison against base commit cb88e3b. ♻️ This comment has been updated with latest results. |
fjetter
force-pushed
the
track-worker-task-state-instances
branch
from
7be8d35 to
1edbb34
Compare
Partially addresses #6250
pre-commit run --all-files