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

Support newer pip versions. #17555

Merged
merged 3 commits into from
Nov 16, 2022
Merged

Support newer pip versions. #17555

merged 3 commits into from
Nov 16, 2022

Conversation

benjyw
Copy link
Contributor

@benjyw benjyw commented Nov 16, 2022

Plumbs --pip-version through to pex commands that support it.

Newer pips have perf improvements that may benefit users, in particular for very long-running resolves.

Does not change the default. We can do that via deprecation in a followup.

Plumbs --pip-version through to pex commands that support it.

Newer pips have perf improvements that may benefit users,
in particular for very long-running resolves.
@@ -180,6 +187,11 @@ class PythonSetup(Subsystem):
"""
),
)
pip_version = EnumOption(
default=PipVersion.V22_3,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is potentially controversial, as we're also upgrading everyone to newer pip, rather than making it opt-in.

I think that's fine, since pip is very heavily tested, and ubiquitous, and we want to bias towards doing the right thing for new users who will not be expecting to be going back to some ancient pip.

We have an escape hatch for those who encounter an unexpected and unlikely problem in an upgrade.

Copy link
Contributor

@jsirois jsirois left a comment

Choose a reason for hiding this comment

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

LGTM. I don't feel good about the world shift on existing users but it is probably mitigated by lockfile use allowing some degree of choice about when to deal with any "breaks" new resolve outputs may cause. We definitely have an ongoing tension where we fight our own deprecation rules with some regularity it seems to me.

@jsirois
Copy link
Contributor

jsirois commented Nov 16, 2022

There probably need not be verbiage about this, but the newer Pips plumbed here will cause Pex to issue a warning and automatically fall back to vendored Pip if the Python being used / specified with --interpreter-constraint falls before 3.7 (These newer Pips all require >=3.7). As such, you can imagine a user that gets different resolve results in different contexts and perhaps becomes confused by that. I'm not sure this is very likely or even much outside of what someone might expect, but FYI in case that was not a known property of the --pip-version support.

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.

Thanks! Thoughts on backporting to 2.15, but not changing the default to reduce the amount of API change? That gives an escape hatch for people with long resolves.

@benjyw
Copy link
Contributor Author

benjyw commented Nov 16, 2022

Hmm, I don't think this justifies backporting to 2.15, it's a feature not a bugfix.

I'm OK with being more conservative, on second thoughts that makes sense. We can deprecate use of the older pip, now or later, the main thing is to give people with bad resolve perf something to try.

@benjyw
Copy link
Contributor Author

benjyw commented Nov 16, 2022

And re the warning you mentioned @jsirois : I had originally thought to check the ICs on the Pants side and only set this flag if >=3.7, for that reason, but then figured that the user won't see a warning, and the fallback is in line with what they can reasonably expect. Or even better than what they can reasonably expect, since the actual expectation might be to error on the incompatibility.

@jsirois
Copy link
Contributor

jsirois commented Nov 16, 2022

Aha, great. Yeah, having them see the warning is probably what's wanted. That does make me question what happens, though, when Pants executes a Process that succeeds but spews stderr. Does that stderr automatically just go to the terminal? I think it does, since we used to get black emoji anyhow, but I'm not positive at all on this.

@benjyw
Copy link
Contributor Author

benjyw commented Nov 16, 2022

We return Black's stdout/stderr in a LintResult and then emit them manually in src/python/pants/core/goals/lint.py . Users won't see pex's warning unless we take pains to show them.

@jsirois
Copy link
Contributor

jsirois commented Nov 16, 2022

Aha. That's unfortunate.

@benjyw benjyw merged commit 2b42768 into pantsbuild:main Nov 16, 2022
@martimlobao
Copy link
Contributor

Hmm, I don't think this justifies backporting to 2.15, it's a feature not a bugfix.

I'm OK with being more conservative, on second thoughts that makes sense. We can deprecate use of the older pip, now or later, the main thing is to give people with bad resolve perf something to try.

fwiw, we're considering leapfrogging from 2.12 straight to 2.16 just so we can address some issues we've been seeing with performance 😄

@benjyw
Copy link
Contributor Author

benjyw commented Feb 13, 2023

fwiw, we're considering leapfrogging from 2.12 straight to 2.16 just so we can address some issues we've been seeing with performance 😄

I'd still recommend upgrading one version at a time, in a branch, so you get all the deprecation warnings. You can just merge the final 2.16 result into main of course.

@benjyw benjyw deleted the support_newer_pips branch May 24, 2023 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants