Skip to content

Commit

Permalink
Remove [python-protobuf].runtime_dependencies in favor of Pants dis…
Browse files Browse the repository at this point in the history
…covering the dependency (pantsbuild#14691)

@tdyas had the idea that rather than you having to tell Pants where to load `protobuf`, `grpcio`, and `thrift`, we can simply discover it! We do this for Scala already and it works well.

There's minimal performance hit because we already will have created a third-party module mapping.

Two motivations:

1. Ergonomics.
2. Unblock codegen supporting multiple resolves: pantsbuild#14484. Otherwise we would need to add `[python-protobuf].runtime_dependencies_per_resolve`, which violates our goal for resolves to not make things more complex for the average user who only wants one resolve.

Note that this was already deprecated in 2.10, so we can outright remove here.

[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
  • Loading branch information
Eric-Arellano committed Mar 3, 2022
1 parent f14359e commit 5344b97
Show file tree
Hide file tree
Showing 12 changed files with 259 additions and 74 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,6 @@

from pants.backend.codegen.protobuf.java.rules import GenerateJavaFromProtobufRequest
from pants.backend.codegen.protobuf.java.rules import rules as protobuf_rules
from pants.backend.codegen.protobuf.python.python_protobuf_subsystem import (
rules as protobuf_subsystem_rules,
)
from pants.backend.codegen.protobuf.target_types import (
ProtobufSourceField,
ProtobufSourcesGeneratorTarget,
Expand Down Expand Up @@ -50,7 +47,6 @@ def rule_runner() -> RuleRunner:
return RuleRunner(
rules=[
*protobuf_rules(),
*protobuf_subsystem_rules(),
*stripped_source_files.rules(),
*target_types_rules(),
QueryRule(HydratedSources, [HydrateSourcesRequest]),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,21 @@
# Licensed under the Apache License, Version 2.0 (see LICENSE).


from pants.backend.codegen.protobuf.target_types import ProtobufDependenciesField
from pants.backend.codegen.protobuf.target_types import (
ProtobufDependenciesField,
ProtobufGrpcToggleField,
)
from pants.backend.codegen.utils import find_python_runtime_library_or_raise_error
from pants.backend.python.dependency_inference.module_mapper import ThirdPartyPythonModuleMapping
from pants.backend.python.goals import lockfile
from pants.backend.python.goals.lockfile import GeneratePythonLockfile
from pants.backend.python.subsystems.python_tool_base import PythonToolRequirementsBase
from pants.core.goals.generate_lockfiles import GenerateToolLockfileSentinel
from pants.engine.addresses import Addresses, UnparsedAddressInputs
from pants.engine.addresses import Address
from pants.engine.rules import Get, collect_rules, rule
from pants.engine.target import InjectDependenciesRequest, InjectedDependencies
from pants.engine.target import InjectDependenciesRequest, InjectedDependencies, WrappedTarget
from pants.engine.unions import UnionRule
from pants.option.option_types import BoolOption, TargetListOption
from pants.option.option_types import BoolOption
from pants.option.subsystem import Subsystem
from pants.util.docutil import doc_url, git_url

Expand All @@ -20,16 +25,6 @@ class PythonProtobufSubsystem(Subsystem):
options_scope = "python-protobuf"
help = f"Options related to the Protobuf Python backend.\n\nSee {doc_url('protobuf')}."

_runtime_dependencies = TargetListOption(
"--runtime-dependencies",
help=(
"A list of addresses to `python_requirement` targets for the runtime "
"dependencies needed for generated Python code to work. For example, "
"`['3rdparty/python:protobuf', '3rdparty/python:grpcio']`. These dependencies will "
"be automatically added to every `protobuf_sources` target"
),
)

mypy_plugin = BoolOption(
"--mypy-plugin",
default=False,
Expand All @@ -39,9 +34,19 @@ class PythonProtobufSubsystem(Subsystem):
),
)

@property
def runtime_dependencies(self) -> UnparsedAddressInputs:
return UnparsedAddressInputs(self._runtime_dependencies, owning_address=None)
infer_runtime_dependency = BoolOption(
"--infer-runtime-dependency",
default=True,
help=(
"If True, will add a dependency on a `python_requirement` target exposing the "
"`protobuf` module (usually from the `protobuf` requirement). If the `protobuf_source` "
"target sets `grpc=True`, will also add a dependency on the `python_requirement` "
"target exposing the `grpcio` module.\n\n"
"Unless this option is disabled, Pants will error if no relevant target is found or "
"if more than one is found which causes ambiguity."
),
advanced=True,
)


class PythonProtobufMypyPlugin(PythonToolRequirementsBase):
Expand Down Expand Up @@ -81,10 +86,40 @@ class InjectPythonProtobufDependencies(InjectDependenciesRequest):

@rule
async def inject_dependencies(
_: InjectPythonProtobufDependencies, python_protobuf: PythonProtobufSubsystem
request: InjectPythonProtobufDependencies,
python_protobuf: PythonProtobufSubsystem,
# TODO(#12946): Make this a lazy Get once possible.
module_mapping: ThirdPartyPythonModuleMapping,
) -> InjectedDependencies:
addresses = await Get(Addresses, UnparsedAddressInputs, python_protobuf.runtime_dependencies)
return InjectedDependencies(addresses)
if not python_protobuf.infer_runtime_dependency:
return InjectedDependencies()

result = [
find_python_runtime_library_or_raise_error(
module_mapping,
request.dependencies_field.address,
"google.protobuf",
recommended_requirement_name="protobuf",
recommended_requirement_url="https://pypi.org/project/protobuf/",
disable_inference_option=f"[{python_protobuf.options_scope}].infer_runtime_dependency",
)
]

wrapped_tgt = await Get(WrappedTarget, Address, request.dependencies_field.address)
if wrapped_tgt.target.get(ProtobufGrpcToggleField).value:
result.append(
find_python_runtime_library_or_raise_error(
module_mapping,
request.dependencies_field.address,
# Note that the library is called `grpcio`, but the module is `grpc`.
"grpc",
recommended_requirement_name="grpcio",
recommended_requirement_url="https://pypi.org/project/grpcio/",
disable_inference_option=f"[{python_protobuf.options_scope}].infer_runtime_dependency",
)
)

return InjectedDependencies(result)


def rules():
Expand Down
Original file line number Diff line number Diff line change
@@ -1,45 +1,62 @@
# Copyright 2020 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from pants.backend.codegen.protobuf import target_types
from pants.backend.codegen.protobuf.python import python_protobuf_subsystem
from pants.backend.codegen.protobuf.python.python_protobuf_subsystem import (
InjectPythonProtobufDependencies,
)
from pants.backend.codegen.protobuf.target_types import (
ProtobufDependenciesField,
ProtobufSourcesGeneratorTarget,
from pants.backend.codegen.protobuf.target_types import ProtobufSourcesGeneratorTarget
from pants.backend.codegen.utils import (
AmbiguousPythonCodegenRuntimeLibrary,
MissingPythonCodegenRuntimeLibrary,
)
from pants.core.target_types import GenericTarget
from pants.backend.python.dependency_inference import module_mapper
from pants.backend.python.target_types import PythonRequirementTarget
from pants.core.util_rules import stripped_source_files
from pants.engine.addresses import Address
from pants.engine.target import InjectedDependencies
from pants.testutil.rule_runner import QueryRule, RuleRunner
from pants.engine.target import Dependencies, InjectedDependencies
from pants.testutil.rule_runner import QueryRule, RuleRunner, engine_error


def test_inject_dependencies() -> None:
def test_find_protobuf_python_requirement() -> None:
rule_runner = RuleRunner(
rules=[
*python_protobuf_subsystem.rules(),
*target_types.rules(),
*module_mapper.rules(),
*stripped_source_files.rules(),
QueryRule(InjectedDependencies, (InjectPythonProtobufDependencies,)),
],
target_types=[ProtobufSourcesGeneratorTarget, GenericTarget],
target_types=[ProtobufSourcesGeneratorTarget, PythonRequirementTarget],
)
rule_runner.set_options(["--python-protobuf-runtime-dependencies=protos:injected_dep"])
# Note that injected deps can be any target type for `--python-protobuf-runtime-dependencies`.

rule_runner.write_files(
{
"protos/BUILD": "protobuf_sources()\ntarget(name='injected_dep')",
"protos/f.proto": "",
}
{"codegen/dir/f.proto": "", "codegen/dir/BUILD": "protobuf_sources(grpc=True)"}
)
proto_tgt = rule_runner.get_target(Address("codegen/dir", relative_file_path="f.proto"))
request = InjectPythonProtobufDependencies(proto_tgt[Dependencies])

def assert_injected(addr: Address) -> None:
tgt = rule_runner.get_target(addr)
injected = rule_runner.request(
InjectedDependencies, [InjectPythonProtobufDependencies(tgt[ProtobufDependenciesField])]
)
assert injected == InjectedDependencies([Address("protos", target_name="injected_dep")])
# Start with no relevant requirements.
with engine_error(MissingPythonCodegenRuntimeLibrary, contains="protobuf"):
rule_runner.request(InjectedDependencies, [request])
rule_runner.write_files({"proto1/BUILD": "python_requirement(requirements=['protobuf'])"})
with engine_error(MissingPythonCodegenRuntimeLibrary, contains="grpcio"):
rule_runner.request(InjectedDependencies, [request])

# If exactly one, match it.
rule_runner.write_files({"grpc1/BUILD": "python_requirement(requirements=['grpc'])"})
assert rule_runner.request(InjectedDependencies, [request]) == InjectedDependencies(
[Address("proto1"), Address("grpc1")]
)

assert_injected(Address("protos"))
assert_injected(Address("protos", relative_file_path="f.proto"))
# If multiple, error.
rule_runner.write_files({"grpc2/BUILD": "python_requirement(requirements=['grpc'])"})
with engine_error(
AmbiguousPythonCodegenRuntimeLibrary, contains="['grpc1:grpc1', 'grpc2:grpc2']"
):
rule_runner.request(InjectedDependencies, [request])
rule_runner.write_files({"proto2/BUILD": "python_requirement(requirements=['protobuf'])"})
with engine_error(
AmbiguousPythonCodegenRuntimeLibrary, contains="['proto1:proto1', 'proto2:proto2']"
):
rule_runner.request(InjectedDependencies, [request])
4 changes: 4 additions & 0 deletions src/python/pants/backend/codegen/protobuf/python/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
ProtobufSourceTarget,
)
from pants.backend.codegen.protobuf.target_types import rules as protobuf_target_rules
from pants.backend.python.dependency_inference import module_mapper
from pants.core.util_rules import stripped_source_files


def rules():
Expand All @@ -32,6 +34,8 @@ def rules():
*protobuf_tailor.rules(),
*export_codegen_goal.rules(),
*protobuf_target_rules(),
*module_mapper.rules(),
*stripped_source_files.rules(),
]


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
ProtobufSourcesGeneratorTarget,
)
from pants.backend.codegen.protobuf.target_types import rules as target_types_rules
from pants.backend.python.dependency_inference import module_mapper
from pants.core.util_rules import stripped_source_files
from pants.engine.addresses import Address
from pants.engine.target import GeneratedSources, HydratedSources, HydrateSourcesRequest
Expand Down Expand Up @@ -58,6 +59,7 @@ def rule_runner() -> RuleRunner:
*additional_fields.rules(),
*stripped_source_files.rules(),
*target_types_rules(),
*module_mapper.rules(),
QueryRule(HydratedSources, [HydrateSourcesRequest]),
QueryRule(GeneratedSources, [GeneratePythonFromProtobufRequest]),
],
Expand All @@ -74,7 +76,11 @@ def assert_files_generated(
mypy: bool = False,
extra_args: list[str] | None = None,
) -> None:
args = [f"--source-root-patterns={repr(source_roots)}", *(extra_args or ())]
args = [
f"--source-root-patterns={repr(source_roots)}",
"--no-python-protobuf-infer-runtime-dependency",
*(extra_args or ()),
]
if mypy:
args.append("--python-protobuf-mypy-plugin")
rule_runner.set_options(args, env_inherit={"PATH", "PYENV_ROOT", "HOME"})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
ThriftSourcesGeneratorTarget,
ThriftSourceTarget,
)
from pants.backend.python.dependency_inference import module_mapper
from pants.core.util_rules import stripped_source_files


