New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Early support for @console_rule. #6088

Merged
merged 7 commits into from Jul 17, 2018

Conversation

Projects
None yet
3 participants
@kwlzn
Copy link
Member

kwlzn commented Jul 10, 2018

Closes #6001

@kwlzn kwlzn force-pushed the twitter:kwlzn/moon_landing branch 2 times, most recently from be57d44 to f95c064 Jul 10, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks Kris, this looks great. My main surprise was the changes in GoalRunner... I figured that if the v2 pipeline was running, it would be long-completed by then.

@@ -153,73 +170,75 @@ def _setup_context(self):
context = Context(options=self._options,
run_tracker=self._run_tracker,
target_roots=target_root_instances,
v2_target_roots=target_roots,

This comment has been minimized.

@stuhood

stuhood Jul 10, 2018

Member

The fact that Context enters into the picture at all here is surprising to me. It seems like the context could/should just always be created after all v2 logic has already completed?

This comment has been minimized.

@kwlzn

kwlzn Jul 14, 2018

Member

fixed.

global_options = self._context.options.for_global_scope()
self._validate_workdir(global_options.pants_workdir)

# N.B. For daemon runs, console rules execute pre-fork.

This comment has been minimized.

@stuhood

stuhood Jul 10, 2018

Member

I think that my thinking on this was that @console_rules always ran "first"... in the sense that they were sortof a different phase of the PantsRunner that didn't require the GoalRunner at all. Then the GoalRunner could (optionally) be created after they had run.

This comment has been minimized.

@kwlzn

kwlzn Jul 14, 2018

Member

makes sense. refactored this a bit - let me know what you think now.

# Toggles v1/v2 `Task` vs `@rule` pipelines on/off.
register('--v1', advanced=True, type=bool, default=True,
help='Enables execution of v1 Tasks.')
register('--v2', advanced=True, type=bool, default=False,

This comment has been minimized.

@stuhood

stuhood Jul 10, 2018

Member

It took a little while to grok the split here... I think it makes sense though. Would probably want to explain the heck out of it in the help.

@kwlzn kwlzn force-pushed the twitter:kwlzn/moon_landing branch 3 times, most recently from 9d34d32 to eb579d3 Jul 14, 2018

@kwlzn kwlzn force-pushed the twitter:kwlzn/moon_landing branch from eb579d3 to 859f6ee Jul 14, 2018

kwlzn added some commits Jul 14, 2018

@kwlzn kwlzn requested review from stuhood and jsirois Jul 15, 2018

@jsirois
Copy link
Member

jsirois left a comment

Found 1 bug to fix, the other two comments are just ideas. I re-read the v2 Frontend HLD doc and these two ideas are decidedly off-plan, especially the Console product idea. So these are ignorable wrt landing on the moon, but otherwise perhaps useful longer term.

self._target_roots
)
except Exception:
logger.warn('Encountered unhandled exception {!r} during rule execution!')

This comment has been minimized.

@jsirois

jsirois Jul 15, 2018

Member

Missing format call and arg.

This comment has been minimized.

@kwlzn

kwlzn Jul 16, 2018

Member

fixed!

@staticmethod
def _make_goal_map_from_rules(rules):
goal_map = {}
goal_to_rule = [(rule.goal, rule) for rule in rules if getattr(rule, 'goal', None) is not None]

This comment has been minimized.

@jsirois

jsirois Jul 15, 2018

Member

This getattr gets to the heart of things - it's not about @console_rule's (with the stress on console) - it's about user-facing goal names. One bit of what seems like conflation is @console_rule - which is just a rule made invokable by a user (from the console) - could be called @goal_rule or @exposed_rule or @cli_rule - and Console. You can imagine you'd wan't a CLI-invokable rule that has no (direct) Console interaction.

This comment has been minimized.

@stuhood

stuhood Jul 15, 2018

Member

@foreground_rule is another name that was floated, since it's the "run in the foreground, exclusively" bit that makes it safe to give them console access.

This comment has been minimized.

@kwlzn

kwlzn Jul 16, 2018

Member

sure, could be called any of those - but didn't feel strongly enough about it in any particular way beyond what we'd called them in the HLD.

I think from an end user approchability/grokability perspective, the @console_rule naming feels clearest to me to imply "a rule that runs with exclusive access to the terminal console". e.g. most new authors won't understand off the bat what an "@exposed_rule" is etc. ultimately, this is all a matter of conceptual overview and framing in the docs tho.

if either of you feel more strongly about the naming, happy to discuss on Slack. we can always change the naming later too, if deemed more appropriate to be referred to as e.g. @foreground_rule. but I like @console_rule, for the moment.

This comment has been minimized.

@stuhood

stuhood Jul 16, 2018

Member

I don't feel strongly about it.



def console_rule(goal_name, input_selectors):
output_type = GoalProduct.for_name(goal_name)

This comment has been minimized.

@jsirois

jsirois Jul 15, 2018

Member

If Console were a product type, inverting the moon-landing flow, alot of future goals for console handling might fall out a bit more naturally.

A sketch - here IO is installed as a singleton that is opaque with no methods to take advantage of in user code. In order for user rule code to create a Console (or InteractiveConsole (stdin)) or Logger, they must pass in an IO. Behind the scenes it would probably carry a Clock.

@rule(Console, [Select(IO), Select(BuildFileAddresses)], goal='list')
def fast_list(io, addresses):
  """A fast variant of `./pants list` with a reduced feature set."""
  console = Console(io)
  for address in addresses.dependencies:
    console.print_stdout(address.spec)
  return console

So now a @console_rule is just a @rule that produces a Console and has a user-facing CLI name (goal='list'). The console would have no handle to real io, it would just buffer data. The engine could then validate and aggregate and/or serialize CLI @rules and Console products, actually driving the Console with one real console.

If the native engine were modified slightly to handle nullary Get and rules returning a generator - ie: feed generator rules until StopIteration instead of assuming a single yielded value, this could be:

@rule(Stream(Console), [Select(BuildFileAddresses)], goal='list')
def fast_list(addresses):
  """A fast variant of `./pants list` with a reduced feature set."""
  console = yield Console(Get(IO))
  for address in addresses.dependencies:
    yield console.print_stdout(address.spec)

Where Console.print_stdout returned a Console.

This comment has been minimized.

@stuhood

stuhood Jul 15, 2018

Member

Hm... I think that from my perspective, Select(IO) is approximately what Select(Console) does currently: ie, represents exclusive access to a console.

I'm not sure I see the advantage of the console being an output product when the other guarantees need to be met (exclusive, foreground access). Even in a world where we added back streaming of the console product with Stream(Console), the API would seem to be unnecessarily restrictive given that we know it "owns" the console while running.

This comment has been minimized.

@kwlzn

kwlzn Jul 16, 2018

Member

yeah, during design discussions I had initially proposed the idea of a Console as buffered virtual console in a similar vein to what you're suggesting (except without the Console/IO split - just wholesale copies) - but we'd decided against that in favor of more direct, exclusive access to accommodate a wider set of rule types to start.

I think for now, I agree with Stu that these are approximately equivalent - but I definitely see this iteratively evolving way beyond this current ~throwaway impl.

@stuhood
Copy link
Member

stuhood left a comment

This looks awesome... much cleaner than before.

Thanks Kris!

@kwlzn kwlzn merged commit 5f36f15 into pantsbuild:master Jul 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

stuhood added a commit that referenced this pull request Aug 7, 2018

Add a `--loop` flag, to allow for running continuously (#6270)
### Problem

The ability to run a command continuously as files change has been a long-standing feature request, and something which is implemented in competing tools. It is very useful for low-latency compiler and test feedback.

### Solution

Builds atop @kwlzn 's work in #6088 and adds a `--loop` flag which re-runs all `@console_rule`s whenever input files have changed.

Adds a covering integration test (and refactors the pantsd test harness a bit to do so). 

### Result

```
./pants --enable-pantsd --v2 --no-v1 --loop list ${dir}::
```
... will continuously list the contents of a directory when files under the directory are invalidated.

CMLivingston pushed a commit to CMLivingston/pants that referenced this pull request Aug 27, 2018

Add a `--loop` flag, to allow for running continuously (pantsbuild#6270)
### Problem

The ability to run a command continuously as files change has been a long-standing feature request, and something which is implemented in competing tools. It is very useful for low-latency compiler and test feedback.

### Solution

Builds atop @kwlzn 's work in pantsbuild#6088 and adds a `--loop` flag which re-runs all `@console_rule`s whenever input files have changed.

Adds a covering integration test (and refactors the pantsd test harness a bit to do so). 

### Result

```
./pants --enable-pantsd --v2 --no-v1 --loop list ${dir}::
```
... will continuously list the contents of a directory when files under the directory are invalidated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment