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

Move fast-dep handling to RequirementPreparer #8685

Merged
merged 18 commits into from
Aug 4, 2020

Conversation

chrahunt
Copy link
Member

@chrahunt chrahunt commented Aug 2, 2020

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

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.
@chrahunt chrahunt added the skip news Does not need a NEWS file entry (eg: trivial changes) label Aug 2, 2020
@chrahunt chrahunt changed the title use preparer for lazy wheels Move fast-dep handling to RequirementPreparer Aug 2, 2020
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.
@chrahunt chrahunt force-pushed the use-preparer-for-lazy-wheels branch from 6357c8e to 8b838eb Compare August 2, 2020 23:42
@chrahunt
Copy link
Member Author

chrahunt commented Aug 2, 2020

Tip: run tests once with --new-resolver and once without, or wait half an hour for tests to fail in Travis

Copy link
Contributor

@McSinyx McSinyx left a comment

Choose a reason for hiding this comment

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

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!

@@ -169,6 +159,9 @@ def resolve(self, root_reqs, check_supported_wheels):

req_set.add_named_requirement(ireq)

for actual_req in req_set.all_requirements:
self.factory.preparer.prepare_linked_requirement_more(actual_req)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, this is fantastic!

@chrahunt chrahunt marked this pull request as ready for review August 3, 2020 13:22
@@ -448,12 +459,48 @@ def _get_linked_req_hashes(self, req):
# showing the user what the hash should be.
return req.hashes(trust_internet=False) or MissingHashes()

def _fetch_metadata(preparer, link):
Copy link
Member

@pradyunsg pradyunsg Aug 4, 2020

Choose a reason for hiding this comment

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

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>

Copy link
Member

Choose a reason for hiding this comment

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

Let's do this in a follow up. @McSinyx wanna pick this up? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I'm glad to.

@pradyunsg pradyunsg merged commit ee4371c into pypa:master Aug 4, 2020
@chrahunt chrahunt deleted the use-preparer-for-lazy-wheels branch August 4, 2020 13:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants