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

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

Merged
merged 9 commits into from Aug 7, 2018

Conversation

Projects
None yet
3 participants
@stuhood
Copy link
Member

stuhood commented Jul 29, 2018

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_rules 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.

@stuhood stuhood requested review from jsirois , illicitonion , ity and kwlzn Jul 29, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 29, 2018

Individual commits are useful to review independently.

@stuhood stuhood force-pushed the twitter:stuhood/loop branch from 6b66a7a to db4beaa Jul 29, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

This is really nice! :)

def warm_product_graph(self, target_roots):
"""Warm the scheduler's `ProductGraph` with `TransitiveHydratedTargets` products.
This method raises only fatal errors, and does not cosider failed roots: in the v1 codepath,

This comment has been minimized.

@illicitonion

illicitonion Jul 30, 2018

Contributor

What's a failed root?

This comment has been minimized.

@jsirois

jsirois Aug 1, 2018

Member

s/cosider/consider/

@@ -41,7 +42,7 @@ def _synthesize_goal_product(name):
product_type_name = '{}GoalExecution'.format(name.capitalize())
if PY2:
product_type_name = product_type_name.encode('utf-8')
return type(product_type_name, (datatype(['result']),), {})
return type(product_type_name, (datatype([]),), {})

This comment has been minimized.

@illicitonion

illicitonion Jul 30, 2018

Contributor

Is the point of this empty datatype to just have a unique type per goal, for graph constraint solving? Maybe add a comment explaining that console rules aren't allowed output, but need a unique type?

This comment has been minimized.

@illicitonion

illicitonion Aug 6, 2018

Contributor

Still an open question :)

This comment has been minimized.

@stuhood

stuhood Aug 6, 2018

Member

Yes. Will add.

pants_result = handle.join()
self.assert_success(pants_result)
self.assertEquals(
['{}:{}'.format(rel_tmpdir, name) for name in ('one', 'two', 'three')],

This comment has been minimized.

@illicitonion

illicitonion Jul 30, 2018

Contributor

I'm slightly confused by this expectation; I like it, but why isn't the expectation:

one
one
two
one
two
three

? What happened to the previous results?

This comment has been minimized.

@jsirois

jsirois Aug 1, 2018

Member

Each dump is an overwrite of the same BUILD file, so in each loop 1 target is listed. First it's one, then two, then three.

This comment has been minimized.

@stuhood

This comment has been minimized.

@illicitonion

illicitonion Aug 6, 2018

Contributor

I may be being dense here, but what causes the old values to be cleared?

(It's good that they're cleared, probably, but I'm not sure what the mechanism is...)

This comment has been minimized.

@jsirois

jsirois Aug 6, 2018

Member

Since the same BUILD file is overwritten twice with new contents in each overwrite the list task, which requests addresses from the engine will get a single new address each time. In other words the overwrites invalidate (a portion of) the build graph and it's exactly this portion which is used to produce the output of list each of the three times it is run.

This comment has been minimized.

@stuhood

stuhood Aug 6, 2018

Member

One possible sequence of events is:

  1. user creates a BUILD file containing a target named one
  2. user runs pants list, sees one
  3. user renames the target from one to two
  4. user runs pants list, sees two (which they're expecting, because they renamed the target)
  5. etc.

That's all this is, except the loop is running list for you when it sees that files have changed.

Why does the user see "two" at step 4? Because the engine invalidates Nodes in the graph when you change files.

This comment has been minimized.

@illicitonion

illicitonion Aug 7, 2018

Contributor

Aha, for some reason I thought this was appending, not replacing!

while iterations and not self.is_killed:
try:
target_roots = self._prefork_body(session, options)
except session.scheduler_session.execution_error_type as e:

This comment has been minimized.

@illicitonion

illicitonion Jul 30, 2018

Contributor

This feels like a pretty major behaviour change? Can you talk through a little why raising -> not raising is safe?

This comment has been minimized.

@jsirois

jsirois Aug 1, 2018

Member

Isn't this because the whole point of looping is to just display errors but continue to allow for a live edit to fix the error(s)?

This comment has been minimized.

@stuhood

stuhood Aug 1, 2018

Member

Yep, that. And it's not a behaviour change, because this method is only called when you're looping.

print(e, file=sys.stderr)

iterations -= 1
while iterations and not self.is_killed and not self._loop_condition.wait(timeout=1):

This comment has been minimized.

@illicitonion

illicitonion Jul 30, 2018

Contributor

Can this be restructured to not be nested while loops with control flow, for clarity? I suspect the inner while could probably just become an if...

This comment has been minimized.

@stuhood

stuhood Aug 1, 2018

Member

It needs to be a while loop, because wait can timeout.

@@ -125,6 +125,12 @@ def register_bootstrap_options(cls, register):
register('-q', '--quiet', type=bool, recursive=True, daemon=False,
help='Squelches most console output. NOTE: Some tasks default to behaving quietly: '
'inverting this option supports making them noisier than they would be otherwise.')

register('--loop', type=bool, daemon=True,

This comment has been minimized.

@illicitonion

illicitonion Jul 30, 2018

Contributor

Probably making it a hard error to specify this flag if not --v2 --no-v1 :)

This comment has been minimized.

@stuhood

stuhood Aug 1, 2018

Member

Yea, agreed. Will add.

@jsirois

jsirois approved these changes Aug 1, 2018

def warm_product_graph(self, target_roots):
"""Warm the scheduler's `ProductGraph` with `TransitiveHydratedTargets` products.
This method raises only fatal errors, and does not cosider failed roots: in the v1 codepath,

This comment has been minimized.

@jsirois

jsirois Aug 1, 2018

Member

s/cosider/consider/

while iterations and not self.is_killed:
try:
target_roots = self._prefork_body(session, options)
except session.scheduler_session.execution_error_type as e:

This comment has been minimized.

@jsirois

jsirois Aug 1, 2018

Member

Isn't this because the whole point of looping is to just display errors but continue to allow for a live edit to fix the error(s)?

pants_result = handle.join()
self.assert_success(pants_result)
self.assertEquals(
['{}:{}'.format(rel_tmpdir, name) for name in ('one', 'two', 'three')],

This comment has been minimized.

@jsirois

jsirois Aug 1, 2018

Member

Each dump is an overwrite of the same BUILD file, so in each loop 1 target is listed. First it's one, then two, then three.

@stuhood stuhood force-pushed the twitter:stuhood/loop branch 2 times, most recently from 2625834 to 2e4d07e Aug 1, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Aug 3, 2018

@illicitonion : I'll rebase this to land once you've reviewed.

@stuhood stuhood force-pushed the twitter:stuhood/loop branch from 2e4d07e to cc58014 Aug 7, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Aug 7, 2018

Rebased, and documented (and made private) _GoalProduct.

@illicitonion
Copy link
Contributor

illicitonion left a comment

Thanks!

@stuhood stuhood merged commit 008ce42 into pantsbuild:master Aug 7, 2018

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/loop branch Aug 7, 2018

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