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

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

Merged

Conversation

chrisjrn
Copy link
Contributor

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

Rust tests and lints will be skipped. Delete if not intended.

[ci skip-rust]

Building wheels and fs_util will be skipped. Delete if not intended.

[ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thanks Chris

src/python/pants/backend/python/util_rules/pex.py Outdated Show resolved Hide resolved
src/python/pants/backend/python/util_rules/pex.py Outdated Show resolved Hide resolved
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thanks! As mentioned, I think we'll want to improve this later to be more precise, eg changing the error message based on which part of the metadata was invalid. But we're going to be changing these code paths a lot for multiple user lockfiles, and this is a good improvement, so we can land this as an incremental improvement.

@chrisjrn
Copy link
Contributor Author

@Eric-Arellano I'm working on pulling out the parts of the error message next

Christopher Neugebauer added 4 commits August 23, 2021 09:43
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
…ecked

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@chrisjrn
Copy link
Contributor Author

@Eric-Arellano See further commits for updated error messages

Christopher Neugebauer added 2 commits August 23, 2021 12:06
…d-lockfile-messages

[ci skip-rust]

[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Sweet! Thanks for taking the time to capture why specifically the lockfile was incorrect. This looks great.

I think a later followup can improve this even more by having call sites put things in user terms. Rather than "update your interpreter constraints", we can say "update your interpreter constraints by setting [black].interpreter_constraints. That will hurt code elegance, but we value better error messages more than that. But, call sites will keep changing so much the next week that I think it's fine to punt on this.

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?

src/python/pants/backend/python/util_rules/pex_test.py Outdated Show resolved Hide resolved
src/python/pants/backend/python/util_rules/pex_test.py Outdated Show resolved Hide resolved
Comment on lines 523 to 531
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

src/python/pants/backend/python/util_rules/pex.py Outdated Show resolved Hide resolved
Christopher Neugebauer added 2 commits August 25, 2021 14:27
…d-lockfile-messages

# Conflicts:
#	src/python/pants/backend/python/util_rules/pex_test.py
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@chrisjrn
Copy link
Contributor Author

@Eric-Arellano Finally got this PR green.

Comment on lines +504 to +512
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")
Copy link
Contributor

Choose a reason for hiding this comment

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

My previous comment isn't showing up anymore:

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.

I used append because the strings had been bumping up against the line length limit, so using append resulted in more compact code in the end.

Christopher Neugebauer added 3 commits August 26, 2021 12:12
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@Eric-Arellano Eric-Arellano merged commit 2a3e950 into pantsbuild:main Aug 27, 2021
@illicitonion illicitonion mentioned this pull request Aug 30, 2021
illicitonion added a commit to illicitonion/pants that referenced this pull request Aug 30, 2021
* Clean up the config parsing code. ([pantsbuild#12678](pantsbuild#12678))

* [internal] `--generate-lockfiles-resolve` does not determine `PythonLockfileRequest` for unspecified tools ([pantsbuild#12692](pantsbuild#12692))

* [internal] Add missing `resources` targets for default tool lockfiles ([pantsbuild#12670](pantsbuild#12670))

* [internal] Reorganize lockfile code to no longer be in `backend/python/experimental` ([pantsbuild#12669](pantsbuild#12669))

* [internal] Test that default tool lockfiles work on macOS ([pantsbuild#12666](pantsbuild#12666))

* [internal] Improve the error message for stale/invalid lockfiles ([pantsbuild#12618](pantsbuild#12618))

* [internal] Rename `./pants lock` to `./pants generate-user-lockfile` and `./pants tool-lock` to `./pants generate-lockfiles` ([pantsbuild#12641](pantsbuild#12641))

* [internal] Only run macOS tests in CI annotated with `@pytest.mark.platform_specific_behavior` ([pantsbuild#12665](pantsbuild#12665))

* [internal] Test that Python tools work with all expected interpreter versions ([pantsbuild#12656](pantsbuild#12656))

* [internal] Factor out a helper rule for setting up `--resolve-all-constraints` ([pantsbuild#12630](pantsbuild#12630))

* [internal] Add links to changelog and Twitter in Pants PyPI project page. ([pantsbuild#12636](pantsbuild#12636))

* [internal] Update CoC with proper markdown + https links. ([pantsbuild#12637](pantsbuild#12637))

* GitHub issue templates ([pantsbuild#12617](pantsbuild#12617))

* Register subsystems and target types by backend/plugin. ([pantsbuild#12623](pantsbuild#12623))

* [internal] Sort input requirements for `LockfileMetadata` ([pantsbuild#12615](pantsbuild#12615))

* [internal] Expect lockfile metadata to be defined ([pantsbuild#12616](pantsbuild#12616))

* Simplify subsystem registration. ([pantsbuild#12620](pantsbuild#12620))

* [internal] Use constants for magic strings `<none>` and `<default>` for tool lockfiles ([pantsbuild#12613](pantsbuild#12613))

* [internal] Refactor `lockfile_metadata.py` to be more class-based ([pantsbuild#12611](pantsbuild#12611))

* Embrace that help strings are markdown. ([pantsbuild#12606](pantsbuild#12606))

* Stop fetching a lockfile request just to figure out the requirements digest ([pantsbuild#12607](pantsbuild#12607))

* [internal] Refactor MyPy first party plugins ([pantsbuild#12599](pantsbuild#12599))

* [internal] Refactor MyPy config file setup ([pantsbuild#12593](pantsbuild#12593))

* [internal] use `is_union()` rather than `union.is_instance()`. ([pantsbuild#12595](pantsbuild#12595))

* fs_util can use mTLS and ignore certificates ([pantsbuild#12586](pantsbuild#12586))

* [internal] Fix Pylint lockfile to actually be wired up ([pantsbuild#12590](pantsbuild#12590))

* [internal] Update lockfile TODOs with current state of project ([pantsbuild#12592](pantsbuild#12592))

* [internal] Check for lockfile staleness by using context's interpreter constraints, rather than global constraints  ([pantsbuild#12566](pantsbuild#12566))

* [internal] Refactor Pylint first-party plugins ([pantsbuild#12583](pantsbuild#12583))

* [internal] Fix Pytest and IPython to check for stale lockfiles ([pantsbuild#12582](pantsbuild#12582))

* [internal] Do not cache lockfile generation ([pantsbuild#12674](pantsbuild#12674))
illicitonion added a commit that referenced this pull request Aug 30, 2021
* Clean up the config parsing code. ([#12678](#12678))

* [internal] `--generate-lockfiles-resolve` does not determine `PythonLockfileRequest` for unspecified tools ([#12692](#12692))

* [internal] Add missing `resources` targets for default tool lockfiles ([#12670](#12670))

* [internal] Reorganize lockfile code to no longer be in `backend/python/experimental` ([#12669](#12669))

* [internal] Test that default tool lockfiles work on macOS ([#12666](#12666))

* [internal] Improve the error message for stale/invalid lockfiles ([#12618](#12618))

* [internal] Rename `./pants lock` to `./pants generate-user-lockfile` and `./pants tool-lock` to `./pants generate-lockfiles` ([#12641](#12641))

* [internal] Only run macOS tests in CI annotated with `@pytest.mark.platform_specific_behavior` ([#12665](#12665))

* [internal] Test that Python tools work with all expected interpreter versions ([#12656](#12656))

* [internal] Factor out a helper rule for setting up `--resolve-all-constraints` ([#12630](#12630))

* [internal] Add links to changelog and Twitter in Pants PyPI project page. ([#12636](#12636))

* [internal] Update CoC with proper markdown + https links. ([#12637](#12637))

* GitHub issue templates ([#12617](#12617))

* Register subsystems and target types by backend/plugin. ([#12623](#12623))

* [internal] Sort input requirements for `LockfileMetadata` ([#12615](#12615))

* [internal] Expect lockfile metadata to be defined ([#12616](#12616))

* Simplify subsystem registration. ([#12620](#12620))

* [internal] Use constants for magic strings `<none>` and `<default>` for tool lockfiles ([#12613](#12613))

* [internal] Refactor `lockfile_metadata.py` to be more class-based ([#12611](#12611))

* Embrace that help strings are markdown. ([#12606](#12606))

* Stop fetching a lockfile request just to figure out the requirements digest ([#12607](#12607))

* [internal] Refactor MyPy first party plugins ([#12599](#12599))

* [internal] Refactor MyPy config file setup ([#12593](#12593))

* [internal] use `is_union()` rather than `union.is_instance()`. ([#12595](#12595))

* fs_util can use mTLS and ignore certificates ([#12586](#12586))

* [internal] Fix Pylint lockfile to actually be wired up ([#12590](#12590))

* [internal] Update lockfile TODOs with current state of project ([#12592](#12592))

* [internal] Check for lockfile staleness by using context's interpreter constraints, rather than global constraints  ([#12566](#12566))

* [internal] Refactor Pylint first-party plugins ([#12583](#12583))

* [internal] Fix Pytest and IPython to check for stale lockfiles ([#12582](#12582))

* [internal] Do not cache lockfile generation ([#12674](#12674))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve error/warning messages when consuming invalid lockfiles.
2 participants