Lockfile invalidation consumption #12448

chrisjrn · 2021-07-28T21:37:43Z

Addresses #12415

This adds tools for consuming the lockfile invalidation digests that were added in #12427, both for lockfiles specified for the project, and for individual build tools.

Behavior on invalid lockfiles can be configured to either be a warning or an error.

src/python/pants/backend/python/subsystems/python_tool_base.py

src/python/pants/backend/python/util_rules/pex.py

src/python/pants/python/python_setup.py

src/python/pants/backend/experimental/python/lockfile_metadata.py

[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]

[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]

[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]

[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]

[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]

chrisjrn · 2021-08-10T16:50:26Z

@Eric-Arellano this is ready for review now

# 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

Awesome change!

Eric-Arellano · 2021-08-10T19:17:12Z

src/python/pants/backend/awslambda/python/rules.py

+    lockfile_digest = None
+    if lambdex.lockfile != "<none>":
+        lockfile_request = await Get(PythonLockfileRequest, LambdexLockfileSentinel())
+        lockfile_digest = lockfile_request.hex_digest


Maybe lockfile_hash? Now that I see this in an @rule, the word "digest" is overloaded to think "directory digest" from pants.engine.fs.

I think this could get really confusing, e.g. people accidentally using it with MergeDigests

I still think that throwing around the word hash in a Python database is likely to bring up similar confusion. That said, these lines from our JVM support make digest on its own a non-starter for our lockfile support.

wheeeeeeeeeeeee

Oh yeah true. Then, maybe being explicit is the key. invalidation_digest or invalidation_hash, but never just hash or just digest.

I'll go with hex_digest, since that's what I'm using elsewhere.

Eric-Arellano · 2021-08-10T19:21:31Z

src/python/pants/backend/python/util_rules/pex.py

+                raise ValueError("Invalid lockfile provided. [TODO: Improve message]")
+            else:
+                logger.warning("%s", "Invalid lockfile provided. [TODO: Improve message]")


Did you mean to address this here, or it will be a followup?

It wasn't clear to me how to provide a really meaningful error here. I'll discuss this with you out-of-band

Sg! Because this is experimental, it's fine to land in this state imo.

Great. Let's follow up in a separate PR.

Eric-Arellano · 2021-08-10T19:22:34Z

src/python/pants/backend/python/util_rules/pex.py

        requirements_file_digest = await Get(
            Digest,
-            PathGlobs(
-                [request.requirements.file_path],
-                glob_match_error_behavior=GlobMatchErrorBehavior.error,
-                description_of_origin=request.requirements.file_path_description_of_origin,
-            ),
+            PathGlobs,
+            globs,


FYI this is split over 3 lines because there was a trailing comma so Black forces it to be split. You can get back to one line by removing the comma and rerunning Black.

You don't need to fix as that's what Black did, only an FYI. (can't remember if I've mentioned this already)

Eric-Arellano · 2021-08-10T19:25:12Z

src/python/pants/backend/python/util_rules/pex.py

+                raise ValueError("Invalid lockfile provided. [TODO: Improve message]")
+            else:
+                logger.warning("%s", "Invalid lockfile provided. [TODO: Improve message]")


We will likely want this error message to differ from the one above. This is currently always the <default> lockfile, whereas the above are always lockfiles checked in by the user. Our guidance is different depending on those two edge cases.

To formalize that implicit association, we may in a followup want to change PexRequirements to be less generic than it is now, e.g. change the field to be "default_requirements_file: FileContentrather thanfile_content: FileContent`.

Addressing in a follow-up

Okay, fair enough. I do think we have some tests using this style out there and iirc Benjy prefers your style - not blocking

Eric-Arellano · 2021-08-10T19:27:35Z

src/python/pants/backend/python/util_rules/pex_test.py

@@ -535,3 +536,67 @@ def assert_description(
        use_repo_pex=True,
        expected="Extracting all requirements in lock.txt from repo.pex to build new.pex",
    )
+
+
+def test_error_on_invalid_lockfile_with_path(rule_runner: RuleRunner) -> None:


Subjective style suggestion, feel free to disagree. It may be more readable to have this in a single test that puts _run_pex_for_lockfile_test as an inner helper function. These are all the same functionality, so I think makes sense to be in the same test as they're simply different edge cases of the same functionality. Kind of like pytest.parametrize.

I thought about doing it that way, but my preferred behaviour is to match what pytest.parametrize does, which is to run every case and report every failure. Each one of these tests is running a completely different code path, and the behaviour has to be tested in a different way, otherwise I'd have done it with parametrize

src/python/pants/backend/python/util_rules/pex_test.py

src/python/pants/python/python_setup.py

Eric-Arellano · 2021-08-10T19:32:51Z

src/python/pants/python/python_setup.py

+            "--invalid-lockfile-behavior",
+            advanced=True,
+            type=InvalidLockfileBehavior,
+            default=InvalidLockfileBehavior.warn,


I think we talked about this a couple times, what the default should be. I can't remember what the conclusion was, but I'm thinking we should probably default to error for now, at least until we get feedback from users it's an annoying default. We want to encourage people to do the right thing, and warnings are too easy to miss (e.g. in CI).

Lockfiles are feature gated, so this is fine to change.

Fine by me.

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>

# 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

Thanks!

Eric-Arellano · 2021-08-10T20:42:23Z

src/python/pants/backend/python/util_rules/pex.py

+                raise ValueError("Invalid lockfile provided. [TODO: Improve message]")
+            else:
+                logger.warning("%s", "Invalid lockfile provided. [TODO: Improve message]")


Okay, fair enough. I do think we have some tests using this style out there and iirc Benjy prefers your style - not blocking

Eric-Arellano · 2021-08-11T22:59:32Z

Needs a new commit because of GitHub outage yesterday

# 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]

Turns out our lockfiles aren't quite valid: https://github.com/pantsbuild/pants/pull/12448/checks # 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]

stuhood

The implementation looks good, but there is a lot of redundancy for consumers. It will be important to make doing the right thing easy, so it's possible that fixing some of that redundancy before landing would be good.

stuhood · 2021-08-12T17:40:35Z

src/python/pants/backend/awslambda/python/rules.py

@@ -86,10 +87,15 @@ async def package_python_awslambda(
        ],
    )

