issues with api.step.context and auto-guessing things from kwargs

[Paweł Hajdan, Jr.]

Recently I encountered an issue with api.step.context, while making changes to blink_downstream recipe.

I'm concerned that usage of api.step.context may become error-prone because of this, and we don't seem to have a suitable API in the recipe engine to solve this properly.

This is the tricky part (from https://codereview.chromium.org/2171733002):

  # Run all steps in the checkout dir (consistent with chromium_tests).
  with api.step.context(context):
    # TODO(phajdan.jr): remove redundant **context below once we fix things
    # to behave the same without it.
    step_result = api.bot_update.ensure_checkout(force=True, **context)

If I remove the seemingly redundant **context from ensure_checkout call, recipe expectations change: https://codereview.chromium.org/2175683003 .

I traced this to the following piece of code in bot_update recipe module:

      if step_result.json.output['did_run']:
        co_root = step_result.json.output['root']
        cwd = kwargs.get('cwd', self.m.path['slave_build'])
        if 'checkout' not in self.m.path:
          self.m.path['checkout'] = cwd.join(*co_root.split(self.m.path.sep))

Note that with api.step.context, cwd from the context is applied to the step, but is not part of ensure_checkout kwargs. Above snippet does not see it, and applies the slave_build default.

I was looking for a way to expose the real cwd, which turned out to be non-trivial.

First I tried getting step details from step_result (which seems to reference the step as well), but at that point paths are strings and not Path objects, and I don't think we can easily convert them back.

I also considered somehow extracting the code which applies the context (in step recipe module) so that it could be re-used e.g. in bot_update above. That also seems tricky, because it references variables that are not in kwargs (e.g. "name"):

    compositor = recipe_api._STEP_CONTEXT
    name = compositor.get_with_context('name', name)
    if 'cwd' not in kwargs:
      kwargs['cwd'] = compositor.get('cwd')
    kwargs['env'] = compositor.get_with_context('env', kwargs.get('env', {}))
    kwargs['infra_step'] = compositor.get_with_context(
        'infra_step', bool(infra_step))
    kwargs['step_nest_level'] = compositor.get_with_context('nest_level', 0)

This is where we lose original step dict and stringify everything:

    schema = self.make_config()
    schema.set_val(kwargs)
    return self.run_from_dict(self._engine.create_step(schema))

One idea might be to preserve original kwargs so they can still be extracted from step_result.

Another would be not to stringify (as_jsonish in RecipeEngine.create_step) so early, but at the leaf nodes of code where it's necessary, so that original step is still available in step_result.

Any alternative ideas are welcome. In case any of this seems unclear, I hope by providing a specific example/test case above you'd be able to experiment with it.

Paweł

[Daniel Jacques]

You are trying to have the step return its CWD in its step result? I'm not sure the step's working directory is something that we want to expose at all, as it is something of an implementation detail. If you care what the CWD is, you can supply it, in which case you already know the value.

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAATLsPaPFLhsJw-2gjy_PTfLoC8oq_o7s_%2BbAJfMN2J591HvrA%40mail.gmail.com.

[Stephen Martinis]

I think we should just be more explicit about where to checkout code. The first argument of bot_update should be the directory that we want the code to be checked out in. Then, calling code will know where the git repo is checked out, and the bot update code won't have to guess what the root of the checkout is. The code would look like this:

(blink)

api.path['checkout'] = api.path['slave_build'].join('checkout) # or whatever blink want it to be, could be api.chromium_tests.common_checkout_path()

step_result = api.bot_update.ensure_checkout(api.path['checkout'], force=True)

(bot_update)

  def ensure_checkout(self, checkout_path, ...):            ...      try:        self(..., cwd=checkout_path)      finally:        if step_result.json.output['did_run']:
          ... other code

This change should be able to be done as a pure refactor, since we are tracking exactly what gets checked out where. 

Not sure how hard this would be, but I think it'd be better than monkeying around with hidden global variables. I'm not volunteering to do this, though, so if you want to go a different direction, that's up to you. Just my opinion :)

[Robert Iannucci]

Let the record show that I think api.path['checkout'] was actually a really bad idea. If someone wants to rip it out, it would clear up a lot of things.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/f535ec5d-091a-40cc-9df8-f10fd56d258d%40chromium.org.

[Paweł Hajdan, Jr.]

Robbie, could you explain more why you think api.path['checkout'] is a bad idea? I don't have an opinion about it yet, and would like to understand your reasoning better.

I intend this thread to have wider scope than just the bot_update issue. I have already worked around that https://codereview.chromium.org/2171733002 , and I believe I have good understanding how to apply a bot_update specific change.

Instead what I'm looking for is a systemic fix. We expose api.step.context which is a kind of (scoped) hidden variable. It's part of recipe engine's API. In infra we're aiming for good APIs, and I believe this example demonstrates an issue with it.

In other words, if the answer is not to use api.step.context in the first place (not sure if you're suggesting that), I'd say it should be removed.

My opinion is api.step.context is useful, and the real issue is we don't expose a way to read effective values that api.step is going to use in places that currently access kwargs and provide some defaults. Does that make sense?

Once we agree on a high-level approach we can proceed with discussing how a possible fix may look like. I do have some ideas. Looking forward to your thoughts on above first.

Paweł

[Robert Iannucci]

api.path['checkout'] at this point is basically a global variable whose manipulation can be very unclear, especially for new developers (but sometimes even I have a hard time tracking it down too :)). It was intended to simplify code and improve understanding and modularity, but I think it's actually doing the opposite at this point. I would recommend that we change our various checkout methods to take an explicit path to do the checkout in, and then pass that checkout explicitly to methods that consume it.

This would require us to change the way that config Paths work and introduce an explicit RelativePath. Doing so may also simplify some of the convolution around configs anyway. The RelativePath could be .join'd to a Path to produce a new Path. Only Path objects would be resolvable to strings (so we don't start introducing lots of implicit relative paths in our command invocations). I think that would achieve the benefits we realize today w.r.t. absolute paths, but decouple configs from the 'checkout_path' global datum.

As for context, there's currently a weird mix of kwargs+context that causes confusion. I think it could be OK to use EITHER one, but we need to pick one and be consistent about it. The benefit of context is that things like environment variables don't need to be plumbed through every method. The downside is that they're implicitly carried through everything. However the pass-the-**kwargs model basically has that problem already; we're doing extra typing to implicitly pass this data along, and extra typing means extra bugs.

I'm more comfortable with .context than the traditional environmental manipulation methods (e.g. getenv, setenv, chdir), because it's always lexically scoped; it's not possible to accidentally leak a chdir outside of the with statement, for example.

So all that considered, I would be in favor of removing the kwargs from /everything/ and /only/ using context. Yes, it would mean that for a single step you'd need a with statement to set the current working directory, but I feel like MOST uses of this wind up calling 5-6 steps with the same cwd and env arguments anyway.

As for bot_update, removing 'checkout' from path would also simplify that weird code; the caller would explicitly state where they want bot_update to run, and bot_update would just respect the caller's wishes (no implicit guessing or defaults).

This would mean that we'd need to update various APIs that essentially target a given directory to always work correctly with cwd in the context, and then document that as the way (and the ONLY way) of adjusting where those APIs work. We have a lot of things where you can specify e.g. a target directory in way too many ways:

  * cwd

    * via context

    * via kwarg

  * argument

  * config

There are too many choices here; nothing good can come of this :). At the end of the day, I don't really mind WHICH one we pick, but I do think we need to pick ONE, document it, and stick to it EVERYWHERE (including as part of style reviews for non-infra-written modules), until it becomes second nature, and is easy to pick up for new recipe developers (because they see the exact same pattern everywhere).

