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

Fix some dependencies-like fields not showing up with project introspection #10923

Merged
merged 2 commits into from
Oct 9, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion src/python/pants/backend/project_info/dependees.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ async def map_addresses_to_dependees() -> AddressToDependees:
)
all_targets = {*all_expanded_targets, *all_explicit_targets}
dependencies_per_target = await MultiGet(
Get(Addresses, DependenciesRequest(tgt.get(Dependencies))) for tgt in all_targets
Get(Addresses, DependenciesRequest(tgt.get(Dependencies), include_special_cased_deps=True))
for tgt in all_targets
)

address_to_dependees = defaultdict(set)
Expand Down
15 changes: 13 additions & 2 deletions src/python/pants/backend/project_info/dependees_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,17 @@
from pants.backend.project_info.dependees import DependeesGoal
from pants.backend.project_info.dependees import DependeesOutputFormat as OutputFormat
from pants.backend.project_info.dependees import rules as dependee_rules
from pants.engine.target import Dependencies, Target
from pants.engine.target import Dependencies, SpecialCasedDependencies, Target
from pants.testutil.test_base import TestBase


class SpecialDeps(SpecialCasedDependencies):
alias = "special_deps"


class MockTarget(Target):
alias = "tgt"
core_fields = (Dependencies,)
core_fields = (Dependencies, SpecialDeps)


