-
-
Notifications
You must be signed in to change notification settings - Fork 74
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
Make ElasticManager more convenient #136
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,14 +12,19 @@ struct ElasticManager <: ClusterManager | |
terminated::Set{Int} # terminated worker ids | ||
topology::Symbol | ||
sockname | ||
printing_kwargs | ||
|
||
function ElasticManager(;addr=IPv4("127.0.0.1"), port=9009, cookie=nothing, topology=:all_to_all) | ||
function ElasticManager(;addr=IPv4("127.0.0.1"), port=9009, cookie=nothing, topology=:all_to_all, printing_kwargs=()) | ||
Distributed.init_multi() | ||
cookie !== nothing && cluster_cookie(cookie) | ||
|
||
if addr == :auto | ||
addr = get_private_ip() | ||
end | ||
|
||
l_sock = listen(addr, port) | ||
|
||
lman = new(Dict{Int, WorkerConfig}(), Channel{TCPSocket}(typemax(Int)), Set{Int}(), topology, getsockname(l_sock)) | ||
lman = new(Dict{Int, WorkerConfig}(), Channel{TCPSocket}(typemax(Int)), Set{Int}(), topology, getsockname(l_sock), printing_kwargs) | ||
|
||
@async begin | ||
while true | ||
|
@@ -113,6 +118,9 @@ function Base.show(io::IO, mgr::ElasticManager) | |
seek(iob, position(iob)-1) | ||
println(iob, "]") | ||
|
||
println(iob, " Worker connect command : ") | ||
print(iob, " ", get_connect_cmd(mgr; mgr.printing_kwargs...)) | ||
|
||
print(io, String(take!(iob))) | ||
end | ||
|
||
|
@@ -125,3 +133,38 @@ function elastic_worker(cookie, addr="127.0.0.1", port=9009; stdout_to_master=tr | |
stdout_to_master && redirect_stdout(c) | ||
start_worker(c, cookie) | ||
end | ||
|
||
|
||
function get_private_ip() | ||
if Sys.islinux() | ||
cmd = `hostname --ip-address` | ||
elseif Sys.isapple() | ||
cmd = `ipconfig getifaddr en0` | ||
else | ||
error("`addr=:auto` is only supported on Linux and Mac") | ||
end | ||
try | ||
return IPv4(first(split(strip(read(cmd, String))))) | ||
catch err | ||
error("""Failed to automatically get host's IP address (output below). Please specify `addr=` explicitly. | ||
\$ $(repr(cmd)) | ||
$err | ||
""") | ||
end | ||
end | ||
|
||
function get_connect_cmd(em::ElasticManager; absolute_exename=true, same_project=true) | ||
|
||
ip = string(em.sockname[1]) | ||
port = convert(Int,em.sockname[2]) | ||
cookie = cluster_cookie() | ||
exename = absolute_exename ? joinpath(Sys.BINDIR, Base.julia_exename()) : "julia" | ||
project = same_project ? ("--project=$(Pkg.API.Context().env.project_file)",) : () | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure this one function is sufficient reason to take a dependency on Pkg. And how sure are we that that implementation is stable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for looking this over. I'm not sure I understand the issue with taking a dependency. Everyone's Julia installation will always include There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The dependency is not so much about loading time, and more about maintenance burden. The reason I ask about stability is because, if it's not exported/part of the stable API, then updates are free to break it without consideration. Given that this package doesn't have great (or any really) tests yet, it may be particularly important that we don't add too much maintenance burden. That said, I'm not the one to approve/reject, just wanted to flag it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as of julia 1.4 there is also There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Everything back to the lowest supported version by this package (1.0) looks good, as does nightly which I just checked by hand. |
||
|
||
join([ | ||
exename, | ||
project..., | ||
"-e 'using ClusterManagers; ClusterManagers.elastic_worker(\"$cookie\",\"$ip\",$port)'" | ||
]," ") | ||
|
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,18 @@ | ||
# only package loading for now | ||
using Test | ||
using ClusterManagers | ||
|
||
TIMEOUT = 10. | ||
|
||
@testset "ElasticManager" begin | ||
|
||
em = ElasticManager(addr=:auto, port=0) | ||
|
||
# launch worker | ||
run(`sh -c $(ClusterManagers.get_connect_cmd(em))`, wait=false) | ||
|
||
# wait at most TIMEOUT seconds for it to connect | ||
@test :ok == timedwait(TIMEOUT) do | ||
length(em.active) == 1 | ||
end | ||
|
||
end |
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.
Seems like there are only two allowed printing_kwargs here, a tuple seems like a weird way to do this.
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 just didn't want to balloon the number of keyword arguments for
ElasticManager
and alsoElasticManager(..., printing_kwargs=(same_project=true,))
impliessame_project=true
applies just to printing butElasticManager(..., same_project=true)
might not. But I'm happy to change it however you see fit.