Use lazy wheel to obtain dep info for new resolver#8532
Use lazy wheel to obtain dep info for new resolver#8532McSinyx wants to merge 5 commits intopypa:masterfrom
Conversation
98455c2 to
0872339
Compare
ad5d3dc to
21de107
Compare
|
Need rebase fyi |
|
Thanks @ofek, you're even faster than @BrownTruck! Just curious, do you have some sort of hook to keep track of the PRs or you just happen to see it at the right time? |
21de107 to
55dafe2
Compare
|
I constantly cycle open tabs :) |
b46fb6b to
2192d0f
Compare
By invoking pip with --use-feature=lazy-wheel, the new resolver will try to obtain dependency information by lazily download wheels by HTTP range requests.
2192d0f to
c93db09
Compare
c93db09 to
4db613c
Compare
|
Per a private discussion with @pradyunsg a few days ago, we came to a consensus that lazy-wheel is not a particularly intuitive name to specify the feature this PR is introducing. Any suggestion would be really appreciated. |
|
I’d call it something along the line of |
fa7ca9c to
c53aeac
Compare
|
@uranusjr, I've renamed the user-facing feature name to |
| env: | ||
| - GROUP=1 | ||
| - NEW_RESOLVER=1 | ||
| - env: |
There was a problem hiding this comment.
Are any of these tests exercising the new code? It will only impact the network tests, and there aren't many in our unit tests. Personally I'd trade all of these for 1 or 2 integration tests where we know the new code is being used. werkzeug (which we build on for our mock server test helper) has some built-in helpers for handling range requests so we could do all of this locally, without reaching out to PyPI. The PR in pallets/werkzeug#977 might provide some help in using it.
There was a problem hiding this comment.
To add on to this, because of the code that we're touching, these are the specific integration tests I think would give us the most impact:
pip wheelpip downloadpip installof a wheel that has extras that are also wheels
One approach would be to find existing integration tests, then convert them to use our mock server and then parameterize which mock server to use: plain one or range-request-supporting one.
| force_reinstall=force_reinstall, | ||
| upgrade_strategy=upgrade_strategy, | ||
| py_version_info=py_version_info, | ||
| lazy_wheel='fast-deps' in options.features_enabled, |
There was a problem hiding this comment.
The RequirementPreparer currently hides all details about the way that we're accessing distributions, so the only thing that the Candidate needs to be aware of is the abstract distribution after preparing. If we put the lazy wheel logic there instead (probably used to create the abstract distribution), then:
- it preserves that separation of concerns, with all of the associated benefits
- we get more code reuse, since the lazy-wheel-backed abstract distribution would follow the same path as the eager-wheel-backed one, with all of the associated benefits
- it gives us control over whether the wheel download is actually lazy - IIUC currently we're relying on the fact that no one happens to access
_InstallRequirementBackedCandidate.distprior toiter_dependencies() - it gives us control over when the real wheel download happens - IIUC currently we're relying on the fact that someone happens to access
_InstallRequirementBackedCandidate.distbefore actually trying to install the wheel - it works when we compose candidates, like in
ExtrasCandidate, which currently directly accessesself.base.distand bypasses the lazy wheel logic
There was a problem hiding this comment.
Oooo! I missed ExtrasCandidate when I discussed this w/ @McSinyx.
I think we can cover this by changing how self.dist is populated (instead of changing how iter_dependencies works), since what's really happening in _iter_dependencies -- we're creating a separate distribution object and using it.
There was a problem hiding this comment.
@pradyunsg, I think we did discuss point (5) and what you came up with (1a28d08) will handle it just fine. So now we came to a consensus on how to cover the listed points above, which is different from what this PR is pushing, I'm thinking about filing another one (likely within today) to avoid intensive rebasing which I'm not exactly good at 😄
@chrahunt, thank you for the thoughtful heads up as well as the tips on testing.
|
I just thought I'd share pypi/warehouse#8254 here, as it's an idea that tackles this from a different angle, that I think would possibly be better in the long term (although I don't think it needs to stop this work in the short term). |
src/pip/_internal/cli/req_command.py
Outdated
| force_reinstall=force_reinstall, | ||
| upgrade_strategy=upgrade_strategy, | ||
| py_version_info=py_version_info, | ||
| lazy_wheel='lazy-wheel' in options.features_enabled, |
There was a problem hiding this comment.
This also needs a warning to be printed that this functionality is not stable and not meant for production use at this time.
| from pip._internal.network.session import PipSession | ||
|
|
||
|
|
||
| class HTTPRangeRequestUnsupported(RuntimeError): |
There was a problem hiding this comment.
| class HTTPRangeRequestUnsupported(RuntimeError): | |
| class HTTPRangeRequestUnsupported(Exception): |
|
Thank you @dstufft, the initiation for METADATA via the simple API looks really promising and is very likely to be out before the revised JSON API. I'll try to find a way, if possible and not ugly, to make the metadata acquisition part to be a bit future-proof so that we don't have to refactor again when we want to change the method to obtain them. |
|
I had a call with @McSinyx yesterday, where we discussed how we'd implement this functionality. Following @chrahunt's review (who stated what I was thinking during my initial review much more clearly!), we concluded that a better approach than what's being done right now, would be to do something like pradyunsg@1a28d08. It's a smaller, more easily reviewable, self-contained change overall. |
|
I'm closing this since it is superseded by GH-8588. |
This PR is created to continue the path to implement GH-7819 (this is half way there). I file this a bit early to iron out the UX that we'd want to have. At the time of writing, the patch produce something like the following, which IMHO a bit too verbose
Installation of django-rest-swagger
cc @cosmicexplorer for review and other thoughts on the lazy wheel
cc @nlhkabu and @ei8fdb for the UI/UX
TODOs:
--use-feature