Replace `include_special_cased_deps` flag with `should_traverse_deps_predicate` #19272

cognifloyd · 2023-06-08T02:28:37Z

This PR is extracted from #19155 which provides a way to stop dependency traversal through package targets. Several other improvements will probably benefit from this predicate as well (including proposals around multiple dependencies fields and/or tagged dependencies).

Passing a predicate on `Request` types

This PR replaces the include_special_cased_deps flag (on DependenciesRequest, TransitiveTargetsRequest, and CoarsenedTargetsRequest) with the should_traverse_deps_predicate predicate:

pants/src/python/pants/engine/target.py

Lines 2519 to 2521 in b856127

    
           class DependenciesRequest(EngineAwareParameter): 
        
               field: Dependencies 
        
               should_traverse_deps_predicate: ShouldTraverseDepsPredicate = TraverseIfDependenciesField()

pants/src/python/pants/engine/target.py

Lines 919 to 927 in b856127

    
           class TransitiveTargetsRequest: 
        
               """A request to get the transitive dependencies of the input roots. 
        
               Resolve the transitive targets with `await Get(TransitiveTargets, 
        
               TransitiveTargetsRequest([addr1, addr2])`. 
        
               """ 
        
               roots: Tuple[Address, ...] 
        
               should_traverse_deps_predicate: ShouldTraverseDepsPredicate

pants/src/python/pants/engine/target.py

Lines 882 to 887 in b856127

    
           class CoarsenedTargetsRequest: 
        
               """A request to get CoarsenedTargets for input roots.""" 
        
               roots: Tuple[Address, ...] 
        
               expanded_targets: bool 
        
               should_traverse_deps_predicate: ShouldTraverseDepsPredicate

Here is the ShouldTraverseDepsPredicate base class. This is a dataclass that serves as the "closure" around the __call__ predicate function; if it needs any data, the subclass should add fields for that data:

pants/src/python/pants/engine/target.py

Lines 717 to 739 in b856127

    
           @dataclass(frozen=True) 
        
           class ShouldTraverseDepsPredicate(metaclass=ABCMeta): 
        
               """This callable determines whether to traverse through deps of a given Target + Field. 
        
               This is a callable dataclass instead of a function to avoid issues with hashing closures. 
        
               Only the id is used when hashing a function; any closure vars are NOT accounted for. 
        
               NB: When subclassing a dataclass (like this one), generated methods on a dataclass are NOT 
        
               inherited because the @dataclass decorator only inspects the current class before generating 
        
               a function--the decorator does NOT inspect __mro__. 
        
               """ 
        
               # This _callable field ensures that __call__ is included in the __hash__ method generated by @dataclass. 
        
               _callable: Callable[ 
        
                   [Any, Target, Dependencies | SpecialCasedDependencies], bool 
        
               ] = dataclasses.field(init=False, repr=False) 
        
               def __post_init__(self): 
        
                   object.__setattr__(self, "_callable", type(self).__call__) 
        
               @abstractmethod 
        
               def __call__(self, target: Target, field: Dependencies | SpecialCasedDependencies) -> bool: 
        
                   """If this predicate returns false, then the target's field's deps will be ignored."""

Calling the requested predicate

The resolve_dependencies rule is responsible for calling this predicate in two places.

To bail out early and avoid dep hydration and inference if unneeded:

pants/src/python/pants/engine/internals/graph.py

Lines 1320 to 1326 in b856127

    
           tgt = wrapped_tgt.target 
        
           # This predicate allows the dep graph to ignore dependencies of selected targets 
        
           # including any explicit deps and any inferred deps. 
        
           # For example, to avoid traversing the deps of package targets. 
        
           if not request.should_traverse_deps_predicate(tgt, request.field): 
        
               return Addresses([])

To replace the include_special_cased_deps flag on a per-field basis. If there are no SpecialCasedDependencies fields, then the predicate won't be called.

pants/src/python/pants/engine/internals/graph.py

Lines 1411 to 1416 in b856127

    
           special_cased_fields = tuple( 
        
               field 
        
               for field in tgt.field_values.values() 
        
               if isinstance(field, SpecialCasedDependencies) 
        
               and request.should_traverse_deps_predicate(tgt, field) 
        
           )