class DependeesTest(TestBase):
Expand Down Expand Up @@ -139,3 +143,10 @@ def test_multiple_specified_targets(self) -> None:
}"""
).splitlines(),
)

def test_special_cased_dependencies(self) -> None:
self.add_to_build_file("special", "tgt(special_deps=['intermediate'])")
self.assert_dependees(targets=["intermediate"], expected=["leaf", "special"])
self.assert_dependees(
targets=["base"], transitive=True, expected=["intermediate", "leaf", "special"]
)
10 changes: 8 additions & 2 deletions src/python/pants/backend/project_info/dependencies.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,18 @@ async def dependencies(
console: Console, addresses: Addresses, dependencies_subsystem: DependenciesSubsystem
) -> Dependencies:
if dependencies_subsystem.transitive:
transitive_targets = await Get(TransitiveTargets, TransitiveTargetsRequest(addresses))
transitive_targets = await Get(
TransitiveTargets, TransitiveTargetsRequest(addresses, include_special_cased_deps=True)
)
targets = Targets(transitive_targets.dependencies)
else:
target_roots = await Get(UnexpandedTargets, Addresses, addresses)
dependencies_per_target_root = await MultiGet(
Get(Targets, DependenciesRequest(tgt.get(DependenciesField))) for tgt in target_roots
Get(
Targets,
DependenciesRequest(tgt.get(DependenciesField), include_special_cased_deps=True),
cosmicexplorer marked this conversation as resolved.
Show resolved Hide resolved
)
for tgt in target_roots
)
targets = Targets(itertools.chain.from_iterable(dependencies_per_target_root))

Expand Down
31 changes: 30 additions & 1 deletion src/python/pants/backend/project_info/dependencies_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,26 @@

from pants.backend.project_info.dependencies import Dependencies, DependencyType, rules
from pants.backend.python.target_types import PythonLibrary, PythonRequirementLibrary
from pants.engine.target import SpecialCasedDependencies, Target
from pants.testutil.rule_runner import RuleRunner


# We verify that any subclasses of `SpecialCasedDependencies` will show up with the `dependencies`
# goal by creating a mock target.
class SpecialDepsField(SpecialCasedDependencies):
alias = "special_deps"


class SpecialDepsTarget(Target):
alias = "special_deps_tgt"
core_fields = (SpecialDepsField,)


@pytest.fixture
def rule_runner() -> RuleRunner:
return RuleRunner(rules=rules(), target_types=[PythonLibrary, PythonRequirementLibrary])
return RuleRunner(
rules=rules(), target_types=[PythonLibrary, PythonRequirementLibrary, SpecialDepsTarget]
)


def create_python_library(
Expand Down Expand Up @@ -65,6 +79,21 @@ def test_no_dependencies(rule_runner: RuleRunner) -> None:
assert_dependencies(rule_runner, specs=["some/target"], expected=[], transitive=True)


def test_special_cased_dependencies(rule_runner: RuleRunner) -> None:
rule_runner.add_to_build_file(
"",
dedent(
"""\
special_deps_tgt(name='t1')
special_deps_tgt(name='t2', special_deps=[':t1'])
special_deps_tgt(name='t3', special_deps=[':t2'])
"""
),
)
assert_dependencies(rule_runner, specs=["//:t3"], expected=["//:t2"])
assert_dependencies(rule_runner, specs=["//:t3"], expected=["//:t1", "//:t2"], transitive=True)


def test_python_dependencies(rule_runner: RuleRunner) -> None:
create_python_requirement_library(rule_runner, name="req1")
create_python_requirement_library(rule_runner, name="req2")
Expand Down
4 changes: 3 additions & 1 deletion src/python/pants/backend/project_info/filedeps.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,9 @@ async def file_deps(
) -> Filedeps:
targets: Iterable[Target]
if filedeps_subsystem.transitive:
transitive_targets = await Get(TransitiveTargets, TransitiveTargetsRequest(addresses))
transitive_targets = await Get(
TransitiveTargets, TransitiveTargetsRequest(addresses, include_special_cased_deps=True)
cosmicexplorer marked this conversation as resolved.
Show resolved Hide resolved
)
targets = transitive_targets.closure
elif filedeps_subsystem.globs:
targets = await Get(UnexpandedTargets, Addresses, addresses)
Expand Down
26 changes: 14 additions & 12 deletions src/python/pants/backend/python/goals/pytest_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -157,21 +157,23 @@ async def setup_pytest_for_target(

# Create any assets that the test depends on through the `runtime_package_dependencies` field.
assets: Tuple[BuiltPackage, ...] = ()
if (
request.field_set.runtime_package_dependencies.value
or request.field_set.runtime_binary_dependencies.value
):
unparsed_addresses = (
*(request.field_set.runtime_package_dependencies.value or ()),
*(request.field_set.runtime_binary_dependencies.value or ()),
)
runtime_package_targets = await Get(
Targets,
UnparsedAddressInputs(unparsed_addresses, owning_address=request.field_set.address),
unparsed_runtime_packages = (
request.field_set.runtime_package_dependencies.to_unparsed_address_inputs()
)
unparsed_runtime_binaries = (
request.field_set.runtime_binary_dependencies.to_unparsed_address_inputs()
)
if unparsed_runtime_packages.values or unparsed_runtime_binaries.values:
runtime_package_targets, runtime_binary_dependencies = await MultiGet(
Get(Targets, UnparsedAddressInputs, unparsed_runtime_packages),
Get(Targets, UnparsedAddressInputs, unparsed_runtime_binaries),
)
field_sets_per_target = await Get(
FieldSetsPerTarget,
FieldSetsPerTargetRequest(PackageFieldSet, runtime_package_targets),
FieldSetsPerTargetRequest(
PackageFieldSet,
itertools.chain(runtime_package_targets, runtime_binary_dependencies),
),
cosmicexplorer marked this conversation as resolved.
Show resolved Hide resolved
)
assets = await MultiGet(
Get(BuiltPackage, PackageFieldSet, field_set)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -446,11 +446,17 @@ def test_runtime_package_dependency(rule_runner: RuleRunner) -> None:
rule_runner.create_file(
f"{PACKAGE}/test_binary_call.py",
dedent(
"""\
f"""\
import os.path
import subprocess

def test_embedded_binary():
assert b"Hello, test!" in subprocess.check_output(args=['./bin.pex'])

# Ensure that we didn't accidentally pull in the binary's sources. This is a
# special type of dependency that should not be included with the rest of the
# normal dependencies.
assert os.path.exists("{BINARY_SOURCE.path}") is False
"""
),
)
Expand Down
12 changes: 5 additions & 7 deletions src/python/pants/backend/python/target_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@
ProvidesField,
ScalarField,
Sources,
SpecialCasedDependencies,
StringField,
StringOrStringSequenceField,
StringSequenceField,
Target,
WrappedTarget,
)
Expand Down Expand Up @@ -269,9 +269,7 @@ class PythonTestsDependencies(Dependencies):
supports_transitive_excludes = True


