-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Introduce AbstractWorkerPool, CachingPool #16808
Conversation
ClusterManager, | ||
default_worker_pool, | ||
define, |
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.
This is a pretty generic name to start exporting for this. Is it more like a remoteset!
or remotedefine
maybe?
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.
It is generic, but that is the beauty of multiple dispatch, no?
(That said it still might be too generic, I'm not sure either way)
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.
if we can't say "this is what this verb should mean" then maybe it's not a name we should be exporting as a generic function yet
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.
Agree with @tkelman ; if we have this function it should be called something like remoteset!
. Modifying global variables, especially in other processes, is not the kind of thing that gets a nice name.
I don't think global variables can be the way to do broadcasting. It has significant problems for composability and performance. |
I suspect people end up doing this anyways since we don't have an easy way to achieve this. Storing variable data in remote references and using Declaring them as Wrapping them in getter functions is slightly better - the functions could be named |
It's well accepted that one shouldn't use global variables for everything. We need to provide a good alternative so that people don't have to do it. Not being able to combine two functions because they both have a variable called |
I understand. Maybe we can leave it unexported for now? While an inefficient practice there are enough requests on the forums for just this, especially for smaller and simpler programs. And people end up defining via |
0a3cfcf
to
cc3bf6a
Compare
The PR has been updated with an alternate implementation for
|
cc3bf6a
to
8a2cbcb
Compare
Added docs. I propose we either add both |
# An AbstractWorkerPool should implement | ||
# | ||
# `push!` - add a new worker into the pool | ||
# `put!` - put back a worker to the 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.
what's the difference between push!
and put!
here?
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.
My penchant for reusing verbs. take!
and put!
remove and add back workers from the list of workers available for remote execution - this is maintained in pool.channel
.
push!
adds a new worker to the pool itself. Maintained in pool.workers
Have addressed feedback comments. Have also changed
|
|
58a0767
to
6094c6c
Compare
Changed to Will leave it open for a couple more days. Would like some feedback on the interface and how it can be improved. I believe Jeff's reservations have been partially addressed with defining variables as consts and in their own namespaces by default. In case of any strong reservations I am open to have only |
6094c6c
to
f2ba5ee
Compare
f2ba5ee
to
96633b7
Compare
Have removed the hack to broadcast globals. Large globals can be captured in closures by wrapping them with a For global foo of 10^8 floats with a caching pool:
Without
For very small closures, the regular workerpool is faster by around 10-20%. The crossover is around 8MB (10^6 floats) when they are equal. Closes #10438 |
|
||
|
||
""" | ||
remotecall_wait(f, pool::WorkerPool, args...; kwargs...) | ||
remotecall_wait(f, pool::AbstractWorkerPool, args...; kwargs...) |
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.
this worker-pool signature of remotecall_wait
doesn't look like it's present in the rst docs
Merging in a short while. |
…ool (#33892) Once upon a time, there was a young julia user first getting started with parallelism. And she found it fearsomely slow. And so she did investigate, and she did illuminate upon her issue. Her closures, they were being reserialized again and again. And so this young woman, she openned an issue #16345. Lo and behold, a noble soul did come and resolve it, by making the glorious `CachingPool()` in #16808. 3 long years a later this julia user did bravely return to the world of parallism, with many battle worn scars. and once more she did face the demon that is `pmap` over closures. But to her folly, she felt no fear, for she believed the demon to be crippled and chained by the glorious `CachingPool`. Fearlessly, she threw his closure over 2GB of data into the maw of the demon `pmap`. But alas, alas indeed, she was wrong. The demon remained unbound, and it slew her, and slew her again. 100 times did it slay her for 101 items was the user iterating upon. For the glorious chains of the the `CachingPool()` remains unused, left aside in the users tool chest, forgotten.
…ool (JuliaLang/julia#33892) Once upon a time, there was a young julia user first getting started with parallelism. And she found it fearsomely slow. And so she did investigate, and she did illuminate upon her issue. Her closures, they were being reserialized again and again. And so this young woman, she openned an issue JuliaLang/julia#16345. Lo and behold, a noble soul did come and resolve it, by making the glorious `CachingPool()` in JuliaLang/julia#16808. 3 long years a later this julia user did bravely return to the world of parallism, with many battle worn scars. and once more she did face the demon that is `pmap` over closures. But to her folly, she felt no fear, for she believed the demon to be crippled and chained by the glorious `CachingPool`. Fearlessly, she threw his closure over 2GB of data into the maw of the demon `pmap`. But alas, alas indeed, she was wrong. The demon remained unbound, and it slew her, and slew her again. 100 times did it slay her for 101 items was the user iterating upon. For the glorious chains of the the `CachingPool()` remains unused, left aside in the users tool chest, forgotten.
This PR does the following:
AbstractWorkerPool
.WorkerPool
is an implementation ofAbstractWorkerPool
CachingPool <: AbstractWorkerPool
provides two functionalitiescaches functions executed via
remote
,remotecall_fetch(f, cp::CachingPool.....)
and others.This helps in efficiently executing closures with large closed data multiple times on remote workers.
implements a
define(wp::CachingPool; kwargs...)
method which broadcasts all the arguments specified onto workers in the pool. These are bound underMain
with the same name as the keyword arg.empty!(wp::CachingPool)
releases the closure cache as well as set all previously bound names tonothing
. We cannot "unbind" a name in Julia yet.Addresses #10438, #16345 and other issues on easily broadcasting variables.
Example of large closure:
Example of broadcast (from the test file):
Doc updates are pending.
Edit: Means of broadcasting globals across workers have been removed