GH-80789: Get rid of the `ensurepip` infra for many wheels#109245

webknjaz · 2023-09-10T23:32:22Z

This is a refactoring change that aims to simplify ensurepip. Before it, this module had legacy infrastructure that made an assumption that ensurepip would be provisioning more then just a single wheel. That assumption is no longer true since [1][2][3].

In this change, the improvement is done around removing unnecessary loops and supporting structures to change the assumptions to expect only the bundled or replacement pip wheel.

This is a spin-off of #12791.

Issue: Replace bundled pip and setuptools with a downloader in the ensurepip module #80789

webknjaz · 2023-09-10T23:41:15Z

cc @AA-Turner @pfmoore @pradyunsg

Lib/ensurepip/__init__.py

Misc/NEWS.d/next/Library/2023-09-10-23-35-20.gh-issue-109245.yCVQbo.rst

AA-Turner · 2023-09-16T07:10:52Z

I've taken the liberty of pushing two changes to your branch; feel free to revert in whole or part.

Both look to simplify what ensurepip is doing internally -- e.g. with only pip, having a _Package type doesn't really make sense any-more -- similarly with caching the output, which is fast to compute.

I've also removed type annotations, as these are ignored and we shouldn't introduce more (tomllib and importlib.resources are special cases).

I think there are further simplifications we could make, but I thought this was a reasonable middle-ground -- let me know your thoughts!

A

vstinner

LGTM. But I would prefer to get a second review on this one.

pfmoore

One minor comment, but it doesn't block this - feel free to leave it if you think your version is better.

pfmoore · 2023-09-16T18:41:14Z

Lib/ensurepip/__init__.py

 # of the same package, but don't attempt to implement correct version
 # comparison since this case should not happen.
-filenames=sorted(filenames)
+filenames=sorted(filenames, reverse=True)


Why reverse? I guess it gives better odds of getting the correct version, but as the comment above says, we're not doing version comparison because it should never be needed.

@AA-Turner ^

I noted this in the commit message but not a PR comment; sorry. The implementation of this function now short-circuits and returns the first match, but that means we need to reverse the order as previously the final match would be returned, and we want to preserve that behaviour. We retain no guarantees about finding the correct version.

webknjaz · 2023-09-16T20:36:20Z

I've taken the liberty of pushing two changes to your branch; feel free to revert in whole or part.
Both look to simplify what ensurepip is doing internally -- e.g. with only pip, having a _Package type doesn't really make sense any-more -- similarly with caching the output, which is fast to compute.
I've also removed type annotations, as these are ignored and we shouldn't introduce more (tomllib and importlib.resources are special cases).

I think annotations contribute to maintainability.

I think there are further simplifications we could make, but I thought this was a reasonable middle-ground -- let me know your thoughts!

I was specifically avoiding any refactoring in this PR, trying to make it as small as possible and keep the potential conflicts with the rest of related PRs at minimum. I don't like some of the changes but if this gets it merged faster so be it.

Lib/ensurepip/__init__.py

webknjaz · 2023-09-16T20:53:29Z

FreeBSD failing check reason:

⚠️ Failed to start an instance: FAILED_PRECONDITION: Monthly free compute limit exceeded!

pfmoore · 2023-09-16T21:27:12Z

I think annotations contribute to maintainability.

I'm pretty sure there's still a policy that we don't use type annotations in the stdlib.

webknjaz · 2023-09-16T22:18:43Z

I'm pretty sure there's still a policy that we don't use type annotations in the stdlib.

Interesting.. https://devguide.python.org/search/?q=type+annotations&check_keywords=yes&area=default doesn't return anything related (nor does using Google search against that site).

AA-Turner · 2023-09-17T07:15:09Z

doesn't return anything related

See https://discuss.python.org/t/21487; #108125 (comment). Quoting Brett: "It's codified based on discussions among the core developers that we have all agreed to over the years"

A

Lib/ensurepip/__init__.py

AA-Turner · 2023-09-17T07:21:57Z

Lib/ensurepip/__init__.py

 # of the same package, but don't attempt to implement correct version
 # comparison since this case should not happen.
-filenames=sorted(filenames)
+filenames=sorted(filenames, reverse=True)


I noted this in the commit message but not a PR comment; sorry. The implementation of this function now short-circuits and returns the first match, but that means we need to reverse the order as previously the final match would be returned, and we want to preserve that behaviour. We retain no guarantees about finding the correct version.

Lib/ensurepip/__init__.py

webknjaz · 2023-10-11T02:42:02Z

@AA-Turner so I tried something which resulted in some simplification. I used pathlib but also made functions to return paths instead of complex structures with data that is never used together by the callers. I made sure that the coverage of the changed parts is at 100%, while making it less branchy...

