-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Create new threadpool when operating from thread #1487
Conversation
When we call dask.threaded.get from a separate thread we shouldn't use the default threadpool. ThreadPool objects are not safe in this manner. Now, if we call dask.threaded.get from the non-master thread we create a new ThreadPool. This needs a test and some more careful thought. WIP
I have a test for this in the distributed repository. I'll create something here soon as well. |
fa5cc21
to
9936b26
Compare
9936b26
to
73d9076
Compare
pool = pools[thread][num_workers] | ||
else: | ||
pool = ThreadPool(num_workers) | ||
pools[thread][num_workers] = pool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you need a lock here? I'm imagining a race condition where multiple threads try to create thread pools at the same time, e.g., there wasn't an existing pool when the check on line 61 was run, but there is when you overwrite the dictionary key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a lock fairly conservatively around the all pools
management code
Merging soon if no comments |
This seems sane to me now. On Mon, Aug 22, 2016 at 9:58 AM, Matthew Rocklin notifications@github.com
|
This cleans up some issues in the threaded scheduler when operating from other threads.
num_workers=
keyword argumentNow we keep a pool of pools around, keyed by thread and num_workers. These get cleaned up at every call to get.
Reported by @AlbertDeFusco