Adds a pipenv_requirements macro

[jperkelens]

Problem

Would like to add support for managing python dependencies via Pipenv

Solution

Added a pipenv_requirements CAFO modeled after the existing python_requirements one. There seems to be plenty of opportunity for code reuse (perhaps by introducing a factory method pattern), that might make it easier to support other dep management tools (Poetry, for instance).

In order to support a personal usecase in which we also manage the dev environment with pipenv and thus maintain our lock file in the source root, I added an additional parameter that allows one to pass in a _python_requirements_file target as discussed in Slack, but more than willing to discuss alternatives here.

Result

requirement_library targets can now be generated from a Pipfile.lock file.

[coveralls]

Coverage remained the same at 0.0% when pulling c8583cb on jperkelens:pipenv-requirements into b7ad997 on pantsbuild:master.

[Eric-Arellano]

Excellent! Thank you for upstreaming this, JP!

Here's a diff to fix the formatting issues, along with some tiny tweaks:

diff --git a/src/python/pants/backend/python/pipenv_requirements.py b/src/python/pants/backend/python/pipenv_requirements.py
index 3cf062a68..f89e00aaf 100644
--- a/src/python/pants/backend/python/pipenv_requirements.py
+++ b/src/python/pants/backend/python/pipenv_requirements.py
@@ -1,18 +1,15 @@
-# Copyright 2014 Pants project contributors (see CONTRIBUTORS.md).
+# Copyright 2020 Pants project contributors (see CONTRIBUTORS.md).
 # Licensed under the Apache License, Version 2.0 (see LICENSE).
 
-from json import load
 import os
+from json import load
 from typing import Iterable, Mapping, Optional
 
 from pkg_resources import Requirement
 
 
 class PipenvRequirements:
-    """
-    Translates a Pipenv.lock file into an equivalent set `python_requirement_library`
-    targets.
-
+    """Translates a Pipenv.lock file into an equivalent set of `python_requirement_library` targets.
 
     You may also use the parameter `module_mapping` to teach Pants what modules each of your
     requirements provide. For any requirement unspecified, Pants will default to the name of the
@@ -43,19 +40,16 @@ class PipenvRequirements:
             For example, `{"ansicolors": ["colors"]}`. Any unspecified requirements will use the
             requirement name as the default module, e.g. "Django" will default to
             `modules=["django"]`.
