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.

Sign up for GitHub

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

Support for peeking at dependency/dependents rules #18112

Merged
merged 13 commits into from
Mar 1, 2023
Merged

Support for peeking at dependency/dependents rules #18112

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
560772d
Peek: add `_effective_dep_rules` output field.
kaos Jan 28, 2023
7202e9e
refactor build_files.get_dependencies_rule_application to expose fami…
kaos Jan 28, 2023
2437d56
Support `peek`ing into dependency rule sets.
kaos Jan 28, 2023
89bc7f4
Peek into dependency/dependent rule sets.
kaos Jan 28, 2023
83bf41b
Add tests.
kaos Jan 28, 2023
9c91ba6
Merge branch 'main' into 17694/peek-visibility
kaos Feb 7, 2023
60563c4
drop `--include-dep-rules` option.
kaos Feb 7, 2023
955bc87
Merge remote-tracking branch 'upstream/main' into 17694/peek-visibility
kaos Feb 10, 2023
95e92b7
rename "effective" -> "applicable".
kaos Feb 10, 2023
df7c0b8
Revert "drop `--include-dep-rules` option."
kaos Feb 25, 2023
b693b47
Merge branch 'main' into 17694/peek-visibility
kaos Feb 25, 2023
eb175f5
Include structured dependency rule application data in peek output.
kaos Feb 25, 2023
fba1250
bump some test timeouts.
kaos Feb 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions src/python/pants/backend/project_info/peek.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,18 @@

from typing_extensions import Protocol, runtime_checkable