+    lockfile_hex_digest = None
+    if lambdex.lockfile != "<none>":


This magic string ended up peppered a lot of places. Would be good to have def lockfile convert "<none>" to str | None instead.

Imo fine to do in a followup. This code path is still changing frequently.

stuhood · 2021-08-12T17:42:32Z

src/python/pants/backend/codegen/protobuf/python/rules.py

+    if python_protobuf_mypy_plugin.lockfile != "<none>":
+        lockfile_request = await Get(PythonLockfileRequest, MypyProtobufLockfileSentinel())
+        lockfile_hex_digest = lockfile_request.hex_digest


This code is very redundant...

Could PexRequest take the sentinel type + lockfile and make this request for you instead? Or could the sentinel type be converted into a wrapper around the filename (i.e., no longer just a sentinel)?

Or, more fundamentally, should PexRequest.requirements become a union of PexRequirements | PythonToolRequirementsBase?

I think that we will absolutely want to fix this, but it's fine in a followup.

Yeah I recommend we wait to fix this. For example, #12542 will make lots of changes to these call sites.

Yeah, I don't particularly like this code, and I'd be happy to take suggestions on how to make the improvement.

# 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]

These were missed in #12448. To reduce the risk of this happening again, we remove the default argument for `PythonToolBase.pex_requirements(expected_lockfile_hex_digest)`. In the process, I realized that Pylint is not actually consuming it's lockfile at all because of how we handle first-party plugins. I'll address that in a followup that adds lockfile support to MyPy. [ci skip-rust] [ci skip-build-wheels]

chrisjrn marked this pull request as draft July 28, 2021 21:37

Eric-Arellano reviewed Jul 28, 2021

View reviewed changes

src/python/pants/backend/python/subsystems/python_tool_base.py Outdated Show resolved Hide resolved

src/python/pants/backend/python/util_rules/pex.py Outdated Show resolved Hide resolved

Eric-Arellano reviewed Jul 28, 2021

View reviewed changes

src/python/pants/python/python_setup.py Outdated Show resolved Hide resolved

jsirois reviewed Jul 29, 2021

View reviewed changes

src/python/pants/backend/experimental/python/lockfile_metadata.py Outdated Show resolved Hide resolved

src/python/pants/backend/experimental/python/lockfile_metadata.py Outdated Show resolved Hide resolved

chrisjrn force-pushed the chrisjrn/12415-invalidation-consumption branch 3 times, most recently from 27f13b1 to 020777d Compare July 30, 2021 15:45

chrisjrn marked this pull request as ready for review July 30, 2021 15:46

chrisjrn marked this pull request as draft July 30, 2021 15:46

Christopher Neugebauer added 18 commits August 9, 2021 08:51

WIP Consumption of lockfiles

9b82d1f

[ci skip-rust] [ci skip-build-wheels]

Adds file paths

e43b451

# 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]

adds lockfile hashes for consumption to every tool that uses lockfiles

a5abeb5

[ci skip-rust] [ci skip-build-wheels]

Adds python-setup-fail-on-invalid-lockfile

561eeda

[ci skip-rust] [ci skip-build-wheels]

Adds invalidation behavior to pex.py

1489186

# 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]

Adds dataclass for lockfile metadata

74f246f

[ci skip-rust] [ci skip-build-wheels]

