Incremental strategy for running/skipping targets via tag #6947

wisechengyi · 2018-12-17T02:47:51Z

Problem and Solution

When folks want to enable any linter/formatter, not all targets can be compliant at the same time, and normally this would be an incremental process.

Therefore this PR introduces a generic mechanism to allow incrementally skip/no-skip target per skippable task via target tags. This requires task to declare:

@property
def supports_skipping_target_by_tag(self):
  return True

Behavior

For example with target X:

X Tag Skip	Task Skip	Whether X is skipped when calling `./pants <task> X`
T	F	T
F	F	F
T	T	T
F	T	F
unspecified	F	F
unspecified	T	T

One may think of the tag as the highest ranked value. When a target's tag is specified, e.g. no-lint.scalafmt.skip, this target will run as long as the context includes it.

What this does not solve

If running a particular task depends on the whole context, say java binary A depends on java lib B, then we mark compile-skip on B via tag, this will inadvertently cause A not to compile. This feature only serves a way to specify whether a target should be skipped, but user is ultimately responsible making the choice.

wisechengyi · 2018-12-25T05:28:10Z

src/python/pants/engine/round_engine.py

@@ -43,8 +43,6 @@ def attempt(self, explain):
        with self._context.new_workunit(name=name, labels=[WorkUnitLabel.TASK], log_config=log_config):
          if explain:
            self._context.log.debug('Skipping execution of {} in explain mode'.format(name))
-          elif task.skip_execution:
-            self._context.log.info('Skipping {}'.format(name))


Ref 1

Need to remove this because if a target says no-...-skip in the tag, we can only find out at the task level by filtering targets.

wisechengyi · 2018-12-25T05:29:25Z

src/python/pants/task/task.py

+    if not force_run_targets and self.skip_execution:
+      self.context.log.info('Skipping by returning empty targets '
+                            'because task level is skipped and there is no forced run targets')
+      return []


get_targets returns empty targets to compensate the removal of Ref 1

wisechengyi · 2018-12-25T05:30:09Z

tests/python/pants_test/task/test_goal_options_mixin_integration.py

@@ -43,21 +43,21 @@ def get_echo(which):
            return None
          else:
            with open(path, 'r') as fp:
-              return [os.path.basename(x.strip()) for x in fp.readlines()]
+              return {os.path.basename(x.strip()) for x in fp.readlines()}


changed these to sets since the target list ordering isn't consistent.

wisechengyi · 2018-12-25T23:06:33Z

This is ready for review.

benjyw

Looks great! Just a couple of small suggestions.

benjyw · 2018-12-26T08:17:41Z

tests/python/pants_test/task/echo_plugin/register.py

-    with open(os.path.join(self.workdir, 'output'), 'w') as fp:
-      fp.write('\n'.join(t.address.spec for t in self.get_targets()))
+    relevant_targets = self.get_targets()
+    if relevant_targets:


Do we not want to write an empty file if relevant_targets is empty? Seems more consistent.

benjyw · 2018-12-26T08:19:19Z

tests/python/pants_test/task/test_goal_options_mixin_integration.py

+              return {os.path.basename(x.strip()) for x in fp.readlines()}
+
+        self.assertEqual(expected_one, get_echo('one'),
+                         'one expected {}, but got {}'.format(expected_one, get_echo('one')))


Why add these messages? assertEqual is supposed to already generate a sensible message.

benjyw · 2018-12-26T08:31:33Z

src/python/pants/task/target_restriction_mixins.py

@@ -55,6 +55,10 @@ class HasSkipOptionMixin(object):
  def skip_execution(self):
    return self.get_options().skip

+  @property
+  def supports_skipping_target_by_tag(self):


I'd call this can_skip_individual_targets(), so that it's not so closely bound to the fact that we happen to use tags to express how we choose which targets to skip.

stuhood

I don't have time today to suggest an alternative, but: this particular model is not going to be portable at all to v2, so I don't think we should land it in this form.

The easiest way to improve portability to v2 would be to incorporate Subsystems in some way... possibly by having Tasks that want to filter their target sets explicitly request a scoped Subsystem that holds the relevant options.

wisechengyi · 2018-12-26T19:25:11Z

@stuhood when you get a chance, could you elaborate why this is not portable to v2?

wisechengyi · 2018-12-27T19:10:12Z

Also depending on the size and the timeline v2 involves, it may also be a possibility to get the feature in first then refactor into something v2 friendly.

stuhood · 2018-12-28T01:43:37Z

Also depending on the size and the timeline v2 involves, it may also be a possibility to get the feature in first then refactor into something v2 friendly.

#6880 is out, and should be reviewable/landable in the first two weeks of 2019. When it lands, ./pants list will be the first Task to be fully ported to v2. We've also discussed following up to port fmt or lint relatively soon afterward, so it's pretty important to consider what this will look like there.

when you get a chance, could you elaborate why this is not portable to v2?

Because Tasks will be relatively quickly phased out in v2 in favor of @rules. Tasks implicitly have a scope and options attached: and this patch uses the Task's scope to decide which tags to pay attention to.

So as an alternative: rather than using the Task's implicit scope, using an explicitly requested Subsystem to get the scope (maybe something like TagFiltering.scoped(cls); ie: an instance of a TagFiltering Subsystem scoped to the Task) would be easier to port to @rules. The plan (and implementation in #6993) is for @rules to be able to easily consume Subsystems.

