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

WorkerPool and remote() #15073

Merged
merged 1 commit into from
Mar 9, 2016
Merged

Conversation

samoconnor
Copy link
Contributor

Second step in Simplifying and generalising pmap (as per #14843).

Please limit comments on this PR to technical / implementation detail relating to the code in the PR.

Please make comments relating the interface and the overall pmap refactoring in issue #14843.

See also #15058 asyncmap


Equivalent to WorkerPool(CPU_CORES)
"""
WorkerPool() = WorkerPool(addprocs())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WorkerPool() as an empty pool to which we can add workers is more appropriate? In the use case in #14843 (comment), workers are added to the pool as and when they come online. put!(::WorkerPool, pid) would add to the pool.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

samoconnor added a commit to samoconnor/julia that referenced this pull request Feb 14, 2016
samoconnor added a commit to samoconnor/julia that referenced this pull request Feb 16, 2016
# ...
end


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have had tests hitting upper limits and timing out on CI. Would be better to rewrite these tests with a different strategy so that they do not add 10 seconds to the parallel test run.

samoconnor added a commit to samoconnor/julia that referenced this pull request Mar 8, 2016
JuliaLang#15073 (comment)

Remove sleep() from test/parallel_exec.jl

Use Nullable for _default_worker_pool. Init only when myid() == 1.
@samoconnor
Copy link
Contributor Author

How about
doing away with a _worker_pool. Worker pools are created as and when required with a list of workers. WorkerPool() is equivalent to WorkerPool(workers())
_default_worker_pool exists on pid 1. A worker is added to it here

function handle_msg(msg::JoinCompleteMsg, r_stream, w_stream)

_default_worker_pool on other workers are initialized via the JoinPGRP message
type JoinPGRPMsg <: AbstractMsg

On other workers the channel referred by it is the same one as on pid 1

See latest commit.

Note: I've left WorkerPool() returning an empty WorkerPool.
default_worker_pool() is equivalent to WorkerPool(workers()) except that default_worker_pool() is dynamically updated if new workers are added, whereas WorkerPool(workers()) produces a static pool.

@test count == 2
remote_wait(c3)
@test count == 3
@test isready(pool) == false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be worthwhile to make these and below tests work with any number of workers by re-writing like

testchannels = [RemoteChannel() for i in 1:nworkers()]
testcount = 0
@test isready(pool) == true
for c in testchannels
    @test count == testcount
     remote_wait(c)
     testcount += 1 
end
@test isready(pool) == false

This way if the number of workers changes in the future we do not have to revisit this block of tests. Also more readable.

@amitmurthy
Copy link
Contributor

Also please squash the commits.

samoconnor added a commit to samoconnor/julia that referenced this pull request Mar 8, 2016
Refinements per @amitmurthy comments:
JuliaLang#15073

Update _default_worker_pool via JoinPGRPMsg and JoinCompleteMsg per
JuliaLang#15073 (comment)

Remove sleep() from test/parallel_exec.jl

Use Nullable for _default_worker_pool. Init only when myid() == 1.

rewrite default_worker_pool() test for n workers
@samoconnor
Copy link
Contributor Author

@amitmurthy, I've tweaked the test per your suggestions and done a rebase/squash.



"""
remote(f)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to go into the rst docs if it's going to be exported

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about calling it remotef? it becomes clearer that it returns a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless there is a solid convention that functions that return a function have an f suffix, I'd rather not change it as this point and introduce new conventions on the fly. It has been remote as proposed in #14843 since Jan without objection.

I'd be happy for it to be changed in a later functions-that-return-functions naming cleanup PR.
(There is probably an argument for a functions-that-return-iterators naming convention too...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.rst update done

Refinements per @amitmurthy comments:
JuliaLang#15073

Update _default_worker_pool via JoinPGRPMsg and JoinCompleteMsg per
JuliaLang#15073 (comment)

Use Nullable for _default_worker_pool. Init only when myid() == 1.
amitmurthy added a commit that referenced this pull request Mar 9, 2016
@amitmurthy amitmurthy merged commit ec6f886 into JuliaLang:master Mar 9, 2016
@amitmurthy
Copy link
Contributor

We will also need to

  • export WorkerPool and related methods
  • document the concept and usage in the manual
  • mention remote and WorkerPool in NEWS.md



type WorkerPool
channel::RemoteChannel{Channel{Int}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add a default constructor here to avoid the following build warning :
WARNING: Method definition (::Type{Base.WorkerPool})(Any) in module Base at workerpool.jl:5 overwritten at workerpool.jl:17

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

3 participants