# TODO(#10888): Teach project introspection goals that this is a special type of the `Dependencies`
# field.
class PythonRuntimePackageDependencies(StringSequenceField):
class PythonRuntimePackageDependencies(SpecialCasedDependencies):
"""Addresses to targets that can be built with the `./pants package` goal and whose resulting
assets should be included in the test run.

Expand All @@ -286,14 +284,14 @@ class PythonRuntimePackageDependencies(StringSequenceField):
alias = "runtime_package_dependencies"


class PythonRuntimeBinaryDependencies(StringSequenceField):
class PythonRuntimeBinaryDependencies(SpecialCasedDependencies):
"""Deprecated in favor of the `runtime_build_dependencies` field, which works with more target
types like `archive` and `python_awslambda`."""

alias = "runtime_binary_dependencies"

@classmethod
def compute_value(
def sanitize_raw_value(
cls, raw_value: Optional[Iterable[str]], *, address: Address
) -> Optional[Tuple[str, ...]]:
if raw_value is not None:
Expand All @@ -302,7 +300,7 @@ def compute_value(
"field is now deprecated in favor of the more flexible "
"`runtime_package_dependencies` field, and it will be removed in 2.1.0.dev0."
)
return super().compute_value(raw_value, address=address)
return super().sanitize_raw_value(raw_value, address=address)


class PythonTestsTimeout(IntField):
Expand Down
33 changes: 12 additions & 21 deletions src/python/pants/core/target_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from dataclasses import dataclass
from typing import Tuple

from pants.core.goals.package import BuiltPackage, OutputPathField, PackageFieldSet
from pants.core.util_rules.archive import ArchiveFormat, CreateArchive
Expand All @@ -19,8 +18,8 @@
HydratedSources,
HydrateSourcesRequest,
Sources,
SpecialCasedDependencies,
StringField,
StringSequenceField,
Target,
Targets,
WrappedTarget,
Expand Down Expand Up @@ -62,9 +61,7 @@ class RelocatedFilesSources(Sources):
expected_num_files = 0


# TODO(#10888): Teach project introspection goals that this is a special type of the `Dependencies`
# field.
class RelocatedFilesOriginalTargets(StringSequenceField):
class RelocatedFilesOriginalTargets(SpecialCasedDependencies):
"""Addresses to the original `files()` targets that you want to relocate, such as
`['//:json_files']`.

Expand All @@ -74,7 +71,6 @@ class RelocatedFilesOriginalTargets(StringSequenceField):

alias = "files_targets"
required = True
value: Tuple[str, ...]


