Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure scheduler events do not hold on to TaskState objects #6226

Merged

Conversation

fjetter
Copy link
Member

@fjetter fjetter commented Apr 27, 2022

ws.processing: dict[TaskState, float]

These event logs hold on to TaskState objects which is not great because they can have a bit of a memory footprint due to the run_spec.
I initially thought this would also pose a problem about GC but we're clearing all references if a task is forgotten so this should not hold indirect references to the entire graph.

I know the test is a bit weird but it does capture this problem ¯_(ツ)_/¯

distributed/tests/test_scheduler.py Outdated Show resolved Hide resolved
@mrocklin
Copy link
Member

🤔 should we track instances of TaskState, perhaps with TaskState._instances = WeakSet() and verify the following in a validation function:

len(TaskState._instances) <= 2*sum(len(s.tasks) for s in Scheduler._instances)

This might be a good check for leaks more broadly.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 27, 2022

Unit Test Results

       16 files  ±0         16 suites  ±0   7h 8m 31s ⏱️ - 42m 8s
  2 741 tests  - 1    2 657 ✔️  - 3       80 💤  - 1  4 +3 
21 829 runs  +8  20 777 ✔️ +2  1 048 💤 +3  4 +3 

For more details on these failures, see this check.

Results for commit afac239. ± Comparison against base commit b6a62f8.

♻️ This comment has been updated with latest results.

@mrocklin
Copy link
Member

@fjetter should I merge this in or are you planning to do more work here?

@fjetter
Copy link
Member Author

fjetter commented Apr 28, 2022

I think adding the Task instance tracking would be a nice things to have. I'll have a look and add it if nothing breaks immediately

@fjetter
Copy link
Member Author

fjetter commented Apr 29, 2022

should we track instances of TaskState, perhaps with TaskState._instances = WeakSet()

I tried but will drop this for now. I think this is worth spending some time on but it's a bit messy right now #6246

len(TaskState._instances) <= 2*sum(len(s.tasks) for s in Scheduler._instances)

FWIW I think this should len(TaskState._instances) == sum(len(s.tasks) for s in Scheduler._instances)
since every TaskState per scheduler should be unique and instances should not be shared across schedulers.

@mrocklin mrocklin merged commit a39bd15 into dask:main Apr 29, 2022
@fjetter fjetter deleted the ensure_scheduler_events_dont_keep_taskstate branch April 29, 2022 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants