addprocs does not throw on unused keyword arguments #9958 Closed simonster opened this Issue on Jan 29, 2015 · 12 comments Projects None yet Labels parallel Milestone No milestone Assignees No one assigned 3 participants @simonster @amitmurthy @nalimilan Notifications You’re not receiving notifications from this thread. @simonster The Julia Language member simonster commented on Jan 29, 2015 julia> addprocs(1, foo=:bar, bar=:foo) 1-element Array{Any,1}: 2 Given the recent API changes, it would be good if previous forms of addprocs didn't work, instead of appearing to work but silently launching local workers. @simonster simonster added the parallel label on Jan 29, 2015 @amitmurthy The Julia Language member amitmurthy commented on Jan 30, 2015 This is by design. params in launch(manager::ClusterManager, params::Dict, launched::Array, c::Condition) has all the keyword arguments passed to addprocs. Custom cluster managers can use them. The current canonical form of addprocs is addprocs(manager::ClusterManager; kwargs...) The previous forms of addprocs that work are addprocs(np::Int) and addprocs(machines::Vector) which default to using LocalManager and SSHManager respectively. What doesn't work is addprocs(np; manager=) @simonster The Julia Language member simonster commented on Jan 30, 2015 I don't like the idea of silently ignoring unused keyword arguments. It seems important that the user get an error if they use the manager= syntax, make a typo, or pass a keyword argument that a given ClusterManager can't understand. Maybe we can pass the keyword arguments to addprocs as keyword arguments to launch, so that launch will throw an error if it gets a keyword argument that it wasn't expecting? @amitmurthy The Julia Language member amitmurthy commented on Jan 30, 2015 Hmmm, there are quite a few of them and defining them in launch methods doesn't seem like a good idea - for one, the defaults will have to repeated everywhere. At least for the built-in managers, i.e., LocalManager and SSHManager, I'll put in checks to ensure that an error is thrown on unused keyword arguments. @amitmurthy amitmurthy added a commit that closed this issue on Jan 30, 2015 @amitmurthy Inbuilt managers throw on unused arguments. Closes #9958 b78bc8d @amitmurthy amitmurthy closed this in b78bc8d on Jan 30, 2015 @nalimilan nalimilan commented on Jan 30, 2015 Hmmm, there are quite a few of them and defining them in launch methods doesn't seem like a good idea - for one, the defaults will have to repeated everywhere. So if I understand correctly the situation, every manager knows what arguments can be passed to it, some with defaults coming from launch, and some with defaults it defines itself? Thus some keyword arguments are actually not optional. If so, then it's easier to make the various (possibly custom) methods for addprocs be addprocs(manager; arg1=stop("arg1 must be passed"), arg2=stop(...)) instead of ending with addprocs(manager; kwargs...). I'm asking because this looks like a more general pattern which would be very useful in Julia to make programs more reliable. Built-in syntax for mandatory keyword arguments could be supported, by not providing a default value (this was discussed somewhere, I can't find the issue). @amitmurthy The Julia Language member amitmurthy commented on Jan 30, 2015 launch is a method implemented by each cluster manager to launch julia worker processes in their respective cluster environments. addprocs is the userland interface to add workers using a particular manager. All the keyword arguments when using LocalManager and SSHManager are optional, with some keyword arguments common to both. Why do you feel the need for a mandatory keyword argument? Wouldn't that situation be best served with a positional argument itself? @nalimilan nalimilan commented on Jan 30, 2015 Sorry, I think I hadn't understood the relationship between launch and addprocs. The point was that the natural way to get an error when an unsupported keyword argument is passed seems to be having addprocs list all the keyword arguments it expects in its signature, instead of kwargs.... Checking the list of arguments by hand isn't great (though it certainly works). But now I see what you need is simply a way to avoid repeating the defaults for all versions of addprocs. #8643 is about avoiding this kind of situation. But honestly, since there are only three arguments, I don't think it's a big deal. It would certainly be clearer IMHO to have addprocs actually list the keyword argument it supports instead of just kwargs..., especially if in the future the signature is automatically shown in the documentation. @simonster The Julia Language member simonster commented on Jan 30, 2015 I think the issue @nalimilan is referring to is #5111. To continue bikeshedding, it seems like there is some unnecessary indirection in having the same addprocs in Base call launch for the ClusterManager, which then has to push workers onto an array and notify a condition. Is there a reason we can't make ClusterManagers themselves implement addprocs and have addprocs call a function to connect the workers? For local workers, this might look something like: function addprocs(manager::LocalManager, np::Int; dir=pwd(), exename=joinpath(JULIA_HOME,julia_exename()), exeflags=``) worker_ids = Int[] @sync for i in 1:np io, pobj = open(detach(setenv(`$(julia_cmd(exename)) $exeflags --bind-to $(LPROC.bind_addr) --worker`, dir=dir)), "r") @async push!(worker_ids, connect_local_worker(pobj, io)) end worker_ids end @amitmurthy The Julia Language member amitmurthy commented on Jan 30, 2015 There are 3 common arguments for the in-built managers, but I would like custom managers to also have the means of accepting additional arguments. addprocs is implemented in Base while launch, manage, etc will be implemented by the respective cluster manager modules/packages. @amitmurthy The Julia Language member amitmurthy commented on Jan 30, 2015 addprocs is sort of the driver function that sets up the cluster. launch is only expected to launch julia workers processes. addprocs tries to parallelize both the launching, and the connection setup with and among the workers. @nalimilan nalimilan commented on Jan 30, 2015 @simonster Yeah, that was it. @amitmurthy I don't think my suggestion is incompatible at all with custom managers supporting more keyword arguments. Just replace addprocs with launch in my last comment if only the latter is implemented by custom managers. Then start_cluster_workers can just splat params when calling launch. The advantage of that approach is that implementations are encouraged to only list keyword arguments they support in launch, thus getting an error for unsupported ones. They don't need to think about writing a specific function to check the list of passed arguments. @amitmurthy The Julia Language member amitmurthy commented on Jan 30, 2015 That could be done, but then dir, exename and exeflags which could probably be used by custom managers, and have defaults defined in Base, should be positional arguments or mandatory keyword args a la #5111 for launch since any default values for them will be meaningless. @amitmurthy The Julia Language member amitmurthy commented on Jan 30, 2015 Another issue that I foresee with splatting, is that if and when we add any more keyword args (with appropriate defaults) to addprocs, it effectively breaks the interface and packages would need to change their launch methods.