webknjaz · 2023-10-15T15:30:47Z

I have made the requested changes; please review again

Lib/ensurepip/__init__.py

This is a refactoring change that aims to simplify ``ensurepip``. Before it, this module had legacy infrastructure that made an assumption that ``ensurepip`` would be provisioning more then just a single wheel. That assumption is no longer true since [[1]][[2]][[3]]. In this change, the improvement is done around removing unnecessary loops and supporting structures to change the assumptions to expect only the bundled or replacement ``pip`` wheel. [1]: python@ece20db [2]: python#101039 [3]: python#95299

This change is intended to make it clear that the helper now only returns one package and no more. Co-Authored-By: vstinner@python.org

- Exit early if there are no files in the directory - Return a match early by iterating in reverse order - Merge filename checks - Inline the path variable - Remove type annotations

- Return a dict with consistent fields - Remove caching - Remove type annotations - Leverage the known wheel package dir value to calculate full paths

It provides us with the ability to write simpler high-level logic that is easier to understand. As a side effect, it became possible to make the code less branchy.

It was returning bytes on FreeBSD which was incorrectly injected into the Python code snippet executed in a subprocess and had a b-preffix.

Some code comments and test function names were still referring to the removed function name. Not anymore!

This script is separate and is only used in CI as opposed to runtime.

Co-authored-by: Pradyun Gedam <pradyunsg@gmail.com>

It was tripping the linters and became unnecessary after hardcoding the pip wheel filename prefix in the string.

webknjaz · 2024-01-25T13:35:42Z

@pradyunsg there was a merge conflict that I solved with rebase. No other changes made.

pradyunsg

One last change, but LGTM otherwise!

Lib/ensurepip/__init__.py

webknjaz · 2024-01-30T01:19:25Z

@pradyunsg 🛎 the CI remains green after your branch sync FYI.

webknjaz · 2024-02-01T02:26:29Z

@AA-Turner @pradyunsg @vstinner apply the backport labels?

vstinner · 2024-02-01T12:36:25Z

I don't think that such refactoring change should be backported to stable branches. Only bugfixes.

…ython#109245) Co-authored-by: vstinner@python.org Co-authored-by: Pradyun Gedam <pradyunsg@gmail.com> Co-authored-by: Adam Turner <9087854+aa-turner@users.noreply.github.com>

bedevere-bot added the awaiting review label Sep 10, 2023

webknjaz force-pushed the maintenance/ensurepip-single-wheel branch 2 times, most recently from b9fe227 to 6b2ce22Compare September 10, 2023 23:33

webknjaz changed the title ~~Get rid of the ensurepip infra for many wheels~~GH-109245: Get rid of the ensurepip infra for many wheelsSep 10, 2023

webknjaz force-pushed the maintenance/ensurepip-single-wheel branch from 71911de to bf909c0Compare September 10, 2023 23:36

webknjaz mentioned this pull request Sep 10, 2023
gh-80789: Implement build-time pip bundling in ensurepip#12791
Closed

This was referenced Sep 10, 2023
GH-80789: Bundle ensurepip wheels at build time #109130
Closed
Ensure only one wheel for each vendored package in verify_ensurepip_wheels.py#109002
Closed

AA-Turner self-requested a review September 11, 2023 05:02

vstinner reviewed Sep 15, 2023
View reviewed changes

Lib/ensurepip/__init__.py Outdated Show resolvedHide resolved
Misc/NEWS.d/next/Library/2023-09-10-23-35-20.gh-issue-109245.yCVQbo.rst Outdated Show resolvedHide resolved

AA-Turner added the skip news label Sep 16, 2023

AA-Turner changed the title ~~GH-109245: Get rid of the ensurepip infra for many wheels~~GH-80789: Get rid of the ensurepip infra for many wheelsSep 16, 2023

bedevere-appbot mentioned this pull request Sep 16, 2023
Replace bundled pip and setuptools with a downloader in the ensurepip module #80789
Closed

vstinner approved these changes Sep 16, 2023
View reviewed changes

bedevere-appbot added awaiting core review and removed awaiting review labels Sep 16, 2023

pfmoore approved these changes Sep 16, 2023
View reviewed changes

webknjaz commented Sep 16, 2023
View reviewed changes

Lib/ensurepip/__init__.pyShow resolvedHide resolved

webknjaz commented Sep 16, 2023
View reviewed changes

Lib/ensurepip/__init__.pyShow resolvedHide resolved

webknjaz commented Sep 16, 2023
View reviewed changes