class RelocatedFilesSrcField(StringField):
Expand Down Expand Up @@ -161,7 +157,11 @@ async def relocate_files(request: RelocateFilesViaCodegenRequest) -> GeneratedSo
AddressInput,
AddressInput.parse(v, relative_to=request.protocol_target.address.spec_path),
)
for v in request.protocol_target.get(RelocatedFilesOriginalTargets).value
for v in (
request.protocol_target.get(RelocatedFilesOriginalTargets)
.to_unparsed_address_inputs()
.values
)
)
original_files_sources = await MultiGet(
Get(HydratedSources, HydrateSourcesRequest(wrapped_tgt.target.get(Sources)))
Expand Down Expand Up @@ -222,9 +222,8 @@ class GenericTarget(Target):
# `archive` target
# -----------------------------------------------------------------------------------------------

# TODO(#10888): Teach project introspection goals that this is a special type of the `Dependencies`
# field.
class ArchivePackages(StringSequenceField):

class ArchivePackages(SpecialCasedDependencies):
"""Addresses to any targets that can be built with `./pants package`.

Pants will build the assets as if you had run `./pants package`. It will include the
Expand All @@ -238,9 +237,7 @@ class ArchivePackages(StringSequenceField):
alias = "packages"


# TODO(#10888): Teach project introspection goals that this is a special type of the `Dependencies`
# field.
class ArchiveFiles(StringSequenceField):
class ArchiveFiles(SpecialCasedDependencies):
"""Addresses to any `files` or `relocated_files` targets to include in the archive, e.g.
`["resources:logo"]`.

Expand Down Expand Up @@ -293,14 +290,8 @@ async def package_archive_target(
field_set: ArchiveFieldSet, global_options: GlobalOptions
) -> BuiltPackage:
package_targets, files_targets = await MultiGet(
Get(
Targets,
UnparsedAddressInputs(field_set.packages.value or (), owning_address=field_set.address),
),
Get(
Targets,
UnparsedAddressInputs(field_set.files.value or (), owning_address=field_set.address),
),
Get(Targets, UnparsedAddressInputs, field_set.packages.to_unparsed_address_inputs()),
Get(Targets, UnparsedAddressInputs, field_set.files.to_unparsed_address_inputs()),
)

package_field_sets_per_target = await Get(
Expand Down
40 changes: 39 additions & 1 deletion src/python/pants/engine/internals/graph.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
InjectedDependencies,
RegisteredTargetTypes,
Sources,
SpecialCasedDependencies,
Subtargets,
Target,
TargetRootsToFieldSets,
Expand Down Expand Up @@ -309,7 +310,14 @@ async def transitive_targets(request: TransitiveTargetsRequest) -> TransitiveTar
dependency_mapping: Dict[Address, Tuple[Address, ...]] = {}
while queued:
direct_dependencies = await MultiGet(
Get(Targets, DependenciesRequest(tgt.get(Dependencies))) for tgt in queued
Get(
Targets,
DependenciesRequest(
tgt.get(Dependencies),
include_special_cased_deps=request.include_special_cased_deps,
),
)
for tgt in queued
)

dependency_mapping.update(
Expand Down Expand Up @@ -865,13 +873,43 @@ async def resolve_dependencies(
t.address for t in subtargets.subtargets if t.address != request.field.address
)

# If the target has `SpecialCasedDependencies`, such as the `archive` target having
# `files` and `packages` fields, then we possibly include those too. We don't want to always
# include those dependencies because they should often be excluded from the result due to
# being handled elsewhere in the calling code.
special_cased: Tuple[Address, ...] = ()
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this would be incredibly difficult to move to a separate rule for DependenciesWithIndirectDeps, which would append the special_cased to the result as is done here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we added a new rule, we would probably want its result type to be Addresses to avoid having to invent more wrapper dataclasse. But, that will result in graph ambiguity to use await Get(Addresses, Foo) in this rule, unless we added the new dataclass.

I don't think there's strong enough of motivation yet to factor this out and pay the cost of Yet Another Dataclass and rule. Note that we don't really expect call sites to resolve all their special deps in one single call, without their normal deps included. Instead, we expect them to resolve each distinct field separately, because each field has its own magical handling. archive is a good example of this:

package_targets, files_targets = await MultiGet(
Get(
Targets,
UnparsedAddressInputs(field_set.packages.value or (), owning_address=field_set.address),
),
Get(
Targets,
UnparsedAddressInputs(field_set.files.value or (), owning_address=field_set.address),
),
)

Copy link
Contributor

Choose a reason for hiding this comment

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

because each field has its own magical handling

I'm not sure I understand. If this is the case, why aren't we separating the calls for Dependencies and SpecialCasedDependencies then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because there are very select places that we want all dependencies, both normal and special cased. Specifically, project introspection needs all dependencies.

It is simpler in those places to have one call await Get(Addresses, DependenciesRequest) than a call for normal deps + call for all the special cased. In particular, this is apparent with TransitiveTargets. With this current design, we don't need to implement a separate transitive targets rule that duplicates the normal rule. It sounds like you tried implementing new distinct dataclasses yesterday and found this to be the case?

My above comment was trying to explain that in almost all other contexts, you do not want all dependencies, both normal and magical, to be in the same result. You want to separate them out. I was pointing to how that it possible with that design.

Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds like you tried implementing new distinct dataclasses yesterday and found this to be the case?

It would have been extremely trivial if #8542 had been merged, which would have allowed providing a @union base as an @rule argument. We could have then made a TransitiveTargetsRequestBase union, and it would have generated a version of that rule for each type. I have asked you and others to take a look at that PR many times to no avail. Would you consider looking at it again if I updated it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll take a look at #8542 and comment there

if request.include_special_cased_deps:
wrapped_tgt = await Get(WrappedTarget, Address, request.field.address)
# Unlike normal, we don't use `tgt.get()` because there may be >1 subclass of
# SpecialCasedDependencies.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we link to #10888 here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, could we add a TODO pointing to #10924?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there's any TODO we need to do here, tbh. This is idiomatic Python and idiomatic usage of the Target API. We get the functionality we need in 4 lines of code, without needing to invent any new abstractions.

The comment is solely to draw attention to why we don't use tgt.get() like normal.

Is there something you think we could improve with these 4 lines?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's just confusing to me why we would iterate over the target's fields again here instead of making that part of the original DependenciesRequest. I think it was a good decision to make sure each Field has an Address, but it feels like that's useful for when we discover new things we need to introspect about the original target. I'm not sure it's really a problem, but it feels like that might be part of why the graph cycle is happening here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really following the argument here. At this point, we had only discovered that there is a Dependencies field (or subclass). We need to check if there are any other special cased dependencies fields that are in play, and the way to do that is to iterate over all the fields to see if they subclass this marker.

Copy link
Contributor

Choose a reason for hiding this comment

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

At this point, we had only discovered that there is a Dependencies field (or subclass).

What if we included the special-cased dependencies fields in the original DependenciesRequest?

special_cased_fields = tuple(
field
for field in wrapped_tgt.target.field_values.values()
if isinstance(field, SpecialCasedDependencies)
)
# We can't use the normal `Get(Addresses, UnparsedAddressInputs)` due to a graph cycle.
cosmicexplorer marked this conversation as resolved.
Show resolved Hide resolved
special_cased = await MultiGet(
Get(
Address,
AddressInput,
AddressInput.parse(
addr,
relative_to=request.field.address.spec_path,
subproject_roots=global_options.options.subproject_roots,
),
)
for special_cased_field in special_cased_fields
for addr in special_cased_field.to_unparsed_address_inputs().values
)

result = {
addr
for addr in (
*subtarget_addresses,
*literal_addresses,
*itertools.chain.from_iterable(injected),
*itertools.chain.from_iterable(inferred),
*special_cased,
)
if addr not in ignored_addresses
}
Expand Down
Loading