Make ElasticManager more convenient #136
Conversation
| 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.
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.
Thanks for looking this over. I'm not sure I understand the issue with taking a dependency. Everyone's Julia installation will always include Pkg and load time is tiny. Re stability, I can say at least that Pkg.API.Context().env.project_file seems to be the way Pkg itself looks up the current package to e.g. print when you do Pkg.status().
There was a problem hiding this comment.
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.
as of julia 1.4 there is also Pkg.project().name. for what version will Pkg.API.Context().env.project_file work?
There was a problem hiding this comment.
$ for ver in 1.0 1.1 1.2 1.3 1.4; do docker run --rm julia:$ver julia -e "using Pkg; @show Pkg.API.Context().env.project_file"; done
(((Pkg.API).Context()).env).project_file = "/root/.julia/environments/v1.0/Project.toml"
(((Pkg.API).Context()).env).project_file = "/root/.julia/environments/v1.1/Project.toml"
(Pkg.API.Context()).env.project_file = "/root/.julia/environments/v1.2/Project.toml"
(Pkg.API.Context()).env.project_file = "/root/.julia/environments/v1.3/Project.toml"
(Pkg.API.Context()).env.project_file = "/root/.julia/environments/v1.4/Project.toml"
Everything back to the lowest supported version by this package (1.0) looks good, as does nightly which I just checked by hand.
|
|
||
| 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=()) |
There was a problem hiding this comment.
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.
I just didn't want to balloon the number of keyword arguments for ElasticManager and also ElasticManager(..., printing_kwargs=(same_project=true,)) implies same_project=true applies just to printing but ElasticManager(..., same_project=true) might not. But I'm happy to change it however you see fit.
|
Just a friendly ping for whoever can merge this. I'm happy to make any changes requested. |
|
I can merge, but don't know if I should merge. I got the write bit to help a bit with tests at some point, but don't feel like I have enough authority to actually merge. |
|
yea, me too. would be nice to have other elasticmanager users chime in with comments. and a test certainly would be nice. could you possibly add one that simply runs the command this PR now prints out to make sure a julia process is actually launched? e,g., no typos in the command string. |
|
Sounds good, will add a test. |
|
Ok, added a test which actually runs the command and checks a worker connects. Also figured out how to do it on Mac. Ready from my end! |
|
Merge? |
|
Hey guys, could this possibly be merged? I'm around to do any quick fixes should problems arise, but personally has been working great for me, e.g.:
|
|
Since no one else has chimed in, there's a test written, and it seems like actual maintainers of this package don't check in too often, I'm going to go ahead and merge. We can always revert if someone bitterly complains after the fact. That said, I don't know how I feel about tagging a version given that one of the maintainers did actively object to that in #118 |
This adds two things discussed in #128
First, it gives
ElasticManagertheaddr=:autooption on Linux, which looks up the master's IP address viahostname --ip-addressso that you get the private IP and other workers on the LAN can connect to it. This isn't foolproof depending on the network setup, but its 4 for 4 on the HPC clusters I have access to.Second, it adds a line to what is printed when you evaluate an
ElasticManager,So that you can just copy/paste that last line without thinking and get a worker.
Finally, I reworked the readme and removed the part about the deprecated
@scheduledwhich doesn't work anymore, even if you switch to@async.Hopefully this makes setting up an ElasticManager way more streamlined. Let me know if you there's any feedback, I'm happy to make changes.
Closes #128