Lib/ensurepip/__init__.py Outdated Show resolvedHide resolved

AA-Turner reviewed Sep 17, 2023
View reviewed changes

webknjaz requested a review from AA-Turner October 11, 2023 02:43

webknjaz commented Oct 15, 2023
View reviewed changes

Lib/ensurepip/__init__.pyShow resolvedHide resolved

webknjaz requested a review from pradyunsg October 15, 2023 15:38

webknjazand others added 14 commits January 25, 2024 14:33

Rename _find_packages to _get_replacement_pip_package
d6e51b2
This change is intended to make it clear that the helper now only returns one package and no more. Co-Authored-By: vstinner@python.org

Remove changelog entry
5c3bd26

Simplify WHEEL_PKG_DIR search
96b8b9e
- Exit early if there are no files in the directory - Return a match early by iterating in reverse order - Merge filename checks - Inline the path variable - Remove type annotations

Simplify the pip wheel info
7b04736
- Return a dict with consistent fields - Remove caching - Remove type annotations - Leverage the known wheel package dir value to calculate full paths

Use pathlib in ensurepip internally
f3a49c2
It provides us with the ability to write simpler high-level logic that is easier to understand. As a side effect, it became possible to make the code less branchy.

Drop os.fsencode from wheel path preprocessing
3d7ee78
It was returning bytes on FreeBSD which was incorrectly injected into the Python code snippet executed in a subprocess and had a b-preffix.

Update replaced _get_pip_whl_path_ctx references
72b8ebe
Some code comments and test function names were still referring to the removed function name. Not anymore!

Drop looping from the wheel verification script
0d94ec9
This script is separate and is only used in CI as opposed to runtime.

Multiline complex wheel path conditional
5c97cda
Co-authored-by: Pradyun Gedam <pradyunsg@gmail.com>

Drag the wheel path object through os.fsdecode
bce6e3a

Use None-sentinel for missing alternative wheel
50efc2a
Co-authored-by: Pradyun Gedam <pradyunsg@gmail.com>

Un-f-string alternative wheel path glob
865c41f
It was tripping the linters and became unnecessary after hardcoding the pip wheel filename prefix in the string.

Melt the second wheel lookup error into sentinel
2472d87

webknjaz force-pushed the maintenance/ensurepip-single-wheel branch from 45f9944 to 2472d87Compare January 25, 2024 13:33

pradyunsg requested changes Jan 29, 2024
View reviewed changes

Lib/ensurepip/__init__.pyShow resolvedHide resolved
Lib/ensurepip/__init__.pyShow resolvedHide resolved

pradyunsg approved these changes Jan 29, 2024
View reviewed changes

bedevere-appbot added awaiting merge and removed awaiting changes labels Jan 29, 2024

Merge branch 'main' into maintenance/ensurepip-single-wheel
da1ae79

pradyunsg merged commit 9639043 into python:mainJan 30, 2024

bedevere-appbot removed the awaiting merge label Jan 30, 2024

Uh oh!

GH-80789: Get rid of the ensurepip infra for many wheels#109245

GH-80789: Get rid of the ensurepip infra for many wheels #109245

Uh oh!

Conversation

webknjaz commented Sep 10, 2023• edited by bedevere-app botLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webknjaz commented Sep 10, 2023

Uh oh!

Uh oh!

Uh oh!

AA-Turner commented Sep 16, 2023

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

pfmoore left a comment

Choose a reason for hiding this comment

Uh oh!

pfmooreSep 16, 2023

Choose a reason for hiding this comment

Uh oh!

webknjazSep 16, 2023

Choose a reason for hiding this comment

Uh oh!

AA-TurnerSep 17, 2023

Choose a reason for hiding this comment

Uh oh!

webknjaz commented Sep 16, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

webknjaz commented Sep 16, 2023

Uh oh!

pfmoore commented Sep 16, 2023

Uh oh!

webknjaz commented Sep 16, 2023

Uh oh!

AA-Turner commented Sep 17, 2023

Uh oh!

Uh oh!

Uh oh!

AA-TurnerSep 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

webknjaz commented Oct 11, 2023

Uh oh!

webknjaz commented Oct 15, 2023

Uh oh!

Uh oh!

webknjaz commented Jan 25, 2024

Uh oh!

pradyunsg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

webknjaz commented Jan 30, 2024

Uh oh!

webknjaz commented Feb 1, 2024

Uh oh!

vstinner commented Feb 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

GH-80789: Get rid of the `ensurepip` infra for many wheels#109245

GH-80789: Get rid of the `ensurepip` infra for many wheels #109245

webknjaz commented Sep 10, 2023•
edited by bedevere-app bot
Loading