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

use .metadata distribution info when possible #11512

Closed
wants to merge 3 commits into from

Conversation

jjhelmus
Copy link

When performing install --dry-run and PEP 658 .metadata files are available to guide the resolve, do not download the associated wheels.

Rather use the distribution information directly from the .metadata files when reporting the results on the CLI and in the --report file.

Jonathan Helmus added 2 commits October 13, 2022 15:20
When performing `install --dry-run` and PEP 658 .metadata files are
available to guide the resolve, do not download the associated wheels.

Rather use the distribution information directly from the .metadata
files when reporting the results on the CLI and in the --report file.
@jjhelmus
Copy link
Author

pre-commit.ci autofix

@uranusjr
Copy link
Member

The legacy resolver does not support the dry_run argument so extra conditionals are needed.

@ddelange
Copy link
Contributor

ddelange commented May 22, 2023

Hi 👋

With the warehouse PR now merged: is there animo to move this PR forward? cc @pradyunsg @pfmoore

I would love to experience near-instant reports for big wheels like:

pip install --quiet --ignore-installed --dry-run --no-deps --report - torch

I can also offer my support if that helps!

@uranusjr
Copy link
Member

This is not the PR you are looking for. PEP 658 support is alreay merged in #11111.

@ddelange
Copy link
Contributor

I think without this PR, my command above will still download the torch wheel despite metadata being available. Or did I misunderstand?

$ pip install --no-cache-dir --ignore-installed --dry-run --no-deps --report - botocore
Collecting botocore
  Downloading botocore-1.29.138-py3-none-any.whl (10.8 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━╸━━━━━━━━━━━━ 7.5/10.8 MB 2.4 MB/s eta 0:00:02
ERROR: Operation cancelled by user

@ddelange
Copy link
Contributor

@uranusjr is there a cli flag I'm missing in my command above, such that there is no wheel downloaded?

that's what this PR achieves right: early return because the wheel is not needed for the report generation, due to PEP 658 now being live on both pip and warehouse?

@pfmoore
Copy link
Member

pfmoore commented Jun 15, 2023

There's linked bugs in pip and warehouse - warehouse is serving the data under the wrong name so pip can't use it, but pip has a bug in its processing of the data so warehouse can't fix their bug. And even if pip fixes our bug, warehouse can't fix theirs as older pips will break in a way that means they can't upgrade. #12078 fixes the pip bug, but it also changes the name of the relevant field. Warehouse has a corresponding fix, but the name change needs a PEP which is currently in progress.

@ddelange
Copy link
Contributor

Thanks a lot for your responses and efforts as always!

@ddelange
Copy link
Contributor

I think without this PR, my command above will still download the torch wheel despite metadata being available. Or did I misunderstand?

$ pip install --no-cache-dir --ignore-installed --dry-run --no-deps --report - botocore
Collecting botocore
  Downloading botocore-1.29.138-py3-none-any.whl (10.8 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━╸━━━━━━━━━━━━ 7.5/10.8 MB 2.4 MB/s eta 0:00:02
ERROR: Operation cancelled by user

cc @cosmicexplorer via #11478 (comment)

@cosmicexplorer
Copy link
Contributor

I did my best guess to resolve the merge conflict (the result is at https://github.com/pypa/pip/compare/main...cosmicexplorer:pip:metadata_only_resolve_no_whl?expand=1) and at first glance I can confirm that it avoids downloading that wheel, which is absolutely what I was hoping to achieve with #11111:

(ttt) cosmicexplorer@lightning-strike-teravolts:~/tools/pip$ pip install --no-cache-dir --ignore-installed --dry-run --no-deps --report out.json botocore
Collecting botocore
  Obtaining dependency information for botocore from https://files.pythonhosted.org/packages/29/22/d3313243e0e09ff421edb249011dbb8093089de76b84b5ec3438f4c868eb/botocore-1.31.12-py3-none-any.whl.metadata
  Downloading botocore-1.31.12-py3-none-any.whl.metadata (5.9 kB)
Downloading botocore-1.31.12-py3-none-any.whl (11.0 MB)
   ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 11.0/11.0 MB 15.3 MB/s eta 0:00:00
Would install botocore-1.31.12
(ttt) cosmicexplorer@lightning-strike-teravolts:~/tools/pip$ PYTHONPATH="$(pwd)/dist/pip-23.3.dev0-py3-none-any.whl" python -m pip install --no-cache-dir --ignore-installed --dry-run --no-deps --report out.json botocore
Collecting botocore
  Obtaining dependency information for botocore from https://files.pythonhosted.org/packages/29/22/d3313243e0e09ff421edb249011dbb8093089de76b84b5ec3438f4c868eb/botocore-1.31.12-py3-none-any.whl.metadata
  Downloading botocore-1.31.12-py3-none-any.whl.metadata (5.9 kB)
Would install botocore-1.31.12

Copy link
Contributor

@cosmicexplorer cosmicexplorer left a comment

Choose a reason for hiding this comment

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

@jjhelmus it is always extremely helpful when fixing a bug to figure out how to add a test case to demonstrate that your code solves the stated problem, or if that's infeasible, to just describe what makes it difficult to do so (I am not a pip maintainer and do not speak for them). I am going to try to make such a test case now because I hope to extend this idea to short-circuit other parts of the resolve with local caching (see #12184). If I can figure out a good one, I'll probably put up my own PR.

if dry_run:
self.factory.preparer.prepare_download_info(reqs)
else:
self.factory.preparer.prepare_linked_requirements_more(reqs)
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried this to resolve the merge conflict, but I need to look into the change that introduced this:

Suggested change
self.factory.preparer.prepare_linked_requirements_more(reqs)
self.factory.preparer.prepare_linked_requirements_more(reqs)
for req in reqs:
req.prepared = True
req.needs_more_preparation = False

@cosmicexplorer
Copy link
Contributor

Ok, it turned out there were a lot of changes needed to make this work, and I'm partly to blame for that confusion. Please see #12186, which should hopefully push this across the finish line. Thanks so so much for identifying and even correctly fixing this @jjhelmus!

@ichard26
Copy link
Member

ichard26 commented Jun 23, 2024

Closing in favour of #12186, although if I'm being realistic, neither PR is likely to land anytime soon due to limited maintainer time. Sorry.

@ichard26 ichard26 closed this Jun 23, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants