Support Python requirement target addrs in tool requirements. #19014

benjyw · 2023-05-16T06:27:18Z

Currently users can enumerate requirements for a Python tool, which will cause
Pants to use that requirement subset from the tool's lockfile. This supports resolving
a tool from a larger lockfile, when that makes sense. If no requirements are provided
the entire lockfile is used.

Two examples of when resolving a tool from a subset of a larger lockfile is useful:

Resolving pytest as a tool at the same version as its runtime library without
specifying it in two places.
Having a single lockfile for all tools, and user code, to be consumed by VSCode.

However, the value of this requirements option is typically redundant with the
input requirements used to generate the lockfile.

This change allows requirements to specify addresses (or address specs).
This can be used to avoid the redundancy: You can point to some
python_requirement targets that are a subset of all those used to generate
the lockfile.

The implementation turns out to be reasonably neat, as it just pushes
some code that turns addresses into requirement strings to slightly
later in the pex creation logic, and actually allowed us to reduce some
redundancy and trim some code.

This addresses #18816.

benjyw · 2023-05-16T06:35:10Z

src/python/pants/backend/python/subsystems/python_tool_base.py

@@ -344,7 +352,7 @@ def to_pex_request(
        return PexRequest(
            output_filename=f"{self.options_scope.replace('-', '_')}.pex",
            internal_only=True,
-            requirements=self.pex_requirements(extra_requirements=extra_requirements),
+            requirements=requirements,


This call to self.pex_requirements() was redundant even before any of the other changes in this PR, so this is just an obvious cleanup.

benjyw · 2023-05-16T06:38:26Z

src/python/pants/backend/python/util_rules/pex_from_targets.py

@@ -333,13 +333,8 @@ class _PexRequirementsRequest:
 async def determine_requirement_strings_in_closure(
    request: _PexRequirementsRequest, global_requirement_constraints: GlobalRequirementConstraints
 ) -> PexRequirements:
-    transitive_targets = await Get(TransitiveTargets, TransitiveTargetsRequest(request.addresses))


This computation is now pushed to later.

benjyw · 2023-05-16T06:39:24Z

src/python/pants/backend/python/util_rules/pex_requirements.py

    @classmethod
    def req_strings_from_requirement_fields(
        cls, fields: Iterable[PythonRequirementsField]
    ) -> FrozenOrderedSet[str]:
        """A convenience when you only need the raw requirement strings from fields and don't need
        to consider things like constraints or resolves."""
-        return cls.create_from_requirement_fields(fields, constraints_strings=()).req_strings
+        return FrozenOrderedSet(


This should always have been implemented this way. It seems like a silly oversight that it was implemented using a transient object returned by create_from_requirement_fields. Fixing this allows us to delete create_from_requirement_fields entirely.

benjyw · 2023-05-16T06:42:03Z

Note for reviewers: I wanted to get feedback on the approach. Am working on adding some tests.

benjyw · 2023-05-16T16:08:10Z

src/python/pants/backend/python/util_rules/pex.py

+            addrs.append(req_str_or_addr)
+        else:
+            assert isinstance(req_str_or_addr, str)
+            # A local or VCS requirement will have an ampersand in it, and any other


Reviewers, do you concur?

I think so. But you could also explicitly recommend prefixing an address with // in case of ambiguity, and skip the @ check (note: not ampersand).

Whoops, I meant asperand.

We would have to require prefixing, we can't just recommend it. Otherwise we'll see vcs/local requirements as addresses with no escape hatch.

And yeah, I think we should require the // prefix, because apparently wheel paths can be treated as requirements.

stuhood

Looks good, thanks!

stuhood · 2023-05-16T16:43:32Z

src/python/pants/backend/python/util_rules/pex.py

+            addrs.append(req_str_or_addr)
+        else:
+            assert isinstance(req_str_or_addr, str)
+            # A local or VCS requirement will have an ampersand in it, and any other


I think so. But you could also explicitly recommend prefixing an address with // in case of ambiguity, and skip the @ check (note: not ampersand).

stuhood · 2023-05-16T16:54:56Z

src/python/pants/backend/python/util_rules/pex.py

+                # TODO: Plumb the origin through in PexRequirements?
+                description_of_origin="User-specified requirements",


That would probably be good, yea... maybe as an optional field (initially), at the very least? I would be less concerned about this, but particularly since heuristics are being used to distinguish addresses from requirements...

BTW there may be a future cleanup (post removing tool lockfiles) where this only takes addresses, I'd have to dive more deeply to be sure.

Switched to requiring the // prefix, for extra security (really I just check for a single /).

Also added descriptions of origin.

Also add documentation, and apply this in the Pants repo.

PTAL

stuhood

Thanks!

stuhood · 2023-05-17T02:12:28Z

src/python/pants/backend/python/util_rules/pex.py

+            # Require a `//` prefix, to distinguish address specs from
+            # local or VCS requirements.
+            if req_str_or_addr.startswith(os.path.sep):


This should probably be double //, otherwise it's ambiguous with what someone might try as an absolute file path.

An absolute file path is definitely not any kind of valid requirement, but sure.

…uild#19014) Currently users can enumerate `requirements` for a Python tool, which will cause Pants to use that requirement subset from the tool's lockfile. This supports resolving a tool from a larger lockfile, when that makes sense. If no requirements are provided the entire lockfile is used. Two examples of when resolving a tool from a subset of a larger lockfile is useful: 1. Resolving `pytest` as a tool at the same version as its runtime library without specifying it in two places. 2. Having a single lockfile for all tools, and user code, to be consumed by VSCode. However, the value of this `requirements` option is typically redundant with the input requirements used to generate the lockfile. This change allows `requirements` to specify addresses (or address specs). This can be used to avoid the redundancy: You can point to some python_requirement targets that are a subset of all those used to generate the lockfile. The implementation turns out to be reasonably neat, as it just pushes some code that turns addresses into requirement strings to slightly later in the pex creation logic, and actually allowed us to reduce some redundancy and trim some code. This fixes pantsbuild#18816.

…-pick of #19014) (#19023) Currently users can enumerate `requirements` for a Python tool, which will cause Pants to use that requirement subset from the tool's lockfile. This supports resolving a tool from a larger lockfile, when that makes sense. If no requirements are provided the entire lockfile is used. Two examples of when resolving a tool from a subset of a larger lockfile is useful: 1. Resolving `pytest` as a tool at the same version as its runtime library without specifying it in two places. 2. Having a single lockfile for all tools, and user code, to be consumed by VSCode. However, the value of this `requirements` option is typically redundant with the input requirements used to generate the lockfile. This change allows `requirements` to specify addresses (or address specs). This can be used to avoid the redundancy: You can point to some python_requirement targets that are a subset of all those used to generate the lockfile. The implementation turns out to be reasonably neat, as it just pushes some code that turns addresses into requirement strings to slightly later in the pex creation logic, and actually allowed us to reduce some redundancy and trim some code. This addresses #18816.

Support Python requirement target addrs in tool requirements.

370d928

benjyw added the category:user api change label May 16, 2023

benjyw requested review from kaos and thejcannon as code owners May 16, 2023 06:27

benjyw commented May 16, 2023

View reviewed changes

benjyw requested review from stuhood and Eric-Arellano and removed request for kaos and thejcannon May 16, 2023 06:42

benjyw added 2 commits May 16, 2023 09:04

Fix test

94bca0d

Tweak comment

553613d

benjyw commented May 16, 2023

View reviewed changes

stuhood approved these changes May 16, 2023

View reviewed changes

benjyw added 5 commits May 16, 2023 16:15

Description of origin

8fa109c

Docs

f46af15

Document prefix

c7ab34e

Fix tests

5030304

Set requirements up in this repo

fae761b

stuhood approved these changes May 17, 2023

View reviewed changes

Check for double-slash

2646f16

benjyw added the needs-cherrypick label May 17, 2023

benjyw added this to the 2.16.x milestone May 17, 2023

benjyw merged commit 9d845f0 into pantsbuild:main May 17, 2023
17 checks passed

benjyw deleted the addrs_in_pex_requirements branch May 17, 2023 15:09

benjyw removed the needs-cherrypick label May 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Python requirement target addrs in tool requirements. #19014

Support Python requirement target addrs in tool requirements. #19014

benjyw commented May 16, 2023 •

edited

benjyw May 16, 2023

benjyw May 16, 2023

benjyw May 16, 2023 •

edited

benjyw commented May 16, 2023

benjyw May 16, 2023

stuhood May 16, 2023

benjyw May 16, 2023

benjyw May 16, 2023

benjyw May 16, 2023

stuhood left a comment

stuhood May 16, 2023

stuhood May 16, 2023

benjyw May 16, 2023

benjyw May 17, 2023

stuhood left a comment

stuhood May 17, 2023

benjyw May 17, 2023

		# TODO: Plumb the origin through in PexRequirements?
		description_of_origin="User-specified requirements",

Support Python requirement target addrs in tool requirements. #19014

Support Python requirement target addrs in tool requirements. #19014

Conversation

benjyw commented May 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjyw May 16, 2023 • edited

Choose a reason for hiding this comment

benjyw commented May 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjyw commented May 16, 2023 •

edited

benjyw May 16, 2023 •

edited