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    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  cluster 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.