The 'create_step' thing is silly legacy nonsense; We should really have the engine expose a schemaful class representing a Step instead of turning stuff into a dict when pushing it back down to the engine.

Hopefully there's something useful in all those words :)

There are no sacred cows here, and recipe readability/understandability is a real problem that we should be pushing to improve. +Stephen Martinis started a metabug for all this cleanup effort; it would be worth adding this as a blocker so we can prioritize with other cleanup efforts: crbug.com/633730.

R

[Paweł Hajdan, Jr.]

I'd be fine with removing arguments we care about from StepApi.__call__ (probably everything except name and cmd) and always passing them through context.

What above post doesn't seem to address is a way to read from the context in places other than StepApi.__call__ . Methods for combining the context wouldn't necessarily work, because they override or otherwise modify previous values most of the time. What we sometimes need though is a way to provide a default value, i.e. only set it if it's not been passed.

What do you think about adding something like api.step.get_context (and possibly renaming api.step.context to set_context)?

Having above API would allow to improve existing problematic cases without being blocked on a complete overhaul of how kwargs are passed.

If you have alternative solutions please share. I'm not rushing to make any changes in this area. Above is just my attempt to provide a constructive starting point.

Paweł

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+unsubscribe@chromium.org.

[Robert Iannucci]

Oh! Yeah definitely. Having an explicit API to read the context lgtm. I just don't like having all the conflicting options :P

I think 'with ... context:' actually reads pretty well right now, not sure if the 'set' is necessary. I would definitely not be a fan of a non-lexically scoped context setting method though (unless there is a usecase for that I think it will lead to leaked chdir and environmental variables)

R

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/f535ec5d-091a-40cc-9df8-f10fd56d258d%40chromium.org.