Fmt

e9ab314

# 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]

Factors out invalidation_digest for reasons

2c24ef1

[ci skip-rust] [ci skip-build-wheels]

Adds lockfile invalidation to pex_from_targets.py

23d3f8e

# 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]

Converts to a calculated property based on

085db09

# 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]

pex_requirements_with_digest -> pex_requirements

6c628df

# 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]

Add new lockfile rules to integration tests

ef7825f

[ci skip-rust] [ci skip-build-wheels]

Stops resolving lockfiles when one isn't there.

cbac8bb

# 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]

Update invalid lockfile behavior switch

62c1684

# 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]

Fixed missing pex_requirements call in pytest_runner

4a1534d

# 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]

Fix mypy integration test rules

a3802a1

# 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]

fix mypy integration test rules

a09d94f

# 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]

fmt after rebase

0d691ad

chrisjrn force-pushed the chrisjrn/12415-invalidation-consumption branch from ed70791 to 0d691ad Compare August 9, 2021 16:48

Christopher Neugebauer added 2 commits August 9, 2021 15:43

Adds tests for invalid lockfile behaviour

9de8e3c

# 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]

Add tests for lockfile invalidation switches

7e81613

# 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]

Adds lockfile switch tests based on file content, not file path

d932174

# 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 marked this pull request as ready for review August 10, 2021 16:50

chrisjrn changed the title ~~[WIP] lockfile invalidation consumption~~ Lockfile invalidation consumption Aug 10, 2021

Christopher Neugebauer added 2 commits August 10, 2021 11:50

removes interpreter constraints

5e83830

# 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]

Add retry count to try_with_backoff

1abb5ff

# 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 approved these changes Aug 10, 2021

View reviewed changes

Eric-Arellano requested a review from stuhood August 10, 2021 19:35

Christopher Neugebauer and others added 3 commits August 10, 2021 13:06

Apply suggestions from code review

6a7d876

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>

Change defaults for invalid lockfiles

27b4eea

# 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]

s/lockfile_digest/lockfile_hex_digest/

464e252

# 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 approved these changes Aug 10, 2021

View reviewed changes

chrisjrn mentioned this pull request Aug 10, 2021

Improve error/warning messages when consuming invalid lockfiles. #12541

Closed

Eric-Arellano mentioned this pull request Aug 10, 2021

Lockfiles: when checking for staleness, only use current context's interpreter constraints #12542

Closed

Christopher Neugebauer added 2 commits August 12, 2021 07:56

Commit to re-run CI

dca1bb0

# 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 mentioned this pull request Aug 12, 2021

Set default invalid lockfile behaviour to error #12552

Closed

stuhood approved these changes Aug 12, 2021

View reviewed changes

Adds ignore to InvalidLockfileBehavior enum; uses it.

ac92d19

# 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 approved these changes Aug 12, 2021

View reviewed changes

Eric-Arellano merged commit 4ad7b46 into pantsbuild:main Aug 12, 2021

stuhood mentioned this pull request Aug 14, 2021

Prepare 2.7.0.dev4 #12570

Merged

Eric-Arellano mentioned this pull request Aug 16, 2021

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lockfile invalidation consumption #12448

Lockfile invalidation consumption #12448

chrisjrn commented Jul 28, 2021 •

edited

Loading

chrisjrn commented Aug 10, 2021

Eric-Arellano left a comment

Eric-Arellano Aug 10, 2021

chrisjrn Aug 10, 2021

Eric-Arellano Aug 10, 2021

chrisjrn Aug 10, 2021

Eric-Arellano Aug 10, 2021

chrisjrn Aug 10, 2021

Eric-Arellano Aug 10, 2021

chrisjrn Aug 10, 2021

Eric-Arellano Aug 10, 2021

chrisjrn Aug 10, 2021

Eric-Arellano Aug 10, 2021

chrisjrn Aug 10, 2021

Eric-Arellano Aug 10, 2021

Eric-Arellano Aug 10, 2021

chrisjrn Aug 10, 2021

Eric-Arellano Aug 10, 2021

chrisjrn Aug 10, 2021

Eric-Arellano left a comment

Eric-Arellano Aug 10, 2021

Eric-Arellano commented Aug 11, 2021

stuhood left a comment

stuhood Aug 12, 2021

Eric-Arellano Aug 12, 2021

stuhood Aug 12, 2021

stuhood Aug 12, 2021

Eric-Arellano Aug 12, 2021

chrisjrn Aug 12, 2021

Lockfile invalidation consumption #12448

Lockfile invalidation consumption #12448

Conversation

chrisjrn commented Jul 28, 2021 • edited Loading

chrisjrn commented Aug 10, 2021

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano commented Aug 11, 2021

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjrn commented Jul 28, 2021 •

edited

Loading