[Android] Support generic parallel execution across devices.

[Android] Support generic parallel execution across devices.

BUG=267773
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272159

[jbudorick, 2014-05-15 22:10:49]

While looking at what Armand added to DeviceUtils, I started considering what
exactly he was trying to do -- run something in parallel across all connected
devices -- and how we could support doing so generically. This patch is an
attempt at providing a clean interface to do just that. It does come at the cost
of some complexity under the hood, though.

I'm not totally sold on this, but I want to get some opinions before I spend too
much more time on it.

[jbudorick, 2014-05-15 22:35:14]

On 2014/05/15 22:10:49, jbudorick wrote:
> While looking at what Armand added to DeviceUtils, I started considering what
> exactly he was trying to do -- run something in parallel across all connected
> devices -- and how we could support doing so generically. This patch is an
> attempt at providing a clean interface to do just that. It does come at the
cost
> of some complexity under the hood, though.
> 
> I'm not totally sold on this, but I want to get some opinions before I spend
too
> much more time on it.

working on switching this to a thread-based implementation after discussing w/
Craig

[jbudorick, 2014-05-16 00:54:50]

On 2014/05/15 22:35:14, jbudorick wrote:
> On 2014/05/15 22:10:49, jbudorick wrote:
> > While looking at what Armand added to DeviceUtils, I started considering
what
> > exactly he was trying to do -- run something in parallel across all
connected
> > devices -- and how we could support doing so generically. This patch is an
> > attempt at providing a clean interface to do just that. It does come at the
> cost
> > of some complexity under the hood, though.
> > 
> > I'm not totally sold on this, but I want to get some opinions before I spend
> too
> > much more time on it.
> 
> working on switching this to a thread-based implementation after discussing w/
> Craig

Now using reraiser_thread, which makes the logic _much_ cleaner.

[frankf, 2014-05-16 17:47:25]

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:45: class _ParallelExecutor(object):
this should be in its own module with tests

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:45: class _ParallelExecutor(object):
Also think about how to organize various modes of execution (See 
test_dispatcher.py for a work queue model).

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:46: """ Allows parallel execution
across a provided set of devices.
"""First line of docstinrg.

Additional comments
"""

Please address this everywhere.

[jbudorick, 2014-05-16 17:52:49]

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:45: class _ParallelExecutor(object):
On 2014/05/16 17:47:26, frankf wrote:
> this should be in its own module with tests

I debated about the separate module thing. Opted not to at the time since it is
currently specific to DeviceUtils, but I'm ok with moving it out. Tests are in
this CL.

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:46: """ Allows parallel execution
across a provided set of devices.
On 2014/05/16 17:47:26, frankf wrote:
> """First line of docstinrg.
> 
> Additional comments
> """
> 
> Please address this everywhere.

In general -- what if there are no additional comments ...?

"""Only line of docstring."""

or

"""Only line of docstring.
"""

?

[frankf, 2014-05-16 18:41:27]

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:46: """ Allows parallel execution
across a provided set of devices.
The former for single line docstring.


On 2014/05/16 17:52:50, jbudorick wrote:
> On 2014/05/16 17:47:26, frankf wrote:
> > """First line of docstinrg.
> > 
> > Additional comments
> > """
> > 
> > Please address this everywhere.
> 
> In general -- what if there are no additional comments ...?
> 
> """Only line of docstring."""
> 
> or
> 
> """Only line of docstring.
> """
> 
> ?

[craigdh, 2014-05-16 21:59:35]

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:45: class _ParallelExecutor(object):
On 2014/05/16 17:52:50, jbudorick wrote:
> On 2014/05/16 17:47:26, frankf wrote:
> > this should be in its own module with tests
> 
> I debated about the separate module thing. Opted not to at the time since it
is
> currently specific to DeviceUtils, but I'm ok with moving it out. Tests are in
> this CL.

This is probably worth sticking in utils/, probably with a module level doc
showing some simple examples so people can figure out how to use it :)

It might also be useful to have a mixin class that just has a 'parallel' object,
instead of counting on objects to follow that convention.

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:64: raise ValueError('No devices
connected.')
I don't think ValueError is the right exception to raise here.

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:84: if all([callable(o) for o in
r._objs]):
prefer generator expressions over list comprehensions if you just want an
iterable

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
File build/android/pylib/device/device_utils_test.py (right):

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils_test.py:26: Note that this has to live
at top-level so it can be pickled.
out of date?

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/util...
File build/android/pylib/utils/reraiser_thread.py (right):

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/util...
build/android/pylib/utils/reraiser_thread.py:149: """Get all results, joining
all threads if necessary.
Inconsistent use of results/return value in these docs. Also I think a more
clear name than "Get" would be good.

