Move fast-dep handling to RequirementPreparer#8685
Merged
pradyunsg merged 18 commits intopypa:masterfrom Aug 4, 2020
Merged
Conversation
Previously a call to `_fetch_metadata` could result in several possible outcomes: 1. `_dist` set, `_provided` not set, dist returned - for lazy wheels 2. `_dist` set, `_provided` not set, exception - for bad lazy wheels 3. `_dist` not set, `_provided` not set, exception - for non-lazy req exceptions 4. `_dist` set, `_provided` not set, exception - for bad non-lazy reqs 5. `_dist` set, `_provided` set, dist returned - for non-lazy reqs and probably more. Our intent is to use `_dist` being set as the indicator of "this requirement has been fully processed successfully" and discard `_prepared`, since we don't actually rely on any of the other states (they simply lead to a failure or in the future a retry).
Now that `_dist` is only set on success, we can use it to guard against repeated execution instead of `_prepared`. As a result there are now only two possible outcomes for calling `dist`: 1. `_dist` set and returned - lazy and non-lazy req 2. `_dist` not set and exception raised - bad lazy or bad non-lazy req
Since `_prepare` now internally validates that `_dist` isn't set, we don't need to.
No change in behavior, we just want to unify "requirements processing" and moving this function out is a prereq for moving `_fetch_metadata` in.
Since `_prepare` is called in two places, we preserve the `if self._dist is not None` protection above the new call to `_fetch_metadata`. The second `if` in `_prepare` handles the early return required when processing a lazy wheel.
Returning a `Distribution` makes `_fetch_metadata` look more like `_prepare_distribution`, in preparation for moving it there next.
Instead of an early return, we fall through to the existing check at the end of this function. This aligns our treatment of `_fetch_metadata` and `_prepare_distribution`.
Since wheels can't be editable, we can move this into LinkCandidate, closer to `RequirementPreparer.prepare_linked_requirement` into which we want to integrate `_fetch_metadata`.
This warning just needs to be traced in one place for all commands, there's no need for the resolver to know about it. Moving the warning out of the Resolver will make it easier to change how we provide the option.
Reduces dependence on Candidate (and Resolver (and Factory)).
Reduces dependence on Candidate.
Since when we generate the InstallRequirement we set the link, these must be the same. Reduces dependence on Candidate.
We happen to know that this is the same treatment that gave us `_name` and `_version` for Wheels in the first place (in `LinkEvaluator`). It's not ideal, however the metadata consistency check that occurs in `Candidate` after creation of a `Distribution` guards us against any deviation in the name and version during our processing. Reduces dependence on Candidate.
These are things we know will be true because of the existing wheel processing. In the future we may delegate the extraction of these to the LinkCandidate itself so it doesn't have to be an assertion.
The fact that all of this functionality can be put in terms of the `RequirementPreparer` indicates that, at least at this point, this is the cleanest place to put this functionality.
Reduces dependence on `InstallRequirement` being passed to `_fetch_metadata`.
Removes dependence on `InstallRequirement`.
This keeps all knowledge about preparation and types of requirements in `RequirementPreparer`, so there's one place to look when we're ready to start breaking it apart later.
6357c8e to
8b838eb
Compare
Member
Author
|
Tip: run tests once with |
McSinyx
reviewed
Aug 3, 2020
Contributor
McSinyx
left a comment
There was a problem hiding this comment.
Thank you for your help on the refactoring here! This is going to save me a lot of coding and discussion time and greatly help the fast-deps feature move forward!
| req_set.add_named_requirement(ireq) | ||
|
|
||
| for actual_req in req_set.all_requirements: | ||
| self.factory.preparer.prepare_linked_requirement_more(actual_req) |
pradyunsg
reviewed
Aug 4, 2020
| # showing the user what the hash should be. | ||
| return req.hashes(trust_internet=False) or MissingHashes() | ||
|
|
||
| def _fetch_metadata(preparer, link): |
Member
There was a problem hiding this comment.
This is probably something we can do in a follow-up, but we should add logging here, to denote which branch we're going through, and introduce early returns to make the flow cleaner.
if not preparer.use_lazy_wheel:
return None
if preparer.require_hashes:
logger.debug(...)
return None
if link.is_file or not link.is_wheel:
logger.debug(...)
return None
<all the logic, currently inside if block>
Member
There was a problem hiding this comment.
Let's do this in a follow up. @McSinyx wanna pick this up? :)
McSinyx
approved these changes
Aug 4, 2020
pradyunsg
approved these changes
Aug 4, 2020
This was referenced Aug 4, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
This PR moves our new fast-dep lazy wheel handling to
RequirementPreparer, addressing comments on #8532 that I neglected to follow up on in #8588.To hopefully support that this approach will leave us happy and carefree, in the last commit I also added a hook to download lazy wheels once we're certain we're going to install them (as an alternative to #8638).