The whole thing might then look like having a Task that wants to opt into this request the TagFiltering subsystem, and then use the scoped instance of TagFiltering to filter the target list. It's also possible that you could change the underlying implementation that you have on this PR to do this under the hood (ie, have the Task baseclass automatically request the subsystem and apply the filtering).

Eric-Arellano

Added two small suggestions. Thanks for working on this!

(Disclaimer I don’t have laptop so this feedback is from reading over my phone + I didn’t have time to critically think about the correctness of the implementation)

Eric-Arellano · 2018-12-29T14:54:51Z

src/python/pants/task/task.py

+      return relevant_targets
+
+    force_run_tag = 'no-{}.skip'.format(self.options_scope)
+    force_run_targets = set(t for t in relevant_targets if force_run_tag in t.tags)


Can use a set comprehension with {}.

Eric-Arellano · 2018-12-29T14:55:17Z

src/python/pants/task/task.py

+
+    force_skip_tag = '{}.skip'.format(self.options_scope)
+    force_skip_targets = set(t for t in relevant_targets if force_skip_tag in t.tags)
+


Ditto on set comprehension

wisechengyi · 2018-12-31T06:21:36Z

@stuhood thanks for the pointer. I will try to fiddle around the subsystem option.

benjyw · 2019-02-19T23:54:14Z

Note: @codealchemy will be looking at this part as well (it'll be a great way to learn the new engine concepts).

benjyw · 2019-02-20T00:22:56Z

@stuhood in your Subsystem scheme, how does one express "skip task X for target Y"? Using the options scope of the Subsystem? E.g., a tag my.subsystem.scope-skip or something? And how will the @rule know which subsystem to request?

stuhood · 2019-02-20T00:58:14Z

Using the options scope of the Subsystem?

@benjyw : Yep. And @rules would end up consuming explicit subsystems. What the convention for scopes will be in the long term is unknown... but in the short to medium term, an @rule replacing a v1 Task would continue to request the scope that the v1 Task would have gotten.

And the scoped subsystem might be something like skip.lint.go (scoped subsystem is a prefix on a task).

wisechengyi added 4 commits December 24, 2018 18:15

reest

07b2ce0

reset

8dc895c

force skip/run by tag

e402adc

fix tests

1911841

wisechengyi force-pushed the filterbytaglintfmt branch from dc9777e to 1911841 Compare December 25, 2018 02:46

linty stuff

2742ca2

wisechengyi changed the title ~~[WIP] Force running/skipping targets by tag~~ [WIP] incremental strategy for running/skipping targets via tag Dec 25, 2018

wisechengyi added 3 commits December 24, 2018 22:31

add tests

a6e03ba

add one more

57506f2

simplify

3e0f335

wisechengyi commented Dec 25, 2018

View reviewed changes

use set literal

26bf023

wisechengyi changed the title ~~[WIP] incremental strategy for running/skipping targets via tag~~ Incremental strategy for running/skipping targets via tag Dec 25, 2018

wisechengyi requested review from benjyw, Eric-Arellano and jsirois and removed request for Eric-Arellano December 25, 2018 23:06

benjyw approved these changes Dec 26, 2018

View reviewed changes

stuhood requested changes Dec 26, 2018

View reviewed changes

Eric-Arellano reviewed Dec 29, 2018

View reviewed changes

wisechengyi mentioned this pull request Feb 14, 2019

Generic incremental strategy (white/blacklist) for fmt/lint #6902

Closed

codealchemy mentioned this pull request Feb 21, 2019

Add filtering subsystem to permit skipping targets by tags #7275

Merged

wisechengyi closed this Apr 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incremental strategy for running/skipping targets via tag #6947

Incremental strategy for running/skipping targets via tag #6947

wisechengyi commented Dec 17, 2018 •

edited

Loading

wisechengyi Dec 25, 2018

wisechengyi Dec 25, 2018

wisechengyi Dec 25, 2018

wisechengyi commented Dec 25, 2018

benjyw left a comment

benjyw Dec 26, 2018

benjyw Dec 26, 2018

benjyw Dec 26, 2018

stuhood left a comment

wisechengyi commented Dec 26, 2018

wisechengyi commented Dec 27, 2018

stuhood commented Dec 28, 2018

Eric-Arellano left a comment

Eric-Arellano Dec 29, 2018

Eric-Arellano Dec 29, 2018

wisechengyi commented Dec 31, 2018 •

edited

Loading

benjyw commented Feb 19, 2019

benjyw commented Feb 20, 2019

stuhood commented Feb 20, 2019 •

edited

Loading


		force_skip_tag = '{}.skip'.format(self.options_scope)
		force_skip_targets = set(t for t in relevant_targets if force_skip_tag in t.tags)

Incremental strategy for running/skipping targets via tag #6947

Incremental strategy for running/skipping targets via tag #6947

Conversation

wisechengyi commented Dec 17, 2018 • edited Loading

Problem and Solution

Behavior

What this does not solve

Choose a reason for hiding this comment

Ref 1

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wisechengyi commented Dec 25, 2018

benjyw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

wisechengyi commented Dec 26, 2018

wisechengyi commented Dec 27, 2018

stuhood commented Dec 28, 2018

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wisechengyi commented Dec 31, 2018 • edited Loading

benjyw commented Feb 19, 2019

benjyw commented Feb 20, 2019

stuhood commented Feb 20, 2019 • edited Loading

wisechengyi commented Dec 17, 2018 •

edited

Loading

wisechengyi commented Dec 31, 2018 •

edited

Loading

stuhood commented Feb 20, 2019 •

edited

Loading