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

Avoid network/index related imports for pip uninstall & list (unless necessary) #12637

Merged
merged 3 commits into from
May 6, 2024

Conversation

ichard26
Copy link
Member

@ichard26 ichard26 commented Apr 18, 2024

This PR builds upon the work started in #12566, speeding up pip list and pip uninstall. Towards #4768.

pip uninstall and list currently depend on req_install.py which always imports the expensive network and index machinery. However, it's only in rare situations that these commands actually hit the network:

  • pip list --outdated
  • pip list --uptodate
  • pip uninstall --requirement <url>

This PR refactors req_install.py so these commands can avoid the expensive imports unless truly necessary.

The news entry is trivial as the one already added in #12566 is sufficient.

@ichard26
Copy link
Member Author

ichard26 commented Apr 22, 2024

I'll note that pip list still eventually imports the network/index machinery as the pip version check is still enabled for this command, but I'll file a PR to change that soon ™️ (as per #11677)

Edit: I've filed that PR -> #12646

@ichard26
Copy link
Member Author

ichard26 commented Apr 23, 2024

Hey @pradyunsg, I would like to add this PR in addition to #12646 to the 24.1 milestone. Together with #12566 (and the pyparsing removal scheduled via the packaging bump), it would be a solid responsiveness improvement overall. Feel free to reject this request if you would prefer to preserve the churn budget (or your time 🙂) for something else this release cycle.

@ichard26 ichard26 added this to the 24.1 milestone Apr 24, 2024
@ichard26 ichard26 requested a review from uranusjr April 26, 2024 22:27
@ichard26 ichard26 added type: refactor Refactoring code type: performance Commands take too long to run labels Apr 29, 2024
@pradyunsg
Copy link
Member

@ichard26 This has an outstanding merge conflict, presumably since we've actually had a bunch of activity and merges in the last few days.

While it's reasonable to put this function in req_install.py, putting it
there unfortunately means importing a bunch of heavy network/index code
as well which isn't acceptable.
By moving SessionCommandMixin and IndexGroupCommand into their own
module, lazy imports can be easily used.
@ichard26 ichard26 force-pushed the refactor/imports branch 2 times, most recently from 89e707e to 4a68e4b Compare May 5, 2024 01:00
@ichard26
Copy link
Member Author

ichard26 commented May 5, 2024

Conflicts are fixed @pradyunsg 👍

@pradyunsg
Copy link
Member

Thanks for the quick updates!

@pradyunsg
Copy link
Member

I came around today to merge this but it looks this is failing CI now.

pip uninstall and list currently depend on req_install.py which always imports
the expensive network and index machinery. However, it's only in rare situations
that these commands actually hit the network:

- `pip list --outdated`
- `pip list --uptodate`
- `pip uninstall --requirement <url>`

This commit builds on the previous two refactoring commits and modifies these
commands to avoid the expensive imports unless truly necessary.
@ichard26
Copy link
Member Author

ichard26 commented May 5, 2024

Sorry about that, must've been a botched rebase. It's all green now @pradyunsg.

@pradyunsg pradyunsg merged commit 71a08a7 into pypa:main May 6, 2024
28 checks passed
@ichard26 ichard26 deleted the refactor/imports branch May 6, 2024 15:32
@ichard26
Copy link
Member Author

ichard26 commented May 6, 2024

Thanks for the review @pradyunsg. I realize that I may have been overextending my privilege to add items to milestones this release, let me know if I'm crossing any lines.

@pradyunsg
Copy link
Member

Nah, you're fine. I would've said more if there was more that needed to be said. :)

ichard26 added a commit to ichard26/pip that referenced this pull request May 6, 2024
This reverts commit 71a08a7, reversing
changes made to 22142d6.

This patch made the pip self-version check import
`pip._internal.self_outdated_check` on demand. This blows up when
downgrading (and possibly upgrading) pip as it imports the module
_after_ the downgrade, which is problematic as the older self-check logic
is incompatible with the rest of the pip already loaded into memory.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bot:chronographer:provided type: performance Commands take too long to run type: refactor Refactoring code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants