Skip to content

Commit

Permalink
Fail for deleted-but-depended-on targets in changed (pantsbuild#5636)
Browse files Browse the repository at this point in the history
As described in pantsbuild#382 (mega classic!), it might be possible to more deeply integrate change detection into the v2 engine itself to compute a delta on the graph. But for now we defer a deeper solution and simply ensure that we fail for deleted targets by transitively expanding targets. Adds a test to cover the behaviour.

Due to fully hydrating targets, this represents a linear performance regression from pantsbuild#5579: the runtime of `--changed-include-dependees=transitive` for a small set of roots used to be slightly lower than the runtime for `./pants list ::`, because the operation that occurred "on the entire graph" was computing `HydratedStructs`, rather than computing `TransitiveHydratedTargets`.

The impact for exactly that step is constant, and fairly high:
```
  DEBUG] Root Select(Collection(dependencies=(DescendantAddresses(directory=u''),)), =Collection.of(Struct)) completed.
  DEBUG] computed 1 nodes in 1.709688 seconds. there are 8858 total nodes.

  DEBUG] Root Select(Collection(dependencies=(DescendantAddresses(directory=u''),)), =TransitiveHydratedTargets) completed.
  DEBUG] computed 1 nodes in 2.989497 seconds. there are 15916 total nodes.
```
... but the impact on overall runtime is dependent on the count of targets that are transitively affected, because for all affected targets, we're going to need to compute transitive roots anyway. So for the example change from pantsbuild#5579 which affects 567 targets:
```
time ./pants --changed-diffspec=22ca0604b1c6ce8de019214b821e922aac66b026^..22ca060 --changed-include-dependees=transitive list | wc -l
  real	0m4.877s
  user	0m4.081s
  sys	0m1.068s

  real	0m5.294s
  user	0m4.487s
  sys	0m1.142s
```
For a change impacting only 14 targets the difference is slightly more pronounced:
```
$ time ./pants --changed-diffspec=f35e1e6fb1cdf45fcb5080cfe567bdbae8060125^..f35e1e6 --changed-include-dependees=transitive list | wc -l
  real	0m4.279s
  user	0m3.376s
  sys	0m1.011s

  real	0m4.954s
  user	0m4.284s
  sys	0m1.120s
```
  • Loading branch information
Stu Hood authored and wisechengyi committed Mar 31, 2018
1 parent 399f55e commit 2915e75
Show file tree
Hide file tree
Showing 4 changed files with 370 additions and 24 deletions.
14 changes: 0 additions & 14 deletions src/python/pants/engine/build_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -345,12 +345,6 @@ def _recursive_dirname(f):
yield ''


# TODO: This is a bit of a lie: `Struct` is effectively abstract, so this collection
# will contain subclasses of `Struct` for the symbol table types. These APIs need more
# polish before we make them public: see #4535 in particular.
HydratedStructs = Collection.of(Struct)


BuildFilesCollection = Collection.of(BuildFiles)


Expand All @@ -376,14 +370,6 @@ def create_graph_rules(address_mapper, symbol_table):
hydrate_struct
),
resolve_unhydrated_struct,
TaskRule(
HydratedStructs,
[SelectDependencies(symbol_table_constraint,
BuildFileAddresses,
field_types=(Address,),
field='addresses')],
HydratedStructs
),
# BUILD file parsing.
parse_address_family,
build_files,
Expand Down
17 changes: 9 additions & 8 deletions src/python/pants/scm/change_calculator.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
from pants.base.build_environment import get_scm
from pants.base.specs import DescendantAddresses
from pants.build_graph.address import Address
from pants.engine.build_files import HydratedStructs, Specs
from pants.engine.legacy.graph import target_types_from_symbol_table
from pants.engine.build_files import Specs
from pants.engine.legacy.graph import TransitiveHydratedTargets, target_types_from_symbol_table
from pants.engine.legacy.source_mapper import EngineSourceMapper
from pants.goal.workspace import ScmWorkspace
from pants.util.meta import AbstractClass
Expand Down Expand Up @@ -144,14 +144,15 @@ def iter_changed_target_addresses(self, changed_request):
if changed_request.include_dependees not in ('direct', 'transitive'):
return

# For dependee finding, we need to parse all build files to collect all structs. But we
# don't need to fully hydrate targets (ie, expand their source globs), and so we use
# the `HydratedStructs` product. See #4535 for more info.
# TODO: For dependee finding, we technically only need to parse all build files to collect target
# dependencies. But in order to fully validate the graph and account for the fact that deleted
# targets do not show up as changed roots, we use the `TransitiveHydratedTargets` product.
# see https://github.com/pantsbuild/pants/issues/382
specs = (DescendantAddresses(''),)
adaptor_iter = (t
for targets in self._scheduler.product_request(HydratedStructs,
adaptor_iter = (t.adaptor
for targets in self._scheduler.product_request(TransitiveHydratedTargets,
[Specs(specs)])
for t in targets.dependencies)
for t in targets.roots)
graph = _DependentGraph.from_iterable(target_types_from_symbol_table(self._symbol_table),
adaptor_iter)

Expand Down
Loading

0 comments on commit 2915e75

Please sign in to comment.