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

Refactoring needed to bring in resolvelib's Resolver abstraction #7317

Closed
pradyunsg opened this issue Nov 9, 2019 · 5 comments
Closed

Refactoring needed to bring in resolvelib's Resolver abstraction #7317

pradyunsg opened this issue Nov 9, 2019 · 5 comments
Labels
auto-locked Outdated issues that have been locked by automation C: dependency resolution About choosing which dependencies to install state: needs discussion This needs some more discussion type: refactor Refactoring code

Comments

@pradyunsg
Copy link
Member

I spent some time today, taking a look at where we are, w.r.t. bringing in resolvelib into pip, as part of #6536, for fixing #988.

I think there's only 2 main things remaining:

  1. A fairly "independent" RequirementPreparer.prepare to get an AbstractDistribution from InstallRequirement unconditionally (/cc @chrahunt)
  2. A sorted list of applicable_candidates, as part of BestCandidateResult (/cc @cjerdonek)

I think I have a good idea for how to approach both of these changes. I'll elaborate on them in dedicated comments below.

@pradyunsg pradyunsg added type: refactor Refactoring code C: dependency resolution About choosing which dependencies to install state: needs discussion This needs some more discussion labels Nov 9, 2019
@pradyunsg
Copy link
Member Author

pradyunsg commented Nov 9, 2019

AFAICT, the main way that dependency resolution is entangled with metadata generation today is (1). To decouple them, my plan is:

  • use_user_site into RequirementPreparer
  • move the logic for require_hashes into preparer
    • make it a public attribute that we'd access in Resolver._get_abstract_dist_for
    • Currently, Resolver computes and passes it into RequirementPreparer and InstallationCandidate.populate_link

This would eliminate the cyclic dependency between Resolver and RequirementPreparer (RequirementPreparer won't require arguments held as attributes in Resolver).

I don't think we'd be able to directly use this new prepare method in our existing resolver, but it'd be handy to have when prototyping, so I think we should add it anyway. :)


For self-reference later, this is what the prepare method looks like:

    def prepare(self, candidate):
        if candidate.editable:
            return self.prepare_editable_requirement(candidate)
        if candidate.link:
            return self.prepare_linked_requirement(candidate)
        if candidate.already_installed:
            return self.prepare_installed_requirement(candidate)
        assert False, "should never reach here"

s/candidate/requirement/

@pradyunsg
Copy link
Member Author

pradyunsg commented Nov 9, 2019

For (2), the fix can be pretty targeted:

applicable_candidates is not used anywhere outside of package_finder.py and I don't think any of the uses within package_finder.py expect it to be ordered or anything.


As an aside, we should change InstallationCandidate.project: Any -> InstallationCandidate.name: str.

@pradyunsg
Copy link
Member Author

pradyunsg commented Nov 9, 2019

move the logic for require_hashes into preparer

@chrahunt and I discussed this on voice call.

We figured out that the best place to do this would be in populate_requirement_set. There's already a comment there from a past @pradyunsg about how that method tweaks require_hashes and this change would make it explicit how all that works. :)

@pradyunsg
Copy link
Member Author

pradyunsg commented Nov 12, 2019

With #7328, #7331 and #7332, I think all this got done. For all I understand, we're properly unblocked to do this now. :)

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Dec 12, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 12, 2019
@brainwane
Copy link
Contributor

As a note for posterity: as Paul mentioned in a meeting several weeks ago, we chose resolvelib as the least bad choice with the least blockers to overcome. Resolvelib API is the most straightforward. And we can plug something in later if we change our mind.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation C: dependency resolution About choosing which dependencies to install state: needs discussion This needs some more discussion type: refactor Refactoring code
Projects
None yet
Development

No branches or pull requests

2 participants