[jbudorick, 2014-05-21 16:42:48]

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:45: class _ParallelExecutor(object):
On 2014/05/16 21:59:35, craigdh wrote:
> On 2014/05/16 17:52:50, jbudorick wrote:
> > On 2014/05/16 17:47:26, frankf wrote:
> > > this should be in its own module with tests
> > 
> > I debated about the separate module thing. Opted not to at the time since it
> is
> > currently specific to DeviceUtils, but I'm ok with moving it out. Tests are
in
> > this CL.
> 
> This is probably worth sticking in utils/, probably with a module level doc
> showing some simple examples so people can figure out how to use it :)
> 
> It might also be useful to have a mixin class that just has a 'parallel'
object,
> instead of counting on objects to follow that convention.

Done.

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:64: raise ValueError('No devices
connected.')
On 2014/05/16 21:59:35, craigdh wrote:
> I don't think ValueError is the right exception to raise here.

What would you suggest? IndexError? A separate exception class?

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:84: if all([callable(o) for o in
r._objs]):
On 2014/05/16 21:59:35, craigdh wrote:
> prefer generator expressions over list comprehensions if you just want an
> iterable

Done.

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
File build/android/pylib/device/device_utils_test.py (right):

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils_test.py:26: Note that this has to live
at top-level so it can be pickled.
On 2014/05/16 21:59:35, craigdh wrote:
> out of date?

Done.

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/util...
File build/android/pylib/utils/reraiser_thread.py (right):

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/util...
build/android/pylib/utils/reraiser_thread.py:149: """Get all results, joining
all threads if necessary.
On 2014/05/16 21:59:35, craigdh wrote:
> Inconsistent use of results/return value in these docs. Also I think a more
> clear name than "Get" would be good.

Done.

[craigdh, 2014-05-21 16:50:12]

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:64: raise ValueError('No devices
connected.')
On 2014/05/21 16:42:48, jbudorick wrote:
> On 2014/05/16 21:59:35, craigdh wrote:
> > I don't think ValueError is the right exception to raise here.
> 
> What would you suggest? IndexError? A separate exception class?

Maybe RuntimeError. Or just do an assert. ValueError would only really make
sense if a device was passed in that wasn't actually attached.

[jbudorick, 2014-05-21 19:42:05]

On 2014/05/21 16:50:12, craigdh wrote:
>
https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
> File build/android/pylib/device/device_utils.py (right):
> 
>
https://codereview.chromium.org/290573004/diff/40001/build/android/pylib/devi...
> build/android/pylib/device/device_utils.py:64: raise ValueError('No devices
> connected.')
> On 2014/05/21 16:42:48, jbudorick wrote:
> > On 2014/05/16 21:59:35, craigdh wrote:
> > > I don't think ValueError is the right exception to raise here.
> > 
> > What would you suggest? IndexError? A separate exception class?
> 
> Maybe RuntimeError. Or just do an assert. ValueError would only really make
> sense if a device was passed in that wasn't actually attached.

rebased + switched to assert

[craigdh, 2014-05-21 22:28:09]

lgtm. Nicely implemented, I really like how this makes use of Python's
ducktyping to make the usage elegant.

[frankf, 2014-05-21 22:34:26]

lgtm

https://codereview.chromium.org/290573004/diff/100001/build/android/pylib/dev...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/290573004/diff/100001/build/android/pylib/dev...
build/android/pylib/device/device_utils.py:121: def parallel(devices):
add docstring for this

[jbudorick, 2014-05-21 23:32:18]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-05-21 23:34:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jbudorick@chromium.org/290573004/130001

[jbudorick, 2014-05-21 23:38:42]

The CQ bit was unchecked by jbudorick@chromium.org

[jbudorick, 2014-05-22 00:07:17]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-05-22 00:12:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jbudorick@chromium.org/290573004/150001

[jbudorick, 2014-05-22 00:42:11]

The CQ bit was unchecked by jbudorick@chromium.org

[jbudorick, 2014-05-22 00:50:35]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-05-22 00:53:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jbudorick@chromium.org/290573004/170001

[commit-bot: I haz the power, 2014-05-22 11:13:45]

Change committed as 272159