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

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

Closed
Eric-Arellano opened this issue Aug 10, 2021 · 6 comments · Fixed by #12566
Assignees

Comments

@Eric-Arellano
Copy link
Contributor

Background

For tool lockfiles, #12448 uses the rules to get the PythonLockfileRequest for the tool to check if the current lockfile has become stale.

Some tool lockfile requests are tricky to set up because interpreter constraints depend on the user's code, such as how the ICs used for Flake8 and Pytest depend on what the user has. This has a severe performance impact: when running any of these tools, like running tests on a single file, we must first check if the lockfile is stale by consulting the entire repository to determine its interpreter constraints. That is slow to scan/compute, and means tests will be invalidated much more infrequently.

Proposal

Instead, when checking a lockfile for staleness, we can simply check that the current context's interpreter constraints are compatible with the lockfile's constraints, rather than identical. This would avoid entirely the performance concern.

Tips for implementation

The main challenge will be adding code to interpreter_constraints.py to compare if two interpreter constraints are compatible with each other or not: checking if one is a subset of the other.

Likely the easiest way to implement this is to leverage that we have users define for us already what the finite universe of Python interpreters should be, e.g. Pythons 3.5-3.10. You can use this function to generate all possible versions:

def _valid_patch_versions(self, major: int, minor: int) -> Iterator[int]:
for p in range(0, _EXPECTED_LAST_PATCH_VERSION + 1):
for req in self:
if req.specifier.contains(f"{major}.{minor}.{p}"): # type: ignore[attr-defined]
yield p

(Even better, a PR I'm about to put up to add InterpreterConstraints.flatten adds a PR that computes all valid Py2 and Py3 versions for the ICs. You can use that.)

Then, use set methods to check that one is a subset of the other.

--

We'll also need to refactor #12448 to no longer use the rule to determine the lockfile request, but instead compute what the request would be for that current context (e.g. lockfile for just that test).

The lockfile header will also probably need to start preserving the original interpreter_constraints string, and we'll need a way to serialize/deserialize that.

@chrisjrn
Copy link
Contributor

I'm trying to figure out what "compatible" means here.

The definition I'm thinking of says:

  1. The user's constraints, and the lockfile's constraints are non-disjoint (i.e. lockable_constraints := (user_constraints & lockfile_constraints) != EMPTY)
  2. lockable_constraints matches at least one interpreter that is available on the current machine

Does that line up with your understanding of the constraints?

@Eric-Arellano
Copy link
Contributor Author

I was thinking the mathematical definition of subset: the context's ICs must be completely contained by the lockfiles's ICs. That is, every patch version in the context must be in the lockfile's possible patch versions.

It need not be a "proper subset", it's valid if the context ICs == lockfile ICs.

@chrisjrn
Copy link
Contributor

@Eric-Arellano Thoughts on including platforms as part of the requirements here? Or is that for a later date?

@Eric-Arellano
Copy link
Contributor Author

If we go with Poetry, then it doesn't matter what platform you generate from. The lockfile should handle windows, Linux, and macOS. No need to invalidate based on platform.

(Still thinking about John's question on handling --platform when set in a pex_binary. Will put up some thoughts, but tldr is I think this issue will not need to do anything.)

@chrisjrn
Copy link
Contributor

Cool, I'll ignore that for now.

@chrisjrn
Copy link
Contributor

(Completion of this task is blocked on merging #12448)

Eric-Arellano pushed a commit that referenced this issue Aug 17, 2021
…r constraints, rather than global constraints (#12566)

Part 2 of 3 for #12542.

Still to do: update call sites to leverage this change so that they no longer compute their global constraints.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants