[internal] Sort input requirements for `LockfileMetadata` #12615

Eric-Arellano · 2021-08-20T17:14:19Z

This ensures that callers can't mess up the determinism of the lockfile's invalidation digest. It will also make #12610 more presentable for users, when we store the requirement strings in the lockfile rather than just a hash.

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

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

src/python/pants/backend/experimental/python/lockfile_metadata.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

Looks good to me; stylistic gripes follow:

chrisjrn · 2021-08-20T18:39:58Z

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

@@ -113,7 +114,7 @@ def calculate_invalidation_digest(requirements: Iterable[str]) -> str:
    """Returns an invalidation digest for the given requirements."""
    m = hashlib.sha256()
    inputs = {
-        "requirements": list(requirements),
+        "requirements": sorted(FrozenOrderedSet(requirements)),


I think we might be able to safely leave this as a set() and save the import: sorted takes care of the ordering for us.

I should comment why I did this: I wanted to avoid set because it would break if the input was already ordered. Python uses timsort https://en.wikipedia.org/wiki/Timsort, which is O(n) in the best case if it's already sorted, and O(n logn) in the worst case.

two points:

set is guaranteed to be ordered as of Python 3.6, so running sorted on a set is equivalent to running it on a list.

n is quite small for requirements, so if (1) weren't true, it probably wouldn't matter

dict is ordered, set is not. (We really on dict's ordering to implement FrozenOrderedSet)

🤦 fine :)

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

chrisjrn · 2021-08-20T18:46:11Z

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

    ],
 )
 def test_invalidation_digest(requirements, expected) -> None:
-    assert calculate_invalidation_digest(FrozenOrderedSet(requirements)) == expected
+    a = "flake8-pantsbuild>=2.0,<3"


Stylistically, you can re-write this test in a parametrized way by using the params:

def test_invalidation_digest(reqs_1, reqs_2, are_equivalent): assert (calculate_invalidation_digest(reqs_1) == calculate_invalidation_digest(reqs_2)) == are_equivalent

That will give you the benefit of the test runner showing all failure cases if things start going wrong, otherwise a failure early on hides the extent of things being broken.

If you're worried about the bit where you generate test cases using a for loop, note that the arguments to parametrize could be generated by another function.

Ack that's possible. Imo, it's simpler to have this current style, e.g. the variables a, b, and c being locally scoped. Keep it simple here.

# 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] Sort input requirements for LockfileMetadata

740efa0

# 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:14

chrisjrn requested changes Aug 20, 2021

View reviewed changes

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

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

Eric-Arellano added 2 commits August 20, 2021 10:45

Review feedback

3912450

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

Leverage new type hint

abba14c

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

View reviewed changes

More review feedback

e3ab203

# 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 b37ef21 into pantsbuild:main Aug 23, 2021

Eric-Arellano deleted the sort-requirements branch August 23, 2021 17:45

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] Sort input requirements for `LockfileMetadata` #12615

[internal] Sort input requirements for `LockfileMetadata` #12615

Eric-Arellano commented Aug 20, 2021

chrisjrn left a comment

chrisjrn Aug 20, 2021

Eric-Arellano Aug 20, 2021

chrisjrn Aug 20, 2021

Eric-Arellano Aug 20, 2021

chrisjrn Aug 20, 2021

chrisjrn Aug 20, 2021

Eric-Arellano Aug 20, 2021

[internal] Sort input requirements for LockfileMetadata #12615

[internal] Sort input requirements for LockfileMetadata #12615

Conversation

Eric-Arellano commented Aug 20, 2021

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

chrisjrn 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

Choose a reason for hiding this comment

[internal] Sort input requirements for `LockfileMetadata` #12615

[internal] Sort input requirements for `LockfileMetadata` #12615