Allow plugins to use range requirements by applying constraints to plugin resolution #14058
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Commits are useful to review independently. This still needs a test: ideas welcome. I started to refactor the
|
jsirois
approved these changes
Jan 4, 2022
Yea, reasonable. |
stuhood
force-pushed
the
stuhood/pants-requirements-as-constraints
branch
from
b649d7c
to
b02028b
Compare
This comment has been minimized.
This comment has been minimized.
stuhood
added a commit
to stuhood/pants
that referenced
this pull request
Jan 4, 2022
# 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
added a commit
that referenced
this pull request
Jan 4, 2022
…raints. (#14064) Some callers (#14058) of `PexRequirements` will soon need to be able to specify explicit constraints for building a PEX which are _not_ the global constraints. Additionally, consuming the `constraints` directly as a file (rather than parsing and re-materializing the file) forces whitespace/comment edits to the constraints file to invalidate all resolves ([reported here](pantsbuild/requirements-perf#1 (comment))). To fix this, replace `apply_constraints: bool` (for the global constraints) with an explicit list of constraints. [ci skip-rust] [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]
…ement is constrained by the working set. # 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
force-pushed
the
stuhood/pants-requirements-as-constraints
branch
from
b02028b
to
714a4da
Compare
Eric-Arellano
approved these changes
Jan 4, 2022
| interpreter_constraints: InterpreterConstraints | None | ||
| # Requirement constraints to resolve with. If plugins will be loaded into the global working_set | ||
| # (i.e., onto the `sys.path`), then these should be the current contents of the working_set. | ||
| constraints: tuple[Requirement, ...] |
There was a problem hiding this comment.
Should this be our PipRequirement?
There was a problem hiding this comment.
Maybe...? But it doesn't look like that class does anything right now. cc @benjyw
I think that can wait until the class is differentiated in some way, at which point the type system will be able to guide us through replacement fairly easily.
There was a problem hiding this comment.
The idea is to allow you to have optional args like --hash be preserved.
There was a problem hiding this comment.
Sure... but it doesn't do that right now.
This was referenced Jan 5, 2022
|
I've added https://www.pantsbuild.org/v2.9/docs/plugins-overview#publishing-a-plugin to document this support. |
|
Looks great, thanks! |
stuhood
added a commit
that referenced
this pull request
Jan 28, 2022
As described in #14246, #14058 caused plugin loading to run twice when we missed the cache. This was because after loading plugins, the working set has changed, and represents a new set of constraints on plugin loading. While the current behavior is accurate, it isn't useful. Instead, we adjust `PluginResolver` to make the inputs to plugin loading idempotent, even if the outputs cannot be. Fixes #14246. [ci skip-rust]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As discussed in #14053, currently if a plugin uses a range requirement (even if it matches the requirement used by Pants itself), a different version might be chosen than what is already installed in the virtualenv for Pants, triggering a conflict when we try to add it to the working set (i.e.
sys.path).This change uses the current working set to constrain the plugin resolve, so that plugins with range requirements will choose matching versions from those ranges (if possible). To do that, it consumes #14064 to specify the working set as constraints in the
PluginResolver.Fixes #14053.
[ci skip-rust]