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

Add initial support for a parametrize builtin to generate multiple copies of a target #14408

Merged
merged 3 commits into from
Feb 11, 2022
Merged
Show file tree
Hide file tree
Changes from 2 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
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ async def generate_from_pipenv_requirement(
generator = request.generator
lock_rel_path = generator[PipenvSourceField].value
lock_full_path = generator[PipenvSourceField].file_path
overrides = {
canonicalize_project_name(k): v
for k, v in request.require_unparametrized_overrides().items()
}

file_tgt = PythonRequirementsFileTarget(
{PythonRequirementsFileSourcesField.alias: lock_rel_path},
Expand All @@ -96,7 +100,6 @@ async def generate_from_pipenv_requirement(

module_mapping = generator[ModuleMappingField].value
stubs_mapping = generator[TypeStubsModuleMappingField].value
overrides = {canonicalize_project_name(k): v for k, v in request.overrides.items()}
inherited_fields = {
field.alias: field.value
for field in request.generator.field_values.values()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,10 @@ async def generate_from_python_requirement(
generator = request.generator
pyproject_rel_path = generator[PoetryRequirementsSourceField].value
pyproject_full_path = generator[PoetryRequirementsSourceField].file_path
overrides = {
canonicalize_project_name(k): v
for k, v in request.require_unparametrized_overrides().items()
}

file_tgt = PythonRequirementsFileTarget(
{PythonRequirementsFileSourcesField.alias: pyproject_rel_path},
Expand Down Expand Up @@ -453,7 +457,6 @@ async def generate_from_python_requirement(

module_mapping = generator[ModuleMappingField].value
stubs_mapping = generator[TypeStubsModuleMappingField].value
overrides = {canonicalize_project_name(k): v for k, v in request.overrides.items()}
inherited_fields = {
field.alias: field.value
for field in request.generator.field_values.values()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ async def generate_from_python_requirement(
generator = request.generator
requirements_rel_path = generator[PythonRequirementsSourceField].value
requirements_full_path = generator[PythonRequirementsSourceField].file_path
overrides = {
canonicalize_project_name(k): v
for k, v in request.require_unparametrized_overrides().items()
}

file_tgt = PythonRequirementsFileTarget(
{PythonRequirementsFileSourcesField.alias: requirements_rel_path},
Expand Down Expand Up @@ -108,7 +112,6 @@ async def generate_from_python_requirement(

module_mapping = generator[ModuleMappingField].value
stubs_mapping = generator[TypeStubsModuleMappingField].value
overrides = {canonicalize_project_name(k): v for k, v in request.overrides.items()}
inherited_fields = {
field.alias: field.value
for field in request.generator.field_values.values()
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/backend/python/target_types_rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ async def generate_targets_from_pex_binaries(
) -> GeneratedTargets:
generator_addr = request.generator.address
entry_points_field = request.generator[PexEntryPointsField].value or []
overrides = request.overrides
overrides = request.require_unparametrized_overrides()
inherited_fields = {
field.alias: field.value
for field in request.generator.field_values.values()
Expand Down
37 changes: 16 additions & 21 deletions src/python/pants/build_graph/address.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@
from pants.util.frozendict import FrozenDict
from pants.util.strutil import strip_prefix

# `:` and `#` used as delimiters already. Others are reserved for possible future needs.
# `:`, `#`, `@` are used as delimiters already. Others are reserved for possible future needs.
BANNED_CHARS_IN_TARGET_NAME = frozenset(r":#!@?/\=")
BANNED_CHARS_IN_GENERATED_NAME = frozenset(r":#!@?=")
BANNED_CHARS_IN_PARAMETERS = frozenset(r":#!@?=, ")


class InvalidAddress(ValueError):
Expand Down Expand Up @@ -388,30 +389,24 @@ def sanitize(s: str) -> str:
prefix = sanitize(self.spec_path)
return f"{prefix}{path}{target}{params}{generated}"

def parametrize(self, parameters: Mapping[str, str]) -> Address:
"""Creates a new Address with the given `parameters` merged over self.parameters."""
merged_parameters = {**self.parameters, **parameters}
return self.__class__(
self.spec_path,
target_name=self._target_name,
generated_name=self.generated_name,
relative_file_path=self._relative_file_path,
parameters=merged_parameters,
)

def maybe_convert_to_target_generator(self) -> Address:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is target_generator a misnomer? Maybe base_target?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, it probably is. It used to be build_target, although I like base.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably best as a followup though?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine with me.

Maybe open an issue tracking all the little fixes like this? I know you have some dedicated tickets for bigger changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

"""If this address is generated, convert it to its generator target.
"""If this address is generated or parametrized, convert it to its generator target.

Otherwise, return self unmodified.
"""
if self.is_generated_target:
return self.__class__(
self.spec_path, target_name=self._target_name, parameters=self.parameters
)
return self

def maybe_convert_to_generated_target(self) -> Address:
"""If this address is for a file target, convert it into generated target syntax
(dir/f.ext:lib -> dir:lib#f.ext).

Otherwise, return itself unmodified.
"""
if self.is_file_target:
return self.__class__(
self.spec_path,
target_name=self._target_name,
parameters=self.parameters,
generated_name=self._relative_file_path,
)
if self.is_generated_target or self.is_parametrized:
return self.__class__(self.spec_path, target_name=self._target_name)
return self

def create_generated(self, generated_name: str) -> Address:
Expand Down
28 changes: 0 additions & 28 deletions src/python/pants/build_graph/address_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -383,34 +383,6 @@ def assert_noops(addr: Address) -> None:
assert_noops(Address("a/b"))


def test_address_maybe_convert_to_generated_target() -> None:
stuhood marked this conversation as resolved.
Show resolved Hide resolved
def assert_converts(file_addr: Address, *, expected: Address) -> None:
assert file_addr.maybe_convert_to_generated_target() == expected

assert_converts(
Address("a/b", relative_file_path="c.txt", target_name="c"),
expected=Address("a/b", target_name="c", generated_name="c.txt"),
)
assert_converts(
Address("a/b", relative_file_path="c.txt"), expected=Address("a/b", generated_name="c.txt")
)
assert_converts(
Address("a/b", relative_file_path="subdir/f.txt"),
expected=Address("a/b", generated_name="subdir/f.txt"),
)
assert_converts(
Address("a/b", relative_file_path="subdir/f.txt", target_name="original"),
expected=Address("a/b", target_name="original", generated_name="subdir/f.txt"),
)

def assert_noops(addr: Address) -> None:
assert addr.maybe_convert_to_generated_target() is addr

assert_noops(Address("a/b", target_name="c"))
assert_noops(Address("a/b"))
assert_noops(Address("a/b", generated_name="generated"))


def test_address_create_generated() -> None:
assert Address("dir", target_name="generator").create_generated("generated") == Address(
"dir", target_name="generator", generated_name="generated"
Expand Down
8 changes: 8 additions & 0 deletions src/python/pants/core/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
These are always activated and cannot be disabled.
"""
from pants.bsp.rules import rules as bsp_rules
from pants.build_graph.build_file_aliases import BuildFileAliases
from pants.core.goals import (
check,
export,
Expand Down Expand Up @@ -39,6 +40,7 @@
stripped_source_files,
subprocess_environment,
)
from pants.engine.internals.parametrize import Parametrize
from pants.goal import anonymous_telemetry, stats_aggregator
from pants.python import binaries as python_binaries
from pants.source import source_root
Expand Down Expand Up @@ -86,3 +88,9 @@ def target_types():
ResourcesGeneratorTarget,
RelocatedFiles,
]


def build_file_aliases():
return BuildFileAliases(
objects={"parametrize": Parametrize},
Copy link
Contributor

Choose a reason for hiding this comment

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

V happy this is not a CAOF, I'm eagerly awaiting dropping that infrastructure.

)
85 changes: 66 additions & 19 deletions src/python/pants/engine/internals/graph.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
SpecsSnapshot,
)
from pants.engine.internals import native_engine
from pants.engine.internals.parametrize import Parametrize
from pants.engine.internals.target_adaptor import TargetAdaptor
from pants.engine.rules import Get, MultiGet, collect_rules, rule
from pants.engine.target import (
Expand Down Expand Up @@ -96,7 +97,7 @@
from pants.util.frozendict import FrozenDict
from pants.util.logging import LogLevel
from pants.util.ordered_set import FrozenOrderedSet, OrderedSet
from pants.util.strutil import bullet_list
from pants.util.strutil import bullet_list, pluralize

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -177,17 +178,27 @@ def warn_deprecated_field_type(request: _WarnDeprecatedFieldRequest) -> _WarnDep

@dataclass(frozen=True)
class _TargetParametrizations:
"""All parametrizations and generated targets for a single input Address."""
"""All parametrizations and generated targets for a single input Address.

original_target: Target
If a Target has been parametrized, it might _not_ be present in this output, due to it not being
addressable using its un-parameterized Address.
"""

original_target: Target | None
parametrizations: FrozenDict[Address, Target]

def get(self, address: Address) -> Target | None:
if self.original_target.address == address:
if self.original_target and self.original_target.address == address:
return self.original_target
return self.parametrizations.get(address)

def generated_or_generator(self, maybe_generator: Address) -> Iterator[Target]:
if not self.original_target:
raise ValueError(
"A `parametrized` target cannot be consumed without its parameters specified.\n"
f"Target `{maybe_generator}` can be addressed as:\n"
f"{bullet_list(addr.spec for addr in self.parametrizations)}"
)
if self.parametrizations:
# Generated Targets.
yield from self.parametrizations.values()
Expand Down Expand Up @@ -219,7 +230,7 @@ async def resolve_target_parametrizations(
):
await Get(_WarnDeprecatedTarget, _WarnDeprecatedTargetRequest(target_type))

target: Target
target: Target | None
generate_request = target_types_to_generate_requests.request_for(target_type)
if generate_request:
Copy link
Member

Choose a reason for hiding this comment

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

This is a huge if block... possible/worth to split out (or perhaps await Joshuas rule parsing that supports following function calls)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I rushed this one in without doing this. I'm going to open a ticket for followup (since I'm out of town for the next few days), but will clean this up before 2.11.x.

# Split out the `propagated_fields` before construction.
Expand All @@ -240,6 +251,17 @@ async def resolve_target_parametrizations(
if field_value is not None:
template_fields[field_type.alias] = field_value

generator_fields_parametrized = {
name for name, field in generator_fields.items() if isinstance(field, Parametrize)
}
if generator_fields_parametrized:
noun = pluralize(len(generator_fields_parametrized), "field", include_count=False)
raise ValueError(
f"Only fields which will be moved to generated targets may be parametrized, "
f"so target generator {address} (with type {target_type.alias}) cannot "
f"parametrize the {generator_fields_parametrized} {noun}."
)

target = target_type(generator_fields, address, union_membership)

overrides = {}
Expand All @@ -260,21 +282,44 @@ async def resolve_target_parametrizations(
else:
overrides = overrides_field.flatten()

generated = await Get(
GeneratedTargets,
GenerateTargetsRequest,
generate_request(
target,
template_address=address,
template=template_fields,
overrides=overrides,
),
all_generated = await MultiGet(
Get(
GeneratedTargets,
GenerateTargetsRequest,
generate_request(
target,
template_address=address,
template=template,
overrides={
name: dict(Parametrize.expand(address, override))
for name, override in overrides.items()
},
),
)
for address, template in Parametrize.expand(address, template_fields)
)
generated = FrozenDict(
GeneratedTargets(
target, (t for generated_batch in all_generated for t in generated_batch.values())
)
)
else:
target = target_type(target_adaptor.kwargs, address, union_membership)
generated = GeneratedTargets(target, ())
first, *rest = Parametrize.expand(address, target_adaptor.kwargs)
if rest:
target = None
generated = FrozenDict(
(
parameterized_address,
target_type(parameterized_fields, parameterized_address, union_membership),
)
for parameterized_address, parameterized_fields in (first, *rest)
)
else:
target = target_type(target_adaptor.kwargs, address, union_membership)
generated = FrozenDict()

for field_type in target.field_types:
# TODO: Move to Target constructor.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nack, we need to use the engine for singleton so that we don't warn hundreds of times.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea... but we could use the @memoized infrastructure for that, and it would cause it to be triggered in all cases, including for generated targets. Right now it's only triggered for generator targets, and only for non-moved fields.

for field_type in target.field_types if target else ():
if (
field_type.deprecated_alias is not None
and field_type.deprecated_alias in target_adaptor.kwargs
Expand All @@ -291,8 +336,10 @@ async def resolve_target(
) -> WrappedTarget:
base_address = address.maybe_convert_to_target_generator()
parametrizations = await Get(_TargetParametrizations, Address, base_address)
if address.is_generated_target and not target_types_to_generate_requests.is_generator(
parametrizations.original_target
if (
address.is_generated_target
and parametrizations.original_target
and not target_types_to_generate_requests.is_generator(parametrizations.original_target)
):
# TODO: This is an accommodation to allow using file/generator Addresses for non-generator
# atom targets. See https://github.com/pantsbuild/pants/issues/14419.
Expand Down
Loading