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

Use a list of requirement constraints for lockfile invalidation #16469

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
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
50 changes: 26 additions & 24 deletions src/python/pants/backend/python/goals/lockfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
)
from pants.core.util_rules.lockfile_metadata import calculate_invalidation_digest
from pants.engine.fs import CreateDigest, Digest, DigestContents, FileContent, MergeDigests
from pants.engine.internals.native_engine import FileDigest
from pants.engine.process import ProcessCacheScope, ProcessResult
from pants.engine.rules import Get, MultiGet, collect_rules, rule, rule_helper
from pants.engine.target import AllTargets
Expand Down Expand Up @@ -155,35 +154,41 @@ def warn_python_repos(option: str) -> None:
return MaybeWarnPythonRepos()


@dataclass
class _PipArgsAndConstraintsSetup:
args: list[str]
Copy link
Contributor

Choose a reason for hiding this comment

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

This works since _PipArgsAndConstraintsSetup is private and only a return type of a helper, but having to look at any un-frozen struct, at least for me, raises suspicion reading rule code. If there was some mysterious bug reported and I was scanning the codebase, this would cause me to burn un-needed time reviewing why this is OK. I may be a poor example here though of a code reader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I myself was a bit confused trying to trace whether this was safe or not. Thanks

digest: Digest
constraints: set[PipRequirement]


@rule_helper
async def _setup_pip_args_and_constraints_file(
python_setup: PythonSetup, *, resolve_name: str
) -> tuple[list[str], Digest, FileDigest | None]:
extra_args = []
extra_digests = []
constraints_file_digest: None | FileDigest = None
) -> _PipArgsAndConstraintsSetup:
args = []
digests = []

if python_setup.no_binary or python_setup.only_binary:
pip_args_file = "__pip_args.txt"
extra_args.extend(["-r", pip_args_file])
args.extend(["-r", pip_args_file])
pip_args_file_content = "\n".join(
[f"--no-binary {pkg}" for pkg in python_setup.no_binary]
+ [f"--only-binary {pkg}" for pkg in python_setup.only_binary]
)
pip_args_digest = await Get(
Digest, CreateDigest([FileContent(pip_args_file, pip_args_file_content.encode())])
)
extra_digests.append(pip_args_digest)
digests.append(pip_args_digest)

constraints: set[PipRequirement] = set()
resolve_config = await Get(ResolvePexConfig, ResolvePexConfigRequest(resolve_name))
if resolve_config.constraints_file:
_constraints_file_entry = resolve_config.constraints_file[1]
extra_args.append(f"--constraints={_constraints_file_entry.path}")
constraints_file_digest = _constraints_file_entry.file_digest
extra_digests.append(resolve_config.constraints_file[0])
args.append(f"--constraints={resolve_config.constraints_file.path}")
digests.append(resolve_config.constraints_file.digest)
constraints = set(resolve_config.constraints_file.constraints)

input_digest = await Get(Digest, MergeDigests(extra_digests))
return extra_args, input_digest, constraints_file_digest
input_digest = await Get(Digest, MergeDigests(digests))
return _PipArgsAndConstraintsSetup(args, input_digest, constraints)


@rule(desc="Generate Python lockfile", level=LogLevel.DEBUG)
Expand All @@ -194,16 +199,13 @@ async def generate_lockfile(
python_repos: PythonRepos,
python_setup: PythonSetup,
) -> GenerateLockfileResult:
constraints_file_hash: str | None = None
requirement_constraints: set[PipRequirement] = set()

if req.use_pex:
(
extra_args,
input_digest,
constraints_file_digest,
) = await _setup_pip_args_and_constraints_file(python_setup, resolve_name=req.resolve_name)
if constraints_file_digest:
constraints_file_hash = constraints_file_digest.fingerprint
pip_args_setup = await _setup_pip_args_and_constraints_file(
python_setup, resolve_name=req.resolve_name
)
requirement_constraints = pip_args_setup.constraints