Replacing the `include_special_cased_deps` with the predicate

This predicate replaces include_special_cased_deps to minimize the API around dep calculation as @stuhood recommended in #19155 (comment):

IMO, a true benchmark for this filtering API would be that we can replace the include_special_cased_deps flag with it.

I am very much in favor of adding an API to allow for filtering like this, but I think that from a complexity-budget perspective, killing/replacing "special-cased-deps" would be a really, really good thing to accomplish here.

The default predicate, TraverseIfDependenciesField uses isinstance(field, Dependencies) which is False for any SpecialCasedDependencies subclasses:

pants/src/python/pants/engine/target.py

Lines 742 to 750 in b856127

    
           class TraverseIfDependenciesField(ShouldTraverseDepsPredicate): 
        
               """This is the default ShouldTraverseDepsPredicate implementation. 
        
               This skips resolving dependencies for fields (like SpecialCasedDependencies) that are not 
        
               subclasses of Dependencies. 
        
               """ 
        
               def __call__(self, target: Target, field: Dependencies | SpecialCasedDependencies) -> bool: 
        
                   return isinstance(field, Dependencies)

Any thing that used include_special_cased_deps=True (the default was False) now passes in the AlwaysTraverseDeps which always returns True.

pants/src/python/pants/engine/target.py

Lines 753 to 760 in b856127

    
           class AlwaysTraverseDeps(ShouldTraverseDepsPredicate): 
        
               """A predicate to use when a request needs all deps. 
        
               This includes deps from fields like SpecialCasedDependencies which are ignored in most cases. 
        
               """ 
        
               def __call__(self, target: Target, field: Dependencies | SpecialCasedDependencies) -> bool: 
        
                   return True

These are examples of passing in that predicate:

pants/src/python/pants/backend/project_info/paths.py