def target_types():
Expand All @@ -21,4 +23,6 @@ def rules():
*apache_thrift_rules(),
*apache_thrift_python_rules(),
*python_thrift_module_mapper.rules(),
*module_mapper.rules(),
*stripped_source_files.rules(),
]
26 changes: 21 additions & 5 deletions src/python/pants/backend/codegen/thrift/apache/python/rules.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
# Copyright 2021 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from __future__ import annotations

from pants.backend.codegen.thrift.apache.python import subsystem
from pants.backend.codegen.thrift.apache.python.subsystem import ThriftPythonSubsystem
from pants.backend.codegen.thrift.apache.rules import (
GeneratedThriftSources,
GenerateThriftSourcesRequest,
)
from pants.backend.codegen.thrift.target_types import ThriftDependenciesField, ThriftSourceField
from pants.backend.codegen.utils import find_python_runtime_library_or_raise_error
from pants.backend.python.dependency_inference.module_mapper import ThirdPartyPythonModuleMapping
from pants.backend.python.target_types import PythonSourceField
from pants.engine.addresses import Addresses, UnparsedAddressInputs
from pants.engine.fs import AddPrefix, Digest, Snapshot
from pants.engine.internals.selectors import Get
from pants.engine.rules import collect_rules, rule
Expand Down Expand Up @@ -63,11 +66,24 @@ class InjectApacheThriftPythonDependencies(InjectDependenciesRequest):


@rule
async def inject_apache_thrift_java_dependencies(
_: InjectApacheThriftPythonDependencies, thrift_python: ThriftPythonSubsystem
async def find_apache_thrift_python_requirement(
request: InjectApacheThriftPythonDependencies,
thrift_python: ThriftPythonSubsystem,
# TODO(#12946): Make this a lazy Get once possible.
module_mapping: ThirdPartyPythonModuleMapping,
) -> InjectedDependencies:
addresses = await Get(Addresses, UnparsedAddressInputs, thrift_python.runtime_dependencies)
return InjectedDependencies(addresses)
if not thrift_python.infer_runtime_dependency:
return InjectedDependencies()

addr = find_python_runtime_library_or_raise_error(
module_mapping,
request.dependencies_field.address,
"thrift",
recommended_requirement_name="thrift",
recommended_requirement_url="https://pypi.org/project/thrift/",
disable_inference_option=f"[{thrift_python.options_scope}].infer_runtime_dependency",
)
return InjectedDependencies([addr])


def rules():
Expand Down

0 comments on commit 5344b97

Please sign in to comment.