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

    # Make the code deterministic if a directory contains multiple wheel files
    # 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

    # Make the code deterministic if a directory contains multiple wheel files
    # 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: [email protected]

- 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 <[email protected]>

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: [email protected] Co-authored-by: Pradyun Gedam <[email protected]> Co-authored-by: Adam Turner <[email protected]>

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 6b2ce22 Compare 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 wheels Sep 10, 2023

webknjaz force-pushed the maintenance/ensurepip-single-wheel branch from 71911de to bf909c0 Compare 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 resolved Hide resolved

Misc/NEWS.d/next/Library/2023-09-10-23-35-20.gh-issue-109245.yCVQbo.rst Outdated Show resolved Hide 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 wheels Sep 16, 2023

bedevere-app bot 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-app bot 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__.py Show resolved Hide resolved

webknjaz commented Sep 16, 2023

View reviewed changes

Lib/ensurepip/__init__.py Show resolved Hide resolved

webknjaz commented Sep 16, 2023

View reviewed changes

Lib/ensurepip/__init__.py Outdated Show resolved Hide 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__.py Show resolved Hide resolved

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

webknjaz and 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: [email protected]

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 <[email protected]>

Drag the wheel path object through os.fsdecode

bce6e3a

Use None-sentinel for missing alternative wheel

50efc2a

Co-authored-by: Pradyun Gedam <[email protected]>

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 2472d87 Compare January 25, 2024 13:33

pradyunsg requested changes Jan 29, 2024

View reviewed changes

Lib/ensurepip/__init__.py Show resolved Hide resolved

Lib/ensurepip/__init__.py Show resolved Hide resolved

pradyunsg approved these changes Jan 29, 2024

View reviewed changes

bedevere-app bot 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:main Jan 30, 2024
28 checks passed

bedevere-app bot removed the awaiting merge label Jan 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

webknjaz commented Sep 10, 2023

AA-Turner commented Sep 16, 2023

vstinner left a comment

pfmoore left a comment

pfmoore Sep 16, 2023

webknjaz Sep 16, 2023

AA-Turner Sep 17, 2023

webknjaz commented Sep 16, 2023

webknjaz commented Sep 16, 2023

pfmoore commented Sep 16, 2023

webknjaz commented Sep 16, 2023

AA-Turner commented Sep 17, 2023

AA-Turner Sep 17, 2023

webknjaz commented Oct 11, 2023

webknjaz commented Oct 15, 2023

webknjaz commented Jan 25, 2024

pradyunsg left a comment

webknjaz commented Jan 30, 2024

webknjaz commented Feb 1, 2024

vstinner commented Feb 1, 2024

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

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

Conversation

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

webknjaz commented Sep 10, 2023

AA-Turner commented Sep 16, 2023

vstinner left a comment

Choose a reason for hiding this comment

pfmoore left a comment

Choose a reason for hiding this comment

pfmoore Sep 16, 2023

Choose a reason for hiding this comment

webknjaz Sep 16, 2023

Choose a reason for hiding this comment

AA-Turner Sep 17, 2023

Choose a reason for hiding this comment

webknjaz commented Sep 16, 2023

webknjaz commented Sep 16, 2023

pfmoore commented Sep 16, 2023

webknjaz commented Sep 16, 2023

AA-Turner commented Sep 17, 2023

AA-Turner Sep 17, 2023

Choose a reason for hiding this comment

webknjaz commented Oct 11, 2023

webknjaz commented Oct 15, 2023

webknjaz commented Jan 25, 2024

pradyunsg left a comment

Choose a reason for hiding this comment

webknjaz commented Jan 30, 2024

webknjaz commented Feb 1, 2024

vstinner commented Feb 1, 2024

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