GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-80789: Get rid of the ``ensurepip`` infra for many wheels by webknjaz · Pull Request #109245 · python/cpython

Conversation

@webknjaz

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added the awaiting review label Sep 10, 2023
@webknjaz
webknjaz force-pushed the maintenance/ensurepip-single-wheel branch 2 times, most recently from b9fe227 to 6b2ce22 Compare September 10, 2023 23:33
@webknjaz 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
webknjaz force-pushed the maintenance/ensurepip-single-wheel branch from 71911de to bf909c0 Compare September 10, 2023 23:36
@webknjaz webknjaz mentioned this pull request Sep 10, 2023
gh-80789: Implement build-time pip bundling in ensurepip #12791
Closed
@webknjaz

webknjaz commented Sep 10, 2023

Copy link
Copy Markdown
Member Author

cc @AA-Turner @pfmoore @pradyunsg
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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
AA-Turner self-requested a review September 11, 2023 05:02
vstinner
vstinner reviewed Sep 15, 2023
View reviewed changes
Comment thread Lib/ensurepip/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/Library/2023-09-10-23-35-20.gh-issue-109245.yCVQbo.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@AA-Turner AA-Turner added the skip news label Sep 16, 2023
@AA-Turner 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 bedevere-app Bot mentioned this pull request Sep 16, 2023
Replace bundled pip and setuptools with a downloader in the ensurepip module #80789
Closed
@AA-Turner

AA-Turner commented Sep 16, 2023

Copy link
Copy Markdown
Member

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner approved these changes Sep 16, 2023
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting core review and removed awaiting review labels Sep 16, 2023
pfmoore
pfmoore approved these changes Sep 16, 2023
View reviewed changes

@pfmoore pfmoore left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/ensurepip/__init__.py Outdated
# 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)

@pfmoore pfmoore Sep 16, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@webknjaz webknjaz Sep 16, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@AA-Turner ^

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@AA-Turner AA-Turner Sep 17, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@webknjaz

webknjaz commented Sep 16, 2023

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
webknjaz
webknjaz commented Sep 16, 2023
View reviewed changes
Comment thread Lib/ensurepip/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
webknjaz
webknjaz commented Sep 16, 2023
View reviewed changes
Comment thread Lib/ensurepip/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
webknjaz
webknjaz commented Sep 16, 2023
View reviewed changes
Comment thread Lib/ensurepip/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@webknjaz

webknjaz commented Sep 16, 2023

Copy link
Copy Markdown
Member Author

FreeBSD failing check reason:

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pfmoore

pfmoore commented Sep 16, 2023

Copy link
Copy Markdown
Member

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.
AA-Turner and zware reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@webknjaz

webknjaz commented Sep 16, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AA-Turner

AA-Turner commented Sep 17, 2023

Copy link
Copy Markdown
Member

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
webknjaz reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AA-Turner
AA-Turner reviewed Sep 17, 2023
View reviewed changes
Comment thread Lib/ensurepip/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/ensurepip/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/ensurepip/__init__.py Outdated
# 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)

@AA-Turner AA-Turner Sep 17, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/ensurepip/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/ensurepip/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/ensurepip/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@webknjaz

webknjaz commented Oct 11, 2023

Copy link
Copy Markdown
Member Author

@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...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@webknjaz
webknjaz requested a review from AA-Turner October 11, 2023 02:43
@webknjaz

webknjaz commented Oct 15, 2023

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
webknjaz
webknjaz commented Oct 15, 2023
View reviewed changes
Comment thread Lib/ensurepip/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@webknjaz
webknjaz requested a review from pradyunsg October 15, 2023 15:38
webknjaz and others added 14 commits January 25, 2024 14:33
@webknjaz
Get rid of the ensurepip infra for many wheels
c32777f 
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
@webknjaz
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]
@AA-Turner @webknjaz
Remove changelog entry
5c3bd26
@AA-Turner @webknjaz
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
@AA-Turner @webknjaz
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
@webknjaz
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.
@webknjaz
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.
@webknjaz
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!
@webknjaz
Drop looping from the wheel verification script
0d94ec9 
This script is separate and is only used in CI as opposed to runtime.
@webknjaz @pradyunsg
Multiline complex wheel path conditional
5c97cda 
Co-authored-by: Pradyun Gedam <[email protected]>
@webknjaz
Drag the wheel path object through os.fsdecode
bce6e3a
@webknjaz @pradyunsg
Use None-sentinel for missing alternative wheel
50efc2a 
Co-authored-by: Pradyun Gedam <[email protected]>
@webknjaz
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.
@webknjaz
Melt the second wheel lookup error into sentinel
2472d87
@webknjaz
webknjaz force-pushed the maintenance/ensurepip-single-wheel branch from 45f9944 to 2472d87 Compare January 25, 2024 13:33
@webknjaz

webknjaz commented Jan 25, 2024

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pradyunsg
pradyunsg requested changes Jan 29, 2024
View reviewed changes

@pradyunsg pradyunsg left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

One last change, but LGTM otherwise!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/ensurepip/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/ensurepip/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
pradyunsg
pradyunsg approved these changes Jan 29, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting changes labels Jan 29, 2024
@pradyunsg
Merge branch 'main' into maintenance/ensurepip-single-wheel
da1ae79
@webknjaz

webknjaz commented Jan 30, 2024

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pradyunsg
pradyunsg merged commit 9639043 into python:main Jan 30, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Jan 30, 2024
@webknjaz

webknjaz commented Feb 1, 2024

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Feb 1, 2024

Copy link
Copy Markdown
Member

I don't think that such refactoring change should be backported to stable branches. Only bugfixes.
webknjaz reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@webknjaz @pradyunsg
@AA-Turner @aisk
pythonGH-80789: Get rid of the ensurepip infra for many wheels (p…
dec95a6 
…ython#109245)

Co-authored-by: [email protected]
Co-authored-by: Pradyun Gedam <[email protected]>
Co-authored-by: Adam Turner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pfmoore pfmoore pfmoore approved these changes
@vstinner vstinner vstinner approved these changes
@pradyunsg pradyunsg pradyunsg approved these changes
@AA-Turner AA-Turner Awaiting requested review from AA-Turner AA-Turner is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@webknjaz @AA-Turner @pfmoore @pradyunsg @vstinner @bedevere-bot
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.