Track step nesting in StreamEngine.

Track step nesting in StreamEngine.

Currently, step nesting is tracked by the step runner as an integer,
which is passed to the stream engine as the `set_nest_level` post-create
event. This poses two problems, namely:
- The nest level is known to the runner immediately on step creation,
  but is presented to the stream engine as a separate piece of
  information.
- This forces the step runner's concept of nesting (integer) onto the
  stream engine, when all it really cares about is whether or not a step
  is nested.

This patch updates the API to pass the nested state as a boolean: either
a step is nested in the previous step, or it isn't. The stream engine is
responsible for tracking how it represents this to the stream's
consumer. The nested state is also passed at step creation, making
nesting an atomic step propety instead of an out-of-band option.

Along the way, small cleanups.

BUG=chromium:628770
TEST=local
  - Ran unit tests locally.
  - `./recipes.py --package ./infra/config/recipes.cfg run example/nested`

Committed: https://github.com/luci/recipes-py/commit/4b3ff6014bcb018aff9ae2755faf47cc304f47af

[dnj, 2016-08-15 16:45:24]

dnj@chromium.org changed reviewers:
+ martiniss@chromium.org

[dnj, 2016-08-15 16:45:25]

PTAL!

[dnj, 2016-08-15 17:33:53]

https://codereview.chromium.org/2245113002/diff/1/recipe_engine/simulation_te...
File recipe_engine/simulation_test.py (right):

https://codereview.chromium.org/2245113002/diff/1/recipe_engine/simulation_te...
recipe_engine/simulation_test.py:24: class
SimulationAnnotatorStreamEngine(stream.AnnotatorStreamEngine):
Now that the annotator stream engine tracks state, it's better to route the
simulation through the stream engine instead of having it have a separate weaker
version.

https://codereview.chromium.org/2245113002/diff/1/recipe_engine/simulation_te...
recipe_engine/simulation_test.py:49: annotator =
SimulationAnnotatorStreamEngine()
We pass the annotator directly to the stream engine because it needs to use it
to access the SimluationAnnotatorStreamEngine and, through it, the buffers for
each step.

https://codereview.chromium.org/2245113002/diff/1/recipe_engine/step_runner.py
File recipe_engine/step_runner.py (right):

https://codereview.chromium.org/2245113002/diff/1/recipe_engine/step_runner.p...
recipe_engine/step_runner.py:98: nest_parent: if not None, the nesting parent
for this Step.
oops, will cut change this to 'step_nest_level' in next PS.

https://codereview.chromium.org/2245113002/diff/1/recipe_engine/step_runner.p...
recipe_engine/step_runner.py:446: buf =
self._annotator.step_buffer(step_dict['name'])
Big difference here is that we are now storing buffers by step name instead of
immediately here. This is because the stream differentiates between a step's
annotations/data and the overall recipe's annotations/data.

https://codereview.chromium.org/2245113002/diff/1/recipe_engine/stream.py
File recipe_engine/stream.py (left):

https://codereview.chromium.org/2245113002/diff/1/recipe_engine/stream.py#old...
recipe_engine/stream.py:65: def set_nest_level(self, nest_level):
diediediediediediedie

https://codereview.chromium.org/2245113002/diff/1/recipe_engine/stream.py#old...
recipe_engine/stream.py:268: class
AnnotationStepStream(StreamEngine.StepStream):
Because we no longer have a separate simulation AnnotatorStreamEngine, we can
embed this in the AnnotatorStreamEngine class.

https://codereview.chromium.org/2245113002/diff/1/recipe_engine/stream.py
File recipe_engine/stream.py (right):

https://codereview.chromium.org/2245113002/diff/1/recipe_engine/stream.py#new...
recipe_engine/stream.py:108: assert engine_a and engine_b
Useful assertions, not relevant to CL.

https://codereview.chromium.org/2245113002/diff/1/recipe_engine/stream.py#new...
recipe_engine/stream.py:269: self._current_nest_level = 0
The AnnotatorStreamEngine now tracks the nest level independently.

https://codereview.chromium.org/2245113002/diff/1/recipe_engine/stream.py#new...
recipe_engine/stream.py:326: self._engine._notify_step_finished(self)
This is new, see below.

https://codereview.chromium.org/2245113002/diff/1/recipe_engine/stream.py#new...
recipe_engine/stream.py:328: def output_annotation(self, *args):
Ack, am going to rewrite this in terms of 'write_annotation' in next PS.

https://codereview.chromium.org/2245113002/diff/1/recipe_engine/stream.py#new...
recipe_engine/stream.py:332: def write_line(self, line):
(All of this is copy/paste from outer class when it was relocated here).

https://codereview.chromium.org/2245113002/diff/1/recipe_engine/stream.py#new...
recipe_engine/stream.py:415: if step_stream._nested:
(Called when a step is closed. I put this here instead of within the step
because its proximity to "_create_step_stream" creates a cleaner before/after
picture.)

https://codereview.chromium.org/2245113002/diff/1/recipe_modules/generator_sc...
File recipe_modules/generator_script/api.py (left):

https://codereview.chromium.org/2245113002/diff/1/recipe_modules/generator_sc...
recipe_modules/generator_script/api.py:93: step['step_nest_level'] =
int(step.pop('step_nest_level', 0))
(This will be re-added in next PS, was oversight).

https://codereview.chromium.org/2245113002/diff/1/recipes/example/nested.py
File recipes/example/nested.py (right):

https://codereview.chromium.org/2245113002/diff/1/recipes/example/nested.py#n...
recipes/example/nested.py:15: api.step('wait a bit', ['sleep', '1'])
b/c when actually running this, "1" is a much more tolerable delay.