-        :param pipfile_target: a `_python_requirements_file` target to provide for cache invalidation
-        if the requirements_relpath value is not in the current rel_path
+        :param pipfile_target: a `_python_requirements_file` target to provide for cache
+            invalidation if the requirements_relpath value is not in the current rel_path
         """
 
-        repository = None
-        lock_info = {}
-
         requirements_path = os.path.join(self._parse_context.rel_path, requirements_relpath)
         with open(requirements_path, "r") as fp:
             lock_info = load(fp)
             repos = lock_info.get("_meta", {}).get("sources", [])
             if len(repos) > 1:
-                raise ValueError("Only one repository source is supported")
+                raise ValueError("Only one repository source is supported.")
 
             repository = repos[0] if len(repos) == 1 else None
 
@@ -72,9 +66,9 @@ class PipenvRequirements:
 
         requirements = {**lock_info.get("default", {}), **lock_info.get("develop", {})}
         for req, info in requirements.items():
-            req_str = f"{req}{info.get('version','')}"
-            if info.get("markers"):
-                req_str += f";{info['markers']}"
+            req_str = f"{req}{info.get('version', '')}"
+            if "markers" in info:
+                req_str += f"; {info['markers']}"
 
             parsed_req = Requirement.parse(req_str)
 
diff --git a/src/python/pants/backend/python/register.py b/src/python/pants/backend/python/register.py
index 7840291cb..10954bf19 100644
--- a/src/python/pants/backend/python/register.py
+++ b/src/python/pants/backend/python/register.py
@@ -8,9 +8,9 @@ See https://www.pantsbuild.org/docs/python-backend.
 
 from pants.backend.python.dependency_inference import rules as dependency_inference_rules
 from pants.backend.python.pants_requirement import PantsRequirement
+from pants.backend.python.pipenv_requirements import PipenvRequirements
 from pants.backend.python.python_artifact import PythonArtifact
 from pants.backend.python.python_requirements import PythonRequirements
-from pants.backend.python.pipenv_requirements import PipenvRequirements
 from pants.backend.python.rules import (
     ancestor_files,
     coverage,

FYI you can run build-support/githooks/pre-commit to catch issues like this (https://www.pantsbuild.org/v2.0/docs/contributor-setup#step-2-set-up-git-hooks), but compiling the Rust engine is super slow, so for smaller PRs like this, it's not a big deal to not run this locally.

--

I agree that some of this should possibly be factored up, although not yet until we add support for Poetry. I opened #10655 to track Poetry. I added that ticket to the 2.0 plan.

--

Also, this macro doesn't fully support Pipenv like you'd hope. It will create a python_requirement_library for each dep, including transitive deps. But the user must go out of their way to add those deps to the depenencies field (or use dependency inference). Unless the user explicitly adds those transitive deps to the dependencies field, they won't be used.

Instead, what I think we want is to translate the direct dependencies from Pipfile into python_requirement_library targets, and convert Pipfile.lock into a constraints file that can be used by --python-setup-requirement-constraints. I added that to #10655 for both Poetry and Pipenv.

Even though this doesn't hook up the constraints.txt thing, this is a great starting point.

--

As part of #10655, we'll also want to add some tests. Lmk if you'd be interested in adding tests for this and I'd be happy to help get started, but it's also not blocking as we don't have a test for python_requirements to go off of (our bad).

I can add those tests in the follow up to 10655 and may ping you for what makes sense to test. For now, it sounds like this is all working as you'd expect in your repo?

[jperkelens]

This is currently working in my repo yes, happy to help contribute tests, its something i'm eager to get a handle on since we've not had a lot of success implementing tests that run properly in our custom plugins, so some experience here might be instructive. In regards to the constraints file, that makes a lot of sense! The current (v1) process can be quite slow when starting from a fresh cache, and im sure porting the transtitive deps to a contraints file will speed that process up.

[Eric-Arellano]

we've not had a lot of success implementing tests that run properly in our custom plugins

That makes a lot of sense. The infrastructure is there, but there's zero documentation, so it's not very helpful. I'm almost done with documenting plugins, and one of the last things I have left is https://www.pantsbuild.org/v2.0/docs/rules-api-testing. We'll have some examples in the example plugin repo, too: https://github.com/pantsbuild/example-plugin.

--

happy to help contribute tests

Awesome, thanks. Turns out we actually have a solid example with test_pants_requirement.py, which tests the macro pants_requirement(). See https://github.com/pantsbuild/pants/blob/master/tests/python/pants_test/backend/python/test_pants_requirement.py.

You'll want to create a new file src/python/pants/backend/python/pipenv_requirements_test.py. No need to update any BUILD file, as dependency inference handles everything.

You'll want to subclass TestBase, and register target_types and alias_groups like test_pants_requirement.py, but tweak it to use your new CAFO.

The main logic will be similar to assert_pants_requirement, outside of these differences:

• You'll want to call self.create_file("path/to/Pipfile.lock", "content") near the beginning. This is where you'll put the test's Pipfile.lock that gets read by the macro.
• pants_requirement expects only one single python_requirement_library, whereas your macro generates multiple. Rather than requesting self.request_single_product(WrappedTarget, Params(Address...), you'll want to call self.request_single_produt(Targets, Params(Addresses([addr1, addr2]), create_options_bootstrapper()). (Targets from pants.engine.target, Addresses from pants.engine.addresses). This will get you a sequence of Target objects.
• Once everything else is working, it'd be great to check that the dependency is correctly generated. You can use something like this:

from pants.backend.python.target_types import PythonRequirementsFile
from pants.engine.target import Dependencies, DependenciesRequest, Targets

for tgt in generated_targets:
   deps = self.request_single_product(
         Targets, Params(DependenciesRequest(tgt[Dependencies], create_options_bootstrapper()
   )
   assert len(deps) == 1
   assert isinstance(deps[0], PythonRequirementsFile)
   assert deps[0].address == Address("3rdparty/python", target_name="Pipfile.lock")

Some edge cases that would be helpful to check:

• Both default and develop are used.
• repos and url work properly.
• module_mapping works properly. If left off for that requirement, then we use the default. Otherwise, we use the given value.
• The pipfile_target override works.

To run the tests, run ./pants test $file like you normally would. As described at https://www.pantsbuild.org/v2.0/docs/contributor-setup#step-3-bootstrap-the-rust-engine, this will bootstrap Rust, which is really really slow the first time you run it. (Cost of us using Rust for the engine). You can speed it up by compiling in Debug mode, which will make the compile substantially faster but Pants slower to run. More info in those docs.

Let me know how I can help with any of this!

[jperkelens]

@Eric-Arellano making some good progress on the tests (modeled off of #10669), but seem to be running. into an assertion error that i can't make heads or tails of. I'm guessing it has to do with a reference vs value comparison as the sets look identical, do y'all have a test util for things like this or is there a different approach i need to take for these data structures?

[Eric-Arellano]

Thanks for working on tests!

[Eric-Arellano]

@jperkelens that almost certainly looks like a bug fix I made in #10669. We weren't sorting the fields for a target, so it was common for two targets to not be equal even if all their field values were the same. That's now fixed. I had the exact type of assertion error and was super confused.

[jperkelens]

@Eric-Arellano merging master back in seems to. have fixed it. All tests added, and all feedback addressed. Thanks!

[Eric-Arellano]

Thank you, JP! #10655 tracks the followup for how to get this to play nicely with constraints files. This is a great first step.

[Eric-Arellano]

Excellent. Thanks!

[benjyw]

Thanks for this contribution @jperkelens !

[benjyw]

Excited to have pipenv/poetry support!