Add `TraverseIfNotPackageTarget` deps traversal predicate for use in plugins by cognifloyd · Pull Request #19306 · pantsbuild/pants

cognifloyd · 2023-06-14T01:09:11Z

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

Co-authored-by: Andreas Stenius <andreas.stenius@imanage.com>

thejcannon · 2023-06-16T02:04:57Z

~~Throw #18044 in the pile in the description 😄~~

Whoops. Related, but not the exact same 😛

cognifloyd · 2023-06-16T16:41:24Z

@benjyw could you review this one? It's the next piece of the deps traversal work.

benjyw · 2023-06-16T17:26:14Z

src/python/pants/core/goals/package.py

+            return False
+        if self.always_traverse_roots and target.address in self.roots:
+            return True
+        for field_set_type in self.package_field_set_types:


It would be nice if we could do isinstance(target, PackageTarget) or some such. This is where our very flexible fieldset model becomes a pain... But it is what it is, for now at least.

If you file an issue, assign it to me. I have some thoughts

thejcannon

VERY excited for this. I really can't express my thanks enough to you @cognifloyd for this. It's something that's plagued my headspace for quite some time, and this is quite elegant/simple.

I know this PR is merged, so all of my comments are just suggestions for if this code is going to be touched again.

thejcannon · 2023-06-19T16:11:42Z

src/python/pants/core/goals/package.py

+class TraverseIfNotPackageTarget(ShouldTraverseDepsPredicate):
+    package_field_set_types: FrozenOrderedSet[PackageFieldSet]
+    roots: FrozenOrderedSet[Address]
+    always_traverse_roots: bool = True  # traverse roots even if they are package targets


This doesn't need a default, since you define __init__

Addressed in #19387

thejcannon · 2023-06-19T16:12:18Z

src/python/pants/core/goals/package.py

+                # False means do not traverse dependencies of this target
+                return False


If you find yourself doing this often (and I forsee it happening) please switch to an enum so it is explicit 😄

Ah. Yes, that would make this more clear.

Addressed in #19387

thejcannon · 2023-06-19T16:14:18Z

src/python/pants/core/goals/package.py

+        for field_set_type in self.package_field_set_types:
+            if field_set_type.is_applicable(target):
+                # False means do not traverse dependencies of this target
+                return False
+        return True


Alternatively you could use any(field_set_type.is_applicable(target) for field_set_type in self.package_field_set_types)

I tend to prefer any and all because they express intent better (e.g. "return if any is packageable" is the English way, and it uses the word any)

Addressed in #19387

thejcannon · 2023-06-19T16:14:57Z

src/python/pants/core/goals/package.py



+@dataclass(frozen=True)
+class TraverseIfNotPackageTarget(ShouldTraverseDepsPredicate):


If you're in the vicinity of this code, add a helpful little docstring maybe?

Addressed in #19387

src/python/pants/core/goals/package.py

This addresses feedback from #19306 (review) The `ShouldTraverseDepsPredicate` returned a bool before. But, the bool was sometimes unclear, so this adds an enum to make the code more readable. NB: The values of the enum are not important.

…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 2 commits June 13, 2023 19:46

Add TraverseIfNotPackageTarget

0473c1e

Add test for TraverseIfNotPackageTarget

2343cd3

cognifloyd added the category:new feature label Jun 14, 2023

cognifloyd requested review from jriddy, kaos, stuhood and thejcannon June 14, 2023 01:09

cognifloyd self-assigned this Jun 14, 2023

fmt

08c00f8

cognifloyd mentioned this pull request Jun 14, 2023

Add nFPM backend for building system packages (only the nfpm core bits) #19308

Merged

cognifloyd requested a review from benjyw June 14, 2023 05:47

kaos approved these changes Jun 14, 2023

View reviewed changes

src/python/pants/core/goals/package.py Outdated Show resolved Hide resolved

Update src/python/pants/core/goals/package.py

8e5f9f3

Co-authored-by: Andreas Stenius <andreas.stenius@imanage.com>

kaos approved these changes Jun 14, 2023

View reviewed changes

cognifloyd mentioned this pull request Jun 15, 2023

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

Merged

cognifloyd changed the title ~~Add TraverseIfNotPackageTarget predicate for use in plugins~~ Add TraverseIfNotPackageTarget deps traversal predicate for use in plugins Jun 15, 2023

jriddy approved these changes Jun 16, 2023

View reviewed changes

benjyw approved these changes Jun 16, 2023

View reviewed changes

jriddy merged commit d5812af into main Jun 17, 2023

jriddy deleted the cognifloyd/package-deps-traversal branch June 17, 2023 23:43

thejcannon reviewed Jun 19, 2023

View reviewed changes

cognifloyd mentioned this pull request Jun 27, 2023

Add DepsTraversalBehavior(Enum) and some docstrings #19387

Merged

		# False means do not traverse dependencies of this target
		return False



		@dataclass(frozen=True)
		class TraverseIfNotPackageTarget(ShouldTraverseDepsPredicate):

Uh oh!

Conversation

cognifloyd commented Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

thejcannon commented Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cognifloyd commented Jun 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thejcannon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cognifloyd commented Jun 14, 2023 •

edited

Loading

thejcannon commented Jun 16, 2023 •

edited

Loading