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

The beginnings of a Django plugin. #18173

Merged
merged 6 commits into from
Feb 9, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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 @@ -75,23 +75,31 @@ class ParserScript:
env: FrozenDict[str, str]


_scripts_module = "pants.backend.python.dependency_inference.scripts"
_scripts_package = "pants.backend.python.dependency_inference.scripts"


@rule_helper
async def _get_script_digest(relpaths: Iterable[str]) -> Digest:
scripts = [read_resource(_scripts_module, relpath) for relpath in relpaths]
async def _get_scripts_digest(scripts_package: str, filenames: Iterable[str]) -> Digest:
scripts = [read_resource(scripts_package, filename) for filename in filenames]
assert all(script is not None for script in scripts)
path_prefix = _scripts_module.replace(".", os.path.sep)
digest = await Get(
Digest,
CreateDigest(
[
FileContent(os.path.join(path_prefix, relpath), script)
for relpath, script in zip(relpaths, scripts)
]
),
)
path_prefix = scripts_package.replace(".", os.path.sep)
contents = [
FileContent(os.path.join(path_prefix, relpath), script)
for relpath, script in zip(filenames, scripts)
]

# Python 2 requires all the intermediate __init__.py to exist in the sandbox.
package = scripts_package
while package:
contents.append(
FileContent(
os.path.join(package.replace(".", os.path.sep), "__init__.py"),
read_resource(package, "__init__.py"),
)
)
package = package.rpartition(".")[0]

digest = await Get(Digest, CreateDigest(contents))
return digest


Expand All @@ -102,29 +110,15 @@ async def get_parser_script(union_membership: UnionMembership) -> ParserScript:
Get(PythonDependencyVisitor, PythonDependencyVisitorRequest, dvrt())
for dvrt in dep_visitor_request_types
)
utils = await _get_script_digest(
utils = await _get_scripts_digest(
_scripts_package,
[
"dependency_visitor_base.py",
"main.py",
]
],
)

# Python 2 requires all the intermediate __init__.py to exist in the sandbox.
init_contents = []
module = _scripts_module
while module:
init_contents.append(
FileContent(
os.path.join(module.replace(".", os.path.sep), "__init__.py"),
read_resource(module, "__init__.py"),
)
)
module = module.rpartition(".")[0]
init_scaffolding = await Get(Digest, CreateDigest(init_contents))