from pants.engine.addresses import Addresses
from pants.engine.collection import Collection
from pants.engine.console import Console
from pants.engine.goal import Goal, GoalSubsystem, Outputting
from pants.engine.internals.build_files import _get_target_family_and_adaptor_for_dep_rules
from pants.engine.internals.dep_rules import DependencyRuleSet
from pants.engine.rules import Get, MultiGet, collect_rules, goal_rule, rule
from pants.engine.target import (
Dependencies,
DependenciesRequest,
DependenciesRuleApplication,
DependenciesRuleApplicationRequest,
Field,
HydratedSources,
HydrateSourcesRequest,
Expand Down Expand Up @@ -66,6 +71,10 @@ class TargetData:
expanded_sources: tuple[str, ...] | None
expanded_dependencies: tuple[str, ...]

dependencies_rules: tuple[str, ...] | None = None
dependents_rules: tuple[str, ...] | None = None
effective_dep_rules: tuple[str, ...] | None = None

def to_dict(self, exclude_defaults: bool = False) -> dict:
nothing = object()
fields = {
Expand All @@ -80,6 +89,10 @@ def to_dict(self, exclude_defaults: bool = False) -> dict:
if self.expanded_sources is not None:
fields["sources"] = self.expanded_sources

fields["_dependencies_rules"] = self.dependencies_rules
fields["_dependents_rules"] = self.dependents_rules
fields["_effective_dep_rules"] = self.effective_dep_rules

return {
"address": self.target.address.spec,
"target_type": self.target.alias,
Expand Down Expand Up @@ -124,10 +137,17 @@ def default(self, o):
return str(o)


def describe_ruleset(ruleset: DependencyRuleSet | None) -> tuple[str, ...] | None:
if ruleset is None:
return None
return ruleset.peek()


@rule
async def get_target_data(
# NB: We must preserve target generators, not replace with their generated targets.
targets: UnexpandedTargets,
subsys: PeekSubsystem,
) -> TargetDatas:
sorted_targets = sorted(targets, key=lambda tgt: tgt.address)

Expand Down Expand Up @@ -159,11 +179,44 @@ async def get_target_data(
for tgt, hs in zip(targets_with_sources, hydrated_sources_per_target)
}

family_adaptors = await _get_target_family_and_adaptor_for_dep_rules(
*(tgt.address for tgt in sorted_targets),
description_of_origin="`peek` goal",
)
dependencies_rules_map = {
tgt.address: describe_ruleset(family.dependencies_rules.get_ruleset(tgt.address, adaptor))
for tgt, (family, adaptor) in zip(sorted_targets, family_adaptors)
if family.dependencies_rules is not None
}
dependents_rules_map = {
tgt.address: describe_ruleset(family.dependents_rules.get_ruleset(tgt.address, adaptor))
for tgt, (family, adaptor) in zip(sorted_targets, family_adaptors)
if family.dependents_rules is not None
}
all_effective_dep_rules = await MultiGet(
Copy link
Contributor

Choose a reason for hiding this comment

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

It's odd that we're introducing this new bit of terminology, "effective", but populating it with something using the terminology "application". Why not be consistent, and call this "applied_dependency_rules" or "applicable_dependency_rules"?

Copy link
Contributor

Choose a reason for hiding this comment

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

That said, I'm not sure what the purpose of this is? If a rule bans a dependency then that dependency won't exist, and so there will be no rule that applies to it. So this is just for rules that allow dependencies?

Copy link
Member Author

Choose a reason for hiding this comment

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

It allows you to see what rules are defined, and also, in case you see dependencies you thought would have been denied but aren't, which rules allowed them. So it lets you peek into the defined visibility rules. It's been requested for a way to find out what rules have been defined and what applies to certain targets etc.

Agree on effective vs applicable. I'll change that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think this is a bit confusing? What is in effective_ vs in dependencies/dependents?

Copy link
Member Author

Choose a reason for hiding this comment

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

in dependencies/dependents you set up a bunch of rules yea. They may or may not "capture" anything. Applied (effective) rules are those that actually hit something. You've got a dependency relation between two targets where some rule(s) is/are in play.

Copy link
Member Author

@kaos kaos Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Rules applying to this node as a dest The subset of these that actually applied for some edge

is that not very expensive? or, it shouldn't be worse than pants dependents .. ?
but that would be some extra lookup from what we do in peek now, any way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess so. I'm just trying to wrap my head around the utility of the "applied" data. Most of the time rules will ban edges, so those edges won't exist. So this is only for the case of an edge that was explicitly allowed by a rule, rather than being allowed by default because no rule bans it? And we're not actually putting this information on the edge, but on the source target, and we're not saying which edge it referred to?

Copy link
Member Author

Choose a reason for hiding this comment

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

There may be rules that use "warn" rather than "deny". We may introduce an option to treat "deny" as "warn", we may also, for peek and lint not exclude denied edges for the purpose of reporting (#17671 (comment)).

we're not saying which edge it referred to?

Oh, but we do.

src/python/pants/explorer/server/graphql -> src/python/pants : ALLOW
python_sources src/python/pants/explorer/server/graphql/field_types.py -> python_sources src/python/pants/__init__.py"

This tells us that for the dependency edge from ../graphql/field_types.py to ../pants/__init__.py was allowed (by default, as there where no rules involved.

The other example, for comparison:

src/python/pants/explorer/server/graphql -> 3rdparty/python/BUILD[src/python/pants/explorer/server/**] : ALLOW
python_sources src/python/pants/explorer/server/graphql/field_types.py -> python_requirements 3rdparty/python#strawberry-graphql",

Edge from ../graphql/field_types.py to ../python#strawberry-graphql was allowed by the rule .../server/** of the dependents rule defined in 3rdparty/python/BUILD, and there were no rules in play on the dependency side of the edge.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also worth noting is, that although this data only shows what has been allowed, the logs will hold information about what was denied, so that is already available. And if you expected something to be denied but wasn't, this will help pin point why.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, gotcha. OK, that is neat.

Get(
DependenciesRuleApplication,
DependenciesRuleApplicationRequest(
tgt.address,
Addresses(dep.address for dep in deps),
description_of_origin="`peek` goal",
),
)
for tgt, deps in zip(sorted_targets, dependencies_per_target)
)
effective_dep_rules_map = {
application.address: tuple(str(rule) for rule in application.dependencies_rule.values())
for application in all_effective_dep_rules
}

return TargetDatas(
TargetData(
tgt,
expanded_dependencies=expanded_deps,
expanded_sources=expanded_sources_map.get(tgt.address),
dependencies_rules=dependencies_rules_map.get(tgt.address),
dependents_rules=dependents_rules_map.get(tgt.address),
effective_dep_rules=effective_dep_rules_map.get(tgt.address),
)
for tgt, expanded_deps in zip(sorted_targets, expanded_dependencies)
)
Expand Down
121 changes: 120 additions & 1 deletion src/python/pants/backend/project_info/peek_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

from pants.backend.project_info import peek
from pants.backend.project_info.peek import Peek, TargetData, TargetDatas
from pants.backend.visibility.rules import rules as visibility_rules
from pants.base.specs import RawSpecs, RecursiveGlobSpec
from pants.core.target_types import ArchiveTarget, FilesGeneratorTarget, FileTarget, GenericTarget
from pants.engine.addresses import Address
Expand Down Expand Up @@ -46,6 +47,9 @@
{
"address": "example:files_target",
"target_type": "files",
"_dependencies_rules": null,
"_dependents_rules": null,
"_effective_dep_rules": null,
"dependencies": [],
"overrides": {
"('foo.txt',)": {
Expand All @@ -65,7 +69,7 @@
]
"""
),
id="single-files-target/exclude-defaults",
id="single-files-target/exclude-defaults-regression",
),
pytest.param(
[
Expand All @@ -84,6 +88,9 @@
{
"address": "example:files_target",
"target_type": "files",
"_dependencies_rules": null,
"_dependents_rules": null,
"_effective_dep_rules": null,
"dependencies": [],
"description": null,
"overrides": null,
Expand Down Expand Up @@ -130,6 +137,9 @@
{
"address": "example:files_target",
"target_type": "files",
"_dependencies_rules": null,
"_dependents_rules": null,
"_effective_dep_rules": null,
"dependencies": [],
"sources": [],
"sources_raw": [
Expand All @@ -142,6 +152,9 @@
{
"address": "example:archive_target",
"target_type": "archive",
"_dependencies_rules": null,
"_dependents_rules": null,
"_effective_dep_rules": null,
"dependencies": [
"foo/bar:baz",
"qux:quux"
Expand All @@ -157,6 +170,53 @@
),
id="single-files-target/exclude-defaults",
),
pytest.param(
[
TargetData(
FilesGeneratorTarget({"sources": ["*.txt"]}, Address("foo", target_name="baz")),
("foo/a.txt",),
("foo/a.txt:baz",),
dependencies_rules=("does", "apply", "*"),
dependents_rules=("fall-through", "*"),
effective_dep_rules=(
"foo/BUILD[*] -> foo/BUILD[*] : ALLOW\nfiles foo:baz -> files foo/a.txt:baz",
),
),
],
True,
dedent(
"""\
[
{
"address": "foo:baz",
"target_type": "files",
"_dependencies_rules": [
"does",
"apply",
"*"
],
"_dependents_rules": [
"fall-through",
"*"
],
"_effective_dep_rules": [
"foo/BUILD[*] -> foo/BUILD[*] : ALLOW\\nfiles foo:baz -> files foo/a.txt:baz"
],
"dependencies": [
"foo/a.txt:baz"
],
"sources": [
"foo/a.txt"
],
"sources_raw": [
"*.txt"
]
}
]
"""
),
id="include-dep-rules",
),
],
)
def test_render_targets_as_json(expanded_target_infos, exclude_defaults, expected_output):
Expand All @@ -169,6 +229,7 @@ def rule_runner() -> RuleRunner:
return RuleRunner(
rules=[
*peek.rules(),
*visibility_rules(),
QueryRule(TargetDatas, [RawSpecs]),
],
target_types=[FilesGeneratorTarget, GenericTarget],
Expand Down Expand Up @@ -204,24 +265,82 @@ def test_get_target_data(rule_runner: RuleRunner) -> None:
GenericTarget({"dependencies": [":baz"]}, Address("foo", target_name="bar")),
None,
("foo/a.txt:baz", "foo/b.txt:baz"),
effective_dep_rules=(
"foo -> foo : ALLOW\ntarget foo:bar -> files foo/a.txt:baz",
"foo -> foo : ALLOW\ntarget foo:bar -> files foo/b.txt:baz",
),
),
TargetData(
FilesGeneratorTarget({"sources": ["*.txt"]}, Address("foo", target_name="baz")),
("foo/a.txt", "foo/b.txt"),
("foo/a.txt:baz", "foo/b.txt:baz"),
effective_dep_rules=(
"foo -> foo : ALLOW\nfiles foo:baz -> files foo/a.txt:baz",
"foo -> foo : ALLOW\nfiles foo:baz -> files foo/b.txt:baz",
),
),
TargetData(
FileTarget(
{"source": "a.txt"}, Address("foo", relative_file_path="a.txt", target_name="baz")
),
("foo/a.txt",),
(),
effective_dep_rules=(),
),
TargetData(
FileTarget(
{"source": "b.txt"}, Address("foo", relative_file_path="b.txt", target_name="baz")
),
("foo/b.txt",),
(),
effective_dep_rules=(),
),
]


def test_get_target_data_with_dep_rules(rule_runner: RuleRunner) -> None:
rule_runner.write_files(
{
"foo/BUILD": dedent(
"""\
files(name="baz", sources=["*.txt"])
__dependencies_rules__(
("target", "does", "not", "apply", "*"),
("files", "does", "apply", "*"),
)
__dependents_rules__(
("[b.txt]", "!skip", "this", "*"),
("file", "take", "the", "first", "*"),
("*", "fall-through", "*"),
)
"""
),
"foo/a.txt": "",
}
)
tds = rule_runner.request(
TargetDatas,
[RawSpecs(recursive_globs=(RecursiveGlobSpec("foo"),), description_of_origin="tests")],
)
assert list(tds) == [
TargetData(
FilesGeneratorTarget({"sources": ["*.txt"]}, Address("foo", target_name="baz")),
("foo/a.txt",),
("foo/a.txt:baz",),
dependencies_rules=("does", "apply", "*"),
dependents_rules=("fall-through", "*"),
effective_dep_rules=(
"foo/BUILD[*] -> foo/BUILD[*] : ALLOW\nfiles foo:baz -> files foo/a.txt:baz",
),
),
TargetData(
FileTarget(
{"source": "a.txt"}, Address("foo", relative_file_path="a.txt", target_name="baz")
),
("foo/a.txt",),
(),
dependencies_rules=("does", "apply", "*"),
dependents_rules=("fall-through", "*"),
effective_dep_rules=(),
),
]
7 changes: 6 additions & 1 deletion src/python/pants/backend/visibility/rule_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,9 @@ def parse(cls, build_file: str, arg: Any) -> VisibilityRuleSet:
def __str__(self) -> str:
return self.build_file

def peek(self) -> tuple[str, ...]:
return tuple(map(str, self.rules))

@staticmethod
def _noop_rule(rule: str) -> bool:
return not rule or rule.startswith("#")
Expand Down Expand Up @@ -306,8 +309,10 @@ def get_action(
return ruleset, None, None

def get_ruleset(
self, address: Address, target: TargetAdaptor, relpath: str
self, address: Address, target: TargetAdaptor, relpath: str | None = None
) -> VisibilityRuleSet | None:
if relpath is None:
relpath = self._get_address_relpath(address)
for ruleset in self.rulesets:
if ruleset.match(address, target, relpath):
return ruleset
Expand Down
Loading