Lines 117 to 130 in b856127

    
           transitive_targets = await Get( 
        
               TransitiveTargets, 
        
               TransitiveTargetsRequest( 
        
                   [root.address], should_traverse_deps_predicate=AlwaysTraverseDeps() 
        
               ), 
        
           ) 
        
           adjacent_targets_per_target = await MultiGet( 
        
               Get( 
        
                   Targets, 
        
                   DependenciesRequest( 
        
                       tgt.get(Dependencies), should_traverse_deps_predicate=AlwaysTraverseDeps() 
        
                   ), 
        
               )

This will replace the `include_special_cased_deps` flag

…deps_predicate=always_traverse`

cognifloyd · 2023-06-08T02:29:24Z

src/python/pants/engine/internals/graph.py

+async def convert_dependencies_request_to_explicitly_provided_dependencies_request(
    request: DependenciesRequest,
+) -> ExplicitlyProvidedDependenciesRequest:
+    """This rule discards any deps predicate from DependenciesRequest.
+
+    Calculating ExplicitlyProvidedDependencies does not use any deps traversal predicates as it is
+    meant to list all explicit deps from the given field. By stripping the predicate from the
+    request, we ensure that the cache key for ExplicitlyProvidedDependencies calculation does not
+    include the predicate increasing the cache-hit rate.
+    """
+    # TODO: Maybe require Get(ExplicitlyProvidedDependencies, ExplicitlyProvidedDependenciesRequest)
+    #       and deprecate Get(ExplicitlyProvidedDependencies, DependenciesRequest) via this rule.
+    return ExplicitlyProvidedDependenciesRequest(request.field)


In #19155, I just had a comment on the determine_explicitly_provided_dependencies rule that said: NB: This rule ignores request.should_traverse_deps_predicate.

@thejcannon recommended I use an actual type instead of just a comment in #19155 (comment)

We might consider a different input type that doesn't have the ignored attribute?

So, I added this ExplicitlyProvidedDependenciesRequest with a rule to minimize API churn in this PR.

Do we want to deprecate Get(ExplicitlyProvidedDependencies, DependenciesRequest)? If/when we do that, we can probably put a deprecation warning in this rule.

I think that's a good idea, although feel free to carry this over into a future PR so we can merge this one.

Also up to you here 😄

I think we can deprecate once we have updated all uses within the pants repo. So, it fits best in a future PR.

src/python/pants/engine/target.py

thejcannon

Love it! Likely the only round of comments then 🚀

src/python/pants/backend/project_info/dependencies.py

thejcannon · 2023-06-08T15:01:28Z

src/python/pants/engine/internals/graph.py

+async def convert_dependencies_request_to_explicitly_provided_dependencies_request(
    request: DependenciesRequest,
+) -> ExplicitlyProvidedDependenciesRequest:
+    """This rule discards any deps predicate from DependenciesRequest.
+
+    Calculating ExplicitlyProvidedDependencies does not use any deps traversal predicates as it is
+    meant to list all explicit deps from the given field. By stripping the predicate from the
+    request, we ensure that the cache key for ExplicitlyProvidedDependencies calculation does not
+    include the predicate increasing the cache-hit rate.
+    """
+    # TODO: Maybe require Get(ExplicitlyProvidedDependencies, ExplicitlyProvidedDependenciesRequest)
+    #       and deprecate Get(ExplicitlyProvidedDependencies, DependenciesRequest) via this rule.
+    return ExplicitlyProvidedDependenciesRequest(request.field)


I think that's a good idea, although feel free to carry this over into a future PR so we can merge this one.

Also up to you here 😄

src/python/pants/engine/target.py

kaos

lgtm! 🚀

kaos · 2023-06-08T17:45:36Z

src/python/pants/engine/target.py

+class DepsTraversalPredicates:
+    """All methods on this class are ShouldTraverseDepsPredicate implementations."""
+
+    # NB: ShouldTraverseDepsPredicate is defined below after SpecialCasedDependencies.
+    # Implementations are defined here, however, before they are using them as defaults
+    # in CoarsenedTargetsRequest, TransitiveTargetsRequest, and DependenciesRequest.
+
+    @staticmethod
+    def if_dependencies_field(
+        target: Target, field: Dependencies | SpecialCasedDependencies
+    ) -> bool:
+        """This is the default ShouldTraverseDepsPredicate implementation.
+
+        This skips resolving dependencies for fields (like SpecialCasedDependencies) that are not
+        subclasses of Dependencies.
+        """
+        return isinstance(field, Dependencies)
+
+    @staticmethod
+    def always(target: Target, field: Dependencies | SpecialCasedDependencies) -> bool:
+        """A predicate to use when a request needs all deps.
+
+        This includes deps from fields like SpecialCasedDependencies which are ignored in most
+        cases.
+        """
+        return True


Nice, I think this is a big improvement actually :)

…ations

src/python/pants/engine/target.py

@jriddy

I think we need to address the point raised by @jriddy

closure functions have a useless hash. The hash of a function does not account for any closure vars, including unhashable mutable vars, so that hash is not reliable. Plus, every instance of a closure will have a different id, so there cannot be any reuse of anything cached based on the function's hash. Using a frozen dataclass bypasses these issues. We include the class's __call__ function as a field in the parent dataclass so that it is part of the __hash__ function generated by the @DataClass decorator. Using the class's __call__ is more stable than using an instance's function.

kaos · 2023-06-13T17:56:50Z

src/python/pants/engine/target.py

+        """If this predicate returns false, then the target's field's deps will be ignored."""
+
+
+@dataclass(frozen=True)


do the subclasses actually need the dataclass decorator at all?

Without it, aren't they unfrozen? I'll try dropping the decorator on these subclasses since they don't add any fields.

Good catch. It works just fine without decorating the subclasses. I will have to add the decorator on the predicate that stops at package targets because that will add a field. These, however, don't need it.

I actually think we should still decorate:

It'll work correctly when someone adds a field later

When people copy+paste this code elsewhere they'll have it
My 2c

I adjusted one of the tests to make sure it works without the decorator. Doing that helped me resolve a bug with the type hint on _callable since the test made mypy inspect it more carefully.

I can re-add the decorator, I think I like it with the decorator a bit better as it is more consistent. But I've documented it as is for now.

I think it'll come down to preference then. I'm for not having them until needed, but won't object/block if you prefer to keep them.

This tests 2 things: - that subclasses without an @DataClass decorator are still frozen - that mypy says the types of _callable and __call__ are compatible For the mypy issue, I tried using Self instead of Any, but Self resolved to ShouldTraverseDepsPredicate which did not match SkipDepsTagOrTraverse for some reason.

thejcannon

Beautiful

src/python/pants/engine/target.py

Co-authored-by: Joshua Cannon <joshdcannon@gmail.com>

benjyw

LGTM FWIW

This updates `changelog.py` to write the (non-internal) changes to the relevant release notes file directly, rather than requiring a human to copy-paste it in. For now, the file is just mutated, without committing. The internal changes are still printed for the human to deal with. For instance, `pants run src/python/pants_release/changelog.py -- --prior 2.18.0.dev1 --new 2.18.0.dev2` adds a new section to `src/python/pants/notes/2.18.x.md`: ```diff diff --git a/src/python/pants/notes/2.18.x.md b/src/python/pants/notes/2.18.x.md index e648a4525c..d6668a24b1 100644 --- a/src/python/pants/notes/2.18.x.md +++ b/src/python/pants/notes/2.18.x.md @@ -1,5 +1,35 @@ # 2.18.x Release Series +## 2.18.0.dev2 (Jun 14, 2023) + +### New Features + +* Include complete platforms for FaaS environments for more reliable building ([#19253](#19253)) + +* Add experimental support for Rustfmt ([#18842](#18842)) + +* Helm deployment chart field ([#19234](#19234)) + +### Plugin API Changes + +* Replace `include_special_cased_deps` flag with `should_traverse_deps_predicate` ([#19272](#19272)) + +### Bug Fixes + +* Raise an error if isort can't read a config file ([#19294](#19294)) + +* Improve handling of additional files in Helm unit tests ([#19263](#19263)) + +* Add taplo to the release ([#19258](#19258)) + +* Handle `from foo import *` wildcard imports in Rust dep inference parser ([#19249](#19249)) + +* Support usage of `scala_artifact` addresses in `scalac_plugin` targets ([#19205](#19205)) + +### Performance + +* `scandir` returns `Stat`s relative to its directory. ([#19246](#19246)) + ## 2.18.0.dev1 (Jun 02, 2023) ### New Features ``` This also moves it into the new `pants_release` root, adds some basic tests, and has it fetch the relevant branch directly, rather than prompting the user to do so. It also pulls out a `git.py` support module with helpers for exec-ing `git` as an external process. The commits are individually reviewable. This is a step towards automating more of the "start release" process, per #19279. After this core refactoring of the functionality, I think the next step is to combine it with the CONTRIBUTORS update and version bumping, and then after that string it all together on CI so that starting a release is fully automated.

…plugins (#19306) This builds on #19272, adding another `should_traverse_deps_predicate` that stops dependency traversal at any package targets. This is mostly extracted from #19155. `TraverseIfNotPackageTarget` will be useful whenever a `TransitiveTargetsRequest`, `CoarsenedTargetsRequest`, or `DependenciesRequest` could benefit from treating package targets as leaves. This PR does not change any `TransitiveTargetsRequest`s because that is probably a change in user-facing behavior (even if it counts as a bugfix) and needs to be documented as such. This PR merely adds the feature, which, on its own, does not impact anything else. Related: - #18254 - #17368 - #15855 - #15082 --------- Co-authored-by: Andreas Stenius <andreas.stenius@imanage.com>

…19155) This builds on: - #19272 - #19306 - #19387 This only updates the `pex_binary` rule to use the new `TraverseIfNotPackageTarget` predicate when requesting the source files that should be included in this pex. Wheels, other pex_binaries, and other package targets are not handled via this code path since those are not python sources that get included in the pex based on the `include_sources` flag. Fixes #15855 This fixes #15855 because anything in a `python_distribution` does not need to be included as a source file in the `pex_binary`. wheels get included via the `LocalDists` rules. In some cases, we might still get more sources in the pex than intended. This might happen if a dependency is inferred between sources, bypassing the logic that looks for wheels that own the relevant things. In any case, this is PR provides an improvement and resolves the sample error in #15855. Related: - #18254 - #17368 - #15082 --------- Co-authored-by: Joshua Cannon <joshdcannon@gmail.com>

cognifloyd added 6 commits June 7, 2023 20:17

Add ShouldTraverseDepsPredicate

c95aa60

This will replace the `include_special_cased_deps` flag

Handle SpecialCasedDependencies in ShouldTraverseDepsPredicate

c2c6f26

This will replace the `include_special_cased_deps` flag

Replace include_special_cased_deps=True flag with `should_traverse_…

ef7a09f

…deps_predicate=always_traverse`

Drop include_special_cased_deps flag

e30672c

fmt

941c3c2

Add ExplicitlyProvidedDependenciesRequest

c1c9252

cognifloyd added the category:plugin api change label Jun 8, 2023

cognifloyd requested review from stuhood, kaos, thejcannon and Eric-Arellano June 8, 2023 02:28

cognifloyd self-assigned this Jun 8, 2023

cognifloyd commented Jun 8, 2023

View reviewed changes

cognifloyd mentioned this pull request Jun 8, 2023

pex_binary sandbox: Traversing dep graph, stopping after package targets #19155

Merged

thejcannon reviewed Jun 8, 2023

View reviewed changes

cognifloyd added 3 commits June 8, 2023 11:20

Add DepsTraversalPredicates class as a mini-namespace for predicates

c1ed5a2

Merge branch 'main' into cognifloyd/deps-predicate

39e5bde

Correct type hints for DepsTraversalPredicates methods

434a8fb

cognifloyd requested a review from thejcannon June 8, 2023 16:29

kaos previously approved these changes Jun 8, 2023

View reviewed changes

cognifloyd added 4 commits June 8, 2023 14:13

comment typo

945b50b

Merge branch 'main' into cognifloyd/deps-predicate

869b91f

Merge branch 'main' into cognifloyd/deps-predicate

f0a808c

Use TypeAlias to move ShouldTraverseDepsPredicate closer to implement…

a77b046

…ations

jriddy reviewed Jun 12, 2023

View reviewed changes

src/python/pants/engine/target.py Outdated Show resolved Hide resolved

kaos self-requested a review June 12, 2023 19:52

cognifloyd added 3 commits June 12, 2023 15:45

Merge branch 'main' into cognifloyd/deps-predicate

412e994

cleanup test

57660a6

kaos approved these changes Jun 13, 2023

View reviewed changes

cognifloyd added 2 commits June 13, 2023 13:24

Merge branch 'main' into cognifloyd/deps-predicate

b856127

cognifloyd requested a review from jriddy June 13, 2023 18:34

thejcannon approved these changes Jun 13, 2023

View reviewed changes

src/python/pants/engine/target.py Outdated Show resolved Hide resolved

Improve comment on _callable

7ce2bce

Co-authored-by: Joshua Cannon <joshdcannon@gmail.com>

cognifloyd force-pushed the cognifloyd/deps-predicate branch from 895a562 to 7ce2bce Compare June 13, 2023 19:24

reword comment about subclassing ShouldTraverseDepsPredicate

6a28cd6

jriddy approved these changes Jun 13, 2023

View reviewed changes

cognifloyd merged commit 9ba3a43 into main Jun 13, 2023

cognifloyd deleted the cognifloyd/deps-predicate branch June 13, 2023 21:33

cognifloyd mentioned this pull request Jun 14, 2023

Add TraverseIfNotPackageTarget deps traversal predicate for use in plugins #19306

Merged

benjyw reviewed Jun 14, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `include_special_cased_deps` flag with `should_traverse_deps_predicate` #19272

Replace `include_special_cased_deps` flag with `should_traverse_deps_predicate` #19272

cognifloyd commented Jun 8, 2023 •

edited

Loading

cognifloyd Jun 8, 2023

thejcannon Jun 8, 2023

cognifloyd Jun 8, 2023

thejcannon left a comment

thejcannon Jun 8, 2023

kaos left a comment

kaos Jun 8, 2023

kaos Jun 13, 2023

cognifloyd Jun 13, 2023

cognifloyd Jun 13, 2023

thejcannon Jun 13, 2023

cognifloyd Jun 13, 2023

kaos Jun 13, 2023

thejcannon left a comment

benjyw left a comment

	class DependenciesRequest(EngineAwareParameter):
	field: Dependencies
	should_traverse_deps_predicate: ShouldTraverseDepsPredicate = TraverseIfDependenciesField()

	class TransitiveTargetsRequest:
	"""A request to get the transitive dependencies of the input roots.

	Resolve the transitive targets with `await Get(TransitiveTargets,
	TransitiveTargetsRequest([addr1, addr2])`.
	"""

	roots: Tuple[Address, ...]
	should_traverse_deps_predicate: ShouldTraverseDepsPredicate

	class CoarsenedTargetsRequest:
	"""A request to get CoarsenedTargets for input roots."""

	roots: Tuple[Address, ...]
	expanded_targets: bool
	should_traverse_deps_predicate: ShouldTraverseDepsPredicate

	@dataclass(frozen=True)
	class ShouldTraverseDepsPredicate(metaclass=ABCMeta):
	"""This callable determines whether to traverse through deps of a given Target + Field.

	This is a callable dataclass instead of a function to avoid issues with hashing closures.
	Only the id is used when hashing a function; any closure vars are NOT accounted for.

	NB: When subclassing a dataclass (like this one), generated methods on a dataclass are NOT
	inherited because the @dataclass decorator only inspects the current class before generating
	a function--the decorator does NOT inspect __mro__.
	"""

	# This _callable field ensures that __call__ is included in the __hash__ method generated by @dataclass.
	_callable: Callable[
	[Any, Target, Dependencies \| SpecialCasedDependencies], bool
	] = dataclasses.field(init=False, repr=False)

	def __post_init__(self):
	object.__setattr__(self, "_callable", type(self).__call__)

	@abstractmethod
	def __call__(self, target: Target, field: Dependencies \| SpecialCasedDependencies) -> bool:
	"""If this predicate returns false, then the target's field's deps will be ignored."""

	tgt = wrapped_tgt.target

	# This predicate allows the dep graph to ignore dependencies of selected targets
	# including any explicit deps and any inferred deps.
	# For example, to avoid traversing the deps of package targets.
	if not request.should_traverse_deps_predicate(tgt, request.field):
	return Addresses([])

	special_cased_fields = tuple(
	field
	for field in tgt.field_values.values()
	if isinstance(field, SpecialCasedDependencies)
	and request.should_traverse_deps_predicate(tgt, field)
	)

	class TraverseIfDependenciesField(ShouldTraverseDepsPredicate):
	"""This is the default ShouldTraverseDepsPredicate implementation.

	This skips resolving dependencies for fields (like SpecialCasedDependencies) that are not
	subclasses of Dependencies.
	"""

	def __call__(self, target: Target, field: Dependencies \| SpecialCasedDependencies) -> bool:
	return isinstance(field, Dependencies)

	class AlwaysTraverseDeps(ShouldTraverseDepsPredicate):
	"""A predicate to use when a request needs all deps.

	This includes deps from fields like SpecialCasedDependencies which are ignored in most cases.
	"""

	def __call__(self, target: Target, field: Dependencies \| SpecialCasedDependencies) -> bool:
	return True

	transitive_targets = await Get(
	TransitiveTargets,
	TransitiveTargetsRequest(
	[root.address], should_traverse_deps_predicate=AlwaysTraverseDeps()
	),
	)

	adjacent_targets_per_target = await MultiGet(
	Get(
	Targets,
	DependenciesRequest(
	tgt.get(Dependencies), should_traverse_deps_predicate=AlwaysTraverseDeps()
	),
	)

Replace include_special_cased_deps flag with should_traverse_deps_predicate #19272

Replace include_special_cased_deps flag with should_traverse_deps_predicate #19272

Conversation

cognifloyd commented Jun 8, 2023 • edited Loading

Passing a predicate on Request types

Calling the requested predicate

Replacing the include_special_cased_deps with the predicate

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thejcannon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaos 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

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

thejcannon left a comment

Choose a reason for hiding this comment

benjyw left a comment

Choose a reason for hiding this comment

Replace `include_special_cased_deps` flag with `should_traverse_deps_predicate` #19272

Replace `include_special_cased_deps` flag with `should_traverse_deps_predicate` #19272

cognifloyd commented Jun 8, 2023 •

edited

Loading

Passing a predicate on `Request` types

Replacing the `include_special_cased_deps` with the predicate