[martiniss, 2016-08-24 01:09:58]

Looks pretty good; is it ready for final review?

I'm not sure what code I should be worried about..

[dnj, 2016-08-24 02:43:30]

Oh sorry yes, this is ready for review.

[martiniss, 2016-09-01 21:03:39]

lgtm

https://codereview.chromium.org/2245113002/diff/60001/recipe_engine/stream.py
File recipe_engine/stream.py (right):

https://codereview.chromium.org/2245113002/diff/60001/recipe_engine/stream.py...
recipe_engine/stream.py:410: # Increase our current nest level and emit a nested
annotation for this
Why do you say "Increase our current nest level"? It looks like we're just
setting it.

https://codereview.chromium.org/2245113002/diff/60001/recipe_modules/step/con...
File recipe_modules/step/config.py (right):

https://codereview.chromium.org/2245113002/diff/60001/recipe_modules/step/con...
recipe_modules/step/config.py:7: from recipe_engine.config import List
Delete this line

[dnj, 2016-09-07 16:28:51]

https://codereview.chromium.org/2245113002/diff/60001/recipe_engine/stream.py
File recipe_engine/stream.py (right):

https://codereview.chromium.org/2245113002/diff/60001/recipe_engine/stream.py...
recipe_engine/stream.py:410: # Increase our current nest level and emit a nested
annotation for this
On 2016/09/01 21:03:39, martiniss wrote:
> Why do you say "Increase our current nest level"? It looks like we're just
> setting it.

Oh sorry that was residual from a previous implementation. I'll fix.

https://codereview.chromium.org/2245113002/diff/60001/recipe_modules/step/con...
File recipe_modules/step/config.py (right):

https://codereview.chromium.org/2245113002/diff/60001/recipe_modules/step/con...
recipe_modules/step/config.py:7: from recipe_engine.config import List
On 2016/09/01 21:03:39, martiniss wrote:
> Delete this line

Done.

[dnj, 2016-09-07 20:07:14]

The CQ bit was checked by dnj@chromium.org

[dnj, 2016-09-07 20:07:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
martiniss@chromium.org
Link to the patchset: https://codereview.chromium.org/2245113002/#ps80001
(title: "Rebase, comments.")

[commit-bot: I haz the power, 2016-09-07 20:07:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 20:33:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 20:33:48]

Try jobs failed on following builders:
  Recipe Roll Downstream Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/311f15daa7d72a10)

[dnj, 2016-09-07 22:45:53]

The CQ bit was checked by dnj@chromium.org

[commit-bot: I haz the power, 2016-09-07 22:46:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 22:58:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 22:58:52]

Try jobs failed on following builders:
  Recipe Roll Downstream Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/311fa60813bb4910)

[dnj, 2016-09-07 23:39:32]

The CQ bit was checked by dnj@chromium.org

[commit-bot: I haz the power, 2016-09-07 23:39:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 23:42:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 23:42:46]

Try jobs failed on following builders:
  Recipes-py Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/311fd895901d8110)

[dnj, 2016-09-08 17:33:36]

The CQ bit was checked by dnj@chromium.org

[commit-bot: I haz the power, 2016-09-08 17:33:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 17:41:24]

Description was changed from

==========
Track step nesting in StreamEngine.

Currently, step nesting is tracked by the step runner as an integer,
which is passed to the stream engine as the `set_nest_level` post-create
event. This poses two problems, namely:
- The nest level is known to the runner immediately on step creation,
  but is presented to the stream engine as a separate piece of
  information.
- This forces the step runner's concept of nesting (integer) onto the
  stream engine, when all it really cares about is whether or not a step
  is nested.

This patch updates the API to pass the nested state as a boolean: either
a step is nested in the previous step, or it isn't. The stream engine is
responsible for tracking how it represents this to the stream's
consumer. The nested state is also passed at step creation, making
nesting an atomic step propety instead of an out-of-band option.

Along the way, small cleanups.

BUG=chromium:628770
TEST=local
  - Ran unit tests locally.
  - `./recipes.py --package ./infra/config/recipes.cfg run example/nested`
==========

to

==========
Track step nesting in StreamEngine.

Currently, step nesting is tracked by the step runner as an integer,
which is passed to the stream engine as the `set_nest_level` post-create
event. This poses two problems, namely:
- The nest level is known to the runner immediately on step creation,
  but is presented to the stream engine as a separate piece of
  information.
- This forces the step runner's concept of nesting (integer) onto the
  stream engine, when all it really cares about is whether or not a step
  is nested.

This patch updates the API to pass the nested state as a boolean: either
a step is nested in the previous step, or it isn't. The stream engine is
responsible for tracking how it represents this to the stream's
consumer. The nested state is also passed at step creation, making
nesting an atomic step propety instead of an out-of-band option.

Along the way, small cleanups.

BUG=chromium:628770
TEST=local
  - Ran unit tests locally.
  - `./recipes.py --package ./infra/config/recipes.cfg run example/nested`

Committed:
https://github.com/luci/recipes-py/commit/4b3ff6014bcb018aff9ae2755faf47cc304...
==========

[commit-bot: I haz the power, 2016-09-08 17:41:25]

Committed patchset #5 (id:80001) as
https://github.com/luci/recipes-py/commit/4b3ff6014bcb018aff9ae2755faf47cc304...

[dnj, 2016-09-08 19:11:52]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2320223002/ by dnj@chromium.org.

The reason for reverting is: Broke a build:
https://uberchromegw.corp.google.com/i/internal.infra.cron/builders/recipe-au....