header_delimiter = "//"
result = await Get(
Expand Down Expand Up @@ -233,13 +235,13 @@ async def generate_lockfile(
"mac",
# This makes diffs more readable when lockfiles change.
"--indent=2",
*extra_args,
*pip_args_setup.args,
*python_repos.pex_args,
*python_setup.manylinux_pex_args,
*req.interpreter_constraints.generate_pex_arg_list(),
*req.requirements,
),
additional_input_digest=input_digest,
additional_input_digest=pip_args_setup.digest,
output_files=("lock.json",),
description=f"Generate lockfile for {req.resolve_name}",
# Instead of caching lockfile generation with LMDB, we instead use the invalidation
Expand Down Expand Up @@ -306,7 +308,7 @@ async def generate_lockfile(
metadata = PythonLockfileMetadata.new(
valid_for_interpreter_constraints=req.interpreter_constraints,
requirements={PipRequirement.parse(i) for i in req.requirements},
constraints_file_hash=constraints_file_hash,
requirement_constraints=requirement_constraints,
)
lockfile_with_header = metadata.add_header_to_lockfile(
initial_lockfile_digest_contents[0].content,
Expand Down
50 changes: 29 additions & 21 deletions src/python/pants/backend/python/goals/lockfile_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def _generate(
rule_runner: RuleRunner,
use_pex: bool,
ansicolors_version: str = "==1.1.8",
constraints_file_hash: str | None = None,
requirement_constraints_str: str = '// "requirement_constraints": []',
) -> str:
result = rule_runner.request(
GenerateLockfileResult,
Expand All @@ -64,24 +64,29 @@ def _generate(
if not use_pex:
return content

constraints_file_hash_str = f'"{constraints_file_hash}"' if constraints_file_hash else "null"
pex_header = dedent(
f"""\
// This lockfile was autogenerated by Pants. To regenerate, run:
//
// ./pants generate-lockfiles --resolve=test
//
// --- BEGIN PANTS LOCKFILE METADATA: DO NOT EDIT OR REMOVE ---
// {{
// "version": 3,
// "valid_for_interpreter_constraints": [],
// "generated_with_requirements": [
// "ansicolors{ansicolors_version}"
// ],
// "constraints_file_hash": {constraints_file_hash_str}
// }}
// --- END PANTS LOCKFILE METADATA ---
"""
pex_header = (
dedent(
f"""\
// This lockfile was autogenerated by Pants. To regenerate, run:
//
// ./pants generate-lockfiles --resolve=test
//
// --- BEGIN PANTS LOCKFILE METADATA: DO NOT EDIT OR REMOVE ---
// {{
// "version": 3,
// "valid_for_interpreter_constraints": [],
// "generated_with_requirements": [
// "ansicolors{ansicolors_version}"
// ],
"""
)
+ requirement_constraints_str
+ dedent(
"""
// }
// --- END PANTS LOCKFILE METADATA ---
"""
)
)
assert content.startswith(pex_header)
return strip_prefix(content, pex_header)
Expand Down Expand Up @@ -167,8 +172,11 @@ def test_constraints_file(rule_runner: RuleRunner) -> None:
rule_runner=rule_runner,
use_pex=True,
ansicolors_version=">=1.0",
constraints_file_hash=(
"1999760ce9dd0f82847def308992e3345592fc9e77a937c1e9bbb78a42ae3943"
requirement_constraints_str=dedent(
"""\
// "requirement_constraints": [
// "ansicolors==1.1.7"
// ]"""
),
)
)
Expand Down
3 changes: 3 additions & 0 deletions src/python/pants/backend/python/pip_requirement.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,5 +82,8 @@ def __eq__(self, other):
return False
return self._req == other._req

def __repr__(self) -> str:
return f"{self.__class__.__name__}({self._req})"

def __str__(self):
return str(self._req)
41 changes: 18 additions & 23 deletions src/python/pants/backend/python/util_rules/lockfile_metadata.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def new(
*,
valid_for_interpreter_constraints: InterpreterConstraints,
requirements: set[PipRequirement],
constraints_file_hash: str | None,
requirement_constraints: set[PipRequirement],
) -> PythonLockfileMetadata:
"""Call the most recent version of the `LockfileMetadata` class to construct a concrete
instance.
Expand All @@ -50,7 +50,7 @@ def new(
"""

return PythonLockfileMetadataV3(
valid_for_interpreter_constraints, requirements, constraints_file_hash
valid_for_interpreter_constraints, requirements, requirement_constraints
)

@classmethod
Expand All @@ -70,7 +70,7 @@ def is_valid_for(
user_interpreter_constraints: InterpreterConstraints,
interpreter_universe: Iterable[str],
user_requirements: Iterable[PipRequirement],
constraints_file_path_and_hash: tuple[str, str] | None,
requirement_constraints: Iterable[PipRequirement],
) -> LockfileMetadataValidation:
"""Returns Truthy if this `PythonLockfileMetadata` can be used in the current execution
context."""
Expand Down Expand Up @@ -114,7 +114,7 @@ def is_valid_for(
interpreter_universe: Iterable[str],
# Everything below is not used by v1.
user_requirements: Iterable[PipRequirement],
constraints_file_path_and_hash: tuple[str, str] | None,
requirement_constraints: Iterable[PipRequirement],
) -> LockfileMetadataValidation:
failure_reasons: set[InvalidPythonLockfileReason] = set()

Expand Down Expand Up @@ -168,13 +168,7 @@ def additional_header_attrs(cls, instance: LockfileMetadata) -> dict[Any, Any]:
instance = cast(PythonLockfileMetadataV2, instance)
# Requirements need to be stringified then sorted so that tests are deterministic. Sorting
# followed by stringifying does not produce a meaningful result.
return {
"generated_with_requirements": (
sorted(str(i) for i in instance.requirements)
if instance.requirements is not None
else None
)
}
return {"generated_with_requirements": (sorted(str(i) for i in instance.requirements))}

def is_valid_for(
self,
Expand All @@ -185,7 +179,7 @@ def is_valid_for(
interpreter_universe: Iterable[str],
user_requirements: Iterable[PipRequirement],
# Everything below is not used by V2.
constraints_file_path_and_hash: tuple[str, str] | None,
requirement_constraints: Iterable[PipRequirement],
) -> LockfileMetadataValidation:
failure_reasons = set()

Expand All @@ -210,7 +204,7 @@ def is_valid_for(
class PythonLockfileMetadataV3(PythonLockfileMetadataV2):
"""Lockfile version that considers constraints files."""

constraints_file_hash: str | None
requirement_constraints: set[PipRequirement]

@classmethod
def _from_json_dict(
Expand All @@ -221,19 +215,23 @@ def _from_json_dict(
) -> PythonLockfileMetadataV3:
v2_metadata = super()._from_json_dict(json_dict, lockfile_description, error_suffix)
metadata = _get_metadata(json_dict, lockfile_description, error_suffix)
constraints_file_hash = metadata(
"constraints_file_hash", str, lambda x: x # type: ignore[no-any-return]
requirement_constraints = metadata(
"requirement_constraints",
Set[PipRequirement],
lambda l: {PipRequirement.parse(i) for i in l},
)
return PythonLockfileMetadataV3(
valid_for_interpreter_constraints=v2_metadata.valid_for_interpreter_constraints,
requirements=v2_metadata.requirements,
constraints_file_hash=constraints_file_hash,
requirement_constraints=requirement_constraints,
)

@classmethod
def additional_header_attrs(cls, instance: LockfileMetadata) -> dict[Any, Any]:
instance = cast(PythonLockfileMetadataV3, instance)
return {"constraints_file_hash": instance.constraints_file_hash}
return {
"requirement_constraints": (sorted(str(i) for i in instance.requirement_constraints))
}

def is_valid_for(
self,
Expand All @@ -243,7 +241,7 @@ def is_valid_for(
user_interpreter_constraints: InterpreterConstraints,
interpreter_universe: Iterable[str],
user_requirements: Iterable[PipRequirement],
constraints_file_path_and_hash: tuple[str, str] | None,
requirement_constraints: Iterable[PipRequirement],
) -> LockfileMetadataValidation:
failure_reasons = (
super()
Expand All @@ -253,14 +251,11 @@ def is_valid_for(
user_interpreter_constraints=user_interpreter_constraints,
interpreter_universe=interpreter_universe,
user_requirements=user_requirements,
constraints_file_path_and_hash=constraints_file_path_and_hash,
requirement_constraints=requirement_constraints,
)
.failure_reasons
)

provided_constraints_file_hash = (
constraints_file_path_and_hash[1] if constraints_file_path_and_hash else None
)
if provided_constraints_file_hash != self.constraints_file_hash:
if self.requirement_constraints != set(requirement_constraints):
failure_reasons.add(InvalidPythonLockfileReason.CONSTRAINTS_FILE_MISMATCH)
return LockfileMetadataValidation(failure_reasons)
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def test_metadata_header_round_trip() -> None:
["CPython==2.7.*", "PyPy", "CPython>=3.6,<4,!=3.7.*"]
),
requirements=reqset("ansicolors==0.1.0"),
constraints_file_hash="abc",
requirement_constraints={PipRequirement.parse("constraint")},
)
serialized_lockfile = input_metadata.add_header_to_lockfile(
b"req1==1.0", regenerate_command="./pants lock", delimeter="#"
Expand Down Expand Up @@ -62,7 +62,7 @@ def test_add_header_to_lockfile() -> None:
# "generated_with_requirements": [
# "ansicolors==0.1.0"
# ],
# "constraints_file_hash": null
# "requirement_constraints": []
# }
# --- END PANTS LOCKFILE METADATA ---
dave==3.1.4 \\
Expand All @@ -75,7 +75,7 @@ def line_by_line(b: bytes) -> list[bytes]:
metadata = PythonLockfileMetadata.new(
valid_for_interpreter_constraints=InterpreterConstraints([">=3.7"]),
requirements=reqset("ansicolors==0.1.0"),
constraints_file_hash=None,
requirement_constraints=set(),
)
result = metadata.add_header_to_lockfile(
input_lockfile, regenerate_command="./pants lock", delimeter="#"
Expand Down Expand Up @@ -158,7 +158,7 @@ def test_is_valid_for_v1(user_digest, expected_digest, user_ic, expected_ic, mat
user_interpreter_constraints=InterpreterConstraints(user_ic),
interpreter_universe=INTERPRETER_UNIVERSE,
user_requirements=set(),
constraints_file_path_and_hash=None,
requirement_constraints=set(),
)
)
== matches
Expand Down Expand Up @@ -232,7 +232,7 @@ def test_is_valid_for_interpreter_constraints_and_requirements(
for m in [
PythonLockfileMetadataV2(InterpreterConstraints(lock_ics), reqset(*lock_reqs)),
PythonLockfileMetadataV3(
InterpreterConstraints(lock_ics), reqset(*lock_reqs), constraints_file_hash=None
InterpreterConstraints(lock_ics), reqset(*lock_reqs), requirement_constraints=set()
),
]:
result = m.is_valid_for(
Expand All @@ -241,21 +241,21 @@ def test_is_valid_for_interpreter_constraints_and_requirements(
user_interpreter_constraints=InterpreterConstraints(user_ics),
interpreter_universe=INTERPRETER_UNIVERSE,
user_requirements=reqset(*user_reqs),
constraints_file_path_and_hash=None,
requirement_constraints=set(),
)
assert result.failure_reasons == set(expected)


@pytest.mark.parametrize("is_tool", [True, False])
def test_is_valid_for_constraints_file_hash(is_tool: bool) -> None:
def test_is_valid_for_requirement_constraints(is_tool: bool) -> None:
result = PythonLockfileMetadataV3(
InterpreterConstraints([]), reqset(), constraints_file_hash="abc"
InterpreterConstraints([]), reqset(), requirement_constraints={PipRequirement.parse("c1")}
).is_valid_for(
is_tool=is_tool,
expected_invalidation_digest="",
user_interpreter_constraints=InterpreterConstraints([]),
interpreter_universe=INTERPRETER_UNIVERSE,
user_requirements=reqset(),
constraints_file_path_and_hash=("c.txt", "xyz"),
requirement_constraints={PipRequirement.parse("c2")},
)
assert result.failure_reasons == {InvalidPythonLockfileReason.CONSTRAINTS_FILE_MISMATCH}
Loading