Skip to content
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

Incremental strategy for running/skipping targets via tag #6947

Closed
wants to merge 9 commits into from

Conversation

wisechengyi
Copy link
Contributor

@wisechengyi wisechengyi commented Dec 17, 2018

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 wisechengyi changed the title [WIP] Force running/skipping targets by tag [WIP] incremental strategy for running/skipping targets via tag Dec 25, 2018
@@ -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))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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 []
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_targets returns empty targets to compensate the removal of Ref 1

@@ -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()}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@wisechengyi 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 wisechengyi requested review from benjyw, Eric-Arellano and jsirois and removed request for Eric-Arellano December 25, 2018 23:06
@wisechengyi
Copy link
Contributor Author

This is ready for review.

Copy link
Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just a couple of small suggestions.

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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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')))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@property
def supports_skipping_target_by_tag(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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

@wisechengyi
Copy link
Contributor Author

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
Copy link
Member

stuhood commented Dec 28, 2018

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

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use a set comprehension with {}.


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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto on set comprehension

@wisechengyi
Copy link
Contributor Author

wisechengyi commented Dec 31, 2018

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

@benjyw
Copy link
Contributor

benjyw commented Feb 19, 2019

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

@benjyw
Copy link
Contributor

benjyw commented Feb 20, 2019

@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
Copy link
Member

stuhood commented Feb 20, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants