[internal] Expect lockfile metadata to be defined #12616

Eric-Arellano · 2021-08-20T17:53:04Z

For now, we should expect that the lockfile metadata is well-formed and present. This allows us to simplify some of the code.

Prework for #12610.

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

Eric-Arellano · 2021-08-20T17:54:04Z

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

            try:
-                return t(v) if v is not None else None
-            except Exception:
-                # TODO: this should trigger error/warning behavior


I think it should always be an error if the metadata is malformed, rather than a warning. That [python-setup] option imo is only whether to warn vs error if the metadata is stale, but not if the metadata is missing or malformed.

chrisjrn

This definitely improves the LockfileMetadata class semantics. Thank you!

Stylistic gripes follow:

chrisjrn · 2021-08-20T18:25:54Z

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

-        def coerce(t: Callable[[Any], T], key: str) -> T | None:
-            """Gets a value from `metadata`, coercing it to type `t` if not `None`."""
-            v = metadata.get(key, None)
+        def get(key: str) -> Any:


I think having this function here reduces clarity somewhat -- note that both the isinstance check and the InterpreterConstraints constructor will fail in a useful way if the variable is None, so you could just use metadata.get(key) here:

hex_digest = metadata.get("requirements_invalidation_digest") if not isinstance(hex_digest, str): raise InvalidLockfileError("")

Good point!

I forgot to leave a comment explaining btw why I got rid of coerce. With the upcoming "requirements are compatible, not identical" proposal, the method wasn't flexible enough to convert into list[Requirement].

Ah wait, nvm. I remember why I had this helper function: I want the error to be specifically about the key missing. Vs an error for the value being malformed / wrong type.

Suggestions on a better name for the helper function?

coerce((lambda requirements: [Requirement(i) for i in requirements]), … works just fine, tbqh :)

get_or_raise would do fine if you really want to keep the method here.

I suspect, given the work in #12612, we're going to want to just metadata[key] natively and have different code for requirements (which must be there) vs InterpreterConstraints/Platforms where 1 and only 1 must be present.

where 1 and only 1 must be present.

Only 1 must be defined, but I think we may want both present. I.e. keys vs values. As discussed in DM, I think there's value to seeing platforms: null because it gives a cue that the lockfile is platform-agnostic.

chrisjrn · 2021-08-20T18:34:08Z

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

+                # TODO(#12314): Add a good error.
+                raise InvalidLockfileError("")
+
+        digest = get("requirements_invalidation_digest")


Given the rest of the discussions we've had about confusion around this name, it's probably best that this is called hex_digest

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

* 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))

* 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))

[internal] Require lockfile metadata to be defined

e117e18

# 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 requested review from chrisjrn and tdyas August 20, 2021 17:53

Eric-Arellano commented Aug 20, 2021

View reviewed changes

chrisjrn approved these changes Aug 20, 2021

View reviewed changes

Rename some vars and improve TODOs

71f1e95

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

View reviewed changes

Eric-Arellano merged commit a4713a6 into pantsbuild:main Aug 23, 2021

Eric-Arellano deleted the require-metadata branch August 23, 2021 17:39

illicitonion mentioned this pull request Aug 30, 2021

Prep for 2.7.0rc0 #12695

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[internal] Expect lockfile metadata to be defined #12616

[internal] Expect lockfile metadata to be defined #12616

Eric-Arellano commented Aug 20, 2021

Eric-Arellano Aug 20, 2021

chrisjrn left a comment

chrisjrn Aug 20, 2021

Eric-Arellano Aug 20, 2021

Eric-Arellano Aug 20, 2021

chrisjrn Aug 20, 2021

chrisjrn Aug 20, 2021

Eric-Arellano Aug 20, 2021 •

edited

Loading

chrisjrn Aug 20, 2021

[internal] Expect lockfile metadata to be defined #12616

[internal] Expect lockfile metadata to be defined #12616

Conversation

Eric-Arellano commented Aug 20, 2021

Eric-Arellano Aug 20, 2021

Choose a reason for hiding this comment

chrisjrn left a comment

Choose a reason for hiding this comment

chrisjrn Aug 20, 2021

Choose a reason for hiding this comment

Eric-Arellano Aug 20, 2021

Choose a reason for hiding this comment

Eric-Arellano Aug 20, 2021

Choose a reason for hiding this comment

chrisjrn Aug 20, 2021

Choose a reason for hiding this comment

chrisjrn Aug 20, 2021

Choose a reason for hiding this comment

Eric-Arellano Aug 20, 2021 • edited Loading

Choose a reason for hiding this comment

chrisjrn Aug 20, 2021

Choose a reason for hiding this comment

Eric-Arellano Aug 20, 2021 •

edited

Loading