digest = await Get(
Digest, MergeDigests([utils, init_scaffolding, *(dv.digest for dv in dep_visitors)])
)
digest = await Get(Digest, MergeDigests([utils, *(dv.digest for dv in dep_visitors)]))
env = {
"VISITOR_CLASSNAMES": "|".join(dv.classname for dv in dep_visitors),
"PYTHONPATH": ".",
Expand Down Expand Up @@ -154,10 +148,10 @@ class GeneralPythonDependencyVisitorRequest(PythonDependencyVisitorRequest):
@rule
async def general_parser_script(
python_infer_subsystem: PythonInferSubsystem,
request: GeneralPythonDependencyVisitorRequest,
_: GeneralPythonDependencyVisitorRequest,
) -> PythonDependencyVisitor:
script_digest = await _get_script_digest(["general_dependency_visitor.py"])
classname = f"{_scripts_module}.general_dependency_visitor.GeneralDependencyVisitor"
script_digest = await _get_scripts_digest(_scripts_package, ["general_dependency_visitor.py"])
classname = f"{_scripts_package}.general_dependency_visitor.GeneralDependencyVisitor"
return PythonDependencyVisitor(
digest=script_digest,
classname=classname,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,17 @@ def assert_deps_parsed(
rule_runner: RuleRunner,
content: str,
*,
expected_imports: dict[str, ImpInfo] = {},
expected_assets: list[str] = [],
expected_imports: dict[str, ImpInfo] | None = None,
expected_assets: list[str] | None = None,
filename: str = "project/foo.py",
constraints: str = ">=3.6",
string_imports: bool = True,
string_imports_min_dots: int = 2,
assets: bool = True,
assets_min_slashes: int = 1,
) -> None:
expected_imports = expected_imports or {}
expected_assets = expected_assets or []
rule_runner.set_options(
[
f"--python-infer-string-imports={string_imports}",
Expand Down
7 changes: 7 additions & 0 deletions src/python/pants/backend/python/framework/django/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Copyright 2023 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).


python_sources(dependencies=["src/python/pants/backend/python/framework/django/scripts"])

python_tests(name="tests")
Empty file.
49 changes: 49 additions & 0 deletions src/python/pants/backend/python/framework/django/rules.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# Copyright 2023 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from dataclasses import dataclass

from pants.backend.python.dependency_inference.parse_python_dependencies import (
PythonDependencyVisitor,
PythonDependencyVisitorRequest,
_get_scripts_digest,
)
from pants.backend.python.target_types import PythonSourceField
from pants.engine.rules import collect_rules, rule
from pants.engine.target import FieldSet
from pants.engine.unions import UnionRule
from pants.util.frozendict import FrozenDict


@dataclass(frozen=True)
class DjangoDependencyVisitorRequest(PythonDependencyVisitorRequest):
pass


_scripts_package = "pants.backend.python.framework.django.scripts"


@rule
async def general_parser_script(
_: DjangoDependencyVisitorRequest,
) -> PythonDependencyVisitor:
digest = await _get_scripts_digest(_scripts_package, ["django_dependency_visitor.py"])
return PythonDependencyVisitor(
digest=digest,
classname=f"{_scripts_package}.django_dependency_visitor.DjangoDependencyVisitor",
env=FrozenDict({}),
)


@dataclass(frozen=True)
class DjangoMigrationDependenciesInferenceFieldSet(FieldSet):
required_fields = (PythonSourceField,)

source: PythonSourceField


def rules():
return [
UnionRule(PythonDependencyVisitorRequest, DjangoDependencyVisitorRequest),
*collect_rules(),
]
125 changes: 125 additions & 0 deletions src/python/pants/backend/python/framework/django/rules_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
# Copyright 2023 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from __future__ import annotations

from textwrap import dedent

import pytest

from pants.backend.python.dependency_inference import parse_python_dependencies
from pants.backend.python.dependency_inference.parse_python_dependencies import (
ParsedPythonDependencies,
)
from pants.backend.python.dependency_inference.parse_python_dependencies import (
ParsedPythonImportInfo as ImpInfo,
)
from pants.backend.python.dependency_inference.parse_python_dependencies import (
ParsePythonDependenciesRequest,
)
from pants.backend.python.framework.django import rules as django_rules
from pants.backend.python.target_types import PythonSourceField, PythonSourceTarget
from pants.backend.python.util_rules import pex
from pants.backend.python.util_rules.interpreter_constraints import InterpreterConstraints
from pants.core.util_rules import stripped_source_files
from pants.engine.addresses import Address
from pants.testutil.python_interpreter_selection import (
skip_unless_python27_present,
skip_unless_python37_present,
skip_unless_python38_present,
skip_unless_python39_present,
)
from pants.testutil.rule_runner import QueryRule, RuleRunner


@pytest.fixture
def rule_runner() -> RuleRunner:
return RuleRunner(
rules=[
*parse_python_dependencies.rules(),
*stripped_source_files.rules(),
*pex.rules(),
*django_rules.rules(),
QueryRule(ParsedPythonDependencies, [ParsePythonDependenciesRequest]),
QueryRule(ParsedPythonDependencies, [ParsePythonDependenciesRequest]),
],
target_types=[PythonSourceTarget],
)


def assert_deps_parsed(
rule_runner: RuleRunner,
content: str,
*,
expected_imports: dict[str, ImpInfo] | None = None,
expected_assets: list[str] | None = None,
filename: str = "app0/migrations/0001_initial.py",
constraints: str = ">=3.6",
) -> None:
expected_imports = expected_imports or {}
expected_assets = expected_assets or []
rule_runner.set_options(
[
"--no-python-infer-string-imports",
"--no-python-infer-assets",
],
env_inherit={"PATH", "PYENV_ROOT", "HOME"},
)
rule_runner.write_files(
{
"BUILD": f"python_source(name='t', source={repr(filename)})",
filename: content,
}
)
tgt = rule_runner.get_target(Address("", target_name="t"))
result = rule_runner.request(
ParsedPythonDependencies,
[
ParsePythonDependenciesRequest(
tgt[PythonSourceField],
InterpreterConstraints([constraints]),
)
],
)
assert dict(result.imports) == expected_imports
assert list(result.assets) == sorted(expected_assets)


def do_test_migration_dependencies(rule_runner: RuleRunner, constraints: str) -> None:
content = dedent(
"""\
class Migration(migrations.Migration):
dependencies = [("app1", "0012_some_migration"), ("app2", "0042_some_other_migration")]

operations = []
"""
)
assert_deps_parsed(
rule_runner,
content,
expected_imports={
"app1.migrations.0012_some_migration": ImpInfo(lineno=2, weak=True),
"app2.migrations.0042_some_other_migration": ImpInfo(lineno=2, weak=True),
},
constraints=constraints,
)


@skip_unless_python27_present
def test_works_with_python2(rule_runner: RuleRunner) -> None:
do_test_migration_dependencies(rule_runner, constraints="CPython==2.7.*")


@skip_unless_python37_present
def test_works_with_python37(rule_runner: RuleRunner) -> None:
do_test_migration_dependencies(rule_runner, constraints="CPython==3.7.*")


@skip_unless_python38_present
def test_works_with_python38(rule_runner: RuleRunner) -> None:
do_test_migration_dependencies(rule_runner, constraints="CPython==3.8.*")


@skip_unless_python39_present
def test_works_with_python39(rule_runner: RuleRunner) -> None:
do_test_migration_dependencies(rule_runner, constraints="CPython==3.9.*")
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Copyright 2023 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

python_sources()
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# Copyright 2023 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).
# -*- coding: utf-8 -*-

# NB: This must be compatible with Python 2.7 and 3.5+.
# NB: An easy way to debug this is to invoke it directly on a file.
# E.g.
# $ PYTHONPATH=src/python VISITOR_CLASSNAMES=pants.backend.python.framework.django.scripts\
# .django_dependency_visitor.DjangoDependencyVisitor \
# python src/python/pants/backend/python/dependency_inference/scripts/main.py FILE

from __future__ import print_function, unicode_literals

import ast

from pants.backend.python.dependency_inference.scripts.dependency_visitor_base import (
DependencyVisitorBase,
)


class DjangoDependencyVisitor(DependencyVisitorBase):
def __init__(self, *args, **kwargs):
super(DjangoDependencyVisitor, self).__init__(*args, **kwargs)
self._in_migration = False

def visit_ClassDef(self, node):
# Detect `class Migration(migrations.Migration):`
if (
node.name == "Migration"
and len(node.bases) > 0
and node.bases[0].value.id == "migrations"
and node.bases[0].attr == "Migration"
):
self._in_migration = True
self.generic_visit(node)
self._in_migration = False

def visit_Assign(self, node):
if self._in_migration and len(node.targets) > 0 and node.targets[0].id == "dependencies":
if isinstance(node.value, (ast.Tuple, ast.List)):
for elt in node.value.elts:
if isinstance(elt, (ast.Tuple, ast.List)) and len(elt.elts) == 2:
app = self.maybe_str(elt.elts[0])
migration = self.maybe_str(elt.elts[1])
if app is not None and migration is not None:
module = "{}.migrations.{}".format(app, migration)
Copy link
Member

Choose a reason for hiding this comment

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

This assumes that all apps are at a source root, which may not be the case.

Example (from a real project):

PANTS_SOURCE=../../kaos/pants pants dependencies src/python/org/proj/app1/migrations/0050_auto_20230117_1052.py
10:04:27.73 [INFO] View on BuildSense: https://app.toolchain.com/organizations/redacted/
10:04:29.40 [INFO] Parsed src/python/org/proj/app1/migrations/0050_auto_20230117_1052.py: {'imports': {'app2.migrations.0013_redacted': {'lineno': 9, 'weak': True}, 'app1.migrations.0049_redacted': {'lineno': 9, 'weak': True}, 'django.db.migrations': {'lineno': 3, 'weak': False}, 'django.db.models': {'lineno': 3, 'weak': False}, 'django.db.models.deletion': {'lineno': 4, 'weak': False}}, 'assets': []}
3rdparty/python#Django

Roots are:

build/scripts
src/python
tests/python

Copy link
Member

Choose a reason for hiding this comment

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

I applied this diff to your PR in order to try it live:

diff --git a/src/python/pants/backend/python/dependency_inference/parse_python_dependencies.py b/src/python/pants/backend/python/dependency_inference/parse_python_dependencies.py
index e317121dc..f7165cf75 100644
--- a/src/python/pants/backend/python/dependency_inference/parse_python_dependencies.py
+++ b/src/python/pants/backend/python/dependency_inference/parse_python_dependencies.py
@@ -1,6 +1,7 @@
 # Copyright 2020 Pants project contributors (see CONTRIBUTORS.md).
 # Licensed under the Apache License, Version 2.0 (see LICENSE).

+import logging
 import json
 import os
 from dataclasses import dataclass
@@ -202,6 +203,8 @@ async def parse_python_dependencies(
     process_output = process_result.stdout.decode("utf8") or "{}"
     output = json.loads(process_output)

+    logging.info(f"Parsed {request.source.address}: {output}")
+
     return ParsedPythonDependencies(
         imports=ParsedPythonImports(
             (key, ParsedPythonImportInfo(**val)) for key, val in output.get("imports", {}).items()
diff --git a/src/python/pants/backend/python/register.py b/src/python/pants/backend/python/register.py
index 36fd13714..ee69d506d 100644
--- a/src/python/pants/backend/python/register.py
+++ b/src/python/pants/backend/python/register.py
@@ -54,6 +54,9 @@ from pants.build_graph.build_file_aliases import BuildFileAliases
 from pants.core.target_types import TargetGeneratorSourcesHelperTarget
 from pants.core.util_rules.wrap_source import wrap_source_rule_and_target

+from pants.backend.python.framework.django import rules as django
+
+
 wrap_python = wrap_source_rule_and_target(PythonSourceField, "python_sources")


@@ -63,6 +66,7 @@ def build_file_aliases():

 def rules():
     return (
+        *django.rules(),
         *target_types_rules.rules(),
         # Subsystems
         *coverage_py.rules(),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, there is a problem, but it's not with source roots. I was absent-mindedly assuming that the migration deps use the dotted module path (e.g., django.contrib.auth), which is from the source root, so the source root doesn't matter.

But actually it's the Django app "label", which is typically just the last component of the module path, e.g., auth but I think can be overridden in the AppConfig class (I will experiment).

So it's not the source roots that are the issue, but that we need a map from app label to app module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't forget that this extraction phase returns dependency modules, not files, so it does not know or need to know about source roots.

Copy link
Member

Choose a reason for hiding this comment

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

But actually it's the Django app "label", which is typically just the last component of the module path, e.g., auth but I think can be overridden in the AppConfig class (I will experiment).

Ah, true.

Don't forget that this extraction phase returns dependency modules, not files, so it does not know or need to know about source roots.

Well, I guess it doesn't need to know about source roots or file paths, but the dependency module won't work if it isn't complete from a source root perspective.

With a app label to module mapping this will work just fine, so I think that is the correct way forward :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I guess it doesn't need to know about source roots or file paths, but the dependency module won't work if it isn't complete from a source root perspective.

That's true in general, but the dep inference machinery takes care of this already. The responsibility of this extraction phase is just to say "this file depends on this module". Other code takes care of figuring out who provides the module, which is where source roots come in.

Copy link
Member

@kaos kaos Feb 5, 2023

Choose a reason for hiding this comment

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

Yea, that's what I mean. If the extraction phase says "we have a dependency on module foo.bar" but we only have some.foo.bar (due to how the source roots are set up) then the extraction phase needs to provide some.foo.bar for it to work, right? (as there's no one providing just foo.bar)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The extraction has to get the module paths right, of course, but this should not require it to know about source roots, since import statements (or any other runtime loading/dependency mechanisms) don't know about them.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. That's exactly what I was trying to say with

I guess it doesn't need to know about source roots or file paths, but the dependency module won't work if it isn't complete from a source root perspective.

Copy link
Member

Choose a reason for hiding this comment

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

why is human language so hard to use for communication? :P

self.add_weak_import(module, node.lineno)
self.generic_visit(node)