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

Conversation

benjyw
Copy link
Contributor

@benjyw benjyw commented Feb 5, 2023

Currently does two things:

  1. Detects Django apps, via their apps.py
  2. infers deps from Django migrations to other migrations,
    based on the dependencies field in the Migration classes.

Future changes will add more Django-related functionality.

Currently does dep inference between Django migrations.

Future changes will add more Django-related functionality.
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

@g-cassie
Copy link
Contributor

g-cassie commented Feb 6, 2023

Unfortunately my Pants chops aren't strong enough to comment substantively on the code. The premise you describe sounds reasonable. Is there something that infers the dependency between regular app code and migrations? I think every model declaration needs to infer a dependency on its migration files.

@benjyw
Copy link
Contributor Author

benjyw commented Feb 6, 2023

That will come in the future, but is an open question. For example, do you want to deploy migrations everywhere you deploy your models?

@benjyw benjyw requested a review from kaos February 7, 2023 01:46
@benjyw
Copy link
Contributor Author

benjyw commented Feb 7, 2023

OK, this now adds detection of Django apps, for the purpose of mapping short app labels to the full app package path.

# complicated: we wouldn't know which settings.py to use, or whether it's safe to run Django
# against that settings.py. Instead, we do this statically via parsing the apps.py file.
#
# NB: Legacy Django apps may not have an app.py, in which case the label is assumed to be
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# NB: Legacy Django apps may not have an app.py, in which case the label is assumed to be
# NB: Legacy Django apps may not have an apps.py, in which case the label is assumed to be

@benjyw benjyw merged commit c2d651b into pantsbuild:main Feb 9, 2023
@benjyw benjyw deleted the django_plugin3 branch February 9, 2023 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants