Skip to content

Commit

Permalink
[internal] Improve the error message for stale/invalid lockfiles (#12618
Browse files Browse the repository at this point in the history
)

Refactors the `build_pex` rule to only have the code for erroring/warning on invalid lockfiles in the one place, and updates the corresponding exception and log messages.

I'm not 100% sold on the contents of the log message, but I'm happy enough for now. Here's a sample warning:

> ```13:09:40.67 [WARN] Invalid lockfile for PEX request `black.pex`.  If your requirements or interpreter constraints have changed, follow the instructions in the header of the lockfile to regenerate it. Otherwise, ensure your interpreter constraints are compatible with the constraints specified in the lockfile.```

Closes #12541

[ci skip-rust]
[ci skip-build-wheels]
  • Loading branch information
Christopher Neugebauer authored Aug 27, 2021
1 parent 8415c30 commit 2a3e950
Show file tree
Hide file tree
Showing 4 changed files with 161 additions and 49 deletions.
43 changes: 34 additions & 9 deletions src/python/pants/backend/experimental/python/lockfile_metadata.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import hashlib
import json
from dataclasses import dataclass
from enum import Enum
from typing import Any, Iterable

from pants.backend.python.util_rules.interpreter_constraints import InterpreterConstraints
Expand Down Expand Up @@ -92,16 +93,23 @@ def is_valid_for(
expected_invalidation_digest: str | None,
user_interpreter_constraints: InterpreterConstraints,
interpreter_universe: Iterable[str],
) -> bool:
"""Returns True if the lockfile can be used in the current execution context."""
) -> LockfileMetadataValidation:
"""Returns Truthy if this `LockfileMetadata` can be used in the current execution
context."""
failure_reasons: set[InvalidLockfileReason] = set()

if expected_invalidation_digest is None:
return True
return (
self.requirements_invalidation_digest == expected_invalidation_digest
and self.valid_for_interpreter_constraints.contains(
user_interpreter_constraints, interpreter_universe
)
)
return LockfileMetadataValidation(failure_reasons)

if self.requirements_invalidation_digest != expected_invalidation_digest:
failure_reasons.add(InvalidLockfileReason.INVALIDATION_DIGEST_MISMATCH)

if not self.valid_for_interpreter_constraints.contains(
user_interpreter_constraints, interpreter_universe
):
failure_reasons.add(InvalidLockfileReason.INTERPRETER_CONSTRAINTS_MISMATCH)

return LockfileMetadataValidation(failure_reasons)


def calculate_invalidation_digest(requirements: Iterable[str]) -> str:
Expand All @@ -114,3 +122,20 @@ def calculate_invalidation_digest(requirements: Iterable[str]) -> str:
}
m.update(json.dumps(inputs).encode("utf-8"))
return m.hexdigest()


class InvalidLockfileReason(Enum):
INVALIDATION_DIGEST_MISMATCH = "invalidation_digest_mismatch"
INTERPRETER_CONSTRAINTS_MISMATCH = "interpreter_constraints_mismatch"


class LockfileMetadataValidation:
"""Boolean-like value which additionally carries reasons why a validation failed."""

failure_reasons: set[InvalidLockfileReason]

def __init__(self, failure_reasons: Iterable[InvalidLockfileReason] = ()):
self.failure_reasons = set(failure_reasons)

