diff --git a/src/python/pants/backend/python/dependency_inference/rules.py b/src/python/pants/backend/python/dependency_inference/rules.py index 6ac4605c88c..4f5336053a4 100644 --- a/src/python/pants/backend/python/dependency_inference/rules.py +++ b/src/python/pants/backend/python/dependency_inference/rules.py @@ -5,9 +5,10 @@ import itertools import logging +from collections import defaultdict from enum import Enum from pathlib import PurePath -from typing import Iterable, Iterator, cast +from typing import DefaultDict, Iterable, Iterator, cast from pants.backend.python.dependency_inference import module_mapper, parse_python_dependencies from pants.backend.python.dependency_inference.default_unowned_dependencies import ( @@ -16,6 +17,7 @@ from pants.backend.python.dependency_inference.module_mapper import ( PythonModuleOwners, PythonModuleOwnersRequest, + ResolveName, ) from pants.backend.python.dependency_inference.parse_python_dependencies import ( ParsedPythonAssetPaths, @@ -35,15 +37,16 @@ from pants.core import target_types from pants.core.target_types import AllAssetTargets, AllAssetTargetsByPath, AllAssetTargetsRequest from pants.core.util_rules import stripped_source_files -from pants.engine.addresses import Address +from pants.engine.addresses import Address, Addresses from pants.engine.internals.graph import Owners, OwnersRequest -from pants.engine.rules import Get, MultiGet, SubsystemRule, rule +from pants.engine.rules import Get, MultiGet, SubsystemRule, rule, rule_helper from pants.engine.target import ( Dependencies, DependenciesRequest, ExplicitlyProvidedDependencies, InferDependenciesRequest, InferredDependencies, + Targets, WrappedTarget, ) from pants.engine.unions import UnionRule @@ -75,7 +78,14 @@ class PythonInferSubsystem(Subsystem): imports = BoolOption( "--imports", default=True, - help="Infer a target's imported dependencies by parsing import statements from sources.", + help=softwrap( + """ + Infer a target's imported dependencies by parsing import statements from sources. + + To ignore a false positive, you can either put `# pants: no-infer-dep` on the line of + the import or put `!{bad_address}` in the `dependencies` field of your target. + """ + ), ) string_imports = BoolOption( "--string-imports", @@ -84,6 +94,7 @@ class PythonInferSubsystem(Subsystem): """ Infer a target's dependencies based on strings that look like dynamic dependencies, such as Django settings files expressing dependencies as strings. + To ignore any false positives, put `!{bad_address}` in the `dependencies` field of your target. """ @@ -163,7 +174,19 @@ class PythonInferSubsystem(Subsystem): unowned_dependency_behavior = EnumOption( "--unowned-dependency-behavior", default=UnownedDependencyUsage.DoNothing, - help="How to handle inferred dependencies that don't have any owner.", + help=softwrap( + """ + How to handle imports that don't have an inferrable owner. + + Usually when an import cannot be inferred, it represents an issue like Pants not being + properly configured, e.g. targets not set up. Often, missing dependencies will result + in confusing runtime errors like `ModuleNotFoundError`, so this option can be helpful + to error more eagerly. + + To ignore any false positives, either add `# pants: no-infer-dep` to the line of the + import or put the import inside a `try: except ImportError:` block. + """ + ), ) @@ -242,33 +265,78 @@ def _get_imports_info( return inferred_deps, unowned_imports -def _maybe_warn_unowned( +@rule_helper +async def _handle_unowned_imports( address: Address, file: str, unowned_dependency_behavior: UnownedDependencyUsage, + python_setup: PythonSetup, unowned_imports: Iterable[str], parsed_imports: ParsedPythonImports, + resolve: str, ) -> None: - if unowned_imports and unowned_dependency_behavior is not UnownedDependencyUsage.DoNothing: - unowned_imports_with_lines = [ - f"{module_name} ({file}:{parsed_imports[module_name].lineno})" - for module_name in sorted(unowned_imports) - ] - raise_error = unowned_dependency_behavior is UnownedDependencyUsage.RaiseError - log = logger.error if raise_error else logger.warning - log( - f"The following imports in {address} have no owners:\n\n{bullet_list(unowned_imports_with_lines)}\n\n" - "If you are expecting this import to be provided by your own firstparty code, ensure that it is contained within a source root. " - "Otherwise if you are using a requirements file, consider adding the relevant package.\n" - "Otherwise consider declaring a `python_requirement_library` target, which can then be inferred.\n" - f"See {doc_url('python-third-party-dependencies')}" + if not unowned_imports or unowned_dependency_behavior is UnownedDependencyUsage.DoNothing: + return + + other_resolves_snippet = "" + if len(python_setup.resolves) > 1: + other_owners_from_other_resolves = await MultiGet( + Get(PythonModuleOwners, PythonModuleOwnersRequest(imported_module, resolve=None)) + for imported_module in unowned_imports + ) + other_owners_as_targets = await MultiGet( + Get(Targets, Addresses(owners.unambiguous + owners.ambiguous)) + for owners in other_owners_from_other_resolves ) - if raise_error: - raise UnownedDependencyError( - "One or more unowned dependencies detected. Check logs for more details." + imports_to_other_owners: DefaultDict[str, list[tuple[Address, ResolveName]]] = defaultdict( + list + ) + for imported_module, targets in zip(unowned_imports, other_owners_as_targets): + for t in targets: + other_owner_resolve = t[PythonResolveField].normalized_value(python_setup) + if other_owner_resolve != resolve: + imports_to_other_owners[imported_module].append( + (t.address, other_owner_resolve) + ) + + if imports_to_other_owners: + other_resolves_lines = [] + for import_module, other_owners in sorted(imports_to_other_owners.items()): + owners_txt = ", ".join( + f"'{other_resolve}' from {addr}" for addr, other_resolve in sorted(other_owners) + ) + other_resolves_lines.append(f"{import_module}: {owners_txt}") + other_resolves_snippet = "\n\n" + softwrap( + f""" + These imports are not in the resolve used by the target (`{resolve}`), but they + were present in other resolves: + + {bullet_list(other_resolves_lines)}\n\n + """ ) + unowned_imports_with_lines = [ + f"{module_name} (line: {parsed_imports[module_name].lineno})" + for module_name in sorted(unowned_imports) + ] + + msg = softwrap( + f""" + Pants cannot infer owners for the following imports in the target {address}: + + {bullet_list(unowned_imports_with_lines)}{other_resolves_snippet} + + If you do not expect an import to be inferrable, add `# pants: no-infer-dep` to the + import line. Otherwise, see + {doc_url('troubleshooting#import-errors-and-missing-dependencies')} for common problems. + """ + ) + if unowned_dependency_behavior is UnownedDependencyUsage.LogWarning: + logger.warning(msg) + else: + raise UnownedDependencyError(msg) + @rule(desc="Inferring Python dependencies by analyzing source") async def infer_python_dependencies_via_source( @@ -304,8 +372,9 @@ async def infer_python_dependencies_via_source( ExplicitlyProvidedDependencies, DependenciesRequest(tgt[Dependencies]) ) + resolve = tgt[PythonResolveField].normalized_value(python_setup) + if parsed_imports: - resolve = tgt[PythonResolveField].normalized_value(python_setup) import_deps, unowned_imports = _get_imports_info( address=tgt.address, owners_per_import=await MultiGet( @@ -330,12 +399,14 @@ async def infer_python_dependencies_via_source( ) ) - _maybe_warn_unowned( + _ = await _handle_unowned_imports( tgt.address, request.sources_field.file_path, python_infer_subsystem.unowned_dependency_behavior, + python_setup, unowned_imports, parsed_imports, + resolve=resolve, ) return InferredDependencies(sorted(inferred_deps)) diff --git a/src/python/pants/backend/python/dependency_inference/rules_test.py b/src/python/pants/backend/python/dependency_inference/rules_test.py index a683a03c37f..4a245ba52b1 100644 --- a/src/python/pants/backend/python/dependency_inference/rules_test.py +++ b/src/python/pants/backend/python/dependency_inference/rules_test.py @@ -23,6 +23,7 @@ PythonRequirementTarget, PythonSourceField, PythonSourcesGeneratorTarget, + PythonSourceTarget, PythonTestsGeneratorTarget, PythonTestUtilsGeneratorTarget, ) @@ -30,10 +31,11 @@ from pants.core.target_types import FilesGeneratorTarget, ResourcesGeneratorTarget from pants.core.target_types import rules as core_target_types_rules from pants.engine.addresses import Address -from pants.engine.internals.scheduler import ExecutionError +from pants.engine.internals.parametrize import Parametrize from pants.engine.rules import SubsystemRule from pants.engine.target import InferredDependencies -from pants.testutil.rule_runner import QueryRule, RuleRunner +from pants.testutil.rule_runner import PYTHON_BOOTSTRAP_ENV, QueryRule, RuleRunner, engine_error +from pants.util.strutil import softwrap def test_infer_python_imports(caplog) -> None: @@ -415,8 +417,9 @@ def run_dep_inference(address: Address) -> InferredDependencies: ) -def test_infer_python_strict(caplog) -> None: - rule_runner = RuleRunner( +@pytest.fixture +def imports_rule_runner() -> RuleRunner: + return RuleRunner( rules=[ *import_rules(), *target_types_rules.rules(), @@ -425,13 +428,17 @@ def test_infer_python_strict(caplog) -> None: QueryRule(InferredDependencies, [InferPythonImportDependencies]), ], target_types=[ + PythonSourceTarget, PythonSourcesGeneratorTarget, PythonRequirementTarget, PythonRequirementsTargetGenerator, ], + objects={"parametrize": Parametrize}, ) - rule_runner.write_files( + +def test_infer_python_strict(imports_rule_runner: RuleRunner, caplog) -> None: + imports_rule_runner.write_files( { "src/python/cheesey.py": dedent( """\ @@ -443,46 +450,38 @@ def test_infer_python_strict(caplog) -> None: } ) - def run_dep_inference( - address: Address, - unowned_dependency_behavior: str, - ) -> InferredDependencies: - rule_runner.set_options( + def run_dep_inference(unowned_dependency_behavior: str) -> InferredDependencies: + imports_rule_runner.set_options( [ f"--python-infer-unowned-dependency-behavior={unowned_dependency_behavior}", "--python-infer-string-imports", - "--source-root-patterns=src/python", ], - env_inherit={"PATH", "PYENV_ROOT", "HOME"}, + env_inherit=PYTHON_BOOTSTRAP_ENV, ) - target = rule_runner.get_target(address) - return rule_runner.request( + target = imports_rule_runner.get_target( + Address("src/python", relative_file_path="cheesey.py") + ) + return imports_rule_runner.request( InferredDependencies, [InferPythonImportDependencies(target[PythonSourceField])], ) - # First test with "warning" - run_dep_inference(Address("src/python", relative_file_path="cheesey.py"), "warning") + run_dep_inference("warning") assert len(caplog.records) == 1 - assert "The following imports in src/python/cheesey.py have no owners:" in caplog.text - assert " * venezuelan_beaver_cheese (src/python/cheesey.py:1)" in caplog.text + assert ( + "cannot infer owners for the following imports in the target src/python/cheesey.py:" + in caplog.text + ) + assert " * venezuelan_beaver_cheese (line: 1)" in caplog.text assert "japanese.sage.derby" not in caplog.text - # Now test with "error" - caplog.clear() - with pytest.raises(ExecutionError) as exc_info: - run_dep_inference(Address("src/python", relative_file_path="cheesey.py"), "error") - - assert isinstance(exc_info.value.wrapped_exceptions[0], UnownedDependencyError) - assert len(caplog.records) == 2 # one for the error being raised and one for our message - assert "The following imports in src/python/cheesey.py have no owners:" in caplog.text - assert " * venezuelan_beaver_cheese (src/python/cheesey.py:1)" in caplog.text - assert "japanese.sage.derby" not in caplog.text + with engine_error(UnownedDependencyError, contains="src/python/cheesey.py"): + run_dep_inference("error") caplog.clear() # All modes should be fine if the module is explicitly declared as a requirement - rule_runner.write_files( + imports_rule_runner.write_files( { "src/python/BUILD": dedent( """\ @@ -497,11 +496,11 @@ def run_dep_inference( } ) for mode in UnownedDependencyUsage: - run_dep_inference(Address("src/python", relative_file_path="cheesey.py"), mode.value) + run_dep_inference(mode.value) assert not caplog.records # All modes should be fine if the module is implictly found via requirements.txt - rule_runner.write_files( + imports_rule_runner.write_files( { "src/python/requirements.txt": "venezuelan_beaver_cheese==1.0.0", "src/python/BUILD": dedent( @@ -513,16 +512,71 @@ def run_dep_inference( } ) for mode in UnownedDependencyUsage: - run_dep_inference(Address("src/python", relative_file_path="cheesey.py"), mode.value) + run_dep_inference(mode.value) assert not caplog.records # All modes should be fine if the module is owned by a first party - rule_runner.write_files( + imports_rule_runner.write_files( { "src/python/venezuelan_beaver_cheese.py": "", "src/python/BUILD": "python_sources()", } ) for mode in UnownedDependencyUsage: - run_dep_inference(Address("src/python", relative_file_path="cheesey.py"), mode.value) + run_dep_inference(mode.value) assert not caplog.records + + +def test_infer_python_strict_multiple_resolves(imports_rule_runner: RuleRunner) -> None: + imports_rule_runner.write_files( + { + "project/base.py": "", + "project/utils.py": "", + "project/app.py": "import project.base\nimport project.utils", + "project/BUILD": dedent( + """\ + python_source( + name="base", + source="base.py", + resolve="a", + ) + + python_source( + name="utils", + source="utils.py", + resolve=parametrize("a", "b"), + ) + + python_source( + name="app", + source="app.py", + resolve="z", + ) + """ + ), + } + ) + + imports_rule_runner.set_options( + [ + "--python-infer-unowned-dependency-behavior=error", + "--python-enable-resolves", + "--python-resolves={'a': '', 'b': '', 'z': ''}", + ], + env_inherit=PYTHON_BOOTSTRAP_ENV, + ) + + tgt = imports_rule_runner.get_target(Address("project", target_name="app")) + expected_error = softwrap( + """ + These imports are not in the resolve used by the target (`z`), but they were present in + other resolves: + + * project.base: 'a' from project:base + * project.utils: 'a' from project:utils@resolve=a, 'b' from project:utils@resolve=b + """ + ) + with engine_error(UnownedDependencyError, contains=expected_error): + imports_rule_runner.request( + InferredDependencies, [InferPythonImportDependencies(tgt[PythonSourceField])] + )