Skip to content

Commit

Permalink
Add [python-setup].experimental_lockfile to consume lockfiles (#12316)
Browse files Browse the repository at this point in the history
This allows you to use the new lockfile format, generated by pip-tools via `./pants --tag=-lockfile_ignore lock ::` and #12300. 

A lockfile cannot be used at the same time as a constraints file. This makes the code easier to implement and means that we don't break any prior APIs. We will likely deprecate constraints when the dust settles.

There are several major deficiencies:

- Only `pex_from_targets.py` consumes this lockfile. This means that tool lockfiles will now have no constraints and no lockfile, for now.
- Does not handle requirements disjoint to the lockfile.
- Does not support multiple user lockfiles, which, for example, is necessary to properly handle `platforms` with `pex_binary` and `python_awslambda`: we need one lockfile per platform, as demonstrated in https://github.com/Eric-Arellano/lockfile-platforms-problem/tree/main/multiple_pex__stu_proposal.


### Lockfile vs. constraints file (and `[python-setup].resolve_all_constraints`)

We're currently using pip's `--constraints` file support, which allows you to specify constraints that may not actually be used. At the same time, we default to `[python-setup].resolve_all_constraints`, which _does_ first install the entire constraints file, and then uses Pex's repository PEX feature to extract the relevant subset. This is generally a performance optimization, but there are some times `--resolve-all-constraints` is not desirable:

1. It is not safe to first install the superset, and you can only install the proper subset. This especially can happen when `platforms` are used. See #12222.
     - We proactively disable `--resolve-all-constraints` when `platforms` are used.
2. User does not like the performance tradeoff, e.g. because they have a huge repository PEX so it's slow to access.

--

In contrast, this PR stops using `--constraints` and roughly always does `[python-setup].resolve_all_constraints` (we now run `pex -r requirements.txt --no-transitive` and use repository PEXes). Multiple user lockfiles will allow us to solve the above issues:

> 1. It is not safe to first install the superset, and you can only install the proper subset.

We'll have a distinct lockfile for each `platform`, which avoids this situation. See https://github.com/Eric-Arellano/lockfile-platforms-problem/tree/main/multiple_pex__stu_proposal for an example.

> 2. User does not like the performance tradeoff

They can use multiple lockfiles to work around this.

--

Always using `[python-setup].resolve_all_constraints` reduces complexity: less code to support, fewer concepts for users to learn.

Likewise, if we did still want to use `--constraints`, we would also need to upgrade Pex to use Pip 21+, which gained support for URL constraints. We [hacked around URL constraints before](#11907), but that isn't robust. However, Pip 21+ drops Python 2 and 3.5 support: we'd need to release Pex 3 w/o Py2 support, and upgrade Pants to have workarounds that allow Py2 to still be used. To avoid project creep, it's better to punt on Pex 3.

[ci skip-rust]
[ci skip-build-wheels]
  • Loading branch information
Eric-Arellano authored Jul 14, 2021
1 parent ad3dded commit 8742dfa
Show file tree
Hide file tree
Showing 8 changed files with 386 additions and 63 deletions.
39 changes: 0 additions & 39 deletions 3rdparty/python/constraints.txt

This file was deleted.

296 changes: 296 additions & 0 deletions 3rdparty/python/lockfile.txt

Large diffs are not rendered by default.

12 changes: 0 additions & 12 deletions build-support/bin/generate_lockfile.sh

This file was deleted.

2 changes: 1 addition & 1 deletion pants.toml
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ root_patterns = [
]

[python-setup]
requirement_constraints = "3rdparty/python/constraints.txt"
experimental_lockfile = "3rdparty/python/lockfile.txt"
interpreter_constraints = [">=3.7,<3.10"]

[docformatter]
Expand Down
33 changes: 22 additions & 11 deletions src/python/pants/backend/experimental/python/lockfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ class PipToolsSubsystem(PythonToolBase):
@classmethod
def register_options(cls, register):
super().register_options(register)
# TODO: How should users indicate where to save the lockfile to when we have per-tool
# lockfiles and mmultiple user lockfiles?
# TODO(#12314): How should users indicate where to save the lockfile to when we have
# per-tool lockfiles and multiple user lockfiles?
register(
"--lockfile-dest",
type=str,
Expand Down Expand Up @@ -62,22 +62,33 @@ async def generate_lockfile(
python_setup: PythonSetup,
workspace: Workspace,
) -> LockGoal:
# TODO: Looking at the transitive closure to generate a single lockfile will not work when we
# have multiple lockfiles supported, via per-tool lockfiles and multiple user lockfiles.
# TODO(#12314): Looking at the transitive closure to generate a single lockfile will not work
# when we have multiple lockfiles supported, via per-tool lockfiles and multiple user lockfiles.
# Ideally, `./pants lock ::` would mean "regenerate all unique lockfiles", whereas now it
# means "generate a single lockfile based on this transitive closure."
transitive_targets = await Get(TransitiveTargets, TransitiveTargetsRequest(addresses))

# TODO(#12314): Likely include "dev dependencies" like MyPy and Pytest, which must not have
# conflicting versions with user requirements. This is a simpler alternative to shading, which
# is likely out of scope for the project. See https://github.com/pantsbuild/pants/issues/9206.
#
# We may want to redesign how you set the version and requirements for subsystems in the
# process. Perhaps they should be directly a python_requirement_library, and you use a target
# address? Pytest is a particularly weird case because it's often both a tool you run _and_
# something you import.
#
# Make sure to not break https://github.com/pantsbuild/pants/issues/10819.
reqs = PexRequirements.create_from_requirement_fields(
tgt[PythonRequirementsField]
# NB: By looking at the dependencies, rather than the closure, we only generate for
# requirements that are actually used in the project.
#
# TODO: It's not totally clear to me if that is desirable. Consider requirements like
# TODO(#12314): It's not totally clear to me if that is desirable. Consider requirements like
# pydevd-pycharm. Should that be in the lockfile? I think this needs to be the case when
# we have multiple lockfiles, though: we shouldn't look at the universe in that case,
# only the relevant subset of requirements.
#
# Note that the current generate_lockfile.sh script in our docs also mixes in
# Note that the current generate_lockfile.sh script in our docs also mixes in
# `requirements.txt`, but not inlined python_requirement_library targets if they're not
# in use. We don't have a way to emulate those semantics because at this point, all we
# have is `python_requirement_library` targets without knowing the source.
Expand All @@ -95,8 +106,8 @@ async def generate_lockfile(
input_requirements_get = Get(
Digest, CreateDigest([FileContent("requirements.in", "\n".join(reqs).encode())])
)
# TODO: Figure out named_caches for pip-tools. The best would be to share the cache between
# Pex and Pip. Next best is a dedicated named_cache.
# TODO(#12314): Figure out named_caches for pip-tools. The best would be to share the cache
# between Pex and Pip. Next best is a dedicated named_cache.
pip_compile_get = Get(
VenvPex,
PexRequest(
Expand All @@ -120,7 +131,7 @@ async def generate_lockfile(
description=(
f"Generate lockfile for {pluralize(len(reqs), 'requirements')}: {', '.join(reqs)}"
),
# TODO: Wire up all the pip options like indexes.
# TODO(#12314): Wire up all the pip options like indexes.
argv=[
"requirements.in",
"--generate-hashes",
Expand All @@ -133,8 +144,8 @@ async def generate_lockfile(
output_files=(dest,),
),
)
# TODO: rewrite the file to have Pants header info, like how to regenerate the lockfile w/
# Pants.
# TODO(#12314): rewrite the file to have Pants header info, like how to regenerate the lockfile
# w/ Pants.
workspace.write_digest(result.output_digest)
logger.info(f"Wrote lockfile to {dest}")

Expand Down
25 changes: 25 additions & 0 deletions src/python/pants/backend/python/util_rules/pex.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ class PexRequest(EngineAwareParameter):
output_filename: str
internal_only: bool
requirements: PexRequirements
requirements_file: str | None
interpreter_constraints: InterpreterConstraints
platforms: PexPlatforms
sources: Digest | None
Expand All @@ -107,6 +108,7 @@ def __init__(
output_filename: str,
internal_only: bool,
requirements: PexRequirements = PexRequirements(),
requirements_file: str | None = None,
interpreter_constraints=InterpreterConstraints(),
platforms=PexPlatforms(),
sources: Digest | None = None,
Expand Down Expand Up @@ -148,6 +150,7 @@ def __init__(
self.output_filename = output_filename
self.internal_only = internal_only
self.requirements = requirements
self.requirements_file = requirements_file
self.interpreter_constraints = interpreter_constraints
self.platforms = platforms
self.sources = sources
Expand All @@ -167,6 +170,11 @@ def __post_init__(self):
f"Given platform constraints {self.platforms} for internal only pex request: "
f"{self}."
)
if self.requirements and self.requirements_file:
raise ValueError(
"You should only specify `requirements` or `requirements_file`, but both were set: "
f"{self}"
)

def debug_hint(self) -> str:
return self.output_filename
Expand Down Expand Up @@ -359,6 +367,20 @@ async def build_pex(
),
)

requirements_file_digest = EMPTY_DIGEST
if request.requirements_file:
argv.extend(["--requirement", request.requirements_file])
requirements_file_digest = await Get(
Digest,
PathGlobs(
[request.requirements_file],
glob_match_error_behavior=GlobMatchErrorBehavior.error,
# TODO(#12314): Parametrize this. Figure out a factoring that makes sense, e.g. a
# LockfilePex type.
description_of_origin="the option `[python-setup].experimental_lockfile`",
),
)

sources_digest_as_subdir = await Get(
Digest, AddPrefix(request.sources or EMPTY_DIGEST, source_dir_name)
)
Expand All @@ -374,6 +396,7 @@ async def build_pex(
sources_digest_as_subdir,
additional_inputs_digest,
constraint_file_digest,
requirements_file_digest,
repository_pex_digest,
*(pex.digest for pex in request.pex_path),
)
Expand All @@ -393,6 +416,8 @@ async def build_pex(
f"{pluralize(len(request.requirements), 'requirement')}: "
f"{', '.join(request.requirements)}"
)
elif request.requirements_file:
description = f"Building {request.output_filename} from {request.requirements_file}"
else:
description = f"Building {request.output_filename}"

Expand Down
22 changes: 22 additions & 0 deletions src/python/pants/backend/python/util_rules/pex_from_targets.py
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,28 @@ async def pex_from_targets(request: PexFromTargetsRequest, python_setup: PythonS
"`[python-setup].requirement_constraints` must also be set."
)

if python_setup.lockfile:
# TODO(#12314): This does not handle the case where requirements are disjoint to the
# lockfile. W/ constraints, we would avoid using a repository PEX in that case. We need
# to decide what to do in a lockfile world, likely a) give up on repository PEX and
# lockfile, or b) generate a new lockfile / error / warn to regenerate.
#
# This will likely change when multiple lockfiles are supported. In the meantime, it
# would be an issue because it would force you to have a single consistent resolve for
# your whole repo.
repository_pex = await Get(
Pex,
PexRequest(
description=f"Resolving {python_setup.lockfile}",
output_filename="lockfile.pex",
internal_only=request.internal_only,
requirements_file=python_setup.lockfile,
interpreter_constraints=interpreter_constraints,
platforms=request.platforms,
additional_args=(*request.additional_args, "--no-transitive"),
),
)

return PexRequest(
output_filename=request.output_filename,
internal_only=request.internal_only,
Expand Down
20 changes: 20 additions & 0 deletions src/python/pants/python/python_setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ def register_options(cls, register):
"--requirement-constraints",
advanced=True,
type=file_option,
mutually_exclusive_group="lockfile",
help=(
"When resolving third-party requirements, use this "
"constraints file to determine which versions to use.\n\nSee "
Expand All @@ -70,6 +71,21 @@ def register_options(cls, register):
"\n\nRequires [python-setup].requirement_constraints to be set."
),
)
register(
"--experimental-lockfile",
advanced=True,
type=file_option,
mutually_exclusive_group="constraints",
help=(
"The lockfile to use when resolving requirements for your own code (vs. tools you "
"run).\n\n"
"This is highly experimental and will change, including adding support for "
"multiple lockfiles. This option's behavior may change without the normal "
"deprecation cycle.\n\n"
"To generate a lockfile, activate the backend `pants.backend.experimental.python`"
"and run `./pants lock ::`."
),
)
register(
"--interpreter-search-paths",
advanced=True,
Expand Down Expand Up @@ -132,6 +148,10 @@ def requirement_constraints(self) -> str | None:
"""Path to constraint file."""
return cast("str | None", self.options.requirement_constraints)

@property
def lockfile(self) -> str | None:
return cast("str | None", self.options.experimental_lockfile)

@property
def resolve_all_constraints(self) -> bool:
return cast(bool, self.options.resolve_all_constraints)
Expand Down

0 comments on commit 8742dfa

Please sign in to comment.