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.

Sign up for GitHub

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

Update the error/warning messages for stale/invalid lockfiles #12618

Merged
merged 14 commits into from
Aug 27, 2021
Merged

Update the error/warning messages for stale/invalid lockfiles #12618

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9a94ee9
Update the error/warning messages for stale/invalid lockfiles
Aug 20, 2021
d83c6e9
Address code review feedback
Aug 23, 2021
23aecb2
Adds more detailed error messages for lockfile validation
Aug 23, 2021
5147d25
Add more lockfile message tests to verify for contents of log messages
Aug 23, 2021
c67f1cd
Restructure tests so it's easier to follow what behaviour is being ch…
Aug 23, 2021
c8918ec
Adds expected lockfile digest to error message
Aug 23, 2021
2c23bda
Merge remote-tracking branch 'origin/main' into chrisjrn/12541-invali…
Aug 23, 2021
8b9ea01
Add formatting fixes.
Aug 23, 2021
db1e5d1
Merge remote-tracking branch 'origin/main' into chrisjrn/12541-invali…
Aug 25, 2021
1356096
Make work post-merge
Aug 25, 2021
d2f4387
Fix tests
Aug 26, 2021
ac04497
Improve pex test formatting
Aug 26, 2021
f667881
Update interpreter constraints messages
Aug 26, 2021
6fcd36b
Be extra safe with mutable args
Aug 26, 2021
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
43 changes: 34 additions & 9 deletions src/python/pants/backend/experimental/python/lockfile_metadata.py
View file
Open in desktop
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]
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for this because it's not a dataclass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Declaring slots does make the class slightly more efficient under the hood, but I was mostly doing it for the benefit of type hinting. Is there a way to provide the type hints without declaring the slot?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this results in actually using __slots__? Iiuc, you have to explicitly set __slots__ to get its benefit. Either way, we decided to not use __slots__ w/ Pants when I propsed it in #9469.

Is there a way to provide the type hints without declaring the slot?

Yeah, MyPy should already infer it thanks to your __init__.

Copy link
Contributor

Choose a reason for hiding this comment

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

bump - does this make sense?


def __init__(self, failure_reasons: Iterable[InvalidLockfileReason] = set()):
chrisjrn marked this conversation as resolved.
Show resolved Hide resolved
self.failure_reasons = set(failure_reasons)

def __bool__(self):
return not self.failure_reasons
73 changes: 54 additions & 19 deletions src/python/pants/backend/python/util_rules/pex.py
View file
Open in desktop
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 @@ -436,15 +439,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, python_setup)

requirements_file_digest = await Get(Digest, PathGlobs, globs)

Expand All @@ -453,15 +448,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, python_setup)

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


def _validate_metadata(
metadata: LockfileMetadata, request: PexRequest, python_setup: PythonSetup
) -> None:
validation = metadata.is_valid_for(
request.requirements.lockfile_hex_digest,
request.interpreter_constraints,
python_setup.interpreter_universe,
)
Eric-Arellano marked this conversation as resolved.
Show resolved Hide resolved

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: {request.requirements.lockfile_hex_digest}, "
"actual: {metadata.requirements_invalidation_digest})"
)
message_parts.append("\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

This can use .extend(). Are you using .append() perhaps for the sake of formatting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the strings are bumping up agains the line length limit, using .extend() makes the code more spread out than it is with append (lol)

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. FYI you could do this if you go with extend:

message_parts.extend([
   (
        "blah blah blah "
         "blah blah"
   ),
   (
        "hola hola mundo "
         "blah blah"
   ),
])

this is fine otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I know about extend, it just ended up looking uglier and longer because of extra line breaks


if InvalidLockfileReason.INTERPRETER_CONSTRAINTS_MISMATCH in validation.failure_reasons:
message_parts.append(
"The lockfile was generated under different interpreter constraints to the constraints "
"that are set. If you have overridden your project's interpreter constraints, you can "
chrisjrn marked this conversation as resolved.
Show resolved Hide resolved
"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
71 changes: 55 additions & 16 deletions src/python/pants/backend/python/util_rules/pex_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,50 +541,88 @@ 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,
True,
chrisjrn marked this conversation as resolved.
Show resolved Hide resolved
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, 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, True, behavior="warn", invalid_reqs=True)
assert "requirements set for this" in caplog.text
assert "different 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, True, behavior="warn", invalid_constraints=True)
assert "requirements set for this" not in caplog.text
assert "different 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, True, behavior="warn", invalid_reqs=True, invalid_constraints=True
)
assert "requirements set for this" in caplog.text
assert "different 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, 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, 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, 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, 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, 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,
):
chrisjrn marked this conversation as resolved.
Show resolved Hide resolved
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 ---
Expand All @@ -599,10 +637,11 @@ def _run_pex_for_lockfile_test(rule_runner, use_file, actual, expected, behavior

create_pex_and_get_all_data(
rule_runner,
interpreter_constraints=InterpreterConstraints([expected_constraints]),
requirements=PexRequirements(
**file_args,
file_path_description_of_origin="iceland",
lockfile_hex_digest=expected,
lockfile_hex_digest=expected_digest,
),
additional_pants_args=(
"--python-setup-experimental-lockfile=lockfile.txt",
Expand Down