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
41 changes: 41 additions & 0 deletions src/python/pants/build_graph/target_filter_subsystem.py
@@ -0,0 +1,41 @@
# 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 out targets matching given options.
codealchemy marked this conversation as resolved.
Show resolved Hide resolved

:API: public
"""

options_scope = 'target-filter'

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

register('--exclude-tags', type=list,
default=[],
codealchemy marked this conversation as resolved.
Show resolved Hide resolved
help='Skip targets with given tag(s).')

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


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

@staticmethod
def apply_tag_blacklist(exclude_tags, targets):
return [t for t in targets if not exclude_tags.intersection(t.tags)]
17 changes: 14 additions & 3 deletions src/python/pants/task/task.py
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
Member

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
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 = [
'3rdparty/python:future',
codealchemy marked this conversation as resolved.
Show resolved Hide resolved
'src/python/pants/build_graph',
'tests/python/pants_test:task_test_base',
codealchemy marked this conversation as resolved.
Show resolved Hide resolved
]
)
@@ -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.apply_tag_blacklist({'skip-me'}, [a, b, c])
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.apply_tag_blacklist({'skip-me', 'tag2'}, [a, b, c])
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.apply_tag_blacklist(set(), [a, b, c])
self.assertEqual([a, b, c], filtered_targets)