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

Add filtering subsystem to permit skipping targets by tags #7275

Merged
merged 8 commits into from
Feb 23, 2019
44 changes: 44 additions & 0 deletions src/python/pants/build_graph/target_filter_subsystem.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# coding=utf-8
# Copyright 2019 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from __future__ import absolute_import, division, print_function, unicode_literals

import logging

from pants.subsystem.subsystem import Subsystem


logger = logging.getLogger(__name__)


class TargetFilter(Subsystem):
"""Filter targets matching configured criteria.

:API: public
"""

options_scope = 'target-filter'

@classmethod
def register_options(cls, register):
super(TargetFilter, cls).register_options(register)

register('--exclude-tags', type=list,
default=[], fingerprint=True,
help='Skip targets with given tag(s).')

def apply(self, targets):
exclude_tags = set(self.get_options().exclude_tags)
return TargetFiltering(targets, exclude_tags).apply_tag_blacklist()


class TargetFiltering(object):
codealchemy marked this conversation as resolved.
Show resolved Hide resolved
codealchemy marked this conversation as resolved.
Show resolved Hide resolved
"""Apply filtering logic against targets."""

def __init__(self, targets, exclude_tags):
self.targets = targets
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have left targets as the input to apply_tag_blacklist since the subsystem (and this helper) are about the criteria. Configured with this criteria - once, filter these targets, now these and finally those. That said I'll merge and if you want to circle back - great, but also no need - this is style preference land.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 (cross-linking for reference - this is done in #7280)

self.exclude_tags = exclude_tags

def apply_tag_blacklist(self):
return [t for t in self.targets if not self.exclude_tags.intersection(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.

Nice list comprehension! Very Pythonic.

You'll also find this style in our codebase:

list(filter(lambda t: not self.exclude_tags.intersection(t.tags), self.targets))

I personally prefer the list comprehension, but both do the same thing and appear in our codebase.

17 changes: 14 additions & 3 deletions src/python/pants/task/task.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

from pants.base.exceptions import TaskError
from pants.base.worker_pool import Work
from pants.build_graph.target_filter_subsystem import TargetFilter
from pants.cache.artifact_cache import UnreadableArtifact, call_insert, call_use_cached_files
from pants.cache.cache_setup import CacheSetup
from pants.invalidation.build_invalidator import (BuildInvalidator, CacheKeyGenerator,
Expand Down Expand Up @@ -96,7 +97,7 @@ def _compute_stable_name(cls):
@classmethod
def subsystem_dependencies(cls):
return (super(TaskBase, cls).subsystem_dependencies() +
(CacheSetup.scoped(cls), BuildInvalidator.Factory, SourceRootConfig))
(CacheSetup.scoped(cls), TargetFilter.scoped(cls), BuildInvalidator.Factory, SourceRootConfig))
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

This should probably be conditional on whether a Task actually opts-in to Target filtering: as get_targets notes, very few tasks actually do, and for many Tasks, target filtering doesn't make sense. Having this on "by default" like this will make it show up in the help and documentation for a whole bunch of Tasks for which it will have no effect.

As with @wisechengyi's initial patch, I would suggest a classmethod/classproperty of Task that subclasses could override to enable filtering (with filtering disabled by default), and then conditionally requesting the subsystem here if that property was True.

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Good point! Yes, this will be a necessary follow-up.

But does this subsystem-based design address your v2 engine concerns?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

It does, thanks! This is very easy to transition to v2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Addressing this in #7283


@classmethod
def product_types(cls):
Expand Down Expand Up @@ -237,8 +238,18 @@ def get_targets(self, predicate=None):

:API: public
"""
return (self.context.targets(predicate) if self.act_transitively
else list(filter(predicate, self.context.target_roots)))
initial_targets = (self.context.targets(predicate) if self.act_transitively
else list(filter(predicate, self.context.target_roots)))
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI the reason this code has to wrap filter() with list() is that in Python 3 (and with the Future library's backports), filter() returns a lazy generator.


included_targets = TargetFilter.scoped_instance(self).apply(initial_targets)
excluded_targets = set(initial_targets).difference(included_targets)
codealchemy marked this conversation as resolved.
Show resolved Hide resolved

if excluded_targets:
self.context.log.info("{} target(s) excluded".format(len(excluded_targets)))
for target in excluded_targets:
self.context.log.debug("{} excluded".format(target.address.spec))

return included_targets
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're already aware of this, but not many tasks use this method (they use self.context.targets) so there may need to be follow-up:

$ git grep "\bget_targets(" | cut -d: -f1 | sort -u | wc -l
17

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

For context: This method was a relatively recent addition, to support lint/fmt tasks being optionally transitive or skipped entirely. So currently they are the only tasks that consult this method. However those tasks are also the immediate use case for this feature. Any other task we want to use it for will have to be modified to use this method, as you suggest, assuming we think that's worth doing in the v1 engine.


@memoized_property
def workdir(self):
Expand Down
10 changes: 10 additions & 0 deletions tests/python/pants_test/build_graph/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -146,3 +146,13 @@ python_tests(
'tests/python/pants_test:test_base',
]
)

python_tests(
name = 'target_filter_subsystem',
sources = ['test_target_filter_subsystem.py'],
dependencies = [
'src/python/pants/build_graph',
'src/python/pants/task',
'tests/python/pants_test:task_test_base',
codealchemy marked this conversation as resolved.
Show resolved Hide resolved
]
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
# coding=utf-8
# Copyright 2019 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from __future__ import absolute_import, division, print_function, unicode_literals

from pants.build_graph.target_filter_subsystem import TargetFilter, TargetFiltering
from pants.task.task import Task
from pants_test.task_test_base import TaskTestBase


class TestTargetFilter(TaskTestBase):

class DummyTask(Task):
options_scope = 'dummy'

def execute(self):
self.context.products.safe_create_data('task_targets', self.get_targets)

@classmethod
def task_type(cls):
return cls.DummyTask

def test_task_execution_with_filter(self):
a = self.make_target('a', tags=['skip-me'])
b = self.make_target('b', dependencies=[a], tags=[])

context = self.context(for_task_types=[self.DummyTask], for_subsystems=[TargetFilter], target_roots=[b], options={
TargetFilter.options_scope: {
'exclude_tags': ['skip-me']
}
})

self.create_task(context).execute()
self.assertEqual([b], context.products.get_data('task_targets'))

def test_filtering_single_tag(self):
a = self.make_target('a', tags=[])
b = self.make_target('b', tags=['skip-me'])
c = self.make_target('c', tags=['tag1', 'skip-me'])

filtered_targets = TargetFiltering([a, b, c], {'skip-me'}).apply_tag_blacklist()
self.assertEqual([a], filtered_targets)

def test_filtering_multiple_tags(self):
a = self.make_target('a', tags=['tag1', 'skip-me'])
b = self.make_target('b', tags=['tag1', 'tag2', 'skip-me'])
c = self.make_target('c', tags=['tag2'])

filtered_targets = TargetFiltering([a, b, c], {'skip-me', 'tag2'}).apply_tag_blacklist()
self.assertEqual([], filtered_targets)

def test_filtering_no_tags(self):
a = self.make_target('a', tags=['tag1'])
b = self.make_target('b', tags=['tag1', 'tag2'])
c = self.make_target('c', tags=['tag2'])

filtered_targets = TargetFiltering([a, b, c], set()).apply_tag_blacklist()
codealchemy marked this conversation as resolved.
Show resolved Hide resolved
self.assertEqual([a, b, c], filtered_targets)