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

[internal] Replace apply_constraints with an explicit list of constraints. #14064

Conversation

stuhood
Copy link
Sponsor Member

@stuhood stuhood commented Jan 4, 2022

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

To fix this, replace apply_constraints: bool (for the global constraints) with an explicit list of constraints.

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

@stuhood stuhood force-pushed the stuhood/pass-explicit-constraints-in-pex-requirements branch from 9ef02b2 to 6eb21a2 Compare January 4, 2022 18:31
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Good point

@@ -219,7 +219,8 @@ async def pylint_first_party_plugins(pylint: Pylint) -> PylintFirstPartyPlugins:

return PylintFirstPartyPlugins(
requirement_strings=PexRequirements.create_from_requirement_fields(
requirements_fields
requirements_fields,
constraints_strings=(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

followup to #14058 (comment). Okay sounds good to use constraints on first-party plugins for MyPy, I was misreading the code thinking it was for generating the tool lockfile.

We should also update these Pylint plugins to use it too.

Copy link
Sponsor Member Author

@stuhood stuhood Jan 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Eric-Arellano : You were right: see my comment there. PexRequirements.create_from_requirement_fields is only actually being used to gather the requirement strings, so the constraints are not consumed. I've added a top commit here fixing mypy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks! So should it still be an explicit arg for constraints_string?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so... in this case, it calls out that we're (ab)using PexRequirements to gather the requirements rather than to (directly) create a PEX. In any other position you'd want to consider whether to apply the constraints.

Once lockfiles roll out, I think that our current plan will be to remove the constraints...? But they certainly won't be a global singleton anymore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once lockfiles roll out, I think that our current plan will be to remove the constraints...? But they certainly won't be a global singleton anymore.

Yeah, at least for 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]
@stuhood stuhood enabled auto-merge (squash) January 4, 2022 18:41
@stuhood stuhood merged commit 0b1ef20 into pantsbuild:main Jan 4, 2022
@stuhood stuhood deleted the stuhood/pass-explicit-constraints-in-pex-requirements branch January 4, 2022 19:16
stuhood added a commit that referenced this pull request Jan 4, 2022
…ugin resolution (#14058)

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]
@stuhood stuhood mentioned this pull request Jan 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants