> On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 278 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line278> > > > > Kill extra space after 'args[0]'.
Done. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 258 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line258> > > > > Why the indentation? And add a period at the end of the sentence please. Done. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 257 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line257> > > > > s/executor/executor. Done. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 225 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line225> > > > > s/mesos/Mesos Done. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 219 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line219> > > > > What about s/TOTAL_TASKS/TOTAL_MPDS Done. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 193 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line193> > > > > s-slots/mpd:s-mpd's Done. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 177 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line177> > > > > No need for the intermediate 'count'. Done - removed 'count' > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 176 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line176> > > > > Since mpderr is unused, how about instead: > > > > mpdtraceout, _ = mpdtraceproc.communicate() > > > > or > > > > mpdtraceout = mpdtraceproc.communicate()[0] Went with mpdtraceout = mpdtraceproc.communicate()[0]. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 146 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line146> > > > > Please use driver.declineOffer. Done. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 145 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line145> > > > > s/Rejecting slot/Declining offer Done. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 142 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line142> > > > > How about: > > > > print "Launching mpd " + tid + " on host " + offer.hostname Changed to: print "Replying to offer: launching mpd %d on host %s" % (tid, offer.hostname) > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 114 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line114> > > > > s/slot/offer Done. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 109 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line109> > > > > s/Rejecting slot/Declining offer > > > > Also, why not do driver.declineOffer right here? Done. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 102 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line102> > > > > s/r/resource Done. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 100 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line100> > > > > Kill this line (or alternatively add the offer.id.value up on line 87). Done, merged with line 87. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 96 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line96> > > > > s/slot/resources Done. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 89 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line89> > > > > s/Rejecting/Declining Done. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 69 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line69> > > > > No longer use, kill please. Done. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 66 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line66> > > > > No longer used, kill please. Done. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 59 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line59> > > > > How about s/tasksLaunched/mpdsLaunched Done. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 55 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line55> > > > > It would be great to give this a real name, e.g., MPIScheduler. Done. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 22 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line22> > > > > No need to take driver as an argument anymore. Deleted parameter. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 18 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line18> > > > > Optional path. Changed. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/nmpiexec.py, line 17 > > <https://reviews.apache.org/r/4768/diff/3/?file=103693#file103693line17> > > > > This default isn't the same as what gets printed out from --help. > > Probably makes sense to kill these here and just put the value down in the > > add_option call (like you do for --num and TOTAL_TASKS). Done - default value set at add_option. > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/README.txt, line 62 > > <https://reviews.apache.org/r/4768/diff/3/?file=103692#file103692line62> > > > > I'd prefer if we just had everyone do 'make', since that should build > > the Python dependencies (including protobuf). Ok. I probably didn't configure properly when installing mine... > On 2012-04-24 21:45:19, Benjamin Hindman wrote: > > frameworks/mpi/README.txt, line 23 > > <https://reviews.apache.org/r/4768/diff/3/?file=103692#file103692line23> > > > > Please kill all whitespace in this review. Done. - Harvey ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4768/#review7179 ----------------------------------------------------------- On 2012-05-02 13:29:50, Harvey Feng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4768/ > ----------------------------------------------------------- > > (Updated 2012-05-02 13:29:50) > > > Review request for mesos, Benjamin Hindman, Charles Reiss, and Jessica. > > > Summary > ------- > > Some updates to point out: > > -nmpiexec.py > -> 'mpdallexit' should terminate all slaves' mpds in the ring. I moved > 'driver.stop()' to statusUpdate() so that it stops when all tasks have been > finished, which occurs when the executor's launched mpd processes have all > exited. > -startmpd.py > -> Didn't remove cleanup(), and added code in shutdown() that manually > kills mpd processes. They might be useful during abnormal (cleanup) and > normal (shutdown) framework/executor termination...I think. cleanup() still > terminates all mpd's in the slave, but shutdown doesn't. > -> killtask() stops the mpd associated with the given tid. > -> Task states update nicely now. They correspond to the state of a task's > associated mpd process. > -Readme > -> Included additional info on how to setup and run MPICH2 1.2 and nmpiexec > on OS X and Ubuntu/Linux > > > This addresses bug MESOS-183. > https://issues.apache.org/jira/browse/MESOS-183 > > > Diffs > ----- > > frameworks/mpi/README.txt cdb4553 > frameworks/mpi/nmpiexec 517bdbc > frameworks/mpi/nmpiexec.py a5db9c0 > frameworks/mpi/startmpd.py 8eeba5e > frameworks/mpi/startmpd.sh 44faa05 > > Diff: https://reviews.apache.org/r/4768/diff > > > Testing > ------- > > > Thanks, > > Harvey > >