def __bool__(self):
return not self.failure_reasons
Original file line number Diff line number Diff line change
Expand Up @@ -124,10 +124,12 @@ def assert_neq(left: Iterable[str], right: Iterable[str]) -> None:
def test_is_valid_for(user_digest, expected_digest, user_ic, expected_ic, matches) -> None:
m = LockfileMetadata(expected_digest, InterpreterConstraints(expected_ic))
assert (
m.is_valid_for(
user_digest,
InterpreterConstraints(user_ic),
["2.7", "3.5", "3.6", "3.7", "3.8", "3.9", "3.10"],
bool(
m.is_valid_for(
user_digest,
InterpreterConstraints(user_ic),
["2.7", "3.5", "3.6", "3.7", "3.8", "3.9", "3.10"],
)
)
== matches
)
79 changes: 60 additions & 19 deletions src/python/pants/backend/python/util_rules/pex.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@
import packaging.version
from pkg_resources import Requirement

from pants.backend.experimental.python.lockfile_metadata import LockfileMetadata
from pants.backend.experimental.python.lockfile_metadata import (
InvalidLockfileReason,
LockfileMetadata,
)
from pants.backend.python.target_types import MainSpecification
from pants.backend.python.target_types import PexPlatformsField as PythonPlatformsField
from pants.backend.python.target_types import PythonRequirementsField
Expand Down Expand Up @@ -413,15 +416,7 @@ async def build_pex(

requirements_file_digest_contents = await Get(DigestContents, PathGlobs, globs)
metadata = LockfileMetadata.from_lockfile(requirements_file_digest_contents[0].content)
if not metadata.is_valid_for(
request.requirements.lockfile_hex_digest,
request.interpreter_constraints,
python_setup.interpreter_universe,
):
if python_setup.invalid_lockfile_behavior == InvalidLockfileBehavior.error:
raise ValueError("Invalid lockfile provided. [TODO(#12314): Improve message]")
elif python_setup.invalid_lockfile_behavior == InvalidLockfileBehavior.warn:
logger.warning("%s", "Invalid lockfile provided. [TODO(#12314): Improve message]")
_validate_metadata(metadata, request, request.requirements, python_setup)

requirements_file_digest = await Get(Digest, PathGlobs, globs)

Expand All @@ -430,15 +425,7 @@ async def build_pex(
argv.extend(["--requirement", file_content.path])

metadata = LockfileMetadata.from_lockfile(file_content.content)
if not metadata.is_valid_for(
request.requirements.lockfile_hex_digest,
request.interpreter_constraints,
python_setup.interpreter_universe,
):
if python_setup.invalid_lockfile_behavior == InvalidLockfileBehavior.error:
raise ValueError("Invalid lockfile provided. [TODO(#12314): Improve message]")
elif python_setup.invalid_lockfile_behavior == InvalidLockfileBehavior.warn:
logger.warning("%s", "Invalid lockfile provided. [TODO(#12314): Improve message]")
_validate_metadata(metadata, request, request.requirements, python_setup)

requirements_file_digest = await Get(Digest, CreateDigest([file_content]))
else:
Expand Down Expand Up @@ -492,6 +479,60 @@ async def build_pex(
)


def _validate_metadata(
metadata: LockfileMetadata,
request: PexRequest,
requirements: (Lockfile | LockfileContent),
python_setup: PythonSetup,
) -> None:

validation = metadata.is_valid_for(
requirements.lockfile_hex_digest,
request.interpreter_constraints,
python_setup.interpreter_universe,
)

if validation:
return

message_parts = [
f"Invalid lockfile for PEX request `{request.output_filename}`.",
"\n",
]

if InvalidLockfileReason.INVALIDATION_DIGEST_MISMATCH in validation.failure_reasons:
message_parts.append(
"The requirements set for this PEX request are different to the requirements set when "
"the lockfile was generated. To fix this, you will need to regenerate the lockfile. "
)
message_parts.append(
f"(Expected requirements digest: {requirements.lockfile_hex_digest}, "
"actual: {metadata.requirements_invalidation_digest})"
)
message_parts.append("\n")

if InvalidLockfileReason.INTERPRETER_CONSTRAINTS_MISMATCH in validation.failure_reasons:
message_parts.append(
"The lockfile was generated under interpreter constraints "
f"({ metadata.valid_for_interpreter_constraints }) that are incompatible with the "
f"constraints set in the project ({request.interpreter_constraints }). If you have "
"overridden your project's interpreter constraints, you can update them to specify a "
"subset of the interpreters specified in the lockfile. If not, you will need to "
"regenerate your lockfile. "
)

message_parts.append(
"To regenerate the lockfile, follow the instructions in the header of the lockfile."
)

message = "\n".join(message_parts).strip()

if python_setup.invalid_lockfile_behavior == InvalidLockfileBehavior.error:
raise ValueError(message)
elif python_setup.invalid_lockfile_behavior == InvalidLockfileBehavior.warn:
logger.warning("%s", message)


def _build_pex_description(request: PexRequest) -> str:
if request.description:
return request.description
Expand Down
78 changes: 61 additions & 17 deletions src/python/pants/backend/python/util_rules/pex_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -533,69 +533,113 @@ def assert_description(
def test_error_on_invalid_lockfile_with_path(rule_runner: RuleRunner) -> None:
with pytest.raises(ExecutionError):
_run_pex_for_lockfile_test(
rule_runner, True, actual="1bad", expected="900d", behavior="error"
rule_runner,
use_file=True,
behavior="error",
invalid_reqs=True,
)


def test_warn_on_invalid_lockfile_with_path(rule_runner: RuleRunner, caplog) -> None:
_run_pex_for_lockfile_test(rule_runner, True, actual="1bad", expected="900d", behavior="warn")
assert "Invalid lockfile provided." in caplog.text
_run_pex_for_lockfile_test(rule_runner, use_file=True, behavior="warn", invalid_reqs=True)
assert "Invalid lockfile for PEX request" in caplog.text


def test_warn_on_requirements_mismatch(rule_runner: RuleRunner, caplog) -> None:
_run_pex_for_lockfile_test(rule_runner, use_file=True, behavior="warn", invalid_reqs=True)
assert "requirements set for this" in caplog.text
assert "generated under interpreter constraints" not in caplog.text


def test_warn_on_interpreter_constraints_mismatch(rule_runner: RuleRunner, caplog) -> None:
_run_pex_for_lockfile_test(
rule_runner, use_file=True, behavior="warn", invalid_constraints=True
)
assert "requirements set for this" not in caplog.text
assert "generated under interpreter constraints" in caplog.text


def test_warn_on_mismatched_requirements_and_interpreter_constraints(
rule_runner: RuleRunner, caplog
) -> None:
_run_pex_for_lockfile_test(
rule_runner, use_file=True, behavior="warn", invalid_reqs=True, invalid_constraints=True
)
assert "requirements set for this" in caplog.text
assert "generated under interpreter constraints" in caplog.text


def test_ignore_on_invalid_lockfile_with_path(rule_runner: RuleRunner, caplog) -> None:
_run_pex_for_lockfile_test(rule_runner, True, actual="1bad", expected="900d", behavior="ignore")
_run_pex_for_lockfile_test(rule_runner, use_file=True, behavior="ignore", invalid_reqs=True)
assert not caplog.text.strip()


def test_no_warning_on_valid_lockfile_with_path(rule_runner: RuleRunner, caplog) -> None:
_run_pex_for_lockfile_test(rule_runner, True, actual="900d", expected="900d", behavior="warn")
_run_pex_for_lockfile_test(rule_runner, use_file=True, behavior="warn")
assert not caplog.text.strip()


def test_error_on_invalid_lockfile_with_content(rule_runner: RuleRunner) -> None:
with pytest.raises(ExecutionError):
_run_pex_for_lockfile_test(
rule_runner, False, actual="1bad", expected="900d", behavior="error"
)
_run_pex_for_lockfile_test(rule_runner, use_file=False, behavior="error", invalid_reqs=True)


def test_warn_on_invalid_lockfile_with_content(rule_runner: RuleRunner, caplog) -> None:
_run_pex_for_lockfile_test(rule_runner, False, actual="1bad", expected="900d", behavior="warn")
assert "Invalid lockfile provided." in caplog.text
_run_pex_for_lockfile_test(rule_runner, use_file=False, behavior="warn", invalid_reqs=True)
assert "Invalid lockfile for PEX request" in caplog.text


def test_no_warning_on_valid_lockfile_with_content(rule_runner: RuleRunner, caplog) -> None:
_run_pex_for_lockfile_test(rule_runner, False, actual="900d", expected="900d", behavior="warn")
_run_pex_for_lockfile_test(rule_runner, use_file=False, behavior="warn")
assert not caplog.text.strip()


def _run_pex_for_lockfile_test(rule_runner, use_file, actual, expected, behavior):
constraints = "CPython>=3.9"
def _run_pex_for_lockfile_test(
rule_runner,
*,
use_file,
behavior,
invalid_reqs=False,
invalid_constraints=False,
) -> None:
actual_digest = "900d"
expected_digest = actual_digest
if invalid_reqs:
expected_digest = "baad"

actual_constraints = "CPython>=3.6,<3.10"
expected_constraints = actual_constraints
if invalid_constraints:
expected_constraints = "CPython>=3.9"

lockfile = f"""
# --- BEGIN PANTS LOCKFILE METADATA: DO NOT EDIT OR REMOVE ---
# {{
# "requirements_invalidation_digest": "{actual}",
# "requirements_invalidation_digest": "{actual_digest}",
# "valid_for_interpreter_constraints": [
# "{ constraints }"
# "{ actual_constraints }"
# ]
# }}
# --- END PANTS LOCKFILE METADATA ---
ansicolors==1.1.8
"""
requirements: Lockfile | LockfileContent

if use_file:
file_path = "lockfile.txt"
rule_runner.write_files({file_path: lockfile})
requirements = Lockfile(
file_path=file_path,
file_path_description_of_origin="iceland",
lockfile_hex_digest=expected,
lockfile_hex_digest=expected_digest,
)
else:
content = FileContent("lockfile.txt", lockfile.encode("utf-8"))
requirements = LockfileContent(file_content=content, lockfile_hex_digest=expected)
requirements = LockfileContent(file_content=content, lockfile_hex_digest=expected_digest)

create_pex_and_get_all_data(
rule_runner,
interpreter_constraints=InterpreterConstraints([expected_constraints]),
requirements=requirements,
additional_pants_args=(
"--python-setup-experimental-lockfile=lockfile.txt",
Expand Down

0 comments on commit 2a3e950

Please sign in to comment.